Flag inline proto message fluent chains that are built but never used.

PiperOrigin-RevId: 361883165
sgammon · Mar 9, 2021 · d789cde · d789cde
1 parent ef9d7f8
commit d789cde
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 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
@@ -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<ExpressionTree> 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
@@ -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<ExpressionTree> 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<? super ExpressionTree> 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
@@ -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
@@ -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();
+  }
 }