[
https://issues.apache.org/jira/browse/GROOVY-7785?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18055798#comment-18055798
]
ASF GitHub Bot commented on GROOVY-7785:
----------------------------------------
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");
```
> StackoverflowException when using too many chained method calls
> ---------------------------------------------------------------
>
> Key: GROOVY-7785
> URL: https://issues.apache.org/jira/browse/GROOVY-7785
> Project: Groovy
> Issue Type: Bug
> Affects Versions: 2.4.6
> Reporter: Daniel Kuppitz
> Priority: Major
> Labels: StackOverflowError
>
> If the following statement is pasted into a Groovy shell, it's going to throw
> a {{StackoverflowException}}:
> {code}
> new
> StringBuilder().append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a").append("a")
> {code}
> Trying to nail it to a certain number of method calls by starting with less
> methods and then adding one at a time doesn't work, since that apparently
> never throws an exception.
> Note that this is not tied to {{StringBuilder::append}}. We discovered this
> issue in Gremlin, where it's pretty common to have a lot of chained method
> calls.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
