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

Conversation

mohamedsamehsalah
Copy link
Contributor

@mohamedsamehsalah mohamedsamehsalah commented Oct 28, 2024

Suggested commit message

Introduce `ClassCastLambdaUsage` check (#1381)

This new check replaces the `ClassLiteralCast` Refaster rule, as the 
latter produced invalid code by suggesting expressions of the form
`T.class::cast` for generic `T`.

Copy link

  • Surviving mutants in this change: 2
  • Killed mutants in this change: 13
class surviving killed
🧟tech.picnic.errorprone.bugpatterns.ClassCast 2 13

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@Stephan202 Stephan202 changed the title Introduce ClassLiteralCast bug checker Introduce ClassCastLambdaUsage check Oct 28, 2024
Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is an alternative to #1373.

Added a commit and updated the suggested commit message. Tnx @mohamedsamehsalah!

linkType = CUSTOM,
severity = SUGGESTION,
tags = SIMPLIFICATION)
public final class ClassCast extends BugChecker implements LambdaExpressionTreeMatcher {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name is a bit too generic: it's really about preferring the Class::cast method reference. Given the precedence set by IsInstanceLambdaUsage, we could name this check ClassCastLambdaUsage. (Perhaps a better name is ClassCastMethodReference, but let's stay consistent.)


/**
* A {@link BugChecker} that flags lambda expressions that can be replaced with a method reference
* of the form {@code T.class::cast}, if applicable.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those last words are implied by the word "can".

Suggested change
* of the form {@code T.class::cast}, if applicable.
* of the form {@code T.class::cast}.

Comment on lines 21 to 33
" Number foo = 0;",
" Number bar = 1;",
"",
" // BUG: Diagnostic contains:",
" Optional.of(foo).map(i -> (Integer) i);",
"",
" Optional.of(foo).map(i -> 2);",
" Optional.of(foo).map(i -> (Integer) 2);",
" Optional.of(foo).map(i -> bar);",
" Optional.of(foo).map(i -> (Integer) bar);",
"",
" ImmutableList.of(Set.of(foo)).stream().map(l -> (ImmutableSet<Number>) l);",
" ImmutableList.of(Set.of(foo)).stream().map(l -> (ImmutableSet<?>) l);",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would have been nice to stick a bit closer to the test cases in IsInstanceLambdaUsageTest. W.r.t. the ImmutableSet<Number> case: let's more explicitly generate a stream of raw type elements.

Comment on lines 61 to 63
private static boolean isGenericCastExpression(String expression) {
return expression.contains("<") && expression.contains(">");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't be checking the source code like this, but instead ask the type system 😬.

Copy link

Looks good. All 10 mutations in this change were killed.

class surviving killed
🎉tech.picnic.errorprone.bugpatterns.ClassCastLambdaUsage 0 10

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Copy link
Member

@rickie rickie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @mohamedsamehsalah @Stephan202 🚀 !

@rickie rickie force-pushed the mohamedsamehsalah/clas-cast branch from 259948b to 0d45c3a Compare October 29, 2024 09:26
Copy link

Copy link

Looks good. All 10 mutations in this change were killed.

class surviving killed
🎉tech.picnic.errorprone.bugpatterns.ClassCastLambdaUsage 0 10

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@rickie rickie merged commit caf2a86 into master Oct 29, 2024
16 checks passed
@rickie rickie deleted the mohamedsamehsalah/clas-cast branch October 29, 2024 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants