-
Notifications
You must be signed in to change notification settings - Fork 120
8359370: [lworld] allow instance fields of identity classes to be readable in the prologue phase #1523
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
Open
vicente-romero-oracle
wants to merge
31
commits into
openjdk:lworld
Choose a base branch
from
vicente-romero-oracle:JDK-8359370-v2
base: lworld
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
8359370: [lworld] allow instance fields of identity classes to be readable in the prologue phase #1523
Changes from 8 commits
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
338f5d8
removing all checks from Resolve
vicente-romero-oracle 68bd63e
additional experiments
vicente-romero-oracle 32c3f4c
Merge branch 'lworld' into JDK-8359370-v2
vicente-romero-oracle 808fbef
additional changes
vicente-romero-oracle 993b657
additional changes
vicente-romero-oracle fd2cd23
restoring removed class files
vicente-romero-oracle 3b4e77d
minor fixes
vicente-romero-oracle c175f76
test changes
vicente-romero-oracle 6433fe2
refactoring
vicente-romero-oracle 399bd51
more changes
vicente-romero-oracle 841bfc0
renaming methods in LocalProxyVarGen
vicente-romero-oracle 48f12d2
merge with lworld
vicente-romero-oracle bfd0cda
bug fix
vicente-romero-oracle af7939a
another bug fix
vicente-romero-oracle e55b897
refactorings
vicente-romero-oracle 260ddf3
merge with lworld
vicente-romero-oracle 0eb8168
fixing bugs: new test cases brought in with latest merge are not acce…
vicente-romero-oracle 31c7b8c
minor refactorings and simplifications
vicente-romero-oracle a5f2947
more simplifications and bug fixes
vicente-romero-oracle 5cb0f42
documentation
vicente-romero-oracle cdfdb76
minor bug fix
vicente-romero-oracle 74945ee
some simplifications
vicente-romero-oracle b7eab98
adding documentation
vicente-romero-oracle 3965918
code simplifications
vicente-romero-oracle ad1fb80
minor diff
vicente-romero-oracle 29fc914
addressing review comments
vicente-romero-oracle 48a4713
removing unnecessary imports
vicente-romero-oracle c987a08
addressing review comments
vicente-romero-oracle 32ccabf
additional changes, more tests
vicente-romero-oracle b101d1e
moving isEarlyReference to Attr
vicente-romero-oracle e87108d
some documentation
vicente-romero-oracle File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,6 +124,7 @@ public class Attr extends JCTree.Visitor { | |
final ArgumentAttr argumentAttr; | ||
final MatchBindingsComputer matchBindingsComputer; | ||
final AttrRecover attrRecover; | ||
final LocalProxyVarsGen localProxyVarsGen; | ||
|
||
public static Attr instance(Context context) { | ||
Attr instance = context.get(attrKey); | ||
|
@@ -163,6 +164,7 @@ protected Attr(Context context) { | |
argumentAttr = ArgumentAttr.instance(context); | ||
matchBindingsComputer = MatchBindingsComputer.instance(context); | ||
attrRecover = AttrRecover.instance(context); | ||
localProxyVarsGen = LocalProxyVarsGen.instance(context); | ||
|
||
Options options = Options.instance(context); | ||
|
||
|
@@ -1250,6 +1252,22 @@ public void visitMethodDef(JCMethodDecl tree) { | |
|
||
// Attribute method body. | ||
attribStat(tree.body, localEnv); | ||
if (isConstructor) { | ||
ListBuffer<JCTree> prologueCode = new ListBuffer<>(); | ||
for (JCTree stat : tree.body.stats) { | ||
prologueCode.add(stat); | ||
// gather all the stats in the body until a `super` or `this` invocation is found | ||
if (stat instanceof JCExpressionStatement expStmt && | ||
expStmt.expr instanceof JCMethodInvocation mi && | ||
TreeInfo.isConstructorCall(mi)) { | ||
break; | ||
} | ||
} | ||
if (!prologueCode.isEmpty()) { | ||
CtorPrologueVisitor ctorPrologueVisitor = new CtorPrologueVisitor(localEnv); | ||
ctorPrologueVisitor.scan(prologueCode.toList()); | ||
} | ||
} | ||
} | ||
|
||
localEnv.info.scope.leave(); | ||
|
@@ -1261,6 +1279,177 @@ public void visitMethodDef(JCMethodDecl tree) { | |
} | ||
} | ||
|
||
class CtorPrologueVisitor extends TreeScanner { | ||
Env<AttrContext> localEnv; | ||
CtorPrologueVisitor(Env<AttrContext> localEnv) { | ||
this.localEnv = localEnv; | ||
} | ||
|
||
@Override | ||
public void visitLambda(JCLambda lambda) { | ||
super.visitLambda(lambda); | ||
java.util.List<TreeInfo.SymAndTree> symbols = TreeInfo.symbolsFor(lambda.body); | ||
for (TreeInfo.SymAndTree symAndTree : symbols) { | ||
Symbol sym = symAndTree.symbol(); | ||
JCTree tree = filter(symAndTree.tree()); | ||
if (sym.kind == VAR && rs.isEarlyReference(localEnv, tree instanceof JCFieldAccess fa ? fa.selected : null, (VarSymbol) sym)) { | ||
log.error(tree, Errors.CantRefBeforeCtorCalled(sym)); | ||
} | ||
} | ||
} | ||
|
||
@Override | ||
public void visitClassDef(JCClassDecl classDecl) { | ||
super.visitClassDef(classDecl); | ||
// local class in prologue | ||
java.util.List<TreeInfo.SymAndTree> symbols = TreeInfo.symbolsFor(classDecl.defs); | ||
for (TreeInfo.SymAndTree symAndTree : symbols) { | ||
Symbol sym = symAndTree.symbol(); | ||
JCTree tree = filter(symAndTree.tree()); | ||
if (sym.kind == VAR && rs.isEarlyReference(localEnv, tree instanceof JCFieldAccess fa ? fa.selected : null, (VarSymbol) sym)) { | ||
log.error(tree, Errors.CantRefBeforeCtorCalled(sym)); | ||
} | ||
} | ||
} | ||
|
||
@Override | ||
public void visitApply(JCMethodInvocation tree) { | ||
super.visitApply(tree); | ||
Name name = TreeInfo.name(tree.meth); | ||
boolean isConstructorCall = name == names._this || name == names._super; | ||
Symbol msym = TreeInfo.symbolFor(tree.meth); | ||
if (!isConstructorCall && !msym.isStatic()) { | ||
if (msym.owner == env.enclClass.sym) { | ||
// instance method? | ||
if (tree.meth instanceof JCFieldAccess fa) { | ||
if (TreeInfo.isExplicitThisReference(types, (ClassType)env.enclClass.sym.type, fa.selected)) { | ||
log.error(tree.meth, Errors.CantRefBeforeCtorCalled(msym)); | ||
} | ||
} else { | ||
log.error(tree.meth, Errors.CantRefBeforeCtorCalled(msym)); | ||
} | ||
} else if (msym.isMemberOf(env.enclClass.sym, types)){ | ||
// here we are dealing with an inherited method, probably an error too | ||
if (tree.meth instanceof JCFieldAccess fa) { | ||
if (TreeInfo.isExplicitThisReference(types, (ClassType)env.enclClass.sym.type, fa.selected)) { | ||
log.error(tree.meth, Errors.CantRefBeforeCtorCalled(msym)); | ||
} | ||
} else { | ||
log.error(tree.meth, Errors.CantRefBeforeCtorCalled(msym)); | ||
} | ||
} | ||
} | ||
if (isConstructorCall) { | ||
if (tree.meth instanceof JCFieldAccess fa) { | ||
if (TreeInfo.isExplicitThisReference(types, (ClassType)env.enclClass.sym.type, fa.selected)) { | ||
log.error(tree.meth, Errors.CantRefBeforeCtorCalled(msym)); | ||
} | ||
} | ||
} | ||
// we still need to check the args | ||
for (JCExpression arg : tree.args) { | ||
analyzeTree(arg); | ||
} | ||
} | ||
|
||
@Override | ||
public void visitNewClass(JCNewClass tree) { | ||
super.visitNewClass(tree); | ||
if (tree.type.tsym.isEnclosedBy(env.enclClass.sym) && !tree.type.tsym.isStatic() && !tree.type.tsym.isDirectlyOrIndirectlyLocal()) { | ||
log.error(tree, Errors.CantRefBeforeCtorCalled(tree.constructor)); | ||
} | ||
} | ||
|
||
@Override | ||
public void visitReference(JCMemberReference tree) { | ||
super.visitReference(tree); | ||
if (tree.getMode() == JCMemberReference.ReferenceMode.NEW) { | ||
if (tree.expr.type.tsym.isEnclosedBy(env.enclClass.sym) && !tree.expr.type.tsym.isStatic() && !tree.expr.type.tsym.isDirectlyOrIndirectlyLocal()) { | ||
log.error(tree, Errors.CantRefBeforeCtorCalled(tree.expr.type.getEnclosingType().tsym)); | ||
} | ||
} | ||
} | ||
|
||
@Override | ||
public void visitAssign(JCAssign tree) { | ||
super.visitAssign(tree); | ||
if (tree.rhs.type.constValue() == null) { | ||
analyzeTree(tree.rhs); | ||
} | ||
} | ||
|
||
@Override | ||
public void visitVarDef(JCVariableDecl tree) { | ||
super.visitVarDef(tree); | ||
if (tree.init != null && tree.init.type.constValue() == null) { | ||
analyzeTree(tree.init); | ||
} | ||
} | ||
|
||
JCTree filter(JCTree tree) { | ||
tree = TreeInfo.skipParens(tree); | ||
if (tree.hasTag(TYPECAST)) { | ||
tree = filter(((JCTypeCast)tree).expr); | ||
} | ||
return tree; | ||
} | ||
|
||
void analyzeTree(JCTree jcTree) { | ||
jcTree = filter(jcTree); | ||
java.util.List<TreeInfo.SymAndTree> symbols = TreeInfo.symbolsFor(jcTree); | ||
for (TreeInfo.SymAndTree symAndTree : symbols) { | ||
Symbol sym = symAndTree.symbol(); | ||
JCTree tree = filter(symAndTree.tree()); | ||
if (!sym.isStatic() && !isMethodParam(tree)) { | ||
if (sym.name == names._this || sym.name == names._super) { | ||
if (TreeInfo.isExplicitThisReference(types, (ClassType)localEnv.enclClass.sym.type, tree)) { | ||
log.error(tree, Errors.CantRefBeforeCtorCalled(sym)); | ||
} | ||
} else { | ||
if (sym != null && | ||
!sym.owner.isAnonymous() && | ||
sym.owner != localEnv.enclClass.sym && | ||
sym.kind == VAR && | ||
sym.owner.kind == TYP) { | ||
if (!tree.hasTag(IDENT) && !tree.hasTag(SELECT)) { | ||
throw new AssertionError("unexpected tree " + tree + " with tag " + tree.getTag()); | ||
} | ||
Symbol owner = tree.hasTag(IDENT) ? sym.owner : ((JCFieldAccess)tree).selected.type.tsym; | ||
if (localEnv.enclClass.sym.isSubClass(owner, types) && sym.isInheritedIn(localEnv.enclClass.sym, types)) { | ||
log.error(tree, Errors.CantRefBeforeCtorCalled(sym)); | ||
} | ||
} else { | ||
if ((sym.isFinal() || sym.isStrict()) && | ||
sym.kind == VAR && | ||
sym.owner.kind == TYP) | ||
localProxyVarsGen.addStrictFieldReadInPrologue(localEnv.enclMethod, sym); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
boolean isMethodParam(JCTree tree) { | ||
JCTree treeToCheck = null; | ||
if (tree.hasTag(IDENT)) { | ||
treeToCheck = tree; | ||
} else if (tree instanceof JCFieldAccess) { | ||
JCFieldAccess fa = (JCFieldAccess) tree; | ||
while (fa.selected.hasTag(SELECT)) { | ||
fa = (JCFieldAccess)fa.selected; | ||
} | ||
treeToCheck = fa; | ||
} | ||
if (treeToCheck != null) { | ||
Symbol sym = TreeInfo.symbolFor(treeToCheck instanceof JCFieldAccess fa ? fa.selected : treeToCheck); | ||
if (sym != null){ | ||
return sym.owner.kind == MTH; | ||
} | ||
} | ||
return false; | ||
} | ||
} | ||
|
||
public void visitVarDef(JCVariableDecl tree) { | ||
// Local variables have not been entered yet, so we need to do it now: | ||
if (env.info.scope.owner.kind == MTH || env.info.scope.owner.kind == VAR) { | ||
|
@@ -1333,6 +1522,10 @@ public void visitVarDef(JCVariableDecl tree) { | |
//fixup local variable type | ||
v.type = chk.checkLocalVarType(tree, tree.init.type, tree.name); | ||
} | ||
if (v.owner.kind == TYP && !v.isStatic() && v.isStrict()) { | ||
CtorPrologueVisitor ctorPrologueVisitor = new CtorPrologueVisitor(initEnv); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice reuse |
||
ctorPrologueVisitor.scan(tree.init); | ||
} | ||
} finally { | ||
initEnv.info.ctorPrologue = previousCtorPrologue; | ||
} | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1537,16 +1537,6 @@ Symbol findVar(DiagnosticPosition pos, Env<AttrContext> env, Name name) { | |
(sym.flags() & STATIC) == 0) { | ||
if (staticOnly) | ||
return new StaticError(sym); | ||
if (env1.info.ctorPrologue && !isAllowedEarlyReference(pos, env1, (VarSymbol)sym)) { | ||
if (!env.tree.hasTag(ASSIGN) || !TreeInfo.isIdentOrThisDotIdent(((JCAssign)env.tree).lhs)) { | ||
if (!sym.isStrictInstance()) { | ||
return new RefBeforeCtorCalledError(sym); | ||
} else { | ||
localProxyVarsGen.addStrictFieldReadInPrologue(env.enclMethod, sym); | ||
return sym; | ||
} | ||
} | ||
} | ||
} | ||
return sym; | ||
} else { | ||
|
@@ -2054,8 +2044,6 @@ Symbol findFun(Env<AttrContext> env, Name name, | |
(sym.flags() & STATIC) == 0) { | ||
if (staticOnly) | ||
return new StaticError(sym); | ||
if (env1.info.ctorPrologue && env1 == env) | ||
return new RefBeforeCtorCalledError(sym); | ||
} | ||
return sym; | ||
} else { | ||
|
@@ -3826,9 +3814,6 @@ Symbol findSelfContaining(DiagnosticPosition pos, | |
if (staticOnly) { | ||
// current class is not an inner class, stop search | ||
return new StaticError(sym); | ||
} else if (env1.info.ctorPrologue && !isAllowedEarlyReference(pos, env1, (VarSymbol)sym)) { | ||
// early construction context, stop search | ||
return new RefBeforeCtorCalledError(sym); | ||
} else { | ||
// found it | ||
return sym; | ||
|
@@ -3887,8 +3872,6 @@ Symbol resolveSelf(DiagnosticPosition pos, | |
if (sym != null) { | ||
if (staticOnly) | ||
sym = new StaticError(sym); | ||
else if (env1.info.ctorPrologue && !isAllowedEarlyReference(pos, env1, (VarSymbol)sym)) | ||
sym = new RefBeforeCtorCalledError(sym); | ||
return accessBase(sym, pos, env.enclClass.sym.type, | ||
name, true); | ||
} | ||
|
@@ -3902,8 +3885,6 @@ else if (env1.info.ctorPrologue && !isAllowedEarlyReference(pos, env1, (VarSymbo | |
//this might be a default super call if one of the superinterfaces is 'c' | ||
for (Type t : pruneInterfaces(env.enclClass.type)) { | ||
if (t.tsym == c) { | ||
if (env.info.ctorPrologue) | ||
log.error(pos, Errors.CantRefBeforeCtorCalled(name)); | ||
env.info.defaultSuperCallSite = t; | ||
return new VarSymbol(0, names._super, | ||
types.asSuper(env.enclClass.type, c), env.enclClass.sym); | ||
|
@@ -3940,64 +3921,6 @@ private List<Type> pruneInterfaces(Type t) { | |
return result.toList(); | ||
} | ||
|
||
/** | ||
* Determine if an early instance field reference may appear in a constructor prologue. | ||
* | ||
* <p> | ||
* This is only allowed when: | ||
* - The field is being assigned a value (i.e., written but not read) | ||
* - The field is not inherited from a superclass | ||
* - The assignment is not within a lambda, because that would require | ||
* capturing 'this' which is not allowed prior to super(). | ||
* | ||
* <p> | ||
* Note, this method doesn't catch all such scenarios, because this method | ||
* is invoked for symbol "x" only for "x = 42" but not for "this.x = 42". | ||
* We also don't verify that the field has no initializer, which is required. | ||
* To catch those cases, we rely on similar logic in Attr.checkAssignable(). | ||
*/ | ||
private boolean isAllowedEarlyReference(DiagnosticPosition pos, Env<AttrContext> env, VarSymbol v) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. moved this one to Attr |
||
|
||
// Check assumptions | ||
Assert.check(env.info.ctorPrologue); | ||
Assert.check((v.flags_field & STATIC) == 0); | ||
|
||
// The symbol must appear in the LHS of an assignment statement | ||
if (!(env.tree instanceof JCAssign assign)) | ||
return false; | ||
|
||
// The assignment statement must not be within a lambda | ||
if (env.info.isLambda) | ||
return false; | ||
|
||
// Get the symbol's qualifier, if any | ||
JCExpression lhs = TreeInfo.skipParens(assign.lhs); | ||
JCExpression base; | ||
switch (lhs.getTag()) { | ||
case IDENT: | ||
base = null; | ||
break; | ||
case SELECT: | ||
JCFieldAccess select = (JCFieldAccess)lhs; | ||
base = select.selected; | ||
if (!TreeInfo.isExplicitThisReference(types, (ClassType)env.enclClass.type, base)) | ||
return false; | ||
break; | ||
default: | ||
return false; | ||
} | ||
|
||
// If an early reference, the field must not be declared in a superclass | ||
if (isEarlyReference(env, base, v) && v.owner != env.enclClass.sym) | ||
return false; | ||
|
||
// The flexible constructors feature must be enabled | ||
preview.checkSourceLevel(pos, Feature.FLEXIBLE_CONSTRUCTORS); | ||
|
||
// OK | ||
return true; | ||
} | ||
|
||
/** | ||
* Determine if the variable appearance constitutes an early reference to the current class. | ||
* | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment should explain that super/this invocation is included because its arguments are restricted too.