-
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
base: lworld
Are you sure you want to change the base?
8359370: [lworld] allow instance fields of identity classes to be readable in the prologue phase #1523
Changes from 25 commits
338f5d8
68bd63e
32c3f4c
808fbef
993b657
fd2cd23
3b4e77d
c175f76
6433fe2
399bd51
841bfc0
48f12d2
bfd0cda
af7939a
e55b897
260ddf3
0eb8168
31c7b8c
a5f2947
5cb0f42
cdfdb76
74945ee
b7eab98
3965918
ad1fb80
29fc914
48a4713
c987a08
32ccabf
b101d1e
e87108d
9014afc
792d98f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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); | ||
|
||
|
@@ -1252,6 +1254,25 @@ 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` constructor invocation is found, | ||
* including the constructor invocation, that way we don't need to worry in the visitor below if | ||
* if we are dealing or not with prologue code | ||
*/ | ||
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(); | ||
|
@@ -1263,6 +1284,209 @@ 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); | ||
classDeclAndLambdaHelper(TreeInfo.symbolsFor(lambda.body)); | ||
} | ||
|
||
@Override | ||
public void visitClassDef(JCClassDecl classDecl) { | ||
super.visitClassDef(classDecl); | ||
classDeclAndLambdaHelper(TreeInfo.symbolsFor(classDecl.defs)); | ||
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. Does this look for early references in all the symbols referenced inside the class? 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. Also, why do we use |
||
} | ||
|
||
private void classDeclAndLambdaHelper(java.util.List<TreeInfo.SymAndTree> symbols) { | ||
for (TreeInfo.SymAndTree symAndTree : symbols) { | ||
Symbol sym = symAndTree.symbol(); | ||
JCTree tree = TreeInfo.skipParens(symAndTree.tree()); | ||
if (sym.kind == VAR && | ||
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. Why doesn't this call analyze symbol? |
||
rs.isEarlyReference( | ||
true, // localEnv could potentially have the `ctorPrologue` field set to false | ||
localEnv, | ||
tree instanceof JCFieldAccess fa ? | ||
fa.selected : | ||
null, | ||
sym)) { | ||
reportPrologueError(tree, sym); | ||
} | ||
} | ||
} | ||
|
||
private void reportPrologueError(JCTree tree, Symbol sym) { | ||
preview.checkSourceLevel(tree, Feature.FLEXIBLE_CONSTRUCTORS); | ||
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); | ||
// is this an instance method call or an illegal constructor invocation like: `this.super()`? | ||
if (msym != null && // for erroneous invocations msym can be null, ignore those | ||
(!isConstructorCall || | ||
isConstructorCall && tree.meth.hasTag(SELECT))) { | ||
if (rs.isEarlyReference( | ||
true, | ||
localEnv, | ||
tree.meth instanceof JCFieldAccess fa ? fa.selected : null, | ||
msym)) | ||
reportPrologueError(tree.meth, msym); | ||
} | ||
} | ||
|
||
@Override | ||
public void visitIdent(JCIdent tree) { | ||
// skip if this identifier is part of a select, context is important here | ||
if (!analyzingSelect) { | ||
analyzeSymbol(tree); | ||
} | ||
} | ||
|
||
boolean analyzingSelect = false; | ||
|
||
@Override | ||
public void visitSelect(JCFieldAccess tree) { | ||
// skip if part of a larger select, context is important here | ||
if (!analyzingSelect) { | ||
boolean previousAnalyzingSelect = analyzingSelect; | ||
try { | ||
analyzingSelect = true; | ||
super.visitSelect(tree); | ||
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. Can't we cut recursion here (instead of using 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. if one has a complex select like for example: 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. Ok, the issue might then be that, while looking inside 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. Example:
This seems to compile, but then fails with verifier error. 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. (e.g. we really need to make sure that 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. true, now that we removed the visitor at TreeInfo that is a problem 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. Actually -- this is enough:
No lambda. So probably was an issue even before, with 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. yep :( |
||
analyzeSymbol(tree); | ||
} finally { | ||
analyzingSelect = previousAnalyzingSelect; | ||
} | ||
} | ||
} | ||
|
||
@Override | ||
public void visitNewClass(JCNewClass tree) { | ||
super.visitNewClass(tree); | ||
checkNewClassAndMethRefs(tree, tree.type); | ||
} | ||
|
||
@Override | ||
public void visitReference(JCMemberReference tree) { | ||
super.visitReference(tree); | ||
if (tree.getMode() == JCMemberReference.ReferenceMode.NEW) { | ||
checkNewClassAndMethRefs(tree, tree.expr.type); | ||
} | ||
} | ||
|
||
void checkNewClassAndMethRefs(JCTree tree, Type t) { | ||
if (t.tsym.isEnclosedBy(localEnv.enclClass.sym) && | ||
!t.tsym.isStatic() && | ||
!t.tsym.isDirectlyOrIndirectlyLocal()) { | ||
reportPrologueError(tree, t.getEnclosingType().tsym); | ||
} | ||
} | ||
|
||
/* if a symbol is in the LHS of an assignment expression we won't consider it as a candidate | ||
* for a proxy local variable later on | ||
*/ | ||
boolean isInLHS = false; | ||
|
||
@Override | ||
public void visitAssign(JCAssign tree) { | ||
boolean previousIsInLHS = isInLHS; | ||
try { | ||
isInLHS = true; | ||
scan(tree.lhs); | ||
} finally { | ||
isInLHS = previousIsInLHS; | ||
} | ||
scan(tree.rhs); | ||
} | ||
|
||
@Override | ||
public void visitMethodDef(JCMethodDecl tree) { | ||
// ignore any declarative part, mainly to avoid scanning receiver parameters | ||
scan(tree.body); | ||
} | ||
|
||
void analyzeSymbol(JCTree tree) { | ||
tree = TreeInfo.skipParens(tree); | ||
Symbol sym = TreeInfo.symbolFor(tree); | ||
if (sym != null) { | ||
if (!sym.isStatic() && !isMethodArgument(tree)) { | ||
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. if you have a 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. this is for cases when we have an argument that is for example of the same type as the current class so like:
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. yes, so isn't just checking owner.kind != TYP enough? (e.g. "not a field") 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. I tried to replace this with:
And no tests seem to fail. |
||
if (sym.name == names._this || sym.name == names._super) { | ||
// are we seeing something like `this` or `CurrentClass.this` or `SuperClass.super::foo`? | ||
if (TreeInfo.isExplicitThisReference( | ||
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. Do we always report an error when seeing 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. all the code we analyze in this visitor is in the prologue, this is why we pre-select what code we will see 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. I understand -- but in the prologue of Foo we can have a Bar.this where Bar is some enclosing class?
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. yep I see, there seems to be a bug here, thanks 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. What has fixed this exactly? It seems like this was already working as expected because 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.
yes that could be the case |
||
types, | ||
(ClassType)localEnv.enclClass.sym.type, | ||
tree)) { | ||
reportPrologueError(tree, sym); | ||
} | ||
} else if (sym.kind == VAR && sym.owner.kind == TYP) { // now fields only | ||
if (sym.owner != localEnv.enclClass.sym) { | ||
if (localEnv.enclClass.sym.isSubClass(sym.owner, types) && | ||
sym.isInheritedIn(localEnv.enclClass.sym, types)) { | ||
/* if we are dealing with a field that doesn't belong to the current class, but the | ||
* field is inherited, this is an error. Unless, the super class is also an outer | ||
* class and the field's qualifier refers to the outer class | ||
*/ | ||
if (tree.hasTag(IDENT) || | ||
TreeInfo.isExplicitThisReference( | ||
types, | ||
(ClassType)localEnv.enclClass.sym.type, | ||
((JCFieldAccess)tree).selected)) { | ||
reportPrologueError(tree, sym); | ||
} | ||
} | ||
} else if (rs.isEarlyReference( // now this is a `proper` instance field of the current class | ||
true, | ||
localEnv, | ||
tree.hasTag(SELECT) ? ((JCFieldAccess)tree).selected : null, | ||
sym | ||
)) { | ||
/* references to fields of identity classes which happen to have initializers are | ||
* not allowed in the prologue | ||
*/ | ||
if (!localEnv.enclClass.sym.isValueClass() && (sym.flags_field & HASINIT) != 0) | ||
reportPrologueError(tree, sym); | ||
// we will need to generate a proxy for this field later on | ||
if (!isInLHS) | ||
localProxyVarsGen.addFieldReadInPrologue(localEnv.enclMethod, sym); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
boolean isMethodArgument(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) { | ||
|
@@ -1335,6 +1559,11 @@ 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()) { | ||
// strict field initializers are inlined in constructor's prologues | ||
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; | ||
} | ||
|
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 understand that you wanted to simplify the visitor -- but doing a linear pass on the constructor and creating a new list of statements is also kind of expensive -- maybe when we're done with this change we can see if there's a way to set a flag on the visitor to shortcircuit the analysis after the super call is found.
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 I like the trade-off, we could try to infer if a constructor invocation corresponds to the class we are interested in. Like for example analyzing the symbol associated to a
super
orthis
invocation. But for erroneous invocations the symbol could be null. So what to do when we find a null symbol? We would have no clues I think.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.
let's wait until we address the other comments first. What I had in mind (but can be addressed in a separate PR) was maybe have a general visitor for prologue -- e.g. an helper visitor class that only visits things inside the prologue. Then you can extend that helper visitor here, to do what you need to do.