ConstructorInvokesOverridable: fix some false positives

Do not match methods of anonymous classes.  Those methods can't be
overriden because those classes can't be extended.

Do not match methods of certain enum classes.  It is not possible to
override the methods of an enum class except in the (anonymous) class
bodies of its enum constants.  If all the enum constants lack class
bodies, then the enum class's methods can't be overridden.

Fix cases where the bug pattern would become confused about the receiver
of unqualified or this-qualified method invocations, and where it would
wrongly conclude that the method belonged to the instance under
construction and issue a warning.

Support @SuppressWarnings("ConstructorInvokesOverridable") on more
elements.  Fixes google#656.

core/src/main/java/com/google/errorprone/bugpatterns/ConstructorInvokesOverridable.java

@@ -18,6 +18,8 @@
 
 import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
 
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.util.ASTHelpers;
@@ -26,10 +28,15 @@
 import com.sun.source.tree.IdentifierTree;
 import com.sun.source.tree.MemberSelectTree;
 import com.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.MethodTree;
+import com.sun.source.tree.NewClassTree;
 import com.sun.source.tree.Tree;
+import com.sun.source.tree.VariableTree;
 import com.sun.source.util.TreeScanner;
 import com.sun.tools.javac.code.Symbol.ClassSymbol;
 import com.sun.tools.javac.code.Symbol.MethodSymbol;
+import java.util.ArrayDeque;
+import java.util.Deque;
 import javax.lang.model.element.Modifier;
 import javax.lang.model.element.Name;
 
@@ -43,6 +50,9 @@
     severity = WARNING)
 public class ConstructorInvokesOverridable extends ConstructorLeakChecker {
 
+  private static final ImmutableSet<Modifier> ENUM_CONSTANT_MODIFIERS =
+      Sets.immutableEnumSet(Modifier.PUBLIC, Modifier.STATIC, Modifier.FINAL);
+
   @Override
   protected void traverse(Tree tree, VisitorState state) {
     // If class is final, no method is overridable.
@@ -51,43 +61,113 @@ protected void traverse(Tree tree, VisitorState state) {
       return;
     }
     ClassSymbol classSym = ASTHelpers.getSymbol(classTree);
+    // If the class is anonymous, no method is overridable.
+    //
+    //   "it is impossible to declare a subclass of an anonymous class, despite
+    //    an anonymous class being non-final, because an anonymous class cannot
+    //    be named by an extends clause"
+    //
+    // https://docs.oracle.com/javase/specs/jls/se11/html/jls-15.html#jls-15.9.5
+    if (classSym.isAnonymous()) {
+      return;
+    }
+    //  "An enum declaration is implicitly final unless it contains at least one
+    //   enum constant that has a class body"
+    //
+    // https://docs.oracle.com/javase/specs/jls/se11/html/jls-8.html#jls-8.9
+    if (classSym.isEnum()) {
+      boolean hasSubclass =
+          classTree.getMembers().stream()
+              .filter(VariableTree.class::isInstance)
+              .map(VariableTree.class::cast)
+              .filter(variableTree -> variableTree.getModifiers().getFlags().containsAll(ENUM_CONSTANT_MODIFIERS))
+              .filter(variableTree -> classSym.type.equals(ASTHelpers.getType(variableTree)))
+              .map(VariableTree::getInitializer)
+              .filter(NewClassTree.class::isInstance)
+              .map(NewClassTree.class::cast)
+              .anyMatch(newClassTree -> newClassTree.getClassBody() != null);
+
+      if (!hasSubclass) {
+        return;
+      }
+    }
+
+    Deque<ClassSymbol> nestedClasses = new ArrayDeque<>();
 
     tree.accept(
         new TreeScanner<Void, Void>() {
           @Override
-          public Void visitMethodInvocation(MethodInvocationTree node, Void data) {
-            MethodSymbol method = ASTHelpers.getSymbol(node);
-            if (method != null
-                && !method.isConstructor()
-                && !method.isStatic()
-                && !method.isPrivate()
-                && !method.getModifiers().contains(Modifier.FINAL)
-                && isOnThis(node)
-                && method.isMemberOf(classSym, state.getTypes())) {
+          public Void visitClass(ClassTree node, Void unused) {
+            if (isSuppressed(node)) {
+              return null;
+            }
+            nestedClasses.push(ASTHelpers.getSymbol(node));
+            super.visitClass(node, null);
+            nestedClasses.pop();
+            return null;
+          }
+
+          @Override
+          public Void visitMethod(MethodTree node, Void unused) {
+            if (isSuppressed(node)) {
+              return null;
+            }
+            return super.visitMethod(node, null);
+          }
+
+          @Override
+          public Void visitMethodInvocation(MethodInvocationTree node, Void unused) {
+            if (isOverridable(node)) {
               state.reportMatch(describeMatch(node));
             }
-            return super.visitMethodInvocation(node, data);
+            return super.visitMethodInvocation(node, null);
+          }
+
+          @Override
+          public Void visitVariable(VariableTree node, Void unused) {
+            if (isSuppressed(node)) {
+              return null;
+            }
+            return super.visitVariable(node, null);
+          }
+
+          private boolean isOverridable(MethodInvocationTree node) {
+            MethodSymbol method = ASTHelpers.getSymbol(node);
+            if (method == null
+                || method.isConstructor()
+                || method.isStatic()
+                || method.isPrivate()
+                || method.getModifiers().contains(Modifier.FINAL)
+                || !method.isMemberOf(classSym, state.getTypes())) {
+              return false;
+            }
+
+            ExpressionTree receiver = ASTHelpers.getReceiver(node);
+            if (receiver != null) {
+              Name receiverName;
+              switch (receiver.getKind()) {
+                case IDENTIFIER:
+                  receiverName = ((IdentifierTree) receiver).getName();
+                  break;
+                case MEMBER_SELECT:
+                  receiverName = ((MemberSelectTree) receiver).getIdentifier();
+                  break;
+                default:
+                  return false;
+              }
+              return (receiverName.contentEquals("this") || receiverName.contentEquals("super"))
+                  && classSym.equals(ASTHelpers.getReceiverType(node).tsym);
+            }
+
+            for (ClassSymbol nestedClass : nestedClasses) {
+              if (method.isMemberOf(nestedClass, state.getTypes())) {
+                return false;
+              }
+            }
+
+            return true;
           }
         },
         null);
   }
-
-  private static boolean isOnThis(MethodInvocationTree tree) {
-    ExpressionTree receiver = ASTHelpers.getReceiver(tree);
-    if (receiver == null) {
-      return true;
-    }
-    Name receiverName;
-    switch (receiver.getKind()) {
-      case IDENTIFIER:
-        receiverName = ((IdentifierTree) receiver).getName();
-        break;
-      case MEMBER_SELECT:
-        receiverName = ((MemberSelectTree) receiver).getIdentifier();
-        break;
-      default:
-        return false;
-    }
-    return receiverName.contentEquals("this") || receiverName.contentEquals("super");
-  }
 }

core/src/test/java/com/google/errorprone/bugpatterns/testdata/ConstructorInvokesOverridableNegativeCases.java

@@ -38,9 +38,50 @@ public ConstructorInvokesOverridableNegativeCases() {
     safeStatic();
     safePrivate();
 
+    @SuppressWarnings("ConstructorInvokesOverridable")
+    class Suppressed {
+      final int suppressed = unsafe();
+    }
+
+    class SuppressedMembers {
+      @SuppressWarnings("ConstructorInvokesOverridable")
+      final int suppressed = unsafe();
+
+      @SuppressWarnings("ConstructorInvokesOverridable")
+      int suppressed() {
+        return unsafe();
+      }
+    }
+
     // Safe: on a different instance.
     new ConstructorInvokesOverridableNegativeCases().localVariable();
 
+    new ConstructorInvokesOverridableNegativeCases() {
+      // Safe: calls its own method and cannot be subclassed because it's anonymous.
+      final int i = unsafe();
+      final int j = this.unsafe();
+    };
+
+    final class Local extends ConstructorInvokesOverridableNegativeCases {
+      // Safe: calls its own method and cannot be subclassed because it's final.
+      final int i = unsafe();
+      final int j = this.unsafe();
+      final int k = Local.this.unsafe();
+    }
+
+    class Parent extends ConstructorInvokesOverridableNegativeCases {
+      class Inner extends ConstructorInvokesOverridableNegativeCases {
+        // OK to call an overridable method of the containing class
+        final int i = Parent.this.unsafe();
+      }
+    }
+
+    new java.util.HashMap<String, String>() {
+      {
+        put("Hi", "Mom");
+      }
+    };
+
     new Thread() {
       @Override
       public void run() {
@@ -96,11 +137,33 @@ class Local {
         unsafe();
       }
     }
+    class Local2 extends ConstructorInvokesOverridableNegativeCases {
+      {
+        // same as above, but try to confuse the bug pattern
+        ConstructorInvokesOverridableNegativeCases.this.unsafe();
+      }
+    }
   }
 
   // Lookup is handled correctly for inner classes as well
   class Inner {
     // OK to call an overridable method of the containing class
     final int safeValue = unsafe();
   }
+  class Inner2 extends ConstructorInvokesOverridableNegativeCases {
+    // same as above, but try to confuse the bug pattern
+    final int safeValue = ConstructorInvokesOverridableNegativeCases.this.unsafe();
+  }
+
+  enum AnEnum implements java.util.function.IntSupplier {
+    INSTANCE;
+
+    final String s = name();
+    final int i = getAsInt();
+
+    @Override
+    public int getAsInt() {
+      return s.length();
+    }
+  }
 }

core/src/test/java/com/google/errorprone/bugpatterns/testdata/ConstructorInvokesOverridablePositiveCases.java

@@ -47,6 +47,24 @@ public void run() {
 
     // BUG: Diagnostic contains: Constructors should not invoke overridable
     new Thread(() -> unsafe()).start();
+
+    // final but calls our method
+    final class Local1 {
+      // BUG: Diagnostic contains: Constructors should not invoke overridable
+      final int i = unsafe();
+    }
+
+    // final and implements the method but calls ours
+    final class Local2 extends ConstructorInvokesOverridablePositiveCases {
+      // BUG: Diagnostic contains: Constructors should not invoke overridable
+      final int i = ConstructorInvokesOverridablePositiveCases.this.unsafe();
+    }
+
+    // implements and calls its own method, but non-final
+    class Local3 extends ConstructorInvokesOverridablePositiveCases {
+      // BUG: Diagnostic contains: Constructors should not invoke overridable
+      final int i = unsafe();
+    }
   }
 
   protected int unsafe() {
@@ -71,4 +89,18 @@ protected int innerUnsafe() {
       return 7;
     }
   }
+
+  enum AnEnum implements java.util.function.IntSupplier {
+    INSTANCE {
+      final String s = name();
+
+      @Override
+      public int getAsInt() {
+        return s.length();
+      }
+    };
+
+    // BUG: Diagnostic contains: Constructors should not invoke overridable
+    final int i = getAsInt();
+  }
 }