Partial handling of module imports - Organize and Add imports #8827

sid-srini · 2025-09-15T05:35:31Z

Added further handling and support for module imports, especially with regards to OrganizeImports.

Enhanced support for module imports in java.source/CasualDiff to allow re-ordering of imports.
Enhanced java.hints/OrganizeImports to support the presence of module imports in checking for usage, and, transferring the module import scope. Also included sort comparison for module imports, adding them as the last group.
- This avoids removal of module imports from java source files, especially due if the setting is enabled to organize imports on save.
Enhanced java.source/GeneratorUtilities.addImports() to support the writing of existing module imports in the compilation unit, as well as, the addition of module imports if needed in the future by any hint/completion. This includes:
- checking for the usage of a module import.
- checking for simple name clashes in the consolidated import scope which includes module imports now, for adding as named imports.
- sorting comparison for module imports (added as the last group).

^Add meaningful description above

Click to collapse/expand PR instructions

By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -

are all your own work, and you have the right to contribute them.
are contributed solely under the terms and conditions of the Apache License 2.0 (see section 5 of the license for more information).

Please make sure (eg. git log) that all commits have a valid name and email address for you in the Author field.

If you're a first time contributor, see the Contributing guidelines for more information.

If you're a committer, please label the PR before pressing "Create pull request" so that the right test jobs can run.

PR approval and merge checklist:

Was this PR correctly labeled, did the right tests run? When did they run?
Is this PR squashed?
Are author name / email address correct? Are co-authors correctly listed? Do the commit messages need updates?
Does the PR title and description still fit after the Nth iteration? Is the description sufficient to appear in the release notes?

If this PR targets the delivery branch: don't merge. (full wiki article)

lahodaj

Thanks for doing this. I've added some comments inline.

lahodaj · 2025-09-15T06:11:15Z

java/java.source.base/src/org/netbeans/api/java/source/GeneratorUtilities.java

+        // Add modulesToImport to the importScope, to check name clashes, if any
+        if (!modulesToImport.isEmpty()) {
+            for (ModuleElement m : modulesToImport) {
+                for (Element e : m.getEnclosedElements()) {


This should probably use ElementUtilities.transitivelyExportedPackages, which is intended to mirror the import module semantics.

Thank you. I have tried to update with using this.

lahodaj · 2025-09-15T06:17:13Z

java/java.hints/src/org/netbeans/modules/java/hints/OrganizeImports.java

        CompilationUnitTree cut = maker.CompilationUnit(cu.getPackageAnnotations(), cu.getPackageName(), imps, cu.getTypeDecls(), cu.getSourceFile());
        ((JCCompilationUnit)cut).packge = ((JCCompilationUnit)cu).packge;
-        if (starImports != null || staticStarImports != null) {
+        if (!starImports.isEmpty() || !staticStarImports.isEmpty()) {


Sorry, but this is changes semantics. Before, given startImports was never null, the scope was always set. Now, it may or may not be set, and that's possibly problematic. I am not completely sure if setting the scopes is necessary, but given the existing code sets them, I would suggest to continue to do that unconditionally. (And the same for moduleImportScope, I guess.)

Thank you. I presumed wrongly that the null check was meant to perform conditional assignment. I have updated this now to do unconditional assignment.

lahodaj · 2025-09-15T06:23:02Z

java/java.source.base/src/org/netbeans/modules/java/source/save/CasualDiff.java

        return bounds[1];
    }

+    protected int diffModuleImport(JCModuleImport oldT, JCModuleImport newT, int[] bounds) {


It would be good to add test for this here:
java/java.source.base/test/unit/src/org/netbeans/api/java/source/gen/ImportsTest.java

Thank you for the guidance about the location of the tests. I have added a couple of tests to address the cases of reordering of the module imports here, which exercises the paths in CasualDiff. Please review. Thanks.

lahodaj · 2025-09-15T06:24:18Z

java/java.source.base/test/unit/src/org/netbeans/api/java/source/GeneratorUtilitiesTest.java

    }

+    public void testAddImportsWithModule1() throws Exception {
+        performTest("test/Process.java", "package test;\nimport java.util.Collections;\npublic class Process {\npublic static void main(String... args) {\n" +


Nit: I would probably use a text block for the snippet.

Thanks. I've updated this now.

lahodaj · 2025-09-15T06:39:09Z

java/java.source.base/src/org/netbeans/api/java/source/GeneratorUtilities.java

+            if (explicitNamedImports.contains(currentToImportElement)) {
                cnt = 0;
+            } else {
+                Integer enclosingStarCnt = el == null ? null : isStatic ? typeCounts.get((TypeElement)el) :  pkgCounts.get((PackageElement)el);


I may need to think of this logic again, but seems reasonable at this time.

Thank you. I have split the earlier ternary expression into these lines before adding the module-import specific checks. I will take a re-look to simplify if possible. Thanks.

I have tried to add explanatory comments and simplifying the module-import specific section by making it similar to that for star imports replacement. Please review. Thank you.

sid-srini · 2025-09-15T13:00:37Z

Thank you @lahodaj for the review. I've attempted to address all the issues highlighted. Please review once again when possible. Thanks.

sid-srini · 2025-09-17T12:33:04Z

I have fixed the issues causing the couple of test failures and also incorporated support for transitive module imports in OrganizeImports and GeneratorUtilities.addImports. I request your review @lahodaj. Thank you.

Please note that one case produces sub-optimal results, as in GeneratorUtilitiesTest.testAddImportsWithModule6.

The results produced are correct and do not cause a compiler error.
Here theimport java.util.List; remains in the output even though the star import java.util.* is added.
This is due to the way the GeneratorUtilities.addImports checks for clashes against a CompoundScope which includes the module scope too.
The solution may be to further special-case by deducing the scope that causes the clash, and, then apply precedence rules whenever star import thresholds are checked.
However, as of now, this case is not addressed, since the priority was to first avoid the production of incorrect imports causing compile failures etc.

Please do weigh in on this aspect too. Thanks.

lahodaj · 2025-09-22T13:16:20Z

java/java.source.base/src/org/netbeans/api/java/source/GeneratorUtilities.java

+                        ModuleElement ml;
+                        try {
+                            ml = elements.getModuleOf(e);
+                        } catch (IllegalArgumentException ex) {


Using the catch here seems a bit sub-optimal here. The problematic cases originate in ImmutableTreeTranslator, and eventually in ElementOverlay. I think I would suggest to use ElementOverlay.moduleOf in the problematic cases. Like:

diff --git a/java/java.source.base/src/org/netbeans/api/java/source/GeneratorUtilities.java b/java/java.source.base/src/org/netbeans/api/java/source/GeneratorUtilities.java index 59aa8067aa..6bb6462698 100644 --- a/java/java.source.base/src/org/netbeans/api/java/source/GeneratorUtilities.java +++ b/java/java.source.base/src/org/netbeans/api/java/source/GeneratorUtilities.java @@ -88,6 +88,7 @@ import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.Function; import javax.lang.model.SourceVersion; import javax.lang.model.element.Element; @@ -1087,10 +1088,10 @@ public final class GeneratorUtilities { * @since 0.86 */ public CompilationUnitTree addImports(CompilationUnitTree cut, Set<? extends Element> toImport) { - return addImports(cut, cut.getImports(), toImport); + return addImports(cut, cut.getImports(), toImport, copy.getElements()::getModuleOf); } - private CompilationUnitTree addImports(CompilationUnitTree cut, List<? extends ImportTree> cutImports, Set<? extends Element> toImport) { + private CompilationUnitTree addImports(CompilationUnitTree cut, List<? extends ImportTree> cutImports, Set<? extends Element> toImport, Function<Element, ModuleElement> moduleOf) { assert cut != null && toImport != null && !toImport.isEmpty(); ArrayList<Element> elementsToImport = new ArrayList<>(toImport.size()); @@ -1207,12 +1208,7 @@ public final class GeneratorUtilities { } else { pkgCounts.put((PackageElement)el, cnt); if (cnt >= -1 && modCounts != null) { - ModuleElement ml; - try { - ml = elements.getModuleOf(e); - } catch (IllegalArgumentException ex) { - ml = null; - } + ModuleElement ml = moduleOf.apply(e); if (ml != null) { modCounts.merge(ml, 1, Integer::sum); } @@ -1443,12 +1439,7 @@ public final class GeneratorUtilities { } else { int modCount = 0; if (modCounts != null) { - ModuleElement m; - try { - m = elements.getModuleOf(currentToImportElement); - } catch (IllegalArgumentException ex) { - m = null; - } + ModuleElement m = moduleOf.apply(currentToImportElement); Integer mc = m == null ? null : modCounts.get(m); if (mc != null && mc < 0) { modCount = mc; @@ -2407,8 +2398,8 @@ public final class GeneratorUtilities { static { GeneratorUtilitiesAccessor.setInstance(new GeneratorUtilitiesAccessor() { @Override - public CompilationUnitTree addImports(GeneratorUtilities gu, CompilationUnitTree cut, List<? extends ImportTree> cutImports, Set<? extends Element> toImport) { - return gu.addImports(cut, cutImports, toImport); + public CompilationUnitTree addImports(GeneratorUtilities gu, CompilationUnitTree cut, List<? extends ImportTree> cutImports, Set<? extends Element> toImport, Function<Element, ModuleElement> moduleOf) { + return gu.addImports(cut, cutImports, toImport, moduleOf); } }); } diff --git a/java/java.source.base/src/org/netbeans/modules/java/source/GeneratorUtilitiesAccessor.java b/java/java.source.base/src/org/netbeans/modules/java/source/GeneratorUtilitiesAccessor.java index 3e96b8d5fd..75041ca8a8 100644 --- a/java/java.source.base/src/org/netbeans/modules/java/source/GeneratorUtilitiesAccessor.java +++ b/java/java.source.base/src/org/netbeans/modules/java/source/GeneratorUtilitiesAccessor.java @@ -22,7 +22,9 @@ import com.sun.source.tree.CompilationUnitTree; import com.sun.source.tree.ImportTree; import java.util.List; import java.util.Set; +import java.util.function.Function; import javax.lang.model.element.Element; +import javax.lang.model.element.ModuleElement; import org.netbeans.api.java.source.GeneratorUtilities; public abstract class GeneratorUtilitiesAccessor { @@ -60,6 +62,6 @@ public abstract class GeneratorUtilitiesAccessor { protected GeneratorUtilitiesAccessor() { } - public abstract CompilationUnitTree addImports(GeneratorUtilities gu, CompilationUnitTree cut, List<? extends ImportTree> cutImports, Set<? extends Element> toImport); + public abstract CompilationUnitTree addImports(GeneratorUtilities gu, CompilationUnitTree cut, List<? extends ImportTree> cutImports, Set<? extends Element> toImport, Function<Element, ModuleElement> moduleOf); } diff --git a/java/java.source.base/src/org/netbeans/modules/java/source/save/ElementOverlay.java b/java/java.source.base/src/org/netbeans/modules/java/source/save/ElementOverlay.java index 3c6144151f..7692df5a45 100644 --- a/java/java.source.base/src/org/netbeans/modules/java/source/save/ElementOverlay.java +++ b/java/java.source.base/src/org/netbeans/modules/java/source/save/ElementOverlay.java @@ -432,7 +432,7 @@ public class ElementOverlay { return (PackageElement) resolve(ast, elements, "", modle); } - private ModuleElement moduleOf(Elements elements, Element el) { + public ModuleElement moduleOf(Elements elements, Element el) { if (el instanceof TypeElementWrapper) return moduleOf(elements, ((TypeElementWrapper) el).delegateTo); if (el instanceof FakeTypeElement) diff --git a/java/java.source.base/src/org/netbeans/modules/java/source/transform/ImmutableTreeTranslator.java b/java/java.source.base/src/org/netbeans/modules/java/source/transform/ImmutableTreeTranslator.java index 463f4a5b91..6c074adb42 100644 --- a/java/java.source.base/src/org/netbeans/modules/java/source/transform/ImmutableTreeTranslator.java +++ b/java/java.source.base/src/org/netbeans/modules/java/source/transform/ImmutableTreeTranslator.java @@ -651,7 +651,7 @@ public class ImmutableTreeTranslator implements TreeVisitor<Tree,Object> { Set<? extends Element> newImports = importAnalysis.getImports(); if (copy != null && newImports != null && !newImports.isEmpty()) { imps = GeneratorUtilitiesAccessor.getInstance() - .addImports(GeneratorUtilities.get(copy), tree, imps, newImports) + .addImports(GeneratorUtilities.get(copy), tree, imps, newImports, el -> overlay.moduleOf(elements, el)) .getImports(); }

Thank you very much. I have committed your patch. I'll keep this approach in mind for the future.

mbien

I suggest to append "part 2" to the PR title (or change the name in some other way) since we had one with the same name one release ago #8425

java/java.hints/src/org/netbeans/modules/java/hints/OrganizeImports.java

sid-srini · 2025-09-23T04:28:56Z

Thanks @mbien for your review. I've updated the PR title and force-pushed the change with the review comment addressed. Please let me know if this is fine. Thanks.

1. Enhanced support for module imports in java.source/CasualDiff to allow re-ordering of imports. 2. Enhanced java.hints/OrganizeImports to support the presence of module imports in checking for usage, and, transferring the module import scope. Also included sort comparison for module imports, adding them as the last group. 3. Enhanced java.source/GeneratorUtilities.addImports() to support the writing of existing module imports in the compilation unit, as well as, the addition of module imports if needed in the future by any hint/completion. This includes: - checking for the usage of a module import. - checking for simple name clashes in the consolidated import scope which includes module imports now, for adding as named imports. - sorting comparison for module imports (added as the last group). 4. Incorporated handling of transitive module imports in partial support - In OrganizeImports and GeneratorUtilities. Note: Uses ElementOverlay.moduleOf when GeneratorUtilities.addImports is invoked from ImmutableTreeTranslator Co-authored-by: Jan Lahoda <[email protected]> Signed-off-by: Siddharth Srinivasan <[email protected]>

lahodaj · 2025-09-23T11:33:01Z

Unless there are objections, I'll integrate sometime soon. Thanks!

lahodaj reviewed Sep 15, 2025

View reviewed changes

lahodaj added the Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) label Sep 15, 2025

apache locked and limited conversation to collaborators Sep 15, 2025

apache unlocked this conversation Sep 15, 2025

sid-srini force-pushed the module-imports-in-organize-imports-etc branch from d54f876 to a9f1fa5 Compare September 15, 2025 12:50

lahodaj reviewed Sep 22, 2025

View reviewed changes

sid-srini force-pushed the module-imports-in-organize-imports-etc branch from b2d5423 to 1b6eff1 Compare September 22, 2025 16:53

mbien reviewed Sep 22, 2025

View reviewed changes

java/java.hints/src/org/netbeans/modules/java/hints/OrganizeImports.java Outdated Show resolved Hide resolved

sid-srini changed the title ~~Partial handling of module imports~~ Partial handling of module imports - Organize and Add imports Sep 23, 2025

sid-srini force-pushed the module-imports-in-organize-imports-etc branch from 1b6eff1 to af4df79 Compare September 23, 2025 04:25

sid-srini force-pushed the module-imports-in-organize-imports-etc branch from af4df79 to a84f212 Compare September 23, 2025 04:34

mbien added this to the NB28 milestone Sep 23, 2025

mbien added the hints label Sep 23, 2025

lahodaj merged commit 5595825 into apache:master Sep 23, 2025
69 of 70 checks passed

sid-srini mentioned this pull request Sep 23, 2025

Backporting NB28 support for organize and add with module imports oracle/javavscode#474

Merged

sid-srini deleted the module-imports-in-organize-imports-etc branch September 23, 2025 15:14

Partial handling of module imports - Organize and Add imports #8827

Partial handling of module imports - Organize and Add imports #8827

Uh oh!

Conversation

sid-srini commented Sep 15, 2025 • edited by mbien Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR approval and merge checklist:

Uh oh!

lahodaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sid-srini commented Sep 15, 2025

Uh oh!

sid-srini commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbien left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sid-srini commented Sep 23, 2025

Uh oh!

lahodaj commented Sep 23, 2025

Uh oh!

Uh oh!

Uh oh!

sid-srini commented Sep 15, 2025 •

edited by mbien

Loading

sid-srini commented Sep 17, 2025 •

edited

Loading