Fix treatment of non-trivial receivers of method selects in Unused.

RELNOTES: Fix a false positive in Unused. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=211768635
google · Sep 12, 2018 · c6c010f · c6c010f
1 parent 45cc93d
commit c6c010f
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 31 deletions.
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/Unused.java b/core/src/main/java/com/google/errorprone/bugpatterns/Unused.java
@@ -60,7 +60,6 @@
 import com.sun.source.tree.EnhancedForLoopTree;
 import com.sun.source.tree.ErroneousTree;
 import com.sun.source.tree.ExpressionStatementTree;
-import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.IdentifierTree;
 import com.sun.source.tree.LambdaExpressionTree;
 import com.sun.source.tree.MemberReferenceTree;
@@ -470,20 +469,18 @@ public Void visitAssignment(AssignmentTree tree, Void unused) {
 
       @Override
       public Void visitMemberSelect(MemberSelectTree memberSelectTree, Void unused) {
-        super.visitMemberSelect(memberSelectTree, null);
         Symbol symbol = getSymbol(memberSelectTree);
         if (isUsed(symbol)) {
           unusedElements.remove(symbol);
-        } else {
-          if (currentExpressionStatement != null && unusedElements.containsKey(symbol)) {
-            usageSites.put(symbol, currentExpressionStatement);
-          }
-        }
-        // Removing the base symbol of this select tree from unused variables:
-        Symbol baseSymbol = extractBaseSymbol(memberSelectTree);
-        if (baseSymbol != null) {
-          unusedElements.remove(baseSymbol);
+        } else if (currentExpressionStatement != null && unusedElements.containsKey(symbol)) {
+          usageSites.put(symbol, currentExpressionStatement);
         }
+        // Clear leftHandSideAssignment and descend down the tree to catch any variables in the
+        // receiver of this member select, which _are_ considered used.
+        boolean wasLeftHandAssignment = leftHandSideAssignment;
+        leftHandSideAssignment = false;
+        super.visitMemberSelect(memberSelectTree, null);
+        leftHandSideAssignment = wasLeftHandAssignment;
         return null;
       }
 
@@ -500,26 +497,6 @@ public Void visitMemberReference(MemberReferenceTree tree, Void unused) {
         return null;
       }
 
-      /**
-       * Extracts the base symbol of a member select expression. The base symbol is the first symbol
-       * appeared in the select expression. For instance, if the select expression is {@code a.b.c},
-       * the base symbol is {@code a}. If the base is not a variable but a method, this function
-       * returns null.
-       *
-       * @return the symbol for the base reference or null if the base is not a variable
-       */
-      @Nullable
-      private Symbol extractBaseSymbol(ExpressionTree access) {
-        switch (access.getKind()) {
-          case MEMBER_SELECT:
-            return extractBaseSymbol(((MemberSelectTree) access).getExpression());
-          case IDENTIFIER:
-            return getSymbol(access);
-          default:
-            return null;
-        }
-      }
-
       @Override
       public Void visitCompoundAssignment(CompoundAssignmentTree tree, Void unused) {
         if (isInExpressionStatementTree()) {

diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/UnusedTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/UnusedTest.java
@@ -740,4 +740,23 @@ public void exemptedFieldsByType() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void findingBaseSymbol() {
+    helper
+        .addSourceLines(
+            "Test.java",
+            "class Test {",
+            "  int a;",
+            "  void test() {",
+            "    Test o = new Test();",
+            "    ((Test) o).a = 1;",
+            "    (((o))).a = 1;",
+            "    Test p = new Test();",
+            "    id(p).a = 1;",
+            "  }",
+            "  Test id(Test t) { return t; }",
+            "}")
+        .doTest();
+  }
 }