daniellansun commented on PR #2707: URL: https://github.com/apache/groovy/pull/2707#issuecomment-5004526614
> AI read: > > > Verdict: sound, well-engineered, and ship-able — with one design inconsistency worth reconciling before merge. > > The one issue to reconcile — discarded-`CHECKCAST` CCE is treated inconsistently. > > Within this same PR, a cast whose result is discarded is handled two opposite ways: > > > > * before `POP` (`tryRemovePendingLoad`): the cast is dropped — "drop the dead cast, keep the pop." The PR extends this so even `ALOAD; CHECKCAST; POP` collapses to nothing (test `removesLoadAndAttachedCheckcastBeforePop` asserts → `['RETURN']`). → the `ClassCastException` is eliminated. > > * before void `RETURN` (`tryDropPendingLoadOnReturn`): the cast is preserved — emits `CHECKCAST; POP; RETURN` "so a type-check side effect is preserved." → the CCE is **kept**. > > > > Same situation (cast value thrown away), opposite decisions based only on `POP` vs `RETURN`. By the author's own stated principle (preserve the type-check side effect), the POP path is the inconsistent one. Impact is likely low — the standalone drop shipped back in GROOVY-12065, the full CI matrix is green, and Groovy rarely emits a bare `CHECKCAST; POP` for a discarded cast (dynamic casts go through `castToType`) — so I'd rate it PLAUSIBLE, not a confirmed regression. But it's a real semantic question worth settling: either a discarded cast's CCE matters (then preserve it on the POP path too) or it doesn't (then simplify the RETURN path to drop it). A behavior test pinning a discarded failing cast under @CompileStatic would lock the intended semantics. Agreed — same situation, opposite decisions. We settled on **preserve CCE on both paths** (Java-like; matches the void-RETURN intent). **Change:** `tryRemovePendingLoad` now emits attached/standalone `CHECKCAST` before `POP` instead of dropping it. Pure loads without a cast still collapse. **Tests:** - unit: POP vs RETURN both keep `CHECKCAST` (`preservesLoadAndAttachedCheckcastBeforePop`, `preservesStandaloneCheckcastBeforeReturn`, …) - SC: `Groovy12162` — discarded Groovy cast keeps side effect; after `instanceof String`, discarded `(String) o` still emits `CHECKCAST; POP` (not erased) -- 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]
