review: feat: add support for module imports #6586
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for Java module imports (e.g., import module java.base;) to the Spoon framework. Module imports are a Java 9+ feature that allows importing entire modules.
Key changes:
- Added
MODULEenum value toCtImportKindto represent module imports - Extended
CtImportVisitorinterface withvisitModuleImportmethod to handle module import traversal - Implemented module import parsing in
JDTImportBuilderto detect and process module imports from source code - Updated all visitor implementations and switch statements to handle the new MODULE import kind
- Added module import support to the pretty printer to output module imports correctly
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/resources/imports/ModuleImport.java | Test resource demonstrating module import usage (missing necessary type imports) |
| src/test/java/spoon/test/imports/ImportTest.java | Added test case for module imports and fixed trailing whitespace |
| src/test/java/spoon/reflect/visitor/DefaultJavaPrettyPrinterTest.java | Added test to verify module imports are correctly pretty-printed |
| src/main/java/spoon/support/reflect/declaration/CtImportImpl.java | Updated to recognize MODULE import kind and migrated switch to expression syntax |
| src/main/java/spoon/support/compiler/jdt/JDTImportBuilder.java | Added logic to parse and create module imports from JDT AST |
| src/main/java/spoon/reflect/visitor/TypeNameScope.java | Added empty visitModuleImport implementation (modules don't affect type resolution) |
| src/main/java/spoon/reflect/visitor/ImportCleaner.java | Ensures module imports are never removed during cleanup |
| src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java | Added visitModuleImport to print module imports correctly |
| src/main/java/spoon/reflect/visitor/CtImportVisitor.java | Extended interface with visitModuleImport method |
| src/main/java/spoon/reflect/visitor/CtAbstractImportVisitor.java | Added empty default implementation of visitModuleImport |
| src/main/java/spoon/reflect/declaration/CtImportKind.java | Added MODULE enum value for module imports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/java/spoon/support/reflect/declaration/CtImportImpl.java
Outdated
Show resolved
Hide resolved
| */ | ||
| package spoon.support.compiler.jdt; | ||
|
|
||
| import org.eclipse.jdt.internal.compiler.ast.CompilationUnitDeclaration; |
There was a problem hiding this comment.
The import for StringUtils is added but not used anywhere in the file. This unused import should be removed.
| import module java.base; | ||
|
|
||
| public class ModuleImport { | ||
| public static void main(String[] args) { | ||
| String[] fruits = new String[] { "apple", "berry", "citrus" }; | ||
| Map<String, String> m = Stream | ||
| .of(fruits) | ||
| .collect(Collectors.toMap( | ||
| s -> s.toUpperCase().substring(0,1), | ||
| Function.identity())); | ||
| m.forEach((k, v) -> | ||
| System.out.println(k + " " + v)); | ||
| } | ||
| } |
There was a problem hiding this comment.
The test file is missing necessary import statements for Map, Stream, Collectors, and Function. These types are used in the code but not imported. This will cause compilation errors. The missing imports should be added after the module import statement.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
MartinWitt
changed the title
| if (oldImport.getImportKind() == CtImportKind.MODULE) { | ||
| //never remove module imports | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I assume this is a conservative solution, i.e., removing such an import would be legal/might make sense in some cases (e.g., if we're in the same module?).
In that case, I think the comment should reflect that a bit better, e.g., "be conservative: do not remove any module imports". This should help if anyone ever tries to do more here.
| } | ||
|
|
||
| @ModelTest(value = {"src/test/resources/imports/ModuleImport.java"}, autoImport = true, complianceLevel = 25) | ||
| void moduleImportsArePrinted(@BySimpleName("ModuleImport") CtType<?> type, Launcher launcher) { |
There was a problem hiding this comment.
I already forgot about the @BySimpleName annotation again, but it definitely makes the test cleaner. Can you add a // contract: comment though?
Co-authored-by: Hannes Greule <[email protected]>
|
Hi @MartinWitt Great. Just wondering: no |
needed for java25