From 9a76d2423001b3e0f86231dd73019f84fcc73b6c Mon Sep 17 00:00:00 2001 From: forozco Date: Thu, 19 Sep 2019 18:04:56 -0400 Subject: [PATCH 1/3] StrictUnusedVariable maintains side effects --- .../errorprone/StrictUnusedVariable.java | 23 +++-------------- .../errorprone/StrictUnusedVariableTest.java | 25 +++++++++++++++++++ 2 files changed, 28 insertions(+), 20 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StrictUnusedVariable.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StrictUnusedVariable.java index 8dc7688ee..491c26346 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StrictUnusedVariable.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StrictUnusedVariable.java @@ -409,10 +409,7 @@ private static ImmutableList 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,17 +419,14 @@ private static ImmutableList 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 = @@ -440,19 +434,14 @@ private static ImmutableList buildUnusedVarFixes( 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 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 buildUnusedParameterFixes( diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StrictUnusedVariableTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StrictUnusedVariableTest.java index d73fee0cf..07386f9b8 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StrictUnusedVariableTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StrictUnusedVariableTest.java @@ -145,6 +145,31 @@ 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 From b5fa3110d3478ffd29c14423a71573372947867c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20S=C4=83nduleac?= Date: Fri, 20 Sep 2019 11:21:29 +0100 Subject: [PATCH 2/3] Fix broken test --- .../baseline/errorprone/StrictUnusedVariableTest.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StrictUnusedVariableTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StrictUnusedVariableTest.java index 07386f9b8..17769f6fb 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StrictUnusedVariableTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StrictUnusedVariableTest.java @@ -160,14 +160,13 @@ public void side_effects_are_preserved() { .addOutputLines( "Test.java", "class Test {", - " private static int field = 1;", + " private static int _field = 1;", " public static void privateMethod() {", " someMethod();", " }", " private static Object someMethod() { return null; }", - " }", - "}" - ).doTest(TestMode.TEXT_MATCH); + "}") + .doTest(TestMode.TEXT_MATCH); } @Test From 1ea49a136403272669a4dea35aa4c996b6b2a5e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20S=C4=83nduleac?= Date: Fri, 20 Sep 2019 10:21:29 +0000 Subject: [PATCH 3/3] Add generated changelog entries --- changelog/@unreleased/pr-870.v2.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/@unreleased/pr-870.v2.yml diff --git a/changelog/@unreleased/pr-870.v2.yml b/changelog/@unreleased/pr-870.v2.yml new file mode 100644 index 000000000..d1d7c5e8a --- /dev/null +++ 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