[
https://issues.apache.org/jira/browse/GROOVY-11896?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18074027#comment-18074027
]
ASF GitHub Bot commented on GROOVY-11896:
-----------------------------------------
Copilot commented on code in PR #2429:
URL: https://github.com/apache/groovy/pull/2429#discussion_r3094317864
##########
src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java:
##########
@@ -388,6 +406,124 @@ 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 and, recursively,
+ * by any modules it {@code requires transitive} (per JEP 476).
+ * 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>
+ * As per JLS 6.4.1, explicit single-type imports and type-on-demand (star)
+ * imports take priority over module-expanded imports. Ambiguous class
names
+ * from multiple module imports produce a compile-time error (JLS 7.5.5).
+ * <p>
+ * Known difference from Java: Groovy's star imports (including
module-expanded
+ * ones) can resolve package-private types, whereas Java only exposes
public
+ * types from exported packages (see GROOVY-11916).
+ */
+ 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);
+ }
+ Set<String> skip = new HashSet<>(Arrays.asList(
+ org.codehaus.groovy.control.ResolveVisitor.DEFAULT_IMPORTS));
+
moduleNode.getStarImports().stream().map(ImportNode::getPackageName).forEach(skip::add);
+
moduleNode.getModuleStarImports().stream().map(ImportNode::getPackageName).forEach(skip::add);
+ ImportNode lastImport = null;
+ // Collect exported packages from this module and its transitive
dependencies
+ Set<String> visited = new HashSet<>();
+ List<String> packageNames = new ArrayList<>();
+ collectModuleExports(moduleRef, finder, packageNames, visited);
+ for (String pkg : packageNames) {
+ String packageName = pkg + DOT_STR;
+ if (!skip.contains(packageName)) {
+ // Separate list so resolution can apply JLS 6.4.1 shadowing:
+ // a user-written `import foo.*` beats a module-expanded
`foo.*`.
+ moduleNode.addModuleStarImport(packageName, annotations);
+ lastImport = last(moduleNode.getModuleStarImports());
+ skip.add(packageName);
+ }
+ }
+ if (lastImport == null) {
+ // All exported packages were already covered by existing imports
+ List<ImportNode> existing = moduleNode.getModuleStarImports();
+ lastImport = !existing.isEmpty() ? last(existing) :
last(moduleNode.getStarImports());
Review Comment:
`expandModuleImport` can end up with `lastImport == null` when all exported
packages are skipped (e.g., they are already covered by default imports /
existing star imports). In that case `last(existing)` /
`last(moduleNode.getStarImports())` can throw on an empty list, or
`configureAST(lastImport, ctx)` will NPE. Ensure a non-null `ImportNode` is
always returned/configured even when the expansion adds no new imports.
```suggestion
if (!existing.isEmpty()) {
lastImport = last(existing);
} else if (!moduleNode.getStarImports().isEmpty()) {
lastImport = last(moduleNode.getStarImports());
} else {
String packageName = !packageNames.isEmpty() ?
packageNames.get(0) + DOT_STR : moduleName + DOT_STR;
lastImport = new ImportNode(packageName);
}
```
##########
src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java:
##########
@@ -137,18 +137,31 @@
import java.io.BufferedReader;
import java.io.IOException;
import java.lang.annotation.Annotation;
+import java.lang.module.FindException;
+import java.lang.module.ModuleDescriptor;
+import java.lang.module.ModuleFinder;
+import java.lang.module.ModuleReference;
+import java.net.URISyntaxException;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.nio.file.Files;
Review Comment:
`Files` is imported but the implementation uses
`java.nio.file.Files.exists(...)` (fully-qualified), leaving this import unused
and causing a Java compilation error. Use the imported `Files` (i.e.,
`Files.exists(...)`) or remove the import.
```suggestion
```
##########
subprojects/groovy-jmx/build.gradle:
##########
@@ -32,3 +32,8 @@ dependencies {
testRuntimeOnly projects.groovyGrapeIvy
testRuntimeOnly projects.groovySwing
}
+
+tasks.named('test') {
+ dependsOn 'jarjar'
+ systemProperty 'groovy.jmx.jar', tasks.named('jarjar').flatMap {
it.outputFile }.get().asFile.path
Review Comment:
The `systemProperty` value forces provider resolution at configuration time
via `.get()`, which can break task avoidance/configuration cache and eagerly
realizes `jarjar`. Prefer keeping it lazy (e.g., pass a Provider-mapped value)
so the path is only queried at execution time.
```suggestion
systemProperty 'groovy.jmx.jar', tasks.named('jarjar').flatMap {
it.outputFile }.map { it.asFile.path }
```
##########
src/main/java/org/codehaus/groovy/control/customizers/ImportCustomizer.java:
##########
@@ -68,8 +83,82 @@ public void call(final SourceUnit source, final
GeneratorContext context, final
case star:
ast.addStarImport(anImport.star);
break;
+ case moduleImport:
+ expandModuleImport(source, ast, anImport.star);
+ break;
+ }
+ }
+ }
+
+ private static void expandModuleImport(final SourceUnit source, final
ModuleNode ast, final String moduleName) {
+ ModuleFinder finder = ModuleFinder.compose(
+ ModuleFinder.ofSystem(), classpathModuleFinder(source));
+ ModuleReference moduleRef = finder.find(moduleName).orElse(null);
+ if (moduleRef == null) {
+ throw new IllegalArgumentException("Unknown module: " +
moduleName);
+ }
+ Set<String> skip = new
HashSet<>(Arrays.asList(ResolveVisitor.DEFAULT_IMPORTS));
+ ast.getStarImports().stream().map(n ->
n.getPackageName()).forEach(skip::add);
+ Set<String> visited = new HashSet<>();
+ List<String> packageNames = new ArrayList<>();
+ collectModuleExports(moduleRef, finder, packageNames, visited);
+ for (String pkg : packageNames) {
+ String packageName = pkg + ".";
+ if (!skip.contains(packageName)) {
+ ast.addStarImport(packageName);
Review Comment:
`ImportCustomizer` expands module imports into
`ModuleNode.addStarImport(...)`, which gives them the same precedence as
user-written `import foo.*`. Per the new `moduleStarImports` tiering (JLS 6.4.1
shadowing), these should be added via `addModuleStarImport(...)` so user star
imports can shadow module-expanded imports.
```suggestion
ast.addModuleStarImport(packageName);
```
##########
subprojects/groovy-groovysh/src/main/groovy/org/apache/groovy/groovysh/jline/GroovyEngine.java:
##########
@@ -407,7 +407,12 @@ public String getBuffer() {
@Override
public Object execute(String statement) throws Exception {
Object out = null;
- if
(statement.matches("import\\s+(static\\s+)?(([^;\\s])+)(?:\\s+as\\s+(" +
BASE_REGEX_VAR + "))?\\s*(;)?")) {
+ if (statement.matches("import\\s+module\\s+([^;\\s]+)\\s*(;)?")) {
+ String[] p = statement.split("\\s+");
+ String moduleName = p[2].replaceAll(";", "");
+ executeStatement(shell, snippets, EnumSet.of(SnippetType.IMPORT),
statement);
+ imports.put("module " + moduleName, addSnippet(SnippetType.IMPORT,
statement));
+ } else if
(statement.matches("import\\s+(static\\s+)?(([^;\\s])+)(?:\\s+as\\s+(" +
BASE_REGEX_VAR + "))?\\s*(;)?")) {
Review Comment:
Groovysh tracks imported types for completion via `imports`/`nameClass` (see
`refreshNameClass()`/`addToNameClass()`), but the new `import module ...`
branch stores the key as `"module <name>"` and does not populate `nameClass`.
As a result, method/property completion for module-imported simple names (e.g.
`Connection.`) won’t work, and `removeImport` will treat the key like a class
name. Consider expanding the module into package star imports for the
completion cache (or teaching `addToNameClass`/`removeImport` to handle `module
...`).
##########
src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java:
##########
@@ -137,18 +137,31 @@
import java.io.BufferedReader;
import java.io.IOException;
import java.lang.annotation.Annotation;
+import java.lang.module.FindException;
Review Comment:
`FindException` is imported but the code catches
`java.lang.module.FindException` with a fully-qualified name, which makes this
import unused and will fail Java compilation. Either remove the import or use
the unqualified `FindException` in the catch clause.
```suggestion
```
> 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}
> h3. Implementation highlights
> * Grammar extended with a dedicated import module <qualifiedName>
> alternative (no .* or as alias — rejected at parse time)
>
>
> * Modules resolved via ModuleFinder.compose(ofSystem(),
> classpathModuleFinder()) — system/JDK modules first, then modular JARs on the
> compilation classpath
>
> * Explicit JPMS modules (with module-info.class) import all unqualified
> exports; automatic modules (with Automatic-Module-Name manifest entry) import
> all packages
>
> * Default imports and existing star imports are skipped to avoid redundant
> resolution
>
>
> * module remains usable as an identifier (context-sensitive keyword, like
> record/sealed)
>
>
> Passing test scenarios:
> {code}
>
>
>
> // System module — JDK classes available without individual imports
> import module java.base
>
>
>
> LocalDate date = LocalDate.now()
> CountDownLatch latch = new CountDownLatch(1)
>
>
>
>
> // System module — java.sql
>
>
>
> import module java.sql
> assert Connection.name == 'java.sql.Connection'
>
>
>
> assert DriverManager.name == 'java.sql.DriverManager'
>
>
>
>
> // JPMS module from classpath (module-info.class)
>
>
>
> import module org.junit.jupiter.api
>
>
>
> assert Test.name == 'org.junit.jupiter.api.Test'
>
>
>
> assert DisabledIf.name == 'org.junit.jupiter.api.condition.DisabledIf'
>
>
>
>
> // Automatic module from classpath (Automatic-Module-Name in MANIFEST.MF)
>
>
>
> import module org.apache.groovy.jmx
>
>
>
> assert GroovyMBean.name == 'groovy.jmx.GroovyMBean'
>
>
>
>
>
>
>
> // Unknown module — compile error
> import module no.such.module // fails: "Unknown module: no.such.module"
>
>
>
>
>
>
>
> // Star/alias not permitted with module imports — parse error
>
>
>
> import module java.base.* // fails
>
>
>
> import module java.base as jb // fails
>
>
>
>
>
>
>
> // 'module' still works as an identifier
> def module = 'hello'
>
>
>
> assert module.toUpperCase() == 'HELLO'
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
