[
https://issues.apache.org/jira/browse/GROOVY-11985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18080576#comment-18080576
]
ASF GitHub Bot commented on GROOVY-11985:
-----------------------------------------
eric-milles commented on PR #2529:
URL: https://github.com/apache/groovy/pull/2529#issuecomment-4440849519
I was able to try this out yesterday (internet outage) and the change is in
the spot that I would expect. The difficulty I have is reasoning about and
explaining to others why "this.m(x)" can be rewritten to 3 different forms:
* `($static$self or $self.getClass()).m(x)`
* `(this or T$Trait$Helper).m($self or $static$self or
(Class)$self.getClass(), x)`
* `(this or $self or $static$self).m(x)`
I did see that between Groovy 3 and Groovy 4, the behavior was change for
private static methods. Then between Groovy 4 and 5 there was a change for the
"override" or "cannot be replaced by trait method" case. I'm still on the
fence about allowing dynamic dispatch so the seen trait method is not the one
called in all cases.
> 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)
