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

Ignore type variable casts in ClassCastLambdaUsage check #1449

Conversation

mohamedsamehsalah
Copy link
Contributor

@mohamedsamehsalah mohamedsamehsalah commented Dec 5, 2024

Thanks for pointing out @rickie 🙏

Suggested commit message

Ignore type variable casts in `ClassCastLambdaUsage` check (#1449)

@mohamedsamehsalah mohamedsamehsalah added this to the 0.20.0 milestone Dec 5, 2024
Copy link

github-actions bot commented Dec 5, 2024

  • Surviving mutants in this change: 1
  • Killed mutants in this change: 7
class surviving killed
🧟tech.picnic.errorprone.bugpatterns.ClassCastLambdaUsage 1 7

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.

Actually, it was @EnricSala who pointed this out 😉.

Added a suggestion :).

Copy link

github-actions bot commented Dec 6, 2024

  • Surviving mutants in this change: 1
  • Killed mutants in this change: 7
class surviving killed
🧟tech.picnic.errorprone.bugpatterns.ClassCastLambdaUsage 1 7

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

@@ -48,7 +49,10 @@ public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState
}

Type type = ASTHelpers.getType(typeCast);
if (type == null || type.isParameterized() || type.isPrimitive()) {
if (type == null
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused now, was the pitest warning already here or is it new 🤔?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK:

  1. It was in the original commit, pre suggestions.
  2. It was introduced with this PR, so it is probably referencing mutating the #isPrimitive call 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for checking @mohamedsamehsalah 🚀🚀🚀!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any clue why is this reported? 😅

Copy link
Contributor Author

@mohamedsamehsalah mohamedsamehsalah Dec 11, 2024

Choose a reason for hiding this comment

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

Any clue why is this reported? 😅

I re-read the warning line

A change can be made to line 52 without causing a test to fail

referncing mutating the nullability check; which seems like a common warning for similar PRs (for example, here).

@rickie rickie added the bug fix label Dec 6, 2024
@Stephan202 Stephan202 force-pushed the mohamedsamehsalah/exclude-type-variables-in-class-cast-checker branch from 3dbc7fa to 79d807b Compare December 14, 2024 14:10
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.

Rebased and added a commit. Tnx @mohamedsamehsalah and @EnricSala!

Suggested commit message:

Ignore type variable casts in `ClassCastLambdaUsage` check (#1449)

@rickie I'll let you merge if you agree with the changes.

Comment on lines 56 to 58
/*
* The method reference syntax does not support casting to parameterized types. Additionally,
* `Class#cast` does not support the same range of type conversions between (boxed) primitive
Copy link
Member

Choose a reason for hiding this comment

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

We should update this comment.

Comment on lines 44 to 45
" private <T> void m2() {",
" Stream.of(11).map(i -> (T) i);",
Copy link
Member

Choose a reason for hiding this comment

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

We can add the type variable to m() instead.

@Stephan202 Stephan202 changed the title Exclude generic type variables from ClassCastLambdaUsage checker Ignore type variable casts in ClassCastLambdaUsage check Dec 14, 2024
Copy link

  • Surviving mutants in this change: 1
  • Killed mutants in this change: 7
class surviving killed
🧟tech.picnic.errorprone.bugpatterns.ClassCastLambdaUsage 1 7

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

@rickie rickie force-pushed the mohamedsamehsalah/exclude-type-variables-in-class-cast-checker branch from 79d807b to fc3ecbd Compare December 16, 2024 13:38
@rickie
Copy link
Member

rickie commented Dec 16, 2024

Rebased, changes LGTM!

Copy link

  • Surviving mutants in this change: 1
  • Killed mutants in this change: 7
class surviving killed
🧟tech.picnic.errorprone.bugpatterns.ClassCastLambdaUsage 1 7

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

@Stephan202 Stephan202 merged commit 8dbff50 into master Dec 16, 2024
16 checks passed
@Stephan202 Stephan202 deleted the mohamedsamehsalah/exclude-type-variables-in-class-cast-checker branch December 16, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants