Update AbstractReturnValueIgnored to detect cases where the function …

…is called in an expression-type lambda targeting a void return type.

RELNOTES: Applies CheckReturnValue, ReturnValueIgnored, FutureReturnValueIgnored, and PromiseReturnValueIgnored when a function is called in a lambda with a void return type.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=160471640

README.md

@@ -29,27 +29,23 @@ its type int is not compatible with its collection's type argument Short
 
 Our documentation is at [errorprone.info](http://errorprone.info).
 
-Error Prone works with [Bazel](http://bazel.io),
-[Maven](http://maven.apache.org), [Ant](http://ant.apache.org), and
-[Gradle](http://gradle.org). See our [installation instructions][installation]
-for details.
-
-[installation]: http://errorprone.info/docs/installation
+Error Prone works with [Bazel](http://bazel.io), [Maven]
+(http://maven.apache.org), [Ant](http://ant.apache.org), and [Gradle]
+(http://gradle.org). See our [installation instructions]
+(http://errorprone.info/docs/installation) for details.
 
 ## Developing Error Prone
 
-Developing and building Error Prone is documented on the
-[wiki](https://github.com/google/error-prone/wiki/For-Developers).
+Developing and building Error Prone is documented on the [wiki]
+(https://github.com/google/error-prone/wiki/For-Developers).
 
 ## Links
 
 -   Mailing lists
-    -   [General discussion][error-prone-discuss]
-    -   [Announcements][error-prone-announce]
+    -   [General discussion]
+        (https://groups.google.com/forum/#!forum/error-prone-discuss)
+    -   [Announcements]
+        (https://groups.google.com/forum/#!forum/error-prone-announce)
 -   [Javadoc](http://errorprone.info/api/latest/)
--   Pre-release snapshots are available from [Sonatype's snapshot
-    repository][snapshots].
-
-[error-prone-discuss]: https://groups.google.com/forum/#!forum/error-prone-discuss
-[error-prone-announce]: https://groups.google.com/forum/#!forum/error-prone-announce
-[snapshots]: https://oss.sonatype.org/content/repositories/snapshots/com/google/errorprone/
+-   Pre-release snapshots are available from [Sonatype's snapshot repository]
+    (https://oss.sonatype.org/content/repositories/snapshots/com/google/errorprone/).

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

@@ -42,13 +42,15 @@
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.IdentifierTree;
+import com.sun.source.tree.LambdaExpressionTree;
 import com.sun.source.tree.MethodInvocationTree;
 import com.sun.source.tree.StatementTree;
 import com.sun.source.tree.Tree;
 import com.sun.source.tree.Tree.Kind;
 import com.sun.tools.javac.code.Type;
 import com.sun.tools.javac.tree.JCTree.JCFieldAccess;
 import com.sun.tools.javac.tree.JCTree.JCIdent;
+import com.sun.tools.javac.tree.JCTree.JCLambda;
 import com.sun.tools.javac.tree.JCTree.JCMethodInvocation;
 import java.util.regex.Pattern;
 
@@ -64,7 +66,10 @@ public abstract class AbstractReturnValueIgnored extends BugChecker
   public Description matchMethodInvocation(
       MethodInvocationTree methodInvocationTree, VisitorState state) {
     if (allOf(
-            parentNode(Matchers.<MethodInvocationTree>kindIs(Kind.EXPRESSION_STATEMENT)),
+            parentNode(
+                anyOf(
+                    AbstractReturnValueIgnored::isVoidReturningLambdaExpression,
+                    Matchers.<Tree>kindIs(Kind.EXPRESSION_STATEMENT))),
             not(
                 methodSelect(
                     Matchers.<ExpressionTree>allOf(
@@ -78,6 +83,15 @@ public Description matchMethodInvocation(
     return Description.NO_MATCH;
   }
 
+  private static boolean isVoidReturningLambdaExpression(Tree tree, VisitorState state) {
+    if (!(tree instanceof LambdaExpressionTree)) {
+      return false;
+    }
+
+    return ASTHelpers.isVoidType(
+        state.getTypes().findDescriptorType(((JCLambda) tree).type).getReturnType(), state);
+  }
+
   /**
    * Match whatever additional conditions concrete subclasses want to match (a list of known
    * side-effect-free methods, has a @CheckReturnValue annotation, etc.).
@@ -184,7 +198,9 @@ static boolean expectedExceptionTest(Tree tree, VisitorState state) {
           allOf(enclosingNode(kindIs(Kind.TRY)), nextStatement(expressionStatement(FAIL_METHOD))),
           // assertThrows(Throwable.class, () => { me(); })
           allOf(
-              isLastStatementInBlock(),
+              anyOf(
+                  isLastStatementInBlock(),
+                  parentNode(AbstractReturnValueIgnored::isVoidReturningLambdaExpression)),
               enclosingNode(
                   // Extra kindIs is needed as methodInvocation will cast each parent node to
                   // ExpressionTree.

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

@@ -28,6 +28,8 @@
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.VariableTree;
 import com.sun.tools.javac.code.Symbol;
+import com.sun.tools.javac.code.Symbol.ClassSymbol;
+import com.sun.tools.javac.code.Symbol.VarSymbol;
 import com.sun.tools.javac.code.Type;
 import javax.lang.model.element.ElementKind;
 import javax.lang.model.element.Modifier;
@@ -58,14 +60,36 @@ public Description matchVariable(VariableTree tree, VisitorState state) {
     if (!name.equals(name.toUpperCase())) {
       return Description.NO_MATCH;
     }
-    return buildDescription(tree)
-        .addFix(SuggestedFixes.addModifiers(tree, state, Modifier.FINAL, Modifier.STATIC))
+
+    Description.Builder fixBuilder = buildDescription(tree);
+    if (canBecomeStaticMember(sym)) {
+      fixBuilder.addFix(SuggestedFixes.addModifiers(tree, state, Modifier.FINAL, Modifier.STATIC));
+    }
+    return fixBuilder
         .addFix(
             SuggestedFixes.renameVariable(
                 tree, CaseFormat.UPPER_UNDERSCORE.to(CaseFormat.LOWER_CAMEL, name), state))
         .build();
   }
 
+  private static boolean canBecomeStaticMember(VarSymbol sym) {
+    // JLS 8.1.3: It is a compile-time error if an inner class declares a member that is
+    // explicitly or implicitly static, unless the member is a constant variable (§4.12.4).
+
+    // We could try and figure out if the declaration *would* be a compile time constant if made
+    // static, but that's a bit much to keep adding this fix.
+    ClassSymbol owningClass = sym.enclClass();
+
+    // Enum anonymous classes aren't considered isInner() even though they can't hold static fields
+    switch (owningClass.getNestingKind()) {
+      case LOCAL:
+      case ANONYMOUS:
+        return false;
+      default:
+        return !owningClass.isInner();
+    }
+  }
+
   private Description checkImmutable(
       VariableTree tree, VisitorState state, Symbol.VarSymbol sym, String name) {
     Type type = sym.type;

core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java

@@ -417,7 +417,6 @@ public static ScannerSupplier errorChecks() {
           JavaLangClash.class,
           JUnit3FloatingPointComparisonWithoutDelta.class,
           JUnitAmbiguousTestClass.class,
-          LiteralClassName.class,
           LogicalAssignment.class,
           MissingFail.class,
           MissingOverride.class,
@@ -475,6 +474,7 @@ public static ScannerSupplier errorChecks() {
           IterablePathParameter.class,
           JMockTestWithoutRunWithOrRuleAnnotation.class,
           JavaxInjectOnFinalField.class,
+          LiteralClassName.class,
           LockMethodChecker.class,
           LongLiteralLowerCaseSuffix.class,
           MethodCanBeStatic.class,

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

@@ -63,6 +63,30 @@ public void rename() {
         .doTest();
   }
 
+  @Test
+  public void skipStaticFixOnInners() throws Exception {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            "class Test {",
+            "  class Inner {",
+            "    // BUG: Diagnostic matches: F",
+            "    final String CONSTANT_CASE_NAME = \"a\";",
+            "  }",
+            "  enum InnerEnum {",
+            "   FOO {",
+            "     // BUG: Diagnostic matches: F",
+            "     final String CONSTANT_CASE_NAME = \"a\";",
+            "   };",
+            "   // BUG: Diagnostic contains: static final String CAN_MAKE_STATIC",
+            "   final String CAN_MAKE_STATIC = \"\";",
+            "  }",
+            "}")
+        .expectErrorMessage(
+            "F", d -> !d.contains("static final String") && d.contains("ConstantField"))
+        .doTest();
+  }
+
   @Test
   public void negative() {
     compilationHelper

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

@@ -16,6 +16,9 @@
 
 package com.google.errorprone.bugpatterns.testdata;
 
+import java.util.function.Supplier;
+import javax.annotation.CheckReturnValue;
+
 /** @author eaftan@google.com (Eddie Aftandilian) */
 public class CheckReturnValueNegativeCases {
 
@@ -27,4 +30,21 @@ public void test1() {
 
   @SuppressWarnings("foo") // wrong annotation
   public void test2() {}
+
+  @CheckReturnValue
+  private int mustCheck() {
+    return 5;
+  }
+
+  private void callSupplier(Supplier<Integer> supplier) {
+    supplier.get();
+  }
+
+  public void testResolvedToVoidLambda() {
+    callSupplier(() -> mustCheck());
+  }
+
+  public void testMethodReference() {
+    callSupplier(this::mustCheck);
+  }
 }

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

@@ -47,6 +47,29 @@ public void testIntValue() {
     value.increment();
   }
 
+  private void callRunnable(Runnable runnable) {
+    runnable.run();
+  }
+
+  public void testResolvedToVoidLambda() {
+    // BUG: Diagnostic contains: Ignored return value
+    callRunnable(() -> this.intValue.increment());
+  }
+
+  public void testResolvedToVoidMethodReference() {
+    // TODO(b/62960293): This should be an error too, but it's tricky to adapt existing code to
+    // catch it.
+    callRunnable(this.intValue::increment);
+  }
+
+  public void testRegularLambda() {
+    callRunnable(
+        () -> {
+          // BUG: Diagnostic contains: Ignored return value
+          this.intValue.increment();
+        });
+  }
+
   public void testBeforeAndAfterRule() {
     // BUG: Diagnostic contains: remove this line
     new IntValue(1).increment();