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

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

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

joe darcy
Hello,

When the possibility of "_" being removed from the future set of valid
identifiers was determined, javac was updated to issue a warning to that
effect in JDK 8. Underscore was removed from the set of valid
identifiers as of JDK 9.

With local variable type inference, the name "var" should be given
similar treatment in source versions 9 and earlier. Please review the
webrev which implements this:

     http://cr.openjdk.java.net/~darcy/8189146.0/

As written, the patch warns for a type or type variable named "var"; it
would be possible to expand the patch to warn about other uses of "var"
that would conflict with the JDK 10 feature. As a aid to reviewing, the
non-raw diagnostic output from
test/langtools/tools/javac/lvti/ParserTest.java under --release 9 is
listed below.

IMO It would be preferable to issue this kind of warning in the context
of an -Xlint:future category (JDK-8189145), but until that feature is
developed, it is still better to issue the warning than to not issue it.

The webrev as presented is based off of JDK 11. Upon a successful code
review, I will explore getting the change into JDK 10 as an RFE and make
any needed adjustments to rebase the patch. If that is not fruitful,
I'll push the change to JDK 11.

Thanks,

-Joe


ParserTest.java:14: warning: as of release 10, 'var' is a restricted
local variable type and cannot be used for type declarations
class ParserTest<var> {
                  ^
ParserTest.java:16: warning: as of release 10, 'var' is a restricted
local variable type and cannot be used for type declarations
         static class var { } //illegal
                      ^
ParserTest.java:20: warning: as of release 10, 'var' is a restricted
local variable type and cannot be used for type declarations
         interface var { } //illegal
                   ^
ParserTest.java:24: warning: as of release 10, 'var' is a restricted
local variable type and cannot be used for type declarations
         enum var { } //illegal
              ^
ParserTest.java:28: warning: as of release 10, 'var' is a restricted
local variable type and cannot be used for type declarations
         @interface var { } //illegal
                    ^
ParserTest.java:36: warning: as of release 10, 'var' is a restricted
local variable type and cannot be used for type declarations
     static class var extends RuntimeException { } //illegal
                  ^
6 warnings

Reply | Threaded
Open this post in threaded view
|

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

Maurizio Cimadamore
Hi Joe,
I'm a bit unsure about having a retroactive warning in JDK 10 - e.g. I'm
not sure how useful that would be, but I get the spirit of what you're
getting at.

In terms of code, I think your warning should go inside the
isRestrictedLocalVarTypeName(Name), which is called in a few places (not
just the one you have touched). For instance, I don't think your patch
will cover this case:

List<var> l = null;

So, the code for isRestrictedLocalVarTypeName(Name) is currently:

return allowLocalVariableTypeInference && name == names.var;

I think it should be updated to:

if (name == names.var) {
             if (allowLocalVariableTypeInference) {
                 return true;
             } else {
                 warning(pos, "var.not.allowed", name);
             }
         }
         return false;


Of course you need to enhance the method to take an extra position
argument (pos).


Maurizio




On 09/01/18 18:28, joe darcy wrote:

> Hello,
>
> When the possibility of "_" being removed from the future set of valid
> identifiers was determined, javac was updated to issue a warning to
> that effect in JDK 8. Underscore was removed from the set of valid
> identifiers as of JDK 9.
>
> With local variable type inference, the name "var" should be given
> similar treatment in source versions 9 and earlier. Please review the
> webrev which implements this:
>
>     http://cr.openjdk.java.net/~darcy/8189146.0/
>
> As written, the patch warns for a type or type variable named "var";
> it would be possible to expand the patch to warn about other uses of
> "var" that would conflict with the JDK 10 feature. As a aid to
> reviewing, the non-raw diagnostic output from
> test/langtools/tools/javac/lvti/ParserTest.java under --release 9 is
> listed below.
>
> IMO It would be preferable to issue this kind of warning in the
> context of an -Xlint:future category (JDK-8189145), but until that
> feature is developed, it is still better to issue the warning than to
> not issue it.
>
> The webrev as presented is based off of JDK 11. Upon a successful code
> review, I will explore getting the change into JDK 10 as an RFE and
> make any needed adjustments to rebase the patch. If that is not
> fruitful, I'll push the change to JDK 11.
>
> Thanks,
>
> -Joe
>
>
> ParserTest.java:14: warning: as of release 10, 'var' is a restricted
> local variable type and cannot be used for type declarations
> class ParserTest<var> {
>                  ^
> ParserTest.java:16: warning: as of release 10, 'var' is a restricted
> local variable type and cannot be used for type declarations
>         static class var { } //illegal
>                      ^
> ParserTest.java:20: warning: as of release 10, 'var' is a restricted
> local variable type and cannot be used for type declarations
>         interface var { } //illegal
>                   ^
> ParserTest.java:24: warning: as of release 10, 'var' is a restricted
> local variable type and cannot be used for type declarations
>         enum var { } //illegal
>              ^
> ParserTest.java:28: warning: as of release 10, 'var' is a restricted
> local variable type and cannot be used for type declarations
>         @interface var { } //illegal
>                    ^
> ParserTest.java:36: warning: as of release 10, 'var' is a restricted
> local variable type and cannot be used for type declarations
>     static class var extends RuntimeException { } //illegal
>                  ^
> 6 warnings
>

Reply | Threaded
Open this post in threaded view
|

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

joe darcy
Hi Maurizio,


On 1/9/2018 10:49 AM, Maurizio Cimadamore wrote:
> Hi Joe,
> I'm a bit unsure about having a retroactive warning in JDK 10 - e.g.
> I'm not sure how useful that would be, but I get the spirit of what
> you're getting at.

I think this kind of warning allows a smoother cross-release transition.

>
> In terms of code, I think your warning should go inside the
> isRestrictedLocalVarTypeName(Name), which is called in a few places
> (not just the one you have touched). For instance, I don't think your
> patch will cover this case:
>
> List<var> l = null;
>
> So, the code for isRestrictedLocalVarTypeName(Name) is currently:
>
> return allowLocalVariableTypeInference && name == names.var;
>
> I think it should be updated to:
>
> if (name == names.var) {
>             if (allowLocalVariableTypeInference) {
>                 return true;
>             } else {
>                 warning(pos, "var.not.allowed", name);
>             }
>         }
>         return false;
>
>
> Of course you need to enhance the method to take an extra position
> argument (pos).
>
>

Webrev updated as suggested:

     http://cr.openjdk.java.net/~darcy/8189146.1/

That does emit more warnings than the earlier iteration in ParserTest,
17 rather than 6. As a review aid, the 17 expanded non-raw warnings are
listed below. Under release 10, there are 24 errors generated for the
file, including different error messages.

Thanks,

-Joe

/home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:14:
warning: as of release 10, 'var' is a restricted local variable type and
cannot be used for type declarations
class ParserTest<var> {
                  ^
/home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:16:
warning: as of release 10, 'var' is a restricted local variable type and
cannot be used for type declarations
         static class var { } //illegal
                      ^
/home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:20:
warning: as of release 10, 'var' is a restricted local variable type and
cannot be used for type declarations
         interface var { } //illegal
                   ^
/home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:24:
warning: as of release 10, 'var' is a restricted local variable type and
cannot be used for type declarations
         enum var { } //illegal
              ^
/home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:28:
warning: as of release 10, 'var' is a restricted local variable type and
cannot be used for type declarations
         @interface var { } //illegal
                    ^
/home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:36:
warning: as of release 10, 'var' is a restricted local variable type and
cannot be used for type declarations
     static class var extends RuntimeException { } //illegal
                  ^
/home/darcy/JDK/10/hg/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
     var x = null; //illegal
     ^
/home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:54:
warning: as of release 10, 'var' is a restricted local variable type and
cannot be used for type declarations
     var m() { //illegal
     ^
/home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:58:
warning: as of release 10, 'var' is a restricted local variable type and
cannot be used for type declarations
     void test2(var x) { //error
                ^
/home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:59:
warning: as of release 10, 'var' is a restricted local variable type and
cannot be used for type declarations
         List<var> l1; //error
              ^
/home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:60:
warning: as of release 10, 'var' is a restricted local variable type and
cannot be used for type declarations
         List<? extends var> l2; //error
                        ^
/home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:61:
warning: as of release 10, 'var' is a restricted local variable type and
cannot be used for type declarations
         List<? super var> l3; //error
                      ^
/home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:63:
warning: as of release 10, 'var' is a restricted local variable type and
cannot be used for type declarations
             Function<var, String> f = (var x2) -> ""; //error
                      ^
/home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:63:
warning: as of release 10, 'var' is a restricted local variable type and
cannot be used for type declarations
             Function<var, String> f = (var x2) -> ""; //error
                                        ^
/home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:64:
warning: as of release 10, 'var' is a restricted local variable type and
cannot be used for type declarations
         } catch (var ex) { } //error
                  ^
/home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:68:
warning: as of release 10, 'var' is a restricted local variable type and
cannot be used for type declarations
         boolean b1 = o instanceof var; //error
                                   ^
/home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:69:
warning: as of release 10, 'var' is a restricted local variable type and
cannot be used for type declarations
         Object o2 = (var)o; //error
                      ^
17 warnings

Reply | Threaded
Open this post in threaded view
|

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

Maurizio Cimadamore
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         }


This should be more or less equivalent, behavior-wise
(isRestrictedLocalVarTypeName returns 'false' if lvti is disabled, so
that makes it equivalent to what was there before), but it will execute
the test regardless of whether LVTI is disabled or not, which is what
you want.

There is another 'special' case where the parser reject 'var' and that
is with the compound case (e.g. 'var x, y = ...'), but I think that's a
corner^3 case and we can live w/o the warning there. But if you want to
get there, this is the code to tweak:

boolean implicit = Feature.LOCAL_VARIABLE_TYPE_INFERENCE.allowedInSource(source) && head.vartype == null;
2960         vdefs.append(head);
2961         while (token.kind == COMMA) {
2962             if (implicit) {
2963                 reportSyntaxError(pos, "var.not.allowed.compound");
2964             }
2965             // All but last of multiple declarators subsume a comma
2966             storeEnd((JCTree)vdefs.last(), token.endPos);
2967             nextToken();
2968             vdefs.append(variableDeclarator(mods, type, reqInit, dc, localDecl));
2969         }


Again, the name of the game is to make sure that we issue the warning
regardless of whether lvti is enable or not. This should do:

2960         vdefs.append(head);
2961         while (token.kind == COMMA) {
2962             if (isRestrictedLocalVarTypeName
(type)) {
2963                 reportSyntaxError(pos, "var.not.allowed.compound");
2964             }
2965             // All but last of multiple declarators subsume a comma
2966             storeEnd((JCTree)vdefs.last(), token.endPos);
2967             nextToken();
2968             vdefs.append(variableDeclarator(mods, type, reqInit, dc, localDecl));
2969         }

I'm less sure about this last one, so take it for a spin and see what
happens.

Maurizio

On 09/01/18 22:29, joe darcy wrote:

> Hi Maurizio,
>
>
> On 1/9/2018 10:49 AM, Maurizio Cimadamore wrote:
>> Hi Joe,
>> I'm a bit unsure about having a retroactive warning in JDK 10 - e.g.
>> I'm not sure how useful that would be, but I get the spirit of what
>> you're getting at.
>
> I think this kind of warning allows a smoother cross-release transition.
>
>>
>> In terms of code, I think your warning should go inside the
>> isRestrictedLocalVarTypeName(Name), which is called in a few places
>> (not just the one you have touched). For instance, I don't think your
>> patch will cover this case:
>>
>> List<var> l = null;
>>
>> So, the code for isRestrictedLocalVarTypeName(Name) is currently:
>>
>> return allowLocalVariableTypeInference && name == names.var;
>>
>> I think it should be updated to:
>>
>> if (name == names.var) {
>>             if (allowLocalVariableTypeInference) {
>>                 return true;
>>             } else {
>>                 warning(pos, "var.not.allowed", name);
>>             }
>>         }
>>         return false;
>>
>>
>> Of course you need to enhance the method to take an extra position
>> argument (pos).
>>
>>
>
> Webrev updated as suggested:
>
>     http://cr.openjdk.java.net/~darcy/8189146.1/
>
> That does emit more warnings than the earlier iteration in ParserTest,
> 17 rather than 6. As a review aid, the 17 expanded non-raw warnings
> are listed below. Under release 10, there are 24 errors generated for
> the file, including different error messages.
>
> Thanks,
>
> -Joe
>
> /home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:14:
> warning: as of release 10, 'var' is a restricted local variable type
> and cannot be used for type declarations
> class ParserTest<var> {
>                  ^
> /home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:16:
> warning: as of release 10, 'var' is a restricted local variable type
> and cannot be used for type declarations
>         static class var { } //illegal
>                      ^
> /home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:20:
> warning: as of release 10, 'var' is a restricted local variable type
> and cannot be used for type declarations
>         interface var { } //illegal
>                   ^
> /home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:24:
> warning: as of release 10, 'var' is a restricted local variable type
> and cannot be used for type declarations
>         enum var { } //illegal
>              ^
> /home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:28:
> warning: as of release 10, 'var' is a restricted local variable type
> and cannot be used for type declarations
>         @interface var { } //illegal
>                    ^
> /home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:36:
> warning: as of release 10, 'var' is a restricted local variable type
> and cannot be used for type declarations
>     static class var extends RuntimeException { } //illegal
>                  ^
> /home/darcy/JDK/10/hg/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
>     var x = null; //illegal
>     ^
> /home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:54:
> warning: as of release 10, 'var' is a restricted local variable type
> and cannot be used for type declarations
>     var m() { //illegal
>     ^
> /home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:58:
> warning: as of release 10, 'var' is a restricted local variable type
> and cannot be used for type declarations
>     void test2(var x) { //error
>                ^
> /home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:59:
> warning: as of release 10, 'var' is a restricted local variable type
> and cannot be used for type declarations
>         List<var> l1; //error
>              ^
> /home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:60:
> warning: as of release 10, 'var' is a restricted local variable type
> and cannot be used for type declarations
>         List<? extends var> l2; //error
>                        ^
> /home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:61:
> warning: as of release 10, 'var' is a restricted local variable type
> and cannot be used for type declarations
>         List<? super var> l3; //error
>                      ^
> /home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:63:
> warning: as of release 10, 'var' is a restricted local variable type
> and cannot be used for type declarations
>             Function<var, String> f = (var x2) -> ""; //error
>                      ^
> /home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:63:
> warning: as of release 10, 'var' is a restricted local variable type
> and cannot be used for type declarations
>             Function<var, String> f = (var x2) -> ""; //error
>                                        ^
> /home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:64:
> warning: as of release 10, 'var' is a restricted local variable type
> and cannot be used for type declarations
>         } catch (var ex) { } //error
>                  ^
> /home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:68:
> warning: as of release 10, 'var' is a restricted local variable type
> and cannot be used for type declarations
>         boolean b1 = o instanceof var; //error
>                                   ^
> /home/darcy/JDK/10/hg/test/langtools/tools/javac/lvti/ParserTest.java:69:
> warning: as of release 10, 'var' is a restricted local variable type
> and cannot be used for type declarations
>         Object o2 = (var)o; //error
>                      ^
> 17 warnings
>

Reply | Threaded
Open this post in threaded view
|

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

joe darcy
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
                        ^

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

Reply | Threaded
Open this post in threaded view
|

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

Maurizio Cimadamore


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
>

Reply | Threaded
Open this post in threaded view
|

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

Reply | Threaded
Open this post in threaded view
|

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

joe darcy
Hi Maurizio,


On 1/12/2018 5:13 AM, Maurizio Cimadamore wrote:

> 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 :-)).

Yes, the simple correspondence between the grammar productions and the
code gets fuzzy without an unambiguous lookahead token to force a unique
code path ;-)

>
> 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 was curious about that; an artifact of supporting "val" and "var"
makes sense.

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

Hmm. I agree the current patch is more intricate than desired for this
stage of JDK 10.

Arguably the largest compatibility impact from var is no longer being
able to use var as a type name, at least it is the  impact that might be
hardest to overcome in terms of having to rename a type rather than
rename a variable. So for a patch appropriate for 10 at this stage, I'll
prepare a simple patch for consideration that just warns on type name.
Then if that is acceptable for 10, your patch could go into JDK 11 under
a follow-up bug.

Thanks,

-Joe

Reply | Threaded
Open this post in threaded view
|

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

joe darcy
PS Link to webrev below.


On 1/12/2018 11:13 AM, joe darcy wrote:
> Hi Maurizio,
>
>
>

[snip]

>
>>
>> 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.
>
> Hmm. I agree the current patch is more intricate than desired for this
> stage of JDK 10.
>
> Arguably the largest compatibility impact from var is no longer being
> able to use var as a type name, at least it is the  impact that might
> be hardest to overcome in terms of having to rename a type rather than
> rename a variable. So for a patch appropriate for 10 at this stage,
> I'll prepare a simple patch for consideration that just warns on type
> name. Then if that is acceptable for 10, your patch could go into JDK
> 11 under a follow-up bug.
>

The iteration at

     http://cr.openjdk.java.net/~darcy/8189146.4/

goes back to the initial limited attempt at only warning for type names
(which also covers type variables) but preserves the more precise
testing developed in subsequent iterations.

Thanks,

-Joe
Reply | Threaded
Open this post in threaded view
|

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

Maurizio Cimadamore
Looks good - and I agree it's a less risky path.

Thanks
Maurizio


On 12/01/18 19:59, joe darcy wrote:

> PS Link to webrev below.
>
>
> On 1/12/2018 11:13 AM, joe darcy wrote:
>> Hi Maurizio,
>>
>>
>>
>
> [snip]
>
>>
>>>
>>> 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.
>>
>> Hmm. I agree the current patch is more intricate than desired for
>> this stage of JDK 10.
>>
>> Arguably the largest compatibility impact from var is no longer being
>> able to use var as a type name, at least it is the  impact that might
>> be hardest to overcome in terms of having to rename a type rather
>> than rename a variable. So for a patch appropriate for 10 at this
>> stage, I'll prepare a simple patch for consideration that just warns
>> on type name. Then if that is acceptable for 10, your patch could go
>> into JDK 11 under a follow-up bug.
>>
>
> The iteration at
>
>     http://cr.openjdk.java.net/~darcy/8189146.4/
>
> goes back to the initial limited attempt at only warning for type
> names (which also covers type variables) but preserves the more
> precise testing developed in subsequent iterations.
>
> Thanks,
>
> -Joe

Reply | Threaded
Open this post in threaded view
|

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

Jonathan Gibbons
In reply to this post by joe darcy


On 01/12/2018 11:59 AM, joe darcy wrote:

> PS Link to webrev below.
>
>
> On 1/12/2018 11:13 AM, joe darcy wrote:
>> Hi Maurizio,
>>
>>
>>
>
> [snip]
>
>>
>>>
>>> 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.
>>
>> Hmm. I agree the current patch is more intricate than desired for
>> this stage of JDK 10.
>>
>> Arguably the largest compatibility impact from var is no longer being
>> able to use var as a type name, at least it is the  impact that might
>> be hardest to overcome in terms of having to rename a type rather
>> than rename a variable. So for a patch appropriate for 10 at this
>> stage, I'll prepare a simple patch for consideration that just warns
>> on type name. Then if that is acceptable for 10, your patch could go
>> into JDK 11 under a follow-up bug.
>>
>
> The iteration at
>
>     http://cr.openjdk.java.net/~darcy/8189146.4/
>
> goes back to the initial limited attempt at only warning for type
> names (which also covers type variables) but preserves the more
> precise testing developed in subsequent iterations.
>
> Thanks,
>
> -Joe

OK.

It is a general rule for javac that javac should be able to compile a
program without generating any warnings. Any warnings should be
suppressible either by using @SuppressWarnings, or a command-line
option, or (in a few cases) changing the source code to something
equivalent, without changing the intended functionality.

I can see how it would be difficult to use the Lint mechanism in the
parser, since even the deferred-lint-warnings mechanism doesn't kick in
until later. So that means in this case, the remediation to avoid this
proposed new warning is to change the source code in question. And that
is acceptable, in part because of the expected low number of occurrences
of this warning, and because the code will have to be changed eventually
anyway, in later releases where the warning is promoted to an error.

-- Jon