[java] UselessOverridingMethod: consider package-level elevation as well

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/UselessOverridingMethodRule.java

@@ -7,9 +7,11 @@ package net.sourceforge.pmd.lang.java.rule.design;
 import static net.sourceforge.pmd.properties.PropertyFactory.booleanProperty;
 
 import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
 import java.util.ArrayList;
 import java.util.List;
 
+import net.sourceforge.pmd.RuleContext;
 import net.sourceforge.pmd.lang.ast.Node;
 import net.sourceforge.pmd.lang.java.ast.ASTAnnotation;
 import net.sourceforge.pmd.lang.java.ast.ASTArgumentList;
@@ -23,6 +25,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTMarkerAnnotation;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTName;
 import net.sourceforge.pmd.lang.java.ast.ASTNameList;
+import net.sourceforge.pmd.lang.java.ast.ASTPackageDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix;
 import net.sourceforge.pmd.lang.java.ast.ASTPrimarySuffix;
@@ -49,11 +52,17 @@ public class UselessOverridingMethodRule extends AbstractJavaRule {
             .desc("Ignore annotations")
             .build();
 
+    private String packageName;
 
     public UselessOverridingMethodRule() {
         definePropertyDescriptor(IGNORE_ANNOTATIONS_DESCRIPTOR);
     }
 
+    @Override
+    public void start(RuleContext ctx) {
+        packageName = "";
+    }
+
     @Override
     public Object visit(ASTClassOrInterfaceDeclaration clz, Object data) {
         if (clz.isInterface()) {
@@ -86,6 +95,12 @@ public class UselessOverridingMethodRule extends AbstractJavaRule {
         return false;
     }
 
+    @Override
+    public Object visit(ASTPackageDeclaration node, Object data) {
+        packageName = node.getPackageNameImage();
+        return super.visit(node, data);
+    }
+
     @Override
     public Object visit(ASTMethodDeclaration node, Object data) {
         // Can skip abstract methods and methods whose only purpose is to
@@ -229,7 +244,6 @@ public class UselessOverridingMethodRule extends AbstractJavaRule {
         }
 
         String overriddenMethodName = node.getName();
-        int overriddenModifiers = node.getModifiers();
 
         List<Class<?>> typeArguments = new ArrayList<>();
         for (ASTFormalParameter parameter : node.getFormalParameters()) {
@@ -256,7 +270,26 @@ public class UselessOverridingMethodRule extends AbstractJavaRule {
             }
             superType = superType.getSuperclass();
         }
-        return declaredMethod != null && overriddenModifiers != declaredMethod.getModifiers();
+
+        return declaredMethod != null && isElevatingAccessModifier(node, declaredMethod);
+    }
+
+    private boolean isElevatingAccessModifier(ASTMethodDeclaration overridingMethod, Method superMethod) {
+        String superPackageName = null;
+        Package p = superMethod.getDeclaringClass().getPackage();
+        if (p != null) {
+            superPackageName = p.getName();
+        }
+        // Note: can't simply compare superMethod.getModifiers() with overridingMethod.getModifiers()
+        // since AccessNode#PROTECTED != Modifier#PROTECTED.
+        boolean elevatingFromProtected = Modifier.isProtected(superMethod.getModifiers())
+                && !overridingMethod.isProtected();
+        boolean elevatingFromPackagePrivate = superMethod.getModifiers() == 0 && overridingMethod.getModifiers() != 0;
+        boolean elevatingIntoDifferentPackage = !packageName.equals(superPackageName);
+
+        return elevatingFromProtected
+                || elevatingFromPackagePrivate
+                || elevatingIntoDifferentPackage;
     }
 
     /**

pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/design/uselessoverridingmethod/TransitiveSubclass.java

@@ -7,7 +7,7 @@ package net.sourceforge.pmd.lang.java.rule.design.uselessoverridingmethod;
 public class TransitiveSubclass extends OtherSubclass {
 
     @Override
-    protected void doBase() {
+    public void doBase() {
         super.doBase();
     }
 }

pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/design/uselessoverridingmethod/other/DirectSubclassInOtherPackage.java

@@ -0,0 +1,15 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.java.rule.design.uselessoverridingmethod.other;
+
+import net.sourceforge.pmd.lang.java.rule.design.uselessoverridingmethod.BaseClass;
+
+public class DirectSubclassInOtherPackage extends BaseClass {
+
+    @Override
+    protected void doBase() {
+        super.doBase();
+    }
+}

pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/design/uselessoverridingmethod/other/OtherClassInOtherPackage.java

@@ -0,0 +1,16 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.java.rule.design.uselessoverridingmethod.other;
+
+public class OtherClassInOtherPackage {
+
+
+    public void foo() {
+        DirectSubclassInOtherPackage instance = new DirectSubclassInOtherPackage();
+        // this call is only possible, because DirectSubclassInOtherPackage makes this
+        // method available in this package as well.
+        instance.doBase();
+    }
+}

pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/UselessOverridingMethod.xml

@@ -398,6 +398,25 @@ public class DirectSubclass extends BaseClass {
         ]]></code>
     </test-code>
 
+    <test-code>
+        <description>[java] UselessOverridingMethod false positive when elevating access modifier #911 - direct different package, same visibility</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+package net.sourceforge.pmd.lang.java.rule.design.uselessoverridingmethod.other;
+
+import net.sourceforge.pmd.lang.java.rule.design.uselessoverridingmethod.BaseClass;
+
+public class DirectSubclassInOtherPackage extends BaseClass {
+
+
+    @Override
+    protected void doBase() {
+        super.doBase();
+    }
+}
+        ]]></code>
+    </test-code>
+
     <test-code>
         <description>[java] UselessOverridingMethod false positive when elevating access modifier #911 - transitive</description>
         <expected-problems>0</expected-problems>
@@ -407,7 +426,7 @@ package net.sourceforge.pmd.lang.java.rule.design.uselessoverridingmethod;
 public class TransitiveSubclass extends OtherSubclass {
 
     @Override
-    protected void doBase() {
+    public void doBase() {
         super.doBase();
     }
 }