[
https://issues.apache.org/jira/browse/GROOVY-10979?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18136478#comment-18136478
]
ASF GitHub Bot commented on GROOVY-10979:
-----------------------------------------
paulk-asert commented on PR #3089:
URL: https://github.com/apache/groovy/pull/3089#issuecomment-6578808467
Following on from my earlier comment: there's some overlap with the GEP-27
work (GROOVY-12143 lambda hoisting, GROOVY-12151 closure packing, and the
Groovy 7 follow-up in #2714) that may help shape these two PRs.
**1. Only change a closure's representation where nobody can tell the
difference.**
Closure packing changes what a closure literal compiles to only when the
compiler can prove the change can't be seen. It also declines closures that
visibly escape their method (`ClosureWriter.escapesEnclosingMethod`): stores to
an index/property/field, `<<`, `return`, and list/map literals. Those are
exactly the sites #3090 converts, plus `<<` in this PR. So by that rule, these
PRs change the representation exactly where the difference is visible. That's
why dynamic callers break (`h.shout('hi')` and `.call()` on a map filled by
`@CompileStatic` code) and closure variables lose identity (`contains`,
`remove`, `indexOf`, one-way `equals`).
Applying the same rule: convert closure **literals** only (nothing else
holds a reference, so identity isn't an issue), and leave values already typed
`Closure` unconverted, or at least report them.
**2. Gate changes that callers can observe.**
Lambda hoisting and closure packing both ship opt-in
(`groovy.target.lambda.hoist`, `groovy.target.closure.pack`). Packing also has
`@PackedClosures` with `LENIENT`/`WARN`/`STRICT`/`DISABLED` modes, a `.report`
flag giving the reason for each decline, and the remaining differences written
up in its javadoc. These PRs change runtime behaviour unconditionally. I'd
suggest either limiting them to cases where nothing observable changes (point
1), or gating and documenting them the same way.
**3. Record the resolved interface as its own piece of metadata.**
`PackStrategy` already anticipates a LambdaMetafactory-based strategy for
closures with a known functional-interface target. The type-checker work here
(resolving `E` of `add(E)` to `Supplier<String>`) is exactly what that strategy
needs as input. But the conversion used now, a runtime `castToType` producing a
new `ConvertedClosure` proxy each time, is the slowest representation available.
Lambdas already show what's possible. With
`-Dgroovy.target.lambda.hoist=true`, `l.add(() -> 'y')` on a
`List<Supplier<String>>`:
- on master compiles to a Closure class and fails with ClassCastException;
- with this PR (and #3090) merged, it is hoisted and generates no class at
all.
So rather than reusing `PARAMETER_TYPE` (which lambdas, method references
and already-functional parameters also use, hence the second
`getFunctionalTypeArgument` in `StaticInvocationWriter`), it would help to
record "this literal's target is interface T" as a separate fact. A transformer
step could insert the cast (as #3090 and `optimizeArrayCollectionAssignment`
do) and a future metafactory strategy could use the same fact.
**4. Interaction with packing goes both ways.**
With `groovy.target.closure.pack=true`, `list.add { 'x' }` is packed (method
arguments aren't in the escape list) and this PR then wraps it in a proxy. It
works, but it isn't tested. Going the other way: once a stored closure is
converted to an interface, packing's reason for declining escaping closures (a
delegate could be set later) no longer applies, so a shared "target interface"
fact could let #3090's sites pack too. Neither PR is currently exercised with
the GEP-27 flags. `org.apache.groovy-tested.gradle` already forwards both, so
`-Pgroovy.target.lambda.hoist=true -Pgroovy.target.closure.pack=true` would
cover that cheaply.
**5. Possible fix for dynamic callers.**
#2714's `OpenClosure.AsClosure` keeps the full Closure contract as a wrapper
over a lighter core. Going the other way, a converted literal could be a value
that is a Closure *and* implements the interface: the generated closure class
(or a packed adapter variant) implements `Supplier` with a bridge `get()`
calling `doCall()`. Then `h.shout('hi')`, `.call()` and `.apply()` all keep
working and identity is preserved. Closure variables would still need a proxy,
which is another reason to restrict conversion to literals.
None of this needs to block fixing the ClassCastException. But it would be
good to pick the semantics (literals only? gated?) and the metadata shape now,
so this lines up with the GEP-27 direction rather than having to be undone for
Groovy 7.
> Cannot provide method reference to a type parameter instantiated with a SAM
> type
> --------------------------------------------------------------------------------
>
> Key: GROOVY-10979
> URL: https://issues.apache.org/jira/browse/GROOVY-10979
> Project: Groovy
> Issue Type: Bug
> Components: Static Type Checker
> Reporter: Thodoris Sotiropoulos
> Priority: Minor
>
> I have the following program
> {code}
> import java.util.function.*;
> import java.util.stream.*;
> public class Test {
> public static void test() {
> Stream<Number> x = null;
> BiFunction<Function<String, Integer>, Number, Function<String,
> Integer>> y = null;
> BinaryOperator<Function<String, Integer>> z = null;
> x.<Function<String, Integer>>reduce(Test::m, y, z); // fails
> x.reduce(Test::m, y, z); // fails
> x.reduce((d) -> 1, y, z); // works
> }
> public static Integer m(String x) { return 1; }
> }
> {code}
> h3. Actual behavior
> {code}
> org.codehaus.groovy.control.MultipleCompilationErrorsException: startup
> failed:
> test.groovy: 12: The argument is a method reference, but the parameter type
> is not a functional interface
> @ line 12, column 43.
> ction<String, Integer>>reduce(Test::m, y
> ^
> test.groovy: 13: The argument is a method reference, but the parameter type
> is not a functional interface
> @ line 13, column 16.
> x.reduce(Test::m, y, z); // fails
> ^
> 2 errors
> {code}
> h3. Expected behavior
> Compile successfully
> Tested against master (commit: 55c6edfd81a64ce759357a3976070c0811bff637)
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
