[
https://issues.apache.org/jira/browse/GROOVY-11985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18077901#comment-18077901
]
Jochen Theodorou commented on GROOVY-11985:
-------------------------------------------
[~paulk] of course I recognized this being AI written text right away. Of
course I did not know you just wanted to capture the current state. Anyway..
yes, traits are different to what Java has, but as what should we understand
them?
And after thinking about this again I think I actually changed my mind. A trait
is not an interface with default methods, the method ordering for what is
realized in class is different. Basically in Java you have current class,
super classes, interfaces default methods as "stack". Traits are more like
current class, traits, super classes, interface default methods. And if we see
those methods as "mixed in" or like "as if they are in the class itself", then
the MyNullableValidateable method must be taken. More specifically a trait has
no "this", neither static nor virtual. So it is probably best to see it not as
a class of some kind.
But we do have a conceptual problem here I think, that is private static
methods. What I would want is for a private static method in the trait to be
callable from within the trait, regardless of what the class defines.
{code:Java}
trait Me {
static foo(x){bar(x)}
private static bar(String x) {"t"}
}
class Super {
static bar(Integer x) {"s"}
}
class Current extends Super implements Me{
static bar(x){"c"}
}
assert Current.foo("") == "t"
assert Current.foo(1) == "s"
assert Current.foo(null) == "c"
[code}
I used static here, but this should actually be the same for static and virtual
method cases. I somehow highly doubt we are doing that right Groovy 4 fails
here compilation because of mixing private and public methods in the same
class, Groovy 5 compiles just to fail at runtime. I think this needs
discussion. The error message is right for the if that is in the same class,
and if a trait is kind of in the same class, then this should not be allowed.
On the other hand we probably want a private trait method to be only on the
trait, not the class.
If I make the bar method in Current also private I have kind of an overload
case, but then execution fails at runtime. In this case then the private trait
method is part of the trait and the bar call is bound to the trait. I mean it
makes sense that the trait cannot call private methods of the class or super
methods, it is just against "as if part of the class" concept. But it is not as
easy actually.
{code:groovy}
trait Me {
static foo(x){bar(x)}
private static bar(String x) {"t"}
}
class Super {
static bar(Integer x) {"s"}
}
class Current extends Super implements Me{
private static bar(x){"c"}
}
assert Current.foo(null) == "t" trait Me {
static foo(x){bar(x)}
private static bar(String x) {"t"}
}
class Super {
static bar(Integer x) {"s"}
}
class Current extends Super implements Me{
private static bar(x){"c"}
}
assert Current.foo(null) == "t"
{code}
here Groovy 4 dispatched bar(null) to the trait method, if bar in the trait and
in Current is public, we dispatch to "c", if the trait method is public and
there is no bar method in Current, we dispatch to the trait method. If we
remove the bar method from the trait and Current, we dispatch to Super#bar. if
all the bar methods had the same signature I would say that this follows
perfectly the method ordering, but these are quasi overloads, and then this
looks not always correct.
> 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)
