paulk-asert commented on code in PR #2709: URL: https://github.com/apache/groovy/pull/2709#discussion_r3648867579
########## src/test/groovy/org/codehaus/groovy/transform/PackedClosureBoundariesTest.groovy: ########## @@ -0,0 +1,129 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.codehaus.groovy.transform + +import org.codehaus.groovy.control.CompilationUnit +import org.codehaus.groovy.control.CompilerConfiguration +import org.codehaus.groovy.control.Phases +import org.junit.jupiter.api.Test + +import static org.junit.jupiter.api.Assertions.assertEquals + +/** + * The packability boundary as one readable matrix — executable documentation of GEP-27's + * "packability decision procedure". Each case is a method body containing one closure literal; + * the assertion is whether that literal packs (no generated closure class) or declines (keeps + * its class), under the flag with {@code @CompileStatic} or dynamic compilation as noted. + * Individual gates have focused behavioural tests elsewhere; this class exists so the whole + * boundary can be read top-to-bottom in one place. + */ +final class PackedClosureBoundariesTest { + + private static final String PROP = CompilerConfiguration.CLOSURE_PACKING + + /** [description, dynamic-packs?, cs-packs?, method body containing exactly one closure literal] */ + private static final List CASES = [ + // ---- the syntactic no-free-name subset: packs everywhere ------------------------------- + ['no free names', true, true, 'def m(List<Integer> xs) { xs.collect { x -> x + 1 } }'], + ['implicit it', true, true, 'def m(List<Integer> xs) { xs.collect { it * 2 } }'], + ['explicit zero params', true, true, 'def m() { def c = { -> 42 }; c() }'], + ['captured local (read-only)', true, true, 'def m(List<Integer> xs) { def k = 10; xs.collect { x -> x + k } }'], + ['captured local (written)', true, true, 'def m(List<Integer> xs) { int t = 0; xs.each { t += it }; t }'], + ['parameter receiver', true, true, 'def m(List<Integer> xs) { xs.collect { it.toString() } }'], + // ---- free names: the types-or-trust boundary (dynamic declines, CS proof packs) -------- + ['implicit-this method call', false, true, 'def helper(x) { x }\ndef m(List<Integer> xs) { xs.collect { helper(it) } }'], + ['bare field-bound name', false, true, 'int field = 1\ndef m(List<Integer> xs) { xs.collect { it + field } }'], + ['explicit this-property', false, true, 'int field = 1\ndef m(List<Integer> xs) { xs.collect { it + this.field } }'], + // ---- real-Closure semantics: declines everywhere ---------------------------------------- + ['uses delegate', false, false, 'def m() { def c = { delegate.toString() }; c }'], + ['uses owner', false, false, 'def m() { def c = { owner.toString() }; c }'], + ['default parameter values', false, false, 'def m() { def c = { int x = 1 -> x }; c(2) }'], + // ---- escapes: declines everywhere -------------------------------------------------------- + ['returned', false, false, 'Closure m() { return { it } }'], + ['stored to property', false, false, 'def m(Map attrs) { attrs.handler = { it } }'], + ['in a collection literal', false, false, 'def m() { [{ it }] }'], Review Comment: Implicit `it` is actually supported by packing — it isn't compiled as a default parameter on the hoisted method; the adapter's `FixedIt` family member reproduces the fuzzy 0/1 arity (`doCall()` + `doCall(Object)`). The `'implicit it'` row near the top of the matrix pins that: `xs.collect { it * 2 }` packs on both paths. So in those last three rows the escape gate genuinely is the operative decline, not default-param support. That said, your point stands that the rows didn't *prove* it — all three happened to use `{ it }`, so escape and implicit-`it` weren't isolated from each other. They now use explicit parameters (`{ x -> x }`), same for the escaping shapes in `PackedClosuresTransformTest`, so the only difference from the packing rows is the escaping position. On why an escaping closure is declined at all: it's a policy gate, not a capability limit. A visibly escaping literal reaches consumers unknown to this compilation, which may set a delegate on it (which fail-fasts at runtime on the dynamic trust path), serialize it, or rely on per-literal class identity — so the gate conservatively keeps it a class rather than converting working code into runtime surprises. The section comment in the test now says exactly this. For the statically-proven path (where a caller-set delegate is stored-and-ignored) the gate could in principle be relaxed later; keeping it uniform seemed the safer S1 default. -- 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]
