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 Refaster rules that resolve EnumOrdinal violations #1104

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

Stephan202
Copy link
Member

@Stephan202 Stephan202 commented Mar 23, 2024

Suggested commit message:

Introduce Refaster rules that resolve `EnumOrdinal` violations (#1104)

These rules resolve some of the violations reported by the EnumOrdinal check new in Error Prone 2.26.0.

@Stephan202 Stephan202 added this to the 0.17.0 milestone Mar 23, 2024
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 Author

@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.

Some context :)

Comment on lines +98 to +114
/** Don't explicitly compare enums by their ordinal. */
abstract static class ComparingEnum<E extends Enum<E>, T> {
@Placeholder(allowsIdentity = true)
abstract E toEnumFunction(@MayOptionallyUse T value);

@BeforeTemplate
@SuppressWarnings("EnumOrdinal" /* This violation will be rewritten. */)
Comparator<T> before() {
return comparingInt(v -> toEnumFunction(v).ordinal());
}

@AfterTemplate
@UseImportPolicy(STATIC_IMPORT_ALWAYS)
Comparator<T> after() {
return comparing(v -> toEnumFunction(v));
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This will rewrite comparingInt(e -> e.getKey().ordinal()) to comparing(v -> v.getKey()). That result will not also be rewritten to comparingByKey(), because MapEntryComparingByKey matches against Map.Entry::getKey but not the associated lambda expression. This limitation applies to many of our rules, so fixing it is out of scope. (We should really "just" fix the MethodReferenceUsage check.)

I'm mentioning this because it'll prevent a cleanup opportunity in our internal code base, I see.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

@mohamedsamehsalah mohamedsamehsalah left a comment

Choose a reason for hiding this comment

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

Neat ⭐

@rickie rickie force-pushed the sschroevers/EnumOrdinal-resolutions branch from de23007 to 0c48f7a Compare March 27, 2024 13:21
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

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.

@rickie rickie merged commit 769779c into master Mar 27, 2024
15 checks passed
@rickie rickie deleted the sschroevers/EnumOrdinal-resolutions branch March 27, 2024 14:44
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