JDK 10/11 RFR of JDK-8189146: Have use of "var" in 9 and earlier source versions issue a warning

[Maurizio Cimadamore]

Hey Joe,
here's my take on the patch:

http://cr.openjdk.java.net/~mcimadamore/8189146/

it is similar to what I initially proposed, but I had to fight a bit 
with the parser in order to get rid of the duplicate warnings. The issue 
is that warnings can be reported both through the 'parseType' path and 
the 'variableDeclaratorRest' path, as the overlapping between the two is 
non trivial (sometimes javac parses a local var decl type as a type, 
sometimes as an expr, depending on whether javac is 'sure' that the 
thing is a variable decl - whose production is ambiguous with almost 
anything else :-)).

The solution is to add a flag to the isRestrictedLocalVarName so that in 
certain cases the warning generation is suppressed. I have also 
consolidated the treatment of the errors occurring because of the use of 
'var' in a compound declaration - now the errors are reported in the 
same place where errors about 'var and array' is reported, which makes a 
bunch of things easier.

Lastly, I've cleaned up some of the diagnostic used - as they were using 
a diagnostic argument which was alway set to the name 'var'. I believe 
that's a leftover of when we accepted both 'var' and 'val', and I have 
now removed it.

I believe what comes out of the compiler is in sync with what you 
wanted, but the patch is not as trivial as we originally had hoped, 
which, in my mind, makes it more for 11 than for 10.

Cheers
Maurizio



On 11/01/18 23:56, Maurizio Cimadamore wrote:
>
>
> On 11/01/18 22:52, joe darcy wrote:
>> Hi Maurizio,
>>
>> On 1/9/2018 4:39 PM, Maurizio Cimadamore wrote:
>>> Thanks for the update. This is close, but compared to the existing 
>>> golden file, as you noticed, there are missing warnings, esp. when 
>>> 'var' is used as an array element type.
>>>
>>> I believe the issue is with this code:
>>>
>>> if (Feature.LOCAL_VARIABLE_TYPE_INFERENCE.allowedInSource(source) && 
>>> elemType.hasTag(IDENT)) {
>>> 2998             Name typeName = ((JCIdent)elemType).name;
>>> 2999             if (isRestrictedLocalVarTypeName(typeName, pos)) {
>>> 3000                 if (type.hasTag(TYPEARRAY)) {
>>> 3001                     //error - 'var' and arrays
>>> 3002                     reportSyntaxError(pos, 
>>> "var.not.allowed.array");
>>> 3003                 } else {
>>> 3004                     startPos = TreeInfo.getStartPos(mods);
>>> 3005                     if (startPos == Position.NOPOS)
>>> 3006                         startPos = TreeInfo.getStartPos(type);
>>> 3007                     //implicit type
>>> 3008                     type = null;
>>> 3009                 }
>>> 3010             }
>>> 3011         }
>>>
>>>
>>> Here, if LVTI is disabled, the 'isRestrictedLocalVarTypeName' call 
>>> would not even take place, hence the missing warnings. I suggest to 
>>> shuffle as follows:
>>>
>>> if (elemType.hasTag(IDENT)) {
>>> 2998             Name typeName = ((JCIdent)elemType).name;
>>> 2999             if (isRestrictedLocalVarTypeName(typeName, pos)) {
>>> 3000                 if (type.hasTag(TYPEARRAY)) {
>>> 3001                     //error - 'var' and arrays
>>> 3002                     reportSyntaxError(pos, 
>>> "var.not.allowed.array");
>>> 3003                 } else {
>>> 3004                     startPos = TreeInfo.getStartPos(mods);
>>> 3005                     if (startPos == Position.NOPOS)
>>> 3006                         startPos = TreeInfo.getStartPos(type);
>>> 3007                     //implicit type
>>> 3008                     type = null;
>>> 3009                 }
>>> 3010             }
>>> 3011         }
>>
>> When I tried that out, there were more warnings; too many actually, 
>> there were double warnings issued in some cases:
>>
>> /test/langtools/tools/javac/lvti/ParserTest.java:38: warning: as of 
>> release 10, 'var' is a restricted local variable type and cannot be 
>> used for type declarations or as the element type of an array
>>     var x = null; //illegal
>>     ^
>> /test/langtools/tools/javac/lvti/ParserTest.java:38: warning: as of 
>> release 10, 'var' is a restricted local variable type and cannot be 
>> used for type declarations or as the element type of an array
>>     var x = null; //illegal
>>         ^
>> and
>>
>> /home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:49: 
>> warning: as of release 10, 'var' is a restricted local variable type 
>> and cannot be used for type declarations or as the element type of an 
>> array
>>         var x9 = null, y = null; //illegal
>>             ^
>> /home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:49: 
>> warning: as of release 10, 'var' is a restricted local variable type 
>> and cannot be used for type declarations or as the element type of an 
>> array
>>         var x9 = null, y = null; //illegal
>>                        ^
> I will think about this a bit more - this (or something very similar) 
> should work.
>
> Maurizio
>>
>> Stepping back a bit, if "var" were added as a full new keyword then 
>> the check for a use of "var" could occur within the ident() method, 
>> as is done for underscore. However, since "var" is allowed its 
>> traditional use in some contexts, the error/warning checks need to be 
>> dependent on the syntactic context. For pre-10, only a syntactic 
>> check needs occur, e.g. a construct like "var x = 5, y = 6" does not 
>> merit a warning about y having a var type; although that check is 
>> needed in 10 and later. Therefore I moved a check out to the method 
>> for the containing grammar production:
>>
>>     http://cr.openjdk.java.net/~darcy/8189146.3/
>>
>> I also changed the wording of the warning message slightly to be more 
>> inclusive of the situations it was covering. (More precisely worded 
>> warning could be given for different situations, but I don't think 
>> that is strictly necessary.)
>>
>> One "var" warning/error check occurs now in typeName, which catches 
>> class/interface/enum/annotation/type var declaration usage, and needs 
>> to occur in some way for at least certain idents occurring in 
>> variable declarations, including in try-with-resources.
>>
>> I made some updates to the ParserTest to better cover 
>> try-with-resources and took a somewhat different tack to attempt to 
>> get the right set of warnings issues. Unfortunately, there were 
>> duplicates here as well:
>>
>> /test/langtools/tools/javac/lvti/ParserTest.java:38: warning: as of 
>> release 10, 'var' is a restricted local variable type and cannot be 
>> used for type declarations or as the element type of an array
>>     var x = null; //illegal
>>     ^
>> /test/langtools/tools/javac/lvti/ParserTest.java:38: warning: as of 
>> release 10, 'var' is a restricted local variable type and cannot be 
>> used for type declarations or as the element type of an array
>>     var x = null; //illegal
>>     ^
>>
>> /test/langtools/tools/javac/lvti/ParserTest.java:50: warning: as of 
>> release 10, 'var' is a restricted local variable type and cannot be 
>> used for type declarations or as the element type of an array
>>         final @DA var x10 = m(); //ok
>>                   ^
>> /test/langtools/tools/javac/lvti/ParserTest.java:50: warning: as of 
>> release 10, 'var' is a restricted local variable type and cannot be 
>> used for type declarations or as the element type of an array
>>         final @DA var x10 = m(); //ok
>>                   ^
>> /test/langtools/tools/javac/lvti/ParserTest.java:51: warning: as of 
>> release 10, 'var' is a restricted local variable type and cannot be 
>> used for type declarations or as the element type of an array
>>         @DA final var x11 = m(); //ok
>>                   ^
>> /test/langtools/tools/javac/lvti/ParserTest.java:51: warning: as of 
>> release 10, 'var' is a restricted local variable type and cannot be 
>> used for type declarations or as the element type of an array
>>         @DA final var x11 = m(); //ok
>>
>> Hopefully any additional modifications needed will be minor!
>>
>> Thanks,
>>
>> -Joe
>>
>