GROOVY-11907: SC: trait static field helper generates invalid bytecod…

[paulk-asert]

…e when method-level DYNAMIC_RESOLUTION is present (GROOVY-11817 regression)

[codecov-commenter]

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.1307%. Comparing base (9941325) to head (ecc4df7).
⚠️ Report is 1 commits behind head on GROOVY_5_0_X.

Files with missing lines	Patch %	Lines
...oovy/transform/trait/TraitReceiverTransformer.java	66.6667%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@                  Coverage Diff                   @@
##             GROOVY_5_0_X      #2443        +/-   ##
======================================================
- Coverage         67.1318%   67.1307%   -0.0012%     
- Complexity          29431      29433         +2     
======================================================
  Files                1382       1382                
  Lines              116800     116805         +5     
  Branches            20473      20475         +2     
======================================================
+ Hits                78410      78412         +2     
- Misses              31905      31907         +2     
- Partials             6485       6486         +1     

Files with missing lines	Coverage Δ
...oovy/transform/trait/TraitReceiverTransformer.java	88.8889% <66.6667%> (-0.8408%)	⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[eric-milles]

I'll see if I can get to this today. Is there a description of the invalid bytecode? There is a lot in the linked issue, but I never saw the build or runtime output of a failure.

[eric-milles]

I don't get any failure for the test given. What is the config script supposed to change about the compilation?

[paulk-asert]

Most of the details are in comments on James Fredley's links about Grails and Groovy 5/6:

apache/grails-core#15557
apache/grails-core#15558

I can't see it there now, but there is lots there, I wonder if he has updated the content. In any case, I check out grails-core, switched to the grails8-groovy5-sb4 branch, changed groovy.version in dependencies.gradle from 5.0.3 to 5.0.4, and ran:

./gradlew :grails-geb:compileTestFixturesGroovy -S

I don't know whether that branch has been changed too. The last commit for me was:

7896ef8743 fix(grails-gradle): skip missing input dirs in mergeTestReports

That reproduces the error. To verify the fix, I had to tweak the build slightly to point to mavenLocal() and use my own local version (different to what is in the Apache snapshot repo) - being careful to clear the gradle cache. I can send you the diff I have to the build files if you want to replicate the fix part. The bug doesn't need build changes apart from changing 5.0.3 to 5.0.4.

[paulk-asert]

The config script simulates what happens when Spock is on the classpath. This part buried in the comment son James' issue:

grails-plugin Gradle plugin + Spock + the trait pattern

I had AI really analyse what Spock, the grails plugin and Groovy were doing and it couldn't see anything being incorrectly set up on the Spock or grails side but it found the "problem" on the Groovy side. I copied its main summary of findings back into the issue but "... AI makes mistakes ...". It seems logical to me but another set of human eyes would be good.

[eric-milles]

I set up your test case, including the config script and it runs fine for me with 5.0.5. So I can't say that the fix is a good one or if there's a better place to make a change. I don't know why "$static$self.getClass()" would be such a problem for classgen.

Side note: Did you know about the instanceof vs checkcast? That's news to me; we have 100s of instanceof tests.

[eric-milles]

Does the checkcast refer to the instanceof-or case (7971)? There is a fix for that in the 5 branch now.

[paulk-asert]

Hmm, test passes for me too with the fix backed out. But the fix does work on the grails branch. I'll try to tweak the test.

[paulk-asert]

AI updated the test. Now fails without the fix but still needs a sanity check.

[eric-milles]

I'm going to try with spock; I'm in the middle of a long test run, so I cannot drop it in just yet. I'm not sure why the helper needs dynamic resolution; I must be missing something. But maybe the getClass() call could just be primed with the target method from Object. Otherwise "$static$self instanceof Class ? $staticSelf : $static$self.getClass()" could be "$static$self instanceof Class self ? self : $static$self.getClass()" -- maybe that does not help...

[eric-milles]

Do I need Spock to process the code that accesses the field? Or does the Spock test need to implement the trait?

[eric-milles]

I have this, which may be related:

[paulk-asert]

Also, I didn't know about instanceof either, I'll try to look at that too at some point if I get time.

[eric-milles]

Do you have a small, self-contained project that demonstrates the error? The unit test still does not get me to a runtime error of some kind.

[eric-milles]

I've been meaning to simplify the receiver handling for this sort of case. If I produce a restructured version, could you try it against the failing grails build?

[paulk-asert]

When I checkout the groovy11907 branch and run this test:

gradlew :test --tests=org.codehaus.groovy.transform.traitx.TraitASTTransformationTest.testTraitStaticFieldHelperGetClassMarkedDynamic

It passes. If I then comment out the second param below to remove the fix:

receiver = asClass(receiver/*, fn*/); // GROOVY-11907
The test fails.

[paulk-asert]

I am travelling for 2 days. When I return I can try it.