fix(IAOM): filter bugs which appear in super classes

MET04-J
spotbugs · Apr 13, 2024 · 912d9fe · 912d9fe
1 parent a483929
commit 912d9fe
Show file tree

Hide file tree

Showing 14 changed files with 153 additions and 74 deletions.
diff --git a/...sts/src/test/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethodsTest.java b/...sts/src/test/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethodsTest.java
@@ -3,90 +3,104 @@
 import edu.umd.cs.findbugs.AbstractIntegrationTest;
 import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
 import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
 
 import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.hasItem;
+import static org.hamcrest.Matchers.not;
 
-public class FindIncreasedAccessibilityOfMethodsTest extends AbstractIntegrationTest {
-
+class FindIncreasedAccessibilityOfMethodsTest extends AbstractIntegrationTest {
     @Test
-    public void findIAOMBugInClass_IncreasedAccessibilityOfMethods_SubClassFromSamePackage() {
+    void findIAOMBugInClass_IncreasedAccessibilityOfMethods_SubClassFromSamePackage() {
         performAnalysis(
                 "increasedAccessibilityOfMethods/SuperClassOfSuperClass.class",
                 "increasedAccessibilityOfMethods/SuperClass.class",
                 "increasedAccessibilityOfMethods/SubClassFromSamePackage.class");
 
-        assertNumberOfIAOMBugs(6);
+        assertContainsCorrectNumberOfIAOMBugs(6);
 
-        assertIAOMBug("SubClassFromSamePackage", "protectedMethodToPublic");
-        assertIAOMBug("SubClassFromSamePackage", "packagePrivateMethodToPublic");
-        assertIAOMBug("SubClassFromSamePackage", "packagePrivateMethodToProtected");
-        assertIAOMBug("SubClassFromSamePackage", "superProtectedMethodToPublic");
-        assertIAOMBug("SubClassFromSamePackage", "superPackagePrivateMethodToPublic");
-        assertIAOMBug("SubClassFromSamePackage", "superPackagePrivateMethodToProtected");
+        assertContainsIAOMBug("SubClassFromSamePackage", "protectedMethodToPublic");
+        assertContainsIAOMBug("SubClassFromSamePackage", "packagePrivateMethodToPublic");
+        assertContainsIAOMBug("SubClassFromSamePackage", "packagePrivateMethodToProtected");
+        assertContainsIAOMBug("SubClassFromSamePackage", "superProtectedMethodToPublic");
+        assertContainsIAOMBug("SubClassFromSamePackage", "superPackagePrivateMethodToPublic");
+        assertContainsIAOMBug("SubClassFromSamePackage", "superPackagePrivateMethodToProtected");
     }
 
     @Test
-    public void findIAOMBugInClass_IncreasedAccessibilityOfMethods_SubClassFromAnotherPackage() {
+    void findIAOMBugInClass_IncreasedAccessibilityOfMethods_SubClassFromAnotherPackage() {
         performAnalysis(
                 "increasedAccessibilityOfMethods/SuperClassOfSuperClass.class",
                 "increasedAccessibilityOfMethods/SuperClass.class",
                 "increasedAccessibilityOfMethods/anotherPackage/SubClassFromAnotherPackage.class");
 
-        assertNumberOfIAOMBugs(1);
+        assertContainsCorrectNumberOfIAOMBugs(1);
 
-        assertIAOMBug("SubClassFromAnotherPackage", "protectedMethodToPublic");
+        assertContainsIAOMBug("SubClassFromAnotherPackage", "protectedMethodToPublic");
     }
 
     @Test
-    public void findNoIAOMBugInClass_IncreasedAccessibilityOfMethods_SubClassFromAnotherPackage() {
+    void findNoIAOMBugInClass_IncreasedAccessibilityOfMethods_CorrectSubClass() {
         performAnalysis(
                 "increasedAccessibilityOfMethods/SuperClassOfSuperClass.class",
-                "increasedAccessibilityOfMethods/SuperClass.class",
                 "increasedAccessibilityOfMethods/anotherPackage/CorrectSubClass.class");
 
-        assertNumberOfIAOMBugs(0);
+        assertContainsCorrectNumberOfIAOMBugs(0);
     }
 
     @Test
-    public void findNoIAOMBugInClass_IncreasedAccessibilityOfMethods_CloneableImplementation() {
+    void findNoIAOMBugInClass_IncreasedAccessibilityOfMethods_CloneableImplementation() {
         performAnalysis(
                 "increasedAccessibilityOfMethods/CloneableImplementation.class");
 
-        assertNumberOfIAOMBugs(0);
+        assertContainsCorrectNumberOfIAOMBugs(0);
     }
 
     @Test
-    public void findNoIAOMBugInClass_IncreasedAccessibilityOfMethods_InterfaceImplementation() {
+    void findNoIAOMBugInClass_IncreasedAccessibilityOfMethods_InterfaceImplementation() {
         performAnalysis(
                 "increasedAccessibilityOfMethods/Interface.class",
                 "increasedAccessibilityOfMethods/InterfaceImplementation.class");
 
-        assertNumberOfIAOMBugs(0);
+        assertContainsCorrectNumberOfIAOMBugs(0);
     }
 
     @Test
-    public void findNoIAOMBugInClass_IncreasedAccessibilityOfMethods_GenericImplementation() {
+    void findNoIAOMBugInClass_IncreasedAccessibilityOfMethods_GenericImplementation() {
         performAnalysis(
                 "increasedAccessibilityOfMethods/GenericSuperClass.class",
                 "increasedAccessibilityOfMethods/GenericSubClass.class");
 
-        assertNumberOfIAOMBugs(3);
+        assertContainsCorrectNumberOfIAOMBugs(3);
 
-        assertIAOMBug("GenericSubClass", "protectedMethodToPublicWithGenericParameter");
-        assertIAOMBug("GenericSubClass", "packagePrivateMethodToPublicWithGenericParameter");
-        assertIAOMBug("GenericSubClass", "packagePrivateMethodToProtectedWithGenericParameter");
+        assertContainsIAOMBug("GenericSubClass", "protectedMethodToPublicWithGenericParameter");
+        assertContainsIAOMBug("GenericSubClass", "packagePrivateMethodToPublicWithGenericParameter");
+        assertContainsIAOMBug("GenericSubClass", "packagePrivateMethodToProtectedWithGenericParameter");
     }
 
-    private void assertNumberOfIAOMBugs(int numberOfBugs) {
+    @Test
+    void findNoIAOMBugInClass_IncreasedAccessibilityOfMethods_SubClassWithRepeatedOverride() {
+        performAnalysis(
+                "increasedAccessibilityOfMethods/SuperClassOfSuperClass.class",
+                "increasedAccessibilityOfMethods/SuperClassWithOverride.class",
+                "increasedAccessibilityOfMethods/SubClassWithRepeatedOverride.class");
+
+        assertContainsCorrectNumberOfIAOMBugs(3);
+
+        assertContainsIAOMBug("SuperClassWithOverride", "superProtectedMethodToPublicOverriddenInSuperClass");
+        assertContainsIAOMBug("SuperClassWithOverride", "superPackagePrivateMethodToProtectedOverriddenInSuperClass");
+        assertContainsIAOMBug("SubClassWithRepeatedOverride", "superPackagePrivateMethodToProtectedOverriddenInSuperClass");
+        assertNotContainsIAOMBug("SubClassWithRepeatedOverride", "superProtectedMethodToPublicOverriddenInSuperClass");
+    }
+
+    private void assertContainsCorrectNumberOfIAOMBugs(int numberOfBugs) {
         BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
                 .bugType("IAOM_DO_NOT_INCREASE_METHOD_ACCESSIBILITY").build();
         assertThat(getBugCollection(), containsExactly(numberOfBugs, bugTypeMatcher));
     }
 
-    private void assertIAOMBug(String className, String methodName) {
+    private void assertContainsIAOMBug(String className, String methodName) {
         BugInstanceMatcher bugInstanceMatcher = new BugInstanceMatcherBuilder()
                 .bugType("IAOM_DO_NOT_INCREASE_METHOD_ACCESSIBILITY")
                 .inClass(className)
@@ -95,4 +109,12 @@ private void assertIAOMBug(String className, String methodName) {
         assertThat(getBugCollection(), hasItem(bugInstanceMatcher));
     }
 
+    private void assertNotContainsIAOMBug(String className, String methodName) {
+        BugInstanceMatcher bugInstanceMatcher = new BugInstanceMatcherBuilder()
+                .bugType("IAOM_DO_NOT_INCREASE_METHOD_ACCESSIBILITY")
+                .inClass(className)
+                .inMethod(methodName)
+                .build();
+        assertThat(getBugCollection(), not(hasItem(bugInstanceMatcher)));
+    }
 }
diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethods.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethods.java
@@ -31,9 +31,9 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Objects;
+import java.util.stream.Collectors;
 
 public class FindIncreasedAccessibilityOfMethods implements Detector {
-
     private static final String BUG_TYPE = "IAOM_DO_NOT_INCREASE_METHOD_ACCESSIBILITY";
     private static final String PACKAGE_PRIVATE = "package private";
     private static final String PROTECTED = "protected";
@@ -48,52 +48,63 @@ public FindIncreasedAccessibilityOfMethods(BugReporter bugReporter) {
     @Override
     public void visitClassContext(ClassContext classContext) {
         JavaClass subClass = classContext.getJavaClass();
+        for (IAOMBug bug : findIAOMBugs(subClass)) {
+            bugReporter.reportBug(new BugInstance(this, BUG_TYPE, LOW_PRIORITY)
+                    .addClassAndMethod(bug.clazz, bug.method)
+                    .addString(bug.oldAccessibility)
+                    .addString(bug.newAccessibility));
+        }
+    }
+
+    @Override
+    public void report() {
+    }
+
+    private List<IAOMBug> findIAOMBugs(JavaClass subClass) {
+        List<IAOMBug> iaomBugs = new ArrayList<>();
         JavaClass[] superClasses;
         try {
             superClasses = subClass.getSuperClasses();
         } catch (ClassNotFoundException e) {
             AnalysisContext.reportMissingClass(e);
-            return;
+            return iaomBugs;
         }
 
         List<Method> subClassMethods = getMethodsExceptConstructors(subClass);
+        List<IAOMBug> superClassBugs = new ArrayList<>();
         for (JavaClass superClass : superClasses) {
+            superClassBugs.addAll(findIAOMBugs(superClass));
             for (Method superClassMethod : getMethodsExceptConstructors(superClass)) {
                 if (isAccessibleFromSubClass(superClassMethod, subClass, superClass)) {
                     for (Method subClassMethod : subClassMethods) {
                         if (isOverridable(superClassMethod, subClassMethod) && !isCloneMethodFromCloneable(subClass, superClassMethod)) {
                             if (superClassMethod.isProtected() && subClassMethod.isPublic()) {
-                                bugReporter.reportBug(new BugInstance(this, BUG_TYPE, NORMAL_PRIORITY)
-                                        .addClassAndMethod(subClass, subClassMethod)
-                                        .addString(PROTECTED)
-                                        .addString(PUBLIC));
+                                iaomBugs.add(new IAOMBug(subClass, subClassMethod, PROTECTED, PUBLIC));
                             }
                             if (isPackagePrivate(superClassMethod)) {
                                 if (subClassMethod.isPublic()) {
-                                    bugReporter.reportBug(new BugInstance(this, BUG_TYPE, NORMAL_PRIORITY)
-                                            .addClassAndMethod(subClass, subClassMethod)
-                                            .addString(PACKAGE_PRIVATE)
-                                            .addString(PUBLIC));
+                                    iaomBugs.add(new IAOMBug(subClass, subClassMethod, PACKAGE_PRIVATE, PUBLIC));
                                 } else if (subClassMethod.isProtected()) {
-                                    bugReporter.reportBug(new BugInstance(this, BUG_TYPE, NORMAL_PRIORITY)
-                                            .addClassAndMethod(subClass, subClassMethod)
-                                            .addString(PACKAGE_PRIVATE)
-                                            .addString(PROTECTED));
+                                    iaomBugs.add(new IAOMBug(subClass, subClassMethod, PACKAGE_PRIVATE, PROTECTED));
                                 }
                             }
                         }
                     }
                 }
             }
         }
-    }
 
-    @Override
-    public void report() {
+        return iaomBugs.stream().filter(bug -> !containsBug(superClassBugs, bug)).collect(Collectors.toList());
     }
 
-    private boolean isPackagePrivate(Method method) {
-        return !(method.isPublic() || method.isProtected() || method.isPrivate());
+    private List<Method> getMethodsExceptConstructors(JavaClass javaClass) {
+        ArrayList<Method> methods = new ArrayList<>();
+        for (Method method : javaClass.getMethods()) {
+            if (!Const.CONSTRUCTOR_NAME.equals(method.getName())) {
+                methods.add(method);
+            }
+        }
+        return methods;
     }
 
     private boolean isAccessibleFromSubClass(Method superClassMethod, JavaClass subClass, JavaClass superClass) {
@@ -105,14 +116,8 @@ private boolean isOverridable(Method original, Method overrider) {
         return Objects.equals(original, overrider) && !original.isFinal();
     }
 
-    private List<Method> getMethodsExceptConstructors(JavaClass javaClass) {
-        ArrayList<Method> methods = new ArrayList<>();
-        for (Method method : javaClass.getMethods()) {
-            if (!Const.CONSTRUCTOR_NAME.equals(method.getName())) {
-                methods.add(method);
-            }
-        }
-        return methods;
+    private boolean isPackagePrivate(Method method) {
+        return !(method.isPublic() || method.isProtected() || method.isPrivate());
     }
 
     private boolean isCloneMethodFromCloneable(JavaClass javaClass, Method method) {
@@ -124,4 +129,32 @@ private boolean isCloneMethodFromCloneable(JavaClass javaClass, Method method) {
         }
     }
 
+    private boolean containsBug(List<IAOMBug> bugs, IAOMBug bug) {
+        for (IAOMBug other : bugs) {
+            if (bug.isSameMethodAndAccessibility(other)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    private static class IAOMBug {
+        final JavaClass clazz;
+        final Method method;
+        final String oldAccessibility;
+        final String newAccessibility;
+
+        IAOMBug(JavaClass clazz, Method method, String oldAccessibility, String newAccessibility) {
+            this.clazz = clazz;
+            this.method = method;
+            this.oldAccessibility = oldAccessibility;
+            this.newAccessibility = newAccessibility;
+        }
+
+        boolean isSameMethodAndAccessibility(IAOMBug other) {
+            return Objects.equals(method.getSignature(), other.method.getSignature())
+                    && Objects.equals(oldAccessibility, other.oldAccessibility)
+                    && Objects.equals(newAccessibility, other.newAccessibility);
+        }
+    }
 }
diff --git a/spotbugsTestCases/src/java/increasedAccessibilityOfMethods/CloneableImplementation.java b/spotbugsTestCases/src/java/increasedAccessibilityOfMethods/CloneableImplementation.java
@@ -1,7 +1,6 @@
 package increasedAccessibilityOfMethods;
 
 public class CloneableImplementation implements Cloneable {
-
     @Override
     public CloneableImplementation clone() {
         try {
@@ -10,5 +9,4 @@ public CloneableImplementation clone() {
             throw new AssertionError();
         }
     }
-
 }
diff --git a/spotbugsTestCases/src/java/increasedAccessibilityOfMethods/GenericSubClass.java b/spotbugsTestCases/src/java/increasedAccessibilityOfMethods/GenericSubClass.java
@@ -1,7 +1,6 @@
 package increasedAccessibilityOfMethods;
 
 public class GenericSubClass<K> extends GenericSuperClass<K> {
-
     @Override
     public String publicMethodWithGenericParameter(K parameter) {
         return "GenericSubClass.publicMethodWithGenericParameter";
@@ -31,5 +30,4 @@ protected String packagePrivateMethodToProtectedWithGenericParameter(K parameter
     String packagePrivateWithGenericParameter(K parameter) {
         return "GenericSubClass.packagePrivateWithGenericParameter";
     }
-
 }
diff --git a/spotbugsTestCases/src/java/increasedAccessibilityOfMethods/GenericSuperClass.java b/spotbugsTestCases/src/java/increasedAccessibilityOfMethods/GenericSuperClass.java
@@ -3,7 +3,6 @@
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 
 public class GenericSuperClass<T> {
-
     public String publicMethodWithGenericParameter(T parameter) {
         return "GenericSuperClass.publicMethodWithGenericParameter";
     }
@@ -32,5 +31,4 @@ String packagePrivateWithGenericParameter(T parameter) {
     private String privateMethodWithGenericParameter(T parameter) {
         return "GenericSuperClass.privateMethodWithGenericParameter";
     }
-
 }
diff --git a/spotbugsTestCases/src/java/increasedAccessibilityOfMethods/Interface.java b/spotbugsTestCases/src/java/increasedAccessibilityOfMethods/Interface.java
@@ -1,7 +1,5 @@
 package increasedAccessibilityOfMethods;
 
 public interface Interface {
-
     String packagePrivateInterfaceMethod();
-
 }
diff --git a/spotbugsTestCases/src/java/increasedAccessibilityOfMethods/InterfaceImplementation.java b/spotbugsTestCases/src/java/increasedAccessibilityOfMethods/InterfaceImplementation.java
@@ -1,10 +1,8 @@
 package increasedAccessibilityOfMethods;
 
 public class InterfaceImplementation implements Interface {
-
     @Override
     public String packagePrivateInterfaceMethod() {
         return "InterfaceImplementation.packagePrivateInterfaceMethod";
     }
-
 }
diff --git a/spotbugsTestCases/src/java/increasedAccessibilityOfMethods/SubClassFromSamePackage.java b/spotbugsTestCases/src/java/increasedAccessibilityOfMethods/SubClassFromSamePackage.java
@@ -1,7 +1,6 @@
 package increasedAccessibilityOfMethods;
 
 public class SubClassFromSamePackage extends SuperClass {
-
     public SubClassFromSamePackage() {
         super();
     }
@@ -35,5 +34,4 @@ public String superPackagePrivateMethodToPublic() {
     protected String superPackagePrivateMethodToProtected() {
         return "SubClassFromSamePackage.superPackagePrivateMethodToProtected";
     }
-
 }
diff --git a/spotbugsTestCases/src/java/increasedAccessibilityOfMethods/SubClassWithRepeatedOverride.java b/spotbugsTestCases/src/java/increasedAccessibilityOfMethods/SubClassWithRepeatedOverride.java
@@ -0,0 +1,17 @@
+package increasedAccessibilityOfMethods;
+
+public class SubClassWithRepeatedOverride extends SuperClassWithOverride {
+    public SubClassWithRepeatedOverride() {
+        super();
+    }
+
+    @Override
+    public String superPackagePrivateMethodToProtectedOverriddenInSuperClass() {
+        return "SubClassWithRepeatedOverride.superPackagePrivateMethodToProtectedOverriddenInSuperClass";
+    }
+
+    @Override
+    public String superProtectedMethodToPublicOverriddenInSuperClass() {
+        return "SubClassWithRepeatedOverride.superProtectedMethodToPublicOverriddenInSuperClass";
+    }
+}
diff --git a/spotbugsTestCases/src/java/increasedAccessibilityOfMethods/SuperClass.java b/spotbugsTestCases/src/java/increasedAccessibilityOfMethods/SuperClass.java
@@ -3,7 +3,6 @@
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 
 public class SuperClass extends SuperClassOfSuperClass {
-
     protected SuperClass() {
         super();
     }
@@ -36,5 +35,4 @@ String packagePrivate() {
     private String privateMethod() {
         return "SuperClass.privateMethod";
     }
-
 }