Avoid invalid ExplicitArgumentEnumeration suggestions
#1637
Conversation
|
Looks good. All 9 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
There was a problem hiding this comment.
Thanks for jumping on his @mohamedsamehsalah! I added a commit. Suggested commit message:
Avoid invalid `ExplicitArgumentEnumeration` suggestions (#1637)
While still imperfect, this change improves the heuristics that
determine whether an explicit `Iterable` creator invocation can be
unwrapped, such that the enumerated values will be passed to a
compatible varargs overload instead. The new logic validates that the
overload accepts values of the appropriate type.
| } | ||
|
|
||
| /** | ||
| * Return true when at least one of the overloads accepts varargs parameter that is of the same |
There was a problem hiding this comment.
| * Return true when at least one of the overloads accepts varargs parameter that is of the same | |
| * Returns true when at least one of the overloads has a varargs parameter that is of the same |
| private static boolean isSubtype( | ||
| Type methodParameterType, Type overloadArgumentType, VisitorState state) { | ||
| return ASTHelpers.isSubtype( | ||
| methodParameterType.getTypeArguments().get(0), | ||
| state.getTypes().elemtype(overloadArgumentType), | ||
| state); |
There was a problem hiding this comment.
ASTHelpers.isSubtype performs type erasure; we should avoid that. At the same time, in some test cases this code is now used to check whether ? extends ELEMENT is considered a subtype of ELEMENT, and that's not true without erasure. (With erasure both side just become java.lang.Object.) I played around with some options, and it appears that we can use the containsType operation instead.
There was a problem hiding this comment.
Ah; I was also not 💯 sure about this one 👍
| * parameter type as the original method's. | ||
| */ | ||
| private static boolean hasVarArgsOverLoadOfSameParameterType( | ||
| ImmutableList<MethodSymbol> overloads, MethodSymbol method, VisitorState state) { |
There was a problem hiding this comment.
Logically I'd expect the method parameter to be listed first.
| boolean hasLikelySuitableVarargsOverload = | ||
| overloads.stream().allMatch(m -> m.params().size() == 1) | ||
| && overloads.stream().anyMatch(MethodSymbol::isVarArgs); | ||
| && hasVarArgsOverLoadOfSameParameterType(overloads, method, state); |
There was a problem hiding this comment.
Let's just encapsulate all logic into the new method.
| .anyMatch( | ||
| overload -> | ||
| isSubtype( | ||
| method.getParameters().get(0).type, |
There was a problem hiding this comment.
This expression is loop invariant; it can be pulled out.
There was a problem hiding this comment.
Let's also use Iterables.getOnlyElement, to explicitly communicate our expectations.
| .collect(toImmutableList()); | ||
|
|
||
| /* | ||
| * If all overloads have a single parameter, and at least one of them is a varargs method, then |
There was a problem hiding this comment.
Now it's not just any varargs method :)
| // XXX: There are certainly cases where it _would_ be nice to unwrap the arguments to | ||
| // `org.jooq.impl.DSL#row(Collection<?>)`. Look into this. | ||
| // XXX: Ideally we do check that one of the overloads accepts the unwrapped arguments. | ||
| // XXX: Ideally we validate that eligible overloads have compatible return types. |
There was a problem hiding this comment.
We can now drop one of the XXX comments, IIUC.
| private static boolean isSubtype( | ||
| Type methodParameterType, Type overloadArgumentType, VisitorState state) { | ||
| return ASTHelpers.isSubtype( | ||
| methodParameterType.getTypeArguments().get(0), |
There was a problem hiding this comment.
This code makes the assumption that the unwrapped iterable's type has generic type parameters, and that the first represents the element type. This doesn't hold in general. (Consider e.g. class StringList extends ArrayList<String>() {}). Due to the matchers used this construct may code may in practice, but then we should at least document this gotcha.
| " // BUG: Diagnostic contains:", | ||
| " private final int value = unaryMethod(ImmutableList.of(1, 2));", | ||
| "", | ||
| " private final Builder value2 =", | ||
| " Counter.builder(\"foo\").tags(ImmutableList.of(Tag.of(\"bar\", \"baz\")));", |
There was a problem hiding this comment.
The existing field declaration is there to cover a branch in the isLocalOverload; we can simplify this test case by moving the expression to the method below, and omitting the field declaration.
Stephan202
changed the title
ExplicitArgumentEnumeration suggestionExplicitArgumentEnumeration suggestions
|
Looks good. All 15 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
Thanks a lot for the review and suggestions @Stephan202 🙏 LGTM! |
By checking that the varargs overload accepts a parameter of the same type as the original method invocation.
rickie
force-pushed
the
mohamedsamehsalah/fix-explicit-argument-enumeration
branch
from
a8979c0 to
0539457
Compare
|
|
Looks good. All 15 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Stephan202
deleted the
mohamedsamehsalah/fix-explicit-argument-enumeration
branch
3 participants
When
ExplicitArgumentEnumerationmakes a replacement suggestion, it does not take into account that the replacement method is an override of the same type.Example:
would have been replaced to
While the
#tagsmethod has no overloaded method that accepts varargs of typeTag(onlyString...).Suggested commit message 🎉