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

[Maurizio Cimadamore]

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
>>