paulk-asert opened a new pull request, #3257:
URL: https://github.com/apache/groovy/pull/3257
## The problem
When overloads take functional interfaces whose abstract methods have
different numbers of parameters, Groovy could not use the functional argument's
arity to choose between them, except for closures and lambdas, and then only as
a tie-break (GROOVY-9881, GROOVY-10905, GROOVY-11121). So:
- A method reference or pointer was ambiguous: `red(numbers, Integer::sum)`
with `red(Collection, BinaryOperator)` and `red(Collection, Function)`, or
`from(Main::m)` with `Function` and `Supplier` overloads (GROOVY-11010). Static
type checking reported `Reference to method is ambiguous`, and dynamic code
reported `Ambiguous method overloading`.
- Where `Runnable` or `Callable` competed with an interface that takes
arguments, the argument matched `Runnable` or `Callable`, because `Closure`
implements them and they win on distance before any tie-break applies
(GROOVY-10714). This covered `andThenTry(table::putItem)`, `andThenTry { String
s -> … }` and `andThenTry((String s) -> …)` with `Consumer`/`Runnable`
overloads. Under static type checking the call was then rejected. At run time
the closure was called with `null`, and the method reference failed when run.
The vavr shape in GROOVY-10714 (`CheckedConsumer`/`CheckedRunnable`)
compiled under `@CompileStatic` on 4.0.33 but is ambiguous on 5.1.3 and 6.0.0,
the regression reported there.
## The change
As in Java (JLS 15.12.2.1), a functional argument can only implement an
interface whose abstract method it can be called with. Before ranking
candidates by distance, method selection now drops those whose
functional-interface parameter takes a number of arguments the argument cannot
accept. If none fits, all candidates are kept and selection proceeds as before.
1. **`@TypeChecked`/`@CompileStatic`, method references and pointers
(GROOVY-11010).** The argument counts come from the methods of the reference's
name: their own parameters for a static method or a bound receiver, and one
more for the receiver of `Type::instanceMethod`. Varargs methods accept any
count from one fewer, and constructor references use the constructors.
2. **Run time, method closures (GROOVY-10714).** This covers dynamic code
and `@TypeChecked` calls, which are dispatched at run time. A `MethodClosure`'s
class says nothing about the method it names, so a choice that depends on it is
not cached, neither by the metaclass nor by the indy call site. That only
happens where a method closure is passed for parameters whose functional
interfaces differ in arity among the valid candidates, and such calls failed or
misbehaved before. All other selections are cached as before.
- `MethodClosure` works out its argument counts when first asked, and
again when `rehydrate` gives it an owner of another class.
- The counts are `transient`, so the serial form is unchanged.
- A dehydrated method closure, whose methods are unknown, is chosen for
as before.
3. **Closures and lambdas with declared parameters (GROOVY-10714), static
and dynamic.** Under static type checking the counts come from the declared
parameters, allowing for default values and a trailing array. At run time they
come from the class's `doCall` methods, cached per class, so these selections
stay cacheable. Packed closures present the same view through their fixed-arity
classes.
4. `COMPATIBILITY.md` entry.
## What is unchanged
These cases avoid the problems noted on GROOVY-10714 with re-ranking
`Runnable`/`Callable`:
- A closure with an implicit parameter (`{ … }`, `{ it }`) and `{ -> … }`
still prefer `Runnable`/`Callable`.
- Interfaces of the same arity, such as `Runnable` and `Callable`, are
chosen between as before, so `executor.submit { -> … }` (GROOVY-11681) is
unaffected.
- Parameters of type `Closure`, `Object`, or an interface without a single
abstract method (`Serializable`, `Cloneable`, `GroovyObject`) are never dropped.
- A method reference whose methods accept both arities, such as
`Integer::valueOf`, stays ambiguous.
## Compatibility
- **Behaviour:** dynamic code that passes a closure with declared
parameters, or a method reference, where `Runnable`/`Callable` competes with an
interface of matching arity now gets that interface. The closure receives its
arguments instead of `null`. Under static type checking those calls did not
compile before, so no code that compiles today changes.
- **API:** two `@since 6.0.1` static helpers on the internal
`MetaClassHelper`. `MethodClosure` gains only package-private members, and its
serial form is unchanged (pinned by a test).
## Tests
Each new regression test fails on master and passes with the change. The few
that pin unchanged behaviour pass on both.
- `MethodReferenceTest` (`@CompileStatic`) covers:
- each kind of reference: static, unbound instance, bound, constructor and
`.&` pointer;
- GROOVY-11010 `from(Main::m)` and `from(Main::n)`;
- the GROOVY-10714 `table::putItem` shape, and the `Checked*` pair;
- `Runnable` still preferred over `Callable`;
- `Integer::valueOf` still ambiguous.
- `ClosuresSTCTest`, run under `@TypeChecked` and, through
`ClosuresStaticCompileTest`, `@CompileStatic`: `Consumer`/`Runnable` and
`Function`/`Callable` with explicit-parameter, no-parameter and implicit
closures, and a lambda.
- `bugs.Groovy10714` covers run-time dispatch:
- dynamic and `@TypeChecked` method references, and the
`Consumer`/`Runnable` and `Checked*` shapes;
- closures with declared and default parameters, also under
`@PackedClosures`, with an assertion that the closure is packed;
- one call site given references, and then closures, of different arity,
called past the first link, so nothing is wrongly cached;
- rehydrated and dehydrated method closures and closures;
- the multi-arity reference;
- the serial form of `MethodClosure`.
The full root `:test` suite ran 19,777 tests with one failure,
`TestDgmConverter.testProxyFactoryIsShardedAndReflectionFree` ("expected 2012,
was 2010"), which fails identically on master. `./gradlew lintGate` passes.
## Not included
- Interfaces of the same arity that differ only in returning a value or
`void` (`Runnable` vs `Callable`/`Supplier` for `K::value`): Java prefers the
value-returning one. That needs a return-compatibility check and would change
the `Runnable`-over-`Callable` preference, so it is a separate decision.
- `@CompileStatic` still rejects a closure with default or trailing-array
parameters for a functional interface, as on master. Its overload is now
selected correctly.
- Closures returned by `memoize()`, `curry()` and `trampoline()` are not
closure literals, so they are selected for as before. Their arity is per
instance.
- While checking round trips, I noticed that `MethodClosure.readObject` has
thrown even when `MethodClosure.ALLOW_RESOLVE` is set since GROOVY-10057. So a
method closure cannot be deserialized at all, which may or may not be intended.
This PR does not change it.
The `@CompileStatic` part (first commit) is self-contained and fixes the 4.0
→ 5.x regression, so it could be backported on its own. The run-time parts
change method selection and caching, so I would keep them for master.
--
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]
