[
https://issues.apache.org/jira/browse/GROOVY-12492?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18131915#comment-18131915
]
ASF GitHub Bot commented on GROOVY-12492:
-----------------------------------------
codeconsole commented on PR #3026:
URL: https://github.com/apache/groovy/pull/3026#issuecomment-6401358069
Pushed f904db9268. The review was written against 896ef4b31f; replies
describe the current head, in the order of the review:
1. The guide's claim did not match the code in either direction. A closure
written directly in a trait method kept the proxy only because the trait
transformation replaces it with `closure.rehydrate($self, $self, $self)`, which
is typed `Closure`; a closure nested in it, or in a lambda, already got an
adapter. Fixed in 164db287b6: the rehydrated copy, an instance of the closure
class, keeps that type, so every closure in a trait gets its adapter with the
proxy's behaviour, and the guide no longer lists traits
(`closuresWrittenInATraitGetTheirAdapter`). Probing this turned up five bugs in
closures and lambdas of trait methods that exist on master with or without the
option; they are fixed below.
2. Test added in 447ded735a: `equalsAndHashCodeAreThoseOfObject`. Two
proxies of one closure are equal; two adapters of one closure, or an adapter
and a proxy of it, are not; the adapter's hash is its identity hash.
3. 2e1b07d0ed states it in the `CLOSURE_SAM_CONVERSION`, `SamAdapterFactory`
and `AdapterMethod` Javadoc (the last one claimed "the same result").
447ded735a adds `metaClassInterceptingDoCallIsNotConsultedByTheDirectCall`: a
String argument bypasses an intercepting `metaClass.doCall`, a GString goes
through it, and the proxy uses it for both. The direct path is taken only at
`doCall`'s exact arity and without array parameters; defaults and destructuring
go through `Closure.call` (`implicitParameterDefaultValuesAndDestructuring`,
`arrayParameters`).
4. Not reachable today: `ClosureWriter.createClosureClass` declares `doCall`
with the wrapper of a primitive (or `Void`) inferred return type. Hardened
anyway in 1c0fea7385: `writeReturn` boxes a primitive and pushes null for void
before `castToType`, as `Closure.call` returns them. A probe driving
`AdapterMethod` with `int`, `long` and `void` `doCall` methods failed
verification before and verifies now.
5. Already the case since 04b3c80c34: `withClosurePacking` asserts which
literals are proxies (the packed ones) and which one is an adapter (the
returned literal, which packing declines).
6. Checked. With the option off the stack type stays `Closure`. With it on,
the code that reads the top operand after a closure literal either tests
`isDerivedFrom` or compatibility (`StaticInvocationWriter`,
`BinaryExpressionTransformer`, `CheckcastReceiverExpression`,
`doConvertAndCast`), adds at most a redundant CHECKCAST (`InvocationWriter`,
`storeVar`), or uses the type in a descriptor, where the public closure class
is valid. The only consumer that lost the precise type was the trait
`rehydrate` call, fixed in 164db287b6.
7. The breadth was not deliberate: a default method with another descriptor
is an overload that the adapter inherits, as the proxy does. Narrowed in
2550df0a12: `prepare` declines only for a default with the erased descriptor of
an adapter method (javac's bridge in a Java interface that redeclares its
method). `defaultMethodsOverloadingTheInterfaceMethodAreInherited` shows
overloads keep the proxy's results with the adapter.
8. Noted in 2e1b07d0ed, in the `CLOSURE_SAM_CONVERSION` Javadoc and the
guide.
9. Kept: a delegating bridge has to cast to the bound type, so a raw
`Function.apply(GString)` would throw `ClassCastException` where the proxy
converts the GString (`defaultMethodsAndUserInterfaces`). A full bridge costs
about 70 bytes of code.
10. The guide makes no performance claim, only that such a conversion needs
no proxy entry; the timings are only in the PR description, labelled as a rough
loop.
11. Deliberate, as for `closure.pack` and `lambda.hoist`. 2e1b07d0ed says so
in the `CLOSURE_SAM_CONVERSION` Javadoc: the option is read from the compiling
JVM's system properties, with no `CompilerConfiguration` setting.
Test gaps:
- Trait-method closure: `closuresWrittenInATraitGetTheirAdapter`
(164db287b6).
- `equals`/`hashCode`: item 2.
- Non-public SAM type in another package: already covered by
`protectedInterfaceFromAnotherPackageKeepsItsConversion` (04b3c80c34) and
`nonPublicInterfaceFromAnotherClassLoaderKeepsItsConversion` (16d9622864).
- Super-interface of the literal's type:
`literalOfASubInterfaceUsedAsItsSuperInterface` (447ded735a): a `Shout` literal
assigned, coerced and passed as a raw `Function`, called through the erased
method with a String and a GString.
- MetaClass `doCall` intercept: item 3.
Nits:
- `@since 7.0.0` added in 2e1b07d0ed. No JIRA pointer: CONTRIBUTING.md asks
to keep JIRA keys out of Javadoc.
- Fixed in b2ea0c0bdc: the constructor rejects null
(`adapterRejectsANullClosure`).
- 019c000b6d adds `@ResourceLock(Resources.SYSTEM_PROPERTIES)`, as the other
tests that toggle compiler system properties do.
Closures and lambdas in trait methods, fixed with tests that fail without
each change (each also run with the option on, so the adapter works for the
shape):
- cfe53ed76e: `owner`, `thisObject`, `delegate`, `getOwner()` and
`getThisObject()` in a closure of a `@CompileStatic` or `@TypeChecked` trait
method threw `GroovyCastException`; they were typed as the helper class while
the closure is rehydrated to `$self`. They are now typed as the trait
(`testOwnerAndThisObjectOfTraitClosure`).
- b88824b32d, 7819d1d1d3: under `@CompileStatic`, a lambda in a trait method
was created with the helper class as owner and this object, so `thisObject` or
an implicit method call in it, or in a closure inside it, threw
`ClassCastException`. A lambda that uses an implicit this now gets `$self`. In
any class, a closure in a lambda that reads only `owner` got the lambda's class
as owner; that lambda now captures (`testClosureInTraitLambda`,
`testOwnerOfClosureInLambda`).
- 2ebc8ecdc5: `this.m()` in a trait closure or lambda called the static
helper class in all three modes: `ClassCastException` under `@CompileStatic`,
`MissingMethodException` for methods with arguments, `@SelfType` or abstract
methods, and the helper's `toString()` and `getClass()`. It now calls `m` on
`$self` (`testExplicitThisMethodCallInTraitClosure`).
- cbe0b9ea71: calling a closure typed `Closure<? extends A & B>` was typed
as the wildcard placeholder, so an enclosing closure got a `doCall` returning
class `?` and failed with `NoClassDefFoundError`. The call is now typed with
the bounds (`testCallOfClosureReturningIntersectionType`).
- f904db9268: under `@CompileStatic`, `f()` on a static closure field of a
trait read the field through the trait's static accessor and threw
`MissingMethodException`; it now reads the field of the implementing class
(`testStaticClosureFieldCalledAsMethod`).
With the option off, compiling 1,670 sources from the trait, closure and
lambda tests three ways (6,813 class files) gives the same bytes as before
these commits, except for three classes that have the shapes fixed above.
> Static compilation converts closure literals to functional interfaces through
> a runtime proxy
> ---------------------------------------------------------------------------------------------
>
> Key: GROOVY-12492
> URL: https://issues.apache.org/jira/browse/GROOVY-12492
> Project: Groovy
> Issue Type: Improvement
> Components: Static compilation
> Affects Versions: 6.0.0
> Reporter: Scott Murphy Heiberg
> Priority: Major
>
> Under {{@CompileStatic}}, a closure literal whose target is a functional
> interface is still compiled to a {{Closure}} subclass and converted at run
> time. That covers a literal passed to a SAM parameter, assigned or returned
> as a SAM type, cast with {{(SAM)}}, or coerced with {{as SAM}}. For example:
> {code:groovy}
> @CompileStatic
> int sum(int n) { IntStream.range(0, n).reduce(0) { int a, int b -> a + b } }
> {code}
> compiles to {{invokedynamic
> cast:(Lgroovy/lang/Closure;)Ljava/util/function/IntBinaryOperator;}}, and the
> same closure coerced with {{as IntBinaryOperator}} to an {{asType}} call. At
> run time, {{CachedSAMClass.coerceToSAM}} /
> {{TypeTransformers.createSAMTransform}} create a {{java.lang.reflect.Proxy}}
> with a {{ConvertedClosure}} handler, and each call goes through the proxy and
> {{Closure.call(Object...)}}. The same code written as a lambda uses
> {{LambdaMetafactory}}.
> Consequences: the proxy and reflective dispatch cost on every call, and in a
> GraalVM native image each interface must be registered as a proxy, otherwise
> the statically compiled conversion fails with {{GroovyCastException}}.
> Proposal: an incubating {{groovy.target.closure.sam}} compiler option, off by
> default, that converts such literals at compile time with a generated adapter
> class that behaves as the proxy does, apart from the differences the
> documentation lists, such as the class, {{equals}} and {{hashCode}}, and the
> serialized form.
> PR: https://github.com/apache/groovy/pull/3026
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
