GROOVY-11896: Support module import declarations

[paulk-asert]

see https://issues.apache.org/jira/browse/GROOVY-11896

[codecov-commenter]

Codecov Report

❌ Patch coverage is 77.19298% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.6274%. Comparing base (2c25b58) to head (dcb3ecb).
⚠️ Report is 58 commits behind head on master.

Files with missing lines	Patch %	Lines
...va/org/apache/groovy/parser/antlr4/AstBuilder.java	77.1930%	9 Missing and 4 partials ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##               master      #2429        +/-   ##
==================================================
+ Coverage     66.4890%   66.6274%   +0.1384%     
- Complexity      30230      30773       +543     
==================================================
  Files            1408       1435        +27     
  Lines          117815     119676      +1861     
  Branches        20934      21197       +263     
==================================================
+ Hits            78334      79737      +1403     
- Misses          33035      33367       +332     
- Partials         6446       6572       +126     

Files with missing lines	Coverage Δ
...va/org/apache/groovy/parser/antlr4/AstBuilder.java	86.3208% <77.1930%> (-0.3129%)	⬇️

... and 69 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[daniellansun]

It's better to verify if current implementation supports --add-opens.

Here are some discussions about module discovery functionality:
https://mail.openjdk.org/pipermail/jigsaw-dev/2017-December/013439.html

[eric-milles]

I think Java's semantic is all public classes. Adding all these star imports includes private types as well.

[paulk-asert]

Yes, existing star imports includes package-private while Java doesn't. I created GROOVY-11916 in case we want to change that. I think it is okay to follow existing Groovy behavior and change it in GROOVY-11916 if we don't like that. I am not sure we document that difference for star imports. But we should do for both the same way. I'll await thoughts on GROOVY-11916 before making changes here.

[jonnybot0]

I could see it getting vexing if module imports and star imports have conflicts around one module's package-private class beating out another one's public class. That would violate the principle of least surprise, I think. It's hard to think of a case where implementing https://issues.apache.org/jira/browse/GROOVY-11916 would trip somebody up, since actual construction or use of the classes would throw an exception anyway.

I'd support merging this and moving on GROOVY-11916 separately, though, if we're trying to avoid that becoming a blocker.

[paulk-asert]

@jonnybot0 I agree. Public trumps package private from my testing if I tested all cases.

[eric-milles]

Does the original import stay in the AST? If not, then there is nothing left for code navigation support in eclipse. Although, I'm not 100% what sort of nav is possible for a module name.

[paulk-asert]

Does the original import stay in the AST? If not, then there is nothing left for code navigation support in eclipse. Although, I'm not 100% what sort of nav is possible for a module name.

This doesn't create some kind of (new) ModuleImportNode AST type but rather replaces with one or more star imports. We could track via node metadata as some kind of alternative to a new AST type. But as you say, is there a need?

[jonnybot0]

Does the original import stay in the AST? If not, then there is nothing left for code navigation support in eclipse. Although, I'm not 100% what sort of nav is possible for a module name.

This doesn't create some kind of (new) ModuleImportNode AST type but rather replaces with one or more star imports. We could track via node metadata as some kind of alternative to a new AST type. But as you say, is there a need?

For module navigation, I can see that IntelliJ just takes me to module-info.java if I use the Go-To Declaration feature for the import statement import module java.base. So navigation is certainly possible from a module name and it does refer to something in particular that a static star wouldn't. Outside of code editors, I can't think of a particular use case for that metadata.

[jonnybot0]

Overall, I agree that following Java's example seems more prudent than Scala's.

I suspect that treating module imports as a distinct thing from static star imports, at least at some level in the AST, might be prudent. That could be because I never fully recovered from the psychic damage I took in the OSGi salt mines. 🙂

That caveat aside, it does seem like there's something from the module imports JEP that we're not supporting: Transitive dependencies across modules (as described in JEP 476).

The ability to import at the level of modules would be especially helpful when APIs in one module have a close relationship with APIs in another module. This is common in large multi-module libraries such as the JDK. For example, the java.sql module provides database access via its java.sql and javax.sql packages, but one of its interfaces, java.sql.SQLXML, declares public methods whose signatures use interfaces from the javax.xml.transform package in the java.xml module. Developers who call these methods in java.sql.SQLXML typically import both the java.sql package and the javax.xml.transform package. To facilitate this extra import, the java.sql module depends on the java.xml module transitively, so that a program which depends on the java.sql module depends automatically on the java.xml module.

Unless I have made an error, the existing branch does not do this. I've added a test to my fork (see 301582f) that demonstrates the issue.

This is part of what makes me think we may well need specific semantics for the AST of module imports. There are additional expectations built into the JEP that make me think we'd need some specific things to handle them properly.

[paulk-asert]

Overall, I agree that following Java's example seems more prudent than Scala's.

I suspect that treating module imports as a distinct thing from static star imports, at least at some level in the AST, might be prudent. That could be because I never fully recovered from the psychic damage I took in the OSGi salt mines. 🙂

That caveat aside, it does seem like there's something from the module imports JEP that we're not supporting: Transitive dependencies across modules (as described in JEP 476).

The ability to import at the level of modules would be especially helpful when APIs in one module have a close relationship with APIs in another module. This is common in large multi-module libraries such as the JDK. For example, the java.sql module provides database access via its java.sql and javax.sql packages, but one of its interfaces, java.sql.SQLXML, declares public methods whose signatures use interfaces from the javax.xml.transform package in the java.xml module. Developers who call these methods in java.sql.SQLXML typically import both the java.sql package and the javax.xml.transform package. To facilitate this extra import, the java.sql module depends on the java.xml module transitively, so that a program which depends on the java.sql module depends automatically on the java.xml module.

Unless I have made an error, the existing branch does not do this. I've added a test to my fork (see 301582f) that demonstrates the issue.

This is part of what makes me think we may well need specific semantics for the AST of module imports. There are additional expectations built into the JEP that make me think we'd need some specific things to handle them properly.

Nice catch @jonnybot0! I updated the PR to include this ability.

[jonnybot0]

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, 494, 511) and the Java Language Specification for Module Import Declarations. 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 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!