RFR: 8263614: javac allows local variables to be accessed from a static context [v2]

[Maurizio Cimadamore]

On Thu, 13 May 2021 20:37:37 GMT, Vicente Romero <vromero at openjdk.org> wrote:

>> javac breaks with NPE if compiles this code:
>> 
>> 
>> public class LocalClasses {
>>     public static void main(String[] args) {
>>         int i = 5;
>>         class Local {
>>             static void m() {
>>                 System.out.println("Value of i = " + i);
>>             }
>>         }
>>         Local.m();
>>     }
>> } 
>> 
>> 
>> actually the compiler should issue a compiling error as local variable `i` is being accessed from a static context. Please review this fix to address this issue. Some notes:
>> 
>> `com.sun.tools.javac.comp.AttrContext.staticLevel` keeps a the number of nested static contexts but this calculation doesn't consider static type declarations. This is because static declaration doesn't introduce a static context. But if they have a static modifier, even if implicit as it is for local records, then this affects what variables, type variables, etc are accessible from the body of the static type declaration. For this reason I have used an `adjusted` static level that takes static type declarations into account.
>> 
>> TIA
>
> Vicente Romero has updated the pull request incrementally with one additional commit since the last revision:
> 
>   update after review comments

This is what I came up with:


diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java
index 86f68f94efd..a325a08459f 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java
@@ -1488,30 +1488,24 @@ public class Resolve {
         boolean staticOnly = false;
         while (env1.outer != null) {
             Symbol sym = null;
-            if (isStatic(env1)) staticOnly = true;
             for (Symbol s : env1.info.scope.getSymbolsByName(name)) {
                 if (s.kind == VAR && (s.flags_field & SYNTHETIC) == 0) {
                     sym = s;
+                    if (staticOnly) {
+                        return new StaticError(sym);
+                    }
                     break;
                 }
             }
+            if (isStatic(env1)) staticOnly = true;
             if (sym == null) {
                 sym = findField(env1, env1.enclClass.sym.type, name, env1.enclClass.sym);
             }
             if (sym.exists()) {
                 if (staticOnly &&
-                   (sym.flags() & STATIC) == 0 &&
-                    sym.kind == VAR &&
-                        // if it is a field
-                        (sym.owner.kind == TYP ||
-                        // or it is a local variable but it is not declared inside of the static local type
-                        // then error
-                        allowRecords &&
-                        (sym.owner.kind == MTH) &&
-                        env1 != env &&
-                        !isInnerClassOfMethod(sym.owner, env.tree.hasTag(CLASSDEF) ?
-                                ((JCClassDecl)env.tree).sym :
-                                env.enclClass.sym)))
+                        sym.kind == VAR &&
+                        sym.owner.kind == TYP &&
+                        (sym.flags() & STATIC) == 0)
                     return new StaticError(sym);
                 else
                     return sym;
@@ -2313,35 +2307,22 @@ public class Resolve {
         return bestSoFar;
     }
 
-    Symbol findTypeVar(Env<AttrContext> currentEnv, Env<AttrContext> originalEnv, Name name, boolean staticOnly) {
-        for (Symbol sym : currentEnv.info.scope.getSymbolsByName(name)) {
+    Symbol findTypeVar(Env<AttrContext> env, Name name, boolean staticOnly) {
+        for (Symbol sym : env.info.scope.getSymbolsByName(name)) {
             if (sym.kind == TYP) {
-                if (staticOnly &&
-                    sym.type.hasTag(TYPEVAR) &&
-                    ((sym.owner.kind == TYP) ||
-                    // are we trying to access a TypeVar defined in a method from a local static type: interface, enum or record?
-                    allowRecords &&
-                    (sym.owner.kind == MTH &&
-                    currentEnv != originalEnv &&
-                    !isInnerClassOfMethod(sym.owner, originalEnv.tree.hasTag(CLASSDEF) ?
-                            ((JCClassDecl)originalEnv.tree).sym :
-                            originalEnv.enclClass.sym)))) {
+                if (sym.type.hasTag(TYPEVAR) &&
+                        (staticOnly || (isStatic(env) && sym.owner.kind == TYP)))
+                    // if staticOnly is set, it means that we have recursed through a static declaration,
+                    // so type variable symbols should not be accessible. If staticOnly is unset, but
+                    // we are in a static declaration (field or method), we should not allow type-variables
+                    // defined in the enclosing class to "leak" into this context.
                     return new StaticError(sym);
-                }
                 return sym;
             }
         }
         return typeNotFound;
     }
 
-    boolean isInnerClassOfMethod(Symbol msym, Symbol csym) {
-        while (csym.owner != msym) {
-            if (csym.isStatic()) return false;
-            csym = csym.owner.enclClass();
-        }
-        return (csym.owner == msym && !csym.isStatic());
-    }
-
     /** Find an unqualified type symbol.
      *  @param env       The current environment.
      *  @param name      The type's name.
@@ -2353,9 +2334,9 @@ public class Resolve {
         Symbol sym;
         boolean staticOnly = false;
         for (Env<AttrContext> env1 = env; env1.outer != null; env1 = env1.outer) {
-            if (isStatic(env1)) staticOnly = true;
             // First, look for a type variable and the first member type
-            final Symbol tyvar = findTypeVar(env1, env, name, staticOnly);
+            final Symbol tyvar = findTypeVar(env1, name, staticOnly);
+            if (isStatic(env1)) staticOnly = true;
             sym = findImmediateMemberType(env1, env1.enclClass.sym.type,
                                           name, env1.enclClass.sym);
 ```
 
 It reverts some of the code changes made for records, and reverts the code to a simpler state (the isInnerClassOfMethod) is no longer needed. This passes all tests, including the modified RecordCompilationTests.

-------------

PR: https://git.openjdk.java.net/jdk/pull/4004