Check for mutable enclosing instances

MOE_MIGRATED_REVID=129176422
google · Aug 17, 2016 · c497a04 · c497a04
1 parent 6723144
commit c497a04
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 11 deletions.
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
@@ -158,7 +158,7 @@ public Description matchClass(ClassTree tree, VisitorState state) {
     // Check that the fields (including inherited fields) are immutable, and
     // validate the type hierarchy superclass.
     Violation info =
-        checkFieldsAndSupertypesForImmutability(
+        checkForImmutability(
             Optional.of(tree),
             immutableTypeParametersInScope(ASTHelpers.getSymbol(tree)),
             ASTHelpers.getType(tree),
@@ -174,14 +174,18 @@ public Description matchClass(ClassTree tree, VisitorState state) {
   }
 
   /**
-   * Check that an {@code @Immutable}-annotated class does not declare or inherit
+   * Check that an {@code @Immutable}-annotated class:
-   * any mutable fields. Check also that any immutable supertypes are instantiated
-   * with immutable type arguments as required by their containerOf spec.
    *
-   * <p>This is the only place we needed to recursively examine members of other
+   * <ul>
-   * types, since requiring supertypes to be annotated immutable would be too restrictive.
+   * <li>does not declare or inherit any mutable fields,
+   * <li>any immutable supertypes are instantiated with immutable type arguments as required by
+   *     their containerOf spec, and
+   * <li>any enclosing instances are immutable.
+   * </ul>
+   *
+   * requiring supertypes to be annotated immutable would be too restrictive.
    */
-  private Violation checkFieldsAndSupertypesForImmutability(
+  private Violation checkForImmutability(
       Optional<ClassTree> tree,
       ImmutableSet<String> immutableTyParams,
       ClassType type,
@@ -206,6 +210,30 @@ private Violation checkFieldsAndSupertypesForImmutability(
       }
     }
 
+    info = checkSuper(immutableTyParams, type, state);
+    if (info.isPresent()) {
+      return info;
+    }
+
+    Type enclosing = type.getEnclosingType();
+    while (!Type.noType.equals(enclosing)) {
+      // require the enclosing instance to be annotated @Immutable
+      // don't worry about containerOf, this isn't an explicit type use
+      System.err.println(enclosing);
+      if (getImmutableAnnotation(enclosing.tsym) == null) {
+        return info.plus(
+            String.format(
+                "'%s' has mutable enclosing instance '%s'",
+                getPrettyName(type.tsym, state), getPrettyName(enclosing.tsym, state)));
+      }
+      enclosing = enclosing.getEnclosingType();
+    }
+
+    return Violation.absent();
+  }
+
+  private Violation checkSuper(
+      ImmutableSet<String> immutableTyParams, ClassType type, VisitorState state) {
     ClassType superType = (ClassType) state.getTypes().supertype(type);
     if (superType.getKind() == TypeKind.NONE
         || state.getTypes().isSameType(state.getSymtab().objectType, superType)) {
@@ -216,7 +244,7 @@ private Violation checkFieldsAndSupertypesForImmutability(
     if (superannotation != null) {
       // If the superclass does happen to be immutable, we don't need to recursively
       // inspect it. We just have to check that it's instantiated correctly:
-      info = immutableInstantiation(immutableTyParams, superannotation, superType, state);
+      Violation info = immutableInstantiation(immutableTyParams, superannotation, superType, state);
       if (!info.isPresent()) {
         return Violation.absent();
       }
@@ -228,9 +256,8 @@ private Violation checkFieldsAndSupertypesForImmutability(
     }
 
     // Recursive case: check if the supertype is 'effectively' immutable.
-    info =
+    Violation info =
-        checkFieldsAndSupertypesForImmutability(
+        checkForImmutability(Optional.<ClassTree>absent(), immutableTyParams, superType, state);
-            Optional.<ClassTree>absent(), immutableTyParams, superType, state);
     if (!info.isPresent()) {
       return Violation.absent();
     }

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
@@ -1241,4 +1241,22 @@ public void mutableExtendsAnnotation() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void mutableEnclosing() {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            "import com.google.errorprone.annotations.Immutable;",
+            "public class Test {",
+            "  int x = 0;",
+            "  // BUG: Diagnostic contains: 'Inner' has mutable enclosing instance 'Test'",
+            "  @Immutable public class Inner {",
+            "    public int count() {",
+            "      return x++;",
+            "    }",
+            "  }",
+            "}")
+        .doTest();
+  }
 }