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 all 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
223 changes: 176 additions & 47 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 @@ -35,6 +35,7 @@

import com.google.auto.service.AutoService;
import com.google.common.base.Ascii;
import com.google.common.base.CaseFormat;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
Expand Down Expand Up @@ -114,7 +115,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 @@ -155,6 +156,8 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s
VariableFinder variableFinder = new VariableFinder(state);
variableFinder.scan(state.getPath(), null);

checkUsedVariables(state, variableFinder);

// Map of symbols to variable declarations. Initially this is a map of all of the local variable
// and fields. As we go we remove those variables which are used.
Map<Symbol, TreePath> unusedElements = variableFinder.unusedElements;
Expand Down Expand Up @@ -205,8 +208,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 All @@ -233,6 +235,73 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s
return Description.NO_MATCH;
}

private void checkUsedVariables(VisitorState state, VariableFinder variableFinder) {
VariableUsage variableUsage = new VariableUsage();
variableUsage.scan(state.getPath(), null);
variableFinder.exemptedVariables.entrySet().forEach(entry -> {
List<TreePath> usageSites = variableUsage.usageSites.get(entry.getKey());
if (usageSites.size() <= 1) {
return;
}
state.reportMatch(
buildDescription(entry.getValue())
.setMessage(String.format(
"The %s '%s' is read but has 'StrictUnusedVariable' " +
"suppressed because of its name.",
describeVariable((Symbol.VarSymbol) entry.getKey()),
entry.getKey().name))
.addFix(constructUsedVariableSuggestedFix(usageSites, state))
.build());

});
}

private static SuggestedFix constructUsedVariableSuggestedFix(List<TreePath> usagePaths, VisitorState state) {
SuggestedFix.Builder fix = SuggestedFix.builder();
for (TreePath usagePath : usagePaths) {
if (usagePath.getLeaf() instanceof VariableTree) {
VariableTree variableTree = (VariableTree) usagePath.getLeaf();
int startPos = state.getEndPosition(variableTree.getType()) + 1;
int endPos = state.getEndPosition(variableTree);

// Ignore the initializer if there is one
if (variableTree.getInitializer() != null) {
endPos = startPos + variableTree.getName().toString().length();
}
if (startPos == Position.NOPOS || endPos == Position.NOPOS) {
// TODO(b/118437729): handle bogus source positions in enum declarations
continue;
}

renameVariable(startPos, endPos, variableTree.getName().toString(), fix);
} else if (usagePath.getLeaf() instanceof IdentifierTree) {
JCTree.JCIdent identifierTree = (JCTree.JCIdent) usagePath.getLeaf();
int startPos = identifierTree.getStartPosition();
int endPos = state.getEndPosition(identifierTree);
if (startPos == Position.NOPOS || endPos == Position.NOPOS) {
// TODO(b/118437729): handle bogus source positions in enum declarations
continue;
}

renameVariable(startPos, endPos, identifierTree.getName().toString(), fix);
}
}
return fix.build();
}

private static void renameVariable(int startPos, int endPos, String name, SuggestedFix.Builder fix) {
EXEMPT_PREFIXES.stream()
.filter(name::startsWith)
.findFirst()
.ifPresent(prefix -> fix.replace(
startPos,
endPos,
prefix.length() == name.length()
// Fall back to a generic variable name if the prefix is the entire variable name
? "value"
: CaseFormat.UPPER_CAMEL.to(CaseFormat.LOWER_CAMEL, name.substring(prefix.length()))));
}

private static SuggestedFix makeAssignmentDeclaration(
Symbol unusedSymbol,
Collection<UnusedSpec> specs,
Expand Down Expand Up @@ -423,61 +492,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;
fix.replace(startPos, endPos, "");
}
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));
}.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 @@ -506,7 +610,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 All @@ -516,6 +620,8 @@ private class VariableFinder extends TreePathScanner<Void, Void> {

private final ListMultimap<Symbol, TreePath> usageSites = ArrayListMultimap.create();

private final Map<Symbol, VariableTree> exemptedVariables = new HashMap<>();

private final VisitorState state;

private VariableFinder(VisitorState state) {
Expand All @@ -525,16 +631,17 @@ private VariableFinder(VisitorState state) {
@Override
@SuppressWarnings("SwitchStatementDefaultCase")
public Void visitVariable(VariableTree variableTree, Void unused) {
if (exemptedByName(variableTree.getName())) {
return null;
}
if (isSuppressed(variableTree)) {
return null;
}
Symbol.VarSymbol symbol = getSymbol(variableTree);
if (symbol == null) {
return null;
}
if (exemptedByName(variableTree.getName())) {
exemptedVariables.put(symbol, variableTree);
return null;
}
if (symbol.getKind() == ElementKind.FIELD
&& exemptedFieldBySuperType(getType(variableTree), state)) {
return null;
Expand Down Expand Up @@ -884,6 +991,28 @@ public Void visitMethodInvocation(MethodInvocationTree tree, Void unused) {
}
}

static class VariableUsage extends TreePathScanner<Void, Void> {
public final ListMultimap<Symbol, TreePath> usageSites = ArrayListMultimap.create();

@Override
public Void visitVariable(VariableTree tree, Void unused) {
usageSites.put(getSymbol(tree), getCurrentPath());
return super.visitVariable(tree, null);
}

@Override
public Void visitIdentifier(IdentifierTree tree, Void unused) {
usageSites.put(getSymbol(tree), getCurrentPath());
return super.visitIdentifier(tree, null);
}

@Override
public Void visitMemberSelect(MemberSelectTree memberSelectTree, Void unused) {
usageSites.put(getSymbol(memberSelectTree), getCurrentPath());
return super.visitMemberSelect(memberSelectTree, null);
}
}

interface UnusedSpec {
/** {@link Symbol} of the unsued element. */
Symbol symbol();
Expand Down
Loading