Copilot commented on code in PR #2375:
URL: https://github.com/apache/groovy/pull/2375#discussion_r2751582581
##########
src/main/java/org/codehaus/groovy/classgen/asm/indy/InvokeDynamicWriter.java:
##########
@@ -113,12 +116,133 @@ private String prepareIndyCall(final Expression
receiver, final boolean implicit
// load normal receiver as first argument
compileStack.pushImplicitThis(implicitThis);
- receiver.visit(controller.getAcg());
+ // GROOVY-7785: use iterative approach to avoid stack overflow for
chained method calls
+ visitReceiverOfMethodCall(receiver);
compileStack.popImplicitThis();
return "(" + getTypeDescription(operandStack.getTopOperand());
}
+ /**
+ * Visit receiver expression iteratively to avoid stack overflow for
deeply nested method call chains.
+ * For chained calls like a().b().c()...z(), the AST forms a deep
right-recursive structure where
+ * each method call's receiver is another method call. This method
flattens the chain and processes
+ * it iteratively from the innermost receiver outward.
+ */
+ private void visitReceiverOfMethodCall(final Expression receiver) {
+ // Collect the chain of method calls that can be handled by indy
+ List<MethodCallExpression> chain = new ArrayList<>();
+ Expression current = receiver;
+ while (current instanceof MethodCallExpression mce &&
canUseIndyForChain(mce)) {
+ chain.add(mce);
+ current = mce.getObjectExpression();
+ }
+
+ if (chain.isEmpty()) {
+ // Not a chainable method call or chain cannot be optimized, use
normal visit
+ receiver.visit(controller.getAcg());
+ return;
+ }
+
+ // Process the innermost non-chainable receiver first
+ current.visit(controller.getAcg());
+
+ // Process each method call in the chain, from innermost to outermost
+ AsmClassGenerator acg = controller.getAcg();
+ for (int i = chain.size() - 1; i >= 0; i -= 1) {
+ MethodCallExpression mce = chain.get(i);
+ acg.onLineNumber(mce, "visitMethodCallExpression (chained): \"" +
mce.getMethod() + "\":");
+ // Process this method call with its receiver already on the stack
+ makeIndyCallWithReceiverOnStack(mce);
+ controller.getAssertionWriter().record(mce.getMethod());
+ }
+ }
+
+ /**
+ * Check if a method call can be handled in the chained call optimization.
+ * Only simple method calls that go through the standard indy path can be
optimized.
+ */
+ private boolean canUseIndyForChain(final MethodCallExpression call) {
+ // Spread safe calls need special handling and cannot be optimized
+ if (call.isSpreadSafe()) return false;
+ // Super calls have different invocation semantics and should not be
optimized
+ if (isSuperExpression(call.getObjectExpression())) return false;
+ // This calls and implicit this calls have special context handling
+ if (isThisExpression(call.getObjectExpression())) return false;
+ if (call.isImplicitThis()) return false;
+ // Dynamic method names (non-constant) cannot be handled
+ String methodName = getMethodName(call.getMethod());
+ if (methodName == null) return false;
+ // "call" method invocations may need special handling for functional
interfaces (GROOVY-8466)
+ if ("call".equals(methodName)) return false;
+ return true;
+ }
+
+ /**
+ * Process a method call expression assuming its receiver is already on
the operand stack.
+ */
+ private void makeIndyCallWithReceiverOnStack(final MethodCallExpression
call) {
+ MethodCallerMultiAdapter adapter = invokeMethod;
+ Expression receiver = call.getObjectExpression();
+ if (isSuperExpression(receiver)) {
+ adapter = invokeMethodOnSuper;
+ } else if (isThisExpression(receiver)) {
+ adapter = invokeMethodOnCurrent;
+ }
Review Comment:
The adapter selection logic in makeIndyCallWithReceiverOnStack checks if the
receiver expression is a super or this expression. However, this check is
redundant because canUseIndyForChain already excludes both super and this
expressions from the chained optimization. When a call passes
canUseIndyForChain, its receiver cannot be super or this, so the adapter will
always be invokeMethod. Consider removing lines 187-191 or adjusting
canUseIndyForChain to allow this/super calls while keeping the adapter logic
here.
```suggestion
```
##########
src/main/java/org/codehaus/groovy/classgen/asm/indy/InvokeDynamicWriter.java:
##########
@@ -113,12 +116,133 @@ private String prepareIndyCall(final Expression
receiver, final boolean implicit
// load normal receiver as first argument
compileStack.pushImplicitThis(implicitThis);
- receiver.visit(controller.getAcg());
+ // GROOVY-7785: use iterative approach to avoid stack overflow for
chained method calls
+ visitReceiverOfMethodCall(receiver);
compileStack.popImplicitThis();
return "(" + getTypeDescription(operandStack.getTopOperand());
}
+ /**
+ * Visit receiver expression iteratively to avoid stack overflow for
deeply nested method call chains.
+ * For chained calls like a().b().c()...z(), the AST forms a deep
right-recursive structure where
+ * each method call's receiver is another method call. This method
flattens the chain and processes
+ * it iteratively from the innermost receiver outward.
+ */
+ private void visitReceiverOfMethodCall(final Expression receiver) {
+ // Collect the chain of method calls that can be handled by indy
+ List<MethodCallExpression> chain = new ArrayList<>();
+ Expression current = receiver;
+ while (current instanceof MethodCallExpression mce &&
canUseIndyForChain(mce)) {
+ chain.add(mce);
+ current = mce.getObjectExpression();
+ }
+
+ if (chain.isEmpty()) {
+ // Not a chainable method call or chain cannot be optimized, use
normal visit
+ receiver.visit(controller.getAcg());
+ return;
+ }
+
+ // Process the innermost non-chainable receiver first
+ current.visit(controller.getAcg());
+
+ // Process each method call in the chain, from innermost to outermost
+ AsmClassGenerator acg = controller.getAcg();
+ for (int i = chain.size() - 1; i >= 0; i -= 1) {
+ MethodCallExpression mce = chain.get(i);
+ acg.onLineNumber(mce, "visitMethodCallExpression (chained): \"" +
mce.getMethod() + "\":");
+ // Process this method call with its receiver already on the stack
+ makeIndyCallWithReceiverOnStack(mce);
+ controller.getAssertionWriter().record(mce.getMethod());
+ }
+ }
+
+ /**
+ * Check if a method call can be handled in the chained call optimization.
+ * Only simple method calls that go through the standard indy path can be
optimized.
+ */
+ private boolean canUseIndyForChain(final MethodCallExpression call) {
+ // Spread safe calls need special handling and cannot be optimized
+ if (call.isSpreadSafe()) return false;
+ // Super calls have different invocation semantics and should not be
optimized
+ if (isSuperExpression(call.getObjectExpression())) return false;
+ // This calls and implicit this calls have special context handling
+ if (isThisExpression(call.getObjectExpression())) return false;
+ if (call.isImplicitThis()) return false;
+ // Dynamic method names (non-constant) cannot be handled
+ String methodName = getMethodName(call.getMethod());
+ if (methodName == null) return false;
+ // "call" method invocations may need special handling for functional
interfaces (GROOVY-8466)
+ if ("call".equals(methodName)) return false;
+ return true;
+ }
+
+ /**
+ * Process a method call expression assuming its receiver is already on
the operand stack.
+ */
+ private void makeIndyCallWithReceiverOnStack(final MethodCallExpression
call) {
+ MethodCallerMultiAdapter adapter = invokeMethod;
+ Expression receiver = call.getObjectExpression();
+ if (isSuperExpression(receiver)) {
+ adapter = invokeMethodOnSuper;
+ } else if (isThisExpression(receiver)) {
+ adapter = invokeMethodOnCurrent;
+ }
+
+ String methodName = getMethodName(call.getMethod());
+ if (methodName == null) {
+ // fallback to normal path which will handle dynamic method names
+ call.visit(controller.getAcg());
+ return;
Review Comment:
The fallback logic in makeIndyCallWithReceiverOnStack (lines 194-198) calls
visit on the entire MethodCallExpression, which will re-process the receiver
and may cause issues since the receiver is already on the operand stack. This
could lead to the receiver being pushed twice onto the stack, resulting in
incorrect bytecode. Since canUseIndyForChain already validates that methodName
is not null before a call is added to the chain, this fallback should never be
reached. Consider either removing this fallback or documenting why it's needed
despite the pre-validation.
```suggestion
// This should never happen: canUseIndyForChain ensures
methodName is non-null
// before makeIndyCallWithReceiverOnStack is used. Re-visiting
the call here
// would re-process the receiver, which is already on the
operand stack, and
// could generate incorrect bytecode, so treat this as an
internal error.
throw new IllegalStateException("Unexpected null method name in
makeIndyCallWithReceiverOnStack");
```
--
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]
