diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/DoNotCallSuggester.java b/core/src/main/java/com/google/errorprone/bugpatterns/DoNotCallSuggester.java index eee9fc5f4fc..063f9cd886f 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/DoNotCallSuggester.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/DoNotCallSuggester.java @@ -22,6 +22,7 @@ import static com.google.errorprone.matchers.Description.NO_MATCH; import static com.google.errorprone.util.ASTHelpers.enclosingClass; import static com.google.errorprone.util.ASTHelpers.findClass; +import static com.google.errorprone.util.ASTHelpers.findSuperMethods; import static com.google.errorprone.util.ASTHelpers.getSymbol; import static com.google.errorprone.util.ASTHelpers.getType; import static com.google.errorprone.util.ASTHelpers.hasAnnotation; @@ -37,11 +38,9 @@ import com.sun.source.tree.MethodTree; import com.sun.source.tree.StatementTree; import com.sun.source.tree.ThrowTree; -import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Symbol.ClassSymbol; import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.code.Type; -import com.sun.tools.javac.code.Types; import javax.lang.model.element.Modifier; /** @@ -166,7 +165,7 @@ public Description matchMethod(MethodTree tree, VisitorState state) { } // if the method is an "effective override" (they forgot to add @Override), exit - if (isEffectivelyOverride(symbol, state.getTypes())) { + if (!findSuperMethods(symbol, state.getTypes()).isEmpty()) { return NO_MATCH; } @@ -186,31 +185,4 @@ public Description matchMethod(MethodTree tree, VisitorState state) { .addFix(fix) .build(); } - - // TODO(b/216306810): copied from MissingOverride.java - private static boolean isEffectivelyOverride(Symbol sym, Types types) { - // static methods can't be overrides - if (sym.isStatic()) { - return false; - } - ClassSymbol owner = sym.enclClass(); - for (Type s : types.closure(owner.type)) { - if (s.asElement().equals(owner)) { - continue; - } - for (Symbol m : s.tsym.members().getSymbolsByName(sym.name)) { - if (!(m instanceof MethodSymbol)) { - continue; - } - MethodSymbol msym = (MethodSymbol) m; - if (msym.isStatic()) { - continue; - } - if (sym.overrides(msym, owner, types, /* checkResult= */ false)) { - return true; - } - } - } - return false; - } } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MissingOverride.java b/core/src/main/java/com/google/errorprone/bugpatterns/MissingOverride.java index 151d45eac31..27352f22183 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/MissingOverride.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/MissingOverride.java @@ -17,6 +17,9 @@ package com.google.errorprone.bugpatterns; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.util.ASTHelpers.findSuperMethods; +import static com.google.errorprone.util.ASTHelpers.hasAnnotation; import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern.StandardTags; @@ -27,12 +30,6 @@ import com.google.errorprone.matchers.Description; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.MethodTree; -import com.sun.tools.javac.code.Symbol; -import com.sun.tools.javac.code.Symbol.ClassSymbol; -import com.sun.tools.javac.code.Symbol.MethodSymbol; -import com.sun.tools.javac.code.Type; -import com.sun.tools.javac.code.Types; -import javax.annotation.Nullable; import javax.lang.model.element.Modifier; /** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ @@ -56,68 +53,36 @@ public MissingOverride(ErrorProneFlags flags) { @Override public Description matchMethod(MethodTree tree, VisitorState state) { - Symbol sym = ASTHelpers.getSymbol(tree); + var sym = ASTHelpers.getSymbol(tree); if (sym.isStatic()) { - return Description.NO_MATCH; + return NO_MATCH; } - if (ASTHelpers.hasAnnotation(sym, Override.class, state)) { - return Description.NO_MATCH; + if (hasAnnotation(sym, Override.class, state)) { + return NO_MATCH; } - MethodSymbol override = getFirstOverride(sym, state.getTypes()); - if (override == null) { - return Description.NO_MATCH; + if (ignoreInterfaceOverrides && sym.enclClass().isInterface()) { + return NO_MATCH; } - if (!ASTHelpers.getGeneratedBy(state).isEmpty()) { - return Description.NO_MATCH; - } - if (ASTHelpers.hasAnnotation(override, Deprecated.class, state)) { - // to allow deprecated methods to be removed non-atomically, we permit overrides of - // @Deprecated to skip the annotation - return Description.NO_MATCH; - } - return buildDescription(tree) - .addFix(SuggestedFix.prefixWith(tree, "@Override ")) - .setMessage( - String.format( - "%s %s method in %s; expected @Override", - sym.getSimpleName(), - override.enclClass().isInterface() - || override.getModifiers().contains(Modifier.ABSTRACT) - ? "implements" - : "overrides", - override.enclClass().getSimpleName())) - .build(); - } - - /** - * Returns the {@link MethodSymbol} of the first method that sym overrides in its supertype - * closure, or {@code null} if no such method exists. - */ - // TODO(b/216306810): consider adding a generalized version of this to ASTHelpers - @Nullable - private MethodSymbol getFirstOverride(Symbol sym, Types types) { - ClassSymbol owner = sym.enclClass(); - if (ignoreInterfaceOverrides && owner.isInterface()) { - // pretend the method does not override anything - return null; - } - for (Type s : types.closure(owner.type)) { - if (types.isSameType(s, owner.type)) { - continue; - } - for (Symbol m : s.tsym.members().getSymbolsByName(sym.name)) { - if (!(m instanceof MethodSymbol)) { - continue; - } - MethodSymbol msym = (MethodSymbol) m; - if (msym.isStatic()) { - continue; - } - if (sym.overrides(msym, owner, types, /* checkResult= */ false)) { - return msym; - } - } - } - return null; + return findSuperMethods(sym, state.getTypes()).stream() + .findFirst() + .filter(unused -> ASTHelpers.getGeneratedBy(state).isEmpty()) + // to allow deprecated methods to be removed non-atomically, we permit overrides of + // @Deprecated to skip the annotation + .filter(override -> !hasAnnotation(override, Deprecated.class, state)) + .map( + override -> + buildDescription(tree) + .addFix(SuggestedFix.prefixWith(tree, "@Override ")) + .setMessage( + String.format( + "%s %s method in %s; expected @Override", + sym.getSimpleName(), + override.enclClass().isInterface() + || override.getModifiers().contains(Modifier.ABSTRACT) + ? "implements" + : "overrides", + override.enclClass().getSimpleName())) + .build()) + .orElse(NO_MATCH); } } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/TooManyParameters.java b/core/src/main/java/com/google/errorprone/bugpatterns/TooManyParameters.java index 6ce97425454..d2e0f97a3f3 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/TooManyParameters.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/TooManyParameters.java @@ -20,6 +20,7 @@ import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.matchers.Description.NO_MATCH; import static com.google.errorprone.util.ASTHelpers.findEnclosingNode; +import static com.google.errorprone.util.ASTHelpers.findSuperMethods; import static com.google.errorprone.util.ASTHelpers.getSymbol; import static com.google.errorprone.util.ASTHelpers.hasAnnotation; @@ -31,12 +32,6 @@ import com.google.errorprone.matchers.Description; import com.sun.source.tree.ClassTree; import com.sun.source.tree.MethodTree; -import com.sun.source.tree.Tree; -import com.sun.tools.javac.code.Symbol; -import com.sun.tools.javac.code.Symbol.ClassSymbol; -import com.sun.tools.javac.code.Symbol.MethodSymbol; -import com.sun.tools.javac.code.Type; -import com.sun.tools.javac.code.Types; import javax.lang.model.element.Modifier; /** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ @@ -94,7 +89,7 @@ public Description matchMethod(MethodTree tree, VisitorState state) { // Copied + modified from GoodTime API checker // TODO(kak): we should probably move this somewhere that future API checks can use - private static boolean shouldApplyApiChecks(Tree tree, VisitorState state) { + private static boolean shouldApplyApiChecks(MethodTree tree, VisitorState state) { for (String annotation : ANNOTATIONS_TO_IGNORE) { if (hasAnnotation(tree, annotation, state)) { return false; @@ -110,36 +105,9 @@ private static boolean shouldApplyApiChecks(Tree tree, VisitorState state) { return false; } // don't match overrides (even "effective overrides") - if (isEffectivelyOverride(getSymbol(tree), state.getTypes())) { + if (!findSuperMethods(getSymbol(tree), state.getTypes()).isEmpty()) { return false; } return true; } - - // TODO(b/216306810): copied from MissingOverride.java - private static boolean isEffectivelyOverride(Symbol sym, Types types) { - // static methods can't be overrides - if (sym.isStatic()) { - return false; - } - ClassSymbol owner = sym.enclClass(); - for (Type s : types.closure(owner.type)) { - if (s.asElement().equals(owner)) { - continue; - } - for (Symbol m : s.tsym.members().getSymbolsByName(sym.name)) { - if (!(m instanceof MethodSymbol)) { - continue; - } - MethodSymbol msym = (MethodSymbol) m; - if (msym.isStatic()) { - continue; - } - if (sym.overrides(msym, owner, types, /* checkResult= */ false)) { - return true; - } - } - } - return false; - } }