GROOVY-7785: StackoverflowException when using too many chained method calls

[daniellansun]

https://issues.apache.org/jira/browse/GROOVY-7785

[Copilot]

Pull request overview

This pull request fixes GROOVY-7785, a StackOverflowError that occurred when compiling deeply nested chained method calls in Groovy. The fix replaces a recursive approach with an iterative approach for handling method call chains in the invokedynamic writer.

Changes:

• Modified InvokeDynamicWriter to handle chained method calls iteratively instead of recursively, preventing stack overflow for deep chains
• Added comprehensive test suite with 12 test cases covering various chain patterns (StringBuilder, String methods, collections, builders, streams, etc.)
• Removed @DisabledIfEnvironmentVariable annotation from tests, enabling CI execution now that the issue is fixed

Reviewed changes

Copilot reviewed 1 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/test/groovy/bugs/Groovy7785.groovy	Added comprehensive regression tests with 500+ chained calls per test to verify the fix handles deeply nested method chains without stack overflow
src/main/java/org/codehaus/groovy/classgen/asm/indy/InvokeDynamicWriter.java	Implemented iterative approach for visiting receiver expressions of chained method calls with helper methods for chain detection and processing

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

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.

Suggested change

	Expression receiver = call.getObjectExpression();
	if (isSuperExpression(receiver)) {
	adapter = invokeMethodOnSuper;
	} else if (isThisExpression(receiver)) {
	adapter = invokeMethodOnCurrent;
	}

[Copilot]

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.

Suggested change

	// fallback to normal path which will handle dynamic method names
	call.visit(controller.getAcg());
	return;
	// 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");

[codecov-commenter]

Codecov Report

❌ Patch coverage is 90.00000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.6114%. Comparing base (4d7fc0a) to head (b8e01b5).
⚠️ Report is 6 commits behind head on master.

Files with missing lines	Patch %	Lines
.../groovy/classgen/asm/indy/InvokeDynamicWriter.java	90.0000%	2 Missing and 3 partials ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##               master      #2375        +/-   ##
==================================================
+ Coverage     68.3817%   68.6114%   +0.2297%     
- Complexity      30342      30797       +455     
==================================================
  Files            1382       1382                
  Lines          116151     117221      +1070     
  Branches        20462      20919       +457     
==================================================
+ Hits            79426      80427      +1001     
- Misses          30229      30264        +35     
- Partials         6496       6530        +34     

Files with missing lines	Coverage Δ
.../groovy/classgen/asm/indy/InvokeDynamicWriter.java	94.6309% <90.0000%> (-2.3691%)	⬇️

... and 15 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[paulk-asert]

LGTM

[eric-milles]

For the original stated problem, how many object expressions approximately does it take to overflow the stack? And how much stack space given as a command line argument can resolve the issue?

[daniellansun]

For the original stated problem, how many object expressions approximately does it take to overflow the stack?

About 800

And how much stack space given as a command line argument can resolve the issue?

I have not tried to estimate the stack space because I want to solve the issue in a better way from the beginning.

[eric-milles]

Yes, but a general issue we have is tree navigation via nested method calls. So we have stack depth sensitivity all over the place. And if 800 chained calls is a rare edge case, why do we need to build in a solution for it when such a user could just set "-Xss2m" as a JVM arg on their compiler?

[daniellansun]

"-Xss2m" will can help us workaround the issue but it will impact the whole JVM, usually not recommended for the issue may happen at the server side.

Chained method calls are common for fluent style API, and long chained method calls may appear in the application that need generate Groovy code according to complex business logic.

Truely, the PR can not solve the general stack overflow but it can solve the issue mentioned above.