From bc3309a7dbe95d006ee190fb36f2d654779858d4 Mon Sep 17 00:00:00 2001
From: cpovirk <cpovirk@google.com>
Date: Wed, 1 May 2024 04:59:33 -0700
Subject: [PATCH] Flag comparisons of `SomeEnum.valueOf(...)` to `null`.

(also `SomeNumber.valueOf(...)`, but that comes up very rarely, enough so that I didn't bother to explore `Boolean.valueOf` or `String.valueOf`)

PiperOrigin-RevId: 629692421
---
 .../bugpatterns/ImpossibleNullComparison.java | 20 +++++++++++++++++++
 .../ImpossibleNullComparisonTest.java         | 18 +++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ImpossibleNullComparison.java b/core/src/main/java/com/google/errorprone/bugpatterns/ImpossibleNullComparison.java
index c72b6c3ac81..6986bed107d 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/ImpossibleNullComparison.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/ImpossibleNullComparison.java
@@ -145,12 +145,14 @@ private static boolean isNull(ExpressionTree tree) {
 
   private final boolean matchTestAssertions;
   private final boolean checkPrimitives;
+  private final boolean checkValueOf;
 
   @Inject
   ImpossibleNullComparison(ErrorProneFlags flags) {
     this.matchTestAssertions =
         flags.getBoolean("ProtoFieldNullComparison:MatchTestAssertions").orElse(true);
     this.checkPrimitives = flags.getBoolean("ImmutableNullComparison:CheckPrimitives").orElse(true);
+    this.checkValueOf = flags.getBoolean("ImpossibleNullComparison:CheckValueOf").orElse(true);
   }
 
   @Override
@@ -262,6 +264,7 @@ private Optional<Fixer> getFixer(ExpressionTree tree, VisitorState state) {
       }
       return stream(GetterTypes.values())
           .filter(gt -> !gt.equals(GetterTypes.PRIMITIVE) || checkPrimitives)
+          .filter(gt -> !gt.equals(GetterTypes.VALUE_OF) || checkValueOf)
           .map(type -> type.match(resolvedTree, state))
           .filter(Objects::nonNull)
           .findFirst();
@@ -311,6 +314,12 @@ private interface Fixer {
   private static final Matcher<ExpressionTree> TABLE_COLUMN_MATCHER =
       instanceMethod().onDescendantOf("com.google.common.collect.Table").named("column");
 
+  private static final Matcher<ExpressionTree> NON_NULL_VALUE_OF =
+      staticMethod()
+          .onDescendantOfAny("java.lang.Enum", "java.lang.Number")
+          .named("valueOf")
+          .withParameters("java.lang.String");
+
   private enum GetterTypes {
     OPTIONAL_GET {
       @Nullable
@@ -394,6 +403,17 @@ Fixer match(ExpressionTree tree, VisitorState state) {
         return type != null && type.isPrimitive() ? GetterTypes::emptyFix : null;
       }
     },
+    VALUE_OF {
+      @Nullable
+      @Override
+      Fixer match(ExpressionTree tree, VisitorState state) {
+        if (!NON_NULL_VALUE_OF.matches(tree, state)) {
+          return null;
+        }
+        // TODO(cpovirk): Suggest Enums.getIfPresent, Ints.tryParse, etc.
+        return GetterTypes::emptyFix;
+      }
+    },
     /** {@code proto.getFoo()} */
     SCALAR {
       @Nullable
diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ImpossibleNullComparisonTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ImpossibleNullComparisonTest.java
index 61b418155d0..2e2e8017c03 100644
--- a/core/src/test/java/com/google/errorprone/bugpatterns/ImpossibleNullComparisonTest.java
+++ b/core/src/test/java/com/google/errorprone/bugpatterns/ImpossibleNullComparisonTest.java
@@ -497,4 +497,22 @@ public void primitives() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void valueOf() {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            "import static com.google.common.truth.Truth.assertThat;",
+            "import java.util.concurrent.TimeUnit;",
+            "public class Test {",
+            "  public void o(String s) {",
+            "    // BUG: Diagnostic contains:",
+            "    assertThat(TimeUnit.valueOf(s)).isNotNull();",
+            "    // BUG: Diagnostic contains:",
+            "    assertThat(Integer.valueOf(s)).isNotNull();",
+            "  }",
+            "}")
+        .doTest();
+  }
 }