jamesfredley commented on PR #15557: URL: https://github.com/apache/grails-core/pull/15557#issuecomment-4327650290
## Standalone audit of every Groovy 5 workaround claim in this PR Continuing the exercise from the [render(Map) reproducer](https://github.com/jamesfredley/groovy5-compiledynamic-trait-bug), I built a standalone Groovy-only check for each remaining Groovy 5 workaround claim - same Java 21, same Gradle 9.4.1, same Apache snapshots resolution. Each check is the smallest faithful test for the failure mode the corresponding commit message or inline comment describes. All five checks live in `quick-checks/` of the same repo: https://github.com/jamesfredley/groovy5-compiledynamic-trait-bug/tree/main/quick-checks ### Results | # | Claim | Source | Reproduced? | Notes | |---|---|---|---|---| | 1 | `@CompileStatic` `x instanceof Y` -> `checkcast` (`ClassCastException` for the false case) | a6e9881ad5 | **NO** | `javap -c` confirms Groovy 5.0.5 and 5.0.6-SNAPSHOT emit `1: instanceof #N` bytecode for all five test shapes (direct, via method return, in if-branch, negated, against subtype). No exceptions. | | 2 | `ConfigObject` iteration triggers infinite recursion under Groovy 5 | NavigableMap inline comment, edb40f2628 | **NO** | Identical behaviour on Groovy 4.0.31 and Groovy 5.0.6-SNAPSHOT for keySet iteration, deep recursive iteration via `.each`, deep convert to LinkedHashMap, and merge of two ConfigObjects. Dynamic property creation on read of a non-existent key happens on **both** versions (it's documented Groovy semantics). | | 3 | `@Slf4j` + `LogASTTransformation` triggers `VariableScopeVisitor` NPE | LoggingTransformer.java inline comment, 4a2715973e | **NO (simple cases)** | All three forms (`@Slf4j` + `@CompileStatic`, dynamic, with closure body) compile and run cleanly on Groovy 5.0.6-SNAPSHOT. The workaround may still be needed for the specific Grails AST transformation chains, but the simple-case mechanism described does not reproduce. | | 4 | Interface with default methods + `$getCallSiteArray()` -> `IncompatibleClassChangeError` under indy=false | b8ee60d460 | **YES** | Reproduces exactly: `IncompatibleClassChangeError: Method 'CallSite[] IConfig.$getCallSiteArray()' must be InterfaceMethodref constant` on Groovy 5.0.6-SNAPSHOT + indy=false. Same code on Groovy 4.0.31 + indy=false runs cleanly. **Real Groovy 5 regression.** | | 5 | `@CompileStatic` trait + static fields -> `Trait$Helper` invalid bytecode under indy=false | GROOVY-11907 incomplete, 0804a4fc73 | **YES** | Reproduces exactly: `VerifyError: get long/double overflows locals` on Groovy 5.0.6-SNAPSHOT + indy=false. Same code on Groovy 4.0.31 + indy=false runs cleanly. **Real Groovy 5 regression. True follow-up to GROOVY-11907.** | ### Implications for this PR **For `#1` (instanceof) and `#2` (ConfigObject):** The 21 GORM tests / `ConfigMapSpec` / etc. tests *do* fail without the workaround in place (per prior CI runs in the commit history), so the workarounds are kept for CI passage. But the diagnosed mechanism is wrong - whatever is actually breaking those tests is something else. The inline comments and PR description should not perpetuate the wrong diagnosis. **Re-investigation is owed.** **For `#3` (@Slf4j NPE):** Simple cases compile fine. The workaround may be needed for some specific Grails AST transformation chain that my standalone test doesn't reach, but the simple mechanism described in the comment does not reproduce. **For `#4` and `#5` (indy=false bytecode bugs):** Confirmed real Groovy 5 regressions. The workarounds are justified, and these belong as upstream Groovy bug reports. ### What this means in code - a0ee062e89 (trait -> abstract class for GrailsApplicationCommand) has been **reverted** in 47ed74fef2 - the standalone reproducer shows the bug is at the `@CompileStatic` `render(Map)` call site, not at the `@Delegate` field on a trait. The typed positional `templateRenderer.render(Resource, File, Map, boolean)` shape in `GenerateControllerCommand` (case D in the reproducer) is the actual workaround and it stays. - The other workarounds (`#1`, `#2`, `#3`) stay in the code for now (their related tests need them) but the PR description has been updated to flag "diagnosed mechanism does not reproduce - actual root cause unknown". - Workarounds for `#4` and `#5` stay with diagnoses confirmed; both should be filed upstream with the linked reproducers. cc @paulk-asert - wanted to flag this in case the audit results are useful for upstream Groovy investigation. The `#4` and `#5` reproducers are the ones I'd most expect you to want to look at. -- 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]
