From d789cde0ef4a668f84d81ad648628afec53adf1e Mon Sep 17 00:00:00 2001 From: Kurt Alfred Kluever Date: Tue, 9 Mar 2021 13:23:01 -0800 Subject: [PATCH] Flag inline proto message fluent chains that are built but never used. PiperOrigin-RevId: 361883165 --- .../ProtoBuilderReturnValueIgnored.java | 4 +--- .../bugpatterns/ReturnValueIgnored.java | 14 ++++++++++--- .../ProtoBuilderReturnValueIgnoredTest.java | 21 ------------------- .../bugpatterns/ReturnValueIgnoredTest.java | 21 +++++++++++++++++++ 4 files changed, 33 insertions(+), 27 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ProtoBuilderReturnValueIgnored.java b/core/src/main/java/com/google/errorprone/bugpatterns/ProtoBuilderReturnValueIgnored.java index 126e322d582..08a53cc6803 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ProtoBuilderReturnValueIgnored.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ProtoBuilderReturnValueIgnored.java @@ -17,7 +17,6 @@ package com.google.errorprone.bugpatterns; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; -import static com.google.errorprone.BugPattern.StandardTags.FRAGILE_CODE; import static com.google.errorprone.matchers.Matchers.allOf; import static com.google.errorprone.matchers.Matchers.anyOf; import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; @@ -45,8 +44,7 @@ + "#build(), the result is discarded and the only effect is to verify that all " + "required fields are set, which can be expressed more directly with " + "#isInitialized().", - severity = ERROR, - tags = FRAGILE_CODE) + severity = ERROR) public final class ProtoBuilderReturnValueIgnored extends AbstractReturnValueIgnored { private static final Matcher BUILDER = 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 1c9da9db174..a243a12cf99 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ReturnValueIgnored.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ReturnValueIgnored.java @@ -210,10 +210,18 @@ private static boolean functionalMethod(ExpressionTree tree, VisitorState state) instanceMethod().onExactClass("java.util.Optional").named("isPresent"), instanceMethod().onExactClass("java.util.Optional").named("isEmpty")); - /** The return values of {@code ProtoMessage.newBuilder()} should always be checked. */ - // TODO(b/9467048): consolidate this with ProtoBuilderReturnValueIgnored + private static final String PROTO_MESSAGE = "com.google.protobuf.MessageLite"; + + /** + * The return values of {@code ProtoMessage.newBuilder()}, {@code protoBuilder.build()} and {@code + * protoBuilder.buildPartial()} should always be checked. + */ private static final Matcher PROTO_METHODS = - staticMethod().onClass(isDescendantOf("com.google.protobuf.MessageLite")).named("newBuilder"); + anyOf( + staticMethod().onClass(isDescendantOf(PROTO_MESSAGE)).named("newBuilder"), + instanceMethod() + .onDescendantOf(PROTO_MESSAGE + ".Builder") + .namedAnyOf("build", "buildPartial")); private static final Matcher SPECIALIZED_MATCHER = anyOf( diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ProtoBuilderReturnValueIgnoredTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ProtoBuilderReturnValueIgnoredTest.java index 1a51f4eb83a..e1e104b0ef0 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/ProtoBuilderReturnValueIgnoredTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/ProtoBuilderReturnValueIgnoredTest.java @@ -116,25 +116,4 @@ public void refactoringSecondFix() { .setFixChooser(FixChoosers.SECOND) .doTest(); } - - @Test - public void testFoo() { - helper - .addSourceLines( - "Test.java", - "import com.google.protobuf.Duration;", - "final class Test {", - " public void proto_build() {", - // TODO(b/9467048): this will later be flagged after the depot is scrubbed - " Duration.newBuilder().setSeconds(4).build();", - " Duration duration = Duration.newBuilder().setSeconds(4).build();", - " }", - " public void proto_buildPartial() {", - // TODO(b/9467048): this will later be flagged after the depot is scrubbed - " Duration.newBuilder().setSeconds(4).buildPartial();", - " Duration duration = Duration.newBuilder().setSeconds(4).buildPartial();", - " }", - "}") - .doTest(); - } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ReturnValueIgnoredTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ReturnValueIgnoredTest.java index 3feed1fd253..42294cf5940 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/ReturnValueIgnoredTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/ReturnValueIgnoredTest.java @@ -306,4 +306,25 @@ public void testProtoMessageNewBuilder() { "}") .doTest(); } + + @Test + public void testProtoMessageBuildBuildPartial() { + compilationHelper + .addSourceLines( + "Test.java", + "import com.google.protobuf.Duration;", + "final class Test {", + " public void proto_build() {", + " // BUG: Diagnostic contains: ReturnValueIgnored", + " Duration.newBuilder().setSeconds(4).build();", + " Duration duration = Duration.newBuilder().setSeconds(4).build();", + " }", + " public void proto_buildPartial() {", + " // BUG: Diagnostic contains: ReturnValueIgnored", + " Duration.newBuilder().setSeconds(4).buildPartial();", + " Duration duration = Duration.newBuilder().setSeconds(4).buildPartial();", + " }", + "}") + .doTest(); + } }