paulk-asert commented on PR #2714: URL: https://github.com/apache/groovy/pull/2714#issuecomment-6734171808
@blackdrag — classic and strict do exclude each other: `openOver` is package-private, and its only caller, `OpenClosure.create`, always passes `strict=false`, so an open closure's resolver replaces the strict guard rather than coexisting with it. I've moved the strict check first in both `setDelegate` and `setResolveStrategy`, so a rejected call changes nothing, and documented the exclusivity on the `classic` field. @daniellansun — on your four points: **1. Parity matrix.** `OpenClosureTest` (renamed from `OpenClosureSpikeTest`) compiles the same source both ways and compares the results against the generated-class compilation, for: - each `resolveStrategy`: `OWNER_FIRST`, `DELEGATE_FIRST`, `DELEGATE_ONLY`, `OWNER_ONLY` and `TO_SELF`, including the exceptions for a name that can't be resolved; - `setDelegate` / `setResolveStrategy` after creation; - owner vs delegate shadowing; - `methodMissing` / `propertyMissing` on the owner and on the delegate; - a script owner as well as a class owner; - plain and compound writes to free names. Nested closures don't take the open path: both the outer and the nested closure keep their generated classes. A test pins that decline, with parity rows. Supporting them needs owner retargeting, which is later work. **2. Landing shape.** Done as above. The ABI, fixed-arity and exception-parity changes are their own commit under GROOVY-12750, independent of GEP-27. Open closures are behind a system property, now `groovy.closure.open` (renamed from `groovy.spike.openclosure`), and off by default. **3. Static / mixed modes.** The static compiler never packs a closure open: `StaticTypesClosureWriter` turns the open trigger off, so `@CompileStatic` code is unchanged. An open closure is an ordinary `PackedClosure` at run time. Passing one into statically compiled code, storing it in a `Closure`-typed field, and serialising it aren't covered by tests yet, so GROOVY-12749 lists them under "still to address": they need either tests or a statement that they're unsupported or fall back. **4. Jochen's earlier comments.** The ones that bear on this PR: - [#2692 `PackedClosure` unwrapping `InvokerInvocationException`](https://github.com/apache/groovy/pull/2692#discussion_r3565636142): the catch is gone from the direct `call` paths in GROOVY-12750. There's no reflective step there to undo, so it was unwrapping exceptions the body itself threw. `mopCall` keeps it. - [#2692 `doCall` an inlining nightmare](https://github.com/apache/groovy/pull/2692#discussion_r3565640053): in GROOVY-12750 the fixed-arity `doCall`s go straight to `dispatchOne` / `dispatchTwo` without building an argument array. `ClosureDoCallBench` shows the monomorphic one- and two-parameter dynamic calls going from 0.6–0.7x to ~1.0x of a generated closure class, and from 128 to 32 B/op. - [#2709 interface surface with bytecode](https://github.com/apache/groovy/pull/2709#discussion_r3603051314): the bootstraps moved to `IndyInterface` in #2709. GROOVY-12750 now records the rest of the packing runtime link in the `@GroovyABI` registry (`since = "6.0.0"`), so `checkGroovyABISync` catches a breaking change. GROOVY-12749 adds a single creation entry, `ScriptBytecodeAdapter.openClosure` (`@GroovyABI(since = "7.0.0")`), and only the `Resolver` interface appears in generated class files. - [#2709 dynamic general, static a specialisation](https://github.com/apache/groovy/pull/2709#discussion_r3602932225): the new open path follows that split. The trigger, `isOpenPackTriggered`, lives in `ClosureWriter`, and `StaticTypesClosureWriter` overrides it. - [#2692 fields in between code](https://github.com/apache/groovy/pull/2692#discussion_r3565459241): `ClosureWriter`'s constants are grouped at the top of the class. -- 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]
