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.
--
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]
