Skip to content

Commit

Permalink
Clean up CollectionIncompatibleType's treatment of methods that accept
Browse files Browse the repository at this point in the history 
a parameterized type (e.g. Collection#containsAll(Collection<?>), and add
a cast fix for that case.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=106351805
  • Loading branch information
eaftan committed Oct 27, 2015
1 parent 2474101 commit 17215fe
Show file tree
Hide file tree
Showing 5 changed files with 95 additions and 20 deletions.
12 changes: 9 additions & 3 deletions ...one/bugpatterns/collectionincompatibletype/AbstractCollectionIncompatibleTypeMatcher.java
View file
Expand Up @@ -99,10 +99,15 @@ abstract static class MatchResult {
public abstract ExpressionTree sourceTree();
public abstract Type sourceType();
public abstract Type targetType();
public abstract AbstractCollectionIncompatibleTypeMatcher matcher();

public static MatchResult create(ExpressionTree sourceTree, Type sourceType, Type targetType) {
public static MatchResult create(
ExpressionTree sourceTree,
Type sourceType,
Type targetType,
AbstractCollectionIncompatibleTypeMatcher matcher) {
return new AutoValue_AbstractCollectionIncompatibleTypeMatcher_MatchResult(
sourceTree, sourceType, targetType);
sourceTree, sourceType, targetType, matcher);
}
}

Expand All @@ -123,7 +128,8 @@ public final MatchResult matches(MethodInvocationTree tree, VisitorState state)
return MatchResult.create(
extractSourceTree(tree, state),
extractSourceType(tree, state),
extractTargetType(tree, state));
extractTargetType(tree, state),
this);
}

/**
Expand Down
69 changes: 52 additions & 17 deletions .../google/errorprone/bugpatterns/collectionincompatibletype/CollectionIncompatibleType.java
View file
Expand Up @@ -20,11 +20,16 @@
import static com.google.errorprone.BugPattern.MaturityLevel.EXPERIMENTAL;
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;

import com.google.common.base.MoreObjects;
import com.google.common.base.Splitter;
import com.google.common.base.Verify;
import com.google.common.collect.Iterables;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.bugpatterns.collectionincompatibletype.AbstractCollectionIncompatibleTypeMatcher.MatchResult;
import com.google.errorprone.fixes.Fix;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.util.ASTHelpers;
Expand Down Expand Up @@ -64,17 +69,32 @@
)
public class CollectionIncompatibleType extends BugChecker implements MethodInvocationTreeMatcher {

private static enum FixType {
public static enum FixType {
NONE,
CAST_TO_OBJECT,
CAST,
PRINT_TYPES_AS_COMMENT,
}

private static final FixType fixType = FixType.NONE;
private final FixType fixType;

/**
* Creates a new {@link CollectionIncompatibleType} checker that provides no fix.
*/
public CollectionIncompatibleType() {
this(FixType.NONE);
}

/**
* Creates a new {@link CollectionIncompatibleType} checker with the given {@code fixType}.
*/
public CollectionIncompatibleType(FixType fixType) {
this.fixType = fixType;
}

// TODO(eaftan): Support an @CompatibleWith("K") annotation for non-JDK code, particularly Guava.

private static final Iterable<? extends AbstractCollectionIncompatibleTypeMatcher> MATCHERS =
// The "normal" case of extracting the type of a method argument
private static final Iterable<MethodArgMatcher> DIRECT_MATCHERS =
Arrays.asList(
// "Normal" cases, e.g. Collection#remove(Object)
new MethodArgMatcher("java.util.Collection", "contains(java.lang.Object)", 0, 0),
Expand All @@ -92,10 +112,12 @@ private static enum FixType {
new MethodArgMatcher("java.util.Stack", "search(java.lang.Object)", 0, 0),
new MethodArgMatcher("java.util.Vector", "indexOf(java.lang.Object,int)", 0, 0),
new MethodArgMatcher("java.util.Vector", "lastIndexOf(java.lang.Object,int)", 0, 0),
new MethodArgMatcher("java.util.Vector", "removeElement(java.lang.Object)", 0, 0),
new MethodArgMatcher("java.util.Vector", "removeElement(java.lang.Object)", 0, 0));

// Cases where we need to extract the type argument from a method argument, e.g.
// Collection#containsAll(Collection<?>)
// Cases where we need to extract the type argument from a method argument, e.g.
// Collection#containsAll(Collection<?>)
private static final Iterable<TypeArgOfMethodArgMatcher> TYPE_ARG_MATCHERS =
Arrays.asList(
new TypeArgOfMethodArgMatcher(
"java.util.Collection", // class that defines the method
"containsAll(java.util.Collection<?>)", // method signature
Expand All @@ -122,21 +144,23 @@ private static enum FixType {
@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {

MatchResult result = firstNonNullMatchResult(MATCHERS, tree, state);
if (result == null) {
MatchResult directResult = firstNonNullMatchResult(DIRECT_MATCHERS, tree, state);
MatchResult typeArgResult = null;
if (directResult == null) {
typeArgResult = firstNonNullMatchResult(TYPE_ARG_MATCHERS, tree, state);
}
if (directResult == null && typeArgResult == null) {
return Description.NO_MATCH;
}
Verify.verify(directResult == null ^ typeArgResult == null);
MatchResult result = MoreObjects.firstNonNull(directResult, typeArgResult);

Types types = state.getTypes();
if (types.isCastable(
result.sourceType(), types.erasure(ASTHelpers.getUpperBound(result.targetType(), types)))) {
return Description.NO_MATCH;
}

boolean useMessageTemplateWithGenerics = !types.isSameType(
ASTHelpers.getType(result.sourceTree()),
result.sourceType());

// For error message, use simple names instead of fully qualified names unless they are
// identical.
String sourceTreeType = getSimpleName(ASTHelpers.getType(result.sourceTree()));
Expand All @@ -148,7 +172,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
}

Description.Builder description = buildDescription(tree);
if (useMessageTemplateWithGenerics) {
if (typeArgResult != null) {
description.setMessage(
String.format(
"Argument '%s' should not be passed to this method; its type %s has a type argument "
Expand Down Expand Up @@ -177,11 +201,22 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
ASTHelpers.getUpperBound(result.targetType(), types),
result.sourceType())));
break;
case CAST_TO_OBJECT:
description.addFix(SuggestedFix.prefixWith(result.sourceTree(), "(Object) "));
case CAST:
Fix fix;
if (typeArgResult != null) {
TypeArgOfMethodArgMatcher matcher = (TypeArgOfMethodArgMatcher) typeArgResult.matcher();
String fullyQualifiedType = matcher.getMethodArgTypeName();
String simpleType = Iterables.getLast(Splitter.on('.').split(fullyQualifiedType));
fix = SuggestedFix.builder()
.prefixWith(result.sourceTree(), String.format("(%s<?>) ", simpleType))
.addImport(fullyQualifiedType)
.build();
} else {
fix = SuggestedFix.prefixWith(result.sourceTree(), "(Object) ");
}
description.addFix(fix);
break;
case NONE:
// No fix
break;

}
Expand Down
4 changes: 4 additions & 0 deletions ...m/google/errorprone/bugpatterns/collectionincompatibletype/TypeArgOfMethodArgMatcher.java
View file
Expand Up @@ -96,4 +96,8 @@ Type extractTargetType(MethodInvocationTree tree, VisitorState state) {
receiverTypeArgIndex,
state.getTypes());
}

String getMethodArgTypeName() {
return methodArgTypeName;
}
}
25 changes: 25 additions & 0 deletions ...gle/errorprone/bugpatterns/collectionincompatibletype/CollectionIncompatibleTypeTest.java
View file
Expand Up @@ -17,6 +17,9 @@
package com.google.errorprone.bugpatterns.collectionincompatibletype;

import com.google.errorprone.CompilationTestHelper;
import com.google.errorprone.bugpatterns.collectionincompatibletype.CollectionIncompatibleType.FixType;
import com.google.errorprone.scanner.ErrorProneScanner;
import com.google.errorprone.scanner.ScannerSupplier;

import org.junit.Before;
import org.junit.Ignore;
Expand Down Expand Up @@ -58,6 +61,28 @@ public void testClassCast() {
compilationHelper.addSourceFile("CollectionIncompatibleTypeClassCast.java").doTest();
}

@Test
public void testCastFixes() {
CompilationTestHelper compilationHelperWithCastFix =
CompilationTestHelper.newInstance(
ScannerSupplier.fromScanner(
new ErrorProneScanner(new CollectionIncompatibleType(FixType.CAST))),
getClass());
compilationHelperWithCastFix
.addSourceLines(
"Test.java",
"import java.util.Collection;",
"public class Test {",
" public void doIt(Collection<String> c1, Collection<Integer> c2) {",
" // BUG: Diagnostic contains: c1.contains((Object) 1);",
" c1.contains(1);",
" // BUG: Diagnostic contains: c1.containsAll((Collection<?>) c2);",
" c1.containsAll(c2);",
" }",
"}")
.doTest();
}

// This test is disabled because calling Types#asSuper in the check removes the upper bound on K.
@Test
@Ignore
Expand Down
5 changes: 5 additions & 0 deletions ...patterns/collectionincompatibletype/testdata/CollectionIncompatibleTypeNegativeCases.java
View file
Expand Up @@ -237,5 +237,10 @@ public void methodArgHasWildcardTypeArgument(
Collection<? extends Number> collection1, Collection<? extends Integer> collection2) {
collection1.containsAll(collection2);
}

public void methodArgCastToCollectionWildcard(
Collection<Integer> collection1, Collection<String> collection2) {
collection1.containsAll((Collection<?>) collection2);
}

}

0 comments on commit 17215fe

Please sign in to comment.