[
https://issues.apache.org/jira/browse/GROOVY-11737?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18071914#comment-18071914
]
ASF GitHub Bot commented on GROOVY-11737:
-----------------------------------------
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.
> Improve clarity of Groovy main method selection priority
> --------------------------------------------------------
>
> Key: GROOVY-11737
> URL: https://issues.apache.org/jira/browse/GROOVY-11737
> Project: Groovy
> Issue Type: Question
> Reporter: Georgii Ustinov
> Assignee: Paul King
> Priority: Major
>
> Good afternoon
>
> I am currently trying to support instance main methods inside IntelliJ IDEA
> groovy plugin as a part of Groovy 5 release, however, I can’t find any
> specification of how it works.
>
> I have found that the feature was implemented here -
> [https://github.com/apache/groovy/pull/1910/files].
>
> Looking into the source code -
> [https://github.com/paulk-asert/groovy/blob/90480bfade8c20f935311055e2ceb85288870605/src/main/java/groovy/lang/GroovyShell.java#L275]
> I can see the following order:
>
> 1) static void main(String[] args);
> 2) void main(String[] args);
> 3) static void main(args);
> 4) void main(args);
> 5) static void main();
> 6) void main();
>
> The problem is that it doesn’t correspond to the reality.
>
> Let’s consider the following code main.groovy
>
>
> {code:java}
> void main(args) {
> println "1"
> }
>
> static void main() {
> println "2"
> }
> {code}
>
>
> If I run
> {code:java}
> groovy main.groovy{code}
> 1 will be printed (and it looks correct according to the semantics),
> But if I change example:
>
>
> {code:java}
> void main(String[] args) {
> println "1"
> }
>
> static void main() {
> println "2"
> }
> {code}
> 2 will be printed (and thus the order in the code is violated). But if I run
> {code:java}
> groovy main.groovy someArg{code}
> , then 1 will be printed
>
> Could you, please, provide the correct main method order resolution?
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
