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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
