Skip to content

Commit

Permalink
Check specific functional interface types (which would otherwise be u…
Browse files Browse the repository at this point in the history 
…nchecked) for parameter usage.

PiperOrigin-RevId: 587714550
  • Loading branch information
@graememorgan
graememorgan authored and Error Prone Team committed Dec 4, 2023
1 parent 7acf133 commit 4be12bc
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 35 deletions.
46 changes: 40 additions & 6 deletions core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<String> FUNCTIONAL_INTERFACE_TYPES_TO_CHECK =
ImmutableSet.of("java.util.Comparator");

private final boolean reportInjectedFields;

@Inject
Expand Down Expand Up @@ -492,6 +497,12 @@ private static ImmutableList<SuggestedFix> buildUnusedVarFixes(

private static ImmutableList<SuggestedFix> buildUnusedParameterFixes(
Symbol varSymbol, List<TreePath> 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<Integer> deletions = TreeRangeSet.create();
Expand Down Expand Up @@ -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)) {
Expand Down Expand Up @@ -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.
Expand All @@ -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)) {
Expand Down
73 changes: 44 additions & 29 deletions core/src/test/java/com/google/errorprone/bugpatterns/UnusedVariableTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Integer, Integer> usedInLambda() {",
" return x -> 1;",
" }",
" private String print(Object o) {",
" return o.toString();",
" }",
" public List<String> print(List<Object> 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
Expand Down Expand Up @@ -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<Integer> xs) {",
" // BUG: Diagnostic contains: 'b' is never read",
" Collections.sort(xs, (a, b) -> a > a ? 1 : 0);",
" Collections.sort(xs, new Comparator<Integer>() {",
" // 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<Integer> xs) {",
" Collections.sort(xs, (a, b) -> a > a ? 1 : 0);",
" Collections.sort(xs, (a, unused) -> a > a ? 1 : 0);",
" Collections.sort(xs, new Comparator<Integer>() {",
" @Override public int compare(Integer a, Integer b) { return a; }",
" });",
" }",
"}")
.expectUnchanged()
.doTest();
}
}

0 comments on commit 4be12bc

Please sign in to comment.