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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
