When running in conservative mode, no longer assume that implementati…

…ons of `Map.get`, etc. return `null`. This eliminates the problem reported in #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
google · Aug 24, 2023 · 9f8c332 · 9f8c332
1 parent 032b938
commit 9f8c332
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 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
@@ -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
@@ -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());
   }