Skip to content

Commit

Permalink
Dedupe stuff reimplementing #findSuperMethods.
Browse files Browse the repository at this point in the history 
PiperOrigin-RevId: 424305981
  • Loading branch information
@graememorgan
graememorgan authored and Error Prone Team committed Jan 26, 2022
1 parent dd0ee50 commit 18eff81
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 130 deletions.
32 changes: 2 additions & 30 deletions core/src/main/java/com/google/errorprone/bugpatterns/DoNotCallSuggester.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

/**
Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
}
}
95 changes: 30 additions & 65 deletions core/src/main/java/com/google/errorprone/bugpatterns/MissingOverride.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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. */
Expand All @@ -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);
}
}
38 changes: 3 additions & 35 deletions core/src/main/java/com/google/errorprone/bugpatterns/TooManyParameters.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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. */
Expand Down Expand Up @@ -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;
Expand All @@ -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;
}
}

0 comments on commit 18eff81

Please sign in to comment.