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