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

Extend CollectionIsEmpty Refaster rule #1027

Merged
merged 2 commits into from
Feb 15, 2024

Conversation

giovannizotta
Copy link
Contributor

@giovannizotta giovannizotta commented Feb 10, 2024

Add the following expression to CollectionIsEmpty: collection.stream().findAny().isEmpty().

Clashing issue

Currently, this rule clashes with the rule OptionalFirstCollectionElement, which is matching collection.stream().findAny(), rewriting it to collection.stream().findFirst(). Not sure what's the best way to solve this issue, both use cases seem valid 🤔

  1. Suppressing the suggestion to rewrite collection.stream().findAny().isEmpty() to collection.stream().findFirst().isEmpty() leads to a loop since stream().findFirst().isEmpty() is rewritten to stream().findAny().isEmpty()

Benefits

This acts as a useful partner to StreamIsEmpty: expressions that match the one introduced in this PR can result as a consequence of applying the StreamIsEmpty rule: this allows for refactors like the following to be applied in multiple steps:

1. collection.stream().collect(collectingAndThen(toImmutableList(), List::isEmpty))
3. collection.stream().findAny().isEmpty()
4. collection.isEmpty()

Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@Stephan202 Stephan202 self-requested a review February 11, 2024 10:24
@Stephan202 Stephan202 force-pushed the giovannizotta/extend-collection-is-empty branch from ce8f078 to 35fb0f7 Compare February 11, 2024 11:16
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.

Right, so we generally prefer findFirst() over findAny() (because in the vast majority of cases the former better expresses semantics), but when followed by isEmpty()/isPresent() we prefer the latter. This is a bit of a quirk in our setup; usually we try to avoid those "contextual preferences", as it can cause a combinatorial explosion of cases to consider.

So a second option would be to change the other rules to unconditionally prefer findFirst(). But in this case I think a suppression may be nicer. I rebased and pushed a proposal :)

Suggested commit message:

Extend `CollectionIsEmpty` Refaster rule (#1027)

Comment on lines +51 to +52
collection.stream().findAny().isEmpty(),
collection.stream().findFirst().isEmpty());
Copy link
Member

Choose a reason for hiding this comment

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

So OptionalFirstCollectionElement and StreamIsEmpty prefer different variants. But our Refaster integration prefers writing the longest expression matched, so this one will win.

Copy link
Contributor Author

@giovannizotta giovannizotta Feb 11, 2024

Choose a reason for hiding this comment

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

Understood, I was wondering indeed what happens when an expression is matched by multiple rules. It makes sense that the longest one wins 👍

Copy link
Member

Choose a reason for hiding this comment

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

Yeah; implemented here :)

Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@Stephan202 Stephan202 added this to the 0.16.0 milestone Feb 11, 2024
@Stephan202 Stephan202 requested a review from rickie February 11, 2024 11:29
@giovannizotta
Copy link
Contributor Author

Thank you @Stephan202, indeed the need for suppression comes from the fact that we're "inconsistent" over the preference of findAny() and findFirst(). The suggestions LGTM, thanks a lot!

@Stephan202 Stephan202 force-pushed the giovannizotta/extend-collection-is-empty branch from 35fb0f7 to 4cd8e25 Compare February 11, 2024 15:05
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@Stephan202 Stephan202 force-pushed the giovannizotta/extend-collection-is-empty branch from 4cd8e25 to 0d226df Compare February 11, 2024 16:55
Copy link

Looks good. No mutations were possible for these changes.
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.

🚀

@rickie rickie force-pushed the giovannizotta/extend-collection-is-empty branch from 0d226df to 095e3f1 Compare February 15, 2024 07:11
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@rickie rickie merged commit 8855ba3 into master Feb 15, 2024
15 checks passed
@rickie rickie deleted the giovannizotta/extend-collection-is-empty branch February 15, 2024 07:42
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