[
https://issues.apache.org/jira/browse/GROOVY-11985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18077905#comment-18077905
]
Eric Milles commented on GROOVY-11985:
--------------------------------------
I have not digested all the back and forth, but the use of the term override
for static methods seems suspect. I really wish traits never added static for
its members, because it causes confusion as to how the trait is like a template
(more or less).
When adding static to the mix, it makes reasoning about use of trait members
more difficult, since the trait class itself retains none of the static
members. Thus "trait T \{ static p \}" does not allow "T.p" from anywhere.
Within the trait class code and within an implementer, it gets to use p, but
elsewhere it is erased (relocated).
In this specific instance, if you want allow {{defaultNullable()}} to be
overridden, then it should not be static. Essentially static implies final.
> 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)
