Skip to content

Commit

Permalink
Don't fire some NonApiType checks inside of GraphWrappers.
Browse files Browse the repository at this point in the history 
#klippy4apis

PiperOrigin-RevId: 540296486
  • Loading branch information
@kluever
kluever authored and Error Prone Team committed Jun 14, 2023
1 parent e3743fc commit 4f11cba
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 21 deletions.
20 changes: 15 additions & 5 deletions check_api/src/main/java/com/google/errorprone/predicates/TypePredicates.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,16 @@
/** A collection of {@link TypePredicate}s. */
public final class TypePredicates {

/** Matches nothing (always {@code false}). */
public static TypePredicate nothing() {
return (type, state) -> false;
}

/** Matches everything (always {@code true}). */
public static TypePredicate anything() {
return (type, state) -> true;
}

/** Match arrays. */
public static TypePredicate isArray() {
return Array.INSTANCE;
Expand All @@ -55,16 +65,16 @@ public static TypePredicate isDescendantOf(Supplier<Type> type) {
return new DescendantOf(type);
}

/** Match types that are a sub-type of one of the given types. */
public static TypePredicate isDescendantOfAny(Iterable<String> types) {
return new DescendantOfAny(fromStrings(types));
}

/** Match sub-types of the given type. */
public static TypePredicate isDescendantOf(String type) {
return isDescendantOf(Suppliers.typeFromString(type));
}

/** Match types that are a sub-type of one of the given types. */
public static TypePredicate isDescendantOfAny(Iterable<String> types) {
return new DescendantOfAny(fromStrings(types));
}

public static TypePredicate allOf(TypePredicate... predicates) {
return (type, state) -> {
for (TypePredicate predicate : predicates) {
Expand Down
61 changes: 45 additions & 16 deletions core/src/main/java/com/google/errorprone/bugpatterns/NonApiType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,16 @@
import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.predicates.TypePredicate;
import com.google.errorprone.predicates.TypePredicates;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.Tree;
import com.sun.tools.javac.code.Type;
import java.util.ArrayDeque;
import java.util.Deque;

/** Flags instances of non-API types from being accepted or returned in public APIs. */
/** Flags instances of non-API types from being accepted or returned in APIs. */
@BugPattern(
summary = "Certain types should not be passed across public API boundaries.",
summary = "Certain types should not be passed across API boundaries.",
// something about reducing build visibility
severity = WARNING)
public final class NonApiType extends BugChecker implements MethodTreeMatcher {
Expand All @@ -56,6 +57,10 @@ public final class NonApiType extends BugChecker implements MethodTreeMatcher {
private static final String OPTIONAL_AS_PARAM_LINK = "";
private static final String PREFER_JDK_OPTIONAL_LINK = "";

private static final TypePredicate GRAPH_WRAPPER =
TypePredicates.not(
TypePredicates.isDescendantOf("com.google.apps.framework.producers.GraphWrapper"));

private static final ImmutableSet<TypeToCheck> NON_API_TYPES =
ImmutableSet.of(
// primitive arrays
Expand Down Expand Up @@ -91,19 +96,23 @@ public final class NonApiType extends BugChecker implements MethodTreeMatcher {
// ImmutableFoo as params
withPublicVisibility(
isExactType("com.google.common.collect.ImmutableCollection"),
GRAPH_WRAPPER,
"Consider accepting a java.util.Collection or Iterable instead. "
+ TYPE_GENERALITY_LINK,
ApiElementType.PARAMETER),
withPublicVisibility(
isExactType("com.google.common.collect.ImmutableList"),
GRAPH_WRAPPER,
"Consider accepting a java.util.List or Iterable instead. " + TYPE_GENERALITY_LINK,
ApiElementType.PARAMETER),
withPublicVisibility(
isExactType("com.google.common.collect.ImmutableSet"),
GRAPH_WRAPPER,
"Consider accepting a java.util.Set or Iterable instead. " + TYPE_GENERALITY_LINK,
ApiElementType.PARAMETER),
withPublicVisibility(
isExactType("com.google.common.collect.ImmutableMap"),
GRAPH_WRAPPER,
"Consider accepting a java.util.Map instead. " + TYPE_GENERALITY_LINK,
ApiElementType.PARAMETER),

Expand Down Expand Up @@ -179,21 +188,27 @@ private static Type makeArrayType(String typeName, VisitorState state) {

@Override
public Description matchMethod(MethodTree tree, VisitorState state) {
Type enclosingType = getSymbol(tree).owner.type;

boolean isPublicApi =
methodIsPublicAndNotAnOverride(getSymbol(tree), state)
&& state.errorProneOptions().isPubliclyVisibleTarget();

for (Tree parameter : tree.getParameters()) {
checkType(parameter, ApiElementType.PARAMETER, isPublicApi, state);
checkType(parameter, ApiElementType.PARAMETER, isPublicApi, enclosingType, state);
}
checkType(tree.getReturnType(), ApiElementType.RETURN_TYPE, isPublicApi, state);
checkType(tree.getReturnType(), ApiElementType.RETURN_TYPE, isPublicApi, enclosingType, state);

// the accumulated matches (if any) are reported via state.reportMatch(...)
return NO_MATCH;
}

private void checkType(
Tree tree, ApiElementType elementType, boolean isPublicApi, VisitorState state) {
Tree tree,
ApiElementType elementType,
boolean isPublicApi,
Type enclosingType,
VisitorState state) {
if (isSuppressed(tree, state)) {
return;
}
Expand All @@ -202,7 +217,7 @@ private void checkType(
return;
}
for (TypeToCheck typeToCheck : NON_API_TYPES) {
if (typeToCheck.matches(type, state)) {
if (typeToCheck.matches(type, enclosingType, state)) {
if (typeToCheck.elementType() == ApiElementType.ANY
|| typeToCheck.elementType() == elementType) {
if (isPublicApi || typeToCheck.visibility() == ApiVisibility.ANY) {
Expand All @@ -227,35 +242,49 @@ enum ApiVisibility {

private static TypeToCheck withPublicVisibility(
TypePredicate typePredicate, String failureMessage, ApiElementType elementType) {
return withPublicVisibility(
typePredicate, TypePredicates.anything(), failureMessage, elementType);
}

private static TypeToCheck withPublicVisibility(
TypePredicate typePredicate,
TypePredicate enclosingTypePredicate,
String failureMessage,
ApiElementType elementType) {
return new AutoValue_NonApiType_TypeToCheck(
typePredicate, failureMessage, ApiVisibility.PUBLIC, elementType);
typePredicate, enclosingTypePredicate, failureMessage, ApiVisibility.PUBLIC, elementType);
}

private static TypeToCheck withAnyVisibility(
TypePredicate typePredicate, String failureMessage, ApiElementType elementType) {
return new AutoValue_NonApiType_TypeToCheck(
typePredicate, failureMessage, ApiVisibility.ANY, elementType);
typePredicate, TypePredicates.anything(), failureMessage, ApiVisibility.ANY, elementType);
}

@AutoValue
abstract static class TypeToCheck {

final boolean matches(Type type, VisitorState state) {
Deque<Type> types = new ArrayDeque<>();
types.add(type);
while (!types.isEmpty()) {
Type head = types.poll();
if (typePredicate().apply(head, state)) {
return true;
final boolean matches(Type type, Type enclosingType, VisitorState state) {
// only fire this check inside certain subtypes
if (enclosingTypePredicate().apply(enclosingType, state)) {
Deque<Type> types = new ArrayDeque<>();
types.add(type);
while (!types.isEmpty()) {
Type head = types.poll();
if (typePredicate().apply(head, state)) {
return true;
}
types.addAll(head.getTypeArguments());
}
types.addAll(head.getTypeArguments());
}
// TODO(kak): do we want to check var-args as well?
return false;
}

abstract TypePredicate typePredicate();

abstract TypePredicate enclosingTypePredicate();

abstract String failureMessage();

abstract ApiVisibility visibility();
Expand Down

0 comments on commit 4f11cba

Please sign in to comment.