Introduce ExplicitArgumentEnumeration check
#985
Conversation
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
| // XXX: Once we target JDK 14+, drop this method in favour of `Symbol#isPublic()`. | ||
| private static boolean isPublic(Symbol symbol) { | ||
| return symbol.getModifiers().contains(Modifier.PUBLIC); | ||
| } |
There was a problem hiding this comment.
Also present in #972, but a separate utility method doesn't seem worth it. (Or we can just drop JDK 11 support 🙃.)
Stephan202
force-pushed
the
sschroevers/prefer-varargs-alternatives
branch
from
d3877c5 to
a74fd35
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Stephan202
force-pushed
the
sschroevers/prefer-varargs-alternatives
branch
from
a74fd35 to
9e7ac68
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Stephan202
force-pushed
the
sschroevers/prefer-varargs-alternatives
branch
from
9e7ac68 to
714713b
Compare
| if (tree.getArguments().size() != 1) { | ||
| return Description.NO_MATCH; | ||
| } |
There was a problem hiding this comment.
Pitest reports that this if-statement can be mutated away. That's correct: it's a performance optimization. While we haven't done so in the past, it may be good to start documenting such cases. Will do for this case.
| private static boolean isUnaryIterableAcceptingMethod(MethodSymbol method, VisitorState state) { | ||
| List<VarSymbol> params = method.params(); | ||
| return !method.isVarArgs() | ||
| && params.size() == 1 | ||
| && ASTHelpers.isSubtype(params.get(0).type, state.getSymtab().iterableType, state); | ||
| } |
There was a problem hiding this comment.
Pitest flags that the params.size() == 1 can be mutated away. This is true thanks to the initial tree.getArguments().size() != 1 check, but it doesn't seem right to drop this condition; it makes the code less clear, IMHO.
|
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Stephan202
force-pushed
the
sschroevers/prefer-varargs-alternatives
branch
from
714713b to
6ae584f
Compare
|
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
1 similar comment
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Stephan202
force-pushed
the
sschroevers/prefer-varargs-alternatives
branch
from
6ae584f to
4101867
Compare
|
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Stephan202
force-pushed
the
sschroevers/prefer-varargs-alternatives
branch
from
4101867 to
5f14738
Compare
|
|
And this....
|
I filed #1455 for this. |
Same also happened for a |
|
Oops, sorry, don't know what happened there 😓. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
/integration-test |
There was a problem hiding this comment.
Only one small typo and a suggestion :). Nice check @Stephan202 . Sorry for the slow review 🐌.
pom.xml
Outdated
| <dependency> | ||
| <groupId>org.jooq</groupId> | ||
| <artifactId>jooq</artifactId> | ||
| <version>3.16.23</version> |
There was a problem hiding this comment.
There is a higher version, a specific reason to not use that yet?
There was a problem hiding this comment.
Well, maybe the age of this PR has something to do with that? 🙃
There was a problem hiding this comment.
Haha, my bad. I just remembered in JPLA that JOOQ usually had some edge cases. Didn't check on which version we are internally but was thinking maybe there was something related to that .
| " assertThat(ImmutableList.of()).containsExactlyInAnyOrderElementsOf(Set.of());", | ||
| " assertThat(ImmutableList.of()).containsSequence(Arrays.asList());", | ||
| " assertThat(ImmutableList.of()).containsSubsequence(ImmutableList.of(1));", | ||
| " assertThat(ImmutableList.of()).doesNotContainAnyElementsOf(ImmutableMultiset.of(1));", |
There was a problem hiding this comment.
No incrementing numbers this time? 😋
I see that the methods are different but might be "more" consistent 🤔
There was a problem hiding this comment.
Hmm, the number is per statement here, just like in the identification test above. Will tweak here, but leave the rest as-is.
| .flatMap(e -> trySuggestCallingCustomAlternative(tree, argument, state, e.getValue())); | ||
| } | ||
|
|
||
| private static Optional<SuggestedFix> trySuggestCallingCustomAlternative( |
There was a problem hiding this comment.
Probably the only exception where VisitorState is not the last parameter :).
There was a problem hiding this comment.
Yeah, that's to appease the "consistent overloads" rule. The alternative is to find another method name.
There was a problem hiding this comment.
No it's fine, just found it funny that indeed the overload rule caused the first exception :).
| .build()); | ||
| } | ||
|
|
||
| private static boolean isAtLeastAsVisible(Element symbol, Element reference) { |
There was a problem hiding this comment.
Was thinking that the name modifier in this method name would be nice as addition, but also fine with leaving it as is.
There was a problem hiding this comment.
The modifier is a syntactic means of influencing visibility, but for the purpose of this method that doesn't seem like a relevant detail.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Stephan202
force-pushed
the
sschroevers/prefer-varargs-alternatives
branch
from
7043cd0 to
d602a2b
Compare
| .flatMap(e -> trySuggestCallingCustomAlternative(tree, argument, state, e.getValue())); | ||
| } | ||
|
|
||
| private static Optional<SuggestedFix> trySuggestCallingCustomAlternative( |
There was a problem hiding this comment.
Yeah, that's to appease the "consistent overloads" rule. The alternative is to find another method name.
| .build()); | ||
| } | ||
|
|
||
| private static boolean isAtLeastAsVisible(Element symbol, Element reference) { |
There was a problem hiding this comment.
The modifier is a syntactic means of influencing visibility, but for the purpose of this method that doesn't seem like a relevant detail.
| " assertThat(ImmutableList.of()).containsExactlyInAnyOrderElementsOf(Set.of());", | ||
| " assertThat(ImmutableList.of()).containsSequence(Arrays.asList());", | ||
| " assertThat(ImmutableList.of()).containsSubsequence(ImmutableList.of(1));", | ||
| " assertThat(ImmutableList.of()).doesNotContainAnyElementsOf(ImmutableMultiset.of(1));", |
There was a problem hiding this comment.
Hmm, the number is per statement here, just like in the identification test above. Will tweak here, but leave the rest as-is.
pom.xml
Outdated
| <dependency> | ||
| <groupId>org.jooq</groupId> | ||
| <artifactId>jooq</artifactId> | ||
| <version>3.16.23</version> |
There was a problem hiding this comment.
Well, maybe the age of this PR has something to do with that? 🙃
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
Changed the scope of the new dependency. @rickie: also here, feel free to merge if you agree with the final changes. |
Suggested commit message:
I found this on an old branch from early last year. Extended it a bit and now putting it up for review. Perhaps we can find a better name. Likely this code should also be tested against the internal code base before landing. But it's certainly ready for review.