Skip to content

Commit

Permalink
Don't fire AvoidObjectArrays if an Iterable-based overload exists.
Browse files Browse the repository at this point in the history 
#klippy4apis

PiperOrigin-RevId: 518719986
  • Loading branch information
@kluever
kluever authored and Error Prone Team committed Mar 23, 2023
1 parent baba0c5 commit c262ba7
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 74 deletions.
53 changes: 53 additions & 0 deletions check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java
View file
Expand Up @@ -38,6 +38,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.SetMultimap;
import com.google.common.collect.Sets;
import com.google.common.collect.Streams;
Expand Down Expand Up @@ -2634,5 +2635,57 @@ public static boolean isKotlin(Symbol symbol, VisitorState state) {
return hasAnnotation(symbol.enclClass(), "kotlin.Metadata", state);
}

/**
* Returns whether {@code existingMethod} has an overload (or "nearly" an overload) with the given
* {@code targetMethodName}, and only a single parameter of type {@code onlyParameterType}.
*/
public static boolean hasOverloadWithOnlyOneParameter(
MethodSymbol existingMethod,
Name targetMethodName,
Type onlyParameterType,
VisitorState state) {
@Nullable MethodTree t = state.findEnclosing(MethodTree.class);
@Nullable MethodSymbol enclosingMethod = t == null ? null : getSymbol(t);

return hasMatchingMethods(
targetMethodName,
input ->
!input.equals(existingMethod)
// Make sure we're not currently *inside* that overload, to avoid
// creating an infinite loop.
&& !input.equals(enclosingMethod)
&& (enclosingMethod == null
|| !enclosingMethod.overrides(
input, (TypeSymbol) input.owner, state.getTypes(), true))
&& input.isStatic() == existingMethod.isStatic()
&& input.getParameters().size() == 1
&& isSameType(input.getParameters().get(0).asType(), onlyParameterType, state)
&& isSameType(input.getReturnType(), existingMethod.getReturnType(), state),
enclosingClass(existingMethod).asType(),
state.getTypes());
}

// Adapted from findMatchingMethods(); but this short-circuits
private static boolean hasMatchingMethods(
Name name, Predicate<MethodSymbol> predicate, Type startClass, Types types) {
Predicate<Symbol> matchesMethodPredicate =
sym -> sym instanceof MethodSymbol && predicate.test((MethodSymbol) sym);

// Iterate over all classes and interfaces that startClass inherits from.
for (Type superClass : types.closure(startClass)) {
// Iterate over all the methods declared in superClass.
TypeSymbol superClassSymbol = superClass.tsym;
Scope superClassSymbols = superClassSymbol.members();
if (superClassSymbols != null) { // Can be null if superClass is a type variable
if (!Iterables.isEmpty(
scope(superClassSymbols)
.getSymbolsByName(name, matchesMethodPredicate, NON_RECURSIVE))) {
return true;
}
}
}
return false;
}

private ASTHelpers() {}
}
13 changes: 11 additions & 2 deletions core/src/main/java/com/google/errorprone/bugpatterns/AvoidObjectArrays.java
View file
Expand Up @@ -23,6 +23,7 @@
import static com.google.errorprone.util.ASTHelpers.getSymbol;
import static com.google.errorprone.util.ASTHelpers.getType;
import static com.google.errorprone.util.ASTHelpers.hasAnnotation;
import static com.google.errorprone.util.ASTHelpers.hasOverloadWithOnlyOneParameter;
import static com.google.errorprone.util.ASTHelpers.methodIsPublicAndNotAnOverride;
import static java.util.function.Predicate.isEqual;

Expand Down Expand Up @@ -63,12 +64,20 @@ public Description matchMethod(MethodTree method, VisitorState state) {
createDescription(method.getReturnType(), state, "returning", "ImmutableList"));
}

MethodSymbol methodSymbol = getSymbol(method);

// check each method parameter
for (int i = 0; i < method.getParameters().size(); i++) {
VariableTree varTree = method.getParameters().get(i);
if (isObjectArray(varTree)) {
// we allow an object array if it's the last parameter and it's var args
if (getSymbol(method).isVarArgs() && i == method.getParameters().size() - 1) {
// we allow an Object[] param if it's the last parameter and it's var args
if (methodSymbol.isVarArgs() && i == method.getParameters().size() - 1) {
continue;
}

// we allow an Object[] param if there's also an overload with an Iterable param
if (hasOverloadWithOnlyOneParameter(
methodSymbol, methodSymbol.name, state.getSymtab().iterableType, state)) {
continue;
}

Expand Down
90 changes: 20 additions & 70 deletions core/src/main/java/com/google/errorprone/bugpatterns/time/PreferJavaTimeOverload.java
View file
Expand Up @@ -22,9 +22,9 @@
import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod;
import static com.google.errorprone.util.ASTHelpers.getStartPosition;
import static com.google.errorprone.util.ASTHelpers.getSymbol;
import static com.google.errorprone.util.ASTHelpers.hasOverloadWithOnlyOneParameter;
import static com.google.errorprone.util.ASTHelpers.isSameType;
import static com.google.errorprone.util.ASTHelpers.isSubtype;
import static com.sun.tools.javac.code.Scope.LookupKind.NON_RECURSIVE;
import static java.util.concurrent.TimeUnit.DAYS;
import static java.util.concurrent.TimeUnit.HOURS;
import static java.util.concurrent.TimeUnit.MICROSECONDS;
Expand All @@ -34,7 +34,6 @@
import static java.util.concurrent.TimeUnit.SECONDS;

import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.errorprone.BugPattern;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
Expand All @@ -48,22 +47,14 @@
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.NewClassTree;
import com.sun.tools.javac.code.Scope;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Symbol.MethodSymbol;
import com.sun.tools.javac.code.Symbol.TypeSymbol;
import com.sun.tools.javac.code.Symbol.VarSymbol;
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.code.Types;
import com.sun.tools.javac.util.Name;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.TimeUnit;
import java.util.function.Predicate;
import javax.annotation.Nullable;
import javax.inject.Inject;

/** This check suggests the use of {@code java.time}-based APIs, when available. */
Expand Down Expand Up @@ -161,18 +152,18 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
// foo(String, long, TimeUnit, Frobber) -> foo(String, Duration, Frobber)

if (isNumericMethodCall(tree, state)) {
if (hasJavaTimeOverload(tree, state, JAVA_DURATION)) {
if (hasJavaTimeOverload(tree, JAVA_DURATION_TYPE.get(state), state)) {
return buildDescriptionForNumericPrimitive(tree, state, arguments, "Duration");
}
if (hasJavaTimeOverload(tree, state, JAVA_INSTANT)) {
if (hasJavaTimeOverload(tree, JAVA_INSTANT_TYPE.get(state), state)) {
return buildDescriptionForNumericPrimitive(tree, state, arguments, "Instant");
}
}

if (isLongTimeUnitMethodCall(tree, state)) {
Optional<TimeUnit> optionalTimeUnit = DurationToLongTimeUnit.getTimeUnit(arguments.get(1));
if (optionalTimeUnit.isPresent()) {
if (hasJavaTimeOverload(tree, state, JAVA_DURATION)) {
if (hasJavaTimeOverload(tree, JAVA_DURATION_TYPE.get(state), state)) {
String durationFactory = TIMEUNIT_TO_DURATION_FACTORY.get(optionalTimeUnit.get());
if (durationFactory != null) {
SuggestedFix.Builder fix = SuggestedFix.builder();
Expand All @@ -187,7 +178,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
if (JAVA_DURATION_DECOMPOSITION_MATCHER.matches(maybeDurationDecomposition, state)) {
if (isSameType(
ASTHelpers.getReceiverType(maybeDurationDecomposition),
JAVA_TIME_DURATION.get(state),
JAVA_DURATION_TYPE.get(state),
state)) {
replacement =
state.getSourceForNode(ASTHelpers.getReceiver(maybeDurationDecomposition));
Expand Down Expand Up @@ -221,7 +212,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState

if (isMethodCallWithSingleParameter(tree, state, "org.joda.time.ReadableDuration")) {
ExpressionTree arg0 = arguments.get(0);
if (hasJavaTimeOverload(tree, state, JAVA_DURATION)) {
if (hasJavaTimeOverload(tree, JAVA_DURATION_TYPE.get(state), state)) {
SuggestedFix.Builder fix = SuggestedFix.builder();
// TODO(kak): Maybe only emit a match if Duration doesn't have to be fully qualified?
String qualifiedDuration = SuggestedFixes.qualifyType(state, fix, JAVA_DURATION);
Expand Down Expand Up @@ -272,7 +263,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState

if (isMethodCallWithSingleParameter(tree, state, "org.joda.time.ReadableInstant")) {
ExpressionTree arg0 = arguments.get(0);
if (hasJavaTimeOverload(tree, state, JAVA_INSTANT)) {
if (hasJavaTimeOverload(tree, JAVA_INSTANT_TYPE.get(state), state)) {
SuggestedFix.Builder fix = SuggestedFix.builder();
// TODO(kak): Maybe only emit a match if Instant doesn't have to be fully qualified?
String qualifiedInstant = SuggestedFixes.qualifyType(state, fix, JAVA_INSTANT);
Expand Down Expand Up @@ -345,7 +336,7 @@ private static boolean isMethodCallWithSingleParameter(

private static boolean isLongTimeUnitMethodCall(MethodInvocationTree tree, VisitorState state) {
Type longType = state.getSymtab().longType;
Type timeUnitType = JAVA_UTIL_CONCURRENT_TIMEUNIT.get(state);
Type timeUnitType = TIME_UNIT_TYPE.get(state);
List<VarSymbol> params = getSymbol(tree).getParameters();
if (params.size() == 2) {
return isSameType(params.get(0).asType(), longType, state)
Expand All @@ -355,68 +346,27 @@ private static boolean isLongTimeUnitMethodCall(MethodInvocationTree tree, Visit
}

private static boolean hasJavaTimeOverload(
MethodInvocationTree tree, VisitorState state, String typeName) {
MethodInvocationTree tree, Type type, VisitorState state) {
MethodSymbol calledMethod = getSymbol(tree);
return hasJavaTimeOverload(state, typeName, calledMethod, calledMethod.name);
}

private static boolean hasJavaTimeOverload(
VisitorState state, String typeName, MethodSymbol calledMethod, Name methodName) {

MethodTree t = state.findEnclosing(MethodTree.class);
@Nullable MethodSymbol enclosingMethod = t == null ? null : getSymbol(t);

Type type = state.getTypeFromString(typeName);
return hasMatchingMethods(
methodName,
input ->
!input.equals(calledMethod)
// Make sure we're not currently *inside* that overload, to avoid
// creating an infinite loop.
&& !input.equals(enclosingMethod)
&& (enclosingMethod == null
|| !enclosingMethod.overrides(
input, (TypeSymbol) input.owner, state.getTypes(), true))
&& input.isStatic() == calledMethod.isStatic()
&& input.getParameters().size() == 1
&& isSameType(input.getParameters().get(0).asType(), type, state)
&& isSameType(input.getReturnType(), calledMethod.getReturnType(), state),
ASTHelpers.enclosingClass(calledMethod).asType(),
state.getTypes());
return hasOverloadWithOnlyOneParameter(calledMethod, calledMethod.name, type, state);
}

private static boolean hasTimeSourceMethod(MethodInvocationTree tree, VisitorState state) {
MethodSymbol calledMethod = getSymbol(tree);
String timeSourceBasedName = calledMethod.name.toString().replace("Clock", "TimeSource");
return hasJavaTimeOverload(
state, TIME_SOURCE, calledMethod, state.getName(timeSourceBasedName));
return hasOverloadWithOnlyOneParameter(
calledMethod, state.getName(timeSourceBasedName), TIME_SOURCE_TYPE.get(state), state);
}

// Adapted from ASTHelpers.findMatchingMethods(); but this short-circuits
private static boolean hasMatchingMethods(
Name name, Predicate<MethodSymbol> predicate, Type startClass, Types types) {
Predicate<Symbol> matchesMethodPredicate =
sym -> sym instanceof MethodSymbol && predicate.test((MethodSymbol) sym);

// Iterate over all classes and interfaces that startClass inherits from.
for (Type superClass : types.closure(startClass)) {
// Iterate over all the methods declared in superClass.
TypeSymbol superClassSymbol = superClass.tsym;
Scope superClassSymbols = superClassSymbol.members();
if (superClassSymbols != null) { // Can be null if superClass is a type variable
if (!Iterables.isEmpty(
ASTHelpers.scope(superClassSymbols)
.getSymbolsByName(name, matchesMethodPredicate, NON_RECURSIVE))) {
return true;
}
}
}
return false;
}

private static final Supplier<Type> JAVA_TIME_DURATION =
private static final Supplier<Type> JAVA_DURATION_TYPE =
VisitorState.memoize(state -> state.getTypeFromString(JAVA_DURATION));

private static final Supplier<Type> JAVA_UTIL_CONCURRENT_TIMEUNIT =
private static final Supplier<Type> JAVA_INSTANT_TYPE =
VisitorState.memoize(state -> state.getTypeFromString(JAVA_INSTANT));

private static final Supplier<Type> TIME_UNIT_TYPE =
VisitorState.memoize(state -> state.getTypeFromString("java.util.concurrent.TimeUnit"));

private static final Supplier<Type> TIME_SOURCE_TYPE =
VisitorState.memoize(state -> state.getTypeFromString(TIME_SOURCE));
}
2 changes: 0 additions & 2 deletions core/src/test/java/com/google/errorprone/bugpatterns/AvoidObjectArraysTest.java
View file
Expand Up @@ -84,8 +84,6 @@ public void methodParam_instanceMethods_withIterableOverload() {
"public class IterableSubject {",
" public final void containsAnyIn(Iterable<?> expected) {",
" }",
// TODO(b/273948064): we shouldn't fire here because there's an Iterable overload
" // BUG: Diagnostic contains: consider an Iterable<Object> instead",
" public final void containsAnyIn(Object[] expected) {",
" }",
"}")
Expand Down

0 comments on commit c262ba7

Please sign in to comment.