GROOVY-11896: Support module import declarations

[paulk-asert]

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

[codecov-commenter]

Codecov Report

❌ Patch coverage is 74.46809% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.4922%. Comparing base (2c25b58) to head (174e2f2).

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

Additional details and impacted files

@@                Coverage Diff                 @@
##               master      #2429        +/-   ##
==================================================
+ Coverage     66.4890%   66.4922%   +0.0032%     
- Complexity      30230      30244        +14     
==================================================
  Files            1408       1408                
  Lines          117815     117862        +47     
  Branches        20934      20946        +12     
==================================================
+ Hits            78334      78369        +35     
- Misses          33035      33043         +8     
- Partials         6446       6450         +4     

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

... and 4 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.

[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.