paulk-asert commented on PR #3089:
URL: https://github.com/apache/groovy/pull/3089#issuecomment-6578389828
Thanks for this, and for splitting out #3090. I think these two need a bit
more thought before merging, and should be decided together since they share
the root cause (a closure flowing into a type-parameter slot whose erasure
isn't the functional interface).
A few things that came up when probing master vs both PRs merged:
1. Dynamic Groovy never converts in any of these cases, so this makes
@CompileStatic behaviour diverge from dynamic. Arguably the right direction,
but it should be a deliberate, documented change.
2. It changes behaviour for dynamic code that reads values built by
@CompileStatic code. A @CompileStatic class that fills a Map<String,
Function<String,String>> with `m['shout'] = { ... }` or `m.put('whisper') { ...
}` currently gives dynamic callers a Closure, so `h.shout('hi')` and
`h.shout.call('hi')` work. With the PRs, the stored value is a proxy and both
throw MissingMethodException.
3. Closure variables are converted too, not just literals, and each
conversion makes a new proxy. So `Closure c = {..}; l.add(c)` then
`l.contains(c)` is false, `l.remove((Object) c)` does nothing, `l.indexOf(c)`
is -1, and equality is one-sided (`proxy.equals(c)` holds but `c.equals(proxy)`
doesn't). Converting only literals would avoid this.
4. Under plain @TypeChecked, the type checker now accepts things like `l.add
{ it.toUpperCase() }` and `l.add(String::toUpperCase)`, but nothing is
converted at runtime, so the later `apply` call throws MissingMethodException.
5. Implementation: the two PRs use different conversion mechanisms. #3089
adds a cast in the writer, plus `castX` for varargs; #3090 inserts a
CastExpression in the transformer and, for named constructor args, uses yet
another metadata convention. Recording the target type in the type checker and
inserting the cast in one transformer step, as
`optimizeArrayCollectionAssignment` does, would remove the second
`getFunctionalTypeArgument` in StaticInvocationWriter. Please also share one
closure test and one rule for skipping Runnable/Callable (#3090 skips them,
#3089 doesn't).
Whatever we decide on 2 and 3, it would be good to have tests pinning that
behaviour.
--
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]
