This is an automated email from the ASF dual-hosted git repository.
paulk-asert pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/groovy.git
The following commit(s) were added to refs/heads/master by this push:
new 3740ee7e4c GROOVY-12164: extend Closure.call fast path to typed
one-arg overrides
3740ee7e4c is described below
commit 3740ee7e4c96992bb7ba3d915df5947b01ed2e06
Author: Paul King <[email protected]>
AuthorDate: Tue Jul 14 16:35:55 2026 +1000
GROOVY-12164: extend Closure.call fast path to typed one-arg overrides
A closure with a typed parameter generates call(T)/doCall(T) rather than
call(Object), which the GROOVY-11911 CallOverride lookup cannot see, so
every such call fell back to full metaclass dispatch (~2.5x slower via
DGM iteration). Cache the single unambiguous typed one-arg override with
an instance-of guard (boxed for primitives): exact-instance arguments
dispatch directly; anything needing Groovy coercion (GString->String,
number conversions, null) still falls through to the metaclass, which
behaves exactly as before. Untyped closures are unaffected.
---
src/main/java/groovy/lang/Closure.java | 86 ++++++++++++++-
src/test/groovy/bugs/Groovy12164.groovy | 186 ++++++++++++++++++++++++++++++++
2 files changed, 267 insertions(+), 5 deletions(-)
diff --git a/src/main/java/groovy/lang/Closure.java
b/src/main/java/groovy/lang/Closure.java
index d559bc55a8..cde84c068d 100644
--- a/src/main/java/groovy/lang/Closure.java
+++ b/src/main/java/groovy/lang/Closure.java
@@ -41,6 +41,7 @@ import java.io.Serializable;
import java.io.Writer;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
import java.util.ArrayDeque;
import java.util.Collections;
import java.util.Deque;
@@ -540,6 +541,14 @@ public abstract class Closure<V> extends
GroovyObjectSupport implements Cloneabl
Method target = null;
if (arguments.length == 1 && override.oneArg != null) {
target = override.oneArg;
+ } else if (arguments.length == 1 && override.oneArgTyped != null
&& override.oneArgGuard.isInstance(arguments[0])) {
+ // GROOVY-12164: a closure with a typed parameter declares
call(T)/doCall(T) rather
+ // than call(Object), which the lookup above cannot see --
without this branch every
+ // such call pays full metaclass dispatch. An argument that is
already an instance of
+ // the declared type dispatches directly; anything that needs
Groovy coercion
+ // (GString -> String, number conversions, null) falls through
to the metaclass below,
+ // which coerces exactly as before.
+ target = override.oneArgTyped;
} else if (arguments.length == 0 && override.zeroArg != null) {
target = override.zeroArg;
}
@@ -1378,7 +1387,7 @@ public abstract class Closure<V> extends
GroovyObjectSupport implements Cloneabl
/**
* Marker instance indicating that no call override exists.
*/
- static final CallOverride NONE = new CallOverride(null, null);
+ static final CallOverride NONE = new CallOverride(null, null, null);
/**
* Cached zero-argument call override.
*/
@@ -1387,10 +1396,25 @@ public abstract class Closure<V> extends
GroovyObjectSupport implements Cloneabl
* Cached single-argument call override.
*/
final Method oneArg;
+ /**
+ * Cached single-argument call override with a declared (non-Object)
parameter type,
+ * used only when {@link #oneArg} is absent (GROOVY-12164). A closure
literal with a
+ * typed parameter generates {@code call(T)}/{@code doCall(T)} instead
of
+ * {@code call(Object)}.
+ */
+ final Method oneArgTyped;
+ /**
+ * Instance-of guard for {@link #oneArgTyped}: the declared parameter
type (boxed for a
+ * primitive). An argument that is not already an instance needs
Groovy coercion, which
+ * only the metaclass fallback provides.
+ */
+ final Class<?> oneArgGuard;
- private CallOverride(Method zeroArg, Method oneArg) {
+ private CallOverride(Method zeroArg, Method oneArg, Method
oneArgTyped) {
this.zeroArg = zeroArg;
this.oneArg = oneArg;
+ this.oneArgTyped = oneArgTyped;
+ this.oneArgGuard = (oneArgTyped == null) ? null :
wrapperOf(oneArgTyped.getParameterTypes()[0]);
}
/**
@@ -1403,14 +1427,17 @@ public abstract class Closure<V> extends
GroovyObjectSupport implements Cloneabl
if (type == Closure.class) return NONE;
Method zero = findOverride(type);
Method one = findOverride(type, Object.class);
- if (zero == null && one == null) return NONE;
- return new CallOverride(zero, one);
+ Method typed = (one == null) ? findTypedOneArgOverride(type) :
null;
+ if (zero == null && one == null && typed == null) return NONE;
+ return new CallOverride(zero, one, typed);
}
private static Method findOverride(Class<?> type, Class<?>... params) {
try {
Method m = type.getMethod("call", params);
- if (m.getDeclaringClass() == Closure.class) return null;
+ // a static call(...) is not an instance override: reflective
invocation would
+ // silently ignore the receiver, where the MOP fallback
dispatches instance doCall
+ if (m.getDeclaringClass() == Closure.class ||
Modifier.isStatic(m.getModifiers())) return null;
try {
m.setAccessible(true);
} catch (RuntimeException ignored) {
@@ -1422,5 +1449,54 @@ public abstract class Closure<V> extends
GroovyObjectSupport implements Cloneabl
return null;
}
}
+
+ /**
+ * Finds the single unambiguous one-argument {@code call} override
with a declared
+ * (non-Object) parameter type (GROOVY-12164). Array-typed parameters
are skipped —
+ * that shape belongs to vararg collection, which is metaclass work —
as are bridge
+ * methods (their erased twin is the real override) and overloaded
typed overrides
+ * (ambiguous: the metaclass performs the selection).
+ *
+ * @param type the closure subclass
+ * @return the override, or null when absent, ambiguous, or
inaccessible
+ */
+ private static Method findTypedOneArgOverride(Class<?> type) {
+ Method candidate = null;
+ for (Method m : type.getMethods()) {
+ // getMethods() includes public statics: a static call(T) is
not an instance
+ // override (reflective invocation would ignore the receiver),
so skip it and
+ // let the MOP fallback dispatch instance doCall as before
+ if (m.getParameterCount() != 1 || !"call".equals(m.getName())
|| m.isBridge()
+ || Modifier.isStatic(m.getModifiers()) ||
m.getDeclaringClass() == Closure.class) {
+ continue;
+ }
+ Class<?> param = m.getParameterTypes()[0];
+ if (param == Object.class || param.isArray()) continue;
+ if (candidate != null) return null; // overloaded: leave
selection to the metaclass
+ candidate = m;
+ }
+ if (candidate != null) {
+ try {
+ candidate.setAccessible(true);
+ } catch (RuntimeException ignored) {
+ // module/package access denied; fall through to MOP doCall
+ return null;
+ }
+ }
+ return candidate;
+ }
+
+ private static Class<?> wrapperOf(Class<?> type) {
+ if (!type.isPrimitive()) return type;
+ if (type == int.class) return Integer.class;
+ if (type == long.class) return Long.class;
+ if (type == boolean.class) return Boolean.class;
+ if (type == double.class) return Double.class;
+ if (type == char.class) return Character.class;
+ if (type == byte.class) return Byte.class;
+ if (type == short.class) return Short.class;
+ if (type == float.class) return Float.class;
+ return type; // void: unreachable for a parameter type
+ }
}
}
diff --git a/src/test/groovy/bugs/Groovy12164.groovy
b/src/test/groovy/bugs/Groovy12164.groovy
new file mode 100644
index 0000000000..a91b80e060
--- /dev/null
+++ b/src/test/groovy/bugs/Groovy12164.groovy
@@ -0,0 +1,186 @@
+/*
+ * 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 bugs
+
+import org.junit.jupiter.api.Test
+
+import static groovy.test.GroovyAssert.assertScript
+
+/**
+ * {@code Closure.call}'s cached fast path (GROOVY-11911) only recognised a
+ * {@code call(Object)} override, so a closure with a <em>typed</em> parameter
—
+ * which generates {@code call(T)}/{@code doCall(T)} — fell back to full
+ * metaclass dispatch on every call. GROOVY-12164 extends the cache with a
+ * guarded typed override: an argument already an instance of the declared type
+ * dispatches directly; anything needing Groovy coercion (GString to String,
+ * number conversions, null) still goes through the metaclass. These tests pin
+ * the behaviour either side of that guard — results must be indistinguishable
+ * from metaclass dispatch. Iteration goes through DGM, whose Java code calls
+ * {@code closure.call(item)}, the path the cache serves.
+ */
+final class Groovy12164 {
+
+ @Test
+ void testTypedParameterViaIteration() {
+ assertScript '''
+ int sum = 0
+ [1, 2, 3, 4].each { Integer x -> sum += x }
+ assert sum == 10
+ assert [1, 2, 3].collect { Integer x -> x * x } == [1, 4, 9]
+ '''
+ }
+
+ @Test
+ void testTypedParameterDirectCall() {
+ assertScript '''
+ Closure<Integer> c = { Integer x -> x + 1 }
+ assert c.call(41) == 42
+ assert c(41) == 42
+ '''
+ }
+
+ @Test
+ void testNullArgumentStillDispatches() {
+ // null is not an instance of the declared type: must fall through to
the
+ // metaclass and reach the body as null, exactly as before
+ assertScript '''
+ def seen = []
+ [null, 2].each { Integer x -> seen << x }
+ assert seen == [null, 2]
+ '''
+ }
+
+ @Test
+ void testCoercionStillApplies() {
+ assertScript '''
+ // GString -> String coercion happens on the metaclass path
+ def name = 'world'
+ def result = ["hello ${name}"].collect { String s ->
s.toUpperCase() }
+ assert result == ['HELLO WORLD']
+ assert result[0] instanceof String
+ '''
+ }
+
+ @Test
+ void testNumberConversionMatchesMetaclassDispatch() {
+ assertScript '''
+ import groovy.test.GroovyAssert
+ // The metaclass coerces some number shapes (BigDecimal literal to
a double
+ // parameter) and rejects others (Long to an int parameter) with a
+ // MissingMethodException. The typed guard declines both to the
metaclass,
+ // so each keeps its pre-existing outcome.
+ def seen = []
+ [1.5, 2.5].each { double x -> seen << x }
+ assert seen == [1.5d, 2.5d]
+ GroovyAssert.shouldFail(MissingMethodException) {
+ [1L].each { int x -> }
+ }
+ '''
+ }
+
+ @Test
+ void testPrimitiveParameterExactMatch() {
+ // an Integer argument to an int parameter passes the (boxed) guard
and unboxes
+ assertScript '''
+ int sum = 0
+ [1, 2, 3].each { int x -> sum += x }
+ assert sum == 6
+ '''
+ }
+
+ @Test
+ void testDefaultParameterValues() {
+ // default values generate overloaded doCall/call: the guard must not
disturb selection
+ assertScript '''
+ def c = { Integer x, Integer y = 10 -> x + y }
+ assert c.call(1) == 11
+ assert c.call(1, 2) == 3
+ '''
+ }
+
+ @Test
+ void testVarargClosureCollection() {
+ // an array-typed parameter is vararg collection, which stays
metaclass work
+ assertScript '''
+ def c = { Object... items -> items.size() }
+ assert c.call('a') == 1
+ assert c.call('a', 'b') == 2
+ '''
+ }
+
+ @Test
+ void testMultiParameterDestructuring() {
+ // a single List argument to a two-parameter closure destructures via
the metaclass
+ assertScript '''
+ def seen = []
+ [[1, 'a'], [2, 'b']].each { Integer n, String s -> seen <<
"$n$s".toString() }
+ assert seen == ['1a', '2b']
+ '''
+ }
+
+ @Test
+ void testTypedSubclassOverride() {
+ // a Closure subclass with a typed call override is served by the same
cache
+ assertScript '''
+ class Doubler extends Closure<Integer> {
+ Doubler() { super(null) }
+ Integer call(Integer x) { x * 2 }
+ }
+ Closure<Integer> d = new Doubler()
+ assert d.call(21) == 42
+ assert [1, 2, 3].collect(d) == [2, 4, 6]
+ '''
+ }
+
+ @Test
+ void testStaticCallDeclarationIsNotAnOverride() {
+ // A public static call(T) — or static call(Object) — is not an
instance override:
+ // the cached fast path must skip it (reflective invocation would
silently ignore the
+ // receiver) and keep dispatching instance doCall via the metaclass.
Exercised through
+ // DGM iteration, the Closure.call(Object...) entry point the cache
serves; a direct
+ // c.call(x) from Groovy code is metaclass dispatch, where selecting a
static is legal.
+ assertScript '''
+ class TypedStatic extends Closure<String> {
+ TypedStatic() { super(null) }
+ static String call(Integer x) { 'static' }
+ String doCall(Integer x) { 'instance' }
+ }
+ class ObjectStatic extends Closure<String> {
+ ObjectStatic() { super(null) }
+ static String call(Object x) { 'static' }
+ String doCall(Object x) { 'instance' }
+ }
+ assert [1].collect(new TypedStatic()) == ['instance']
+ assert [1].collect(new ObjectStatic()) == ['instance']
+ '''
+ }
+
+ @Test
+ void testExceptionPropagation() {
+ // an exception thrown by the typed body surfaces unwrapped
+ assertScript '''
+ import groovy.test.GroovyAssert
+ def boom = { Integer x -> throw new IllegalStateException('boom '
+ x) }
+ def e = GroovyAssert.shouldFail(IllegalStateException) {
+ [7].each(boom)
+ }
+ assert e.message == 'boom 7'
+ '''
+ }
+}
