[
https://issues.apache.org/jira/browse/GROOVY-11985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18080617#comment-18080617
]
Paul King commented on GROOVY-11985:
------------------------------------
Claude's response:
[~emilles] Thanks for taking it for a spin. On both points:
*On the field-ref parallel* — yes, same shape as GROOVY-11907 / GROOVY-7255:
route through the implementer's runtime class, mark {{DO_DYNAMIC}} so static
type checking doesn't reject the dispatched call. Two things beyond the marker
though:
* The guards ({{!isPrivate && !inClosure}}) keep private statics and closure
context on the helper path. Private statics aren't composed onto the
implementer so the override pattern doesn't apply; closure bodies need the
existing {{classX(traitHelper)}} re-rooting for delegation/type-checking.
* The argument list drops the synthetic {{$self}} / {{Class}} first arg because
we're calling the implementer's natural signature directly, not the helper's
{{(Class, ...)}} shape.
So it's the static-method analogue of the field fix, not literally the same
transform.
*On the three forms* — agreed it's hard to explain in the abstract. Keyed on
properties of the _callee_ (and call-site context) it's actually a decision
tree, not three alternatives:
||#||{{m}} found in trait?||static/instance||visibility||call site||rewritten
form||impl override wins?||
|1|yes|static|public|trait method body|{{((Class)$self.getClass()).m(args)}} +
{{DO_DYNAMIC}}|*yes*|
|2|yes|static|public|inside closure|{{T$Helper.m($static$self, args)}}|no|
|3|yes|static|private|anywhere|{{T$Helper.m($static$self, args)}}|no|
|4|no (only on impl/super)|static|—|trait method body|{{$static$self.m(args)}}
(fallback)|*yes*|
|5|no|static|—|inside closure|{{$static$self.m(args)}} via {{thisExpr}}|*yes*|
|6|yes|instance|any|trait method body|{{this.m($self, args)}} (existing)|*yes*
at runtime|
Row 1 is the only new behaviour this PR introduces. Rows 4–6 were already
dynamic pre-PR — instance methods have always dispatched through {{$self}}, and
not-found names have always fallen through to {{$static$self}}. Rows 2/3 are
the deliberately conservative cells: closures stay on the helper for
compile-time type-checking of trait closure bodies, and private statics stay on
the helper because they aren't overridable to begin with.
*On the "on the fence" point* — the asymmetry argument cuts the other way for
me. Pre-PR, instance methods (row 6) already let the implementer's override
win; only public statics didn't. Row 1 just removes a special case so public
statics behave like instance methods. Detecting "implementer actually
overrides" at trait-transform time isn't generally possible — the implementer
set isn't known then — so runtime dispatch is the conservative choice and
matches what instance methods already do.
The remaining sharp edge is one [~blackdrag] raised on this issue: in {{B
extends A implements T}}, even with row 1's dynamic dispatch, the dispatch
anchors on the _direct implementer_ ({{A}}), not the receiver class ({{B}}),
because trait composition is fixed at {{A}}. So {{B.m1()}} still picks {{A}}'s
{{m2}}, not {{B}}'s, unless {{B}} also redeclares {{m1}}. That's a separate
documentation point but worth flagging — I'll add it to the GEP text.
> Static method override on trait implementer ignored when called via this in
> trait body
> --------------------------------------------------------------------------------------
>
> Key: GROOVY-11985
> URL: https://issues.apache.org/jira/browse/GROOVY-11985
> Project: Groovy
> Issue Type: Bug
> Reporter: Paul King
> Priority: Major
>
> 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)
