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.

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

Introduce ClassCastLambdaUsage check #1381

Merged
merged 2 commits into from
Oct 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package tech.picnic.errorprone.bugpatterns;

import static com.google.errorprone.BugPattern.LinkType.CUSTOM;
import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION;
import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL;

import com.google.auto.service.AutoService;
import com.google.common.collect.Iterables;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.LambdaExpressionTreeMatcher;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.LambdaExpressionTree;
import com.sun.source.tree.TypeCastTree;
import com.sun.source.tree.VariableTree;
import com.sun.tools.javac.code.Type;
import tech.picnic.errorprone.utils.SourceCode;

/**
* A {@link BugChecker} that flags lambda expressions that can be replaced with a method reference
* of the form {@code T.class::cast}.
*/
// XXX: Consider folding this logic into the `MethodReferenceUsage` check of the
// `error-prone-experimental` module.
// XXX: This check and its tests are structurally nearly identical to `IsInstanceLambdaUsage`.
// Unless folded into `MethodReferenceUsage`, consider merging the two.
@AutoService(BugChecker.class)
@BugPattern(
summary = "Prefer `Class::cast` method reference over equivalent lambda expression",
link = BUG_PATTERNS_BASE_URL + "ClassCastLambdaUsage",
linkType = CUSTOM,
severity = SUGGESTION,
tags = SIMPLIFICATION)
public final class ClassCastLambdaUsage extends BugChecker implements LambdaExpressionTreeMatcher {
private static final long serialVersionUID = 1L;

/** Instantiates a new {@link ClassCastLambdaUsage} instance. */
public ClassCastLambdaUsage() {}

@Override
public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState state) {
if (tree.getParameters().size() != 1 || !(tree.getBody() instanceof TypeCastTree typeCast)) {
return Description.NO_MATCH;
}

Type type = ASTHelpers.getType(typeCast);
if (type == null || type.isParameterized()) {
return Description.NO_MATCH;
}

VariableTree param = Iterables.getOnlyElement(tree.getParameters());
if (!ASTHelpers.getSymbol(param).equals(ASTHelpers.getSymbol(typeCast.getExpression()))) {
return Description.NO_MATCH;
}

return describeMatch(
tree,
SuggestedFix.replace(
tree, SourceCode.treeToString(typeCast.getType(), state) + ".class::cast"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
*/
// XXX: Consider folding this logic into the `MethodReferenceUsage` check of the
// `error-prone-experimental` module.
// XXX: This check and its tests are structurally nearly identical to `ClassCastLambdaUsage`. Unless
// folded into `MethodReferenceUsage`, consider merging the two.
@AutoService(BugChecker.class)
@BugPattern(
summary = "Prefer `Class::isInstance` method reference over equivalent lambda expression",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,24 +73,6 @@ Predicate<S> after(Class<T> clazz) {
}
}

/**
* Prefer {@link Class#cast(Object)} method references over lambda expressions that require naming
* a variable.
*/
// XXX: Once the `ClassReferenceCast` rule is dropped, rename this rule to just `ClassCast`.
static final class ClassLiteralCast<T, S> {
@BeforeTemplate
@SuppressWarnings("unchecked")
Function<T, S> before() {
return t -> (S) t;
}

@AfterTemplate
Function<T, S> after() {
return Refaster.<S>clazz()::cast;
}
}

/**
* Prefer {@link Class#cast(Object)} method references over lambda expressions that require naming
* a variable.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package tech.picnic.errorprone.bugpatterns;

import com.google.errorprone.BugCheckerRefactoringTestHelper;
import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode;
import com.google.errorprone.CompilationTestHelper;
import org.junit.jupiter.api.Test;

final class ClassCastLambdaUsageTest {
@Test
void identification() {
CompilationTestHelper.newInstance(ClassCastLambdaUsage.class, getClass())
.addSourceLines(
"A.java",
"import com.google.common.collect.ImmutableSet;",
"import java.util.stream.Stream;",
"",
"class A {",
" void m() {",
" Number localVariable = 0;",
"",
" Stream.of(0).map(i -> i);",
" Stream.of(1).map(i -> i + 1);",
" Stream.of(2).map(Integer.class::cast);",
" Stream.of(3).map(i -> (Integer) 2);",
" Stream.of(4).map(i -> (Integer) localVariable);",
" // XXX: Ideally this case is also flagged. Pick this up in the context of merging the",
" // `ClassCastLambdaUsage` and `MethodReferenceUsage` checks, or introduce a separate check that",
" // simplifies unnecessary block lambda expressions.",
" Stream.of(5)",
" .map(",
" i -> {",
" return (Integer) i;",
" });",
" Stream.<ImmutableSet>of(ImmutableSet.of(5)).map(l -> (ImmutableSet<Number>) l);",
" Stream.of(ImmutableSet.of(6)).map(l -> (ImmutableSet<?>) l);",
" Stream.of(7).reduce((a, b) -> (Integer) a);",
"",
" // BUG: Diagnostic contains:",
" Stream.of(8).map(i -> (Integer) i);",
" }",
"}")
.doTest();
}

@Test
void replacement() {
BugCheckerRefactoringTestHelper.newInstance(ClassCastLambdaUsage.class, getClass())
.addInputLines(
"A.java",
"import java.util.stream.Stream;",
"",
"class A {",
" void m() {",
" Stream.of(1).map(i -> (Integer) i);",
" }",
"}")
.addOutputLines(
"A.java",
"import java.util.stream.Stream;",
"",
"class A {",
" void m() {",
" Stream.of(1).map(Integer.class::cast);",
" }",
"}")
.doTest(TestMode.TEXT_MATCH);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,23 @@ void identification() {
" void m() {",
" Integer localVariable = 0;",
"",
" Stream.of(0).map(i -> i + 1);",
" Stream.of(1).filter(Integer.class::isInstance);",
" Stream.of(2).filter(i -> i.getClass() instanceof Class);",
" Stream.of(3).filter(i -> localVariable instanceof Integer);",
" Stream.of(0).map(i -> i);",
" Stream.of(1).map(i -> i + 1);",
" Stream.of(2).filter(Integer.class::isInstance);",
" Stream.of(3).filter(i -> i.getClass() instanceof Class);",
" Stream.of(4).filter(i -> localVariable instanceof Integer);",
" // XXX: Ideally this case is also flagged. Pick this up in the context of merging the",
" // `IsInstanceLambdaUsage` and `MethodReferenceUsage` checks, or introduce a separate check that",
" // simplifies unnecessary block lambda expressions.",
" Stream.of(4)",
" Stream.of(5)",
" .filter(",
" i -> {",
" return localVariable instanceof Integer;",
" return i instanceof Integer;",
" });",
" Flux.just(5, \"foo\").distinctUntilChanged(v -> v, (a, b) -> a instanceof Integer);",
" Flux.just(6, \"foo\").distinctUntilChanged(v -> v, (a, b) -> a instanceof Integer);",
"",
" // BUG: Diagnostic contains:",
" Stream.of(6).filter(i -> i instanceof Integer);",
" Stream.of(7).filter(i -> i instanceof Integer);",
" }",
"}")
.doTest();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,6 @@ Predicate<String> testClassReferenceIsInstancePredicate() {
return s -> clazz.isInstance(s);
}

Function<Number, Integer> testClassLiteralCast() {
return i -> (Integer) i;
}

Function<Number, Integer> testClassReferenceCast() {
return i -> Integer.class.cast(i);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,6 @@ Predicate<String> testClassReferenceIsInstancePredicate() {
return clazz::isInstance;
}

Function<Number, Integer> testClassLiteralCast() {
return Integer.class::cast;
}

Function<Number, Integer> testClassReferenceCast() {
return Integer.class::cast;
}
Expand Down