RELNOTES: UnusedVariable: don't warn when nulling out variables. This…

… is often done to allow the garbage collector to collect an expensive object. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=236814402
google · Apr 12, 2019 · e263f35 · e263f35
1 parent f493911
commit e263f35
Show file tree

Hide file tree

Showing 2 changed files with 71 additions and 5 deletions.
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java
@@ -107,6 +107,7 @@
 import javax.lang.model.element.ElementKind;
 import javax.lang.model.element.Modifier;
 import javax.lang.model.element.Name;
+import javax.lang.model.type.NullType;
 
 /** Bugpattern to detect unused declarations. */
 @BugPattern(
@@ -228,7 +229,7 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s
       if (onlyCheckForReassignments.contains(unusedSymbol) && specs.size() <= 1) {
         continue;
       }
-      Tree unused = specs.iterator().next().variableTree().getLeaf();
+      Tree unused = specs.iterator().next().assignmentPath().getLeaf();
       VarSymbol symbol = (VarSymbol) unusedSymbol;
       ImmutableList<SuggestedFix> fixes;
       if (symbol.getKind() == ElementKind.PARAMETER
@@ -799,6 +800,11 @@ private void handleReassignment(AssignmentTree tree) {
       if (!declarationSites.containsKey(symbol)) {
         return;
       }
+      // Don't regard assigning `null` as a potentially unused assignment, as people do this for GC
+      // reasons.
+      if (getType(tree.getExpression()) instanceof NullType) {
+        return;
+      }
       hasBeenAssigned.add(symbol);
       TreePath assignmentSite = declarationSites.get(symbol);
       if (scopeDepth(assignmentSite) != Iterables.size(getCurrentPath().getParentPath())) {
@@ -932,8 +938,8 @@ abstract static class UnusedSpec {
     /** {@link Symbol} of the unsued element. */
     abstract Symbol symbol();
 
-    /** {@link VariableTree} for the original declaration site. */
-    abstract TreePath variableTree();
+    /** {@link VariableTree} or {@link AssignmentTree} for the original assignment site. */
+    abstract TreePath assignmentPath();
 
     /**
      * All the usage sites of this variable that we claim are unused (including the initial
@@ -949,12 +955,12 @@ abstract static class UnusedSpec {
 
     private static UnusedSpec of(
         Symbol symbol,
-        TreePath variableTree,
+        TreePath assignmentPath,
         Iterable<TreePath> treePaths,
         @Nullable AssignmentTree assignmentTree) {
       return new AutoValue_UnusedVariable_UnusedSpec(
           symbol,
-          variableTree,
+          assignmentPath,
           ImmutableList.copyOf(treePaths),
           Optional.ofNullable(assignmentTree));
     }

diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/UnusedVariableTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/UnusedVariableTest.java
@@ -1033,6 +1033,66 @@ public void unusedAssignment_messages() {
         .doTest();
   }
 
+  @Test
+  public void unusedAssignment_nulledOut_noWarning() {
+    helper
+        .addSourceLines(
+            "Test.java",
+            "package unusedvars;",
+            "public class Test {",
+            "  public void test() {",
+            "    Integer a = 1;",
+            "    a.hashCode();",
+            "    a = null;",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void unusedAssignment_nulledOut_thenAssignedAgain() {
+    refactoringHelper
+        .addInputLines(
+            "Test.java",
+            "package unusedvars;",
+            "public class Test {",
+            "  public void test() {",
+            "    Integer a = 1;",
+            "    a.hashCode();",
+            "    a = null;",
+            "    a = 2;",
+            "  }",
+            "}")
+        .addOutputLines(
+            "Test.java",
+            "package unusedvars;",
+            "public class Test {",
+            "  public void test() {",
+            "    Integer a = 1;",
+            "    a.hashCode();",
+            "    a = null;",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void unusedAssignment_initialAssignmentNull_givesWarning() {
+    helper
+        .addSourceLines(
+            "Test.java",
+            "package unusedvars;",
+            "public class Test {",
+            "  public void test() {",
+            "    // BUG: Diagnostic contains: assignment",
+            "    Integer a = null;",
+            "    a = 1;",
+            "    a.hashCode();",
+            "  }",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void unusedAssignmentAfterUse() {
     refactoringHelper