From 4be12bcfaa3730dde6447dd46d78056cc38e0548 Mon Sep 17 00:00:00 2001 From: ghm Date: Mon, 4 Dec 2023 07:32:50 -0800 Subject: [PATCH] Check specific functional interface types (which would otherwise be unchecked) for parameter usage. PiperOrigin-RevId: 587714550 --- .../bugpatterns/UnusedVariable.java | 46 ++++++++++-- .../bugpatterns/UnusedVariableTest.java | 73 +++++++++++-------- 2 files changed, 84 insertions(+), 35 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java index 4f18642d2e1..4622c14cae5 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java @@ -30,6 +30,7 @@ import static com.google.errorprone.util.ASTHelpers.getType; import static com.google.errorprone.util.ASTHelpers.hasAnnotation; import static com.google.errorprone.util.ASTHelpers.hasExplicitSource; +import static com.google.errorprone.util.ASTHelpers.isAbstract; import static com.google.errorprone.util.ASTHelpers.isStatic; import static com.google.errorprone.util.ASTHelpers.isSubtype; import static com.google.errorprone.util.ASTHelpers.shouldKeep; @@ -95,6 +96,7 @@ import com.sun.source.util.TreePathScanner; import com.sun.source.util.TreeScanner; import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symbol.ClassSymbol; import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.code.Symbol.TypeSymbol; import com.sun.tools.javac.code.Symbol.VarSymbol; @@ -176,6 +178,9 @@ public final class UnusedVariable extends BugChecker implements CompilationUnitT // TAG fields are used by convention in Android apps. "TAG"); + private static final ImmutableSet FUNCTIONAL_INTERFACE_TYPES_TO_CHECK = + ImmutableSet.of("java.util.Comparator"); + private final boolean reportInjectedFields; @Inject @@ -492,6 +497,12 @@ private static ImmutableList buildUnusedVarFixes( private static ImmutableList buildUnusedParameterFixes( Symbol varSymbol, List usagePaths, VisitorState state) { + if (!(varSymbol.owner instanceof MethodSymbol) + || !((MethodSymbol) varSymbol.owner).params().contains(varSymbol) + || !canBeRemoved(varSymbol.owner, state)) { + // We're presumably in a lambda. Don't try to generate a fix. + return ImmutableList.of(); + } MethodSymbol methodSymbol = (MethodSymbol) varSymbol.owner; int index = methodSymbol.params.indexOf(varSymbol); RangeSet deletions = TreeRangeSet.create(); @@ -608,6 +619,15 @@ public Void visitVariable(VariableTree variableTree, Void unused) { return null; } VarSymbol symbol = getSymbol(variableTree); + var parent = getCurrentPath().getParentPath().getLeaf(); + if (parent instanceof LambdaExpressionTree) { + if (FUNCTIONAL_INTERFACE_TYPES_TO_CHECK.stream() + .anyMatch(t -> isSubtype(getType(parent), state.getTypeFromString(t), state))) { + unusedElements.put(symbol, getCurrentPath()); + usageSites.put(symbol, getCurrentPath()); + } + return null; + } if (symbol.getKind() == ElementKind.FIELD && symbol.getSimpleName().contentEquals("CREATOR") && isSubtype(symbol.type, PARCELABLE_CREATOR.get(state), state)) { @@ -700,9 +720,29 @@ private boolean isParameterSubjectToAnalysis(Symbol sym) { return true; } + if (enclosingMethod.owner instanceof ClassSymbol + && !isAbstract((MethodSymbol) enclosingMethod) + && FUNCTIONAL_INTERFACE_TYPES_TO_CHECK.stream() + .map(state::getTypeFromString) + .anyMatch( + t -> + isSubtype(enclosingMethod.owner.type, t, state) + && isFunctionalInterfaceMethod(t, enclosingMethod, state))) { + return true; + } + return canBeRemoved(enclosingMethod, state); } + private boolean isFunctionalInterfaceMethod( + Type functionalInterfaceType, Symbol method, VisitorState state) { + var functionalInterfaceMethod = + state.getTypes().findDescriptorSymbol(functionalInterfaceType.asElement()); + return method.getSimpleName().contentEquals(functionalInterfaceMethod.getSimpleName()) + && method.overrides( + functionalInterfaceMethod, method.owner.type.tsym, state.getTypes(), true); + } + @Override public Void visitTry(TryTree node, Void unused) { // Skip resources, as while these may not be referenced, they are used. @@ -724,12 +764,6 @@ public Void visitClass(ClassTree tree, Void unused) { return super.visitClass(tree, null); } - @Override - public Void visitLambdaExpression(LambdaExpressionTree node, Void unused) { - // skip lambda parameters - return scan(node.getBody(), null); - } - @Override public Void visitMethod(MethodTree tree, Void unused) { if (SERIALIZATION_METHODS.matches(tree, state)) { diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/UnusedVariableTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/UnusedVariableTest.java index d1243c5844b..76ccd5999b8 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/UnusedVariableTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/UnusedVariableTest.java @@ -611,35 +611,6 @@ public void unusedWithComment_interspersedComments() { .doTest(); } - @Test - public void usedInLambda() { - helper - .addSourceLines( - "UsedInLambda.java", - "package unusedvars;", - "import java.util.Arrays;", - "import java.util.List;", - "import java.util.function.Function;", - "import java.util.stream.Collectors;", - "/** Method parameters used in lambdas and anonymous classes */", - "public class UsedInLambda {", - " private Function usedInLambda() {", - " return x -> 1;", - " }", - " private String print(Object o) {", - " return o.toString();", - " }", - " public List print(List os) {", - " return os.stream().map(this::print).collect(Collectors.toList());", - " }", - " public static void main(String[] args) {", - " System.err.println(new UsedInLambda().usedInLambda());", - " System.err.println(new UsedInLambda().print(Arrays.asList(1, 2, 3)));", - " }", - "}") - .doTest(); - } - @Test public void utf8Handling() { helper @@ -1618,4 +1589,48 @@ public void nestedParameterRemoval() { "}") .doTest(); } + + @Test + public void unusedFunctionalInterfaceParameter() { + helper + .addSourceLines( + "Test.java", + "import java.util.Collections;", + "import java.util.Comparator;", + "import java.util.List;", + "class Test {", + " public void test(List xs) {", + " // BUG: Diagnostic contains: 'b' is never read", + " Collections.sort(xs, (a, b) -> a > a ? 1 : 0);", + " Collections.sort(xs, new Comparator() {", + " // BUG: Diagnostic contains: 'b' is never read", + " @Override public int compare(Integer a, Integer b) { return a; }", + " public void foo(int a, int b) {}", + " });", + " Collections.sort(xs, (a, unused) -> a > a ? 1 : 0);", + " }", + "}") + .doTest(); + } + + @Test + public void unusedFunctionalInterfaceParameter_noFix() { + refactoringHelper + .addInputLines( + "Test.java", + "import java.util.Collections;", + "import java.util.Comparator;", + "import java.util.List;", + "class Test {", + " public void test(List xs) {", + " Collections.sort(xs, (a, b) -> a > a ? 1 : 0);", + " Collections.sort(xs, (a, unused) -> a > a ? 1 : 0);", + " Collections.sort(xs, new Comparator() {", + " @Override public int compare(Integer a, Integer b) { return a; }", + " });", + " }", + "}") + .expectUnchanged() + .doTest(); + } }