jamesfredley commented on PR #15557: URL: https://github.com/apache/grails-core/pull/15557#issuecomment-4328340128
## Final audit pass - workaround set is now minimal Continuing from [the previous audit](https://github.com/apache/grails-core/pull/15557#issuecomment-4327650290), I drove every "claim does not reproduce" workaround to a conclusion by reverting it locally and running the actual grails-core tests under Groovy 5.0.6-SNAPSHOT. Three of the five had been applied based on incorrect diagnoses and have either been removed entirely or replaced with a much smaller targeted fix. Two were real bugs misdescribed - the workaround stays but the diagnosis (and the inline comment) is now correct. ### Net workaround changes pushed | Original commit | Original claim | Action | |---|---|---| | a0ee062e89 (`GrailsApplicationCommand` trait -> abstract class, 11 files) | "`@Delegate` on a trait silently returns null" | **Reverted entirely.** Bug is at the call site, not the trait/`@Delegate`. The typed positional `templateRenderer.render(Resource, File, Map, boolean)` shape in `GenerateControllerCommand` is the actual workaround and stays. | | a6e9881ad5 (`PersistentEntityCodec` + `HibernateEntityTransformation` instanceof -> isAssignableFrom) | "Groovy 5 compiles `instanceof` to `checkcast`" | `javap -c` shows that's not what Groovy 5 does. **Hibernate swap reverted** (true branch only `return`s, no smart-cast trigger). **`BsonPersistentEntityCodec.resolvePropertyType` walker reverted** (`.getSuperclass()` works fine). **`PersistentEntityCodec` two ManyToMany swaps kept**, with the inline comment rewritten to point at the actual bug: incorrect smart-cast in the **else** branch of `if (cond && !(x instanceof Y))`. Reproducer: [SmartCastCheck.groovy](https://github.com/jamesfredley/groovy5-compiledynamic-trait-bug/blob/main/quick-checks/src/main/groovy/SmartCastCheck.groovy). The 21 originally-failing GORM tests pass with just those two lines. | | edb40f2628 + 4a2715973e (`NavigableMap.convertConfigObjectToMap` shim + `GroovyConfigPropertySourceLoader.toRegularMap`) | "ConfigObject iteration triggers infinite recursion under Groovy 5" | Both shims **removed entirely**. Replaced with a one-line `containsKey + get` change in `NavigableMap.resolveConfigMapValue` plus a 3-line `readWithoutCreating` helper. The actual bug is in the Spring-profile probe: each missing-key access (`spring`, `config`, `activate`, `on-profile`) was being inserted into the source ConfigObject by Groovy's `[]` operator, then re-read by the merge iteration, then re-probed by the next recursion -> stack overflow. ConfigMapSpec passes. | | 4a2715973e (`LoggingTransformer` "Groovy 5 manual SLF4J injection" comment) | "`@Slf4j` + LogASTTransformation triggers VariableScopeVisitor NPE" | **Reverted to the pre-Groovy-5 comment wording.** The change was *only* a comment rewrite - the actual code (manual SLF4J field injection) has been there since Grails 2.0. There is no Groovy 5 difference here. | | 4a2715973e (4 try/catch around `VariableScopeVisitor` + non-null `VariableScope` guard on `ClosureExpression`) | "Groovy 5 changed how VariableScopeVisitor handles certain AST states" | **Kept**, with the comment rewritten to flag this as upstream-fileable. The actual NPE reproduces during `:grails-datamapping-tck:compileGroovy` on `DataServiceRoutingProductDataService.groovy` (`BUG! exception in phase 'canonicalization' ... unexpected NullPointerException`). The visitor itself didn't change shape - what changed is that some Grails AST transforms now produce a node shape that the visitor fails on. | ### Real Groovy 5 regressions remaining (workaround needed, upstream fix owed) 1. **`@CompileStatic` `render(Map<String,Object>)` overload silently no-ops** when called against a multi-overload interface reference. [Reproducer](https://github.com/jamesfredley/groovy5-compiledynamic-trait-bug). 2. **`@CompileStatic` incorrect smart-cast in else branch of `if (cond && !(x instanceof Y))`**. The compiler narrows `x` to `Y` in the else branch even though the else is also entered when `cond` is false (regardless of `instanceof`). [Reproducer](https://github.com/jamesfredley/groovy5-compiledynamic-trait-bug/blob/main/quick-checks/src/main/groovy/SmartCastCheck.groovy). 3. **VariableScopeVisitor NPE on certain Grails AST transformation outputs**. No standalone repro yet - triggers when compiling `DataServiceRoutingProductDataService.groovy`. 4. **Interface with default methods compiled with `$getCallSiteArray()` causes IncompatibleClassChangeError under indy=false**. [Reproducer](https://github.com/jamesfredley/groovy5-compiledynamic-trait-bug/blob/main/quick-checks/src/main/groovy/InterfaceDefaultsCheck.groovy). 5. **GROOVY-11907 incomplete for indy=false static-setter trait helpers** (`VerifyError: get long/double overflows locals`). [Reproducer](https://github.com/jamesfredley/groovy5-compiledynamic-trait-bug/blob/main/quick-checks/src/main/groovy/TraitStaticFieldsCheck.groovy). cc @paulk-asert - `#1`, `#2`, `#4`, `#5` are all standalone-reproducible against Groovy 5.0.6-SNAPSHOT and would benefit from upstream eyes. The PR description has the full inventory with which sites still carry which workarounds. ### What was removed from this PR - 11 files reverted (GrailsApplicationCommand trait restoration + 9 scaffolding commands + Command.groovy template) - ~80 lines of ConfigObject conversion shim across 2 files - 1 hierarchy walker method (resolvePropertyType + 3 call-site changes) - 1 unnecessary `instanceof` -> `isAssignableFrom` swap in HibernateEntityTransformation - ~5 misleading "Groovy 5 ..." inline comments rewritten to be accurate The remaining workarounds are the minimum acceptable set on Groovy 5.0.6-SNAPSHOT. Each has either a standalone reproducer or an exact failing-without-the-workaround test in grails-core captured in the inline comment. -- 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]
