Only report UnnecessaryOptionalGet findings if the receivers are id…

…entical don't assume that a particular receiver symbol already corresponds to the same `Optional`, which results in false positives when the same method is called on two different instances. Fixes #2101 PiperOrigin-RevId: 351898477
google · Jan 15, 2021 · b1eaa17 · b1eaa17
1 parent 144c760
commit b1eaa17
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 15 deletions.
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryOptionalGet.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryOptionalGet.java
@@ -19,7 +19,7 @@
 import static com.google.errorprone.matchers.Matchers.anyOf;
 import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;
 import static com.google.errorprone.util.ASTHelpers.getReceiver;
-import static com.google.errorprone.util.ASTHelpers.getSymbol;
+import static com.google.errorprone.util.ASTHelpers.sameVariable;
 
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.VisitorState;
@@ -33,7 +33,6 @@
 import com.sun.source.tree.Tree.Kind;
 import com.sun.source.tree.VariableTree;
 import com.sun.source.util.TreeScanner;
-import com.sun.tools.javac.code.Symbol;
 
 /**
  * A refactoring to replace Optional.get() with lambda arg in expressions passed as arg to member
@@ -85,15 +84,14 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
     if (!onlyArg.getKind().equals(Kind.LAMBDA_EXPRESSION)) {
       return Description.NO_MATCH;
     }
-    Symbol optionalVar = getSymbol(getReceiver(tree));
     VariableTree arg = getOnlyElement(((LambdaExpressionTree) onlyArg).getParameters());
     SuggestedFix.Builder fix = SuggestedFix.builder();
     new TreeScanner<Void, VisitorState>() {
       @Override
       public Void visitMethodInvocation(
           MethodInvocationTree methodInvocationTree, VisitorState visitorState) {
         if (OPTIONAL_GET.matches(methodInvocationTree, visitorState)
-            && optionalVar.equals(getSymbol(getReceiver(methodInvocationTree)))) {
+            && sameVariable(getReceiver(tree), getReceiver(methodInvocationTree))) {
           fix.replace(methodInvocationTree, state.getSourceForNode(arg));
         }
         return super.visitMethodInvocation(methodInvocationTree, visitorState);

diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryOptionalGetTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryOptionalGetTest.java
@@ -217,17 +217,7 @@ public void genericOptionalMethods_sameMethodInvocation_replacesWithLamdaArg() {
             "    return Optional.of(\"hello\");",
             "  }",
             "}")
-        .addOutputLines(
-            "Test.java",
-            "import java.util.Optional;",
-            "public class Test {",
-            "  private void home() {",
-            "    myOpFunc().ifPresent(x -> System.out.println(x));",
-            "  }",
-            "  private Optional<String> myOpFunc() {",
-            "    return Optional.of(\"hello\");",
-            "  }",
-            "}")
+        .expectUnchanged()
         .doTest();
   }
 
@@ -266,4 +256,28 @@ public void primitiveOptionals() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void differentReceivers() {
+    refactoringTestHelper
+        .addInputLines(
+            "Test.java",
+            "import java.util.Optional;",
+            "public class Test {",
+            "  abstract static class T {",
+            "    abstract Optional<String> getValue();",
+            "  }",
+            "  static void test(T actual, T expected) {",
+            "    actual",
+            "        .getValue()",
+            "        .ifPresent(",
+            "            actualValue -> {",
+            "              String expectedValue = expected.getValue().get();",
+            "              actualValue.equals(expectedValue);",
+            "            });",
+            "  }",
+            "}")
+        .expectUnchanged()
+        .doTest();
+  }
 }