diff --git a/java-checks-test-sources/default/src/main/files/non-compiling/checks/SunPackagesUsedCheckCustom.java b/java-checks-test-sources/default/src/main/files/non-compiling/checks/SunPackagesUsedCheckCustom.java index e820da1793..4d7f13b844 100644 --- a/java-checks-test-sources/default/src/main/files/non-compiling/checks/SunPackagesUsedCheckCustom.java +++ b/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; diff --git a/java-checks-test-sources/default/src/main/files/non-compiling/checks/SunPackagesUsedCheckSample.java b/java-checks-test-sources/default/src/main/files/non-compiling/checks/SunPackagesUsedCheckSample.java index 48135307e6..75e9913af5 100644 --- a/java-checks-test-sources/default/src/main/files/non-compiling/checks/SunPackagesUsedCheckSample.java +++ b/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 + } } diff --git a/java-checks-test-sources/default/src/main/java/checks/SunPackagesUsedCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/SunPackagesUsedCheckSample.java new file mode 100644 index 0000000000..dd77ef5c9d --- /dev/null +++ b/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 + } +} diff --git a/java-checks-test-sources/default/src/main/java/checks/SunPackagesUsedCheckWithRealSunUsage.java b/java-checks-test-sources/default/src/main/java/checks/SunPackagesUsedCheckWithRealSunUsage.java new file mode 100644 index 0000000000..2718bb94d7 --- /dev/null +++ b/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 + } +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/SunPackagesUsedCheck.java b/java-checks/src/main/java/org/sonar/java/checks/SunPackagesUsedCheck.java index 8ab35200c9..f841771299 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/SunPackagesUsedCheck.java +++ b/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 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)) { diff --git a/java-checks/src/test/java/org/sonar/java/checks/SunPackagesUsedCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/SunPackagesUsedCheckTest.java index 286cdef497..1ff63c19ee 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/SunPackagesUsedCheckTest.java +++ b/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(); }