diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S9149.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S9149.json index a20d01766a7..56f9ca62f8f 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S9149.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S9149.json @@ -2,5 +2,5 @@ "ruleKey": "S9149", "hasTruePositives": true, "falseNegatives": 0, - "falsePositives": 0 + "falsePositives": 1 } diff --git a/its/ruling/src/test/resources/expected/commons-beanutils/java-S9149.json b/its/ruling/src/test/resources/expected/commons-beanutils/java-S9149.json index 3c86dacf3f5..ddd2c82f1fd 100644 --- a/its/ruling/src/test/resources/expected/commons-beanutils/java-S9149.json +++ b/its/ruling/src/test/resources/expected/commons-beanutils/java-S9149.json @@ -8,11 +8,5 @@ 432, 487, 512 -], -"commons-beanutils:src/test/java/org/apache/commons/beanutils2/BeanUtils2TestCase.java": [ -57 -], -"commons-beanutils:src/test/java/org/apache/commons/beanutils2/WrapDynaBeanTestCase.java": [ -75 ] } diff --git a/its/ruling/src/test/resources/expected/guava/java-S9149.json b/its/ruling/src/test/resources/expected/guava/java-S9149.json deleted file mode 100644 index c8f06f77668..00000000000 --- a/its/ruling/src/test/resources/expected/guava/java-S9149.json +++ /dev/null @@ -1,51 +0,0 @@ -{ -"guava:src/com/google/common/collect/ImmutableBiMap.java": [ -41, -48, -57, -66, -75, -85, -97, -246, -268 -], -"guava:src/com/google/common/collect/ImmutableListMultimap.java": [ -51, -58, -67, -77, -88, -101, -118, -237, -278 -], -"guava:src/com/google/common/collect/ImmutableSetMultimap.java": [ -59, -66, -77, -89, -102, -117, -133, -300, -349 -], -"guava:src/com/google/common/collect/ImmutableSortedMap.java": [ -85, -180, -218 -], -"guava:src/com/google/common/collect/ImmutableSortedMultiset.java": [ -63, -171, -189 -], -"guava:src/com/google/common/collect/ImmutableSortedSet.java": [ -78, -200, -237, -256 -] -} diff --git a/java-checks-test-sources/default/src/main/files/non-compiling/checks/StaticMethodHidingCheckSample.java b/java-checks-test-sources/default/src/main/files/non-compiling/checks/StaticMethodHidingCheckSample.java index 7d0bbd9c822..64067f2cea0 100644 --- a/java-checks-test-sources/default/src/main/files/non-compiling/checks/StaticMethodHidingCheckSample.java +++ b/java-checks-test-sources/default/src/main/files/non-compiling/checks/StaticMethodHidingCheckSample.java @@ -27,4 +27,18 @@ static void process(Unknown param) { // Compliant - parameter type is unknown } } + // --- Noncompliant: unknown return types are not considered covariant --- + + static class ParentWithUnknownReturn { + static Unknown create() { + return null; + } + } + + static class ChildWithUnknownReturn extends ParentWithUnknownReturn { + static Unknown create() { // Noncompliant {{Rename this method; it hides "create" in "ParentWithUnknownReturn".}} + return null; + } + } + } diff --git a/java-checks-test-sources/default/src/main/java/checks/StaticMethodHidingCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/StaticMethodHidingCheckSample.java index 482141e71a2..08404f77744 100644 --- a/java-checks-test-sources/default/src/main/java/checks/StaticMethodHidingCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/StaticMethodHidingCheckSample.java @@ -2,7 +2,12 @@ import com.google.errorprone.annotations.DoNotCall; import com.google.errorprone.annotations.InlineMe; +import java.util.ArrayList; +import java.util.Collection; import java.util.List; +import junit.framework.Test; +import junit.framework.TestCase; +import junit.framework.TestSuite; import org.jetbrains.annotations.ApiStatus; class StaticMethodHidingCheckSample { @@ -331,4 +336,68 @@ static class PlainChild extends PlainParent { static void compute() { // Noncompliant {{Rename this method; it hides "compute" in "PlainParent".}} } } + + // --- Compliant: static factory returning a more specific type --- + + static class ImmutableCollectionLike { + static ImmutableCollectionLike of() { + return new ImmutableCollectionLike(); + } + + static Collection copyOf(Collection elements) { + return elements; + } + + static Object create() { + return new Object(); + } + } + + static class ImmutableSetLike extends ImmutableCollectionLike { + static ImmutableSetLike of() { // Compliant - covariant static factory + return new ImmutableSetLike(); + } + + static List copyOf(Collection elements) { // Compliant - covariant static factory + return new ArrayList<>(elements); + } + + static Object create() { // Noncompliant {{Rename this method; it hides "create" in "ImmutableCollectionLike".}} + return new Object(); + } + } + + static class UnrelatedReturnParent { + static Integer value() { + return 1; + } + } + + static class UnrelatedReturnChild extends UnrelatedReturnParent { + static Integer value() { // Noncompliant {{Rename this method; it hides "value" in "UnrelatedReturnParent".}} + return 2; + } + } + + // --- Compliant: JUnit 3 suite() methods have to be redeclared in each test class --- + + public static class ParentTestCase extends TestCase { + public static Test suite() { + return new TestSuite(ParentTestCase.class); + } + + public static Test other() { + return new TestSuite(ParentTestCase.class); + } + } + + public static class ChildTestCase extends ParentTestCase { + public static Test suite() { // Compliant - JUnit 3 suite + return new TestSuite(ChildTestCase.class); + } + + public static Test other() { // Noncompliant {{Rename this method; it hides "other" in "ParentTestCase".}} + return new TestSuite(ChildTestCase.class); + } + } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/StaticMethodHidingCheck.java b/java-checks/src/main/java/org/sonar/java/checks/StaticMethodHidingCheck.java index 858900a4ce6..8974e79b786 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/StaticMethodHidingCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/StaticMethodHidingCheck.java @@ -58,7 +58,9 @@ private boolean checkHiding(MethodTree methodTree, Symbol.MethodSymbol methodSym for (Symbol symbol : superClass.symbol().lookupSymbols(methodSymbol.name())) { if (symbol.isMethodSymbol() && symbol.isStatic() && !symbol.isPrivate() && hasSameParameterTypes(methodSymbol, (Symbol.MethodSymbol) symbol)) { - reportHidingIssue(methodTree, methodSymbol, (Symbol.MethodSymbol) symbol); + if (!isRequiredByDesign(methodSymbol, (Symbol.MethodSymbol) symbol)) { + reportHidingIssue(methodTree, methodSymbol, (Symbol.MethodSymbol) symbol); + } return true; } } @@ -88,6 +90,26 @@ private static boolean isIntentionalHiding(Symbol.MethodSymbol methodSymbol) { || methodSymbol.metadata().isAnnotatedWith("org.jetbrains.annotations.ApiStatus$ScheduledForRemoval"); } + /** + * Some hierarchies are designed so that subclasses have to redeclare static methods of their parent: + *
    + *
  • static factories returning a more specific type than the hidden one (e.g. Guava's {@code ImmutableBiMap.of()} hiding {@code ImmutableMap.of()})
    • + *
  • JUnit 3 {@code public static Test suite()} methods, looked up by the test runner on each test class
    • + *
+ */ + private static boolean isRequiredByDesign(Symbol.MethodSymbol methodSymbol, Symbol.MethodSymbol hiddenMethod) { + Type returnType = methodSymbol.returnType().type().erasure(); + Type hiddenReturnType = hiddenMethod.returnType().type().erasure(); + boolean isCovariantFactory = !returnType.equals(hiddenReturnType) && returnType.isSubtypeOf(hiddenReturnType); + return isCovariantFactory || isJUnit3Suite(methodSymbol); + } + + private static boolean isJUnit3Suite(Symbol.MethodSymbol methodSymbol) { + return "suite".equals(methodSymbol.name()) + && methodSymbol.parameterTypes().isEmpty() + && methodSymbol.returnType().type().is("junit.framework.Test"); + } + private static boolean hasSameParameterTypes(Symbol.MethodSymbol method, Symbol.MethodSymbol candidate) { List methodParams = method.parameterTypes(); List candidateParams = candidate.parameterTypes();