[
https://issues.apache.org/jira/browse/GROOVY-12093?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18090783#comment-18090783
]
ASF GitHub Bot commented on GROOVY-12093:
-----------------------------------------
Copilot commented on code in PR #2618:
URL: https://github.com/apache/groovy/pull/2618#discussion_r3456806143
##########
src/test/groovy/org/codehaus/groovy/transform/traitx/TraitStaticDispatchMatrix.groovy:
##########
@@ -0,0 +1,543 @@
+/*
+ * 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.
+ */
+
+// Trait-static dispatch behaviour matrix — the executable companion to
+// GEP-22 § Static members. Three plain-class baseline rows (A/B/C), the
+// 12 design rows from GEP-22-progression-analysis.html (with row 11
+// expanded to rows 11/11c/11esc covering the trait-shadows-superclass
+// quadrant + the T.super.m() escape; row 9 paired with row 9acc covering
+// the static-field non-goal and its accessor workaround), and four
+// @Anchored-marker rows characterising the GROOVY-12093 proposal.
+//
+// Runs against Groovy 4 and 6 either:
+// * via gradle (`./gradlew :test --tests '*.TraitStaticDispatchMatrix'`)
+// * standalone, against any SDKMAN-installed Groovy:
+// `groovy src/test/groovy/.../traitx/TraitStaticDispatchMatrix.groovy`
+//
+// Two kinds of test:
+// * Matrix tests assert OBSERVED behaviour. They pass on every
+// currently-shipping Groovy that delivers the spec behaviour and turn
+// RED on releases that deviate (e.g. rows 1/10/12 on Groovy 5.0.0–
+// 5.0.6 / 6.0.0-alpha-1, which carry the GROOVY-8854 regression
+// corrected by GROOVY-11985 in 5.0.7 / 6.0.0-alpha-2). That red is
+// the intended bug-detector signal, not a defect in the test.
+// * @NotYetImplemented tests assert GROOVY-12093 desired-end-state
+// behaviour unmet on every currently-shipping Groovy (the four
+// @Anchored variants 1a/7a/8a/13). Each references
+// `groovy.transform.Anchored` by import; on any Groovy without the
+// annotation the import fails and the test body throws, which
+// @NotYetImplemented swallows. On a Groovy built with the spike
+// (branch `spike/anchored-annotation`), the assertions hold and the
+// NYI expectation flips red — the cue to drop @NotYetImplemented.
Review Comment:
The header comment still describes the `@Anchored` rows as
`@NotYetImplemented` tests that only work on a spike branch. In this PR,
`@Anchored` is added and the rows are real assertions, so this description is
misleading for future maintainers.
##########
src/test/groovy/org/codehaus/groovy/transform/traitx/Groovy12093.groovy:
##########
@@ -0,0 +1,249 @@
+/*
+ * 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.traitx
+
+import org.junit.Test
+
+import static groovy.test.GroovyAssert.assertScript
+import static groovy.test.GroovyAssert.shouldFail
+
+/**
+ * Exercises the {@code @Anchored} marker on trait static methods
(GROOVY-12093).
+ *
+ * <p>The marker selects declarer-bound dispatch (Java/interface-static
+ * flavour) in place of the default override-flavour dispatch, and (by
+ * default) also promotes the annotated method onto the generated trait
+ * interface.
+ *
+ * <pre>
+ * plain static m() -> override flavour (dispatch via
+ * implementer; Grails Validateable
+ * canary; Groovy 4.0.32 + post-#2529)
+ * {@literal @}Anchored static m() -> declarer-bound dispatch +
on the
+ * generated trait interface
+ * {@literal @}Anchored(inInterface=false)
+ * static m() -> declarer-bound dispatch only, not
+ * published on the interface
+ * </pre>
+ *
+ * Dispatch is implemented in {@code TraitReceiverTransformer}; interface
+ * promotion is implemented in {@code TraitASTTransformation}.
+ */
+final class Groovy12093 {
+
+ // Basic-case coverage lives in TraitStaticDispatchMatrix:
+ // * plain-static override visible to trait body → matrix row 1
+ // * @Anchored makes dispatch trait-anchored → matrix row 1a
+ // * @Anchored from-trait T.m() → matrix row 7a
+ // * @Anchored external Trait.m() → matrix row 8a
+ // * @Anchored(inInterface=false) opt-out → matrix row 13
+ // This class covers the @Anchored-specific tests that the matrix
+ // doesn't carry: per-callee keying, @CompileStatic variants, the
+ // inInterface=false STC compile-error case, and validation that
+ // @Anchored is rejected on non-public-static targets.
+
+ /**
+ * The marker is keyed on the <em>callee</em>, not the caller — same
+ * discipline as the existing private-static escape.
+ */
+ @Test
+ void anchored_isKeyedOnCallee() {
+ assertScript '''
+ import groovy.transform.Anchored
+
+ trait V {
+ static boolean overridable() { false }
+ @Anchored
+ static boolean anchored() { false }
+ // Same caller, different callees: result depends on each
+ // callee's own marker, not the caller's.
+ static List<Boolean> bothSeen() { [this.overridable(),
this.anchored()] }
+ }
+ class Over implements V {
+ static boolean overridable() { true }
+ static boolean anchored() { true }
+ }
+ assert Over.bothSeen() == [true, false]
+ '''
+ }
+
+ /** Plain {@code static} inside {@code @CompileStatic} still routes via
the implementer. */
+ @Test
+ void plainStatic_compileStatic_overrideVisible() {
+ assertScript '''
+ @groovy.transform.CompileStatic
+ trait V {
+ static String name() { 'trait' }
+ static String seen() { this.name() }
+ }
+ @groovy.transform.CompileStatic
+ class Impl implements V { static String name() { 'impl' } }
+
+ assert Impl.seen() == 'impl'
+ '''
+ }
+
+ /** {@code @Anchored} inside {@code @CompileStatic} stays trait-anchored.
*/
+ @Test
+ void anchored_compileStatic_traitAnchored() {
+ assertScript '''
+ import groovy.transform.Anchored
+
+ @groovy.transform.CompileStatic
+ trait V {
+ @Anchored
+ static String name() { 'trait' }
+ static String seen() { this.name() }
+ }
+ @groovy.transform.CompileStatic
+ class Impl implements V { static String name() { 'impl' } }
+
+ assert Impl.seen() == 'trait'
+ '''
+ }
+
+ /**
+ * STC sanity check: external {@code V.name()} from a {@code
@CompileStatic}
+ * caller type-checks and runs — the interface static is visible to the
+ * static type checker, not just to dynamic dispatch.
+ */
+ @Test
+ void anchored_compileStatic_externalTraitDotM_typeChecks() {
+ assertScript '''
+ import groovy.transform.Anchored
+ import groovy.transform.CompileStatic
+
+ trait V {
+ @Anchored
+ static String name() { 'trait' }
+ }
+ class Impl implements V { }
+
+ @CompileStatic
+ static String callExternally() {
+ V.name() // must type-check against the interface static
+ }
+
+ assert callExternally() == 'trait'
+ '''
+ }
+
+ /**
+ * STC sanity check: from-trait {@code V.m()} from inside a
+ * {@code @CompileStatic} trait body also type-checks against the
+ * interface static.
+ */
+ @Test
+ void anchored_compileStatic_fromTraitT_DotM_typeChecks() {
+ assertScript '''
+ import groovy.transform.Anchored
+
+ @groovy.transform.CompileStatic
+ trait V {
+ @Anchored
+ static String name() { 'trait' }
+ static String forced() { V.name() } // trait-qualified,
under @CS
+ }
+ @groovy.transform.CompileStatic
+ class Over implements V { static String name() { 'over' } }
+
+ assert Over.forced() == 'trait'
+ '''
+ }
+
+ /**
+ * STC sanity check: with the {@code inInterface=false} opt-out, external
+ * {@code V.name()} from a {@code @CompileStatic} caller should fail to
+ * compile — no interface static to bind against.
+ */
+ @Test
+ void anchored_inInterfaceFalse_compileStatic_externalCall_isCompileError()
{
+ def err = null
+ try {
+ new GroovyShell().evaluate '''
+ import groovy.transform.Anchored
+ import groovy.transform.CompileStatic
+
+ trait V {
+ @Anchored(inInterface=false)
+ static String name() { 'trait' }
+ }
+ class Impl implements V { }
+
+ @CompileStatic
+ static String callExternally() {
+ V.name() // not on the interface — should be a
compile error under @CS
+ }
+ callExternally()
+ '''
+ } catch (Throwable t) {
+ err = t
+ }
+ assert err != null : 'expected @CompileStatic external V.name() to
fail when inInterface=false'
+ }
Review Comment:
This is intended to assert a compile-time failure, but catching any
Throwable can let the test pass for unrelated runtime errors. Using
shouldFail(MultipleCompilationErrorsException) makes the expectation precise
and self-documenting.
##########
src/test/groovy/org/codehaus/groovy/transform/traitx/TraitStaticDispatchMatrix.groovy:
##########
@@ -0,0 +1,543 @@
+/*
+ * 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.
+ */
+
+// Trait-static dispatch behaviour matrix — the executable companion to
+// GEP-22 § Static members. Three plain-class baseline rows (A/B/C), the
+// 12 design rows from GEP-22-progression-analysis.html (with row 11
+// expanded to rows 11/11c/11esc covering the trait-shadows-superclass
+// quadrant + the T.super.m() escape; row 9 paired with row 9acc covering
+// the static-field non-goal and its accessor workaround), and four
+// @Anchored-marker rows characterising the GROOVY-12093 proposal.
+//
+// Runs against Groovy 4 and 6 either:
+// * via gradle (`./gradlew :test --tests '*.TraitStaticDispatchMatrix'`)
+// * standalone, against any SDKMAN-installed Groovy:
+// `groovy src/test/groovy/.../traitx/TraitStaticDispatchMatrix.groovy`
+//
+// Two kinds of test:
+// * Matrix tests assert OBSERVED behaviour. They pass on every
+// currently-shipping Groovy that delivers the spec behaviour and turn
+// RED on releases that deviate (e.g. rows 1/10/12 on Groovy 5.0.0–
+// 5.0.6 / 6.0.0-alpha-1, which carry the GROOVY-8854 regression
+// corrected by GROOVY-11985 in 5.0.7 / 6.0.0-alpha-2). That red is
+// the intended bug-detector signal, not a defect in the test.
+// * @NotYetImplemented tests assert GROOVY-12093 desired-end-state
+// behaviour unmet on every currently-shipping Groovy (the four
+// @Anchored variants 1a/7a/8a/13). Each references
+// `groovy.transform.Anchored` by import; on any Groovy without the
+// annotation the import fails and the test body throws, which
+// @NotYetImplemented swallows. On a Groovy built with the spike
+// (branch `spike/anchored-annotation`), the assertions hold and the
+// NYI expectation flips red — the cue to drop @NotYetImplemented.
+//
+// Verdicts encoded here are PROPOSED PENDING TEAM REVIEW where the
+// underlying design point is not yet ratified (the four @Anchored rows
+// in particular).
+
+package org.codehaus.groovy.transform.traitx
+
+import groovy.test.GroovyAssert
+import groovy.test.NotYetImplemented
+import org.junit.Test
+import org.junit.runner.JUnitCore
+
+class TraitStaticDispatchMatrix {
+
+ /** Major version of the running Groovy: 4, 5, 6, ... */
+ static final int MAJOR = GroovySystem.version.tokenize('.')[0] as int
+
+ /** Compile+run a snippet with the *running* Groovy compiler. */
+ private static Object ev(String src) { new GroovyShell().evaluate(src) }
+
+ // =================== PLAIN-CLASS BASELINE (NO TRAITS) ===================
+ // Establishes what plain Groovy does for the same dispatch shapes the
+ // trait rows below test. Read these first — the trait rows characterise
+ // how trait machinery DEPARTS from this baseline, not how it matches it.
+ //
+ // VERIFIED on 4.0.32: plain Groovy static dispatch is declarer-bound in
+ // every context (method body, closure body, dynamic, @CompileStatic);
+ // only INSTANCE methods are polymorphic. Trait row 1's "override visible
+ // to trait method body" is therefore a deliberate departure from this
+ // baseline (the per-implementer $static$self mechanism); the row 2
+ // closure carve-out keeps trait closure-context dispatch consistent
+ // with this baseline. The simplified-model RULE wording "dispatch like
+ // a plain class member" was an aspirational fiction — the empirical
+ // truth is more nuanced and the baseline below records it.
+
+ // --
> Static method override on trait implementer ignored when called via this in
> trait body (cont'd)
> -----------------------------------------------------------------------------------------------
>
> Key: GROOVY-12093
> URL: https://issues.apache.org/jira/browse/GROOVY-12093
> Project: Groovy
> Issue Type: Bug
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> _This is a carry on of GROOVY-11985. It's description is included below._
> Analysis by AI of the description in the Grails canary build comments yielded
> the following:
> h2. Summary
> In Groovy 4.x, calling {{this.someStaticMethod()}} from inside a trait's
> static method body dispatched dynamically and honoured a static method
> override declared on the implementing class. In Groovy 5.x and
> 6.0.0-SNAPSHOT, the same call is rewritten by {{TraitReceiverTransformer}} to
> dispatch through the trait helper, which can never see the implementing-class
> override. The override is silently lost — no exception, no compile warning,
> the trait's default value just always wins.
> This was discovered as part of the Grails 8 / Groovy 5 migration
> (apache/grails-core PRs
> [#15557|https://github.com/apache/grails-core/pull/15557] and
> [#15558|https://github.com/apache/grails-core/pull/15558]) and motivated a
> reflection-based workaround in
> {{{}Validateable.resolveDefaultNullable(Class){}}}.
> h2. Reproducer
> Standalone repro:
> [https://github.com/jamesfredley/groovy-trait-static-method-override-bug]
> {code:java|title=Validateable.groovy}
> trait Validateable {
> static boolean defaultNullable() {
> false
> }
> static boolean defaultNullableSeenByTrait() {
> // expected to dispatch to the implementing class override
> this.defaultNullable()
> }
> }
> {code}
> {code:java|title=MyNullableValidateable.groovy}
> class MyNullableValidateable implements Validateable {
> static boolean defaultNullable() {
> true
> }
> }
> {code}
> {code:java|title=Driver}
> assert MyNullableValidateable.defaultNullable() // direct call:
> true on every version
> assert MyNullableValidateable.defaultNullableSeenByTrait() // expected true;
> gets false on 5.x / 6.0
> {code}
> h2. Observed behaviour
> ||Groovy version||Direct call||{{this.defaultNullable()}} from trait body||
> |4.0.27|true (PASS)|true (PASS)|
> |5.0.5|true (PASS)|*false (FAIL)*|
> |6.0.0-SNAPSHOT|true (PASS)|*false (FAIL)*|
> h2. Root cause
> The bytecode emitted for the trait helper's
> {{defaultNullableSeenByTrait(Class)}} method changed shape:
> {noformat}
> // Groovy 4.0.27 // Groovy 5.0.5 /
> 6.0.0-SNAPSHOT
> 0: aload_0 0: ldc //
> Validateable$Trait$Helper.class
> 1: invokedynamic invoke: 2: aload_0
> (Ljava/lang/Class;)Ljava/lang/Object; 3: invokedynamic invoke:
>
> (Ljava/lang/Class;Ljava/lang/Class;)Ljava/lang/Object;
> {noformat}
> In 4.x the indy receiver is {{aload_0}} — the implementing class — so the
> dynamic dispatch resolves {{defaultNullable()}} against
> {{MyNullableValidateable}} and finds the override. In 5.x+ the receiver is
> hard-coded {{Validateable$Trait$Helper.class}} (an {{{}ldc{}}}) and the
> implementing class is demoted to an argument, so the dispatch resolves
> {{defaultNullable(Class)}} on the trait helper itself and always lands on the
> lowered trait default.
> The behaviour change was introduced by commit {{0aa78d0a33}} (GROOVY-8854,
> Sep 2023):
> {quote}write {{T.m(p)}} as {{this.m($static$self,p)}} not {{$self.m(p)}}
> {quote}
> That commit rewrote {{TraitReceiverTransformer.transformMethodCallOnThis}} so
> that {{this.someStaticMethod()}} inside a trait body is rewritten as {{(this
> | T$Trait$Helper).m((Class)$self.getClass(), args)}} — routed through the
> trait helper's lowered static, with the implementing class passed as the
> {{$static$self}} argument. The existing trait static method test coverage in
> {{TraitASTTransformationTest.testTraitStaticMethod}} (including the
> GROOVY-8854 case at line 2218) doesn't exercise the
> override-on-implementing-class scenario, so the regression wasn't caught.
> h2. Tradeoff
> Groovy 5's behaviour is arguably closer to Java semantics: static methods are
> not virtual, and {{this}} inside a Java static method doesn't exist, so
> "{{{}this.staticMethod(){}}} virtually dispatches to the implementing class's
> static" was always Groovy-specific magic that relied on MOP. However:
> * The Groovy 4 behaviour was depended on by real code — Grails
> {{Validateable}} is the visible canary; the same idiom likely exists
> elsewhere.
> * The failure mode is silent — no exception, no compile warning, the trait
> default just wins every time.
> * The change surfaced as a side effect of GROOVY-8854 (whose ticket was
> about something else), not as a deliberate "trait statics are no longer
> virtual" decision, and it isn't called out in the Groovy 5 release notes.
> h2. Suggested options
> # Restore Groovy 4 semantics for {{this.staticMethod()}} in trait bodies —
> emit a dynamic lookup whose receiver is {{$static$self}} (the implementing
> class) rather than the trait helper.
> # At minimum, emit a compile-time warning when a trait body calls a
> same-named static and the dispatch will provably not hit any override on the
> implementing class.
> # Document the new contract explicitly in the Groovy 5 release notes and
> trait docs, so consumers can adapt at the trait level instead of debugging
> silent no-ops.
> h2. Workaround (already applied in Grails)
> Bypass {{TraitReceiverTransformer}} via plain Java reflection, which it can't
> see through:
> {code:groovy}
> private static boolean resolveDefaultNullable(Class<?> clazz) {
> try {
> return clazz.getMethod('defaultNullable').invoke(null) as boolean
> } catch (NoSuchMethodException ignored) {
> return false
> }
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
