[
https://issues.apache.org/jira/browse/GROOVY-12242?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18103429#comment-18103429
]
ASF GitHub Bot commented on GROOVY-12242:
-----------------------------------------
daniellansun commented on PR #2773:
URL: https://github.com/apache/groovy/pull/2773#issuecomment-5242278626
@paulk-asert Thank you for the review. Responses point-by-point.
---
## 1. Breaking-change documentation
> *…silent behavior change… no release-notes/docs change. It should be in the
> beta-2 notes and the Groovy 6 breaking-changes list.*
> *(You noted: JIRA `breaking` label added; release notes on your next
edit.)*
**In-repo documentation added (this PR):**
| Location | Content |
|----------|---------|
| [`COMPATIBILITY.md`](COMPATIBILITY.md) | New **Groovy 6 — `instanceof`
pattern variable flow scoping (GROOVY-12242)** section: who is affected, silent
dynamic-mode failure mode (`MissingPropertyException`), partial `while`
contract |
| [`src/spec/doc/core-semantics.adoc`](src/spec/doc/core-semantics.adoc) |
New subsection **`instanceof` pattern variables (JEP 394)** under Statements:
user-facing rules, dynamic vs `@TypeChecked`, while note, link to JIRA /
COMPATIBILITY |
Beta-2 release-note prose remains with you via the JIRA `breaking`
cross-check — happy to paste a one-liner into the notes draft if useful:
> GROOVY-12242: `instanceof` / `!instanceof` pattern variables follow Java
flow
> scoping; out-of-scope uses are dynamic property lookups in dynamic Groovy
> (typically `MissingPropertyException`) and compile errors under type
checking.
---
## 2. `while` — partial flow scoping (documented decision)
> *while loops deliberately get only partial flow scoping (no after-loop
> introduction as Java has — fine, but should be an explicit documented
decision)*
**Agreed and made explicit.**
**Behaviour (6.0 contract):**
| Path | Support |
|------|---------|
| Short-circuit inside the condition (`&&` / `\|\|`) | Yes |
| `e.whenTrue()` in the **while body** | Yes (aligned with if-then) |
| `e.whenFalse()` **after** the loop when the body cannot complete normally
(JLS §6.3.2.3) | **No** — intentional |
| Leak past the loop | Never |
**Code:** `VariableScopeVisitor.visitWhileLoop` now isolates the condition
visit,
declares only `whenTrue` for the body, and does not declare after the loop.
Javadoc on that method (and on `visitDoWhileLoop`) states the JLS divergence.
User docs: `core-semantics.adoc` NOTE + `COMPATIBILITY.md`.
**Tests:** AST matrix (`InstanceofScopeTest`) and runtime
(`InstanceofTest.testWhileNegated_bodyAndAfterNotVisible`) cover body / after
and abrupt-body “still no after-loop introduction”.
Full after-loop introduction can be a later enhancement if needed; it is not
a
silent 6.0 follow-up.
---
## 3. `writeIfElse` breakable-frame region (GROOVY-7463)
> *writeIfElse changed the breakable-frame region (it now wraps both arms
where
> it previously ended before the else block) — GROOVY-7463 tests pass, but
the
> labeled-break-from-else behavior change should be confirmed as
intentional.*
**Restored historical region; intentional.**
| Region | Pre-12242 | Mid-redesign | **Now** |
|--------|-----------|--------------|---------|
| Condition | inside breakable | outer (for survivors) | outer |
| Then arm | inside | inside | inside |
| Else arm | **outside** | inside | **outside** |
Pattern slots stay on the **outer** frame (so survivors need no put-back
API);
the breakable still ends **before else**, matching GROOVY-7463. Javadoc on
`StatementWriter.writeIfElse` records that choice.
**Tests added:**
- `BreakContinueLabelTest.testBreakLabelFromElseOfLabeledIf` — break from
else
to the if’s label still works (named-break map is method-level; pop does
not
un-register names).
- `BreakContinueLabelTest.testBreakLabelInIfWithInstanceofPattern` — then-arm
break with a pattern variable.
Existing GROOVY-7463 then-arm tests remain green.
---
## 4. Bottom line / Jochen sign-off
> *technically strong and converging fast, but let Jochen sign off on the
redesign*
Acknowledged. The CompileStack push/hide/pop redesign and
`InstanceofFlowBindings` as single analysis+metadata type are the answer to
blackdrag’s layering review; we will wait for Jochen’s explicit OK before
merge.
> instanceof pattern variable scope is not aligned with Java flow scoping (JEP
> 394)
> ---------------------------------------------------------------------------------
>
> Key: GROOVY-12242
> URL: https://issues.apache.org/jira/browse/GROOVY-12242
> Project: Groovy
> Issue Type: Bug
> Reporter: Daniel Sun
> Priority: Major
> Labels: breaking
>
> h2. Summary
> After {{instanceof}} type patterns landed in GROOVY-11229, pattern variables
> were still scoped with a coarse lexical approximation. That diverges from
> Java’s *flow scoping* (JEP 394): a pattern variable must be visible only
> where the pattern has *definitely* matched.
> The gaps appear as:
> # variables missing where Java allows them
> # variables leaking past the statement that introduced them
> # name resolution and bytecode disagreeing, so an “out of scope” use can
> still load a local slot
> h2. Background
> * GROOVY-11229 added {{e instanceof T t}} (parser, AST, store-on-match).
> * Java (JEP 394 / JLS): scope follows boolean flow and abrupt completion,
> not simple block poison.
> * Groovy initially limited leakage with push/pop around statements, but did
> not implement true/false-path binding or CompileStack polarity.
> h2. Problems (before the fix)
> ||#||Scenario||Java||Groovy (before)||
> |1|negated {{instanceof}} — use pattern var in else|in scope|missing|
> |2|negated {{instanceof}} + early {{return}} — use pattern var after if|in
> scope|missing|
> |3|positive {{instanceof}} + abrupt else — use pattern var after if|in
> scope|missing|
> |4|{{boolean b = (o instanceof String s)}} then use {{s}}|not in
> scope|CompileStack leak (local still loadable)|
> |5|expression statement with pattern, then use pattern var|not in
> scope|CompileStack leak|
> |6|type-checked: pattern var used on RHS of logical-or|error on RHS|often
> accepted|
> |7|type-checked ternary false arm uses pattern var|error|often accepted|
> |8|negated {{instanceof}} — use pattern var in then-branch|not in scope|could
> ALOAD unassigned local (null)|
> h2. Steps to reproduce
> h3. A. Negated instanceof — else branch (should see {{{}s{}}})
> {code:groovy}
> def f = { Object o ->
> if (!(o instanceof String s)) {
> return 'not'
> } else {
> return s.toUpperCase() // expected: OK when o is String
> }
> }
> assert f('hi') == 'HI'
> {code}
> h3. B. Early return after negation (should see {{s}} after if)
> {code:groovy}
> def f = { Object o ->
> if (!(o instanceof String s)) return 'early'
> return s.toUpperCase() // expected: OK when o is String
> }
> assert f('hi') == 'HI'
> {code}
> h3. C. Leak after declaration (must *not* see {{{}s{}}})
> {code:groovy}
> class C {
> Object m(Object o) {
> boolean b = (o instanceof String s)
> return s // expected: MissingPropertyException /
> undeclared
> }
> }
> new C().m('hi')
> {code}
> h3. D. Type-checked {{||}} RHS must not see true-path binding
> {code:groovy}
> @groovy.transform.TypeChecked
> class C {
> static void m(Object o) {
> if (o instanceof String s || s.length() > 0) {
> // expected: undeclared / apparent variable s on RHS of ||
> }
> }
> }
> {code}
> h2. Expected behaviour
> Align with Java JEP 394 flow scoping for the common shapes:
> * true-path bindings (e.g. {{{}e instanceof T t{}}}) live in then-blocks,
> {{&&}} RHS, and ternary true arm
> * false-path bindings (e.g. {{{}!(e instanceof T t){}}}) live in
> else-blocks, after abrupt then, and the matching ternary arm
> * pattern variables do not leak past the introducing statement (declaration
> RHS, expression statement, …)
> * VariableScope (names) and CompileStack (locals) agree on which path a
> pattern local is live
> h2. Actual behaviour (before fix)
> * Lexical push/pop approximated “no leak past statement” but not true/false
> path polarity.
> * CompileStack could keep pattern slots after VariableScope had dropped the
> name (silent local load vs property miss).
> * Negation and abrupt-completion cases from Java were not supported.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
