[
https://issues.apache.org/jira/browse/GROOVY-12616?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18136447#comment-18136447
]
ASF GitHub Bot commented on GROOVY-12616:
-----------------------------------------
daniellansun commented on code in PR #3164:
URL: https://github.com/apache/groovy/pull/3164#discussion_r4442696024
##########
COMPATIBILITY.md:
##########
@@ -901,6 +901,55 @@ of `CompilationUnit` that adds it as a phase operation, or
that calls
`PlaceholderVisitor.isPlaceholder`. Drop the phase operation. Where code needs
to know whether a variable is `_`, check its name.
+### Groovy 7 — values returned to a primitive functional interface type are
checked as a method's return (GROOVY-12616, GROOVY-12617, GROOVY-12623)
+
+Under `@TypeChecked` and `@CompileStatic`, a value that a closure, lambda,
+method reference (`::`) or method pointer (`.&`) returns to a functional
+interface whose method returns a primitive type is now checked as the `return`
+of a method declared with that type. A number may be widened, but not narrowed
+unless it is a constant that fits; `char` is not converted to or from a number
Review Comment:
“char is not converted to or from a number type” disagrees with the int→char
bullet just below, and with `checkCompatibleAssignmentTypes` (char, Character,
or int). Suggest: char is not converted to a numeric type; only int is
converted to char. Also, under “What is unchanged”, `Supplier<Byte> s = P::v`
for an `int` method is now “Possible loss of precision”, which master accepted
for a method reference. Worth an explicit sentence either way.
##########
src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesMethodReferenceExpressionWriter.java:
##########
@@ -725,20 +740,45 @@ private GStringAdaptation adaptGStringParameters(final
FunctionalInterfaceContex
call.setSourcePosition(source);
Statement invocation = impl.isVoidMethod() ? stmt(call) :
returnS(call);
- Statement body =
block(StaticInvocationWriter.gstringBridgePrologue(controller, samParams),
invocation);
+ Statement body = any ?
block(StaticInvocationWriter.gstringBridgePrologue(controller, samParams),
invocation) : invocation;
ClassNode[] exceptions = impl.getExceptions() == null ?
ClassNode.EMPTY_ARRAY : impl.getExceptions();
ClassNode classNode = controller.getClassNode();
MethodNode adapter = classNode.addSyntheticMethod(
- createSyntheticMethodName("gstring", impl.getDeclaringClass(),
impl.getName()),
+ createSyntheticMethodName(any ? "gstring" : "adapt",
impl.getDeclaringClass(), impl.getName()),
// the JVM rejects ACC_FINAL on any method of an interface (JVMS
4.6)
Opcodes.ACC_PRIVATE | Opcodes.ACC_STATIC |
(classNode.isInterface() ? 0 : Opcodes.ACC_FINAL),
- impl.getReturnType(),
+ convertReturn ? samReturnType : impl.getReturnType(),
adapterParams,
exceptions,
body);
adapter.putNodeMetaData(StaticCompilationMetadataKeys.STATIC_COMPILE_NODE,
Boolean.TRUE);
adapter.setSourcePosition(source);
- return new GStringAdaptation(adapter, samParams);
+ return new Adaptation(adapter, samParams);
+ }
+
+ /**
+ * Determines if a value of {@code returnType} must be converted to the
primitive return type
+ * of the functional interface by Groovy, because {@code
LambdaMetafactory} cannot convert it.
+ * For a declared return type, the metafactory unboxes and applies
widening primitive
+ * conversion. For a type parameter (erased to {@code Object}), it casts
to the wrapper type
+ * of {@code boolean} or {@code char}, or to {@code Number} for a numeric
type, and unboxes.
+ * Which conversions are allowed at all is decided by the type checker.
+ *
+ * @param returnType the declared return type, or the type argument for an
erased one
+ * @param erased whether the referenced method returns a type parameter
+ */
+ private static boolean isReturnConversionRequired(final ClassNode
returnType, final boolean erased, final ClassNode samReturnType) {
+ if (!ClassHelper.isPrimitiveType(samReturnType) ||
ClassHelper.isPrimitiveVoid(samReturnType)
+ || ClassHelper.isPrimitiveVoid(returnType)) {
+ return false;
+ }
+ ClassNode type = ClassHelper.getUnwrapper(returnType);
+ if (type.equals(samReturnType)) return false;
Review Comment:
This equality test is what keeps an erased `Boolean`→`boolean` and
`Character`→`char` off the adapter. The `erased` branch below returns true for
every boolean or char SAM; without this line those type arguments would be
Groovy-unboxed (`null` Boolean → false, `null` Character → `'\u0000'`) and
`testPrimitiveSAMFromTypeParameterReturnTypeNoConversion` would fail. A short
comment would stop the next edit from “simplifying” it away. I confirmed the
bytecode: `Predicate<Boolean>`, `ToLongFunction<Integer>`, and
`ToDoubleFunction<BigDecimal>` bind `C.id` directly; `Predicate<String>` uses
one `booleanUnbox` adapter.
##########
src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java:
##########
@@ -1330,6 +1330,8 @@ &&
findSAM(target).getName().equals(spec.get(0).getKeyExpression().getText())) {
inferParameterAndReturnTypesOfClosureOnRHS(target, lambda);
source.putNodeMetaData(PARAMETER_TYPE,
lambda.getNodeMetaData(PARAMETER_TYPE));
source.putNodeMetaData(CLOSURE_ARGUMENTS,
extractTypesFromParameters(lambda.getParameters()));
+ } else if (source instanceof MethodPointerExpression) { // as for
a method pointer argument
Review Comment:
`.&` only receives `PARAMETER_TYPE` here, so a method type parameter
witnessed by the SAM parameter stays a placeholder and `checkReturnValue` is
skipped (3977–3990). `ToIntFunction<Long> f = P.&id` and `new P().&me` compile
and narrow; `P::id` / `new P()::me` do not. The argument path (4727) has the
same split. Please attach the SAM parameter types for a method pointer as well,
and cover `H.&id` in `ReturnConversionConsistencyTest`. `xs.&first` on
`List<Long>` is already a precision error, via the receiver.
> STC rejects a closure or lambda that returns a primitive needing only a
> widening conversion to the SAM's primitive return type
> ------------------------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12616
> URL: https://issues.apache.org/jira/browse/GROOVY-12616
> Project: Groovy
> Issue Type: Bug
> Affects Versions: 4.0.33, 5.0.8, 6.0.0
> Reporter: Scott Murphy Heiberg
> Priority: Major
>
> {code:groovy}
> import java.util.function.ToLongFunction
> @groovy.transform.CompileStatic
> class C {
> static void main(args) {
> ToLongFunction<String> f = (String s) -> s.length()
> assert f.applyAsLong('abc') == 3L
> }
> }
> {code}
> Actual: {{[Static type checking] - Cannot return value of type int for lambda
> expecting long}}. The same happens under @TypeChecked, with a closure (passed
> for a ToLongFunction parameter, or a closure returning 42 assigned to a
> LongSupplier), for ToDoubleFunction from int, long or float, for char to int,
> and for Integer to long.
> Expected: compiles and runs, as in Java (JLS 5.2 allows a widening primitive
> conversion, optionally after unboxing, in an assignment context) and as with
> the method reference {{C::len}} or method pointer {{C.&len}}. Narrowing (long
> to int) should still be rejected.
> Cause: {{StaticTypeCheckingVisitor.checkReturnType}} accepts a closure or
> lambda return value only when the wrapper of the SAM return type is
> compatible with the wrapper of the value type, and Long is not compatible
> with Integer.
> Seen in 4.0.33, 5.0.8 and 6.0.0.
> PR: https://github.com/apache/groovy/pull/3142
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
