RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v8]

[Jim Laskey]

On Fri, 5 May 2023 09:52:37 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Jim Laskey has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Anonymous main classes renamed to unnamed classes
>>  - Add test
>
> src/java.base/share/classes/sun/launcher/LauncherHelper.java line 872:
> 
>> 870:         Method mainMethod = null;
>> 871:         try {
>> 872:             mainMethod = MainMethodFinder.findMainMethod(mainClass);
> 
> The `MainMethodFinder.findMainMethod(...)` throws a `NoSuchMethodException` when it can't find the regular `public static void main(String[])` or, when preview is enabled, any other eligible main methods. That will now mean that the next line here which catches a `NoSuchMethodException` can potentially end up aborting with a (very confusing) error message about JavaFX application. We should perhaps change that catch block to something like:
> 
> } catch (NoSuchMethodException nsme) {
>    if (!PreviewFeatures.isEnabled()) {
>                   // invalid main or not FX application, abort with an error
>             abort(null, "java.launcher.cls.error4", mainClass.getName(),
>                   JAVAFX_APPLICATION_CLASS_NAME);
>    } else {
>        // abort with something more clear error?
>    }

Not convinced that anything has changed or that the message is confusing.

> src/java.base/share/classes/sun/launcher/LauncherHelper.java line 893:
> 
>> 891:          * findMainMethod (above) will choose the correct method, based
>> 892:          * on its name and parameter type, however, we still have to
>> 893:          * ensure that the method is static (non-preview) and returns a void.
> 
> I think it's probably going to be easier to read and maintain if we moved these checks for `static` and `void` return type into the `MainMethodFinder.findMainMethod(...)` itself. 
> What I mean is once we return from the `findMainMethod(...)`, I think the callers, like this code here, shouldn't have to do additional checks to know if this main method is valid and can be used.

I agree, but from my early commentary we have to hold off until later.

> src/jdk.compiler/share/classes/com/sun/tools/javac/launcher/Main.java line 419:
> 
>> 417:     private static boolean isStatic(Method method) {
>> 418:         return method != null && Modifier.isStatic(method.getModifiers());
>> 419:     }
> 
> It looks like these new methods aren't being used.

Remnants - removing

> src/jdk.compiler/share/classes/com/sun/tools/javac/launcher/Main.java line 440:
> 
>> 438:             int mods = mainMethod.getModifiers();
>> 439:             boolean isStatic = Modifier.isStatic(mods);
>> 440:             boolean isPublic = Modifier.isStatic(mods);
> 
> This looks like a typo. This should have been `Modifier.isPublic(mods);`

Changing

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1186258290
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1186259446
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1186261107
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1186259830