GROOVY-12117: replace super trait search with more complete helper stub#2678
GROOVY-12117: replace super trait search with more complete helper stub#2678eric-milles wants to merge 1 commit into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 6c85475 | Previous: 23780cf | Ratio |
|---|---|---|---|
org.apache.groovy.bench.CalibrationBench.memoryPointerChase |
975.0959011983615 us/op |
569.5932258985365 us/op |
1.71 |
This comment was automatically generated by workflow using github-action-benchmark.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2678 +/- ##
==================================================
- Coverage 68.7019% 68.7007% -0.0012%
+ Complexity 34123 34119 -4
==================================================
Files 1536 1536
Lines 128912 128923 +11
Branches 23369 23370 +1
==================================================
+ Hits 88565 88571 +6
- Misses 32517 32521 +4
- Partials 7830 7831 +1
🚀 New features to boost your workflow:
|
|
JMH summary — classic (commit
|
| Group | Speedup | Calibrated | n |
|---|---|---|---|
| bench | 1.033 × | 1.032 × | 78 |
| core | 1.023 × | 1.044 × | 77 |
| grails | 0.988 × | 1.029 × | 80 |
Runner calibration (this run vs baseline hardware): bench 1.01× (26 rulers) · core-ag 0.96× (3 rulers) · core-hz 1.01× (3 rulers) · grails-ad 0.90× (3 rulers) · grails-ez 1.01× (3 rulers)
Baseline: dev/bench/jmh/<part>/classic/data.js on gh-pages, trailing 90 days. Daily dashboard · Per-suite raw data
JMH summary — indy (commit
|
| Group | Speedup | Calibrated | n |
|---|---|---|---|
| bench | 0.992 × | 1.021 × | 78 |
| core | 3.227 × | 3.067 × | 77 |
| grails | 2.075 × | 2.373 × | 80 |
⚠️ Runner speed differs ≥15% from the historical baseline hardware for: grails-ad. Raw speedups are not meaningful for those parts — use the calibrated column.
Runner calibration (this run vs baseline hardware): bench 0.97× (26 rulers) · core-ag 1.11× (3 rulers) · core-hz 0.99× (3 rulers) · grails-ad 0.87× (3 rulers) · grails-ez 0.88× (3 rulers)
Baseline: dev/bench/jmh/<part>/indy/data.js on gh-pages, trailing 90 days. Daily dashboard · Per-suite raw data
🚨 TestLens detected 5 failed tests 🚨Here is what you can do:
Test SummaryBuild and test / lts (17, macos-latest) > :test
Build and test / lts (17, ubuntu-latest) > :test
Build and test / lts (17, windows-latest, 1) > :test
Build and test / lts (21, ubuntu-latest) > :test
Build and test / lts (25, ubuntu-latest) > :test
🏷️ Commit: 6c85475 Test FailuresGroovy11985 > testSuperTraitPublicStaticIsPolymorphicSubTraitFirst() (:test in Build and test / lts (17, macos-latest))Groovy11985 > testSuperTraitPublicStaticIsPolymorphicSubTraitFirst() (:test in Build and test / lts (17, ubuntu-latest))Groovy11985 > testSuperTraitPublicStaticIsPolymorphicSubTraitFirst() (:test in Build and test / lts (17, windows-latest, 1))Groovy11985 > testSuperTraitPublicStaticIsPolymorphicSubTraitFirst() (:test in Build and test / lts (21, ubuntu-latest))Groovy11985 > testSuperTraitPublicStaticIsPolymorphicSubTraitFirst() (:test in Build and test / lts (25, ubuntu-latest))Muted TestsSelect tests to mute in this pull request:
Reuse successful test results:
Click the checkbox to trigger a rerun:
Learn more about TestLens at testlens.app. |
|
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 One behavioural concern before this lands, though: as written, the stub drops the original method's annotations, which makes Why
The removed ReproducerThis is the sub-trait-first counterpart of the existing @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'
'''
}
Suggested fixCopy the original method's annotations onto the stub method, mirroring 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 With this change the full trait suite still passes ( Optional fidelity tweaks (not required to fix the bug)Since the stub now feeds code-assist/inferencing, a couple of small alignments with
|
3 participants
In order to support some code assist or type inferencing for traits, I needed to fill out the trait helper stub a little more. This solution also works for GROOVY-12117 instead of the super trait method search. That is why I could not get the 12117 test case to trip locally.