-
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 27 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,205 @@ public void visitMethodDef(JCMethodDecl tree) { | |
} | ||
} | ||
|
||
class CtorPrologueVisitor extends TreeScanner { | ||
Env<AttrContext> localEnv; | ||
CtorPrologueVisitor(Env<AttrContext> localEnv) { | ||
this.localEnv = localEnv; | ||
} | ||
|
||
boolean insideLambdaOrClassDef = false; | ||
|
||
@Override | ||
public void visitLambda(JCLambda lambda) { | ||
boolean previousInsideLambdaOrClassDef = insideLambdaOrClassDef; | ||
try { | ||
insideLambdaOrClassDef = true; | ||
super.visitLambda(lambda); | ||
} finally { | ||
insideLambdaOrClassDef = previousInsideLambdaOrClassDef; | ||
} | ||
} | ||
|
||
@Override | ||
public void visitClassDef(JCClassDecl classDecl) { | ||
boolean previousInsideLambdaOrClassDef = insideLambdaOrClassDef; | ||
try { | ||
insideLambdaOrClassDef = true; | ||
super.visitClassDef(classDecl); | ||
} finally { | ||
insideLambdaOrClassDef = previousInsideLambdaOrClassDef; | ||
} | ||
} | ||
|
||
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 (insideLambdaOrClassDef || | ||
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'm not entirely convinced about these checks. They seem to lead to very strange asymmetries:
I understand that references to But in the last couple, we have 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. I think that the idea of restricting the access from lambdas and local classes is that they will capture 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. Ah! Forgot about that one -- but... with proxy locals that's no longer the case, no? 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. (or, do we only do local proxies for strict fields? If so, should we be more uniform here?) 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. we do local proxies for every field read in the prologue, a getField with a larval 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 wonder what is the mental model supposed to be here. @mcimadamore what is your opinion on whether this should compile? class A {
int y;
A() {
y = 1;
class B {
static void m() { // static context
System.out.println(y);
}
}
super();
}
} If your answer is "No" then aren't you then implying that If your answer is "Yes", then doesn't that imply that this should also compile... class A {
int y;
A() {
y = 1;
class B {
static void m() { // static context
System.out.println(A.this.y);
}
}
super();
}
} even though this doesn't: class A {
int y;
A() {
y = 1;
class B {
static void m() { // static context
System.out.println(A.this);
}
}
super();
}
} 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 suppose what I'm saying is: I understand why the code doesn't compile in today's world. But as we relax more restrictions and we resort to more complex translation strategies, I do wonder if some of these rules that prevent reads from lambdas will feel too tight. E.g. imagine the case of a final field -- that is written only once. If we already saw a write for that field, what stops us from being able to reference it from a lambda -- through a local proxy? I don't buy the argument 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. (in your example with static context, my answer is that no, it should not compile. A static context can't access instance fields from an 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, sorry that was a dumb example - I should have replaced the static context with a lambda. I'm all for increased flexibility, it's just that it would be nice if that came with a clean mental model. For example, one possible mental model could be "works like effectively final" - which I think is what you're advocating - but for that we'd have to break the equivalence between 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 guess that we should discuss access from a lambda with Dan, and if we decide the rules should be relaxed then do it but I think as part of a future PR. I'm also all in for relaxing restrictions |
||
(!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 +1555,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.