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

[Maurizio Cimadamore]

Looks good.

Maurizio


On 05/10/17 09:15, Jan Lahoda wrote:
> 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
>>>>
>>