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