daniellansun commented on code in PR #3049:
URL: https://github.com/apache/groovy/pull/3049#discussion_r4425287548
##########
src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java:
##########
@@ -1498,9 +1508,9 @@ public void visitPropertyExpression(final
PropertyExpression expression) {
if (!visited) {
MethodCallerMultiAdapter adapter;
if (controller.getCompileStack().isLHS()) {
- adapter = isSuperExpression(objectExpression) ?
setPropertyOnSuper : isGroovyObject(expression) ? setGroovyObjectProperty :
setProperty;
+ adapter = isSuperOrOuterSuper(objectExpression) ?
setPropertyOnSuper : isGroovyObject(expression) ? setGroovyObjectProperty :
setProperty;
Review Comment:
Property form now goes through isSuperOrOuterSuper → get/setPropertyOnSuper,
but the attribute adapters in visitAttributeExpression (tip ~1550) still key
only on isSuperExpression. Outer.super.@q will load Outer.this and then
getField without useSuper, i.e. Outer's own field — the same bug shape this PR
fixes for Outer.super.q.
Please mirror isSuperOrOuterSuper there (and cover Outer.super.@x /
N.super.@x in InnerClassTest), or open a follow-up ticket and call it out in
the PR so the @ form is not left silently wrong.
##########
src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java:
##########
@@ -1355,6 +1355,16 @@ public static FieldNode
getDeclaredFieldOfCurrentClassOrAccessibleFieldOfSuper(f
);
}
+ /**
+ * Determines if an expression is "super" or "Outer.super" for a class: a
+ * property of it is a property of the superclass of the instance.
+ */
+ private static boolean isSuperOrOuterSuper(final Expression expression) {
Review Comment:
isSuperOrOuterSuper matches InvocationWriter.isOuterClassSuper /
isClassWithSuper (GROOVY-11611). Fine as a local copy for this fix, but
consider one shared ExpressionUtils (or package) helper so the !isInterface()
guard stays aligned with GROOVY-12526 (#3071).
##########
src/main/java/org/codehaus/groovy/control/ResolveVisitor.java:
##########
@@ -1192,6 +1192,11 @@ protected Expression transformPropertyExpression(final
PropertyExpression pe) {
}
checkThisAndSuperAsPropertyAccess(xe);
+ if (isSuperOfCurrentNestedClass(xe)) { // "Type.super" within nested
class Type is "super"
Review Comment:
Good place for the rewrite (after checkThisAndSuperAsPropertyAccess). When
rebasing onto or against #3071 (GROOVY-12526), keep the interface-super
rejection as an early path for ClassExpression + interface + "super", and keep
this rewrite only for non-interface nested classes — the interface early-return
must not swallow N.super.
##########
src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java:
##########
@@ -828,6 +841,103 @@ && getField(receiverType, name, FieldNode::isPublic) ==
null) {
super.fallbackAttributeOrPropertySite(expression, objectExpression,
name, adapter);
}
+ /**
+ * Reads or writes "super.name", as the operand of "++" or "--" or the
target
+ * of a compound assignment, through the accessor of the superclass.
Within a
+ * closure or lambda, and for "Outer.super.name", the accessor is called
through
+ * the super$ helper the type checker asked the enclosing class for. An
accessor
+ * that is an extension method is called directly, as it needs no helper.
+ */
+ private boolean makeSuperPropertySite(final PropertyExpression expression,
final Expression objectExpression, final String name, final boolean lhs) {
+ ClassNode helperType =
expression.getNodeMetaData(StaticTypesMarker.SUPER_MOP_METHOD_REQUIRED);
+ MethodNode target = findSuperPropertyExtension(expression,
objectExpression, name, lhs);
+ Expression receiver;
+ if (target != null) {
+ helperType = null;
+ receiver = objectExpression;
+ } else if (helperType != null) {
+ MethodNode accessor = findSuperPropertyAccessor(expression,
helperType.getSuperClass(), name, lhs);
+ List<MethodNode> helpers =
helperType.getNodeMetaData(StaticTypesMarker.SUPER_MOP_METHOD_REQUIRED);
+ if (accessor == null || helpers == null ||
helpers.stream().noneMatch(mn -> isSameSuperMopMethod(mn, accessor))) {
+ return false;
+ }
+ target = StaticCompilationMopWriter.getSuperMopMethod(helperType,
accessor);
+ receiver = varX("this");
+ } else if (isSuperExpression(objectExpression) &&
!controller.isInGeneratedFunction()) {
+ target = findSuperPropertyAccessor(expression,
controller.getClassNode().getSuperClass(), name, lhs);
+ if (target == null) {
+ return false;
+ }
+ receiver = objectExpression;
+ } else {
+ return false;
+ }
+
+ CompileStack compileStack = controller.getCompileStack();
+ OperandStack operandStack = controller.getOperandStack();
+ MethodCallExpression call;
+ int value = 0;
+ if (lhs) { // the value to store is on the operand stack
+ ClassNode valueType = operandStack.getTopOperand();
+ value = compileStack.defineTemporaryVariable("$value", valueType,
true);
+ call = callX(receiver, target.getName(), args(new
VariableSlotLoader(valueType, value, operandStack)));
+ } else {
+ call = callX(receiver, target.getName());
+ }
+ call.setImplicitThis(helperType != null);
+ call.setMethodTarget(target);
+ call.setSourcePosition(expression);
+ compileStack.pushLHS(false);
+ call.visit(controller.getAcg());
+ compileStack.popLHS();
+ if (lhs) {
+ operandStack.pop();
+ compileStack.removeVar(value);
+ }
+ return true;
+ }
+
+ private static MethodNode findSuperPropertyAccessor(final
PropertyExpression expression, final ClassNode superType, final String name,
final boolean setter) {
Review Comment:
findSuperPropertyAccessor looks up isX then getX on the superclass, while
the setter path trusts DIRECT_METHOD_CALL_TARGET. That matches
Verifier-generated boolean properties and the new tests. Optional hardening:
for getters, prefer DIRECT_METHOD_CALL_TARGET when present (as setters do) so a
synthetic isX recorded on SUPER_MOP_METHOD_REQUIRED cannot disagree with a lone
getX on the ClassNode.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
