From c6825d6dbfcfd153ad2dc071d342c83663147b59 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 20 Feb 2020 16:44:24 +0100 Subject: [PATCH] [java] UselessOverridingMethod: consider package-level elevation as well --- .../design/UselessOverridingMethodRule.java | 37 ++++++++++++++++++- .../TransitiveSubclass.java | 2 +- .../other/DirectSubclassInOtherPackage.java | 15 ++++++++ .../other/OtherClassInOtherPackage.java | 16 ++++++++ .../design/xml/UselessOverridingMethod.xml | 21 ++++++++++- 5 files changed, 87 insertions(+), 4 deletions(-) create mode 100644 pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/design/uselessoverridingmethod/other/DirectSubclassInOtherPackage.java create mode 100644 pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/design/uselessoverridingmethod/other/OtherClassInOtherPackage.java diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/UselessOverridingMethodRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/UselessOverridingMethodRule.java index b9f5f4de80..f73a34ebad 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/UselessOverridingMethodRule.java +++ b/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> 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; } /** diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/design/uselessoverridingmethod/TransitiveSubclass.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/design/uselessoverridingmethod/TransitiveSubclass.java index d9c8465c4e..050665ec8e 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/design/uselessoverridingmethod/TransitiveSubclass.java +++ b/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(); } } diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/design/uselessoverridingmethod/other/DirectSubclassInOtherPackage.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/design/uselessoverridingmethod/other/DirectSubclassInOtherPackage.java new file mode 100644 index 0000000000..0585d7e4dd --- /dev/null +++ b/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(); + } +} diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/design/uselessoverridingmethod/other/OtherClassInOtherPackage.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/rule/design/uselessoverridingmethod/other/OtherClassInOtherPackage.java new file mode 100644 index 0000000000..80ec2a66d9 --- /dev/null +++ b/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(); + } +} diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/UselessOverridingMethod.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/UselessOverridingMethod.xml index f5dac830bc..e53b33f547 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/UselessOverridingMethod.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/UselessOverridingMethod.xml @@ -398,6 +398,25 @@ public class DirectSubclass extends BaseClass { ]]> + + [java] UselessOverridingMethod false positive when elevating access modifier #911 - direct different package, same visibility + 0 + + + [java] UselessOverridingMethod false positive when elevating access modifier #911 - transitive 0 @@ -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(); } }