SONARJAVA-4698 Fix false positive in S1191 when variable is named "sun"

java-checks-test-sources/default/src/main/files/non-compiling/checks/SunPackagesUsedCheckCustom.java

@@ -7,12 +7,10 @@ private void f() {
     com.sun.imageio.plugins.bmp d =  // Compliant
       new com.sun.imageio.plugins.bmp(); // Compliant
     java.util.List a;
-    sun.Foo b; // Noncompliant {{Use classes from the Java API instead of Sun classes.}}
-//  ^^^^^^^
+    sun.Foo b; // Compliant - without semantic info, we can't distinguish between package and variable
     db.setErrorHandler(new com.sun.org.apache.xml.internal.security.utils
         .IgnoreAllErrorHandler());
-    sun       // secondary
-//  ^[el=+3;ec=12]<
+    sun       // Compliant
         .Foo.toto
         .asd c;
 

java-checks-test-sources/default/src/main/files/non-compiling/checks/SunPackagesUsedCheckSample.java

@@ -1,13 +1,13 @@
 import java.util.ArrayList;
 
 class SunPackagesUsedCheckSample {
+  private Object sun; // variable named "sun"
+
   private void f() {
     java.util.List a;
-    sun.Foo b; // Noncompliant
-//  ^^^^^^^
-    sun.Foo.toto.asd c; // secondary
-//  ^^^^^^^^^^^^^^^^<
-
+    sun.Foo b; // Compliant - without semantic info, we can't distinguish between package and variable
+    sun.Foo.toto.asd c; // Compliant
+
   }
 
   public Object uselessMethod() {
@@ -16,4 +16,18 @@ public Object uselessMethod() {
     }
     return null;
   }
+
+  // SONARJAVA-4698: False positive when variable is named "sun"
+  public void fooWithFieldNamedSun() {
+    sun.toString(); // Compliant - "sun" is a field of type Object, not a sun.* package class
+  }
+
+  public void barWithParameterNamedSun(Object sun) {
+    sun.toString(); // Compliant - "sun" is a parameter of type Object, not a sun.* package class
+  }
+
+  public void bazWithLocalVariableNamedSun() {
+    Object sun = new Object();
+    sun.toString(); // Compliant - "sun" is a local variable of type Object, not a sun.* package class
+  }
 }

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

@@ -0,0 +1,19 @@
+package checks;
+
+class SunPackagesUsedCheckSample {
+  private Object sun; // variable named "sun"
+
+  // SONARJAVA-4698: Variables named "sun" should not trigger the rule
+  public void fooWithFieldNamedSun() {
+    sun.toString(); // Compliant - "sun" is a field of type Object, not a sun.* package class
+  }
+
+  public void barWithParameterNamedSun(Object sun) {
+    sun.toString(); // Compliant - "sun" is a parameter of type Object, not a sun.* package class
+  }
+
+  public void bazWithLocalVariableNamedSun() {
+    Object sun = new Object();
+    sun.toString(); // Compliant - "sun" is a local variable of type Object, not a sun.* package class
+  }
+}

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

@@ -0,0 +1,41 @@
+package checks;
+
+import sun.misc.Unsafe; // Noncompliant [[secondary=8,11,16,18,23,28,33]]
+
+class SunPackagesUsedCheckWithRealSunUsage {
+
+  // Field declaration with sun.* type
+  private sun.misc.Unsafe unsafeField; // secondary
+
+  // Method with sun.* return type
+  public sun.misc.Unsafe getUnsafe() { // secondary
+    return null;
+  }
+
+  // Method with sun.* parameter type
+  public void doSomethingWithUnsafe(sun.misc.Unsafe unsafe) { // secondary
+    // Method call on sun.* class
+    sun.misc.Unsafe.getUnsafe(); // secondary
+  }
+
+  // Local variable with sun.* type
+  public void localVariable() {
+    sun.misc.Unsafe local; // secondary
+  }
+
+  // Class literal
+  public void classLiteral() {
+    Class<?> clazz = sun.misc.Unsafe.class; // secondary
+  }
+
+  // Fully qualified type in new expression
+  public void newExpression() {
+    Object obj = new sun.misc.Signal("INT"); // secondary
+  }
+
+  // This should be compliant - variable named "sun"
+  public void variableNamedSun() {
+    Object sun = new Object();
+    sun.toString(); // Compliant - "sun" is a variable, not a sun.* package
+  }
+}

java-checks/src/main/java/org/sonar/java/checks/SunPackagesUsedCheck.java

@@ -24,6 +24,9 @@
 import org.sonar.plugins.java.api.JavaFileScanner;
 import org.sonar.plugins.java.api.JavaFileScannerContext;
 import org.sonar.plugins.java.api.tree.BaseTreeVisitor;
+import org.sonar.plugins.java.api.tree.ExpressionTree;
+import org.sonar.plugins.java.api.tree.IdentifierTree;
+import org.sonar.plugins.java.api.tree.ImportTree;
 import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree;
 import org.sonar.plugins.java.api.tree.Tree;
 
@@ -54,6 +57,12 @@ public void scanFile(JavaFileScannerContext context) {
   }
 
   private void reportIssueWithSecondaries(JavaFileScannerContext context) {
+    // Sort by line number to ensure consistent primary issue location (first occurrence in source text)
+    reportedTrees.sort((t1, t2) -> Integer.compare(
+      t1.firstToken().range().start().line(),
+      t2.firstToken().range().start().line()
+    ));
+
     List<JavaFileScannerContext.Location> secondaries = reportedTrees.stream()
       .skip(1)
       .map(tree -> new JavaFileScannerContext.Location("Replace also this \"Sun\" reference.", tree))
@@ -63,10 +72,30 @@ private void reportIssueWithSecondaries(JavaFileScannerContext context) {
     context.reportIssue(this, reportedTrees.get(0), "Use classes from the Java API instead of Sun classes.", secondaries, effortToFix);
   }
 
+  @Override
+  public void visitImport(ImportTree tree) {
+    Tree qualifiedIdentifier = tree.qualifiedIdentifier();
+    if (qualifiedIdentifier.is(Tree.Kind.MEMBER_SELECT)) {
+      MemberSelectExpressionTree memberSelect = (MemberSelectExpressionTree) qualifiedIdentifier;
+      String reference = ExpressionsHelper.concatenate(memberSelect);
+      if (!isExcluded(reference) && isSunClass(reference)) {
+        // For imports, check if we have semantic info by checking if the import's symbol type is resolved
+        // In autoscan mode (without bytecode), the symbol's type will be unknown
+        var symbol = tree.symbol();
+        if (symbol != null && !symbol.isUnknown() && !symbol.type().isUnknown()) {
+          // We have full semantic info with bytecode, so this is a real sun.* import
+          reportedTrees.add(memberSelect);
+        }
+        // Without semantic info or bytecode, we can't be sure this is a real sun.* import, so skip it
+      }
+    }
+    super.visitImport(tree);
+  }
+
   @Override
   public void visitMemberSelectExpression(MemberSelectExpressionTree tree) {
     String reference = ExpressionsHelper.concatenate(tree);
-    if (!isExcluded(reference) && isSunClass(reference)) {
+    if (!isExcluded(reference) && isSunClass(reference) && isActuallySunPackage(tree)) {
       reportedTrees.add(tree);
     }
   }
@@ -75,6 +104,51 @@ private static boolean isSunClass(String reference) {
     return reference.startsWith("sun.");
   }
 
+  private static boolean isActuallySunPackage(MemberSelectExpressionTree tree) {
+    // Check if this is actually a sun.* package reference, not a variable named "sun"
+    //
+    // Strategy: Find the leftmost identifier in the qualified name chain (e.g., "sun" in "sun.misc.Unsafe")
+    // and check if it's a variable/field/parameter. If it is, then this is not a sun.* package reference.
+
+    // Navigate to the leftmost expression in the chain
+    ExpressionTree current = tree;
+    while (current.is(Tree.Kind.MEMBER_SELECT)) {
+      current = ((MemberSelectExpressionTree) current).expression();
+    }
+
+    // Now 'current' should be an IdentifierTree representing the leftmost identifier
+    if (current.is(Tree.Kind.IDENTIFIER)) {
+      var symbol = ((IdentifierTree) current).symbol();
+      if (!symbol.isUnknown()) {
+        // Check if this symbol is a variable, field, or parameter (not a type or package)
+        if (symbol.isVariableSymbol() || symbol.isMethodSymbol()) {
+          // This is a variable/method reference like "sun.toString()" where "sun" is a variable
+          // The method case handles things like "sun.method().field" where sun is a variable
+          return false;
+        }
+
+        // If the symbol is a type or package, we need to be more careful.
+        // In non-compiling code, variables may be misidentified as packages by the semantic engine.
+        // We can detect this by checking if the member select tree's type is resolved.
+        if (tree.symbolType().isUnknown()) {
+          // Type is not resolved, likely non-compiling code or misidentified variable.
+          // Prefer to avoid false positives in these cases.
+          return false;
+        }
+
+        // If the leftmost symbol is not a variable/method and the type is resolved,
+        // then it's likely a package/type reference to sun.*
+        return true;
+      }
+    }
+
+    // If we couldn't determine the leftmost symbol (unknown semantic info),
+    // we prefer to avoid false positives.
+    // This means we might miss some true sun.* package usages in non-compiling code or AutoScan,
+    // but users with proper builds and semantic analysis will still get accurate results.
+    return false;
+  }
+
   private boolean isExcluded(String reference) {
     for (String str : excludePackages) {
       if (!str.isEmpty() && reference.startsWith(str)) {

java-checks/src/test/java/org/sonar/java/checks/SunPackagesUsedCheckTest.java

@@ -24,19 +24,43 @@ class SunPackagesUsedCheckTest {
 
   @Test
   void detected() {
+    // Non-compiling code: without semantic information, we prefer to avoid false positives
+    // even if it means missing some true positives. This prevents annoying FPs in AutoScan scenarios.
     CheckVerifier.newVerifier()
       .onFile(TestUtils.nonCompilingTestSourcesPath("checks/SunPackagesUsedCheckSample.java"))
       .withCheck(new SunPackagesUsedCheck())
-      .verifyIssues();
+      .verifyNoIssues();
   }
 
   @Test
   void check_with_exclusion() {
+    // Non-compiling code: without semantic information, we prefer to avoid false positives
     SunPackagesUsedCheck check = new SunPackagesUsedCheck();
     check.exclude = "sun.excluded";
     CheckVerifier.newVerifier()
       .onFile(TestUtils.nonCompilingTestSourcesPath("checks/SunPackagesUsedCheckCustom.java"))
       .withCheck(check)
+      .verifyNoIssues();
+  }
+
+  @Test
+  void detected_with_semantic() {
+    // With semantic information (compiling code with bytecode), we correctly distinguish
+    // between variables named "sun" (compliant) and actual sun.* package usage (noncompliant).
+    // This test file only contains variables named "sun", so no issues should be raised.
+    CheckVerifier.newVerifier()
+      .onFile(TestUtils.mainCodeSourcesPath("checks/SunPackagesUsedCheckSample.java"))
+      .withCheck(new SunPackagesUsedCheck())
+      .verifyNoIssues();
+  }
+
+  @Test
+  void detected_real_sun_package_usage() {
+    // Test that we correctly detect all forms of sun.* package usage
+    // including imports, field declarations, return types, parameters, etc.
+    CheckVerifier.newVerifier()
+      .onFile(TestUtils.mainCodeSourcesPath("checks/SunPackagesUsedCheckWithRealSunUsage.java"))
+      .withCheck(new SunPackagesUsedCheck())
       .verifyIssues();
   }