From 9f8c332d9ea67dfd31d8a91a023392f61097ae8a Mon Sep 17 00:00:00 2001
From: cpovirk <cpovirk@google.com>
Date: Thu, 24 Aug 2023 14:00:02 -0700
Subject: [PATCH] When running in conservative mode, no longer assume that
 implementations of `Map.get`, etc. return `null`.

This eliminates the problem reported in https://github.com/google/error-prone/issues/2910 for users who run the normal ("conservative") mode, but arguably we should further address that issue by adding heuristics that apply in "aggressive" mode.

PiperOrigin-RevId: 559861301
---
 .../nullness/ReturnMissingNullable.java       | 12 ++++
 .../nullness/ReturnMissingNullableTest.java   | 72 +++++++++----------
 2 files changed, 46 insertions(+), 38 deletions(-)

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java
index 8f066e93afd..e06bc53213b 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java
@@ -257,6 +257,18 @@ public Void visitMethod(MethodTree tree, Void unused) {
       }
 
       void doVisitMethod(MethodTree tree) {
+        if (beingConservative) {
+          /*
+           * In practice, we don't see any of the cases in which a compliant implementation of a
+           * method like Map.get would never return null. But we do see cases in which people have
+           * implemented Map.get to handle non-existent keys by returning "new Foo()" or by throwing
+           * IllegalArgumentException. Presumably, users of such methods would not be thrilled if we
+           * added @Nullable to their return types, given the effort that the types have gone to not
+           * to ever return null. So we avoid making such changes in conservative mode.
+           */
+          return;
+        }
+
         MethodSymbol possibleOverride = getSymbol(tree);
 
         /*
diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullableTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullableTest.java
index 37daeb2cb79..77a756d17f0 100644
--- a/core/src/test/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullableTest.java
+++ b/core/src/test/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullableTest.java
@@ -584,44 +584,6 @@ public void orElseNull() {
         .doTest();
   }
 
-  @Test
-  public void emptyToNull() {
-    createCompilationTestHelper()
-        .addSourceLines(
-            "com/google/errorprone/bugpatterns/nullness/LiteralNullReturnTest.java",
-            "package com.google.errorprone.bugpatterns.nullness;",
-            "import static com.google.common.base.Strings.emptyToNull;",
-            "class LiteralNullReturnTest {",
-            "  public String getMessage(String s) {",
-            "    // BUG: Diagnostic contains: @Nullable",
-            "    return emptyToNull(s);",
-            "  }",
-            "}")
-        .doTest();
-  }
-
-  @Test
-  public void implementsMap() {
-    createCompilationTestHelper()
-        .addSourceLines(
-            "NotMap.java", //
-            "interface NotMap {",
-            "  Integer get(String o);",
-            "}")
-        .addSourceLines(
-            "MyMap.java",
-            "import java.util.Map;",
-            "interface MyMap<K, V> extends Map<K, V>, NotMap {",
-            "  // BUG: Diagnostic contains: @Nullable",
-            "  @Override V get(Object o);",
-            "  // BUG: Diagnostic contains: @Nullable",
-            "  @Override V replace(K k, V v);",
-            "  @Override boolean replace(K k, V expect, V update);",
-            "  @Override Integer get(String o);",
-            "}")
-        .doTest();
-  }
-
   @Test
   public void implementsMapButAlwaysThrows() {
     createCompilationTestHelper()
@@ -1665,6 +1627,18 @@ public void negativeCases_suppressionAboveMethodLevel() {
         .doTest();
   }
 
+  @Test
+  public void negativeCases_implementsMapButRunningInConservativeMode() {
+    createCompilationTestHelper()
+        .addSourceLines(
+            "MyMap.java",
+            "import java.util.Map;",
+            "interface MyMap<K, V> extends Map<K, V> {",
+            "  @Override V get(Object o);",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void returnSameSymbolDifferentObjectInsideIfNull() {
     createCompilationTestHelper()
@@ -1992,6 +1966,28 @@ public void negativeCases_doesNotRemoveNecessarySuppressWarnings() {
         .doTest(TEXT_MATCH);
   }
 
+  @Test
+  public void aggressive_implementsMap() {
+    createAggressiveCompilationTestHelper()
+        .addSourceLines(
+            "NotMap.java", //
+            "interface NotMap {",
+            "  Integer get(String o);",
+            "}")
+        .addSourceLines(
+            "MyMap.java",
+            "import java.util.Map;",
+            "interface MyMap<K, V> extends Map<K, V>, NotMap {",
+            "  // BUG: Diagnostic contains: @Nullable",
+            "  @Override V get(Object o);",
+            "  // BUG: Diagnostic contains: @Nullable",
+            "  @Override V replace(K k, V v);",
+            "  @Override boolean replace(K k, V expect, V update);",
+            "  @Override Integer get(String o);",
+            "}")
+        .doTest();
+  }
+
   private CompilationTestHelper createCompilationTestHelper() {
     return CompilationTestHelper.newInstance(ReturnMissingNullable.class, getClass());
   }