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

[Maurizio Cimadamore]

On Wed, 12 May 2021 20:37:04 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

This is a tricky area, so I played a little with the (unchanged) code, to get an idea of what was wrong. It seems to me that the behavior of `Resolve::findVar` (other methods are probably the same) is as follows:

0. set `staticOnly` to `true` is the current env is static
1. look in local variable of current env
2. look in fields of current env
3. if a field is found and the field is compatible with `staticOnly` return that, otherwise issue a static error
4. go back to (1) with the enclosing env, setting `staticOnly` if a static member is found

This doesn't seem *too* bad - but there is an issue, e.g. in the example mentioned in the JBS issue: we look into locals (step 1) regardless of the value of `isStatic`. This leads to issue, because it doesn't make sense to "find" a local variable in an enclosing scope if the type from which the search started is a static type.

To rectify this issue, I came up with the following, relatively minimal fix:


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 3d4859d1b88..cdc129e8ad6 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
@@ -1510,13 +1510,16 @@ 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);
             }


The idea here is that, whatever symbol is found during step (1), if `staticOnly` is set, we should just disregard it and throw a static error instead (as done for the field lookup in (3). This seems to fix the issue in JBS, and seems to pass all javac test.

It is likely that, as in your patch, similar changes would be needed for other routines such as `Resolve::findType`/`findTypeVar`.

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

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