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

[Alan Bateman]

On Thu, 11 May 2023 11:25:51 GMT, Jim Laskey <jlaskey at openjdk.org> wrote:

>> Add flexible main methods and anonymous main classes to the Java language.
>
> Jim Laskey has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update VirtualParser.java

src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 35:

> 33: public class MainMethodFinder {
> 34:     private static boolean isPrivate(Method method) {
> 35:         return method != null && Modifier.isPrivate(method.getModifiers());

Are you sure you want to allow null here? It seems like it's a bug in the caller if that happens.

src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 38:

> 36:     }
> 37: 
> 38:     private static boolean isPublic(Method method) {

Is this left over from a previous iteration, it doesn't seem to be used.

src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 53:

> 51: 
> 52:     /**
> 53:      * Gather all the "main" methods in the class heirarchy.

heirarchy -> hierarchy

src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 134:

> 132: 
> 133:     /**
> 134:      * {@return priority main method or null if none found}

"or null if none found", is that out of date?

src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 146:

> 144: 
> 145:             if (Modifier.isStatic(mods) && mainMethod.getDeclaringClass() != mainClass) {
> 146:                 System.err.println("WARNING: static main in super class will be deprecated.");

I thought that JEP 445 was deprecating this, in which case the text should be "is deprecated" rather than "will be".

src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 156:

> 154: 
> 155:             List<Method> mains = new ArrayList<>();
> 156:             gatherMains(mainClass, mainClass, mains);

Instead of gatherMains, did you consider first looking for static main(String[], then static main()? Asking because I expected to only see the walk up the hierarchy when looking for an instance main.

src/java.base/share/classes/jdk/internal/misc/MainMethodFinder.java line 162:

> 160:             }
> 161: 
> 162:             if (1 < mains.size()) {

Checking if mains.size() > 1 might be easier on the eyes.

src/java.base/share/classes/sun/launcher/LauncherHelper.java line 872:

> 870: 
> 871:     // Check the existence and signature of main and abort if incorrect
> 872:     public static void validateMainClass(Class<?> mainClass) {

Is there a reason that this is changed to public, maybe left over from a previous iteration?

src/java.base/share/classes/sun/launcher/LauncherHelper.java line 896:

> 894:          * findMainMethod (above) will choose the correct method, based
> 895:          * on its name and parameter type, however, we still have to
> 896:          * ensure that the method is static (non-preview) and returns a void.

Have you looked into findMainMethod checking the return type? Right now, we have findMainMethod returning a Method that needs further checking.

src/java.base/share/classes/sun/launcher/LauncherHelper.java line 904:

> 902: 
> 903:         if (!PreviewFeatures.isEnabled()) {
> 904:             if (!isStatic || !isPublic || noArgs) {

You can use && here and avoid the nested if.

test/jdk/tools/launcher/InstanceMainTest.java line 31:

> 29:  * @run main InstanceMainTest
> 30:  */
> 31: public class InstanceMainTest extends TestHelper {

Are you planning to add tests for the selection/precedence order?

Also wondering if we need a test to check that there is a warning when the main method in found in the super class.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1193379090
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1193378248
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1193379468
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1193415367
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1193385008
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1193440438
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1193419331
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1193444639
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1193455653
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1193443762
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1193390566