[
https://issues.apache.org/jira/browse/GROOVY-11896?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18071653#comment-18071653
]
ASF GitHub Bot commented on GROOVY-11896:
-----------------------------------------
Copilot commented on code in PR #2429:
URL: https://github.com/apache/groovy/pull/2429#discussion_r3044013466
##########
src/antlr/GroovyParser.g4:
##########
@@ -118,7 +118,7 @@ packageDeclaration
;
importDeclaration
- : annotationsOpt IMPORT STATIC? qualifiedName (DOT MUL | AS
alias=identifier)?
+ : annotationsOpt IMPORT (STATIC | MODULE)? qualifiedName (DOT MUL | AS
alias=identifier)?
;
Review Comment:
`importDeclaration` allows `import module <name>.*` and `import module
<name> as alias`, but the implementation in `AstBuilder#visitImportDeclaration`
ignores `MUL`/`alias` when `MODULE` is present. This makes those forms either
silently miscompiled or ambiguously specified. Consider splitting the grammar
into a dedicated `import module` alternative that only permits a qualified
module name (no star / no alias) and rejecting unsupported combinations at
parse time.
```suggestion
: importModuleDeclaration
| annotationsOpt IMPORT STATIC? qualifiedName (DOT MUL | AS
alias=identifier)?
;
importModuleDeclaration
: annotationsOpt IMPORT MODULE qualifiedName
;
```
##########
src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java:
##########
@@ -338,9 +349,15 @@ public ImportNode visitImportDeclaration(final
ImportDeclarationContext ctx) {
List<AnnotationNode> annotations =
this.visitAnnotationsOpt(ctx.annotationsOpt());
boolean hasStatic = asBoolean(ctx.STATIC());
+ boolean hasModule = asBoolean(ctx.MODULE());
boolean hasStar = asBoolean(ctx.MUL());
boolean hasAlias = asBoolean(ctx.alias);
+ if (hasModule) {
+ String moduleName = this.visitQualifiedName(ctx.qualifiedName());
+ return expandModuleImport(moduleName, annotations, ctx);
+ }
+
Review Comment:
When `MODULE` is present, `hasStar` / `hasAlias` are currently ignored. If
those forms are meant to be illegal (recommended for alignment with the
documented module-import syntax), detect them here and throw a parsing error;
otherwise, implement consistent semantics for `import module ... .*` / `as ...`.
##########
src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java:
##########
@@ -387,6 +404,88 @@ public ImportNode visitImportDeclaration(final
ImportDeclarationContext ctx) {
return configureAST(importNode, ctx);
}
+ /**
+ * Expands {@code import module java.base} into star imports for all
+ * packages exported (unqualified) by the named module.
+ * Packages already covered by Groovy's default imports or existing
+ * star imports are skipped to avoid redundant resolution work.
+ * <p>
+ * Modules are located from system modules (JDK) first, then from
+ * modular JARs on the compilation classpath. Automatic modules
+ * (JARs with an {@code Automatic-Module-Name} manifest entry but no
+ * {@code module-info.class}) are supported — all packages in the JAR
+ * are imported since automatic modules have no explicit exports.
+ * <p>
+ * Known differences from Java's module import behavior:
+ * <ul>
+ * <li>Ambiguous class names from multiple module imports silently resolve
+ * to the last match, consistent with Groovy's existing star import
+ * semantics. Java reports a compile-time error for such
ambiguities.</li>
+ * <li>Explicit single-type imports take priority over module-expanded
+ * star imports (same as Java).</li>
+ * </ul>
+ */
+ private ImportNode expandModuleImport(final String moduleName, final
List<AnnotationNode> annotations, final ImportDeclarationContext ctx) {
+ ModuleFinder finder = ModuleFinder.compose(
+ ModuleFinder.ofSystem(), classpathModuleFinder());
+ ModuleReference moduleRef = finder.find(moduleName).orElse(null);
+ if (moduleRef == null) {
+ throw createParsingFailedException("Unknown module: " +
moduleName, ctx);
+ }
+ ModuleDescriptor descriptor = moduleRef.descriptor();
+ Set<String> skip = new HashSet<>(Arrays.asList(
+ org.codehaus.groovy.control.ResolveVisitor.DEFAULT_IMPORTS));
+
moduleNode.getStarImports().stream().map(ImportNode::getPackageName).forEach(skip::add);
+ ImportNode lastImport = null;
+ // Automatic modules have no exports — use packages() instead
+ Collection<String> packageNames = descriptor.isAutomatic()
+ ? descriptor.packages()
+ : descriptor.exports().stream()
+ .filter(e -> !e.isQualified())
+ .map(ModuleDescriptor.Exports::source)
+ .collect(Collectors.toList());
+ for (String pkg : packageNames) {
+ String packageName = pkg + DOT_STR;
+ if (!skip.contains(packageName)) {
+ moduleNode.addStarImport(packageName, annotations);
+ lastImport = last(moduleNode.getStarImports());
+ skip.add(packageName);
+ }
+ }
+ if (lastImport == null) {
+ // All exported packages were already imported — add a no-op
marker so AST is valid
+
moduleNode.addStarImport(org.codehaus.groovy.control.ResolveVisitor.DEFAULT_IMPORTS[0],
annotations);
+ lastImport = last(moduleNode.getStarImports());
+ }
+ return configureAST(lastImport, ctx);
+ }
+
+ /**
+ * Builds a {@link java.lang.module.ModuleFinder} that scans the
compilation
+ * classpath for modular JARs (those containing {@code module-info.class}).
+ * Collects paths from both the compiler configuration classpath and any
+ * URLClassLoaders in the classloader hierarchy.
+ */
+ private ModuleFinder classpathModuleFinder() {
+ Set<Path> paths = new LinkedHashSet<>();
+ for (String entry : sourceUnit.getConfiguration().getClasspath()) {
+ paths.add(Path.of(entry));
+ }
+ ClassLoader cl = sourceUnit.getClassLoader();
+ while (cl != null) {
+ if (cl instanceof URLClassLoader) {
+ for (URL url : ((URLClassLoader) cl).getURLs()) {
+ try {
+ paths.add(Path.of(url.toURI()));
+ } catch (URISyntaxException ignore) {
+ }
+ }
+ }
+ cl = cl.getParent();
+ }
+ return ModuleFinder.of(paths.toArray(Path[]::new));
+ }
Review Comment:
`classpathModuleFinder()` builds a `ModuleFinder` from every
compiler/classloader classpath entry without validating existence/type.
`ModuleFinder.of(...)` can throw `IllegalArgumentException` for
non-existent/unsupported paths, which would make `import module ...` fail
unexpectedly depending on classpath contents. Consider filtering to existing
`Path`s (jar/jmod/dir) and guarding `ModuleFinder.of(...)` with error handling
that reports a clear `ParsingFailedException`.
##########
src/test/groovy/groovy/ImportTest.groovy:
##########
@@ -163,4 +163,95 @@ final class ImportTest {
Entry entry = [foo:'bar'].entrySet().first()
'''
}
+
+ // module imports
+
+ @Test
+ void testImportModuleJavaBase() {
+ assertScript '''
+ import module java.base
+
+ // java.util
+ List list = [1, 2, 3]
+ Map map = [a: 1]
+
+ // java.io
+ File f = new File('.')
+
+ // java.net
+ URI uri = new URI('http://example.com')
+
+ // java.time
+ LocalDate date = LocalDate.now()
+
+ // java.util.concurrent
+ CountDownLatch latch = new CountDownLatch(1)
+
+ assert list.size() == 3
+ '''
+ }
+
+ @Test
+ void testImportModuleJavaSql() {
+ assertScript '''
+ import module java.sql
+
+ // java.sql package
+ assert Connection.name.size() == 19
+ assert DriverManager.name.size() == 22
Review Comment:
These assertions rely on the *length* of the class name string, which is
brittle and obscures intent. Prefer asserting the fully-qualified name (e.g.,
`Connection.name == 'java.sql.Connection'`) so the test fails only when
resolution is wrong, not when unrelated naming/formatting changes occur.
```suggestion
assert Connection.name == 'java.sql.Connection'
assert DriverManager.name == 'java.sql.DriverManager'
```
##########
src/test/groovy/groovy/ImportTest.groovy:
##########
@@ -163,4 +163,95 @@ final class ImportTest {
Entry entry = [foo:'bar'].entrySet().first()
'''
}
+
+ // module imports
+
+ @Test
+ void testImportModuleJavaBase() {
+ assertScript '''
+ import module java.base
+
+ // java.util
+ List list = [1, 2, 3]
+ Map map = [a: 1]
+
+ // java.io
+ File f = new File('.')
+
+ // java.net
+ URI uri = new URI('http://example.com')
+
+ // java.time
+ LocalDate date = LocalDate.now()
+
+ // java.util.concurrent
+ CountDownLatch latch = new CountDownLatch(1)
+
+ assert list.size() == 3
+ '''
+ }
+
+ @Test
+ void testImportModuleJavaSql() {
+ assertScript '''
+ import module java.sql
+
+ // java.sql package
+ assert Connection.name.size() == 19
+ assert DriverManager.name.size() == 22
+ '''
+ }
+
+ @Test
+ void testImportModuleFromClasspath() {
+ // Find JUnit JAR from the test classloader and add it to the compiler
classpath
+ def junitUrl = Test.class.protectionDomain.codeSource.location
+ def config = new org.codehaus.groovy.control.CompilerConfiguration()
+ config.classpathList = [new File(junitUrl.toURI()).path]
+ def loader = new GroovyClassLoader(getClass().classLoader, config)
+ def shell = new GroovyShell(loader)
+ shell.evaluate '''
+ import module org.junit.jupiter.api
+
+ // org.junit.jupiter.api package
+ assert Test.name == 'org.junit.jupiter.api.Test'
+ assert Assertions.name == 'org.junit.jupiter.api.Assertions'
+ assert DisplayName.name == 'org.junit.jupiter.api.DisplayName'
+ assert DisabledIf.name ==
'org.junit.jupiter.api.condition.DisabledIf'
+ '''
+ }
+
+ @Test
+ void testImportModuleAutomatic() {
+ // groovy-jmx is an automatic module (Automatic-Module-Name in
MANIFEST.MF)
+ def jmxJar = new
File('subprojects/groovy-jmx/build/libs').listFiles()?.find {
+ it.name.startsWith('groovy-jmx-') && it.name.endsWith('.jar') &&
!it.name.contains('-raw')
+ }
+ assert jmxJar?.exists(), 'groovy-jmx JAR not found; run ./gradlew
:groovy-jmx:jar first'
+ def config = new org.codehaus.groovy.control.CompilerConfiguration()
+ config.classpathList = [jmxJar.path]
+ def loader = new GroovyClassLoader(getClass().classLoader, config)
+ def shell = new GroovyShell(loader)
Review Comment:
This test depends on a locally-built `subprojects/groovy-jmx/build/libs` jar
and instructs running a Gradle task manually. That makes the test suite
order-/environment-dependent and likely to fail on clean/CI checkouts. Consider
wiring the jar creation as part of the test build (declared task dependency) or
locating the needed artifact from the test runtime classpath instead of the
filesystem.
##########
src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java:
##########
@@ -387,6 +404,88 @@ public ImportNode visitImportDeclaration(final
ImportDeclarationContext ctx) {
return configureAST(importNode, ctx);
}
+ /**
+ * Expands {@code import module java.base} into star imports for all
+ * packages exported (unqualified) by the named module.
+ * Packages already covered by Groovy's default imports or existing
+ * star imports are skipped to avoid redundant resolution work.
+ * <p>
+ * Modules are located from system modules (JDK) first, then from
+ * modular JARs on the compilation classpath. Automatic modules
+ * (JARs with an {@code Automatic-Module-Name} manifest entry but no
+ * {@code module-info.class}) are supported — all packages in the JAR
+ * are imported since automatic modules have no explicit exports.
+ * <p>
+ * Known differences from Java's module import behavior:
+ * <ul>
+ * <li>Ambiguous class names from multiple module imports silently resolve
+ * to the last match, consistent with Groovy's existing star import
+ * semantics. Java reports a compile-time error for such
ambiguities.</li>
+ * <li>Explicit single-type imports take priority over module-expanded
+ * star imports (same as Java).</li>
+ * </ul>
+ */
+ private ImportNode expandModuleImport(final String moduleName, final
List<AnnotationNode> annotations, final ImportDeclarationContext ctx) {
+ ModuleFinder finder = ModuleFinder.compose(
+ ModuleFinder.ofSystem(), classpathModuleFinder());
+ ModuleReference moduleRef = finder.find(moduleName).orElse(null);
+ if (moduleRef == null) {
+ throw createParsingFailedException("Unknown module: " +
moduleName, ctx);
+ }
+ ModuleDescriptor descriptor = moduleRef.descriptor();
+ Set<String> skip = new HashSet<>(Arrays.asList(
+ org.codehaus.groovy.control.ResolveVisitor.DEFAULT_IMPORTS));
+
moduleNode.getStarImports().stream().map(ImportNode::getPackageName).forEach(skip::add);
+ ImportNode lastImport = null;
+ // Automatic modules have no exports — use packages() instead
+ Collection<String> packageNames = descriptor.isAutomatic()
+ ? descriptor.packages()
+ : descriptor.exports().stream()
+ .filter(e -> !e.isQualified())
+ .map(ModuleDescriptor.Exports::source)
+ .collect(Collectors.toList());
+ for (String pkg : packageNames) {
+ String packageName = pkg + DOT_STR;
+ if (!skip.contains(packageName)) {
+ moduleNode.addStarImport(packageName, annotations);
+ lastImport = last(moduleNode.getStarImports());
+ skip.add(packageName);
+ }
+ }
+ if (lastImport == null) {
+ // All exported packages were already imported — add a no-op
marker so AST is valid
+
moduleNode.addStarImport(org.codehaus.groovy.control.ResolveVisitor.DEFAULT_IMPORTS[0],
annotations);
+ lastImport = last(moduleNode.getStarImports());
Review Comment:
The "no-op marker" branch adds an extra star import (`DEFAULT_IMPORTS[0]`)
even when the module import expands to no new packages. This is not a true
no-op: it mutates `moduleNode.getStarImports()` ordering and can affect
ambiguous-type resolution (Groovy resolves ambiguities by last match when FQNs
differ). Prefer returning `null` (since imports are applied via side effects)
or returning an `ImportNode` without adding any additional star import.
```suggestion
// All exported packages were already imported — no AST node
needs to be added
return null;
```
> Support module import declarations
> ----------------------------------
>
> Key: GROOVY-11896
> URL: https://issues.apache.org/jira/browse/GROOVY-11896
> Project: Groovy
> Issue Type: New Feature
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> In Java, _module import declarations_ allow you to import all exported
> packages of a module with a single statement. It was introduced as a preview
> feature in Java 23 ([JEP 476|https://openjdk.org/jeps/476]) and finalized in
> Java 25 ([JEP 511|https://openjdk.org/jeps/511]).
> The syntax is as follows:
> {code:java}
> import module java.base; // Java
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
