jamesfredley commented on PR #15557:
URL: https://github.com/apache/grails-core/pull/15557#issuecomment-4569997108
## Burn-down pass following @paulk-asert's remaining-workarounds assessment
Thanks @paulk-asert. I worked through your assessment row by row, re-ran
every reproducer against multiple Groovy versions (4.0.31, released 5.0.6,
current `5.0.7-SNAPSHOT` off `GROOVY_5_0_X`, and `6.0.0-SNAPSHOT`), and where
relevant under both `indy=true` / `indy=false`. The reproducer repos have each
been corrected and pushed; details + per-repo replies follow on the individual
commit comments. Summary:
| # | Workaround | Your note | Verified outcome |
|---|---|---|---|
| 1 | `render(Map)` File truthiness | (issue #1) use `containsKey`/`== null`
| Already done; real fix, no compiler-bug claim. Kept. |
| 2 | `VariableScopeVisitor` / `setVariableScope` | "Main fails on old and
new" | **Confirmed.** Reclassified - see below. |
| 3 | controller action param scope (indy=false) | "Reproducer passes on old
and new" | **Confirmed.** Not standalone-reproducible - see below. |
| 4 | `@Builder(SimpleStrategy)` | "fixed in GROOVY-12040 (MERGED)" |
**Confirmed** root cause; fix is on `master` only, not yet on `GROOVY_5_0_X`. |
| 5 | `g.taglib` STC | "GROOVY-12041 filed, minor change on the Grails side"
| **Fixed and removed** following your recommendation. |
| 6 | `Validateable` trait static override | GROOVY-11985 | Still
reproduces; reflection shim kept. |
### Row 2 - `setVariableScope` is all-version hygiene, not a Groovy 5
workaround
You're right that the reproducer's `Main` fails on old Groovy too. I
reproduced the `ClosureWriter.createClosureClass` NPE on **4.0.31, 5.0.6 and
5.0.7-SNAPSHOT** alike - a synthesised `ClosureExpression` with a null
`VariableScope` has always NPE'd; it was just never exercised. So
`closureExpression.setVariableScope(new VariableScope())` in
`ResourceTransform` / `AbstractMethodDecoratingTransformation` is a **real,
all-version fix** (like the row 1 `File.asBoolean` one), not a
Groovy-5-conditional workaround, and I've reclassified it as such. The
genuinely Groovy-5-specific item is the separate `VariableScopeVisitor` NPE
during canonicalization (`:grails-datamapping-tck:compileGroovy`), which is
what the `try/catch` guard actually addresses; that piece is not isolable
standalone yet.
### Row 3 - not standalone-reproducible; workaround retained
Confirmed the bare try/catch reproducer passes everywhere. I went further
and faithfully replayed the **full** `wrapMethodBodyWithExceptionHandling`
shape (prepended allowed-methods block, the real `catch` that declares `$method
= getExceptionHandlerMethodFor(...)` and references `this`, and the `finally`
touching `this.request`) at the CANONICALIZATION phase, plus a variant
injecting an unresolved local-var reference post scope-resolution. **All shapes
still PASS on every version/indy combination.** The runtime
`MissingPropertyException` only arises from the complete
`convertToMethodAction` chain (param rewritten to a request-bound local + the
kept parameterised method + the generated no-arg delegating wrapper)
interacting with live request binding - not isolable into a standalone script.
The workaround stays; note it also covers an unrelated SiteMesh3 + Spring 7
incompatibility, so it can't be removed on the Groovy axis alone.
### Row 4 - GROOVY-12040 confirmed, fix not yet on `GROOVY_5_0_X`
Confirmed the root cause is `@Builder` carrying `@Retention(SOURCE)` since
5.0.0 (GROOVY-10855 scope-creep) - `getAnnotation(Builder)` returns
RUNTIME/non-null on 4.0.31 and SOURCE/null on 5.0.6. GROOVY-12040 restores
RUNTIME and is merged on `master` (fix-version 5.0.7 + 6.0.0-alpha-2), but it
is **not yet backported to `GROOVY_5_0_X`**, so the current `5.0.7-SNAPSHOT`
this PR consumes still returns null (verified). The `ConfigurationBuilder`
workaround therefore stays until the backport lands; I'll drop it and re-verify
once a fixed snapshot is published.
### Row 5 - fixed per your GROOVY-12041 recommendation, workaround removed
This one was misdiagnosed in the PR (it was framed as "node identity changes
between callbacks"). Your GROOVY-12041 write-up nailed it: when the receiver
inherits `getProperty(String)` (every compiled GSP page does),
`unresolvedVariable`/`unresolvedProperty` never fire for `g`, so the receiver
is never recorded. I reworked the reproducer so the subject inherits
`getProperty(String)` and it now reproduces exactly your table - PASS on
4.0.31, FAIL on 5.0.6 / 5.0.7-SNAPSHOT / 6.0.0-SNAPSHOT - and a
name-based-match extension repairs every cell.
Applied the Grails-side change you recommended:
`GroovyPageTypeCheckingExtension.methodNotFound` now matches the call receiver
**by name** against the allowed taglib namespaces instead of by recorded node
identity. The three `@IgnoreIf({ ... && data.gDotPrefix })` skips are removed
and `GspCompileStaticSpec` passes on `5.0.7-SNAPSHOT` (21 passed, 0 failed; the
2 remaining skips are an unrelated undeclared-variable check). **This
workaround is burned down.**
### Row 6 - GROOVY-11985 still open, shim kept
`Validateable`'s `this.defaultNullable()` override is still hijacked by
`TraitReceiverTransformer` on 5.0.6, 5.0.7-SNAPSHOT **and** 6.0.0-SNAPSHOT
(reproducer test 2 fails on all three); the reflection shim's reflection path
is the only one that honours the override. GROOVY-11985 is Open and
apache/groovy#2529 isn't merged, so the shim stays.
### Net
Row 5 removed (1 burn-down); Row 2 reclassified from "workaround" to "real
fix". Rows 3, 4 and 6 are genuinely still required on `5.0.7-SNAPSHOT` today -
Row 4 will drop the moment GROOVY-12040 reaches `GROOVY_5_0_X`. @paulk-asert a
backport of GROOVY-12040 to `GROOVY_5_0_X` would let us drop row 4 immediately;
and any movement on GROOVY-11985 / GROOVY-12041 would close out rows 6 and the
upstream side of 5.
--
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]
