Skip to content

Commit

Permalink
Improve anonymous class handling in the @immutable checker
Browse files Browse the repository at this point in the history
When checking anonymous classes, search for @immutable interfaces (not just
superclasses). Also, ignore anonymous implementations of "well-known" immutable
types to be consistent with the other subtype checking. Most well-known types
are final-ish, and the exceptions (e.g. guava's Converter) should be migrated
to use the annotation explicitly instead of being whitelisted.

Also: fix a typo, and add a couple of new whitelist entries.

RELNOTES: N/A
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=113001908
  • Loading branch information
cushon committed Jan 27, 2016
1 parent 2d94e4b commit f07000f
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 21 deletions.
Expand Up @@ -310,7 +310,7 @@ private Violation isFieldImmutable(
// accumulating the path to the error from the top-level class being checked
state.reportMatch(
buildDescription(tree.get())
.setMessage("@Immutable classes cannot have non-final field")
.setMessage("@Immutable classes cannot have non-final fields")
.addFix(SuggestedFix.addModifier(tree.get(), Modifier.FINAL, state))
.build());
return Violation.absent();
Expand Down Expand Up @@ -463,29 +463,15 @@ public Violation visitType(Type type, Void s) {

// Anonymous classes

/** Check anonymous classes whose direct supertype is {@code @Immutable}. */
/** Check anonymous implementations of {@code @Immutable} types. */
private Description handleAnonymousClass(ClassTree tree, VisitorState state) {
Violation info = checkAnonymous(tree, state);
if (!info.isPresent()) {
return Description.NO_MATCH;
}
String message = "Mutable because " + Joiner.on(", ").join(info.path());
return buildDescription(tree).setMessage(message).build();
}

private Violation checkAnonymous(ClassTree tree, VisitorState state) {
ClassSymbol sym = ASTHelpers.getSymbol(tree);
if (sym == null) {
return Violation.absent();
}
ImmutableSet<String> typarams = immutableTypeParametersInScope(sym);
Type superClass = sym.getSuperclass();
if (superClass == null) {
return Violation.absent();
return Description.NO_MATCH;
}
ImmutableAnnotationInfo annotation = getImmutableAnnotation(superClass.tsym);
if (annotation == null) {
return Violation.absent();
Type superType = immutableSupertype(sym, state);
if (superType == null) {
return Description.NO_MATCH;
}
// We don't need to check that the superclass has an immutable instantiation.
// The anonymous instance can only be referred to using a superclass type, so
Expand All @@ -497,7 +483,16 @@ private Violation checkAnonymous(ClassTree tree, VisitorState state) {
// public static <@Immutable T> ImmutableBox<T> create(T t) {
// return new ImmutableBox<>(t);
// }
return areFieldsImmutable(Optional.of(tree), typarams, ASTHelpers.getType(tree), state);
ImmutableSet<String> typarams = immutableTypeParametersInScope(sym);
Violation info =
areFieldsImmutable(Optional.of(tree), typarams, ASTHelpers.getType(tree), state);
if (!info.isPresent()) {
return Description.NO_MATCH;
}
String reason = Joiner.on(", ").join(info.path());
String message = String.format(
"Class extends @Immutable type %s, but is not immutable: %s", superType, reason);
return buildDescription(tree).setMessage(message).build();
}

// Strong behavioural subtyping
Expand Down
Expand Up @@ -96,6 +96,7 @@ private static ImmutableMap<String, ImmutableAnnotationInfo> getBootstrapClasses
.add("org.joda.time.Instant")
.add("org.joda.time.LocalDate")
.add("org.joda.time.LocalDateTime")
.add("org.joda.time.format.DateTimeFormatter")
.add(com.google.common.primitives.UnsignedLong.class)
.add(com.google.common.base.Converter.class)
.add("com.google.protobuf.ByteString")
Expand All @@ -113,10 +114,13 @@ private static ImmutableMap<String, ImmutableAnnotationInfo> getBootstrapClasses
.add(com.google.common.collect.ImmutableList.class, "E")
.add(com.google.common.collect.ImmutableMultiset.class, "E")
.add(com.google.common.collect.ImmutableMap.class, "K", "V")
.add(com.google.common.collect.ImmutableBiMap.class, "K", "V")
.add(com.google.common.collect.ImmutableMultimap.class, "K", "V")
.add(com.google.common.collect.ImmutableRangeMap.class, "K", "V")
.add(com.google.common.collect.ImmutableTable.class, "R", "C", "V")
.add(com.google.common.base.Optional.class, "T")
.add(com.google.common.base.Splitter.class)
.add(com.google.common.base.Joiner.class)
.add(com.google.common.collect.Range.class, "C")
.build();
}
Expand Down
Expand Up @@ -755,6 +755,31 @@ public void positiveAnonymous() {
.doTest();
}

@Test
public void positiveAnonymousInterface() {
compilationHelper
.addSourceLines(
"threadsafety/Super.java",
"package threadsafety;",
"import com.google.errorprone.annotations.Immutable;",
"@Immutable interface Super {",
"}")
.addSourceLines(
"threadsafety/Test.java",
"package threadsafety;",
"import com.google.errorprone.annotations.Immutable;",
"class Test {{",
" new Super() {",
" // BUG: Diagnostic contains: non-final",
" int x = 0;",
" {",
" x++;",
" }",
" };",
"}}")
.doTest();
}

@Test
public void negativeParametricAnonymous() {
compilationHelper
Expand Down

0 comments on commit f07000f

Please sign in to comment.