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
Closed
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -1488,13 +1488,16 @@ Symbol findVar(Env<AttrContext> env, Name name) {
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);
}
@@ -2323,9 +2326,11 @@ Symbol findTypeVar(Env<AttrContext> currentEnv, Env<AttrContext> originalEnv, Na
allowRecords &&
(sym.owner.kind == MTH &&
currentEnv != originalEnv &&
!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(); }
}

((JCClassDecl)originalEnv.tree).sym :
originalEnv.enclClass.sym) ||
originalEnv.info.staticLevel > currentEnv.info.staticLevel)
))) {
return new StaticError(sym);
}
return sym;
@@ -746,12 +746,24 @@ void test(T t) {}
}
""",
"""
record R() {
void test(U u) {}
}
""",
"""
record R() {
void test1() {
class X { void test2(T t) {} }
}
}
""",
"""
record R() {
void test1() {
class X { void test2(U u) {} }
}
}
""",

"""
interface I {
@@ -801,12 +813,24 @@ default void test(T t) {}
}
""",
"""
interface I {
default void test(U u) {}
}
""",
"""
interface I {
default void test1() {
class X { void test2(T t) {} }
}
}
""",
"""
interface I {
default void test1() {
class X { void test2(U u) {} }
}
}
""",

"""
enum E {
@@ -863,13 +887,27 @@ void test(T t) {}
}
""",
"""
enum E {
A;
void test(U u) {}
}
""",
"""
enum E {
A;
void test1() {
class X { void test2(T t) {} }
}
}
""",
"""
enum E {
A;
void test1() {
class X { void test2(U u) {} }
}
}
""",

"""
static class SC {
@@ -919,11 +957,23 @@ void test(T t) {}
}
""",
"""
static class SC {
void test(U u) {}
}
""",
"""
static class SC {
void test1() {
class X { void test2(T t) {} }
}
}
""",
"""
static class SC {
void test1() {
class X { void test2(U u) {} }
}
}
"""
)) {
assertFail("compiler.err.non-static.cant.be.ref",
@@ -965,6 +1015,30 @@ static void m() {}
}
"""
);

// but still non-static declarations can't be accessed from a static method inside a local class
for (String s : List.of(
"System.out.println(localVar)",
"System.out.println(param)",
"System.out.println(field)",
"T t",
"U u"
)) {
assertFail("compiler.err.non-static.cant.be.ref",
"""
class C<T> {
int field = 0;
<U> void foo(int param) {
int localVar = 1;
class Local {
static void m() {
#S;
}
}
}
}
""".replaceFirst("#S", s));
}
}

public void testReturnInCanonical_Compact() {