Merge pull request #3757 from oowekyala:java-override-resolution

[java] Improve ASTMethodDeclaration::isOverridden #3757

* pr-3757:
  Cleanups
  Add tests for UselessOverridingMethod
  Cleanups
  Cleanup UselessOverridingMethod
  Cleanups
  Extract override resolution logic from MissingOverrid rule

pmd-core/src/main/java/net/sourceforge/pmd/internal/util/IteratorUtil.java

@@ -536,14 +536,20 @@ public final class IteratorUtil {
         }
 
         protected final void setNext(T t) {
+            assert state == null : "Must call exactly one of setNext or done";
             next = t;
             state = State.READY;
         }
 
         protected final void done() {
+            assert state == null : "Must call exactly one of setNext or done";
             state = State.DONE;
         }
 
+        /**
+         * Compute the next element. Implementations must call either
+         * {@link #done()} or {@link #setNext(Object)} exactly once.
+         */
         protected abstract void computeNext();
 
         enum State {

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTMethodDeclaration.java

@@ -9,6 +9,8 @@ import org.checkerframework.checker.nullness.qual.Nullable;
 
 import net.sourceforge.pmd.lang.ast.impl.javacc.JavaccToken;
 import net.sourceforge.pmd.lang.java.symbols.JMethodSymbol;
+import net.sourceforge.pmd.lang.java.types.JMethodSig;
+import net.sourceforge.pmd.lang.java.types.TypeSystem;
 import net.sourceforge.pmd.lang.java.types.TypeTestUtil;
 import net.sourceforge.pmd.lang.rule.xpath.DeprecatedAttribute;
 
@@ -41,11 +43,15 @@ import net.sourceforge.pmd.lang.rule.xpath.DeprecatedAttribute;
  */
 public final class ASTMethodDeclaration extends AbstractMethodOrConstructorDeclaration<JMethodSymbol> {
 
+    /**
+     * Populated by {@link OverrideResolutionPass}.
+     */
+    private JMethodSig overriddenMethod = null;
+
     ASTMethodDeclaration(int id) {
         super(id);
     }
 
-
     @Override
     protected <P, R> R acceptVisitor(JavaVisitor<? super P, ? extends R> visitor, P data) {
         return visitor.visit(this, data);
@@ -53,13 +59,26 @@ public final class ASTMethodDeclaration extends AbstractMethodOrConstructorDecla
 
     /**
      * Returns true if this method is overridden.
-     * TODO for now, this just checks for an @Override annotation,
-     *   but this should definitely do what MissingOverride does.
-     *   This could be useful in UnusedPrivateMethod (to check not only private methods),
-     *   and also UselessOverridingMethod, and overall many many rules.
      */
     public boolean isOverridden() {
-        return isAnnotationPresent(Override.class);
+        return overriddenMethod != null;
+    }
+
+    /**
+     * Returns the signature of the method this method overrides in a
+     * supertype. Note that this method may be implementing several methods
+     * of super-interfaces at once, in that case, an arbitrary one is returned.
+     *
+     * <p>If the method has an {@link Override} annotation, but we couldn't
+     * resolve any method that is actually implemented, this will return
+     * {@link TypeSystem#UNRESOLVED_METHOD}.
+     */
+    public JMethodSig getOverriddenMethod() {
+        return overriddenMethod;
+    }
+
+    void setOverriddenMethod(JMethodSig overriddenMethod) {
+        this.overriddenMethod = overriddenMethod;
     }
 
     @Override

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/InternalApiBridge.java

@@ -144,6 +144,12 @@ public final class InternalApiBridge {
             });
     }
 
+    public static void overrideResolution(JavaAstProcessor processor, ASTCompilationUnit root) {
+        root.descendants(ASTAnyTypeDeclaration.class)
+            .crossFindBoundaries()
+            .forEach(OverrideResolutionPass::resolveOverrides);
+    }
+
     public static @Nullable JTypeMirror getTypeMirrorInternal(TypeNode node) {
         return ((AbstractJavaTypeNode) node).getTypeMirrorInternal();
     }

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/OverrideResolutionPass.java

@@ -0,0 +1,126 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.java.ast;
+
+import java.util.BitSet;
+import java.util.HashMap;
+import java.util.LinkedHashSet;
+import java.util.Map;
+import java.util.Set;
+
+import net.sourceforge.pmd.lang.java.symbols.JMethodSymbol;
+import net.sourceforge.pmd.lang.java.symbols.table.internal.SuperTypesEnumerator;
+import net.sourceforge.pmd.lang.java.types.JClassType;
+import net.sourceforge.pmd.lang.java.types.JMethodSig;
+import net.sourceforge.pmd.lang.java.types.TypeOps;
+
+
+/**
+ * Populates method declarations with the method they override.
+ *
+ * @author Clément Fournier
+ * @since 7.0.0
+ */
+class OverrideResolutionPass {
+
+    private OverrideResolutionPass() {
+
+    }
+
+    static void resolveOverrides(ASTAnyTypeDeclaration node) {
+        // collect methods that may override another method (non private, non static)
+
+        RelevantMethodSet relevantMethods = new RelevantMethodSet(node.getTypeMirror());
+
+        for (ASTMethodDeclaration methodDecl : node.getDeclarations(ASTMethodDeclaration.class)) {
+            relevantMethods.addIfRelevant(methodDecl);
+        }
+
+        if (relevantMethods.tracked.isEmpty()) {
+            return;
+        }
+
+        // stream all methods of supertypes
+        SuperTypesEnumerator.ALL_STRICT_SUPERTYPES
+            .stream(node.getTypeMirror())
+            // Filter down to those that may be overridden by one of the possible violations
+            // This considers name, arity, and accessibility
+            //                                      vvvvvvvvvvvvvvvvvvvvvvvvvvv
+            .flatMap(st -> st.streamDeclaredMethods(relevantMethods::isRelevant))
+            // For those methods, a simple override-equivalence check is enough,
+            // because we already know they're accessible, and declared in a supertype
+            .forEach(relevantMethods::findMethodOverridingThisSig);
+    }
+
+
+    /**
+     * This does a prefilter, so that we only collect methods of supertypes
+     * that may be overridden by a sub method. For a method to be potentially
+     * a super method, it must have same arity
+     */
+    private static final class RelevantMethodSet {
+
+        // name to considered arities
+        private final Map<String, BitSet> map = new HashMap<>();
+        // nodes that may be violations
+        private final Set<ASTMethodDeclaration> tracked = new LinkedHashSet<>();
+
+        private final JClassType site;
+
+        private RelevantMethodSet(JClassType site) {
+            this.site = site;
+        }
+
+
+        // add a method if it may be overriding another
+        // this builds the data structure for isRelevant to work
+        void addIfRelevant(ASTMethodDeclaration m) {
+            if (m.getModifiers().hasAny(JModifier.STATIC, JModifier.PRIVATE)) {
+                // cannot override anything
+                return;
+            } else if (m.isAnnotationPresent(Override.class)) {
+                // will be overwritten if we find it
+                m.setOverriddenMethod(m.getTypeSystem().UNRESOLVED_METHOD);
+            }
+            // then add it
+            BitSet aritySet = map.computeIfAbsent(m.getName(), n -> new BitSet(m.getArity() + 1));
+            aritySet.set(m.getArity());
+            tracked.add(m);
+        }
+
+        // we use this to only consider methods that may produce a violation,
+        // among the supertype methods
+        boolean isRelevant(JMethodSymbol superMethod) {
+            if (!TypeOps.isOverridableIn(superMethod, site.getSymbol())) {
+                return false;
+            }
+            BitSet aritySet = map.get(superMethod.getSimpleName());
+            return aritySet != null && aritySet.get(superMethod.getArity());
+        }
+
+        // if the superSig, which comes from a supertype, is overridden
+        // by a relevant method, set the overridden method.
+        void findMethodOverridingThisSig(JMethodSig superSig) {
+            ASTMethodDeclaration subSig = null;
+            for (ASTMethodDeclaration it : tracked) {
+                // note: we don't use override-equivalence, the definition
+                // of an override uses the concept of sub-signature instead,
+                // which is slightly different. We could also use TypeOps.overrides
+                // but at this point we already know much of what that method checks.
+                // https://docs.oracle.com/javase/specs/jls/se15/html/jls-8.html#jls-8.4.8.1
+                if (TypeOps.isSubSignature(it.getGenericSignature(), superSig)) {
+                    subSig = it;
+                    // we assume there is a single relevant method that may match,
+                    // otherwise it would be a compile-time error
+                    break;
+                }
+            }
+            if (subSig != null) {
+                subSig.setOverriddenMethod(superSig);
+                tracked.remove(subSig); // speedup the check for later
+            }
+        }
+    }
+}

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/internal/JavaAstProcessor.java

@@ -141,12 +141,15 @@ public final class JavaAstProcessor {
         // Now symbols are on the relevant nodes
         this.symResolver = SymbolResolver.layer(knownSyms, this.symResolver);
 
+        // this needs to be initialized before the symbol table resolution
+        // as scopes depend on type resolution in some cases.
         InternalApiBridge.initTypeResolver(acu, this, typeInferenceLogger);
 
         bench("2. Symbol table resolution", () -> SymbolTableResolver.traverse(this, acu));
         bench("3. AST disambiguation", () -> InternalApiBridge.disambigWithCtx(NodeStream.of(acu), ReferenceCtx.root(this, acu)));
         bench("4. Comment assignment", () -> InternalApiBridge.assignComments(acu));
         bench("5. Usage resolution", () -> InternalApiBridge.usageResolution(this, acu));
+        bench("6. Override resolution", () -> InternalApiBridge.overrideResolution(this, acu));
     }
 
     public TypeSystem getTypeSystem() {

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/MissingOverrideRule.java

@@ -4,26 +4,9 @@
 
 package net.sourceforge.pmd.lang.java.rule.bestpractices;
 
-import java.util.BitSet;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.LinkedHashSet;
-import java.util.Map;
-import java.util.Set;
-import java.util.stream.Collector;
-
-import net.sourceforge.pmd.RuleContext;
-import net.sourceforge.pmd.lang.java.ast.ASTAnyTypeDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
-import net.sourceforge.pmd.lang.java.ast.JModifier;
-import net.sourceforge.pmd.lang.java.ast.JavaNode;
 import net.sourceforge.pmd.lang.java.ast.internal.PrettyPrintingUtil;
 import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
-import net.sourceforge.pmd.lang.java.symbols.JMethodSymbol;
-import net.sourceforge.pmd.lang.java.symbols.table.internal.SuperTypesEnumerator;
-import net.sourceforge.pmd.lang.java.types.JClassType;
-import net.sourceforge.pmd.lang.java.types.JMethodSig;
-import net.sourceforge.pmd.lang.java.types.TypeOps;
 
 
 /**
@@ -35,124 +18,14 @@ import net.sourceforge.pmd.lang.java.types.TypeOps;
 public class MissingOverrideRule extends AbstractJavaRulechainRule {
 
     public MissingOverrideRule() {
-        super(ASTAnyTypeDeclaration.class);
+        super(ASTMethodDeclaration.class);
     }
 
     @Override
-    public Object visitJavaNode(JavaNode node, Object data) {
-        visitTypeDecl((ASTAnyTypeDeclaration) node, (RuleContext) data);
+    public Object visit(ASTMethodDeclaration node, Object data) {
+        if (node.isOverridden() && !node.isAnnotationPresent(Override.class)) {
+            addViolation(data, node, new Object[] { PrettyPrintingUtil.displaySignature(node) });
+        }
         return data;
     }
-
-    private void visitTypeDecl(ASTAnyTypeDeclaration node, RuleContext data) {
-        // collect methods that may be violations, ie:
-        // - may override another method (non private, non static)
-        // - not already annotated @Override
-
-        RelevantMethodSet relevantMethods = new RelevantMethodSet(node.getTypeMirror());
-
-        for (ASTMethodDeclaration methodDecl : node.getDeclarations(ASTMethodDeclaration.class)) {
-            relevantMethods.addIfRelevant(methodDecl);
-        }
-
-        if (relevantMethods.tracked.isEmpty()) {
-            return;
-        }
-
-        Set<ASTMethodDeclaration> violatingMethods =
-            // stream all methods of supertypes
-            SuperTypesEnumerator.ALL_STRICT_SUPERTYPES
-                .stream(node.getTypeMirror())
-                // Filter down to those that may be overridden by one of the possible violations
-                // This considers name, arity, and accessibility
-                //                                      vvvvvvvvvvvvvvvvvvvvvvvvvvv
-                .flatMap(st -> st.streamDeclaredMethods(relevantMethods::isRelevant))
-                // For those methods, a simple override-equivalence check is enough,
-                // because we already know they're accessible, and declared in a supertype
-                .collect(relevantMethods.overriddenRelevantMethodsCollector());
-
-        for (ASTMethodDeclaration violatingMethod : violatingMethods) {
-            addViolation(data, violatingMethod, new Object[] { PrettyPrintingUtil.displaySignature(violatingMethod) });
-        }
-    }
-
-    /**
-     * This does a prefilter, so that we only collect methods of supertypes
-     * that may be overridden by a sub method. For a method to be potentially
-     * a super method, it must have same arity
-     */
-    private static final class RelevantMethodSet {
-
-        // name to considered arities
-        private final Map<String, BitSet> map = new HashMap<>();
-        // nodes that may be violations
-        private final Set<ASTMethodDeclaration> tracked = new LinkedHashSet<>();
-
-        private final JClassType site;
-
-        private RelevantMethodSet(JClassType site) {
-            this.site = site;
-        }
-
-
-        // add a method if it may be a violation
-        // this builds the data structure for isRelevant to work
-        void addIfRelevant(ASTMethodDeclaration m) {
-            if (m.isAnnotationPresent(Override.class)
-                || m.getModifiers().hasAny(JModifier.STATIC, JModifier.PRIVATE)) {
-                return;
-            }
-            // then add it
-            BitSet aritySet = map.computeIfAbsent(m.getName(), n -> new BitSet(m.getArity() + 1));
-            aritySet.set(m.getArity());
-            tracked.add(m);
-        }
-
-        // we use this to only consider methods that may produce a violation,
-        // among the supertype methods
-        boolean isRelevant(JMethodSymbol superMethod) {
-            if (!TypeOps.isOverridableIn(superMethod, site.getSymbol())) {
-                return false;
-            }
-            BitSet aritySet = map.get(superMethod.getSimpleName());
-            return aritySet != null && aritySet.get(superMethod.getArity());
-        }
-
-        // then, if the superSig, which comes from a supertype, is overridden
-        // by a relevant method (ie a method that is a violation), then that
-        // node truly is a violation, and is added to the output set.
-        void addToSetIfIsOverridden(Set<ASTMethodDeclaration> relevantOverridingMethods,
-                                    JMethodSig superSig) {
-            ASTMethodDeclaration subSig = null;
-            for (ASTMethodDeclaration it : tracked) {
-                // note: we don't use override-equivalence, the definition
-                // of an override uses the concept of sub-signature instead,
-                // which is slightly different. We could also use TypeOps.overrides
-                // but at this point we already know much of what that method checks.
-                // https://docs.oracle.com/javase/specs/jls/se15/html/jls-8.html#jls-8.4.8.1
-                if (TypeOps.isSubSignature(it.getGenericSignature(), superSig)) {
-                    subSig = it;
-                    // we assume there is a single relevant method that may match,
-                    // otherwise it would be a compile-time error
-                    break;
-                }
-            }
-            if (subSig != null) {
-                relevantOverridingMethods.add(subSig);
-                tracked.remove(subSig); // speedup the check for later
-            }
-        }
-
-        Collector<JMethodSig, ?, Set<ASTMethodDeclaration>> overriddenRelevantMethodsCollector() {
-            return Collector.of(
-                HashSet<ASTMethodDeclaration>::new,
-                this::addToSetIfIsOverridden,
-                (map1, map2) -> {
-                    throw new UnsupportedOperationException("Dont use a parallel stream");
-                },
-                set -> set
-            );
-        }
-
-    }
 }

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UnusedPrivateMethodRule.java

@@ -70,7 +70,6 @@ public class UnusedPrivateMethodRule extends AbstractIgnoredAnnotationRule {
                 .filter(it -> it.getVisibility() == Visibility.V_PRIVATE)
                 .filter(it -> !hasIgnoredAnnotation(it)
                         && !hasExcludedName(it) 
-                        && !it.isAnnotationPresent(Override.class)
                         && !(it.getArity() == 0 && methodsUsedByAnnotations.contains(it.getName())))
                 .toStream()
                 .collect(Collectors.groupingBy(ASTMethodDeclaration::getName, HashMap::new, CollectionUtil.toMutableSet()));

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

@@ -4,228 +4,117 @@
 
 package net.sourceforge.pmd.lang.java.rule.design;
 
+import static net.sourceforge.pmd.lang.java.ast.JModifier.FINAL;
+import static net.sourceforge.pmd.lang.java.ast.JModifier.NATIVE;
+import static net.sourceforge.pmd.lang.java.ast.JModifier.SYNCHRONIZED;
 import static net.sourceforge.pmd.properties.PropertyFactory.booleanProperty;
 
 import java.lang.reflect.Modifier;
 
-import org.checkerframework.checker.nullness.qual.NonNull;
-import org.checkerframework.checker.nullness.qual.Nullable;
-
-import net.sourceforge.pmd.RuleContext;
-import net.sourceforge.pmd.lang.ast.Node;
 import net.sourceforge.pmd.lang.java.ast.ASTArgumentList;
-import net.sourceforge.pmd.lang.java.ast.ASTBlock;
-import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration;
-import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceType;
-import net.sourceforge.pmd.lang.java.ast.ASTExpression;
 import net.sourceforge.pmd.lang.java.ast.ASTExpressionStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTFormalParameter;
-import net.sourceforge.pmd.lang.java.ast.ASTFormalParameters;
+import net.sourceforge.pmd.lang.java.ast.ASTList;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodCall;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
-import net.sourceforge.pmd.lang.java.ast.ASTPackageDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTReturnStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTStatement;
 import net.sourceforge.pmd.lang.java.ast.ASTSuperExpression;
-import net.sourceforge.pmd.lang.java.ast.ASTThrowsList;
-import net.sourceforge.pmd.lang.java.ast.ASTType;
-import net.sourceforge.pmd.lang.java.ast.ASTVariableAccess;
-import net.sourceforge.pmd.lang.java.ast.AccessNode.Visibility;
-import net.sourceforge.pmd.lang.java.ast.JModifier;
-import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
-import net.sourceforge.pmd.lang.java.types.JMethodSig;
+import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
+import net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil;
+import net.sourceforge.pmd.lang.java.symbols.JExecutableSymbol;
+import net.sourceforge.pmd.lang.java.symbols.JMethodSymbol;
 import net.sourceforge.pmd.lang.java.types.OverloadSelectionResult;
 import net.sourceforge.pmd.lang.java.types.TypeTestUtil;
 import net.sourceforge.pmd.properties.PropertyDescriptor;
 
-/**
- * @author Romain Pelisse, bugfix for [ 1522517 ] False +:
- *         UselessOverridingMethod
- */
-public class UselessOverridingMethodRule extends AbstractJavaRule {
-    private static final String CLONE_METHOD_NAME = "clone";
+public class UselessOverridingMethodRule extends AbstractJavaRulechainRule {
 
     // TODO extend AbstractIgnoredAnnotationRule node
     // TODO ignore if there is javadoc
     private static final PropertyDescriptor<Boolean> IGNORE_ANNOTATIONS_DESCRIPTOR =
-            booleanProperty("ignoreAnnotations")
+        booleanProperty("ignoreAnnotations")
             .defaultValue(false)
-            .desc("Ignore annotations")
+            .desc("Ignore methods that have annotations (except @Override)")
             .build();
 
-    private String packageName;
 
     public UselessOverridingMethodRule() {
+        super(ASTMethodDeclaration.class);
         definePropertyDescriptor(IGNORE_ANNOTATIONS_DESCRIPTOR);
     }
 
-    @Override
-    public void start(RuleContext ctx) {
-        packageName = "";
-    }
-
-    @Override
-    public Object visit(ASTClassOrInterfaceDeclaration clz, Object data) {
-        if (clz.isInterface()) {
-            return data;
-        }
-        return super.visit(clz, data);
-    }
-
-    // TODO: this method should be externalize into an utility class, shouldn't it ?
-    private boolean isMethodResultType(ASTMethodDeclaration node, Class<?> resultType) {
-        ASTType type = node.getResultTypeNode();
-        return TypeTestUtil.isA(resultType, type);
-    }
-
-    // TODO: this method should be externalize into an utility class, shouldn't it ?
-    private boolean isMethodThrowingType(ASTMethodDeclaration node, Class<? extends Exception> exceptionType) {
-        @Nullable ASTThrowsList thrownExceptions = node.getThrowsList();
-        if (thrownExceptions != null) {
-            for (ASTClassOrInterfaceType type : thrownExceptions) {
-                if (TypeTestUtil.isA(exceptionType, type)) {
-                    return true;
-                }
-            }
-        }
-        return false;
-    }
-
-    @Override
-    public Object visit(ASTPackageDeclaration node, Object data) {
-        packageName = node.getName();
-        return super.visit(node, data);
-    }
-
     @Override
     public Object visit(ASTMethodDeclaration node, Object data) {
-        // Can skip abstract methods and methods whose only purpose is to
-        // guarantee that the inherited method is not changed by finalizing
-        // them.
-        if (node.getModifiers().hasAny(JModifier.ABSTRACT, JModifier.FINAL, JModifier.NATIVE, JModifier.SYNCHRONIZED)) {
-            return super.visit(node, data);
-        }
-        // We can also skip the 'clone' method as they are generally
-        // 'useless' but as it is considered a 'good practice' to
-        // implement them anyway ( see bug 1522517)
-        if (isCloneMethod(node)) {
-            return super.visit(node, data);
-        }
-
-        ASTBlock block = node.getBody();
-        // Only process functions with one BlockStatement
-        if (block.getNumChildren() != 1 || block.descendants(ASTStatement.class).count() != 1) {
-            return super.visit(node, data);
-        }
-
-        Node statement = block.getChild(0);
-        if (statement.getNumChildren() == 0) {
-            return super.visit(node, data); // skips empty return statements
+        if (!node.isOverridden()
+            || node.getBody() == null
+            // Can skip methods which are final or have new behavior (synchronized, native)
+            || node.getModifiers().hasAny(FINAL, NATIVE, SYNCHRONIZED)
+            // We can also skip the 'clone' method as they are generally
+            // 'useless' but as it is considered a 'good practice' to
+            // implement them anyway ( see bug 1522517)
+            || JavaRuleUtil.isCloneMethod(node)) {
+            return null;
         }
 
+        // skip annotated methods
         if (!getProperty(IGNORE_ANNOTATIONS_DESCRIPTOR)
-                && node.getDeclaredAnnotations().any(it -> !TypeTestUtil.isA(Override.class, it))) {
+            && node.getDeclaredAnnotations().any(it -> !TypeTestUtil.isA(Override.class, it))) {
+            return null;
+        }
+
+        ASTStatement statement = ASTList.singleOrNull(node.getBody());
+        // Only process functions with one statement
+        if (statement == null) {
             return super.visit(node, data);
         }
 
-        // merely calling super.foo() or returning super.foo()
-        ASTMethodCall superMethodCall = null;
         if ((statement instanceof ASTExpressionStatement || statement instanceof ASTReturnStatement)
-                && statement.getNumChildren() == 1
-                && statement.getChild(0) instanceof ASTMethodCall) {
+            && statement.getNumChildren() == 1
+            && statement.getChild(0) instanceof ASTMethodCall) {
+
+            // merely calling super.foo() or returning super.foo()
             ASTMethodCall methodCall = (ASTMethodCall) statement.getChild(0);
-            if (methodCall.getQualifier() instanceof ASTSuperExpression) {
-                superMethodCall = methodCall;
-            }
-        }
+            if (methodCall.getQualifier() instanceof ASTSuperExpression
+                && methodCall.getArguments().size() == node.getArity()
+                // might be disambiguating: Interface.super.foo()
+                && JavaRuleUtil.isUnqualifiedSuper(methodCall.getQualifier())) {
 
-        if (superMethodCall == null) {
-            return super.visit(node, data);
-        }
-
-        if (!isSuperCallSameMethod(node, superMethodCall)) {
-            return super.visit(node, data);
-        }
-
-        if (modifiersChanged(node, superMethodCall)) {
-            return super.visit(node, data);
-        }
-
-        // All arguments are passed through directly or there were no arguments
-        addViolation(data, node);
-
-        return super.visit(node, data);
-    }
-
-    private boolean isSuperCallSameMethod(ASTMethodDeclaration node, ASTMethodCall methodCall) {
-        @NonNull
-        ASTFormalParameters formalParameters = node.getFormalParameters();
-        @NonNull
-        ASTArgumentList arguments = methodCall.getArguments();
-
-        OverloadSelectionResult overloadSelectionInfo = methodCall.getOverloadSelectionInfo();
-        JMethodSig methodType = overloadSelectionInfo.getMethodType();
-
-        if (node.getName().equals(methodCall.getMethodName())
-                && formalParameters.size() == arguments.size()) {
-            // simple case - no args
-            if (formalParameters.size() == 0) {
-                return true;
-            }
-
-            // compare each arg
-            for (int i = 0; i < node.getArity(); i++) {
-                ASTFormalParameter formalParam = formalParameters.get(i);
-                ASTExpression arg = arguments.get(i);
-
-                if (!(arg instanceof ASTVariableAccess)) {
-                    return false;
-                }
-                ASTVariableAccess varAccess = (ASTVariableAccess) arg;
-                if (!formalParam.getVarId().getName().equals(varAccess.getName())) {
-                    return false;
-                }
-                // check the type - must be equal for overwrites, but could be different for overloads
-                if (!formalParam.getTypeMirror().equals(methodType.getFormalParameters().get(i))) {
-                    return false;
+                OverloadSelectionResult overload = methodCall.getOverloadSelectionInfo();
+                if (!overload.isFailed()
+                    // note: don't compare symbols, as the equals method for method symbols is broken for now
+                    && overload.getMethodType().equals(node.getOverriddenMethod())
+                    && sameModifiers(node.getOverriddenMethod().getSymbol(), node.getSymbol())
+                    && argumentsAreUnchanged(node, methodCall)) {
+                    addViolation(data, node);
                 }
             }
-
-            // now all args matched
-            return true;
         }
-        return false;
+        return null;
     }
 
-    private boolean isCloneMethod(ASTMethodDeclaration node) {
-        boolean isCloneAndPublic = CLONE_METHOD_NAME.equals(node.getName()) && node.getVisibility() == Visibility.V_PUBLIC;
-        boolean hasNoParameters = node.getArity() == 0;
-        return isCloneAndPublic
-                && hasNoParameters
-                && this.isMethodResultType(node, Object.class)
-                && this.isMethodThrowingType(node, CloneNotSupportedException.class);
+    private boolean argumentsAreUnchanged(ASTMethodDeclaration node, ASTMethodCall methodCall) {
+        ASTArgumentList arg = methodCall.getArguments();
+        int i = 0;
+        for (ASTFormalParameter formal : node.getFormalParameters()) {
+            if (!JavaRuleUtil.isReferenceToVar(arg.getChild(i), formal.getVarId().getSymbol())) {
+                return false;
+            }
+            i++;
+        }
+        return true;
     }
 
-    private boolean modifiersChanged(ASTMethodDeclaration node, ASTMethodCall superMethodCall) {
-        JMethodSig declaredMethod = superMethodCall.getOverloadSelectionInfo().getMethodType();
-        return declaredMethod != null && isElevatingAccessModifier(node, declaredMethod);
+    private boolean sameModifiers(JExecutableSymbol superMethod, JMethodSymbol subMethod) {
+        int visibilityMask = Modifier.PUBLIC | Modifier.PRIVATE | Modifier.PROTECTED;
+        return (visibilityMask & subMethod.getModifiers()) == (visibilityMask & superMethod.getModifiers())
+            // making visible in another package
+            && !isProtectedElevatingVisibility(superMethod, subMethod);
     }
 
-    private boolean isElevatingAccessModifier(ASTMethodDeclaration overridingMethod, JMethodSig superMethod) {
-        String superPackageName = superMethod.getDeclaringType().getSymbol().getPackageName();
-
-        // Note: can't simply compare superMethod.getModifiers() with overridingMethod.getModifiers()
-        // since AccessNode#PROTECTED != Modifier#PROTECTED.
-        boolean elevatingFromProtected = Modifier.isProtected(superMethod.getModifiers())
-                && overridingMethod.getVisibility() != Visibility.V_PROTECTED;
-        boolean elevatingFromPackagePrivate = superMethod.getModifiers() == 0
-            && !overridingMethod.getModifiers().getExplicitModifiers().isEmpty();
-        boolean elevatingIntoDifferentPackage = !packageName.equals(superPackageName)
-                && !Modifier.isPublic(superMethod.getModifiers());
-
-        return elevatingFromProtected
-                || elevatingFromPackagePrivate
-                || elevatingIntoDifferentPackage;
+    private boolean isProtectedElevatingVisibility(JExecutableSymbol superMethod, JMethodSymbol subMethod) {
+        return Modifier.isProtected(subMethod.getModifiers())
+            && !subMethod.getPackageName().equals(superMethod.getPackageName());
     }
 
 }

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/documentation/CommentRequiredRule.java

@@ -158,7 +158,7 @@ public class CommentRequiredRule extends AbstractJavaRulechainRule {
 
     @Override
     public Object visit(ASTMethodDeclaration decl, Object data) {
-        if (isAnnotatedOverride(decl)) {
+        if (decl.isOverridden()) {
             checkCommentMeetsRequirement(data, decl, OVERRIDE_CMT_DESCRIPTOR);
         } else if (JavaRuleUtil.isGetterOrSetter(decl)) {
             checkCommentMeetsRequirement(data, decl, ACCESSOR_CMT_DESCRIPTOR);
@@ -178,11 +178,6 @@ public class CommentRequiredRule extends AbstractJavaRulechainRule {
     }
 
 
-    private boolean isAnnotatedOverride(ASTMethodDeclaration decl) {
-        return decl.isAnnotationPresent(Override.class);
-    }
-
-
     @Override
     public Object visit(ASTFieldDeclaration decl, Object data) {
         if (JavaRuleUtil.isSerialVersionUID(decl)) {

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloneMethodMustImplementCloneableRule.java

@@ -10,6 +10,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
 import net.sourceforge.pmd.lang.java.ast.ASTThrowStatement;
 import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
+import net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil;
 import net.sourceforge.pmd.lang.java.types.TypeTestUtil;
 
 /**
@@ -29,7 +30,7 @@ public class CloneMethodMustImplementCloneableRule extends AbstractJavaRulechain
 
     @Override
     public Object visit(final ASTMethodDeclaration node, final Object data) {
-        if (!isCloneMethod(node)) {
+        if (!JavaRuleUtil.isCloneMethod(node)) {
             return data;
         }
         ASTBlock body = node.getBody();
@@ -57,7 +58,4 @@ public class CloneMethodMustImplementCloneableRule extends AbstractJavaRulechain
                    .nonEmpty();
     }
 
-    private static boolean isCloneMethod(final ASTMethodDeclaration method) {
-        return "clone".equals(method.getName()) && method.getFormalParameters().size() == 0;
-    }
 }

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/ConstructorCallsOverridableMethodRule.java

@@ -22,6 +22,7 @@ import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
 import net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil;
 import net.sourceforge.pmd.lang.java.symbols.JExecutableSymbol;
 import net.sourceforge.pmd.lang.java.symbols.JMethodSymbol;
+import net.sourceforge.pmd.lang.java.types.JMethodSig;
 import net.sourceforge.pmd.lang.java.types.OverloadSelectionResult;
 
 /**
@@ -65,12 +66,9 @@ public final class ConstructorCallsOverridableMethodRule extends AbstractJavaRul
         for (ASTMethodCall call : node.getBody().descendants(ASTMethodCall.class)) {
             JMethodSymbol unsafetyReason = getUnsafetyReason(call, TreePVector.empty());
             if (unsafetyReason != null) {
-                String message;
-                if (unsafetyReason.equals(call.getOverloadSelectionInfo().getMethodType().getSymbol())) {
-                    message = MESSAGE;
-                } else {
-                    message = MESSAGE_TRANSITIVE;
-                }
+                JMethodSig overload = call.getOverloadSelectionInfo().getMethodType();
+                JMethodSig unsafeMethod = call.getTypeSystem().sigOf(unsafetyReason);
+                String message = unsafeMethod.equals(overload) ? MESSAGE : MESSAGE_TRANSITIVE;
                 addViolationWithMessage(data, call, message, new Object[] { PrettyPrintingUtil.prettyPrintOverload(unsafetyReason) });
             }
         }

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/ProperCloneImplementationRule.java

@@ -12,6 +12,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTConstructorCall;
 import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration;
 import net.sourceforge.pmd.lang.java.ast.JModifier;
 import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
+import net.sourceforge.pmd.lang.java.rule.internal.JavaRuleUtil;
 import net.sourceforge.pmd.lang.java.symbols.JClassSymbol;
 
 public class ProperCloneImplementationRule extends AbstractJavaRulechainRule {
@@ -22,7 +23,7 @@ public class ProperCloneImplementationRule extends AbstractJavaRulechainRule {
 
     @Override
     public Object visit(ASTMethodDeclaration method, Object data) {
-        if (isCloneMethod(method) && isNotAbstractMethod(method)) {
+        if (JavaRuleUtil.isCloneMethod(method) && !method.isAbstract()) {
             ASTAnyTypeDeclaration enclosingType = method.getEnclosingType();
             if (isNotFinal(enclosingType) && hasAnyAllocationOfClass(method, enclosingType)) {
                 addViolation(data, method);
@@ -31,14 +32,6 @@ public class ProperCloneImplementationRule extends AbstractJavaRulechainRule {
         return data;
     }
 
-    private boolean isCloneMethod(ASTMethodDeclaration method) {
-        return "clone".equals(method.getName()) && method.getArity() == 0;
-    }
-
-    private boolean isNotAbstractMethod(ASTMethodDeclaration method) {
-        return !method.isAbstract();
-    }
-
     private boolean isNotFinal(ASTAnyTypeDeclaration classOrInterfaceDecl) {
         return !classOrInterfaceDecl.hasModifiers(JModifier.FINAL);
     }

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/internal/JavaRuleUtil.java

@@ -754,6 +754,10 @@ public final class JavaRuleUtil {
         return e instanceof ASTThisExpression && ((ASTThisExpression) e).getQualifier() == null;
     }
 
+    public static boolean isUnqualifiedSuper(ASTExpression e) {
+        return e instanceof ASTSuperExpression && ((ASTSuperExpression) e).getQualifier() == null;
+    }
+
     /**
      * Returns true if the expression is a {@link ASTNamedReferenceExpr}
      * that references any of the symbol in the set.
@@ -997,4 +1001,12 @@ public final class JavaRuleUtil {
         }
         return false;
     }
+
+    public static boolean isCloneMethod(ASTMethodDeclaration node) {
+        // this is enough as in valid code, this signature overrides Object#clone
+        // and the other things like visibility are checked by the compiler
+        return "clone".equals(node.getName())
+            && node.getArity() == 0
+            && !node.isStatic();
+    }
 }

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/table/internal/SuperTypesEnumerator.java

@@ -6,6 +6,8 @@ package net.sourceforge.pmd.lang.java.symbols.table.internal;
 
 import static java.util.Collections.emptySet;
 
+import java.util.ArrayDeque;
+import java.util.Deque;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedHashSet;
@@ -17,6 +19,7 @@ import java.util.stream.StreamSupport;
 import org.checkerframework.checker.nullness.qual.Nullable;
 
 import net.sourceforge.pmd.internal.util.IteratorUtil;
+import net.sourceforge.pmd.internal.util.IteratorUtil.AbstractIterator;
 import net.sourceforge.pmd.lang.java.types.JClassType;
 
 /**
@@ -87,6 +90,7 @@ public enum SuperTypesEnumerator {
         }
     },
 
+
     /**
      * Walks supertypes depth-first, without duplicates. This includes
      * Object if the search starts on an interface. For example for the following:
@@ -94,32 +98,18 @@ public enum SuperTypesEnumerator {
      *
      * interface I1 { } // yields I1, Object
      *
-     * interface I2 extends I1 { }  // yields I2, I1, Object
+     * interface I2 extends I1 { }  // yields I2, Object, I1
      *
-     * class Sup implements I2 { }  // yields Sup, I2, I1, Object
+     * class Sup implements I2 { }  // yields Sup, Object, I2, I1
      *
-     * class Sub extends Sup implements I1 { } // yields Sub, I1, Sup, I2, Object
+     * class Sub extends Sup implements I1 { } // yields Sub, Sup, Object, I2, I1
      *
      * }</pre>
      */
     ALL_SUPERTYPES_INCLUDING_SELF {
         @Override
         public Iterator<JClassType> iterator(JClassType t) {
-            final Set<JClassType> seenInterfaces = new HashSet<>();
-            return IteratorUtil.flatMapWithSelf(SUPERCLASSES_AND_SELF.iterator(t), type -> {
-                final Set<JClassType> currentInterfaces = new LinkedHashSet<>();
-                walkInterfaces(seenInterfaces, currentInterfaces, type);
-                return currentInterfaces.iterator();
-            });
-        }
-
-        private void walkInterfaces(final Set<JClassType> seen, final Set<JClassType> addTo, final JClassType c) {
-            for (final JClassType iface : c.getSuperInterfaces()) {
-                if (seen.add(iface)) {
-                    addTo.add(iface);
-                    walkInterfaces(seen, addTo, iface); // Recurses into all super itfs
-                }
-            }
+            return new SuperTypeWalker(t);
         }
     };
 
@@ -128,10 +118,42 @@ public enum SuperTypesEnumerator {
 
     public Stream<JClassType> stream(JClassType t) {
         return StreamSupport.stream(iterable(t).spliterator(), false);
-
     }
 
     public Iterable<JClassType> iterable(JClassType t) {
         return () -> iterator(t);
     }
+
+    private static class SuperTypeWalker extends AbstractIterator<JClassType> {
+
+        final Set<JClassType> seen = new HashSet<>();
+        final Deque<JClassType> todo = new ArrayDeque<>(2);
+
+        SuperTypeWalker(JClassType start) {
+            todo.push(start);
+        }
+
+        @Override
+        protected void computeNext() {
+            if (todo.isEmpty()) {
+                done();
+            } else {
+                JClassType top = todo.pollFirst();
+                setNext(top);
+                enqueue(top);
+            }
+        }
+
+        private void enqueue(final JClassType c) {
+            JClassType sup = c.getSuperClass();
+            if (sup != null && seen.add(sup)) {
+                todo.addFirst(sup);
+            }
+            for (final JClassType iface : c.getSuperInterfaces()) {
+                if (seen.add(iface)) {
+                    todo.addLast(iface);
+                }
+            }
+        }
+    }
 }

pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/OverrideResolutionTest.kt

@@ -0,0 +1,60 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+package net.sourceforge.pmd.lang.java.ast
+
+import io.kotest.matchers.shouldBe
+
+class OverrideResolutionTest : ProcessorTestSpec({
+
+    parserTest("Test override resolution prefers superclass method") {
+        val acu = parser.parse("""
+            interface Foo { default void foo() {} }
+            interface Bar { default void foo() {} }
+            class Sup { public void foo() {} }
+
+            public class Sub extends Sup implements Foo, Bar {
+                public void foo() {
+                    super.foo(); // useless
+                }
+            }
+        """)
+        val (fooFoo, barFoo, supFoo, subFoo) = acu.descendants(ASTMethodDeclaration::class.java).toList()
+        subFoo.overriddenMethod shouldBe supFoo.genericSignature
+        barFoo.overriddenMethod shouldBe null
+        fooFoo.overriddenMethod shouldBe null
+        supFoo.overriddenMethod shouldBe null
+    }
+
+    parserTest("Test override resolution without superclass") {
+        val acu = parser.parse("""
+            interface Foo { default void foo() {} }
+            interface Bar { default void foo() {} }
+            class Sup implements Bar { public void foo() {} }
+
+            public class Sub implements Foo, Bar {
+                public void foo() {
+                    super.foo(); // useless
+                }
+            }
+        """)
+        val (fooFoo, barFoo, supFoo, subFoo) = acu.descendants(ASTMethodDeclaration::class.java).toList()
+        supFoo.overriddenMethod shouldBe barFoo.genericSignature
+        subFoo.overriddenMethod shouldBe fooFoo.genericSignature
+        barFoo.overriddenMethod shouldBe null
+        fooFoo.overriddenMethod shouldBe null
+    }
+
+    parserTest("Test override resolution unresolved") {
+        val acu = parser.parse("""
+            public class Sub implements Unresolved {
+                @Override
+                public void foo() {
+                }
+            }
+        """)
+        val (subFoo) = acu.descendants(ASTMethodDeclaration::class.java).toList()
+        subFoo.overriddenMethod shouldBe subFoo.typeSystem.UNRESOLVED_METHOD
+    }
+
+})

pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/symbols/table/internal/SuperTypesEnumeratorTest.kt

@@ -38,18 +38,18 @@ class SuperTypesEnumeratorTest : ParserTestSpec({
         doTest("ALL_SUPERTYPES_INCLUDING_SELF") {
             with(acu.typeDsl) {
                 ALL_SUPERTYPES_INCLUDING_SELF.list(i1) should containExactly(i1, ts.OBJECT)
-                ALL_SUPERTYPES_INCLUDING_SELF.list(i2) should containExactly(i2, i1, ts.OBJECT)
-                ALL_SUPERTYPES_INCLUDING_SELF.list(sup) should containExactly(sup, i2, i1, ts.OBJECT)
-                ALL_SUPERTYPES_INCLUDING_SELF.list(sub) should containExactly(sub, i1, sup, i2, ts.OBJECT)
+                ALL_SUPERTYPES_INCLUDING_SELF.list(i2) should containExactly(i2, ts.OBJECT, i1)
+                ALL_SUPERTYPES_INCLUDING_SELF.list(sup) should containExactly(sup, ts.OBJECT, i2, i1)
+                ALL_SUPERTYPES_INCLUDING_SELF.list(sub) should containExactly(sub, sup, ts.OBJECT, i1, i2)
             }
         }
 
         doTest("ALL_STRICT_SUPERTYPES") {
             with(acu.typeDsl) {
                 ALL_STRICT_SUPERTYPES.list(i1) should containExactly(ts.OBJECT)
-                ALL_STRICT_SUPERTYPES.list(i2) should containExactly(i1, ts.OBJECT)
-                ALL_STRICT_SUPERTYPES.list(sup) should containExactly(i2, i1, ts.OBJECT)
-                ALL_STRICT_SUPERTYPES.list(sub) should containExactly(i1, sup, i2, ts.OBJECT)
+                ALL_STRICT_SUPERTYPES.list(i2) should containExactly(ts.OBJECT, i1)
+                ALL_STRICT_SUPERTYPES.list(sup) should containExactly(ts.OBJECT, i2, i1)
+                ALL_STRICT_SUPERTYPES.list(sub) should containExactly(sup, ts.OBJECT, i1, i2)
             }
         }
 

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

@@ -543,7 +543,7 @@ public class DirectSubclass2 extends DirectSubclass {
     public void doBase() {
         super.doBase();
     }
- 
+
     @Override
     public void doBaseWithArg(String foo) {
         super.doBaseWithArg(foo);
@@ -616,4 +616,76 @@ public class OtherSubclass extends BaseClass {
 }
         ]]></code>
     </test-code>
+    <test-code>
+        <description>Overriding with call to super interface default</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+            interface Foo {
+                default void foo() {
+
+                }
+            }
+
+            public class AClass implements Foo {
+
+                public void foo() {
+                    // technically useless, but this form might be required
+                    // by the compiler if there are several unrelated default
+                    // methods that are inherited.
+                    Foo.super.foo();
+                }
+            }
+            ]]></code>
+    </test-code>
+    <test-code>
+        <description>Overriding with call to super interface default, required by compiler</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+            interface Foo {
+                default void foo() {
+
+                }
+            }
+
+            interface Bar {
+                default void foo() {
+
+                }
+            }
+
+            public class AClass implements Foo, Bar {
+
+                public void foo() {
+                    Foo.super.foo();
+                }
+            }
+            ]]></code>
+    </test-code>
+    <test-code>
+        <description>Overriding with call to superclass, useless</description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+            interface Foo {
+                default void foo() {
+
+                }
+            }
+
+            interface Bar {
+                default void foo() {
+
+                }
+            }
+
+            class Sup {
+                public void foo() {}
+            }
+
+            public class AClass extends Sup implements Foo, Bar {
+                public void foo() {
+                    super.foo(); // useless
+                }
+            }
+            ]]></code>
+    </test-code>
 </test-data>