GROOVY-11737: Improve clarity of Groovy main method selection priority by paulk-asert · Pull Request #2428 · apache/groovy

paulk-asert · 2026-04-02T12:12:19Z

No description provided.

codecov-commenter · 2026-04-02T12:37:13Z

Codecov Report

❌ Patch coverage is 62.16216% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.3402%. Comparing base (2a7d1e9) to head (38ecbb8).

Files with missing lines	Patch %	Lines
src/main/java/groovy/lang/GroovyShell.java	69.2308%	6 Missing and 2 partials ⚠️
.../main/java/org/codehaus/groovy/ast/ModuleNode.java	45.4546%	5 Missing and 1 partial ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##               master      #2428        +/-   ##
==================================================
+ Coverage     66.3339%   66.3402%   +0.0063%     
- Complexity      29941      29949         +8     
==================================================
  Files            1396       1396                
  Lines          117085     117101        +16     
  Branches        20730      20737         +7     
==================================================
+ Hits            77667      77685        +18     
- Misses          33037      33039         +2     
+ Partials         6381       6377         -4

Files with missing lines	Coverage Δ
.../main/java/org/codehaus/groovy/ast/ModuleNode.java	`85.7616% <45.4546%> (-1.3132%)`	⬇️
src/main/java/groovy/lang/GroovyShell.java	`63.1579% <69.2308%> (+1.0700%)`	⬆️

... and 2 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.

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Improves how Groovy chooses among multiple valid main methods by documenting the priority order and updating both compilation-time and runtime selection logic to align with that order.

Changes:

Documented main method selection priority and warning behavior in the core spec docs.
Updated ModuleNode to select a “winning” main by priority and emit compile-time warnings for unreachable overloads.
Updated GroovyShell to deterministically select and invoke the prioritized main method via reflection.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File	Description
src/spec/doc/core-program-structure.adoc	Adds a dedicated section describing `main` selection priority and shadowing warnings.
src/main/java/org/codehaus/groovy/ast/ModuleNode.java	Collects valid `main` methods, selects a winner, and warns on unreachable overloads.
src/main/java/groovy/lang/GroovyShell.java	Centralizes prioritized `main` lookup and invokes it directly to avoid multimethod dispatch surprises.

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

Copilot · 2026-04-08T10:07:22Z

src/main/java/org/codehaus/groovy/ast/ModuleNode.java

        boolean foundInstance = false;
        boolean foundStatic = false;
-        MethodNode result = null;
+        List<MethodNode> validMains = new ArrayList<>();
        for (MethodNode node : methods) {
            if ("main".equals(node.getName()) && !node.isPrivate()) {
                int numParams = node.getParameters().length;


The foundStatic/foundInstance guard still throws when there is more than one valid static (or instance) main, which contradicts the new behavior and docs (multiple overloads are now expected and should be prioritized). Remove this guard or replace it with a check that only rejects true duplicates (same signature), which should typically be impossible anyway.

Copilot · 2026-04-08T10:07:22Z

src/main/java/org/codehaus/groovy/ast/ModuleNode.java

                        if (node.isStatic() ? foundStatic : foundInstance) {
                            throw new RuntimeException("Repetitive main method found.");
                        }
-                        if (!foundStatic) { // static trumps instance
-                            result = node;
-                        }
+                        validMains.add(node);

                        if (node.isStatic()) foundStatic = true;
                        else foundInstance = true;


The foundStatic/foundInstance guard still throws when there is more than one valid static (or instance) main, which contradicts the new behavior and docs (multiple overloads are now expected and should be prioritized). Remove this guard or replace it with a check that only rejects true duplicates (same signature), which should typically be impossible anyway.

Copilot · 2026-04-08T10:07:22Z

src/main/java/org/codehaus/groovy/ast/ModuleNode.java

+        // Select winner using JEP-512 priority: static before instance, args before no-args
+        validMains.sort((a, b) -> {
+            if (a.isStatic() != b.isStatic()) return a.isStatic() ? -1 : 1;
+            return Integer.compare(b.getParameters().length, a.getParameters().length);
+        });


This comparator doesn’t implement the documented priority between main(String[] args) and main(Object args) because both have getParameters().length == 1. The resulting winner depends on the original method iteration order (and sort stability), making selection potentially non-deterministic and not aligned with the stated priority. Add a tie-breaker that ranks parameter type (String[] higher priority than Object) after the static/instance comparison and before the no-arg comparison.

Copilot · 2026-04-08T10:07:23Z

src/main/java/org/codehaus/groovy/ast/ModuleNode.java

+            getContext().addWarning("Method '" + unreachable.getText()
+                    + "' is not reachable from the Groovy runner"
+                    + " because a higher-priority main method '"
+                    + result.getText() + "' exists", unreachable);


MethodNode.getText() can be overly verbose and/or unstable for user-facing warnings (it may include more than a concise signature). Consider formatting a compact signature for both methods (e.g., name + parameter types + static/instance) to keep warnings readable and consistent.

Copilot · 2026-04-08T10:07:23Z

src/main/java/groovy/lang/GroovyShell.java

+        Method selected = findMainMethod(scriptClass);
+        if (selected != null) {
+            try {
+                selected.setAccessible(true);


findMainMethod uses Class#getMethod, which returns only public methods, so setAccessible(true) should be unnecessary. Keeping it can introduce avoidable failures under restrictive security/module settings; consider removing it (or only setting accessible when using getDeclaredMethod).

Suggested change

selected.setAccessible(true);

Copilot · 2026-04-08T10:07:23Z

src/main/java/groovy/lang/GroovyShell.java

+                throw e.getCause() instanceof RuntimeException re ? re
+                        : new InvokerInvocationException(e);


When main throws a checked exception or an Error, wrapping the InvocationTargetException itself can obscure the real cause. Consider (a) rethrowing Error causes directly, and (b) wrapping e.getCause() (not e) so stack traces and messages point at the user exception rather than InvocationTargetException.

Suggested change

throw e.getCause() instanceof RuntimeException re ? re

: new InvokerInvocationException(e);

Throwable cause = e.getCause();

if (cause instanceof RuntimeException re) {

throw re;

}

if (cause instanceof Error err) {

throw err;

}

throw new InvokerInvocationException(cause);

GROOVY-11737: Improve clarity of Groovy main method selection priority

38ecbb8

paulk-asert requested a review from Copilot April 8, 2026 10:00

Copilot AI reviewed Apr 8, 2026

View reviewed changes

Copilot started reviewing on behalf of paulk-asert April 8, 2026 10:17 View session

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GROOVY-11737: Improve clarity of Groovy main method selection priority#2428

GROOVY-11737: Improve clarity of Groovy main method selection priority#2428
paulk-asert wants to merge 1 commit intoapache:masterfrom
paulk-asert:groovy11737

paulk-asert commented Apr 2, 2026

Uh oh!

codecov-commenter commented Apr 2, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		throw e.getCause() instanceof RuntimeException re ? re
		: new InvokerInvocationException(e);

-                throw e.getCause() instanceof RuntimeException re ? re
-                        : new InvokerInvocationException(e);
+                Throwable cause = e.getCause();
+                if (cause instanceof RuntimeException re) {
+                    throw re;
+                }
+                if (cause instanceof Error err) {
+                    throw err;
+                }
+                throw new InvokerInvocationException(cause);

Conversation

paulk-asert commented Apr 2, 2026

Uh oh!

codecov-commenter commented Apr 2, 2026

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants