MOE Sync 2019-12-18

check_api/src/main/java/com/google/errorprone/matchers/Matchers.java

@@ -601,8 +601,11 @@ public static Matcher<ExpressionTree> booleanConstant(boolean value) {
         Symbol symbol = getSymbol(expressionTree);
         if (symbol.isStatic()
             && state.getTypes().unboxedTypeOrType(symbol.type).getTag() == TypeTag.BOOLEAN) {
-          return ((value && symbol.getSimpleName().contentEquals("TRUE"))
-              || symbol.getSimpleName().contentEquals("FALSE"));
+          if (value) {
+            return symbol.getSimpleName().contentEquals("TRUE");
+          } else {
+            return symbol.getSimpleName().contentEquals("FALSE");
+          }
         }
       }
       return false;

check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java

@@ -1274,11 +1274,12 @@ public static ImmutableSet<String> getGeneratedBy(VisitorState state) {
     return getGeneratedBy(getSymbol(outerClass), state);
   }
 
+
   /**
    * Returns the values of the given symbol's {@code javax.annotation.Generated} or {@code
    * javax.annotation.processing.Generated} annotation, if present.
    */
-  public static ImmutableSet<String> getGeneratedBy(ClassSymbol symbol, VisitorState state) {
+  public static ImmutableSet<String> getGeneratedBy(Symbol symbol, VisitorState state) {
     checkNotNull(symbol);
     Optional<Compound> c =
         Stream.of("javax.annotation.Generated", "javax.annotation.processing.Generated")

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,115 @@ 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/main/java/com/google/errorprone/bugpatterns/ExtendsAutoValue.java

@@ -16,6 +16,8 @@
 
 package com.google.errorprone.bugpatterns;
 
+import static com.google.common.collect.Streams.stream;
+
 import com.google.common.collect.ImmutableSet;
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.BugPattern.SeverityLevel;
@@ -26,6 +28,7 @@
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.ClassTree;
 import com.sun.tools.javac.util.Name;
+import java.util.Objects;
 
 /** Makes sure that you are not extending a class that has @AutoValue as an annotation. */
 @BugPattern(
@@ -43,24 +46,29 @@ public final class ExtendsAutoValue extends BugChecker implements ClassTreeMatch
 
   @Override
   public Description matchClass(ClassTree tree, VisitorState state) {
-
-    if (!ASTHelpers.getGeneratedBy(ASTHelpers.getSymbol(tree), state).isEmpty()) {
-      // Skip generated code. Yes, I know we can do this via a flag but we should always ignore
-      // generated code, so to be sure, manually check it.
-      return Description.NO_MATCH;
-    }
-
     if (tree.getExtendsClause() == null) {
       // Doesn't extend anything, can't possibly be a violation.
       return Description.NO_MATCH;
     }
+
     if (!ASTHelpers.annotationsAmong(
             ASTHelpers.getSymbol(tree.getExtendsClause()), AUTOS.get(state), state)
         .isEmpty()) {
       // Violation: one of its superclasses extends AutoValue.
-      return buildDescription(tree).build();
+      if (!isInGeneratedCode(state)) {
+        return buildDescription(tree).build();
+      }
     }
 
     return Description.NO_MATCH;
   }
+
+  private static boolean isInGeneratedCode(VisitorState state) {
+    // Skip generated code. Yes, I know we can do this via a flag but we should always ignore
+    // generated code, so to be sure, manually check it.
+    return stream(state.getPath())
+        .map(ASTHelpers::getSymbol)
+        .filter(Objects::nonNull)
+        .anyMatch(sym -> !ASTHelpers.getGeneratedBy(sym, state).isEmpty());
+  }
 }

core/src/test/java/com/google/errorprone/bugpatterns/ExtendsAutoValueTest.java

@@ -150,4 +150,37 @@ public void extendsAutoValue_badButSuppressed() {
             "public class TestClass extends AutoClass {}")
         .doTest();
   }
+
+  @Test
+  public void extendsAutoValue_innerClassExtends() {
+    helper
+        .addSourceLines(
+            "TestClass.java",
+            "import com.google.auto.value.AutoValue;",
+            "@AutoValue class AutoClass {}",
+            "",
+            "public class TestClass {",
+            "  // BUG: Diagnostic contains: ExtendsAutoValue",
+            "  public class Extends extends AutoClass {}",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void extendsAutoValue_innerClassExtends_generated() {
+    helper
+        .addSourceLines(
+            "TestClass.java",
+            "import com.google.auto.value.AutoValue;",
+            (RuntimeVersion.isAtLeast9()
+                ? "import javax.annotation.processing.Generated;"
+                : "import javax.annotation.Generated;"),
+            "@AutoValue class AutoClass {}",
+            "",
+            "@Generated(\"generator\")",
+            "public class TestClass {",
+            "  public class Extends extends AutoClass {}",
+            "}")
+        .doTest();
+  }
 }

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,34 @@ 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();
+    }
+  }
 }