daniellansun commented on PR #3265:
URL: https://github.com/apache/groovy/pull/3265#issuecomment-6718282980
REQUEST_CHANGES
The Pekko assignment and return both type-check, for a lambda and for a
closure, and GenericsSTCTest is green including the three new methods. The
witness is written onto StaticMethodCallExpression and then read only from the
enclosing-call stack, which visitStaticMethodCallExpression never pushes. By
INSTRUCTION_SELECTION, StaticImportVisitor has already turned a same-class
static call and an `import static` call into StaticMethodCallExpression, so
those still fail with the original error:
static Beh<String> go(int max) { setup(c -> new Bot(c, max)) }
// Cannot call Bot#<init>(Ctx<String>, int) with arguments [Ctx<?
extends Object>, int]
javac accepts that. `import static java.util.Comparator.comparing` then
`Comparator<String> c = comparing(s -> s.length())` fails here with
Object#length() and Comparator<Object> assigned to Comparator<String>.
Qualified Behaviors.setup(...) stays a MethodCallExpression, which is the only
shape the new tests hit.
Please push/pop the static call in visitStaticMethodCallExpression the same
way visitMethodCallExpression does, and add those two calls beside
testReturnTypeInferenceWithClosure5. makeArgumentList returns the same
ArgumentListExpression the parser stored, so the identity check in
extractGenericsConnectionsFromCallTarget will then succeed.
A few targets the helper’s comment says it determines are still unresolved.
I compared them with javac on this tip:
- `<T extends CharSequence> List<T> ids(Function<T,T>)` assigned to
`List<String>` leaves the lambda parameter as CharSequence
(`CharSequence#substring` is missing; the assignment is List<CharSequence> to
List<String>). javac uses T = String. An out-of-bound target (`T extends
Number` against a call whose parameter needs String) stays at Number, so this
is under-acceptance, not a bad instantiation. putIfAbsent still does not check
the witness against T’s upper bounds.
- `List<List<T>>` assigned to `List<List<String>>`, and `Map<A,B>` assigned
to `Map<String,Integer>`, both leave the variables at Object
(`Object#length()`, List<List<Object>>, Map<Object,Object>). The return type
still carries placeholders named T, A, and B. ArrayList<T> versus List<String>
works; the nested and two-variable cases do not. Map<T,T> versus
Map<String,Integer> is rejected, which matches javac, but only as a last-write
in storeGenericsConnection rather than a conflict error.
- `Beh<? super String> b = Behaviors.setup(c -> new Bot(c))` is still Ctx<?
extends Object>. javac accepts it: ? super String pins T to String, and
Beh<String> is a subtype of Beh<? super String>. Skipping every wildcard is
right for Beh<? extends Object> (test 7, and javac agrees) and too coarse for a
concrete ? super or ? extends bound.
- A ternary in an assignment, a call passed to take(Beh<String>), and
Optional.of(setup(...)) still show Ctx<? extends Object>. The target is applied
to the ternary node and not to the branch, and checkForTargetType still has the
TODO for argument parameters. A cast does infer T, because visitCastExpression
calls applyTargetType; javac does not treat a cast as a poly context for the
invocation (JLS 15.12) and rejects `(Beh<String>) setup(c -> new Bot(c))`. The
Groovy result is sound. Worth a comment so the cast hook is intentional.
- inferReturnTypeGenerics does not see CALL_TARGET_TYPE. `List<CharSequence>
xs = cons(s -> 'x')` for `Function<? super T, ? extends T>` is inferred as
List<String> and rejected. javac uses T = CharSequence.
GROOVY-11446 still type-checks (`List<Number> x = m(d)` for `List<?> d` and
`<T> List<T> m(List<? extends T>)`). There is no SAM argument, so this patch
does not touch it. javac rejects it. The lambda form of that pattern is
rejected here. Not a regression; not fixed either.
I would keep Closure7 as the unbounded-wildcard rejection, and add:
same-class static and import static; T extends CharSequence against
List<String>; List<List<T>> and Map<A,B>; Beh<? super String> must pass; the
Map<T,T> / Map<String,Integer> conflict must fail; one ternary and one argument
context. String as a target is a weak assertion on its own, because STC will
accept almost any value as a String. The Bot constructor in Closure5 is the
right kind of check.
The ReturnAdder widening looks safe: the listener constructor does not
rewrite the method, and an explicit return is stamped only from
visitReturnStatement. I do not see a merge conflict with the PECS work on the
lines this patch adds; that branch is not an ancestor of 60711c748721. Once a ?
super bound is stored as a witness, it will meet the tighter
compatibleConnection on that branch and should be rechecked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
