RFR JDK-8188225: AST could be improved in presence of var types.

[Jan Lahoda]

Hi,

An updated patch that has a method to generate the synthetic type trees 
is here:

http://cr.openjdk.java.net/~jlahoda/8188225/webrev.01/

Thanks for any feedback,
     Jan

On 3.10.2017 19:58, Maurizio Cimadamore wrote:
>
>
> On 03/10/17 18:53, Jan Lahoda wrote:
>> Hi Maurizio,
>>
>> Thanks for the comments - some responses inline.
>>
>> On 3.10.2017 17:59, Maurizio Cimadamore wrote:
>>> Thanks for taking care of this, I have a couple of suggestion:
>>>
>>> * the routine for generating synthetic type tree should be shared. E.g.
>>> you need a method that takes a type and generates something that is
>>> suitable for a local variable or a lambda parameter - e.g.
>>>
>>> toSyntheticTypeTree(Type t) {
>>>     if (t.isErroneous()) {
>>>        return make.at(Position.NOPOS).Erroneous();
>>>     } else {
>>>        return make.at(Position.NOPOS).Type(t);
>>>     }
>>> }
>>
>> Sure, will do.
>>
>>>
>>> * secondly, while adding the startpos tree is fine - here it seems like
>>> we morally would like to override the getStartPosition for the synthetic
>>> type trees created above, so that the former position is returned (e.g.
>>> that of 'var'). So, I wonder if something like this could work:
>>>
>>> toSyntheticTypeTree(Type t, int preferredPos) {
>>>     if (t.isErroneous()) {
>>>        return make.at(preferredPos).Erroneous();
>>>     } else {
>>>        return make.at(preferredPos).Type(t);
>>>     }
>>> }
>>>
>>> Then, assuming the parser creates the local variable node with the
>>> correct pos (which it doesn't now), I think everything should work even
>>
>> Hm, you mean put the starting position to the JCVariableDecl.pos? If
>> that would be the value, we could surely avoid the field, but AFAIK
>> the JCVariableDecl.pos usually points to the name of the variable, and
>> I didn't want to add an inconsistency, or change that (as the position
>> is useful for error reporting),
>>
>> Alternatives I was thinking of included playing tricks with modifiers
>> (setting the position to modifiers (if there are no/empty modifiers)
>> and filtering it in getStartPos), with nameexpr or having a subclass
>> of JCVariableDecl for vars which would include the extra field.
> I see good points. I also thought about using a tree copier, but that
> also goes through TreeMaker, which makes it hard to e.g. create anon
> tree instances which override getStartPosition.
>
> No bother - your original solution is good enough, at least for now. I
> appreciate that the new pos field is just for var AST nodes.
>
> Maurizio
>>
>> (I was also trying to have -1 as the position of the synthetic type to
>> aid its detection, but it could still be detected by looking at the
>> end position, so this is not that big deal.)
>>
>> Thanks,
>>     Jan
>>
>>> w/o a dedicated field?
>>>
>>> Maurizio
>>>
>>>
>>>
>>>
>>> On 02/10/17 16:35, Jan Lahoda wrote:
>>>> Hello,
>>>>
>>>> JShell produces not ideal errors related to local variable type
>>>> inference in some cases:
>>>> ---
>>>> jshell> var i = () -> {};
>>>> |  Error:
>>>> |  incompatible types: java.lang.Object is not a functional interface
>>>> |  var i = () -> {};
>>>> |          ^------^
>>>> ---
>>>>
>>>> Better would be:
>>>> ---
>>>> jshell> var i = () -> {};
>>>> |  Error:
>>>> |  cannot infer type for local variable i
>>>> |    (lambda expression needs an explicit target-type)
>>>> |  var i = () -> {};
>>>> |  ^---------------^
>>>> ---
>>>>
>>>> (which is the error produced by javac)
>>>>
>>>> The AST model could also be improved for local variables whose type
>>>> have been inferred (which also improves the jshell errors in some
>>>> cases):
>>>> - for "var i = 0;", the VariableTree.getType() will be null even after
>>>> attribution, but for: "for (var i : Arrays.asList(0, 1)) {}", the
>>>> VariableTree.getType() will be filled in by Attr. (The inferred type
>>>> is also filled in for lambda parameters.) This is not only
>>>> inconsistent, but also Attr.PostAttrAnalyzer.visitVarDef may fill in
>>>> the type with an ErroneousTree (with a wrong position). The proposal
>>>> here is to always fill in the type for consistency, and to
>>>> consistently use NOPOS for the synthetic type position
>>>>
>>>> -for "var i = 0;" SourcePositions.getStartPosition does not return a
>>>> proper starting position (the position of "var"), but rather the
>>>> position of the synthetic type, if any (which can also be
>>>> ErroneousTree) or the position of the variable name. The proposal here
>>>> is to remember the real start of the variable and return it.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8188225
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~jlahoda/8188225/webrev.00/index.html
>>>>
>>>> What do you think?
>>>>
>>>> Thanks,
>>>>     Jan
>>>
>