Add validation for Ordering.explicit() to contain all enum values
#14
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Stephan202
force-pushed
the
ivan-fedorov/ordering-explicit-for-enums
branch
from
668b0b5
to
b2a75cc
Compare
Stephan202
approved these changes
Mar 28, 2021
| .map(Name::toString) | ||
| .collect(toImmutableSet()); | ||
|
|
||
| LinkedHashSet<String> enumValues = |
There was a problem hiding this comment.
Suggested change
| LinkedHashSet<String> enumValues = | |
| Set<String> enumValues = |
Comment on lines
45
to
46
| if (not(staticMethod().onClass(Ordering.class.getName()).named("explicit")) | ||
| .matches(tree, state)) { |
There was a problem hiding this comment.
Suggested change
| if (not(staticMethod().onClass(Ordering.class.getName()).named("explicit")) | |
| .matches(tree, state)) { | |
| if (!staticMethod().onClass(Ordering.class.getName()).named("explicit") | |
| .matches(tree, state)) { |
There was a problem hiding this comment.
Also: we should move the matcher construction to a static field, so that this isn't repeated for every method invocation.
| } | ||
|
|
||
| private static boolean isNotEnumType(List<? extends ExpressionTree> arguments) { | ||
| return arguments.stream().anyMatch(Predicate.not(arg -> ASTHelpers.getSymbol(arg).isEnum())); |
There was a problem hiding this comment.
Suggested change
| return arguments.stream().anyMatch(Predicate.not(arg -> ASTHelpers.getSymbol(arg).isEnum())); | |
| return arguments.stream().anyMatch(arg -> !ASTHelpers.getSymbol(arg).isEnum()); |
| .build(); | ||
| } | ||
|
|
||
| private static boolean isNotEnumType(List<? extends ExpressionTree> arguments) { |
There was a problem hiding this comment.
Suggested change
| private static boolean isNotEnumType(List<? extends ExpressionTree> arguments) { | |
| private static boolean containsNonEnumValue(List<? extends ExpressionTree> arguments) { |
| .collect(toImmutableSet()); | ||
|
|
||
| LinkedHashSet<String> enumValues = | ||
| getTypeSymbolStream(arguments).map(ASTHelpers::enumValues).findFirst().orElseThrow(); |
There was a problem hiding this comment.
Making clearer that only one map will be performed:
Suggested change
| getTypeSymbolStream(arguments).map(ASTHelpers::enumValues).findFirst().orElseThrow(); | |
| getTypeSymbolStream(arguments).findFirst().map(ASTHelpers::enumValues).orElseThrow(); |
Though then we can just do:
Suggested change
| getTypeSymbolStream(arguments).map(ASTHelpers::enumValues).findFirst().orElseThrow(); | |
| ASTHelpers.enumValues(getTypeSymbolStream(arguments).findFirst().orElseThrow()); |
| "import java.util.Comparator;", | ||
| "", | ||
| "enum A {", | ||
| " ONE, TWO, THREE", |
There was a problem hiding this comment.
We're using non-AOSP style here, so two space indent :)
| "", | ||
| "class B {", | ||
| " // BUG: Diagnostic contains: Method should include all values from A enum", | ||
| " Comparator<A> explicit = Ordering.explicit(A.TWO, A.ONE);", |
There was a problem hiding this comment.
Let's also test with statically imported enum values.
Comment on lines
36
to
38
| linkType = BugPattern.LinkType.NONE, | ||
| severity = BugPattern.SeverityLevel.WARNING, | ||
| tags = BugPattern.StandardTags.FRAGILE_CODE) |
There was a problem hiding this comment.
Like in the other classes:
Suggested change
| linkType = BugPattern.LinkType.NONE, | |
| severity = BugPattern.SeverityLevel.WARNING, | |
| tags = BugPattern.StandardTags.FRAGILE_CODE) | |
| linkType = LinkType.NONE, | |
| severity = SeverityLevel.WARNING, | |
| tags = StandardTags.FRAGILE_CODE) |
Comment on lines
27
to
30
| /** | ||
| * A {@link BugChecker} which flags that {@link com.google.common.collect.Ordering#explicit(Object, | ||
| * Object[])}} for enums does not contain all it's values. | ||
| */ |
There was a problem hiding this comment.
Suggested change
| /** | |
| * A {@link BugChecker} which flags that {@link com.google.common.collect.Ordering#explicit(Object, | |
| * Object[])}} for enums does not contain all it's values. | |
| */ | |
| /** | |
| * A {@link BugChecker} which flags {@link Ordering#explicit(Object, Object[])}} invocations listing | |
| * a subset of an enum type's values. | |
| */ |
| */ | ||
| @AutoService(BugChecker.class) | ||
| @BugPattern( | ||
| name = "OrderingExplicitMethod", |
There was a problem hiding this comment.
How about:
Suggested change
| name = "OrderingExplicitMethod", | |
| name = "ExplicitEnumOrdering", |
|
Build failure is unrelated to this PR; will fix |
Stephan202
force-pushed
the
ivan-fedorov/ordering-explicit-for-enums
branch
from
b2a75cc
to
0543be9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
According to the
Ordering.explicit()documentation it can throwClassCastExceptionwhen it receives an input parameter that isn't among the provided values.So in case when we had such method in a codebase with enum values and after time someone adds new values to that enum we need to make sure that the logic where
Ordering.explicit()is used works as expected and include a new value there aswell.