jonnybot0 commented on PR #2429: URL: https://github.com/apache/groovy/pull/2429#issuecomment-4255122812
I think this is good to merge, but I have found some other things we might want to fix-forward. I'm going to note them here, but can open up another PR, as they may be worth discussing separately and I don't want to be a blocker. I've combed over this by doing some comparisons with the relevant JEPs ([476](https://openjdk.org/jeps/476), [494](https://openjdk.org/jeps/494), [511](https://openjdk.org/jeps/511)) and the [Java Language Specification for Module Import Declarations](https://docs.oracle.com/en/java/javase/24/docs/specs/module-import-declarations-jls.html). Besides importing the package-private stuff (which is fine and appropriately backlogged), there are a handful of other differences with how Java handles module imports. # Issue 1: Ambiguous Imports The most significant difference is how this would handle ambiguity between module imports. In Java, this is a compile-time error, but Groovy will just pick one silently and run with it. There are a few specific examples of this in the Java specification (7.5.5, https://docs.oracle.com/javase/specs/jls/se25/html/jls-7.html#jls-7.5.5). You can reproduce these in jshell: ``` import module java.base; import module java.desktop; List l; // Error - Ambiguous name! ``` Granted, we're already more chill about static star imports than Java, so maybe *not* throwing that error is the consistent thing? But given that module support is new, I think we could go the more "conservative" way and be okay, since no one is going to depend on the more lax behavior. # Issue 2: Import priority The Java Language specification specifies a priority for imports of single-type > type-on-demand > module-import. See [6.4.1](https://docs.oracle.com/javase/specs/jls/se25/html/jls-6.html#jls-6.4.1) of the spec. On this branch, if a user writes import foo.* and also import module M where M exports foo, the list ordering decides which import wins, not the specification precedence. # Addressing the issues There's a couple of ways Claude & I could hack up that would address these two problems. The first is a smaller change: simply make a separate list of module imports, then resolve them later. https://github.com/jonnybot0/groovy/pull/new/groovy11896-ammendments The second is a bigger departure from this branch, and actually represents the module imports within the AST. https://github.com/jonnybot0/groovy/pull/new/groovy11896-bigger-refactor Both approaches solve these two problems (as shown by having the same passing test in their diff). I suspect that the second path is a better direction to head. It looks like it would better support other aspects of and expansions to the JPMS down the road. That said, it's a more significant departure, and may have more knock-on effects; there's a lot of code using `isStar() || isStatic()` that might want auditing. Also, it should probably be benchmarked, as I suspect it could perform worse. # Way Forward I think the best approach is probably to merge this branch, then open a PR from my second branch and take feedback on that separately. That said, it might be quickest for @paulk-asert to just take the diff from the first branch, incorporate it in, and then do a PR from something like the second branch at our leisure, while still pocketing the win today. Let me know what you think is best. Thanks for giving a generous period for feedback! -- 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]
