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 3a9467869..961e191ed 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 @@ -232,15 +232,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 fixes; if (symbol.getKind() == ElementKind.PARAMETER && !isEverUsed.contains(unusedSymbol)) { Symbol.MethodSymbol methodSymbol = (Symbol.MethodSymbol) symbol.owner; int index; @@ -253,28 +251,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); } - state.reportMatch(buildDescription(unused) - .setMessage(String.format( - "%s %s '%s' is never read. Intentional occurrences are acknowledged by renaming " - + "the unused variable with a leading underscore. '_%s', for example.", - unused instanceof VariableTree ? "The" : "The assignment to this", - describeVariable(symbol), - symbol.name, - symbol.name)) - .addAllFixes(fixes.stream() - .map(f -> SuggestedFix.builder() - .merge(makeFirstAssignmentDeclaration) - .merge(f) - .build()) - .collect(toImmutableList())) - .build()); } return Description.NO_MATCH; } @@ -359,32 +343,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 specs, - ImmutableList allUsageSites, - VisitorState state) { - if (unusedSymbol.getKind() != ElementKind.LOCAL_VARIABLE) { - return SuggestedFix.builder().build(); - } - Optional removedVariableTree = allUsageSites.stream() - .filter(tp -> tp.getLeaf() instanceof VariableTree) - .findFirst() - .map(tp -> (VariableTree) tp.getLeaf()); - Optional 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: @@ -454,12 +412,47 @@ public Boolean visitEnhancedForLoop(EnhancedForLoopTree tree, Void unused) { return firstNonNull(path.getParentPath().getLeaf().accept(new Visitor(), null), false); } - private static ImmutableList buildUnusedVarFixes( - Symbol varSymbol, List usagePaths, VisitorState state) { + private void reportUnusedAssignment( + Symbol.VarSymbol unusedSymbol, + Tree unused, + Collection specs, + List allUsageSites, + VisitorState state) { + if (unusedSymbol.getKind() != ElementKind.LOCAL_VARIABLE) { + return; + } + Optional removedVariableTree = allUsageSites.stream() + .filter(tp -> tp.getLeaf() instanceof VariableTree) + .findFirst() + .map(tp -> (VariableTree) tp.getLeaf()); + Optional 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 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"); @@ -523,12 +516,17 @@ private static ImmutableList 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 buildUnusedLambdaParameterFix( - Symbol.VarSymbol _symbol, Collection values, VisitorState state) { - SuggestedFix.Builder fix = SuggestedFix.builder(); + private void reportUnusedLambdaParameter( + Symbol.VarSymbol symbol, Tree unused, Collection values, VisitorState state) { + SuggestedFix.Builder fix = + SuggestedFix.builder().setShortDescription("acknowledge intentionally unused lambda parameter"); for (UnusedSpec unusedSpec : values) { Tree leaf = unusedSpec.variableTree().getLeaf(); @@ -546,11 +544,21 @@ private static ImmutableList 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 buildUnusedParameterFixes( - Symbol varSymbol, Symbol.MethodSymbol methodSymbol, List usagePaths, VisitorState state) { + private void reportUnusedParameter( + Symbol.VarSymbol varSymbol, + Symbol.MethodSymbol methodSymbol, + Tree unused, + List 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); @@ -562,6 +570,7 @@ private static ImmutableList buildUnusedParameterFixes( // Remove parameter if the method is private since we can automatically fix all invocation sites // Otherwise add `_` prefix to the variable name if (isPrivateMethod) { + fix.setShortDescription("remove unused parameter"); new TreePathScanner() { @Override public Void visitMethodInvocation(MethodInvocationTree tree, Void unused) { @@ -612,7 +621,13 @@ private void removeByIndex(List 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() { @Override public Void visitMethod(MethodTree methodTree, Void unused) { @@ -653,8 +668,15 @@ private void renameByIndex(List 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) { 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 49393f91f..b6755ea85 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 @@ -55,9 +55,11 @@ public void handles_abstract_classes() { "import java.util.Optional;", "abstract class Test {", " abstract void method(String param);", - " // BUG: Diagnostic contains: Unused", + " // BUG: Diagnostic contains: The public method parameter 'param' is never read. Acknowledge" + + " an intentional occurrence by renaming the unused variable with a leading underscore," + + " or remove the parameter.\t", " void defaultMethod(String param) { }", - " // BUG: Diagnostic contains: Unused", + " // BUG: Diagnostic contains: The parameter 'param' is never read.\t", " private void privateMethod(String param) { }", "}") .doTest(); @@ -70,11 +72,15 @@ public void handles_classes() { "Test.java", "import java.util.Optional;", "class Test {", - " // BUG: Diagnostic contains: '_foo', for example", + " // BUG: Diagnostic contains: The public method parameter 'foo' is never read. Acknowledge" + + " an intentional occurrence by renaming the unused variable with a leading underscore," + + " or remove the parameter.\t", " Test(String foo) { }", - " // BUG: Diagnostic contains: '_buggy', for example", + " // BUG: Diagnostic contains: The parameter 'buggy' is never read.\t", " private static void privateMethod(String buggy) { }", - " // BUG: Diagnostic contains: '_buggy', for example", + " // BUG: Diagnostic contains: The public method parameter 'buggy' is never read. Acknowledge" + + " an intentional occurrence by renaming the unused variable with a leading underscore," + + " or remove the parameter.\t", " public static void publicMethod(String buggy) { }", "}") .doTest(); @@ -88,9 +94,11 @@ public void handles_enums() { "import java.util.Optional;", "enum Test {", " INSTANCE;", - " // BUG: Diagnostic contains: Unused", + " // BUG: Diagnostic contains: The parameter 'buggy' is never read.\t", " private static void privateMethod(String buggy) { }", - " // BUG: Diagnostic contains: Unused", + " // BUG: Diagnostic contains: The public method parameter 'buggy' is never read. Acknowledge" + + " an intentional occurrence by renaming the unused variable with a leading underscore," + + " or remove the parameter.\t", " public static void publicMethod(String buggy) { }", "}") .doTest(); @@ -105,9 +113,12 @@ void handles_lambdas() { "import java.util.Optional;", "class Test {", " private static BiFunction doStuff() {", - " // BUG: Diagnostic contains: Unused", + " // BUG: Diagnostic contains: Acknowledge an intentional occurrence by renaming the unused" + + " parameter with a leading underscore, or remove the parameter.\t", " BiFunction first = (String value1, String value2) -> 1;", - " // BUG: Diagnostic contains: Unused", + " // BUG: Diagnostic contains: The lambda parameter 'value3' is never read. Acknowledge an" + + " intentional occurrence by renaming the unused parameter with a leading underscore, or" + + " remove the parameter.\t", " return first.andThen(value3 -> 2);", " }", "}") @@ -122,9 +133,12 @@ void handles_lambdas_in_static_init() { "import java.util.function.BiFunction;", "class Test {", " static {", - " // BUG: Diagnostic contains: Unused", + " // BUG: Diagnostic contains: Acknowledge an intentional occurrence by renaming the unused" + + " parameter with a leading underscore, or remove the parameter.\t", " BiFunction first = (String value1, String value2) -> 1;", - " // BUG: Diagnostic contains: Unused", + " // BUG: Diagnostic contains: The lambda parameter 'value3' is never read. Acknowledge an" + + " intentional occurrence by renaming the unused parameter with a leading underscore, or" + + " remove the parameter.\t", " first.andThen(value3 -> 2);", " }", "}") @@ -232,25 +246,77 @@ void renames_used_lambda_params() { } @Test - public void fails_suppressed_but_used_variables() { + public void handles_unused_variables() { compilationHelper .addSourceLines( "Test.java", "class Test {", - " // BUG: Diagnostic contains: Unused", - " private static final String _field = \"\";", - " // BUG: Diagnostic contains: Unused", - " public static void privateMethod(String _value) {", - " System.out.println(_value);", - " // BUG: Diagnostic contains: Unused", - " String _bar = \"bar\";", - " System.out.println(_bar);", - " System.out.println(_field);", + " // BUG: Diagnostic contains: The field 'field' is never read.\t", + " private static final String field = \"\";", + " // BUG: Diagnostic contains: The parameter 'value' is never read.\t", + " private void privateMethod(String value) {", + " // BUG: Diagnostic contains: The local variable 'bar' is never read.\t", + " String bar = \"bar\";", + " }", + "}") + .doTest(); + } + + @Test + public void fixes_unused_variables() { + refactoringTestHelper + .addInputLines( + "Test.java", + "class Test {", + " private static final String field = \"\";", + " private void privateMethod(String value) {", + " String bar = \"bar\";", " }", "}") + .addOutputLines("Test.java", "class Test {", " private void privateMethod() {", " }", "}") .doTest(); } + @Test + public void handles_unused_local_variable_assignment() { + compilationHelper + .addSourceLines( + "Test.java", + "class Test {", + " private void privateMethod() {", + " // BUG: Diagnostic contains: The assignment to the local variable 'bar' is never read.\t", + " String bar = \"bar\";", + " bar = \"foo\";", + " System.out.println(bar);", + " }", + "}") + .doTest(); + } + + @Test + public void fixes_unused_local_variable_assignment() { + refactoringTestHelper + .addInputLines( + "Test.java", + "class Test {", + " private void privateMethod() {", + " String bar = \"bar\";", + " bar = \"foo\";", + " System.out.println(bar);", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " private void privateMethod() {", + "", + " String bar = \"foo\";", + " System.out.println(bar);", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } + @Test public void side_effects_are_preserved() { refactoringTestHelper @@ -275,6 +341,29 @@ public void side_effects_are_preserved() { .doTest(TestMode.TEXT_MATCH); } + @Test + public void fails_suppressed_but_used_variables() { + compilationHelper + .addSourceLines( + "Test.java", + "class Test {", + " // BUG: Diagnostic contains: The field '_field' is read but has 'StrictUnusedVariable'" + + " suppressed because of its name.\t", + " private static final String _field = \"\";", + " // BUG: Diagnostic contains: The parameter '_value' is read but has 'StrictUnusedVariable'" + + " suppressed because of its name.\t", + " public static void privateMethod(String _value) {", + " System.out.println(_value);", + " // BUG: Diagnostic contains: The local variable '_bar' is read but has" + + " 'StrictUnusedVariable' suppressed because of its name.\t", + " String _bar = \"bar\";", + " System.out.println(_bar);", + " System.out.println(_field);", + " }", + "}") + .doTest(); + } + @Test public void fixes_suppressed_but_used_variables() { refactoringTestHelper @@ -390,7 +479,7 @@ public void allows_unused_loggers() { "class Test {", " private static final Logger slf4j = LoggerFactory.getLogger(Test.class);", " private static final SafeLogger logsafe = SafeLoggerFactory.get(Test.class);", - " // BUG: Diagnostic contains: Unused", + " // BUG: Diagnostic contains: The field 'str' is never read.\t", " private static final String str = \"str\";", "}") .doTest(); diff --git a/changelog/@unreleased/pr-2835.v2.yml b/changelog/@unreleased/pr-2835.v2.yml new file mode 100644 index 000000000..d3793fea4 --- /dev/null +++ b/changelog/@unreleased/pr-2835.v2.yml @@ -0,0 +1,6 @@ +type: improvement +improvement: + description: Provide more specific error message for `StrictUsedVariable` check + based on unused variable type + links: + - https://github.com/palantir/gradle-baseline/pull/2835