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