Skip to content

Commit

Permalink
Add a bunch more mutable types to the AutoValueImmutableFields matcher.
Browse files Browse the repository at this point in the history 
RELNOTES: Add a bunch more mutable types to the AutoValueImmutableFields matcher.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=226030851
  • Loading branch information
@kluever @ronshapiro
kluever authored and ronshapiro committed Dec 24, 2018
1 parent b56cdf3 commit 107b0db
Show file tree
Hide file tree
Showing 2 changed files with 113 additions and 51 deletions.
143 changes: 92 additions & 51 deletions core/src/main/java/com/google/errorprone/bugpatterns/AutoValueImmutableFields.java
View file
Expand Up @@ -19,20 +19,23 @@
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
import static com.google.errorprone.matchers.Description.NO_MATCH; import static com.google.errorprone.matchers.Description.NO_MATCH;
import static com.google.errorprone.matchers.Matchers.allOf; import static com.google.errorprone.matchers.Matchers.allOf;
import static com.google.errorprone.matchers.Matchers.isArrayType;
import static com.google.errorprone.matchers.Matchers.methodReturns;
import static com.google.errorprone.suppliers.Suppliers.typeFromString;


import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableListMultimap;
import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern;
import com.google.errorprone.BugPattern.ProvidesFix; import com.google.errorprone.BugPattern.ProvidesFix;
import com.google.errorprone.VisitorState; import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;
import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.Matchers; import com.google.errorprone.matchers.Matchers;
import com.google.errorprone.suppliers.Suppliers;
import com.google.errorprone.util.ASTHelpers; import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ClassTree; import com.sun.source.tree.ClassTree;
import com.sun.source.tree.MethodTree; import com.sun.source.tree.MethodTree;
import com.sun.source.tree.Tree; import com.sun.source.tree.Tree;
import java.util.Map;
import javax.lang.model.element.Modifier; import javax.lang.model.element.Modifier;


/** /**
Expand All @@ -48,65 +51,103 @@
public class AutoValueImmutableFields extends BugChecker implements ClassTreeMatcher { public class AutoValueImmutableFields extends BugChecker implements ClassTreeMatcher {


private static final String MESSAGE = private static final String MESSAGE =
"Value objects should be immutable, so if a property of one" "AutoValue instances should be deeply immutable. Therefore, we recommend returning a %s "
+ " is a collection then that collection should be immutable too." + "instead. Read more at http://goo.gl/qWo9sC";
+ " Please return %s here. Read more at "
+ "https://github.com/google/auto/blob/master/value/userguide/builders-howto.md#-use-a-collection-valued-property";


private static final ImmutableMap<Matcher<MethodTree>, String> TYPE_MATCHER_TO_REPLACEMENT_MAP = private static final ImmutableListMultimap<String, Matcher<MethodTree>> REPLACEMENT_TO_MATCHERS =
ImmutableMap.<Matcher<MethodTree>, String>builder() ImmutableListMultimap.<String, Matcher<MethodTree>>builder()
.put(Matchers.methodReturns(Matchers.isArrayType()), "ImmutableList") .put("ImmutableCollection", returning("java.util.Collection"))
.put( .putAll(
Matchers.methodReturns(Suppliers.typeFromString("java.util.Collection")), "ImmutableList",
"ImmutableCollection") methodReturns(isArrayType()),
.put(Matchers.methodReturns(Suppliers.typeFromString("java.util.List")), "ImmutableList") returning("java.util.List"),
.put(Matchers.methodReturns(Suppliers.typeFromString("java.util.Map")), "ImmutableMap") returning("java.util.ArrayList"),
.put( returning("java.util.LinkedList"),
Matchers.methodReturns( returning("com.google.common.collect.ImmutableList.Builder"))
Suppliers.typeFromString("com.google.common.collect.Multimap")), .putAll(
"ImmutableMultimap") "ImmutableMap",
.put( returning("java.util.Map"),
Matchers.methodReturns( returning("java.util.HashMap"),
Suppliers.typeFromString("com.google.common.collect.ListMultimap")), returning("java.util.LinkedHashMap"),
"ImmutableListMultimap") returning("com.google.common.collect.ImmutableMap.Builder"))
.put( .putAll(
Matchers.methodReturns( "ImmutableSortedMap",
Suppliers.typeFromString("com.google.common.collect.SetMultimap")), returning("java.util.SortedMap"),
"ImmutableSetMultimap") returning("java.util.TreeMap"),
.put( returning("com.google.common.collect.ImmutableSortedMap.Builder"))
Matchers.methodReturns( .putAll(
Suppliers.typeFromString("com.google.common.collect.Multiset")), "ImmutableBiMap",
"ImmutableMultiset") returning("com.google.common.collect.BiMap"),
.put(Matchers.methodReturns(Suppliers.typeFromString("java.util.Set")), "ImmutableSet") returning("com.google.common.collect.ImmutableBiMap.Builder"))
.put( .putAll(
Matchers.methodReturns(Suppliers.typeFromString("com.google.common.collect.Table")), "ImmutableSet",
"ImmutableTable") returning("java.util.Set"),
returning("java.util.HashSet"),
returning("java.util.LinkedHashSet"),
returning("com.google.common.collect.ImmutableSet.Builder"))
.putAll(
"ImmutableSortedSet",
returning("java.util.SortedSet"),
returning("java.util.TreeSet"),
returning("com.google.common.collect.ImmutableSortedSet.Builder"))
.putAll(
"ImmutableMultimap",
returning("com.google.common.collect.Multimap"),
returning("com.google.common.collect.ImmutableMultimap.Builder"))
.putAll(
"ImmutableListMultimap",
returning("com.google.common.collect.ListMultimap"),
returning("com.google.common.collect.ImmutableListMultimap.Builder"))
.putAll(
"ImmutableSetMultimap",
returning("com.google.common.collect.SetMultimap"),
returning("com.google.common.collect.ImmutableSetMultimap.Builder"))
.putAll(
"ImmutableMultiset",
returning("com.google.common.collect.Multiset"),
returning("com.google.common.collect.ImmutableMultiset.Builder"))
.putAll(
"ImmutableSortedMultiset",
returning("com.google.common.collect.SortedMultiset"),
returning("com.google.common.collect.ImmutableSortedMultiset.Builder"))
.putAll(
"ImmutableTable",
returning("com.google.common.collect.Table"),
returning("com.google.common.collect.ImmutableTable.Builder"))
.putAll(
"ImmutableRangeMap",
returning("com.google.common.collect.RangeMap"),
returning("com.google.common.collect.ImmutableRangeMap.Builder"))
.putAll(
"ImmutablePrefixTrie",
returning("com.google.common.collect.PrefixTrie"),
returning("com.google.common.collect.ImmutablePrefixTrie.Builder"))
.build(); .build();


private static Matcher<MethodTree> returning(String type) {
return methodReturns(typeFromString(type));
}

private static final Matcher<MethodTree> METHOD_MATCHER = private static final Matcher<MethodTree> METHOD_MATCHER =
allOf( allOf(
Matchers.<MethodTree>hasModifier(Modifier.PUBLIC), Matchers.<MethodTree>hasModifier(Modifier.PUBLIC),
Matchers.<MethodTree>hasModifier(Modifier.ABSTRACT)); Matchers.<MethodTree>hasModifier(Modifier.ABSTRACT));


@Override @Override
public Description matchClass(ClassTree tree, VisitorState state) { public Description matchClass(ClassTree tree, VisitorState state) {
if (!ASTHelpers.hasAnnotation(tree, "com.google.auto.value.AutoValue", state)) { if (ASTHelpers.hasAnnotation(tree, "com.google.auto.value.AutoValue", state)) {
return NO_MATCH; for (Tree memberTree : tree.getMembers()) {
} if (memberTree instanceof MethodTree) {
for (Tree memberTree : tree.getMembers()) { MethodTree methodTree = (MethodTree) memberTree;
if (!(memberTree instanceof MethodTree)) { if (METHOD_MATCHER.matches(methodTree, state)) {
continue; for (Map.Entry<String, Matcher<MethodTree>> entry : REPLACEMENT_TO_MATCHERS.entries()) {
} if (entry.getValue().matches(methodTree, state)) {
MethodTree methodTree = (MethodTree) memberTree; return buildDescription(methodTree)
if (!METHOD_MATCHER.matches(methodTree, state)) { .setMessage(String.format(MESSAGE, entry.getKey()))
continue; .build();
} }
for (ImmutableMap.Entry<Matcher<MethodTree>, String> entry : }
TYPE_MATCHER_TO_REPLACEMENT_MAP.entrySet()) { }
if (entry.getKey().matches(methodTree, state)) {
return buildDescription(methodTree)
.setMessage(String.format(MESSAGE, entry.getValue()))
.build();
} }
} }
} }
Expand Down
21 changes: 21 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/AutoValueImmutableFieldsTest.java
View file
Expand Up @@ -190,6 +190,27 @@ public void matchesTable() {
.doTest(); .doTest();
} }


@Test
public void matchesTwoProperties() {
compilationHelper
.addSourceLines(
"in/Test.java",
"import com.google.auto.value.AutoValue;",
"import java.util.Map;",
"import java.util.Set;",
"@AutoValue",
"abstract class Test {",
" // BUG: Diagnostic contains: ImmutableSet",
" public abstract Set<String> countriesSet();",
// TODO(kak): We should show an error here too, but since we scan from the ClassTree
// downward, we only can report a single match for the entire class. We should consider
// re-writing to scan each method instead (and check "upward" to see if the enclosing
// class has the @AutoValue annotation).
" public abstract Map<String, String> countriesMap();",
"}")
.doTest();
}

@Test @Test
public void noMatches() { public void noMatches() {
compilationHelper compilationHelper
Expand Down

0 comments on commit 107b0db

Please sign in to comment.