Copilot commented on code in PR #2692: URL: https://github.com/apache/groovy/pull/2692#discussion_r3566423724
########## src/main/java/org/codehaus/groovy/runtime/PackedClosure.java: ########## @@ -0,0 +1,224 @@ +/* + * 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 java.lang.invoke.MethodHandle; + +/** + * A single, shared {@link Closure} adapter for {@code @PackedClosures} compact closure compilation. + * <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 {@link java.lang.invoke.MethodHandle} constant to the private hoisted + * method (bound to the receiver for an instance method), so there is no reflection. + */ +public final class PackedClosure extends Closure<Object> { + + private static final long serialVersionUID = 1L; + private static final Object[] EMPTY = new Object[0]; + + private final MethodHandle handle; + 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 handle an invoke handle to the private hoisted method (a constant in the hosting + * class): dispatch needs no reflection and binds the exact method, so an + * inherited packed closure can never misdispatch to a subclass's method + * @param method the name of the hoisted synthetic method (kept for diagnostics only) + * @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. {@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. + */ + public PackedClosure(final Object owner, final MethodHandle handle, final String method, final Object[] captured, final Class[] visibleTypes, final boolean strict) { + super(owner, owner); + // Bind the receiver for an instance method so the handle takes (captured..., args...); a static + // hoisted method passes the class as owner and its handle already takes no receiver. Then adapt + // the typed handle ONCE to a canonical (Object[])->Object shape so each call is a single + // invokeExact (JIT-inlinable) rather than a per-call, arg-boxing invokeWithArguments. + MethodHandle bound = (owner instanceof Class) ? handle : handle.bindTo(owner); + this.handle = bound.asType(bound.type().generic()).asSpreader(Object[].class, bound.type().parameterCount()); + 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(); + // Fast path: the caller supplied exactly the declared number of arguments (the common + // each()/collect() call). Skip the List-destructuring, correctArguments and per-arg coercion + // adaptation -- the canonical handle's asType does any primitive (un)boxing -- and dispatch. + if (provided.length == arity) { + Class<?>[] types = getParameterTypes(); + Object[] a = provided; + for (int i = 0; i < arity; i++) { Review Comment: `PackedClosure.doCall` has a fast path when `provided.length == arity` that skips `ParameterTypes.correctArguments`. For varargs-style closure signatures (e.g. `{ Object[] args -> ... }`), a single non-array argument should be wrapped into an array (as `correctArguments` does). Skipping this will cause a `ClassCastException`/wrong dispatch when calling such closures with a single element (common Groovy varargs behavior). Consider applying `correctArguments` (or at least the varargs check) in the fast path too. ########## src/main/java/org/codehaus/groovy/classgen/asm/ClosureWriter.java: ########## @@ -158,6 +206,505 @@ public void writeClosure(final ClosureExpression expression) { controller.getOperandStack().replace(ClassHelper.CLOSURE_TYPE, localVariableParams.length); } + /** + * 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 #isDelegateIndependent}) — 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) + if (!isPackTriggered(expression, annotated)) return PackStrategy.FULL_CLASS; + if (controller.isInGeneratedFunction()) return PackStrategy.FULL_CLASS; + if (escapesEnclosingMethod(expression)) return PackStrategy.FULL_CLASS; + return PackStrategy.PACKED_ADAPTER; + } + + /** + * Whether a packable, non-escaping closure literal should actually be packed. Dynamic compilation + * has no proof of delegate-independence, so the only trigger is the {@code @PackedClosures} trust + * assertion; {@code StaticTypesClosureWriter} overrides this to add the + * {@code groovy.target.closure.pack} flag and the delegate-independence proof. + * + * @param annotated whether a non-DISABLED {@code @PackedClosures} is in scope + */ + protected boolean isPackTriggered(final ClosureExpression expression, final boolean annotated) { + return annotated; + } + + /** + * A trigger-specific decline reason for {@link #declineReason}, or {@code null} if the trigger did + * not decline this closure. Only the static writer has one (a delegate-resolved body); the dynamic + * path trusts the annotation and never declines on trigger grounds. + */ + protected String triggerDeclineReason(final ClosureExpression expression) { + return null; + } + + /** Whether the hoisted body is compiled statically (true only for {@code @CompileStatic}). */ + protected boolean compilesHoistedBodyStatically() { + return false; + } + + /** + * Whether the packed adapter installs the runtime delegate guard. The dynamic trust path needs it + * (an unverifiable assertion must fail fast on misuse); the static writer proved independence, so + * it overrides this to {@code false} and a caller-set delegate is then stored and ignored. + */ + protected boolean packedClosureUsesDelegateGuard() { + return true; + } + + /** 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)"; + } + String triggerReason = triggerDeclineReason(expression); + if (triggerReason != null) return triggerReason; + 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"; + } + + /** + * 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. */ + 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", + "invokeMethod", "getProperty", "setProperty")); + + /** 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()); + } + } + }); Review Comment: `rebindCaptured` rewrites variable references based on the variable *name* and currently visits into nested `ClosureExpression`s (because `CodeVisitorSupport.visitClosureExpression` traverses the closure body). This can incorrectly rebind shadowed variables/parameters inside nested closures that happen to share a name with a captured variable, changing semantics. A minimal guard is to only rewrite true captured references (i.e., closure-shared variables) and leave non-captured/shadowing variables untouched. -- 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]
