Copilot commented on code in PR #2692: URL: https://github.com/apache/groovy/pull/2692#discussion_r3565561943
########## src/main/java/org/codehaus/groovy/runtime/PackedClosure.java: ########## @@ -0,0 +1,243 @@ +/* + * 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.runtime; + +import groovy.lang.Closure; +import groovy.lang.GroovyRuntimeException; + +import java.util.Arrays; + +/** + * A single, shared {@link Closure} adapter used by the {@code @PackedClosures} prototype. + * <p> + * Normally the Groovy compiler generates one inner class per closure literal + * (e.g. {@code Owner$_method_closure1}). Under {@code @PackedClosures} an <em>eligible</em> + * closure body is instead hoisted into a synthetic method on the enclosing ("owner") + * class, and the closure literal is replaced by an instance of this one adapter class, + * which dispatches back to that method. This removes the per-closure generated class + * (and the deeply-nested {@code $_closure1$_closure2$_closure3} name explosion) while + * still yielding a real {@code groovy.lang.Closure} instance, so features that operate + * through {@code call()} (iteration, {@code curry}, {@code memoize}, {@code trampoline}) + * continue to work. + * <p> + * Captured values are stored at construction and prepended to the call arguments before + * dispatch, so the hoisted method has the shape {@code method(captured..., closureParams...)}. + * Dispatch goes through a cached reflective {@link java.lang.reflect.Method} rather than an + * {@code invokedynamic} call site. + */ +public final class PackedClosure extends Closure<Object> { + + private static final long serialVersionUID = 1L; + private static final Object[] EMPTY = new Object[0]; + + private final String method; + private final Object[] captured; + private final boolean strict; + private final org.codehaus.groovy.reflection.ParameterTypes paramInfo; + + /** + * @param owner the enclosing instance the hoisted method lives on (also used as thisObject) + * @param method the name of the hoisted synthetic method + * @param captured values captured from the enclosing scope, prepended to each call + * @param maxParams the number of parameters the original closure declared (its visible arity) + */ + public PackedClosure(final Object owner, final String method, final Object[] captured, final int maxParams) { + this(owner, method, captured, maxParams, true); + } + + /** + * @param owner the enclosing instance the hoisted method lives on (also used as thisObject) + * @param method the name of the hoisted synthetic method + * @param captured values captured from the enclosing scope, prepended to each call + * @param maxParams the number of parameters the original closure declared (its visible arity) + * @param strict whether the delegate guard throws. {@code true} for the dynamic trust path + * (an unverifiable {@code @PackedClosures} assertion must fail fast on misuse); + * {@code false} when the type checker PROVED every free name owner-resolved — + * then a caller-set delegate is provably never consulted, so it is stored and + * ignored, exactly like a statically-compiled closure class behaves today. + */ + public PackedClosure(final Object owner, final String method, final Object[] captured, final int maxParams, final boolean strict) { + super(owner, owner); + this.method = method; + this.captured = (captured != null) ? captured : EMPTY; + this.strict = strict; + this.maximumNumberOfParameters = maxParams; + Class<?>[] pt = new Class<?>[maxParams]; + Arrays.fill(pt, Object.class); + this.parameterTypes = pt; + this.paramInfo = null; + } + + /** + * @param owner the enclosing instance the hoisted method lives on (also used as thisObject) + * @param method the name of the hoisted synthetic method + * @param captured values captured from the enclosing scope, prepended to each call + * @param visibleTypes the closure's declared parameter types, so callers that key behaviour on + * {@code getParameterTypes()} (DGM arity/type decisions) and argument + * adaptation (vararg collection into a trailing array parameter) behave + * exactly as with a generated closure class + * @param strict whether the delegate guard throws (see the other constructor) + */ + public PackedClosure(final Object owner, final String method, final Object[] captured, final Class[] visibleTypes, final boolean strict) { + super(owner, owner); + this.method = method; + this.captured = (captured != null) ? captured : EMPTY; + this.strict = strict; + this.maximumNumberOfParameters = visibleTypes.length; + this.parameterTypes = visibleTypes; + this.paramInfo = new org.codehaus.groovy.reflection.ParameterTypes(visibleTypes); + } + + // GEP-27 runtime guard: a hoisted body binds free names to the owner at compile time, so it + // cannot honour a caller-set delegate. Rather than silently mis-resolve (returning the owner's + // member where a normal closure would reach the delegate), fail fast at the point of misuse. + // Setting the delegate to the owner is the harmless default and stays a no-op. + @Override + public void setDelegate(final Object delegate) { + if (strict && delegate != null && delegate != getOwner()) { + throw new UnsupportedOperationException( + "Cannot set a delegate on a @PackedClosures closure (hoisted method '" + method + + "'): its free names were bound to the owner at compile time, so an external " + + "delegate cannot be honoured. Remove @PackedClosures from this scope, or " + + "exclude this closure, if it relies on delegate-based resolution."); + } + super.setDelegate(delegate); + } + + @Override + public void setResolveStrategy(final int resolveStrategy) { + // DELEGATE_FIRST/DELEGATE_ONLY need a delegate the hoisted body cannot consult; TO_SELF + // would resolve names against this shared adapter, which has no user-defined members. + // Only owner-based resolution (OWNER_FIRST/OWNER_ONLY) is reproducible by a hoisted body. + if (strict && resolveStrategy != OWNER_FIRST && resolveStrategy != OWNER_ONLY) { + throw new UnsupportedOperationException( + "Cannot set resolveStrategy=" + strategyName(resolveStrategy) + + " on a @PackedClosures closure (hoisted method '" + method + "'): a hoisted " + + "body resolves free names against the owner at compile time, so only " + + "OWNER_FIRST/OWNER_ONLY are supported. Remove @PackedClosures from this " + + "scope, or exclude this closure."); + } + super.setResolveStrategy(resolveStrategy); + } + + private static String strategyName(final int s) { + switch (s) { + case DELEGATE_FIRST: return "DELEGATE_FIRST"; + case DELEGATE_ONLY: return "DELEGATE_ONLY"; + case TO_SELF: return "TO_SELF"; + default: return String.valueOf(s); + } + } + + // Dispatch call() straight to doCall: the generic varargs doCall is otherwise vararg-ambiguous + // through the metaclass when a single argument is itself an Object[] (the metaclass would treat + // the element as the whole argument list and spread it). Unwrap invoker exceptions exactly as + // Closure.call does, so user exceptions surface unwrapped. + @Override + public Object call(final Object... args) { + try { + return doCall(args); + } catch (InvokerInvocationException e) { + org.apache.groovy.internal.util.UncheckedThrow.rethrow(e.getCause()); + return null; // unreachable + } + } + + public Object doCall(final Object... args) { + Object[] provided = (args != null) ? args : EMPTY; + int arity = getMaximumNumberOfParameters(); + // A real closure destructures a single List/Tuple argument across a multi-parameter + // signature ({ a, b -> } called with one Tuple2); mirror that before arity normalisation. + if (provided.length == 1 && arity > 1 && provided[0] instanceof java.util.List) { + provided = ((java.util.List<?>) provided[0]).toArray(); + } + // With the real parameter types available, adapt arguments exactly as reflective dispatch on + // a generated closure class would -- collecting excess args into a trailing array parameter + // ({ Object[] args -> } called with several values), and coercing each argument to its + // declared parameter type (Closure -> SAM interface, GString -> String, number conversions) + // as metaclass dispatch does. + if (paramInfo != null) { + try { + provided = paramInfo.correctArguments(provided); + } catch (RuntimeException ignore) { + // fall through to the arity normalisation below + } + Class<?>[] types = getParameterTypes(); + for (int i = 0; i < provided.length && i < types.length; i++) { + Object arg = provided[i]; + Class<?> type = types[i]; + if (arg != null && type != Object.class && !type.isInstance(arg)) { + try { + provided[i] = org.codehaus.groovy.runtime.typehandling.DefaultTypeTransformation.castToType(arg, type); + } catch (RuntimeException ignore) { + // leave the argument as-is; the invoke below reports the mismatch + } + } + } + } + // Normalise the visible arguments to the closure's declared arity: pad missing with + // null (implicit-it / under-application) and drop extras (over-application), matching + // how a normal Groovy closure tolerates arity mismatches from callers such as each(). + Object[] all = new Object[captured.length + arity]; + System.arraycopy(captured, 0, all, 0, captured.length); + for (int i = 0; i < arity; i++) { + all[captured.length + i] = (i < provided.length) ? provided[i] : null; + } + return invokeHoisted(all); + } + + private transient java.lang.reflect.Method target; + + /** + * Invokes the hoisted method reflectively via a cached {@link java.lang.reflect.Method}, NOT the + * metaclass: metaclass argument coercion auto-dereferences a {@code groovy.lang.Reference} + * argument, which would unwrap the shared Reference a written capture must arrive as. + */ + private Object invokeHoisted(final Object[] all) { + java.lang.reflect.Method m = target; + if (m == null) { + Object owner = getOwner(); + // for a closure in a static context the owner IS the declaring class + Class<?> c = (owner instanceof Class) ? (Class<?>) owner : owner.getClass(); + outer: + for (; c != null; c = c.getSuperclass()) { // hoisted onto a superclass when inherited + for (java.lang.reflect.Method cand : c.getDeclaredMethods()) { + if (cand.getName().equals(method) && cand.getParameterCount() == all.length) { + m = cand; + break outer; + } + } + } + if (m == null) { + throw new GroovyRuntimeException("Hoisted closure method not found: " + method); + } + m.setAccessible(true); + target = m; Review Comment: PackedClosure.invokeHoisted unconditionally calls Method#setAccessible(true), but the compiler emits hoisted closure methods as ACC_PUBLIC (see ClosureWriter: mods = ACC_PUBLIC | ACC_SYNTHETIC ...). The extra reflective override is unnecessary and can trigger illegal-access warnings or runtime failures under tighter reflection policies. ########## src/main/java/org/codehaus/groovy/classgen/asm/ClosureWriter.java: ########## @@ -158,6 +197,533 @@ public void writeClosure(final ClosureExpression expression) { controller.getOperandStack().replace(ClassHelper.CLOSURE_TYPE, localVariableParams.length); } + private int packedCounter; + + /** Name prefix of the synthetic methods holding hoisted closure bodies. */ + private static final String PACKED_METHOD_PREFIX = "$packed$closure$"; + + /** + * GEP-27 capability analysis: choose the compilation strategy for a closure literal. + * <p> + * A closure is packed (S1, {@link PackStrategy#PACKED_ADAPTER}) when it is <em>triggered</em> — + * either the enclosing scope opts in via {@code @PackedClosures} (the dynamic trust path), or the + * capability analysis <em>proves</em> it delegate-independent under {@code @CompileStatic} (the + * automatic path, see {@link #isProvablyDelegateIndependent}) — and it is in a packable position: + * it needs no real {@code Closure} semantics we cannot reproduce ({@link #isPackable}), it is Review Comment: Javadoc references a non-existent method (isProvablyDelegateIndependent). This breaks IDE navigation and makes the docs misleading; the implementation method is named isDelegateIndependent. ########## src/main/java/org/codehaus/groovy/classgen/asm/ClosureWriter.java: ########## @@ -158,6 +197,533 @@ public void writeClosure(final ClosureExpression expression) { controller.getOperandStack().replace(ClassHelper.CLOSURE_TYPE, localVariableParams.length); } + private int packedCounter; + + /** Name prefix of the synthetic methods holding hoisted closure bodies. */ + private static final String PACKED_METHOD_PREFIX = "$packed$closure$"; + + /** + * GEP-27 capability analysis: choose the compilation strategy for a closure literal. + * <p> + * A closure is packed (S1, {@link PackStrategy#PACKED_ADAPTER}) when it is <em>triggered</em> — + * either the enclosing scope opts in via {@code @PackedClosures} (the dynamic trust path), or the + * capability analysis <em>proves</em> it delegate-independent under {@code @CompileStatic} (the + * automatic path, see {@link #isProvablyDelegateIndependent}) — and it is in a packable position: + * it needs no real {@code Closure} semantics we cannot reproduce ({@link #isPackable}), it is + * written directly in a method rather than nested in another closure (owner-retargeting for + * nested closures is later work), and it does not visibly escape its method. Otherwise it stays a + * full generated class (S0, {@link PackStrategy#FULL_CLASS}) exactly as today. + */ + private PackStrategy chooseStrategy(final ClosureExpression expression) { + if (!isPackable(expression)) return PackStrategy.FULL_CLASS; + // The most-specific @PackedClosures mode in scope (method wins over class), or null if not + // annotated. DISABLED is a fine-grained opt-out that beats an enclosing opt-in AND the flag. + PackMode mode = annotatedMode(); + if (mode == PackMode.DISABLED) return PackStrategy.FULL_CLASS; + boolean annotated = (mode != null); // LENIENT/WARN/STRICT all opt in (DISABLED handled above) + boolean triggered; + if (isStaticCompilation()) { + // Under @CompileStatic packing requires the STC proof of delegate-independence even for + // the @PackedClosures opt-in: a delegate-resolved body compiles IMPLICIT_RECEIVER + // expressions against Closure.getDelegate(), which a hoisted method cannot honour. + triggered = (annotated || SystemUtil.getBooleanSafe("groovy.target.closure.pack")) + && isDelegateIndependent(expression); + } else { + // Dynamic compilation has no proof; only the annotation's trust assertion packs, and the + // PackedClosure runtime guard backstops a wrong assumption. + triggered = annotated; + } + if (!triggered) return PackStrategy.FULL_CLASS; + if (controller.isInGeneratedFunction()) return PackStrategy.FULL_CLASS; + if (escapesEnclosingMethod(expression)) return PackStrategy.FULL_CLASS; + return PackStrategy.PACKED_ADAPTER; + } + + /** + * True when the type checker resolved every implicitly-received name in the closure (and its + * nested closures) against the owner/parameters rather than a delegate — the delegate-independence + * proof of the capability analysis. Under {@code @CompileStatic} STC records each name's resolution + * path as {@code IMPLICIT_RECEIVER}: a path ending in {@code "owner"} was resolved against the + * owner (safe), anything else (e.g. {@code "delegate"}, from {@code @DelegatesTo} or {@code with}) + * went through a delegate — the same distinction STC itself makes. The marker can sit on a method + * call, a property expression, or a bare variable (e.g. {@code length} inside {@code with}), so + * all three are checked. If nothing resolved through a delegate, packing preserves resolution. + */ + private boolean isDelegateIndependent(final ClosureExpression expression) { + Statement code = expression.getCode(); + if (code == null) return false; + boolean[] delegateResolved = {false}; + // Walk the whole closure tree, including nested closures: a delegate resolution anywhere in + // the body (e.g. an owner-passed closure whose inner closure resolves a name against the + // delegate) means the top closure carries a delegate chain that packing would break. + code.visit(new CodeVisitorSupport() { + @Override public void visitMethodCallExpression(final MethodCallExpression call) { + flag(call.getNodeMetaData(IMPLICIT_RECEIVER)); + super.visitMethodCallExpression(call); + } + @Override public void visitPropertyExpression(final PropertyExpression pexp) { + flag(pexp.getNodeMetaData(IMPLICIT_RECEIVER)); + super.visitPropertyExpression(pexp); + } + @Override public void visitVariableExpression(final VariableExpression ve) { + flag(ve.getNodeMetaData(IMPLICIT_RECEIVER)); + // a name STC left dynamic (no local/param binding, no recorded receiver path) will be + // resolved at runtime through the closure's owner/delegate chain -- not provably owner + if (ve.getAccessedVariable() instanceof org.codehaus.groovy.ast.DynamicVariable + && ve.getNodeMetaData(IMPLICIT_RECEIVER) == null) { + delegateResolved[0] = true; + } + super.visitVariableExpression(ve); + } + private void flag(final Object receiver) { + if (receiver instanceof String && !((String) receiver).endsWith("owner")) delegateResolved[0] = true; + } + }); + return !delegateResolved[0]; + } + + /** Overridden by the static-compilation closure writer; the type checker's resolution only applies there. */ + protected boolean isStaticCompilation() { + return false; + } + + /** The closure's inferred return type, normalised the same way {@code createClosureClass} does for doCall. */ + private static ClassNode inferredClosureReturnType(final ClosureExpression expression) { + ClassNode returnType = expression.getNodeMetaData(INFERRED_RETURN_TYPE); + if (returnType == null) returnType = ClassHelper.OBJECT_TYPE; + else if (returnType.isPrimaryClassNode()) returnType = returnType.getPlainNodeReference(); + else if (ClassHelper.isPrimitiveType(returnType)) returnType = ClassHelper.getWrapper(returnType); + else if (GenericsUtils.hasUnresolvedGenerics(returnType)) returnType = GenericsUtils.nonGeneric(returnType); + return returnType; + } + + /** + * The erasure of a captured variable's type, safe to declare on the hoisted method: generic + * placeholders are replaced by their bound (the hoisted method does not declare the enclosing + * method's type variables) and type arguments are dropped ({@code List<T>} → {@code List}). + */ + private static ClassNode erasedType(final ClassNode type) { + ClassNode t = type; + if (t == null) return ClassHelper.OBJECT_TYPE; + if (t.isGenericsPlaceHolder()) t = t.redirect(); + return t.getPlainNodeReference(); + } + + /** + * Reports a top-level closure that was NOT packed, per the {@code mode} of the enclosing + * {@code @PackedClosures} annotation (WARN => compiler warning, STRICT => compiler error). Only the + * annotation opts in; the automatic {@code groovy.target.closure.pack} path is always lenient. A + * nested closure is a structural decline (its enclosing context is a generated function, not the + * owner class), so it is not reported. + */ + private void reportUnpacked(final ClosureExpression expression) { + if (controller.isInGeneratedFunction()) return; + PackMode mode = annotatedMode(); + // LENIENT/DISABLED are both silent -- DISABLED asked NOT to pack, so a decline is expected + if (mode == null || mode == PackMode.LENIENT || mode == PackMode.DISABLED) return; + String msg = "@PackedClosures: closure was not packed -- " + declineReason(expression); + if (mode == PackMode.STRICT) { + controller.getSourceUnit().getErrorCollector() + .addErrorAndContinue(msg, expression, controller.getSourceUnit()); + } else { + // WARN is opt-in, so emit at LIKELY_ERRORS to show at the default warning level (the plain + // addWarning(text, node) uses POSSIBLE_ERRORS, which the default level suppresses). groovyc + // surfaces collected warnings on success since GROOVY-12132; Gradle/IDEs already do. + controller.getSourceUnit().addWarning(WarningMessage.LIKELY_ERRORS, msg, expression); + } + } + + /** The most specific {@code @PackedClosures} mode in scope (method wins over class), or null if not annotated. */ + private PackMode annotatedMode() { + MethodNode m = controller.getMethodNode(); + PackMode mode = (m != null) ? packModeOf(m.getAnnotations()) : null; + if (mode != null) return mode; + for (ClassNode c = controller.getClassNode(); c != null; c = c.getOuterClass()) { + mode = packModeOf(c.getAnnotations()); + if (mode != null) return mode; + } + return null; + } + + /** The mode of a {@code @PackedClosures} in the given set (LENIENT if present without an explicit mode), or null. */ + private static PackMode packModeOf(final List<AnnotationNode> annotations) { + for (AnnotationNode a : annotations) { + if (a.getClassNode() != null && "groovy.transform.PackedClosures".equals(a.getClassNode().getName())) { + Expression member = a.getMember("mode"); + if (member instanceof PropertyExpression) { + try { + return PackMode.valueOf(((PropertyExpression) member).getPropertyAsString()); + } catch (IllegalArgumentException ignore) { + // malformed member; treat as the default + } + } + return PackMode.LENIENT; + } + } + return null; + } + + /** Why {@link #chooseStrategy} declined this (packable, top-level) closure -- the message WARN/STRICT report. */ + private String declineReason(final ClosureExpression expression) { + if (!isPackable(expression)) { + return "it needs a real Closure (uses owner/delegate/thisObject/resolveStrategy/super or metaClass, " + + "has default parameter values, or contains an anonymous inner class)"; + } + if (isStaticCompilation() && !isDelegateIndependent(expression)) { + return "it resolves a name against a delegate (e.g. via @DelegatesTo or with{})"; + } + if (escapesEnclosingMethod(expression)) { + return "it escapes its method (returned, stored to a field/property/index, appended, or in a collection literal)"; + } + return "it is not eligible for packing in this scope"; + } + + // Closures found in a syntactic position that lets them outlive the method are cached per method. + private final Map<MethodNode, Set<ClosureExpression>> escapingClosuresByMethod = new HashMap<>(); + + /** + * Conservative compile-time escape gate. A packed closure is bound to the owner and backed by a + * shared adapter that fails fast if a delegate is later set on it. To avoid that surprise for the + * clearest cases, decline (keep a real closure class for) a closure literal that visibly escapes + * the method it is written in: stored into a field/property/index (e.g. {@code attrs.optionValue = + * { ... }}), returned, appended with {@code <<}, or placed in a list/map literal. Transitive escapes + * (via a local that is later returned) are intentionally not chased here; the runtime guard remains + * the backstop for those. + */ + private boolean escapesEnclosingMethod(final ClosureExpression expression) { + MethodNode m = controller.getMethodNode(); + if (m == null || m.getCode() == null) return false; + return escapingClosuresByMethod + .computeIfAbsent(m, k -> collectEscapingClosures(k.getCode())) + .contains(expression); + } + + private static Set<ClosureExpression> collectEscapingClosures(final Statement code) { + Set<ClosureExpression> escaping = Collections.newSetFromMap(new IdentityHashMap<>()); + code.visit(new CodeVisitorSupport() { + private void mark(final Expression e) { + if (e instanceof ClosureExpression) escaping.add((ClosureExpression) e); + } + @Override public void visitReturnStatement(final ReturnStatement statement) { + mark(statement.getExpression()); + super.visitReturnStatement(statement); + } + @Override public void visitBinaryExpression(final BinaryExpression be) { + int op = be.getOperation().getType(); + if (op == Types.EQUAL && !isLocalTarget(be.getLeftExpression())) { + mark(be.getRightExpression()); // assigned to a field/property/index + } else if (op == Types.LEFT_SHIFT) { + mark(be.getRightExpression()); // appended, e.g. list << { ... } + } + super.visitBinaryExpression(be); + } + @Override public void visitListExpression(final ListExpression le) { + le.getExpressions().forEach(this::mark); + super.visitListExpression(le); + } + @Override public void visitMapEntryExpression(final MapEntryExpression me) { + mark(me.getValueExpression()); + super.visitMapEntryExpression(me); + } + }); + return escaping; + } + + /** True when an assignment target is a plain local variable (not a field/property that escapes). */ + private static boolean isLocalTarget(final Expression lhs) { + if (!(lhs instanceof VariableExpression)) return false; // property/field/index target + Variable v = ((VariableExpression) lhs).getAccessedVariable(); + return !(v instanceof FieldNode) && !(v instanceof PropertyNode); + } + + /** + * Spike packability gate: pack read-only closures (including nested and captures). Captured-variable + * writes are supported via Reference threading for a flat closure only (declined if the body also + * contains a nested closure, or uses an unsupported compound-assignment operator). Anything needing a + * real Closure — delegate/owner/thisObject/resolveStrategy or {@code super} — is declined. + */ + private static boolean isPackable(final ClosureExpression expression) { + Statement code = expression.getCode(); + if (code == null) return false; + // Default parameter values generate arity-overloaded doCall methods on a closure class; + // the single hoisted method cannot reproduce that dispatch, so decline. + if (expression.isParameterSpecified()) { + for (Parameter p : expression.getParameters()) { + if (p.hasInitialExpression()) return false; + } + } + boolean[] ok = {true}; + code.visit(new CodeVisitorSupport() { + @Override public void visitClosureExpression(final ClosureExpression e) { + // A nested closure constructed inside the hoisted method gets the enclosing INSTANCE + // as its owner instead of the (packed) outer closure object. That is observationally + // equivalent for owner-chain resolution, but not for a nested body that names + // owner/delegate/thisObject explicitly -- so the outer packs only if every nested + // closure is itself packable. + if (!isPackable(e)) ok[0] = false; + } + @Override public void visitVariableExpression(final VariableExpression ve) { + if (ve.isSuperExpression() || FORBIDDEN_CLOSURE_NAMES.contains(ve.getName())) ok[0] = false; + } + @Override public void visitMethodCallExpression(final MethodCallExpression call) { + // implicit-this accessor/mutator forms of the same pseudo-properties, e.g. getDelegate() + if (call.isImplicitThis() && FORBIDDEN_CLOSURE_CALLS.contains(call.getMethodAsString())) ok[0] = false; + super.visitMethodCallExpression(call); + } + @Override public void visitConstructorCallExpression(final ConstructorCallExpression cce) { + // an anonymous inner class in the body is generated against the enclosing closure + // class (its outer instance); hoisting would hand it the wrong enclosing instance + if (cce.isUsingAnonymousInnerClass()) ok[0] = false; + super.visitConstructorCallExpression(cce); + } + }); + // All write forms to captured variables are supported: the shared Reference is passed as a + // holder parameter, so the ASM generator emits the implicit get()/set() exactly as it does + // for a closure class -- no operator restrictions. + return ok[0]; + } + + private static Set<String> capturedNames(final ClosureExpression expression) { + Set<String> captured = new HashSet<>(); + VariableScope vs = expression.getVariableScope(); + if (vs != null) { + for (Iterator<Variable> it = vs.getReferencedLocalVariablesIterator(); it.hasNext(); ) { + captured.add(it.next().getName()); + } + } + return captured; + } + + /** Captured variables that are written (reassigned) in the body — these need Reference threading. */ + // Names assigned anywhere in a method (excluding declarations), cached per method: a capture must + // be Reference-threaded if it is written ANYWHERE in the enclosing method, not just inside the + // closure -- e.g. `def fib; fib = { n -> ... fib(n-1) ... }` writes fib after the closure is + // constructed, so a by-value capture would see the stale (null) value. + private final Map<MethodNode, Set<String>> writtenNamesByMethod = new HashMap<>(); + + private Set<String> writtenCaptureNames(final ClosureExpression expression) { + Set<String> captured = capturedNames(expression); + Set<String> written = new HashSet<>(collectWrittenNames(expression.getCode())); + MethodNode m = controller.getMethodNode(); + if (m != null && m.getCode() != null) { + written.addAll(writtenNamesByMethod.computeIfAbsent(m, k -> collectWrittenNames(k.getCode()))); + } + written.retainAll(captured); + return written; + } + + /** Names assigned or incremented in the given code (declarations with initializers excluded). */ + private static Set<String> collectWrittenNames(final Statement code) { + Set<String> written = new HashSet<>(); + if (code == null) return written; + code.visit(new CodeVisitorSupport() { // descends into nested closures + @Override public void visitBinaryExpression(final BinaryExpression be) { + if (Types.isAssignment(be.getOperation().getType()) + && !(be instanceof org.codehaus.groovy.ast.expr.DeclarationExpression) + && be.getLeftExpression() instanceof VariableExpression) { + written.add(((VariableExpression) be.getLeftExpression()).getName()); + } + super.visitBinaryExpression(be); + } + @Override public void visitPostfixExpression(final PostfixExpression pe) { + recordIncrement(pe.getExpression()); + super.visitPostfixExpression(pe); + } + @Override public void visitPrefixExpression(final PrefixExpression pe) { + recordIncrement(pe.getExpression()); + super.visitPrefixExpression(pe); + } + private void recordIncrement(final Expression operand) { + if (operand instanceof VariableExpression) { + written.add(((VariableExpression) operand).getName()); + } + } + }); + return written; + } + + // NB: metaClass resolves against the closure object ITSELF (per-closure-class identity), which + // the shared adapter cannot reproduce, so it declines like the other closure pseudo-properties. + private static final Set<String> FORBIDDEN_CLOSURE_NAMES = + new HashSet<>(java.util.Arrays.asList("owner", "delegate", "thisObject", "directive", + "resolveStrategy", "metaClass")); + + /** Accessor/mutator forms of the same pseudo-properties, called with implicit this. */ + private static final Set<String> FORBIDDEN_CLOSURE_CALLS = + new HashSet<>(java.util.Arrays.asList("getOwner", "getDelegate", "getThisObject", "getDirective", + "getResolveStrategy", "setDelegate", "setDirective", "setResolveStrategy", + "getMaximumNumberOfParameters", "getParameterTypes", "getMetaClass", "setMetaClass")); + + /** Rebinds read-only captured-variable references in the moved body to the hoisted method's parameters. */ + private static void rebindCaptured(final Statement body, final Map<String, Parameter> capturedParams) { + body.visit(new CodeVisitorSupport() { + @Override public void visitVariableExpression(final VariableExpression ve) { + Parameter p = capturedParams.get(ve.getName()); + if (p != null) { + ve.setAccessedVariable(p); + // read-only captures become plain by-value params; written captures stay + // closure-shared so the ASM generator routes them through the holder + ve.setClosureSharedVariable(p.isClosureSharedVariable()); + } + } + }); + } + + /** + * Emits a packed closure: hoists the body to a synthetic method on the enclosing class (captured + * variables become leading, by-value parameters) and constructs a shared {@code PackedClosure} + * adapter that dispatches to it — no inner class. + */ + private void writePackedClosure(final ClosureExpression expression) { + ClassNode enclosing = controller.getClassNode(); + MethodVisitor mv = controller.getMethodVisitor(); + AsmClassGenerator acg = controller.getAcg(); + OperandStack os = controller.getOperandStack(); + + List<String> captured = new ArrayList<>(); + Map<String, ClassNode> capturedTypes = new HashMap<>(); + VariableScope vs = expression.getVariableScope(); + if (vs != null) { + for (Iterator<Variable> it = vs.getReferencedLocalVariablesIterator(); it.hasNext(); ) { + Variable v = it.next(); + captured.add(v.getName()); + capturedTypes.put(v.getName(), erasedType(v.getOriginType())); + } + } + + Parameter[] closureParams = expression.isParameterSpecified() + ? expression.getParameters() + : new Parameter[]{new Parameter(ClassHelper.OBJECT_TYPE, "it")}; + int arity = closureParams.length; + + Set<String> written = writtenCaptureNames(expression); + Parameter[] methodParams = new Parameter[captured.size() + closureParams.length]; + Map<String, Parameter> readOnlyParams = new HashMap<>(); + Map<String, Parameter> writtenParams = new HashMap<>(); + boolean typedBody = isStaticCompilation(); + for (int i = 0; i < captured.size(); i++) { + String cn = captured.get(i); + boolean isWritten = written.contains(cn); + Parameter p; + if (isWritten) { + // A written capture arrives as the SHARED groovy.lang.Reference itself: declare the + // parameter exactly like a closure-class constructor does (originType = value type, + // descriptor type = Reference, closure-shared + use-existing-reference), so + // CompileStack.init registers it as a holder and the ASM generator emits the implicit + // get()/set() on every read/write -- no AST rewriting, and the untouched body keeps + // the STC metadata that lets it compile statically. + ClassNode vt = capturedTypes.get(cn); + if (ClassHelper.isPrimitiveType(vt)) vt = ClassHelper.getWrapper(vt); + p = new Parameter(vt, cn); + p.setType(ClassHelper.makeReference()); + p.setClosureSharedVariable(true); + p.putNodeMetaData(UseExistingReference.class, Boolean.TRUE); + // the static writer's type chooser must see the VALUE type: the holder load already + // dereferences, so resolving this variable as Reference would add a bogus checkcast + p.putNodeMetaData(org.codehaus.groovy.transform.stc.StaticTypesMarker.INFERRED_TYPE, vt); + } else { + // For a statically-compiled body, type the read-only capture from its declared type so + // the loads match the STC metadata already on the body expressions (otherwise the + // static writer falls back to dynamic dispatch on the type mismatch). + p = new Parameter(typedBody ? capturedTypes.get(cn) : ClassHelper.OBJECT_TYPE, cn); + } + methodParams[i] = p; + (isWritten ? writtenParams : readOnlyParams).put(cn, p); + } + System.arraycopy(closureParams, 0, methodParams, captured.size(), closureParams.length); + + Statement body = expression.getCode(); + Map<String, Parameter> allCaptured = new HashMap<>(readOnlyParams); + allCaptured.putAll(writtenParams); + rebindCaptured(body, allCaptured); // read-only -> plain param; written -> holder param + + // In a static method there is no `this`: the owner is the enclosing class and the hoisted + // method must be static (loaded/dispatched against the class, not local slot 0). + boolean staticContext = controller.isStaticMethod(); + String name = PACKED_METHOD_PREFIX + (packedCounter++); + int mods = ACC_PUBLIC | ACC_SYNTHETIC | (staticContext ? ACC_STATIC : 0); + // Same return type a generated closure class would give doCall, so implicit return-value Review Comment: The hoisted-method name uses only a per-class counter ("$packed$closure$0", "$packed$closure$1", ...). Because WriterController creates a fresh ClosureWriter per generated class, subclasses will also start at $packed$closure$0. PackedClosure.invokeHoisted() resolves the method starting from the *runtime* owner class and walking up the superclass chain, so an inherited packed closure from a superclass can accidentally dispatch to a same-named hoisted method on a subclass (name + parameterCount match), executing the wrong closure body. -- 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]
