Skip to content

GROOVY-12492: Convert @CompileStatic closures to functional interfaces without a runtime proxy (incubating) - #3026

Open
codeconsole wants to merge 12 commits into
apache:masterfrom
codeconsole:sc-closure-sam-without-proxy
Open

codeconsole wants to merge 12 commits into
apache:masterfrom
codeconsole:sc-closure-sam-without-proxy

Conversation

@codeconsole

@codeconsole codeconsole commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Under @CompileStatic, a closure literal that goes to a functional interface is still converted at run time. That covers a literal passed to a single-abstract-method parameter, assigned to or returned as such a type, cast to one, or coerced with as.

ClosureWriter.writeClosure leaves the new instance typed as Closure on the operand stack. OperandStack.doConvertAndCast therefore emits an invokedynamic cast, or asType for as. The runtime then wraps the closure in a java.lang.reflect.Proxy with a ConvertedClosure handler (CachedSAMClass.coerceToSAM, TypeTransformers.createSAMTransform), whose every call goes through Closure.call(Object...) and a reflective dispatch to doCall.

@CompileStatic
int sum(int n) { IntStream.range(0, n).reduce(0) { int a, int b -> a + b } }

compiles to invokedynamic cast:(Lgroovy/lang/Closure;)Ljava/util/function/IntBinaryOperator;, although the same code written as a lambda uses LambdaMetafactory. In a GraalVM native image, each such interface must be registered as a proxy even though the code is statically compiled. Without that, the conversion fails with GroovyCastException. GROOVY-11998 already declares extra interfaces on closure classes for intersection casts, but only marker interfaces.

The option

This adds an incubating option, -Dgroovy.target.closure.sam=true (CompilerConfiguration.CLOSURE_SAM_CONVERSION), that makes the conversion at compile time. When the type checker recorded the interface a closure literal is converted to (the PARAMETER_TYPE metadata), StaticTypesClosureWriter gives the closure class an adapter factory and types the new instance as that class. Converting it to the interface then wraps it in <closure class>$SamAdapter, a class generated the first time a conversion needs it.

The adapter does what the proxy does:

  • it implements the interface and Serializable;
  • it is neither a Closure nor a GroovyObject;
  • its toString is the closure's;
  • it calls call(Object...) and converts the result with castToType, so Predicate still applies Groovy truth;
  • it unwraps a GroovyRuntimeException, and wraps a checked exception the method does not declare in an UndeclaredThrowableException.

When every argument already has doCall's parameter type, or is null for a reference type, and no parameter is an array, it calls doCall directly. That is exactly when call(Object...) would pass the arguments through unchanged, so the result is the same without reflection. When the interface binds a super-interface's type arguments, as interface Shout extends Function<String,String> does, the erased declarations get bridges with the same body.

Otherwise the adapter calls Closure.call, as the proxy does, and that finds doCall reflectively. In a native image the closure class's doCall methods then still need reflection metadata. That is always the case for an array or varargs parameter, and for a primitive parameter of the interface method that the closure declares as a different primitive, such as an int taken as a long. For an argument that reaches the closure only after a conversion, such as a GString that dynamic code or a raw Function passes for a String parameter, it depends on the value. The proxy took this path on every call, so nothing regresses.

What keeps the run-time conversion

Conversions to any other type are unaffected, so a closure whose value goes to Closure or Object stays a closure. These keep the run-time conversion, as without the option:

  • a trait or abstract class as the target;
  • a sealed interface, which a proxy cannot implement either, so the conversion fails at run time as it does without the option;
  • an interface that is not public, unless it is in the closure's package and compiled with the closure, so that one class loader defines both: a protected interface nested in a class of another package, or a package-private Java interface, keeps the run-time conversion;
  • an interface with a default method that one of the adapter's methods would override, which is the case for a Java interface that redeclares the method it inherits with the types it binds, since javac adds a default bridge method (a default method that only overloads the SAM's method is inherited, as the proxy inherits it);
  • with as, an interface without a single abstract method;
  • a closure chosen by a conditional or Elvis expression;
  • a closure that destructures its argument, such as { a, b -> } for a Consumer<List<String>>;
  • a closure given as a default parameter value, as an argument of an enum constant or of an anonymous class's constructor, or as a named constructor argument;
  • a closure that groovy.target.closure.pack packs;
  • a closure whose value is converted to another interface than the one the type checker recorded;
  • Runnable and Callable, which a closure already implements.

A sealed interface and a non-public interface are the cases where an adapter would change whether the program compiles or loads. An adapter for a sealed interface is not a permitted subclass, so the compilation would fail. An adapter for a package-private interface from another class loader would fail with IllegalAccessError when the class is defined. Without the option, only the conversion fails, at first use, and that stays so with it.

What changes with the option on

The class of the converted value is the visible difference, which is why the option is off by default, like groovy.target.closure.pack and groovy.target.lambda.hoist:

  • Proxy.isProxyClass is false, and Proxy.getInvocationHandler throws, so code that dug the closure out through ConvertedClosure can no longer do so;
  • class names in stack traces and exception messages differ;
  • equals and hashCode are identity rather than ConversionHandler's;
  • the serialized form names the adapter class, so values serialized with the option may not deserialize once that class changes;
  • on the direct path, a meta-class customization that intercepts doCall on the generated closure class is not consulted;
  • dynamic code can read the closure from the adapter's private field: f.closure returns it, where the proxy throws MissingPropertyException;
  • for an interface that binds a super-interface's type arguments, reflection finds the bridge as well, e.g. a public apply(String) beside apply(Object);
  • the adapter is a synthetic member class of the closure class, and each converted literal adds one.

The native-image section of the invokedynamic guide describes the option, these differences, when doCall still needs metadata, and the conversions that stay at run time.

Alternatives considered

  • Making the closure class implement the interface. I implemented this first. ClosureMetaClass only dispatches doCall and call, so dynamic callers of the interface method failed with MissingMethodException: [f]*.apply(x), InvokerHelper.invokeMethod(f, 'apply', x), f.&apply, f.with { apply(x) }. It also made the value a Closure, which changes case f:, grep(f) and f.andThen { }, and SAM methods named call, run or andThen collide with Closure's.
  • Compiling the closure as a lambda. The value would no longer be Serializable, toString would change, LambdaMetafactory's argument checkcast would stop a dynamic caller's GString from being coerced to String, and the body would need translating from closure semantics: implicit it, arity adaptation, default parameters, list destructuring, owner and delegate.

Tests

StaticCompileClosureSamTest has 31 tests. It checks the bytecode, which has no invokedynamic cast and no asType but a new …$SamAdapter before the call. It checks against today's behavior:

  • generic interfaces with inferred parameters;
  • GString coercion, null, and MissingMethodException for a wrong argument type;
  • primitives and boxing, including long and double slots;
  • array and varargs parameters;
  • implicit it, zero-argument Supplier and default parameters;
  • result conversion (Predicate, void, Comparator), default methods, default methods that overload the SAM's method, and bridged interfaces called through the erased method;
  • a literal used as a super-interface of its type, through a raw Function;
  • closures written in trait methods, including nested ones;
  • owner, delegate and thisObject, recursion, nested closures, captured-variable mutation;
  • serialization round trip;
  • equals and hashCode, which are identity, and a meta-class doCall interception, which the direct path does not consult;
  • a null closure, which the adapter's constructor rejects;
  • unchecked, declared and undeclared checked exceptions, and GroovyRuntimeException unwrapping;
  • dynamic callers: *., InvokerHelper, .&, with, andThen { }, grep;
  • SAM methods named call, run and andThen;
  • that no adapter class is generated without the option.

For the cases that keep the run-time conversion, it asserts that the value is a proxy: a protected nested interface of another package, a javac-compiled interface that redeclares its inherited method, as targets without a single abstract method, destructuring closures, default parameter values, enum constant, anonymous-class and named constructor arguments, the conditional expression, and closures under closure packing. For a sealed interface, declared in Groovy or compiled by javac, and for a package-private interface loaded by a parent class loader, it asserts that the class compiles and loads and that only the conversion fails, at run time, as without the option.

11,655 tests in the groovy, gls, bugs and STC/SC packages pass with the option off and with it on. The build forwards the option to the test JVM and to the compiler as it does closure.pack, so the option-on run also compiled the test sources with it. With the option off, the 8,508 class files compiled from Groovy's root test sources are byte-identical to master's. The Checkstyle and CodeNarc gates pass.

A native image (GraalVM CE 25.0.4) of a statically compiled program using IntStream.reduce, an IntBinaryOperator variable and a Function variable, with only the metadata the Groovy jar ships, fails at the conversion with GroovyCastException without the option and runs with it. In a rough JVM loop, two interface calls took about 26 ns through the proxy and 2.8 ns through the adapter.

Backport

Grails 9 builds with Groovy 6.0.x. Since the option is off by default, a backport to GROOVY_6_0_X would change nothing unless it is set, and would let Grails compile its statically compiled code without these proxies.

JIRA: GROOVY-12492

@codeconsole codeconsole changed the title Convert @CompileStatic closures to functional interfaces without a runtime proxy (incubating) GROOVY-12492: Convert @CompileStatic closures to functional interfaces without a runtime proxy (incubating) Oct 24, 2026
@codeconsole
codeconsole force-pushed the sc-closure-sam-without-proxy branch from 0ffd703 to 896ef4b Compare October 24, 2026 21:59
@codecov-commenter

codecov-commenter commented Oct 24, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.77273% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.7166%. Comparing base (5b077f9) to head (e27017a).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
...haus/groovy/classgen/asm/sc/SamAdapterFactory.java 89.7436% 5 Missing and 19 partials ⚠️
...groovy/classgen/asm/sc/StaticInvocationWriter.java 70.0000% 0 Missing and 3 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##               master      #3026        +/-   ##
==================================================
+ Coverage     73.6711%   73.7166%   +0.0456%     
- Complexity      42584      42649        +65     
==================================================
  Files            1678       1679         +1     
  Lines          146907     147173       +266     
  Branches        27790      27856        +66     
==================================================
+ Hits           108228     108491       +263     
+ Misses          28528      28519         -9     
- Partials        10151      10163        +12     
Files with missing lines Coverage Δ
...rg/codehaus/groovy/classgen/asm/ClosureWriter.java 90.1882% <100.0000%> (+0.1478%) ⬆️
...org/codehaus/groovy/classgen/asm/OperandStack.java 78.3784% <100.0000%> (+0.7386%) ⬆️
...oovy/classgen/asm/sc/StaticTypesClosureWriter.java 63.4146% <100.0000%> (+1.2298%) ⬆️
...codehaus/groovy/control/CompilerConfiguration.java 75.4190% <ø> (ø)
...vy/transform/sc/StaticCompilationMetadataKeys.java 100.0000% <100.0000%> (ø)
...groovy/classgen/asm/sc/StaticInvocationWriter.java 75.2712% <70.0000%> (-0.1713%) ⬇️
...haus/groovy/classgen/asm/sc/SamAdapterFactory.java 89.7436% <89.7436%> (ø)

... and 18 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@daniellansun daniellansun left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling GROOVY-12492. Under @CompileStatic, turning a closure literal that the type checker already bound to a SAM into a small compile-time adapter instead of an invokedynamic cast / asType → Proxy+ConvertedClosure path is a coherent incubating direction, and it matches the native-image pain the current design creates. The split between SamAdapterFactory.prepare (attach a factory on the generated closure class), StaticTypesClosureWriter.getClosureInstanceType (keep a precise stack type), and the early hook in OperandStack.doConvertAndCast is easy to follow, and StaticCompileClosureSamTest is unusually thorough for an incubating switch.

I am leaving a COMMENT (not an approval): the happy paths look carefully engineered, but a few correctness / contract edges should be tightened before this lands on master, especially where the docs and the code disagree, and where the adapter intentionally diverges from ConversionHandler without tests locking that divergence.

Observations / concerns

  1. Trait-context claim vs prepare(). The docs (invokedynamic-support.adoc) and the factory javadoc state that a closure written in a trait method keeps the run-time conversion. SamAdapterFactory.prepare only declines when the target type (or one of its super-interfaces) is a trait (Traits.isTrait(face)), or when a same-name/arity default method exists, etc. It does not consult enclosing trait context the way ClosureWriter.isInTraitContext() does for packing. Trait helper classes often still go through StaticTypesClosureWriter when the method is statically checked (StaticTypesWriterController#getClosureWriter). As written, a @CompileStatic closure literal inside a trait method that STC annotates with PARAMETER_TYPE would still get a $SamAdapter. Please either:

    • decline in prepare when isInTraitContext()-equivalent is true, and add a regression test; or
    • drop/soften the “closure written in a trait method” claim and add a test that shows the adapter is used there.
      Right now the stated contract and the implementation can disagree.
  2. equals / hashCode divergence is user-visible and untested. ConversionHandler defines delegate-based equals/hashCode (and Proxy.equals routes into the handler). $SamAdapter inherits identity semantics from Object. That is called out as an intentional difference, which is fine for an opt-in incubating flag, but StaticCompileClosureSamTest never asserts it. Please add an explicit test that two adapters wrapping closures that would compare equal under today’s proxy do not compare equal (and that hashCode is not the delegate’s), so the contract cannot drift silently if someone later “helpfully” mirrors ConversionHandler.

  3. Direct doCall path vs Closure.call / MetaClass. The fast path in AdapterMethod.writeDirectCall invokes doCall when every argument is already the parameter type (or null for a reference) and no parameter is an array. That matches “what call(Object...) would pass through unchanged” for the default ClosureMetaClass, but it skips any MetaClass customization that intercepts doCall on the generated closure class, and it also skips arity/default/InvokerHelper logic that only lives on call. The suite covers wrong-type → MissingMethodException via the checked fallback, which is good; please also state in the $SamAdapter / option javadoc (not only the PR text) that MetaClass interception of doCall is out of scope on the direct path. A one-line test with an ExpandoMetaClass / metaClass.doCall = … on the closure class would make the boundary obvious for future readers.

  4. writeReturn and primitive doCall results. AdapterMethod.writeReturn emits DefaultTypeTransformation.castToType(Object, Class) when the SAM return type is a reference and isAssignable(valueType, returnType) is false. castToType expects a reference on the stack. If STC ever leaves doCall with a primitive return type while the SAM method returns a boxed/reference type, the direct path would push a primitive into castToType and fail verification (or need an implicit box that is not there). Today’s tests (primitiveParameter, boxedParameter, longResult, …) appear to land on shapes that work; I am not asking for a speculative rewrite, but a guard (box before castToType when valueType is primitive) or an assert in writeDirectCall that the doCall return is a reference whenever the SAM return is would make this robust against future STC typing tweaks.

  5. Packing interaction is documented but only weakly locked. writePackedClosure never builds a per-literal closure class, so SamAdapterFactory.prepare does not run and conversion stays at run time — that matches the adoc. withClosurePacking only checks functional results with both flags on; it does not assert Proxy.isProxyClass (or the absence of $SamAdapter). Given how easy it is for packing eligibility to change, a single assertion that a packed SAM conversion remains a proxy (or, if packing declines for that literal, remains an adapter) would pin the intended interplay.

  6. Shared OperandStack hook and stack typing. Routing through OperandStack.doConvertAndCast before the coerce / indy-cast branches is the right place to catch both casts and as, and the early ClassNodeUtils.isCompatibleWith(top, targetType) return correctly avoids wrapping when the value is already a Closure/Object. Two softer notes:

    • This introduces a transform.sc.StaticCompilationMetadataKeys dependency into shared classgen.asm.OperandStack (dynamic compilation stays a no-op because the metadata is absent). Acceptable given ClosureWriter already touches SC keys, but worth being conscious of as more SC policy lands in shared operand handling.
    • getClosureInstanceType changing the post-writeClosure stack type from CLOSURE_TYPE to the concrete closure class is necessary for the metadata lookup; please double-check any codegen that pattern-matches CLOSURE_TYPE specifically on the stack after a closure literal (as opposed to isDerivedFrom(Closure)). I did not spot a break, but it is a behavioral surface beyond the adapter itself.
  7. Default-method exclusion is conservative — confirm intentional breadth. prepare declines the whole adapter if any interface in type.getAllInterfaces() declares a default method with the SAM’s name and arity (see Overloaded in the test). That correctly avoids overriding Function-style defaults you meant to keep, but it also declines when an unrelated super-interface happens to carry a same-name default. If that is deliberate (prefer proxy over a wrong override), a one-sentence comment in prepare would help; if not, narrowing the check to defaults that would actually be overridden by the adapter method body would recover more native-image-friendly cases.

  8. Serialization contract. Generating Serializable adapters without a serialVersionUID, with a synthetic class name tied to the closure class, matches “serialized form names the adapter class.” The round-trip test with a custom resolveClass is appreciated. For incubating this is fine; just be aware that any later rename ($SamAdapter → something else) or nesting change is a serialization break for anyone who opted in and persisted values — worth a short note next to CLOSURE_SAM_CONVERSION.

  9. Bridge methods copy the full body. adapterMethods emits a primary parameterized method plus erased bridges, each with a full AdapterMethod (try/catch, direct/call paths). That guarantees every descriptor reaches the closure the same way, at the cost of bytecode size and duplicated exception logic. Delegating bridges (apply(Object) { return apply((String) …); }) would be more conventional; not blocking, but if adapter count grows under a flag-on bootstrap (the 203 adapters mentioned in the PR text), size may matter.

  10. Performance / native-image claims vs evidence in-repo. The ~26 ns vs ~2.8 ns proxy-vs-adapter figure and the GraalVM CE 25 smoke are useful motivation but are not reproduced by the test suite or a checked-in microbenchmark. For an incubating option that is fine; I would avoid implying measured production gains in the adoc beyond “avoids reflective proxies / Graal proxy entries.” The bytecode assertions (no INVOKEDYNAMIC cast, NEW …$SamAdapter) are the right in-repo evidence.

  11. Flag plumbing. -Dgroovy.target.closure.sam via SystemUtil.getBooleanSafe, @Incubating on CompilerConfiguration.CLOSURE_SAM_CONVERSION, and forwarding in org.apache.groovy-tested.gradle mirror closure.pack / lambda.hoist. Good. Please confirm (or document) that there is no CompilerConfiguration instance field / joint-compile API on purpose, so embedders know the only knob is the process-wide property (the test’s withSam toggle already assumes that).

Test gaps I would still like to see

  • Trait-method closure + SAM target (item 1), with an assert on proxy vs $SamAdapter.
  • Explicit equals/hashCode divergence (item 2).
  • Optional: non-public SAM type in another package (isAccessible → fall back to proxy).
  • Optional: assignment/cast to a super-interface of PARAMETER_TYPE (e.g. Shout literal used as raw Function) to lock SamAdapterFactory.apply + ClassNodeUtils.isCompatibleWith(type, target).
  • Optional: MetaClass doCall intercept demonstrating the direct-path boundary (item 3).

Non-blocking nits

  • SamAdapterFactory is package-private final with solid javadoc — thank you; a short @since / JIRA pointer on CLOSURE_SAM_CONVERSION would match sibling incubating constants.
  • Constructor of $SamAdapter does not null-check closure (ConversionHandler rejects null). Unreachable from the emitted NEW/INVOKESPECIAL sequence; only matters if something else constructs the adapter later.
  • withSam mutates global system properties around GroovyShell / compile — fine for this suite, but it is not safe for parallel test execution if the module ever runs concurrent classgen tests with different flag values.

Stance

Directionally I am in favour: opt-in, off by default, SC-only, with a clear escape back to the proxy for traits-as-targets, abstract classes, Runnable/Callable, conditionals, and packing. The main blockers from a review stance are aligning the trait-context contract with the code, locking the equals/hashCode behavioural break in tests, and hardening or documenting the direct-doCall / writeReturn edges. I am not approving yet; happy to re-review once those are addressed.

@codeconsole

Copy link
Copy Markdown
Contributor Author

Pushed f904db9. The review was written against 896ef4b; 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 164db28: 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 447ded7: 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. 2e1b07d states it in the CLOSURE_SAM_CONVERSION, SamAdapterFactory and AdapterMethod Javadoc (the last one claimed "the same result"). 447ded7 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 1c0fea7: 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 04b3c80: 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 164db28.
  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 2550df0: 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 2e1b07d, 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. 2e1b07d 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 (164db28).
  • equals/hashCode: item 2.
  • Non-public SAM type in another package: already covered by protectedInterfaceFromAnotherPackageKeepsItsConversion (04b3c80) and nonPublicInterfaceFromAnotherClassLoaderKeepsItsConversion (16d9622).
  • Super-interface of the literal's type: literalOfASubInterfaceUsedAsItsSuperInterface (447ded7): 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 2e1b07d. No JIRA pointer: CONTRIBUTING.md asks to keep JIRA keys out of Javadoc.
  • Fixed in b2ea0c0: the constructor rejects null (adapterRejectsANullClosure).
  • 019c000 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):

  • cfe53ed: 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).
  • b88824b, 7819d1d: 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).
  • 2ebc8ec: 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).
  • cbe0b9e: 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).
  • f904db9: 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.

@daniellansun daniellansun left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the extensive follow-up on tip f904db9. I re-checked SamAdapterFactory / AdapterMethod and your replies against my earlier COMMENT on 896ef4b.

Prior asks — closed on the incubating path

  1. Trait context. The guide no longer claims trait-method closures keep the proxy. Rehydrate now preserves the closure class type, so closures written in traits get adapters with proxy-equivalent behaviour (closuresWrittenInATraitGetTheirAdapter). That resolves the docs/code disagreement — by using the adapter, not by declining.

  2. equals / hashCode. equalsAndHashCodeAreThoseOfObject locks identity semantics vs ConversionHandler / Proxy.

  3. Direct doCall / MetaClass. Javadoc on CLOSURE_SAM_CONVERSION, SamAdapterFactory, and AdapterMethod states the boundary; metaClassInterceptingDoCallIsNotConsultedByTheDirectCall shows String bypasses intercepting doCall while GString still goes through call().

  4. writeReturn. Boxing primitives / null for void before castToType is the right hardening even if today’s doCall returns wrappers.

  5. Packing. withClosurePacking already asserts proxy vs adapter per literal — acknowledged.

6–8. Stack typing check, narrowed default-method exclusion (only erased-descriptor clashes), serialization note — all addressed.

9–11. Full bridges kept for GString-through-raw-Function (good reason); guide avoids performance claims; system-property-only knob documented like closure.pack / lambda.hoist. Null-reject ctor and @ResourceLock(SYSTEM_PROPERTIES) are welcome.

The trait/closure/lambda correctness fixes found while probing (owner/thisObject typing, lambda owner in traits, this.m(), intersection-typed closure calls, static closure fields on traits) look legitimate and well tested. Thank you for separating “option on” behaviour from “option off bytecode identical except the shapes those fixes change.”

Why I am not approving yet

  1. Merge conflicts. Tip is dirty against current master. Please rebase before any merge consideration.

  2. Scope. The incubating SAM-adapter option is a coherent, off-by-default story. The same PR also carries spaceship / instanceof-narrowing / SAM-via-type-parameter / trait lambda / ClassInfo-adjacent fixes. Those are good, but they enlarge review and backport surface. Prefer either:

    • land the orthogonal STC/SC/trait fixes under their own JIRA(s) first (or as clearly labelled commits that can cherry-pick), then the incubating option; or
    • keep them here but call out in the PR description which commits are “required for the option” vs “bugs found while testing,” so release notes and 6.0.x backport decisions stay honest.
  3. Incubating contract reminder (non-blocking). Process-wide -Dgroovy.target.closure.sam with no CompilerConfiguration field is intentional; embedders need the Javadoc. Serialized adapters and identity equals remain the user-visible differences — already documented.

Stance

COMMENT, not approval: conflicts must clear, and I would like a slightly sharper split (or explicit labelling) between the incubating SAM adapter and the orthogonal correctness fixes before I approve. The incubating design itself looks mergeable once the tip is green and conflict-free. Happy to re-review promptly after that.

…t a runtime proxy

Under @CompileStatic a closure literal that is passed to a parameter of a
single-abstract-method type, assigned to or returned as such a type, cast
to it or coerced with "as" is compiled to a Closure subclass and converted
at run time. ClosureWriter.writeClosure leaves the instance typed as
Closure on the operand stack, so OperandStack.doConvertAndCast emits an
invokedynamic cast, or asType for "as", and the runtime wraps the closure
in a java.lang.reflect.Proxy with a ConvertedClosure handler
(CachedSAMClass.coerceToSAM, TypeTransformers.createSAMTransform). Every
call then goes through the proxy and Closure.call(Object...), which finds
doCall reflectively. In a GraalVM native image each such interface has to
be registered as a proxy although the code is statically compiled.

The new incubating option -Dgroovy.target.closure.sam=true
(CompilerConfiguration.CLOSURE_SAM_CONVERSION) makes the conversion at
compile time. When the type checker recorded that a closure literal is
converted to an interface (the PARAMETER_TYPE metadata),
StaticTypesClosureWriter stores a SamAdapterFactory on the generated
closure class and types the new instance as that class. Converting it to
the interface in OperandStack.doConvertAndCast then wraps it in an adapter,
a final class generated as a nested class of the closure class the first
time a conversion needs it. Like the proxy, the adapter implements the
interface and Serializable, is neither a Closure nor a GroovyObject, and
its toString is the closure's. Its method does what ConvertedClosure does:
it calls Closure.call(Object...), converts the result with
DefaultTypeTransformation.castToType, unwraps a GroovyRuntimeException and
wraps a checked exception the method does not declare in an
UndeclaredThrowableException, as the proxy does. Where call(Object...)
would hand every argument to doCall unchanged (each one already an
instance of the parameter type, or null for a reference type, and no
parameter an array), it calls doCall directly, so the common case needs
no reflection. When the interface binds the type arguments of its
super-interface, as an interface extending Consumer<Event> does, the
method is declared with the bound types and each erased declaration gets
a bridge with the same body, so a caller of the erased method sees what
the proxy would have done. Conversions to other types are unaffected, so
a closure whose recorded target is not where its value goes stays a
closure.

The converted value therefore behaves as the proxy did, including for
dynamic callers, and its class is the visible difference. Since that and
the generated code change, the option is off by default, like
groovy.target.closure.pack and groovy.target.lambda.hoist, and the build
forwards it to the test JVM and to the compiler as it does those. A trait
or abstract class as the target, an interface with a default method of
the same name and arity, a closure in a trait method, a closure chosen by
a conditional or Elvis expression, and a closure that packing packs keep
the run-time conversion. The native-image section of the
invokedynamic guide describes the option.
With groovy.target.closure.sam, a closure literal converted to a sealed
functional interface got an adapter class implementing that interface.
The adapter is not one of the permitted subclasses, so the class
completion check rejected it and the whole compilation unit failed to
compile, naming a class the user never wrote.

Without the option such a program compiles, and only the conversion
fails at run time, because a proxy cannot implement a sealed interface
either. A sealed interface now keeps the run-time conversion, so the
option no longer changes whether the program compiles. This holds for a
sealed interface declared in Groovy and for one compiled by javac.
… the closure

The adapter of a closure converted to an interface that is not public
was generated whenever the interface was in the closure's package. The
package name alone does not make the access legal: a run-time package is
a package name and a class loader. A package-private interface loaded by
a parent class loader, and a statically compiled class of the same
package compiled by a child GroovyClassLoader, are in different run-time
packages. The adapter then failed with IllegalAccessError when the
GroovyClassLoader defined it, eagerly, so parseClass itself failed and
no member of the class could be used.

Without the option only the conversion fails, at first use. A type the
adapter names that is not public must now also be compiled with the
closure, which guarantees the same class loader; any other keeps the
run-time conversion, so the option no longer turns that failure into a
class definition failure. This covers the interface and the types of its
method, which the adapter names in its bytecode as well.
Several tests named a case that keeps the run-time conversion but only
checked its result, which is the same either way: the destructuring
closures, the closure chosen by a conditional expression, and the
closures under groovy.target.closure.pack. They now assert that the
value is a proxy. Under packing, the literal returned as it is escapes
its method and is not packed, so it gets an adapter, and the test checks
that too.

New tests pin the other cases that keep the run-time conversion: a
default parameter value, an argument of an enum constant (an adapter
when written with as), an argument of an anonymous class constructor, a
named constructor argument, a protected nested interface from another
package, a javac-compiled interface that redeclares the method it
inherits (javac's bridge is a default method), and as-targets that have
no single abstract method.

disabledByDefault also checks that no adapter class is generated
without the option, against the six generated with it.
…time

The guide said the adapter behaves as the proxy would apart from being a
different class, and the CLOSURE_SAM_CONVERSION Javadoc that it behaves
as the proxy does. It now lists what code can observe: equals and
hashCode are identity, a metaclass intercepting doCall is not consulted
on the direct path, the serialized form differs, dynamic code can read
the closure from the private closure field, an interface binding a
super-interface's type arguments gets an extra public method, and the
adapter is a synthetic member class.

It also says when the call still needs reflection. The adapter calls
doCall directly only when every argument already has the parameter's
type; otherwise Closure.call finds doCall reflectively, so a native image
needs doCall registered. That is always so for an array or varargs
parameter and for a primitive the closure takes as another primitive,
and for a converted argument such as a GString it depends on the value.

The list of conversions that still happen at run time is now complete:
sealed interfaces, non-public interfaces Groovy does not compile with
the closure, Java interfaces redeclaring the inherited method, as-targets
without a single abstract method, destructuring closures, default
parameter values, enum constant and anonymous class constructor
arguments, named constructor arguments, and a closure whose value goes
to another interface than the recorded one, besides the cases listed
before.
The guide listed a closure written in a trait among the conversions that
stay at run time, but nothing in SamAdapterFactory.prepare said so, and
the code did not agree with itself: a closure written directly in a
trait method kept the proxy, while one nested in another closure or in
a lambda of the same method got an adapter.

The difference came from the trait transformation. After type checking,
a closure written directly in a trait method is replaced by
closure.rehydrate($self, $self, $self), whose result is typed Closure,
so the conversion of the copy found no adapter factory on the operand
stack and fell back to the run-time cast. A nested closure is not
rehydrated and was adapted.

rehydrate returns a clone, an instance of the closure's own class, so
the adapter can wrap the copy as it wraps any other instance. When the
receiver of rehydrate is a closure literal whose class has an adapter
factory, StaticInvocationWriter now casts the result back to that class
and types it as such, and the conversion that follows uses the adapter.
The rehydrated copy keeps its owner, delegate and this object, so the
closure behaves as it did through the proxy. A closure written in a
trait, at any depth, therefore gets its adapter like any other, and the
guide no longer lists it as an exception. Without the option the
closure class has no factory and nothing changes.
The adapter's direct path passes the value doCall returns to
writeReturn, which calls castToType, or returns it, as a reference.
ClosureWriter.createClosureClass declares doCall with the wrapper of a
primitive inferred return type, and with Void's for void, so no
generated closure reaches writeReturn with a primitive. A doCall that
did return one would have produced code the verifier rejects ("Type
integer is not assignable to reference type" for an int returned
through Function.apply).

writeReturn now boxes a primitive value first, and pushes null for a
void one, which is what Closure.call returns for them. The code
generated for the closures the compiler creates is unchanged.
…override

SamAdapterFactory.prepare kept the run-time conversion for any interface
with a default method of the SAM's name and number of parameters. Its
Javadoc gives the reason as a default method the adapter's method would
override, but a default with another descriptor is not overridden: it
overloads the abstract method, and the adapter inherits it as the proxy
does. So an interface such as

    interface Overloaded extends Function<String, String> {
        default String apply(Integer i) { 'int' }
    }

or one that inherits an unrelated same-name default from another
interface kept the proxy for no reason.

prepare now declines only when a default method has the erased
descriptor of one of the methods the adapter declares, the interface
method with its bound types or one of the bridges, which is the case of
the default bridge javac adds to a Java interface that redeclares the
method it inherits. Calls to the overloading default, statically and
from dynamic code, including with a null or GString argument, give the
same results as through the proxy. The guide's list of conversions that
stay at run time says which default methods keep the proxy.
The compiler only constructs an adapter around the closure instance it
has just created, so the closure is never null there. The constructor
is public, though, and an adapter created reflectively around null
failed only when its interface method was called, with a
NullPointerException from the field access. ConversionHandler rejects a
null delegate when the proxy is created. The constructor now calls
Objects.requireNonNull on its argument, so such an adapter cannot be
created.
…e adapter

The guide listed how the adapter differs from the proxy, but the
CLOSURE_SAM_CONVERSION and SamAdapterFactory Javadoc did not say so, and
AdapterMethod's Javadoc claimed the direct call of doCall gives "the
same result", which is not the case when a metaclass intercepts doCall
on the closure class: Closure.call dispatches through the metaclass,
the direct call does not.

CLOSURE_SAM_CONVERSION now states that, and that a converted value
serializes as an instance of the generated class, so values serialized
with the option may not deserialize once that class changes; the guide
says the same of the serialized form. It also says that, like
CLOSURE_PACKING, the option is read from the system properties of the
compiling JVM whenever a closure is compiled, with no CompilerConfiguration
setting, and carries @SInCE 7.0.0 as the incubating ERROR_RECOVERY does.
The SamAdapterFactory and AdapterMethod Javadoc state the metaclass
boundary and the identity equals and hashCode.
… use of the adapter

Three behaviours of the adapter that the guide describes had no test:

- equals and hashCode are those of Object. Two proxies of one closure
  are equal and hash as the closure; an adapter is equal only to itself,
  also against a second adapter or a proxy of the same closure, and its
  hash code is its identity hash.
- a metaclass that intercepts doCall on the closure class is not
  consulted when the adapter calls doCall directly, for a String
  argument, and is consulted when the adapter goes through Closure.call,
  for a GString; the proxy consults it for both.
- a literal converted to a sub-interface such as Shout, which binds the
  type arguments of Function, and used as a raw Function, assigned,
  coerced with as or passed, is a Shout and converts a GString for the
  erased apply(Object) as the proxy does.
The test toggles groovy.target.closure.sam, and for one test
groovy.target.closure.pack, as system properties around each
compilation, since the compiler reads them per closure. A test running
concurrently in the same JVM could see the wrong value. The class now
declares @ResourceLock(Resources.SYSTEM_PROPERTIES), as
PackedDispatcherFactoryTest and AotLinkModeTest do, so JUnit does not
run it in parallel with other tests that use the system properties.
The class Javadoc no longer says the adapter has exactly the proxy's
behavior.
@codeconsole
codeconsole force-pushed the sc-closure-sam-without-proxy branch from f904db9 to e27017a Compare October 30, 2026 03:06
@codeconsole

Copy link
Copy Markdown
Contributor Author

@daniellansun Thanks. I've split it as you suggested. This PR is now only the incubating option: the adapter, its follow-ups, and the trait rehydrate change that lets a closure written in a trait method get its adapter, which does nothing without the option. It is rebased on current master, so the conflicts are gone. Its 31 tests pass without any of the fixes below, so nothing has to land first. With the option off, Groovy's 8,508 root test classes compile to the same bytes as on master.

The fixes found while testing it are now separate:

Each is based on master and has a test that runs without the option. The tests here that combined the option with one of those shapes (a closure for a type parameter or assigned to a property, trait closure owner/thisObject/this.m(), a closure of intersection type, a static closure field of a trait) are left out for now. I'll add them back here, or in whichever of the two PRs merges second.

@testlens-app

testlens-app Bot commented Oct 30, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: e27017a
▶️ Tests: 125124 executed
⚪️ Checks: 26/26 completed


Learn more about TestLens at testlens.app/docs.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants