[
https://issues.apache.org/jira/browse/GROOVY-12214?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18100353#comment-18100353
]
ASF GitHub Bot commented on GROOVY-12214:
-----------------------------------------
Copilot commented on code in PR #2749:
URL: https://github.com/apache/groovy/pull/2749#discussion_r3681464170
##########
src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java:
##########
@@ -3105,9 +3105,17 @@ protected MethodNode typeCheckMapConstructor(final
ConstructorCallExpression cal
* Returns the inferred argument types for the supplied argument list.
*/
protected ClassNode[] getArgumentTypes(final ArgumentListExpression
argumentList) {
- return argumentList.getExpressions().stream().map(exp ->
- isNullConstant(exp) ? UNKNOWN_PARAMETER_TYPE : getType(exp)
- ).toArray(ClassNode[]::new);
+ return argumentList.getExpressions().stream().map(exp -> {
+ if (isNullConstant(exp)) return UNKNOWN_PARAMETER_TYPE;
+ ClassNode type = getType(exp);
+ if (exp instanceof MethodReferenceExpression && type != null) {
+ // a method reference (::) can only coerce to a functional
interface, never to
+ // groovy.lang.Closure; mark it so overload selection does not
prefer a Closure
+ // parameter over the functional-interface twin (which would
then fail to coerce)
Review Comment:
These comments state that a method reference can only coerce to a functional
interface and never to Closure, which is not accurate for Groovy in general
(the test suite already includes `Closure<?> fn = String::length`). Consider
rewording the comment to describe the overload-selection preference without
asserting that Closure coercion is impossible.
##########
src/main/java/org/codehaus/groovy/transform/stc/StaticTypesMarker.java:
##########
@@ -67,5 +67,7 @@ public enum StaticTypesMarker {
/** GROOVY-11998: for an intersection-cast lambda or method reference, the
SAM-bearing component picked from the intersection */
PRIMARY_FUNCTIONAL_TYPE,
/** GROOVY-11998: for an intersection cast on a lambda, method reference
or closure, the additional marker interfaces to thread to {@code
LambdaMetafactory.altMetafactory} */
- LAMBDA_MARKERS
+ LAMBDA_MARKERS,
+ /** marks the argument type of a method reference ({@code ::}) so overload
selection does not prefer a {@code Closure} parameter over a
functional-interface twin, since a method reference can never coerce to {@link
groovy.lang.Closure} */
Review Comment:
The new Javadoc claims a method reference can never coerce to
groovy.lang.Closure, but this is not generally true in Groovy (e.g. the
existing test `testMethodRefAssignedToClosureType` in this same test class
assigns `String::length` to `Closure<?>`). The marker’s purpose here is
overload-selection preference, so the comment should avoid the incorrect
absolute statement.
##########
src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingSupport.java:
##########
@@ -403,6 +403,12 @@ public static int
allParametersAndArgumentsMatch(Parameter[] parameters, final C
ClassNode aType = argumentTypes[i], pType =
parameters[i].getType();
if (!isAssignableTo(aType, pType)) {
return -1;
+ } else if
(Boolean.TRUE.equals(aType.getNodeMetaData(StaticTypesMarker.METHOD_REFERENCE_TYPE))
+ && CLOSURE_TYPE.getName().equals(pType.getName())) {
+ // a method reference (::) can never be a groovy.lang.Closure;
strongly disfavour a
+ // Closure parameter so a functional-interface overload is
preferred (otherwise the
+ // method reference resolves to the Closure overload and then
fails to coerce)
Review Comment:
The comment says a method reference can never be a groovy.lang.Closure, but
Groovy does allow method references to be assigned to Closure in other contexts
(see `testMethodRefAssignedToClosureType`). The behavior change here is about
overload preference; rewording avoids baking an incorrect semantic claim into
the code comment.
> STC: method reference should select the functional-interface overload over a
> Closure overload
> ---------------------------------------------------------------------------------------------
>
> Key: GROOVY-12214
> URL: https://issues.apache.org/jira/browse/GROOVY-12214
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> h3. Summary
> Under {{@CompileStatic}}, a method reference ({{::}}) argument fails to
> resolve to a functional-interface overload when a {{Closure}} overload of the
> same method also exists. Static overload selection binds the method reference
> to the {{Closure}} overload and then rejects it, because a method reference
> can never coerce to {{groovy.lang.Closure}}.
> This affects the "fat-free" DGM methods introduced in GROOVY-12054
> (functional-interface twins of Closure-taking methods, e.g.
> {{collect(Iterable, Function)}} beside {{collect(Iterable, Closure)}}): the
> ergonomic method-reference spelling does not compile under {{@CompileStatic}}.
> h3. Steps to reproduce
> {code:groovy}
> import groovy.transform.CompileStatic
> @CompileStatic
> List<String> upper(List<String> list) {
> list.collect(String::toUpperCase)
> }
> assert upper(['a', 'bb']) == ['A', 'BB']
> {code}
> Fails to compile with:
> {noformat}
> [Static type checking] - Argument is a method reference, but parameter type
> 'groovy.lang.Closure' is not a functional interface
> {noformat}
> The same failure occurs for any DGM method that has both a {{Closure}}
> overload and a functional-interface twin, e.g. {{[1, 2,
> 3].each(result::add)}}.
> h3. Notes
> * It works as expected *dynamically* (without {{@CompileStatic}}), and under
> {{@CompileStatic}} via an explicitly-typed functional value or a cast:
> {{collect(String::toUpperCase as Function)}}.
> * A closure block and a lambda literal ({{s -> s}}) correctly bind to the
> {{Closure}} overload; only method references are affected.
> * The {{each(result::add)}} example in GROOVY-12054's {{LambdasTest}} passes
> only because that test runs dynamically.
> h3. Root cause
> At overload-selection time a method reference presents as plain
> {{groovy.lang.Closure}} (the {{MethodPointerExpression}} default type;
> functors are visited *after* method selection). In
> {{StaticTypeCheckingSupport.allParametersAndArgumentsMatch}}, a {{Closure}}
> argument matching a {{Closure}} parameter is {{equals}} (distance 0), beating
> the {{Closure}}-to-SAM distance of 13, so the {{Closure}} overload wins; the
> deferred functor visit then rejects the method reference.
> h3. Fix
> Mark a method-reference argument's type with a new
> {{StaticTypesMarker.METHOD_REFERENCE_TYPE}} in {{getArgumentTypes}}, and in
> {{allParametersAndArgumentsMatch}} strongly disfavour a {{Closure}} parameter
> for such an argument so a functional-interface overload is preferred. Only
> {{::}} is marked; closures, {{->}} lambdas and {{.&}} method pointers are
> unaffected. No regression is possible: the affected pairing is a hard compile
> error today, so the only behaviour that changes is turning that error into
> correct selection of the functional-interface overload. When no
> functional-interface twin exists, the {{Closure}} overload remains the sole
> candidate and still reports the same clear error.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
