Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autofix StrictUnusedVariable #833

Merged
merged 7 commits into from
Sep 18, 2019
Merged

Autofix StrictUnusedVariable #833

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2c1776e
fix strict variable
Sep 15, 2019
1801b3d
Add generated changelog entries
Sep 15, 2019
0040bb3
Expand valid parameter names
Sep 17, 2019
a1433cd
Detect and automatically fix parameters that are suppressed but used
Sep 17, 2019
2ca4ec9
-PerrorProneApply runs StrictUnusedVariable
iamdanfox Sep 18, 2019
e1cb1f5
empty commit
Sep 18, 2019
c4a18c3
Merge remote-tracking branch 'origin/develop' into fo/fix-strict-vari…
Sep 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 78 additions & 44 deletions ...line-error-prone/src/main/java/com/palantir/baseline/errorprone/StrictUnusedVariable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@
severity = WARNING,
documentSuppression = false)
public final class StrictUnusedVariable extends BugChecker implements BugChecker.CompilationUnitTreeMatcher {
private static final String EXEMPT_PREFIX = "unused";
private static final ImmutableSet<String> EXEMPT_PREFIXES = ImmutableSet.of("unused", "_");

/**
* The set of annotation full names which exempt annotated element from being reported as unused.
Expand Down Expand Up @@ -203,8 +203,7 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s
Tree unused = specs.iterator().next().variableTree().getLeaf();
Symbol.VarSymbol symbol = (Symbol.VarSymbol) unusedSymbol;
ImmutableList<SuggestedFix> fixes;
if (symbol.getKind() == ElementKind.PARAMETER
&& !isEverUsed.contains(unusedSymbol)) {
if (symbol.getKind() == ElementKind.PARAMETER && !isEverUsed.contains(unusedSymbol)) {
fixes = buildUnusedParameterFixes(symbol, allUsageSites, state);
} else {
fixes = buildUnusedVarFixes(symbol, allUsageSites, state);
Expand Down Expand Up @@ -421,61 +420,96 @@ private static ImmutableList<SuggestedFix> buildUnusedVarFixes(
private static ImmutableList<SuggestedFix> buildUnusedParameterFixes(
Symbol varSymbol, List<TreePath> usagePaths, VisitorState state) {
Symbol.MethodSymbol methodSymbol = (Symbol.MethodSymbol) varSymbol.owner;
boolean isPrivateMethod = methodSymbol.getModifiers().contains(Modifier.PRIVATE);
int index = methodSymbol.params.indexOf(varSymbol);
SuggestedFix.Builder fix = SuggestedFix.builder();
for (TreePath path : usagePaths) {
fix.delete(path.getLeaf());
}
new TreePathScanner<Void, Void>() {
@Override
public Void visitMethodInvocation(MethodInvocationTree tree, Void unused) {
if (getSymbol(tree).equals(methodSymbol)) {
removeByIndex(tree.getArguments());
}
return super.visitMethodInvocation(tree, null);
}

@Override
public Void visitMethod(MethodTree tree, Void unused) {
if (getSymbol(tree).equals(methodSymbol)) {
removeByIndex(tree.getParameters());
// Remove parameter if the method is private since we can automatically fix all invocation sites
// Otherwise add `_` prefix to the variable name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this was already here, but I'm a bit worried about this behaviour.
If it's not going to auto-fix the call sites too (which is also dangerous), then it leaves the code in a state where it's harder for an IDE user to fix this (they'd have to hunt down the invocations).
On the other hand, if we don't autofix, they could just navigate to the method, and cmd+del the parameter, after which IntelliJ would autofix the callsites.
(I assume there's similar functionality in Eclipse though I haven't checked)

if (isPrivateMethod) {
new TreePathScanner<Void, Void>() {
@Override
public Void visitMethodInvocation(MethodInvocationTree tree, Void unused) {
if (getSymbol(tree).equals(methodSymbol)) {
removeByIndex(tree.getArguments());
}
return super.visitMethodInvocation(tree, null);
}
return super.visitMethod(tree, null);
}

private void removeByIndex(List<? extends Tree> trees) {
if (index >= trees.size()) {
// possible when removing a varargs parameter with no corresponding formal parameters
return;
@Override
public Void visitMethod(MethodTree tree, Void unused) {
if (getSymbol(tree).equals(methodSymbol)) {
removeByIndex(tree.getParameters());
}
return super.visitMethod(tree, null);
}
if (trees.size() == 1) {
Tree tree = getOnlyElement(trees);
if (((JCTree) tree).getStartPosition() == -1 || state.getEndPosition(tree) == -1) {

private void removeByIndex(List<? extends Tree> trees) {
if (index >= trees.size()) {
// possible when removing a varargs parameter with no corresponding formal parameters
return;
}
if (trees.size() == 1) {
Tree tree = getOnlyElement(trees);
if (((JCTree) tree).getStartPosition() == -1 || state.getEndPosition(tree) == -1) {
// TODO(b/118437729): handle bogus source positions in enum declarations
return;
}
fix.delete(tree);
return;
}
int startPos;
int endPos;
if (index >= 1) {
startPos = state.getEndPosition(trees.get(index - 1));
endPos = state.getEndPosition(trees.get(index));
} else {
startPos = ((JCTree) trees.get(index)).getStartPosition();
endPos = ((JCTree) trees.get(index + 1)).getStartPosition();
}
if (index == methodSymbol.params().size() - 1 && methodSymbol.isVarArgs()) {
endPos = state.getEndPosition(getLast(trees));
}
if (startPos == Position.NOPOS || endPos == Position.NOPOS) {
// TODO(b/118437729): handle bogus source positions in enum declarations
return;
}
fix.delete(tree);
return;
}
int startPos;
int endPos;
if (index >= 1) {
startPos = state.getEndPosition(trees.get(index - 1));
endPos = state.getEndPosition(trees.get(index));
} else {
startPos = ((JCTree) trees.get(index)).getStartPosition();
endPos = ((JCTree) trees.get(index + 1)).getStartPosition();
fix.replace(startPos, endPos, "");
}
if (index == methodSymbol.params().size() - 1 && methodSymbol.isVarArgs()) {
endPos = state.getEndPosition(getLast(trees));
}.scan(state.getPath().getCompilationUnit(), null);
} else {
new TreePathScanner<Void, Void>() {
@Override
public Void visitMethod(MethodTree methodTree, Void unused) {
if (getSymbol(methodTree).equals(methodSymbol)) {
renameByIndex(methodTree.getParameters());
}
return super.visitMethod(methodTree, null);
}
if (startPos == Position.NOPOS || endPos == Position.NOPOS) {
// TODO(b/118437729): handle bogus source positions in enum declarations
return;

private void renameByIndex(List<? extends VariableTree> trees) {
if (index >= trees.size()) {
// possible when removing a varargs parameter with no corresponding formal parameters
return;
}

VariableTree tree = trees.get(index);
int startPos = state.getEndPosition(tree.getType()) + 1;
int endPos = state.getEndPosition(trees.get(index));
if (index == methodSymbol.params().size() - 1 && methodSymbol.isVarArgs()) {
endPos = state.getEndPosition(getLast(trees));
}
if (startPos == Position.NOPOS || endPos == Position.NOPOS) {
// TODO(b/118437729): handle bogus source positions in enum declarations
return;
}
fix.replace(startPos, endPos, "_" + tree.getName());
}
fix.replace(startPos, endPos, "");
}
}.scan(state.getPath().getCompilationUnit(), null);
}.scan(state.getPath().getCompilationUnit(), null);
}
return ImmutableList.of(fix.build());
}

Expand Down Expand Up @@ -504,7 +538,7 @@ private static boolean exemptedByAnnotation(
}

private static boolean exemptedByName(Name name) {
return Ascii.toLowerCase(name.toString()).startsWith(EXEMPT_PREFIX);
return EXEMPT_PREFIXES.stream().anyMatch(prefix -> Ascii.toLowerCase(name.toString()).startsWith(prefix));
}

private class VariableFinder extends TreePathScanner<Void, Void> {
Expand Down
24 changes: 24 additions & 0 deletions ...-error-prone/src/test/java/com/palantir/baseline/errorprone/StrictUnusedVariableTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,21 @@

package com.palantir.baseline.errorprone;

import com.google.errorprone.BugCheckerRefactoringTestHelper;
import com.google.errorprone.CompilationTestHelper;
import org.junit.Before;
import org.junit.Test;

public class StrictUnusedVariableTest {

private CompilationTestHelper compilationHelper;
private BugCheckerRefactoringTestHelper refactoringTestHelper;

@Before
public void before() {
compilationHelper = CompilationTestHelper.newInstance(StrictUnusedVariable.class, getClass());
refactoringTestHelper = BugCheckerRefactoringTestHelper.newInstance(
new StrictUnusedVariable(), getClass());
}

@Test
Expand Down Expand Up @@ -83,4 +87,24 @@ public void handles_enums() {
" public static void publicMethod(String buggy) { }",
"}").doTest();
}

@Test
public void renames_unused_param() {
refactoringTestHelper
.addInputLines(
"Test.java",
"class Test {",
" private void privateMethod(String value) { }",
" public void publicMethod(String value, String value2) { }",
" public void varArgs(String value, String... value2) { }",
"}")
.addOutputLines(
"Test.java",
"class Test {",
" private void privateMethod() { }",
" public void publicMethod(String _value, String _value2) { }",
" public void varArgs(String _value, String... _value2) { }",
"}")
.doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
}
}
7 changes: 7 additions & 0 deletions changelog/@unreleased/pr-833.v2.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
type: improvement
improvement:
description: '`StrictUnusedVariable` now ignores variables prefixed with `_` and
the suggested fix will rename all unused parameters in public methods instead
of removing them'
links:
- https://github.com/palantir/gradle-baseline/pull/833