[
https://issues.apache.org/jira/browse/GROOVY-12117?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18094835#comment-18094835
]
ASF GitHub Bot commented on GROOVY-12117:
-----------------------------------------
paulk-asert commented on PR #2678:
URL: https://github.com/apache/groovy/pull/2678#issuecomment-4921186158
AI said the PR broke ordering, so I added an existing test to show that -
and the PR breaks now - but the fix should be easy.
AI read below:
---------
Thanks for this — replacing the super-trait search with a fuller helper stub
is a nice simplification, and modelling the not-yet-lowered helper as a real
stub (rather than the empty GROOVY-7909 one) is the right direction for the
code-assist/inferencing goal. It also happens to fix a latent bug the old empty
stub had: in `SuperCallTraitTransformer`, `isStatic` is derived from the
helper's methods, so `T.super.staticMethod()` in the not-yet-lowered window
previously received `this` instead of `this.class`; a populated stub makes that
window behave like the lowered case.
One behavioural concern before this lands, though: **as written, the stub
drops the original method's annotations, which makes `@Virtual` static dispatch
depend on trait transform order** — exactly the order-independence this PR's
own javadoc says it preserves.
### Why
`HelperClassStub.helperMethod` adds only `@Implemented`, whereas the real
helper method (`TraitASTTransformation.processMethod`) copies **all** of the
original method's annotations, including `@Virtual`.
`TraitReceiverTransformer.findConcreteMethod`'s result is then branched on by
`hasAnnotation(methodNode, VIRTUAL_TYPE)` to choose dynamic dispatch vs.
declarer-bound dispatch. So for `this.m(x)` where `m` is a `@Virtual public
static` on a super trait:
- super trait already lowered → real helper method carries `@Virtual` →
dynamic-dispatch path ✔
- super trait not yet lowered (stub window) → stub method lacks `@Virtual` →
declarer-bound path ✘
The removed `getDeclaredMethods` fallback returned the original `MethodNode`
with `@Virtual` intact, so it was order-independent here. The failure is silent
— the implementer's static override just stops being visible from trait code
depending on sibling-trait declaration order.
### Reproducer
This is the sub-trait-first counterpart of the existing
`Groovy11985.testSuperTraitPublicStaticIsPolymorphic`:
```groovy
@Test
void testSuperTraitPublicStaticIsPolymorphicSubTraitFirst() {
GroovyAssert.assertScript '''
import groovy.transform.Virtual
trait Mid extends Base { static String greet() { hello() } } //
sub-trait declared FIRST
trait Base { @Virtual static String hello() { 'base' } }
class C implements Mid {}
class D implements Mid { static String hello() { 'override' } }
assert C.greet() == 'base'
assert D.greet() == 'override'
'''
}
```
- Passes on the current baseline (the merged #2644 fallback returns the
original method with `@Virtual`):
https://github.com/apache/groovy/actions/runs/28987961170/job/86021221878
- Fails when this PR is applied (rebased to include the test) — `D.greet()`
returns `'base'` instead of `'override'`:
https://github.com/apache/groovy/actions/runs/28988500410
### Suggested fix
Copy the original method's annotations onto the stub method, mirroring
`processMethod` (which excludes `@Override`). No new imports needed —
`AnnotationNode` and `ClassHelper` are already imported in `Traits.java`.
```diff
var m = new MethodNode(method.getName(), mods,
method.getReturnType(), helperParams, method.getExceptions(), null);
- m.addAnnotation(Traits.IMPLEMENTED_CLASSNODE);
+ // Mirror processMethod's annotation copy: findConcreteMethod
dispatches on
+ // @Virtual, so a stub method missing it silently picks
declarer-bound dispatch.
+ for (AnnotationNode annotation : method.getAnnotations()) {
+ if
(!annotation.getClassNode().equals(ClassHelper.OVERRIDE_TYPE)) {
+ m.addAnnotation(annotation);
+ }
+ }
m.setGenericsTypes(method.getGenericsTypes());
```
(Dropping the explicit `@Implemented` is intentional: the real helper
methods don't carry it either — `processMethod` adds `@Implemented` to the
*original interface method*, not to the helper copy.)
With this change the full trait suite still passes
(`TraitASTTransformationTest`, `TraitStaticDispatchMatrix`,
`TraitGenericsMatrix`, `VirtualAnnotationTest`,
`Groovy11985/12104/12105/12106/12112`), and the reproducer above goes green.
### Optional fidelity tweaks (not required to fix the bug)
Since the stub now feeds code-assist/inferencing, a couple of small
alignments with `processMethod`/`createSelfParameter` would make stub
signatures match the lowered ones:
- self parameter: use `trait.getPlainNodeReference()` and the names `$self`
/ `$static$self` (`Traits.THIS_OBJECT` / `Traits.STATIC_THIS_OBJECT`) instead
of the raw generic node and `"traitImplementer"` — avoids leaking unbound
placeholders into `Class<T<X>>`;
- add `ACC_FINAL` for final instance methods, as `processMethod` does.
> Resolution of inherited static trait method from sub-trait body is
> transform-order dependent
> --------------------------------------------------------------------------------------------
>
> Key: GROOVY-12117
> URL: https://issues.apache.org/jira/browse/GROOVY-12117
> Project: Groovy
> Issue Type: Bug
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
> Fix For: 6.0.0-alpha-2, 5.0.7
>
>
> See: https://github.com/apache/grails-core/pull/15557#issuecomment-4833554145
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
