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

[Jan Lahoda]

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