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;
```
--
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]
