Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

vicente-romero-oracle
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle commented May 12, 2021

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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8263614: javac allows local variables to be accessed from a static context

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4004/head:pull/4004
$ git checkout pull/4004

Update a local copy of the PR:
$ git checkout pull/4004
$ git pull https://git.openjdk.java.net/jdk pull/4004/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4004

View PR using the GUI difftool:
$ git pr show -t 4004

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4004.diff

…d in a local class generates an NPE while compiling
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 12, 2021

👋 Welcome back vromero! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label May 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 12, 2021

@vicente-romero-oracle The following label will be automatically applied to this pull request:

  • compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the compiler label May 12, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 12, 2021

Webrevs

@mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented May 13, 2021

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:

  1. set staticOnly to true is the current env is static
  2. look in local variable of current env
  3. look in fields of current env
  4. if a field is found and the field is compatible with staticOnly return that, otherwise issue a static error
  5. 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). Also, the initialization of staticOnly in (0) is moved after (1) (because we have to search parameter of the current method, at least once). 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.

@vicente-romero-oracle
Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle commented May 13, 2021

@mcimadamore, I have uploaded another commit which builds on your patch and adds a small change to Resolve::findTypeVar, thanks

!isInnerClassOfMethod(sym.owner, originalEnv.tree.hasTag(CLASSDEF) ?
((JCClassDecl)originalEnv.tree).sym :
originalEnv.enclClass.sym)))) {
(!isInnerClassOfMethod(sym.owner, originalEnv.tree.hasTag(CLASSDEF) ?
Copy link
Contributor

@mcimadamore mcimadamore May 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you do a similar approach here and use staticOnly to rule out tvars?

Copy link
Contributor

@mcimadamore mcimadamore May 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can go in findType, move the assignment to staticOnly to after the call to findTypeVar - then you can drop all type var symbol you find with staticOnly = true.

Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle May 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that type variables are trickier than local variables and probably won't be as easy to tame. I tried several combinations of the patch below which I think is what you have in mind:

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 70ea875a4b3..832a8ba8ede 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
@@ -2316,29 +2316,6 @@ 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)) {
-            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) ||
-                        originalEnv.info.staticLevel > currentEnv.info.staticLevel)
-                    ))) {
-                    return new StaticError(sym);
-                }
-                return sym;
-            }
-        }
-        return typeNotFound;
-    }
-
     boolean isInnerClassOfMethod(Symbol msym, Symbol csym) {
         while (csym.owner != msym) {
             if (csym.isStatic()) return false;
@@ -2358,18 +2335,25 @@ 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);
+            Symbol tyvar = typeNotFound;
+            for (Symbol s : env1.info.scope.getSymbolsByName(name)) {
+                if (s.kind == TYP) {
+                    tyvar = s;
+                }
+            }
             sym = findImmediateMemberType(env1, env1.enclClass.sym.type,
                                           name, env1.enclClass.sym);
-
+            if (isStatic(env1)) staticOnly = true;
             // Return the type variable if we have it, and have no
             // immediate member, OR the type variable is for a method.
             if (tyvar != typeNotFound) {
                 if (env.baseClause || sym == typeNotFound ||
                     (tyvar.kind == TYP && tyvar.exists() &&
                      tyvar.owner.kind == MTH)) {
+                    if (staticOnly) {
+                        return new StaticError(tyvar);
+                    }
                     return tyvar;
                 }
             }

if the line: if (isStatic(env1)) staticOnly = true; is left in the position it appears in the patch above then valid code like:

    public static <A> List<A> nil() {
        return (List<A>)EMPTY_LIST;
    }

will stop being accepted as the type variable is being accessed from a static context, but this case is kosher. On the other hand if line if (isStatic(env1)) staticOnly = true; if moved after the if block:

if (tyvar != typeNotFound) {

then the compiler will accept the code below when it should fail:

import java.util.Collection;
class Test<T extends Collection> {
    static void test(T t) { t.iterator(); }
}

@mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented May 14, 2021

I'm doing more investigation. The code for resolving types is definitively complex. It seems to me that the complexity comes from 3 problems:

Problem 1:

class Foo<T> {
static T t;
}

Here the env for t has ` scope which can see T.

Problem 2:

class Foo<T> {
static void m() {
   T t;
}
}

Here the env for m has ` scope which can see T.

Problem 3:

class Foo {
static <Z> void m() {
   Z z;
}
}

This should obviously work, although, in compiler terms, there's not much difference between (3) and (2).

I'm currently looking into all the changes made in this area in recent times, and see if there's an easier path to support all this.

@mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented May 14, 2021

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.

Copy link
Contributor

@mcimadamore mcimadamore left a comment

Looks good

@openjdk
Copy link

@openjdk openjdk bot commented May 17, 2021

@vicente-romero-oracle This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: mcimadamore

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 137 new commits pushed to the master branch:

  • ea36836: 8267236: Versioned platform link in TestMemberSummary.java
  • d5a15f7: 8263438: Unused method AbstractMemberWriter.isInherited
  • dd5a84c: 8267162: Add jtreg test group definitions for langtools
  • 39a454b: 8260331: javax/swing/JInternalFrame/8146321/JInternalFrameIconTest.java failed with "ERROR: icon and imageIcon not same."
  • a29612e: 8255661: TestHeapDumpOnOutOfMemoryError fails with EOFException
  • a555fd8: 8264734: Some SA classes could use better hashCode() implementation
  • 2313a21: 8266637: CHT: Add insert_and_get method
  • 7b736ec: 8266489: Enable G1 to use large pages on Windows when region size is larger than 2m
  • f422787: 8266073: Regression ~2% in Derby after 8261804
  • 02f895c: 8252685: APIs that require JavaThread should take JavaThread arguments
  • ... and 127 more: https://git.openjdk.java.net/jdk/compare/e8405970b9998ff8f77bcf196f1456713a98c47f...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label May 17, 2021
@vicente-romero-oracle
Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle commented May 17, 2021

/integrate

@openjdk openjdk bot closed this May 17, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels May 17, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 17, 2021

@vicente-romero-oracle Since your change was applied there have been 137 commits pushed to the master branch:

  • ea36836: 8267236: Versioned platform link in TestMemberSummary.java
  • d5a15f7: 8263438: Unused method AbstractMemberWriter.isInherited
  • dd5a84c: 8267162: Add jtreg test group definitions for langtools
  • 39a454b: 8260331: javax/swing/JInternalFrame/8146321/JInternalFrameIconTest.java failed with "ERROR: icon and imageIcon not same."
  • a29612e: 8255661: TestHeapDumpOnOutOfMemoryError fails with EOFException
  • a555fd8: 8264734: Some SA classes could use better hashCode() implementation
  • 2313a21: 8266637: CHT: Add insert_and_get method
  • 7b736ec: 8266489: Enable G1 to use large pages on Windows when region size is larger than 2m
  • f422787: 8266073: Regression ~2% in Derby after 8261804
  • 02f895c: 8252685: APIs that require JavaThread should take JavaThread arguments
  • ... and 127 more: https://git.openjdk.java.net/jdk/compare/e8405970b9998ff8f77bcf196f1456713a98c47f...master

Your commit was automatically rebased without conflicts.

Pushed as commit b8856b1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler integrated
2 participants