SONARJAVA-6146 S8445: Relax the rule to allow more styles of sorting imports (#5494)

Author: asya-vorobeva

java-checks-test-sources/default/src/main/java/checks/ImportDeclarationOrderCheckSample.java

@@ -0,0 +1,13 @@
+package checks.ImportDeclarationsOrderCheck;
+
+// fix@qf1 {{Reorganize imports}}
+// edit@qf1 [[sl=6;sc=1;el=10;ec=36]] {{import module java.base;\n\nimport java.sql.Date;\nimport java.util.List;\n\nimport static java.util.Collections.*;\nimport static java.lang.System.out;}}
+// Module import not first, regular first
+import java.sql.Date; // Noncompliant [[quickfixes=qf1]]
+import java.util.List;
+import module java.base;
+import static java.util.Collections.*;
+import static java.lang.System.out;
+
+class ImportDeclarationOrderCheckWithModulesAndRegularFirstSample {
+}

java-checks-test-sources/default/src/main/java/checks/ImportDeclarationOrderCheckSampleNoIssuesSample.java

@@ -0,0 +1,13 @@
+package checks.ImportDeclarationsOrderCheck;
+
+// fix@qf1 {{Reorganize imports}}
+// edit@qf1 [[sl=6;sc=1;el=10;ec=23]] {{import module java.base;\n\nimport static java.util.Collections.*;\nimport static java.lang.System.out;\n\nimport java.sql.Date;\nimport java.util.List;}}
+// Module import not first, static first
+import static java.util.Collections.*; // Noncompliant [[quickfixes=qf1]]
+import static java.lang.System.out;
+import module java.base;
+import java.sql.Date;
+import java.util.List;
+
+class ImportDeclarationOrderCheckWithModulesAndStaticFirstSample {
+}

java-checks-test-sources/default/src/main/java/checks/ImportDeclarationsOrderCheck/ImportDeclarationOrderCheckWithModulesAndRegularFirstSample.java

@@ -0,0 +1,12 @@
+package checks.ImportDeclarationsOrderCheck;
+
+import module java.base;
+
+import java.sql.Date;
+import java.util.List;
+
+import static java.util.Collections.*;
+import static java.lang.System.out;
+
+public class ImportDeclarationOrderCheckWithModulesNoIssues {
+}

java-checks-test-sources/default/src/main/java/checks/ImportDeclarationsOrderCheck/ImportDeclarationOrderCheckWithModulesAndStaticFirstSample.java

@@ -0,0 +1,6 @@
+package checks.ImportDeclarationsOrderCheck;
+
+// No issues expected - rule only applies when module imports are present
+
+class ImportDeclarationOrderCheckWithoutModulesAndRegularFirstSample {
+}

java-checks-test-sources/default/src/main/java/checks/ImportDeclarationsOrderCheck/ImportDeclarationOrderCheckWithModulesNoIssues.java

@@ -0,0 +1,12 @@
+package checks.ImportDeclarationsOrderCheck;
+
+// No issues expected - rule only applies when module imports are present
+import static java.util.Collections.emptyList;
+import static java.util.Collections.*;
+import java.io.*;
+import java.util.List;
+import static java.lang.Math.PI;
+import java.util.Map;
+
+class ImportDeclarationOrderCheckWithoutModulesAndStaticFirstSample {
+}

java-checks-test-sources/default/src/main/java/checks/ImportDeclarationsOrderCheck/ImportDeclarationOrderCheckWithoutModulesAndRegularFirstSample.java

@@ -20,20 +20,19 @@
 import java.util.Collections;
 import java.util.List;
 import org.sonar.check.Rule;
+import org.sonar.java.annotations.VisibleForTesting;
+import org.sonar.java.checks.helpers.ExpressionsHelper;
+import org.sonar.java.checks.helpers.QuickFixHelper;
+import org.sonar.java.reporting.JavaQuickFix;
+import org.sonar.java.reporting.JavaTextEdit;
 import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
-import org.sonar.plugins.java.api.JavaVersion;
-import org.sonar.plugins.java.api.JavaVersionAwareVisitor;
 import org.sonar.plugins.java.api.tree.CompilationUnitTree;
+import org.sonar.plugins.java.api.tree.ExpressionTree;
 import org.sonar.plugins.java.api.tree.ImportTree;
 import org.sonar.plugins.java.api.tree.Tree;
 
 @Rule(key = "S8445")
-public class ImportDeclarationOrderCheck extends IssuableSubscriptionVisitor implements JavaVersionAwareVisitor {
-
-  @Override
-  public boolean isCompatibleWithJavaVersion(JavaVersion version) {
-    return version.isJava25Compatible();
-  }
+public class ImportDeclarationOrderCheck extends IssuableSubscriptionVisitor {
 
   @Override
   public List<Tree.Kind> nodesToVisit() {
@@ -44,75 +43,170 @@ public List<Tree.Kind> nodesToVisit() {
   public void visitNode(Tree tree) {
     CompilationUnitTree compilationUnit = (CompilationUnitTree) tree;
 
-    List<ImportTree> imports = new ArrayList<>();
-    // Collect all import statements
-    compilationUnit.imports()
+    List<ImportTree> imports = compilationUnit.imports()
       .stream()
       .filter(importTree -> importTree.is(Tree.Kind.IMPORT))
       .map(ImportTree.class::cast)
-      .forEach(imports::add);
+      .toList();
 
     analyzeImportOrder(imports);
   }
 
   private void analyzeImportOrder(List<ImportTree> imports) {
-    if (imports.size() <= 1) {
+    // if we don't have any module import declarations, we don't raise the issue
+    if (imports.stream().noneMatch(ImportTree::isModule)) {
       return;
     }
 
-    ImportType previousType = ImportType.SENTINEL_IMPORT;
-    for (ImportTree importTree : imports) {
-      ImportType currentType = classifyImport(importTree);
+    if (hasProblematicImport(imports)) {
+      QuickFixHelper.newIssue(context)
+        .forRule(this)
+        .onTree(imports.get(0))
+        .withMessage("Import declarations should be organized with module imports first, followed by grouped regular imports and grouped static imports.")
+        .withQuickFix(() -> createQuickFix(imports))
+        .report();
+    }
+  }
 
-      if (currentType.ordinal() < previousType.ordinal()) {
-        String message = buildMessage(currentType, previousType);
-        reportIssue(importTree.qualifiedIdentifier(), message);
-      }
+  private static JavaQuickFix createQuickFix(List<ImportTree> imports) {
+    // Separate imports by type and determine order
+    List<ImportTree> moduleImports = new ArrayList<>();
+    List<ImportTree> regularImports = new ArrayList<>();
+    List<ImportTree> staticImports = new ArrayList<>();
+    boolean regularFirst = true;
+    boolean orderDetermined = false;
 
-      previousType = currentType;
+    for (ImportTree importTree : imports) {
+      ImportType type = ImportType.fromTree(importTree);
+      switch (type) {
+        case MODULE:
+          moduleImports.add(importTree);
+          break;
+        case REGULAR:
+          regularImports.add(importTree);
+          orderDetermined = true;
+          break;
+        case STATIC:
+          staticImports.add(importTree);
+          if (!orderDetermined) {
+            regularFirst = false;
+            orderDetermined = true;
+          }
+          break;
+      }
     }
-  }
 
-  private static String buildMessage(ImportType currentType, ImportType previousType) {
-    return String.format("Reorder this %s import to come before %s imports.",
-      currentType.getDescription(),
-      previousType.getDescription());
+    // Build the reorganized import section
+    String reorganized = buildReorganizedImports(moduleImports, regularImports, staticImports, regularFirst);
+
+    // Create text edit to replace the entire import section
+    ImportTree firstImport = imports.get(0);
+    ImportTree lastImport = imports.get(imports.size() - 1);
+
+    return JavaQuickFix.newQuickFix("Reorganize imports")
+      .addTextEdit(JavaTextEdit.replaceBetweenTree(firstImport, lastImport, reorganized))
+      .build();
   }
 
-  private static ImportType classifyImport(ImportTree importTree) {
-    boolean isWildcard = isWildcardImport(importTree);
+  private static String buildReorganizedImports(List<ImportTree> moduleImports, List<ImportTree> regularImports,
+                                                 List<ImportTree> staticImports, boolean regularFirst) {
+    StringBuilder reorganized = new StringBuilder();
 
-    if (importTree.isModule()) {
-      return ImportType.MODULE_IMPORT;
+    // Add module imports first
+    for (ImportTree importTree : moduleImports) {
+      reorganized.append(getImportText(importTree)).append("\n");
     }
-
-    if (importTree.isStatic()) {
-      return isWildcard ? ImportType.STATIC_PACKAGE_IMPORT : ImportType.STATIC_SINGLE_TYPE_IMPORT;
+    reorganized.append("\n");
+    // Add regular and static imports in the determined order
+    if (regularFirst) {
+      appendImportGroup(reorganized, regularImports, !staticImports.isEmpty());
+      appendImportGroup(reorganized, staticImports, false);
+    } else {
+      appendImportGroup(reorganized, staticImports, !regularImports.isEmpty());
+      appendImportGroup(reorganized, regularImports, false);
     }
 
-    return isWildcard ? ImportType.PACKAGE_IMPORT : ImportType.SINGLE_TYPE_IMPORT;
+    return reorganized.toString().trim();
   }
 
-  private static boolean isWildcardImport(ImportTree importTree) {
-    return "*".equals(importTree.qualifiedIdentifier().lastToken().text());
+  private static void appendImportGroup(StringBuilder builder, List<ImportTree> imports, boolean addBlankLineAfter) {
+    for (ImportTree importTree : imports) {
+      builder.append(getImportText(importTree)).append("\n");
+    }
+    if (addBlankLineAfter) {
+      builder.append("\n");
+    }
   }
 
-  enum ImportType {
-    SENTINEL_IMPORT("sentinel"),
-    MODULE_IMPORT("module"),
-    PACKAGE_IMPORT("on-demand package"),
-    SINGLE_TYPE_IMPORT("single-type"),
-    STATIC_PACKAGE_IMPORT("static on-demand package"),
-    STATIC_SINGLE_TYPE_IMPORT("static single-type");
+  private static String getImportText(ImportTree importTree) {
+    // Build the import statement from the tree structure
+    StringBuilder sb = new StringBuilder("import ");
+    if (importTree.isModule()) {
+      sb.append("module ");
+    } else if (importTree.isStatic()) {
+      sb.append("static ");
+    }
+    sb.append(ExpressionsHelper.concatenate((ExpressionTree) importTree.qualifiedIdentifier())).append(";");
+    return sb.toString();
+  }
 
-    private final String description;
+  /**
+   * Checks if there is an import that violates the grouping rules.
+   * Returns false if all imports are properly organized.
+   */
+  @VisibleForTesting
+  static boolean hasProblematicImport(List<ImportTree> imports) {
+    boolean seenRegular = false;
+    boolean seenStatic = false;
+    boolean seenStaticAfterRegular = false;
+    boolean seenRegularAfterStatic = false;
 
-    ImportType(String description) {
-      this.description = description;
+    for (ImportTree importTree : imports) {
+      ImportType type = ImportType.fromTree(importTree);
+      switch (type) {
+        case MODULE:
+          if (seenRegular || seenStatic) {
+            return true;
+          }
+
+          break;
+
+        case REGULAR:
+          if (seenStaticAfterRegular) {
+            //regular -> static -> regular
+            return true;
+          }
+
+          seenRegular = true;
+          seenRegularAfterStatic |= seenStatic;
+          break;
+
+        case STATIC:
+          if (seenRegularAfterStatic) {
+            //static -> regular -> static
+            return true;
+          }
+
+          seenStatic = true;
+          seenStaticAfterRegular |= seenRegular;
+          break;
+      }
     }
 
-    public String getDescription() {
-      return description;
+    return false;
+  }
+
+  enum ImportType {
+    MODULE, REGULAR, STATIC;
+
+    static ImportType fromTree(ImportTree importTree) {
+      if (importTree.isModule()) {
+        return MODULE;
+      } else if (importTree.isStatic()) {
+        return STATIC;
+      } else {
+        return REGULAR;
+      }
     }
   }
 }