diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableChecker.java index 6623f705535..0de99730e2b 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableChecker.java @@ -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(); @@ -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 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 @@ -497,7 +483,16 @@ private Violation checkAnonymous(ClassTree tree, VisitorState state) { // public static <@Immutable T> ImmutableBox create(T t) { // return new ImmutableBox<>(t); // } - return areFieldsImmutable(Optional.of(tree), typarams, ASTHelpers.getType(tree), state); + ImmutableSet 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 diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/WellKnownMutability.java b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/WellKnownMutability.java index 4f16b286410..e0ba7e06868 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/WellKnownMutability.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/WellKnownMutability.java @@ -96,6 +96,7 @@ private static ImmutableMap 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") @@ -113,10 +114,13 @@ private static ImmutableMap 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(); } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableCheckerTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableCheckerTest.java index 9470ab0cc07..daeb3bbd70f 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableCheckerTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/threadsafety/ImmutableCheckerTest.java @@ -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