Skip to content

Commit

Permalink
Use different error messages based on context
Browse files Browse the repository at this point in the history
  • Loading branch information
@IlanaRadinsky
IlanaRadinsky committed Aug 13, 2024
1 parent 01932a8 commit 93fab68
Show file tree
Hide file tree
Showing 2 changed files with 153 additions and 75 deletions.
135 changes: 78 additions & 57 deletions ...line-error-prone/src/main/java/com/palantir/baseline/errorprone/StrictUnusedVariable.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
import static com.sun.source.tree.Tree.Kind.POSTFIX_INCREMENT;
import static com.sun.source.tree.Tree.Kind.PREFIX_DECREMENT;
import static com.sun.source.tree.Tree.Kind.PREFIX_INCREMENT;
import static com.sun.tools.javac.tree.JCTree.*;

import com.google.auto.service.AutoService;
import com.google.common.base.Ascii;
Expand Down Expand Up @@ -232,15 +233,13 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s
if (!unusedElements.containsKey(unusedSymbol)) {
isEverUsed.add(unusedSymbol);
}
SuggestedFix makeFirstAssignmentDeclaration =
makeAssignmentDeclaration(unusedSymbol, specs, allUsageSites, state);

// Don't complain if this is a public method and we only overwrote it once.
if (onlyCheckForReassignments.contains(unusedSymbol) && specs.size() <= 1) {
continue;
}
Tree unused = specs.iterator().next().variableTree().getLeaf();
Symbol.VarSymbol symbol = (Symbol.VarSymbol) unusedSymbol;
ImmutableList<SuggestedFix> fixes;
if (symbol.getKind() == ElementKind.PARAMETER && !isEverUsed.contains(unusedSymbol)) {
Symbol.MethodSymbol methodSymbol = (Symbol.MethodSymbol) symbol.owner;
int index;
Expand All @@ -253,27 +252,14 @@ public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState s
// If we can not find the parameter in the owning method, then it must be a parameter to a lambda
// defined within the method
if (index == -1) {
fixes = buildUnusedLambdaParameterFix(symbol, entry.getValue(), state);
reportUnusedLambdaParameter(symbol, unused, specs, state);
} else {
fixes = buildUnusedParameterFixes(symbol, methodSymbol, allUsageSites, state);
reportUnusedParameter(symbol, methodSymbol, unused, allUsageSites, state);
}
} else {
fixes = buildUnusedVarFixes(symbol, allUsageSites, state);
reportUnusedAssignment(symbol, unused, specs, allUsageSites, state);
reportUnusedVar(symbol, unused, allUsageSites, state);
}
String assignmentDescriptor = unused instanceof VariableTree ? "" : "assignment to this ";
String descriptor = assignmentDescriptor + describeVariable(symbol);
state.reportMatch(buildDescription(unused)
.setMessage(String.format(
"The %s '%s' is never read. Remove the %s or acknowledge an intentional occurrence "
+ "by renaming the unused variable with a leading underscore.",
descriptor, symbol.name, descriptor))
.addAllFixes(fixes.stream()
.map(f -> SuggestedFix.builder()
.merge(makeFirstAssignmentDeclaration)
.merge(f)
.build())
.collect(toImmutableList()))
.build());
}
return Description.NO_MATCH;
}
Expand Down Expand Up @@ -358,32 +344,6 @@ private static void renameVariable(Tree node, String name, SuggestedFix.Builder
stripUnusedPrefix(name).ifPresent(newName -> fix.replace(node, newName));
}

private static SuggestedFix makeAssignmentDeclaration(
Symbol unusedSymbol,
Collection<UnusedSpec> specs,
ImmutableList<TreePath> allUsageSites,
VisitorState state) {
if (unusedSymbol.getKind() != ElementKind.LOCAL_VARIABLE) {
return SuggestedFix.builder().build();
}
Optional<VariableTree> removedVariableTree = allUsageSites.stream()
.filter(tp -> tp.getLeaf() instanceof VariableTree)
.findFirst()
.map(tp -> (VariableTree) tp.getLeaf());
Optional<AssignmentTree> reassignment = specs.stream()
.map(UnusedSpec::terminatingAssignment)
.filter(Optional::isPresent)
.map(Optional::get)
.filter(a -> allUsageSites.stream().noneMatch(tp -> tp.getLeaf().equals(a)))
.findFirst();
if (!removedVariableTree.isPresent() || !reassignment.isPresent()) {
return SuggestedFix.builder().build();
}
return SuggestedFix.prefixWith(
reassignment.get(),
state.getSourceForNode(removedVariableTree.get().getType()) + " ");
}

private static String describeVariable(Symbol.VarSymbol symbol) {
switch (symbol.getKind()) {
case FIELD:
Expand Down Expand Up @@ -453,12 +413,47 @@ public Boolean visitEnhancedForLoop(EnhancedForLoopTree tree, Void unused) {
return firstNonNull(path.getParentPath().getLeaf().accept(new Visitor(), null), false);
}

private static ImmutableList<SuggestedFix> buildUnusedVarFixes(
Symbol varSymbol, List<TreePath> usagePaths, VisitorState state) {
private void reportUnusedAssignment(
Symbol.VarSymbol unusedSymbol,
Tree unused,
Collection<UnusedSpec> specs,
List<TreePath> allUsageSites,
VisitorState state) {
if (unusedSymbol.getKind() != ElementKind.LOCAL_VARIABLE) {
return;
}
Optional<VariableTree> removedVariableTree = allUsageSites.stream()
.filter(tp -> tp.getLeaf() instanceof VariableTree)
.findFirst()
.map(tp -> (VariableTree) tp.getLeaf());
Optional<AssignmentTree> reassignment = specs.stream()
.map(UnusedSpec::terminatingAssignment)
.filter(Optional::isPresent)
.map(Optional::get)
.filter(a -> allUsageSites.stream().noneMatch(tp -> tp.getLeaf().equals(a)))
.findFirst();
if (!removedVariableTree.isPresent() || !reassignment.isPresent()) {
return;
}

SuggestedFix fix = SuggestedFix.prefixWith(
reassignment.get(),
state.getSourceForNode(removedVariableTree.get().getType()) + " ");

state.reportMatch(buildDescription(unused)
.setMessage(String.format(
"The assignment to the %s '%s' is never read.",
describeVariable(unusedSymbol), unusedSymbol.name))
.addFix(fix)
.build());
}

private void reportUnusedVar(
Symbol.VarSymbol varSymbol, Tree unused, List<TreePath> usagePaths, VisitorState state) {
// Don't suggest a fix for fields annotated @Inject: we can warn on them, but they *could* be
// used outside the class.
if (ASTHelpers.hasDirectAnnotationWithSimpleName(varSymbol, "Inject")) {
return ImmutableList.of();
return;
}
ElementKind varKind = varSymbol.getKind();
SuggestedFix.Builder fix = SuggestedFix.builder().setShortDescription("remove unused variable");
Expand Down Expand Up @@ -522,13 +517,17 @@ private static ImmutableList<SuggestedFix> buildUnusedVarFixes(
String replacement = needsBlock(usagePath) ? "{}" : "";
fix.replace(statement, replacement);
}
return ImmutableList.of(fix.build());

state.reportMatch(buildDescription(unused)
.setMessage(String.format("The %s '%s' is never read.", describeVariable(varSymbol), varSymbol.name))
.addFix(fix.build())
.build());
}

private static ImmutableList<SuggestedFix> buildUnusedLambdaParameterFix(
Symbol.VarSymbol _symbol, Collection<UnusedSpec> values, VisitorState state) {
private void reportUnusedLambdaParameter(
Symbol.VarSymbol symbol, Tree unused, Collection<UnusedSpec> values, VisitorState state) {
SuggestedFix.Builder fix =
SuggestedFix.builder().setShortDescription("acknowledge intentionally unused variable");
SuggestedFix.builder().setShortDescription("acknowledge intentionally unused lambda parameter");

for (UnusedSpec unusedSpec : values) {
Tree leaf = unusedSpec.variableTree().getLeaf();
Expand All @@ -546,11 +545,21 @@ private static ImmutableList<SuggestedFix> buildUnusedLambdaParameterFix(
}
}

return ImmutableList.of(fix.build());
state.reportMatch(buildDescription(unused)
.setMessage(String.format(
"The lambda parameter '%s' is never read. Acknowledge an intentional occurrence by renaming"
+ " the unused parameter with a leading underscore, or remove the parameter.",
symbol.name))
.addFix(fix.build())
.build());
}

private static ImmutableList<SuggestedFix> buildUnusedParameterFixes(
Symbol varSymbol, Symbol.MethodSymbol methodSymbol, List<TreePath> usagePaths, VisitorState state) {
private void reportUnusedParameter(
Symbol.VarSymbol varSymbol,
Symbol.MethodSymbol methodSymbol,
Tree unused,
List<TreePath> usagePaths,
VisitorState state) {
boolean isPrivateMethod = methodSymbol.getModifiers().contains(Modifier.PRIVATE);
int index = methodSymbol.params.indexOf(varSymbol);
Preconditions.checkState(index != -1, "symbol %s must be a parameter to the owning method", varSymbol);
Expand Down Expand Up @@ -613,6 +622,11 @@ private void removeByIndex(List<? extends Tree> trees) {
fix.replace(startPos, endPos, "");
}
}.scan(state.getPath().getCompilationUnit(), null);

state.reportMatch(buildDescription(unused)
.setMessage(String.format("The parameter '%s' is never read.", varSymbol.name))
.addFix(fix.build())
.build());
} else {
fix.setShortDescription("acknowledge intentionally unused parameter");
new TreePathScanner<Void, Void>() {
Expand Down Expand Up @@ -655,8 +669,15 @@ private void renameByIndex(List<? extends VariableTree> trees) {
}
}
}.scan(state.getPath().getCompilationUnit(), null);

state.reportMatch(buildDescription(unused)
.setMessage(String.format(
"The public method parameter '%s' is never read. Acknowledge an intentional occurrence by"
+ " renaming the unused variable with a leading underscore, or remove the parameter.",
varSymbol.name))
.addFix(fix.build())
.build());
}
return ImmutableList.of(fix.build());
}

private static boolean isEnhancedForLoopVar(TreePath variablePath) {
Expand Down
Loading

0 comments on commit 93fab68

Please sign in to comment.