From ca56726a67ce9646be4985d1c075d728561a42c4 Mon Sep 17 00:00:00 2001 From: Nick Glorioso Date: Wed, 29 Dec 2021 13:08:43 -0800 Subject: [PATCH] Behind a disabled-by-default flag, enforce return value checking for constructors annotated with `@CheckReturnValue` and via built-in methods in the JDK. PiperOrigin-RevId: 418836367 --- .../AbstractReturnValueIgnored.java | 124 ++++++++++++++---- .../bugpatterns/CheckReturnValue.java | 5 + .../bugpatterns/ReturnValueIgnored.java | 1 + .../bugpatterns/CheckReturnValueTest.java | 107 +++++++++++++++ 4 files changed, 209 insertions(+), 28 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractReturnValueIgnored.java b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractReturnValueIgnored.java index a064efaea59..d05c28d863c 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractReturnValueIgnored.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractReturnValueIgnored.java @@ -40,9 +40,11 @@ import com.google.common.collect.ListMultimap; import com.google.common.collect.Multimap; import com.google.common.collect.MultimapBuilder; +import com.google.errorprone.ErrorProneFlags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.MemberReferenceTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.ReturnTreeMatcher; import com.google.errorprone.fixes.Fix; import com.google.errorprone.fixes.SuggestedFix; @@ -57,6 +59,7 @@ import com.sun.source.tree.MemberReferenceTree.ReferenceMode; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.MethodTree; +import com.sun.source.tree.NewClassTree; import com.sun.source.tree.ReturnTree; import com.sun.source.tree.StatementTree; import com.sun.source.tree.Tree; @@ -77,18 +80,31 @@ import java.util.Optional; import java.util.Queue; import java.util.Set; +import java.util.function.Supplier; import javax.lang.model.element.Name; import javax.lang.model.type.TypeKind; /** - * An abstract base class to match method invocations in which the return value is not used. + * An abstract base class to match API usages in which the return value is not used. + * + *

In addition to regular contexts in which a return value isn't used (e.g.: the result of {@code + * String.trim()} is just ignored), this class has the capacity to determine if the result is cast + * in such a way as to lose important static type information. + * + *

If an analysis extending this base class chooses to care about this circumstance, they can + * override {@link #lostType} to define the type information they wish to keep. * * @author eaftan@google.com (Eddie Aftandilian) */ public abstract class AbstractReturnValueIgnored extends BugChecker - implements MethodInvocationTreeMatcher, MemberReferenceTreeMatcher, ReturnTreeMatcher { + implements MethodInvocationTreeMatcher, + MemberReferenceTreeMatcher, + ReturnTreeMatcher, + NewClassTreeMatcher { + + private static final String CRV_CONSTRUCTOR_FLAG = "CheckConstructorReturnValue"; - private final java.util.function.Supplier> methodInvocationMatcher = + private final Supplier> methodInvocationMatcher = Suppliers.memoize( () -> allOf( @@ -101,23 +117,50 @@ public abstract class AbstractReturnValueIgnored extends BugChecker not(AbstractReturnValueIgnored::mockitoInvocation), not((t, s) -> allowInExceptionThrowers() && expectedExceptionTest(t, s)))); - private final java.util.function.Supplier> - memberReferenceTreeMatcher = - Suppliers.memoize( - () -> - allOf( - (t, s) -> t.getMode() == ReferenceMode.INVOKE, - AbstractReturnValueIgnored::isVoidReturningMethodReferenceExpression, - // Skip cases where the method we're referencing really does return void. - // We're only looking for cases where the referenced method does not return - // void, but it's being used on a void-returning functional interface. - not((t, s) -> isVoidType(getSymbol(t).getReturnType(), s)), - not( - (t, s) -> - allowInExceptionThrowers() - && Matchers.isThrowingFunctionalInterface( - ASTHelpers.getType(t), s)), - specializedMatcher())); + private final Supplier> memberReferenceTreeMatcher = + Suppliers.memoize( + () -> + allOf( + this::isValidMemberReferenceType, + AbstractReturnValueIgnored::isVoidReturningMethodReferenceExpression, + // Skip cases where the method we're referencing really does return void. + // We're only looking for cases where the referenced method does not return + // void, but it's being used on a void-returning functional interface. + not((t, s) -> isVoidReturningMethod(getSymbol(t), s)), + not( + (t, s) -> + allowInExceptionThrowers() + && Matchers.isThrowingFunctionalInterface(ASTHelpers.getType(t), s)), + specializedMatcher())); + + private final Supplier> lostReferenceTreeMatcher = + Suppliers.memoize( + () -> + allOf( + this::isValidMemberReferenceType, + AbstractReturnValueIgnored::isObjectReturningMethodReferenceExpression, + not((t, s) -> isExemptedInterfaceType(ASTHelpers.getType(t), s)), + not((t, s) -> Matchers.isThrowingFunctionalInterface(ASTHelpers.getType(t), s)), + specializedMatcher())); + + private final boolean checkConstructors; + + protected AbstractReturnValueIgnored() { + this(ErrorProneFlags.empty()); + } + + protected AbstractReturnValueIgnored(ErrorProneFlags flags) { + checkConstructors = flags.getBoolean(CRV_CONSTRUCTOR_FLAG).orElse(false); + } + + private boolean isValidMemberReferenceType(MemberReferenceTree mrt, VisitorState state) { + return checkConstructors || mrt.getMode() == ReferenceMode.INVOKE; + } + + private static boolean isVoidReturningMethod(MethodSymbol meth, VisitorState state) { + // Constructors "return" void but produce a real non-void value. + return !meth.isConstructor() && isVoidType(meth.getReturnType(), state); + } @Override public Description matchMethodInvocation( @@ -132,6 +175,13 @@ public Description matchMethodInvocation( return checkLostType(methodInvocationTree, state); } + @Override + public Description matchNewClass(NewClassTree newClassTree, VisitorState state) { + return checkConstructors && methodInvocationMatcher.get().matches(newClassTree, state) + ? describeReturnValueIgnored(newClassTree, state) + : NO_MATCH; + } + @Override public Description matchMemberReference(MemberReferenceTree tree, VisitorState state) { Description description = @@ -141,13 +191,7 @@ public Description matchMemberReference(MemberReferenceTree tree, VisitorState s if (!lostType(state).isPresent() || !description.equals(NO_MATCH)) { return description; } - if (allOf( - (t, s) -> t.getMode() == ReferenceMode.INVOKE, - AbstractReturnValueIgnored::isObjectReturningMethodReferenceExpression, - not((t, s) -> isExemptedInterfaceType(ASTHelpers.getType(t), s)), - not((t, s) -> Matchers.isThrowingFunctionalInterface(ASTHelpers.getType(t), s)), - specializedMatcher()) - .matches(tree, state)) { + if (lostReferenceTreeMatcher.get().matches(tree, state)) { return describeMatch(tree); } return description; @@ -251,10 +295,34 @@ protected Description describeReturnValueIgnored( protected Description describeReturnValueIgnored( MemberReferenceTree memberReferenceTree, VisitorState state) { return buildDescription(memberReferenceTree) - .setMessage(getMessage(memberReferenceTree.getName())) + .setMessage( + getMessage( + state.getName(descriptiveNameForMemberReference(memberReferenceTree, state)))) + .build(); + } + + /** + * Uses the default description for results ignored via a constructor call. Subclasses may + * override if they prefer a different description. + */ + protected Description describeReturnValueIgnored(NewClassTree newClassTree, VisitorState state) { + return buildDescription(newClassTree) + .setMessage( + String.format( + "Ignored return value of '%s'", + state.getSourceForNode(newClassTree.getIdentifier()))) .build(); } + private static String descriptiveNameForMemberReference( + MemberReferenceTree memberReferenceTree, VisitorState state) { + if (memberReferenceTree.getMode() == ReferenceMode.NEW) { + // The qualifier expression *should* just be the name of the class here + return state.getSourceForNode(memberReferenceTree.getQualifierExpression()); + } + return memberReferenceTree.getName().toString(); + } + /** * Returns the diagnostic message. Can be overridden by subclasses to provide a customized * diagnostic that includes the name of the invoked method. diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java b/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java index 3e8750e5161..293b5aecd1b 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/CheckReturnValue.java @@ -20,6 +20,7 @@ import static com.google.errorprone.util.ASTHelpers.hasDirectAnnotationWithSimpleName; import com.google.errorprone.BugPattern; +import com.google.errorprone.ErrorProneFlags; import com.google.errorprone.VisitorState; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; @@ -58,6 +59,10 @@ private static Stream shouldCheckReturnValue(Symbol sym) { return Stream.empty(); } + public CheckReturnValue(ErrorProneFlags flags) { + super(flags); + } + /** * Return a matcher for method invocations in which the method being called has the * {@code @CheckReturnValue} annotation. diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ReturnValueIgnored.java b/core/src/main/java/com/google/errorprone/bugpatterns/ReturnValueIgnored.java index 96ff685eaed..3e5ffba72c6 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ReturnValueIgnored.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ReturnValueIgnored.java @@ -383,6 +383,7 @@ private static boolean functionalMethod(ExpressionTree tree, VisitorState state) private final Matcher matcher; public ReturnValueIgnored(ErrorProneFlags flags) { + super(flags); this.matcher = anyOf( SPECIALIZED_MATCHER, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/CheckReturnValueTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/CheckReturnValueTest.java index 134c4ddb978..13dd8c6f164 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/CheckReturnValueTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/CheckReturnValueTest.java @@ -654,4 +654,111 @@ public void noCRVonClasspath() { .withClasspath(CRVTest.class, CheckReturnValueTest.class) .doTest(); } + + @Test + public void constructor_flagOff() { + compilationHelper + .addSourceLines( + "Test.java", + "class Test {", + " @com.google.errorprone.annotations.CheckReturnValue", + " public Test() {", + " }", + " public static void foo() {", + " new Test();", + " }", + "}") + .doTest(); + } + + @Test + public void constructor() { + compilationHelperLookingAtConstructors() + .addSourceLines( + "Test.java", + "class Test {", + " @com.google.errorprone.annotations.CheckReturnValue", + " public Test() {}", + " public static void foo() {", + " // BUG: Diagnostic contains: Ignored return value of 'Test'", + " new Test();", + " }", + "}") + .doTest(); + } + + @Test + public void constructor_telescoping() { + compilationHelperLookingAtConstructors() + .addSourceLines( + "Test.java", + "class Test {", + " @com.google.errorprone.annotations.CheckReturnValue", + " public Test() {}", + " public Test(int foo) { this(); }", + " public static void foo() {", + " Test foo = new Test(42);", + " }", + "}") + .doTest(); + } + + @Test + public void constructor_superCall() { + compilationHelperLookingAtConstructors() + .addSourceLines( + "Test.java", + "class Test {", + " @com.google.errorprone.annotations.CheckReturnValue", + " public Test() {}", + " static class SubTest extends Test { SubTest() { super(); } }", + " public static void foo() {", + " Test derived = new SubTest();", + " }", + "}") + .doTest(); + } + + @Test + public void constructor_throwingContexts() { + compilationHelperLookingAtConstructors() + .addSourceLines( + "Foo.java", + "@com.google.errorprone.annotations.CheckReturnValue", + "public class Foo {}") + .addSourceLines( + "Test.java", + "class Test {", + " void f() {", + " try {", + " new Foo();", + " org.junit.Assert.fail();", + " } catch (Exception expected) {}", + " org.junit.Assert.assertThrows(IllegalArgumentException.class, () -> new Foo());", + " }", + "}") + .doTest(); + } + + @Test + public void constructor_reference() { + compilationHelperLookingAtConstructors() + .addSourceLines( + "Foo.java", + "@com.google.errorprone.annotations.CheckReturnValue", + "public class Foo {}") + .addSourceLines( + "Test.java", + "class Test {", + " void f() {", + " // BUG: Diagnostic contains: Ignored return value of 'Foo', which is annotated", + " Runnable ignoresResult = Foo::new;", + " }", + "}") + .doTest(); + } + + private CompilationTestHelper compilationHelperLookingAtConstructors() { + return compilationHelper.setArgs("-XepOpt:CheckConstructorReturnValue=true"); + } }