You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by "paulk-asert (via GitHub)" <gi...@apache.org> on 2026/04/02 12:12:23 UTC
[PR] GROOVY-11737: Improve clarity of Groovy main method selection priority [groovy]
paulk-asert opened a new pull request, #2428:
URL: https://github.com/apache/groovy/pull/2428
(no comment)
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11737: Improve clarity of Groovy main method selection priority [groovy]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2428:
URL: https://github.com/apache/groovy/pull/2428#issuecomment-4177571747
## [Codecov](https://app.codecov.io/gh/apache/groovy/pull/2428?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `62.16216%` with `14 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 66.3402%. Comparing base ([`2a7d1e9`](https://app.codecov.io/gh/apache/groovy/commit/2a7d1e9b5a6bc86392dd37d050130ce0929b6eff?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`38ecbb8`](https://app.codecov.io/gh/apache/groovy/commit/38ecbb870dbfd18a52b35b250d730093bd4682f9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/groovy/pull/2428?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [src/main/java/groovy/lang/GroovyShell.java](https://app.codecov.io/gh/apache/groovy/pull/2428?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Flang%2FGroovyShell.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvbGFuZy9Hcm9vdnlTaGVsbC5qYXZh) | 69.2308% | [6 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/groovy/pull/2428?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../main/java/org/codehaus/groovy/ast/ModuleNode.java](https://app.codecov.io/gh/apache/groovy/pull/2428?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fast%2FModuleNode.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2FzdC9Nb2R1bGVOb2RlLmphdmE=) | 45.4546% | [5 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/groovy/pull/2428?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2428?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ 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](https://app.codecov.io/gh/apache/groovy/pull/2428?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [.../main/java/org/codehaus/groovy/ast/ModuleNode.java](https://app.codecov.io/gh/apache/groovy/pull/2428?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fast%2FModuleNode.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2FzdC9Nb2R1bGVOb2RlLmphdmE=) | `85.7616% <45.4546%> (-1.3132%)` | :arrow_down: |
| [src/main/java/groovy/lang/GroovyShell.java](https://app.codecov.io/gh/apache/groovy/pull/2428?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Flang%2FGroovyShell.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvbGFuZy9Hcm9vdnlTaGVsbC5qYXZh) | `63.1579% <69.2308%> (+1.0700%)` | :arrow_up: |
... and [2 files with indirect coverage changes](https://app.codecov.io/gh/apache/groovy/pull/2428/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11737: Improve clarity of Groovy main method selection priority [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert merged PR #2428:
URL: https://github.com/apache/groovy/pull/2428
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11737: Improve clarity of Groovy main method selection priority [groovy]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2428:
URL: https://github.com/apache/groovy/pull/2428#discussion_r3050620131
##########
src/main/java/groovy/lang/GroovyShell.java:
##########
@@ -259,39 +259,29 @@ private Object runScriptOrMainOrTestOrRunnable(Class scriptClass, String[] args)
// ignore instantiation errors, try to do main
}
}
- try {
- // let's find a String[] main method
- Method stringArrayMain = scriptClass.getMethod(MAIN_METHOD_NAME, String[].class);
- // if that main method exists, invoke it
- if (Modifier.isStatic(stringArrayMain.getModifiers())) {
- return InvokerHelper.invokeStaticMethod(scriptClass, MAIN_METHOD_NAME, new Object[]{args});
- } else {
- Object script = InvokerHelper.invokeNoArgumentsConstructorOf(scriptClass);
- return InvokerHelper.invokeMethod(script, MAIN_METHOD_NAME, args);
- }
- } catch (NoSuchMethodException ignore) { }
- try {
- // let's find an Object main method
- Method stringArrayMain = scriptClass.getMethod(MAIN_METHOD_NAME, Object.class);
- // if that main method exists, invoke it
- if (Modifier.isStatic(stringArrayMain.getModifiers())) {
- return InvokerHelper.invokeStaticMethod(scriptClass, MAIN_METHOD_NAME, new Object[]{args});
- } else {
- Object script = InvokerHelper.invokeNoArgumentsConstructorOf(scriptClass);
- return InvokerHelper.invokeMethod(script, MAIN_METHOD_NAME, new Object[]{args});
- }
- } catch (NoSuchMethodException ignore) { }
- try {
- // let's find a no-arg main method
- Method noArgMain = scriptClass.getMethod(MAIN_METHOD_NAME);
- // if that main method exists, invoke it
- if (Modifier.isStatic(noArgMain.getModifiers())) {
- return InvokerHelper.invokeStaticNoArgumentsMethod(scriptClass, MAIN_METHOD_NAME);
- } else {
- Object script = InvokerHelper.invokeNoArgumentsConstructorOf(scriptClass);
- return InvokerHelper.invokeMethod(script, MAIN_METHOD_NAME, EMPTY_ARGS);
+ // Select main method using JEP-512 priority: static before instance, args before no-args.
+ // Uses Method.invoke() directly to avoid Groovy multimethod dispatch selecting a different overload.
+ Method selected = findMainMethod(scriptClass);
+ if (selected != null) {
+ try {
+ selected.setAccessible(true);
+ if (Modifier.isStatic(selected.getModifiers())) {
+ return selected.getParameterCount() == 0
+ ? selected.invoke(null)
+ : selected.invoke(null, (Object) args);
+ } else {
+ Object instance = InvokerHelper.invokeNoArgumentsConstructorOf(scriptClass);
+ return selected.getParameterCount() == 0
+ ? selected.invoke(instance)
+ : selected.invoke(instance, (Object) args);
+ }
+ } catch (InvocationTargetException e) {
+ throw e.getCause() instanceof RuntimeException re ? re
+ : new InvokerInvocationException(e);
Review Comment:
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`.
```suggestion
Throwable cause = e.getCause();
if (cause instanceof RuntimeException re) {
throw re;
}
if (cause instanceof Error err) {
throw err;
}
throw new InvokerInvocationException(cause);
```
##########
src/main/java/org/codehaus/groovy/ast/ModuleNode.java:
##########
@@ -527,16 +530,33 @@ private MethodNode handleMainMethodIfPresent(final List<MethodNode> methods) {
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;
Review Comment:
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.
##########
src/main/java/org/codehaus/groovy/ast/ModuleNode.java:
##########
@@ -509,11 +509,14 @@ private MethodNode findRun() {
* We retain the 'main' method if a compatible one is found.
* A compatible one has no parameters or 1 (Object or String[]) parameter.
* The return type must be void or Object.
+ * When multiple valid main methods exist, a warning is issued for those
+ * that would not be reachable from the command-line runner.
+ * Priority follows JEP-512: static before instance, args before no-args.
*/
private MethodNode handleMainMethodIfPresent(final List<MethodNode> methods) {
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;
Review Comment:
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.
##########
src/main/java/groovy/lang/GroovyShell.java:
##########
@@ -259,39 +259,29 @@ private Object runScriptOrMainOrTestOrRunnable(Class scriptClass, String[] args)
// ignore instantiation errors, try to do main
}
}
- try {
- // let's find a String[] main method
- Method stringArrayMain = scriptClass.getMethod(MAIN_METHOD_NAME, String[].class);
- // if that main method exists, invoke it
- if (Modifier.isStatic(stringArrayMain.getModifiers())) {
- return InvokerHelper.invokeStaticMethod(scriptClass, MAIN_METHOD_NAME, new Object[]{args});
- } else {
- Object script = InvokerHelper.invokeNoArgumentsConstructorOf(scriptClass);
- return InvokerHelper.invokeMethod(script, MAIN_METHOD_NAME, args);
- }
- } catch (NoSuchMethodException ignore) { }
- try {
- // let's find an Object main method
- Method stringArrayMain = scriptClass.getMethod(MAIN_METHOD_NAME, Object.class);
- // if that main method exists, invoke it
- if (Modifier.isStatic(stringArrayMain.getModifiers())) {
- return InvokerHelper.invokeStaticMethod(scriptClass, MAIN_METHOD_NAME, new Object[]{args});
- } else {
- Object script = InvokerHelper.invokeNoArgumentsConstructorOf(scriptClass);
- return InvokerHelper.invokeMethod(script, MAIN_METHOD_NAME, new Object[]{args});
- }
- } catch (NoSuchMethodException ignore) { }
- try {
- // let's find a no-arg main method
- Method noArgMain = scriptClass.getMethod(MAIN_METHOD_NAME);
- // if that main method exists, invoke it
- if (Modifier.isStatic(noArgMain.getModifiers())) {
- return InvokerHelper.invokeStaticNoArgumentsMethod(scriptClass, MAIN_METHOD_NAME);
- } else {
- Object script = InvokerHelper.invokeNoArgumentsConstructorOf(scriptClass);
- return InvokerHelper.invokeMethod(script, MAIN_METHOD_NAME, EMPTY_ARGS);
+ // Select main method using JEP-512 priority: static before instance, args before no-args.
+ // Uses Method.invoke() directly to avoid Groovy multimethod dispatch selecting a different overload.
+ Method selected = findMainMethod(scriptClass);
+ if (selected != null) {
+ try {
+ selected.setAccessible(true);
Review Comment:
`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`).
```suggestion
```
##########
src/main/java/org/codehaus/groovy/ast/ModuleNode.java:
##########
@@ -527,16 +530,33 @@ private MethodNode handleMainMethodIfPresent(final List<MethodNode> methods) {
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;
}
}
}
}
+
+ if (validMains.isEmpty()) return null;
+
+ // 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);
+ });
+ MethodNode result = validMains.get(0);
+
+ // Warn about unreachable main methods
+ for (int i = 1; i < validMains.size(); i++) {
+ MethodNode unreachable = validMains.get(i);
+ getContext().addWarning("Method '" + unreachable.getText()
+ + "' is not reachable from the Groovy runner"
+ + " because a higher-priority main method '"
+ + result.getText() + "' exists", unreachable);
Review Comment:
`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.
##########
src/main/java/org/codehaus/groovy/ast/ModuleNode.java:
##########
@@ -527,16 +530,33 @@ private MethodNode handleMainMethodIfPresent(final List<MethodNode> methods) {
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;
}
}
}
}
+
+ if (validMains.isEmpty()) return null;
+
+ // 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);
+ });
Review Comment:
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.
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org