StrictUnusedVariable maintains side effects (#870)

When removing unused variables, `StrictUnusedVariable` will no longer delete method calls as these may have side effects
palantir · Sep 20, 2019 · 9b12f8a · 9b12f8a
1 parent 2a986ad
commit 9b12f8a
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 20 deletions.
diff --git a/...line-error-prone/src/main/java/com/palantir/baseline/errorprone/StrictUnusedVariable.java b/...line-error-prone/src/main/java/com/palantir/baseline/errorprone/StrictUnusedVariable.java
@@ -409,10 +409,7 @@ private static ImmutableList<SuggestedFix> buildUnusedVarFixes(
             return ImmutableList.of();
         }
         ElementKind varKind = varSymbol.getKind();
-        boolean encounteredSideEffects = false;
         SuggestedFix.Builder fix = SuggestedFix.builder().setShortDescription("remove unused variable");
-        SuggestedFix.Builder removeSideEffectsFix =
-                SuggestedFix.builder().setShortDescription("remove unused variable and any side effects");
         for (TreePath usagePath : usagePaths) {
             StatementTree statement = (StatementTree) usagePath.getLeaf();
             if (statement.getKind() == Tree.Kind.VARIABLE) {
@@ -422,37 +419,29 @@ private static ImmutableList<SuggestedFix> buildUnusedVarFixes(
                 VariableTree variableTree = (VariableTree) statement;
                 ExpressionTree initializer = variableTree.getInitializer();
                 if (hasSideEffect(initializer) && TOP_LEVEL_EXPRESSIONS.contains(initializer.getKind())) {
-                    encounteredSideEffects = true;
                     if (varKind == ElementKind.FIELD) {
                         String newContent =
                                 String.format(
                                         "%s{ %s; }",
                                         varSymbol.isStatic() ? "static " : "", state.getSourceForNode(initializer));
                         fix.merge(SuggestedFixes.replaceIncludingComments(usagePath, newContent, state));
-                        removeSideEffectsFix.replace(statement, "");
                     } else {
                         fix.replace(statement, String.format("%s;", state.getSourceForNode(initializer)));
-                        removeSideEffectsFix.replace(statement, "");
                     }
                 } else if (isEnhancedForLoopVar(usagePath)) {
                     String modifiers =
                             nullToEmpty(
                                     variableTree.getModifiers() == null
                                             ? null
                                             : state.getSourceForNode(variableTree.getModifiers()));
-                    String newContent =
-                            String.format(
-                                    "%s%s unused",
-                                    modifiers.isEmpty() ? "" : (modifiers + " "), variableTree.getType());
+                    String newContent = String.format(
+                            "%s%s unused", modifiers.isEmpty() ? "" : (modifiers + " "), variableTree.getType());
                     // The new content for the second fix should be identical to the content for the first
                     // fix in this case because we can't just remove the enhanced for loop variable.
                     fix.replace(variableTree, newContent);
-                    removeSideEffectsFix.replace(variableTree, newContent);
                 } else {
                     String replacement = needsBlock(usagePath) ? "{}" : "";
                     fix.merge(SuggestedFixes.replaceIncludingComments(usagePath, replacement, state));
-                    removeSideEffectsFix.merge(
-                            SuggestedFixes.replaceIncludingComments(usagePath, replacement, state));
                 }
                 continue;
             } else if (statement.getKind() == Tree.Kind.EXPRESSION_STATEMENT) {
@@ -468,26 +457,20 @@ private static ImmutableList<SuggestedFix> buildUnusedVarFixes(
                                         ((JCTree.JCAssignOp) tree).getExpression().getStartPosition(),
                                         "");
                         fix.merge(replacement);
-                        removeSideEffectsFix.merge(replacement);
                         continue;
                     }
                 } else if (tree instanceof AssignmentTree) {
                     if (hasSideEffect(((AssignmentTree) tree).getExpression())) {
-                        encounteredSideEffects = true;
                         fix.replace(
                                 tree.getStartPosition(), ((JCTree.JCAssign) tree).getExpression().getStartPosition(), "");
-                        removeSideEffectsFix.replace(statement, "");
                         continue;
                     }
                 }
             }
             String replacement = needsBlock(usagePath) ? "{}" : "";
             fix.replace(statement, replacement);
-            removeSideEffectsFix.replace(statement, replacement);
         }
-        return encounteredSideEffects
-                ? ImmutableList.of(removeSideEffectsFix.build(), fix.build())
-                : ImmutableList.of(fix.build());
+        return ImmutableList.of(fix.build());
     }
 
     private static ImmutableList<SuggestedFix> buildUnusedParameterFixes(

diff --git a/...-error-prone/src/test/java/com/palantir/baseline/errorprone/StrictUnusedVariableTest.java b/...-error-prone/src/test/java/com/palantir/baseline/errorprone/StrictUnusedVariableTest.java
@@ -145,6 +145,30 @@ public void fails_suppressed_but_used_variables() {
                 "}").doTest();
     }
 
+    @Test
+    public void side_effects_are_preserved() {
+        refactoringTestHelper
+                .addInputLines(
+                        "Test.java",
+                        "class Test {",
+                        "  private static int _field = 1;",
+                        "  public static void privateMethod() {",
+                        "    Object foo = someMethod();",
+                        "  }",
+                        "  private static Object someMethod() { return null; }",
+                        "}")
+                .addOutputLines(
+                        "Test.java",
+                        "class Test {",
+                        "  private static int _field = 1;",
+                        "  public static void privateMethod() {",
+                        "    someMethod();",
+                        "  }",
+                        "  private static Object someMethod() { return null; }",
+                        "}")
+                .doTest(TestMode.TEXT_MATCH);
+    }
+
     @Test
     public void fixes_suppressed_but_used_variables() {
         refactoringTestHelper

diff --git a/changelog/@unreleased/pr-870.v2.yml b/changelog/@unreleased/pr-870.v2.yml
@@ -0,0 +1,6 @@
+type: fix
+fix:
+  description: When removing unused variables, `StrictUnusedVariable` will preserve
+    side effects
+  links:
+  - https://github.com/palantir/gradle-baseline/pull/870