[
https://issues.apache.org/jira/browse/GROOVY-12242?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18103164#comment-18103164
]
ASF GitHub Bot commented on GROOVY-12242:
-----------------------------------------
daniellansun commented on code in PR #2773:
URL: https://github.com/apache/groovy/pull/2773#discussion_r3744641901
##########
src/test/groovy/groovy/InstanceofTest.groovy:
##########
@@ -223,4 +223,279 @@ final class InstanceofTest {
}
assert y == 'foobar'
}
+
+ // GROOVY-12242: Java-aligned flow scoping for negated instanceof (JEP 394)
+ @Test
+ void testVariableScopeNegatedElse() {
+ def f = { Object o ->
+ if (!(o instanceof String s)) {
+ return 'not'
+ } else {
+ return s.toUpperCase()
+ }
+ }
+ assert f('hi') == 'HI'
+ assert f(1) == 'not'
+ }
+
+ // GROOVY-12242: pattern variable remains in scope after abrupt then-branch
+ @Test
+ void testVariableScopeEarlyReturn() {
+ def f = { Object o ->
+ if (!(o instanceof String s)) return 'early'
+ return s.toUpperCase()
+ }
+ assert f('hi') == 'HI'
+ assert f(42) == 'early'
+ }
+
+ // GROOVY-12242: pattern variable remains after else that cannot complete
normally
+ @Test
+ void testVariableScopeAfterAbruptElse() {
+ def f = { Object o ->
+ if (o instanceof String s) {
+ // matched
+ } else {
+ return 'no'
+ }
+ return s.toUpperCase()
+ }
+ assert f('ab') == 'AB'
+ assert f(9) == 'no'
+ }
+
+ // GROOVY-12242: pattern variable must not leak after a declaration
statement
+ @Test
+ void testVariableNoLeakAfterDeclaration() {
+ def err = shouldFail MissingPropertyException, '''
+ class C {
+ Object m(Object o) {
+ boolean b = (o instanceof String s)
+ return s
+ }
+ }
+ new C().m('hi')
+ '''
+ assert err.message =~ /No such property: s/
+ }
+
+ // GROOVY-12242: pattern variable must not leak after an expression
statement
+ @Test
+ void testVariableNoLeakAfterExpressionStatement() {
+ def err = shouldFail MissingPropertyException, '''
+ class C {
+ Object m(Object o) {
+ o instanceof String s && s.length() > 0
+ return s
+ }
+ }
+ new C().m('hi')
+ '''
+ assert err.message =~ /No such property: s/
+ }
+
+ // GROOVY-12242: true branch of negated instanceof must not see the
pattern local
+ // (CompileStack polarity must match VariableScope — no silent null ALOAD)
+ @Test
+ void testVariableNegatedIfBranchNotInScope() {
+ def err = shouldFail MissingPropertyException, '''
+ class C {
+ Object m(Object o) {
+ if (!(o instanceof String s)) {
+ return s
+ }
+ return 'matched'
+ }
+ }
+ new C().m(1)
+ '''
+ assert err.message =~ /No such property: s/
+ }
+
+ // GROOVY-12242: true-path binding of left of || is not in scope on the
right (Java)
+ @Test
+ void testVariableOrRightHandSideNotInScope() {
+ def shell = GroovyShell.withConfig {
+ ast groovy.transform.TypeChecked
+ }
+ def err = shouldFail shell, '''
+ @groovy.transform.TypeChecked
+ class C {
+ static void m(Object o) {
+ if (o instanceof String s || s.length() > 0) {
+ }
+ }
+ }
+ '''
+ assert err.message =~ /The variable .s. is undeclared|Apparent
variable .s./
+ }
+
+ // GROOVY-12242: false-path binding is in scope on the right of || (Java)
+ @Test
+ void testVariableOrRightHandSideFalsePathInScope() {
+ def f = { Object o ->
+ // when o is String, left is false, right sees s
+ return (!(o instanceof String s) || s.isEmpty())
+ }
+ assert f('') == true
+ assert f('x') == false
+ assert f(1) == true // left true → short-circuit, s not needed
+ }
+
+ // GROOVY-12242: ternary false branch must not see true-path pattern
variable
+ @Test
+ void testVariableTernaryFalseBranchNotInScope() {
+ def shell = GroovyShell.withConfig {
+ ast groovy.transform.TypeChecked
+ }
+ def err = shouldFail shell, '''
+ @groovy.transform.TypeChecked
+ class C {
+ static Object m(Object o) {
+ return o instanceof String s ? 'yes' : s
+ }
+ }
+ '''
+ assert err.message =~ /The variable .s. is undeclared|Apparent
variable .s./
+ }
+
+ // GROOVY-12242: dynamic ternary false branch must not load a pattern local
+ @Test
+ void testVariableTernaryFalseBranchNotInScopeDynamic() {
+ def err = shouldFail MissingPropertyException, '''
+ class C {
+ Object m(Object o) {
+ return o instanceof String s ? 'yes' : s
+ }
+ }
+ new C().m(1)
+ '''
+ assert err.message =~ /No such property: s/
+ }
+
+ // GROOVY-12242: ternary true branch sees pattern variable
+ @Test
+ void testVariableTernaryTrueBranch() {
+ def f = { Object o -> o instanceof String s ? s.toUpperCase() : 'no' }
+ assert f('ab') == 'AB'
+ assert f(1) == 'no'
+ }
+
+ // GROOVY-12242: reassignment of pattern variable (not implicitly final,
JEP 394)
+ @Test
+ void testVariableReassignment() {
+ Object o = 'hi'
+ if (o instanceof String s) {
+ s = s + '!'
+ assert s == 'hi!'
+ } else {
+ assert false
+ }
+ }
+
+ // GROOVY-12242: pattern variable shadows a field only where in scope
+ @Test
+ void testVariableFieldShadowing() {
+ def obj = new Object() {
+ String s = 'field'
+ def test(Object o) {
+ if (o instanceof String s) {
+ return "pv=$s"
+ }
+ return "field=$s"
+ }
+ }
+ assert obj.test('x') == 'pv=x'
+ assert obj.test(1) == 'field=field'
+ }
+
+ // GROOVY-12242: && chain uses pattern variable on subsequent operands
+ @Test
+ void testVariableAndChain() {
+ Object o = 'hello'
+ assert (o instanceof String s && s.length() > 3 && s.startsWith('h'))
+ assert !(o instanceof String s && s.length() > 99)
+ }
+
+ // GROOVY-12242: while body can use true-path pattern variable
+ @Test
+ void testVariableWhileBody() {
+ Object o = 'ab'
+ def n = 0
+ while (o instanceof String s && s.length() > 0) {
+ n += 1
+ o = s.substring(1)
+ }
+ assert n == 2
+ assert o == ''
+ }
+
+ // GROOVY-12242: reuse the same pattern variable name in successive
statements
+ @Test
+ void testVariableNameReuse() {
+ Object a = 'x', b = 1
+ def r = []
+ if (a instanceof String s) r << s
+ if (b instanceof Integer s) r << s
+ assert r == ['x', 1]
+ }
+
+ // GROOVY-12242: type-checked flow scoping for early return
+ @Test
+ void testVariableScopeEarlyReturnTypeChecked() {
+ def shell = GroovyShell.withConfig {
+ ast groovy.transform.TypeChecked
+ }
+ assert shell.evaluate('''
+ @groovy.transform.TypeChecked
+ class C {
+ static String m(Object o) {
+ if (!(o instanceof String s)) return 'early'
+ return s.toUpperCase()
+ }
+ }
+ assert C.m('hi') == 'HI'
+ assert C.m(1) == 'early'
+ true
+ ''')
+ }
+
+ // GROOVY-12242: type-checked — positive instanceof still not in else
+ @Test
+ void testVariableScopePositiveNotInElseTypeChecked() {
+ def shell = GroovyShell.withConfig {
+ ast groovy.transform.TypeChecked
+ }
+ def err = shouldFail shell, '''
+ Number n = 12345
+ if (n instanceof Integer i) {
+ } else {
+ i.toString()
+ }
+ '''
+ assert err.message =~ /The variable .i. is undeclared/
+ }
+
+ // GROOVY-12242: type-checked — negated instanceof is in else
+ @Test
+ void testVariableScopeNegatedInElseTypeChecked() {
+ def shell = GroovyShell.withConfig {
+ ast groovy.transform.TypeChecked
+ }
+ assert shell.evaluate('''
+ @groovy.transform.TypeChecked
+ class C {
+ static String m(Object o) {
+ if (!(o instanceof String s)) {
+ return 'not'
+ } else {
+ return s.toUpperCase()
+ }
+ }
+ }
+ assert C.m('hi') == 'HI'
+ assert C.m(1) == 'not'
+ true
+ ''')
+ }
Review Comment:
Thank you for the careful design review. Your points on *why* slot visibility
matters, and on keeping **CompileStack** as the single administrator of
name↔slot
state, were correct. The earlier `InstanceofFlowSlotPublisher` +
`putVariable`/`removeVariable` design bent that model; the current code
follows
the direction you described.
---
## 1. When does CompileStack visibility matter?
> *if the variable is visible via compile stack even though it should not via
> scoping, does it matter?*
| Case | Does it matter? | Approach now |
|------|-----------------|--------------|
| Name unused on that path | Mostly LVT / debug ranges | Improved as a side
effect of hide + push/pop; not the primary driver |
| **Same name redeclared** where the pattern is not live | **Yes** — name
must be free | `CompileStack.hideVariable` (name free, index unchanged) |
| **Reference that should be dynamic** | **Yes** — `AsmClassGenerator` looks
up locals **by name** | (1) path hide; (2) if `accessedVariable instanceof
DynamicVariable`, never load a same-named local (also covers mid-condition
`\|\|` RHS) |
So action is required exactly for the two cases you flagged:
**redeclaration** and
**reference to a name that scoping has already rejected**.
---
## 2. Entry point is CompileStack; publisher removed
> *Entry point should be CompileStack… hide… name free, index unchanged…
push/pop…*
> *I don’t think we need the bindings or the publisher here anymore.*
**Done:**
- `InstanceofFlowSlotPublisher` **deleted**.
- `putVariable` / `removeVariable` **deleted** (they bypassed push/pop and
implied
CompileStack was no longer the sole producer of locals).
- CompileStack now owns:
- `recordPatternVariable` — slots produced only by `evaluateInstanceof`;
- `hideVariable` — name free, index kept;
- `hidePatternVariablesExcept(candidates, live)` — hide among names
**introduced by this condition** (outer pattern names untouched);
- `patternVariablesIntroducedSince(before)` — **identity**-based
introduced set
(same name re-bound by a later condition still path-hides correctly).
**Control flow (if/else):**
```text
// condition on outer frame → slots defined & recorded
pushBreakable() // arms only (GROOVY-7463)
pushState / hide(except whenTrue) / then / pop
pushState / hide(except whenFalse) / else / pop
pop() // outer still holds the slots
hidePatternVariablesExcept(introduced, survivors) // permanent on outer
```
Then/else use ordinary **push → hide → pop**. Survivors need no put-back: the
condition runs **before** `pushBreakable`, so after `pop` the outer map still
holds those slots and we only hide non-survivors.
---
## 3. asm must not re-run flow analysis
> *the asm part should not have to redo things that had been done before
already*
> *InstanceofFlowBindings should not be required concept wise* [in classgen]
**Agreed.** Layering now:
| Phase | Role |
|-------|------|
| `VariableScopeVisitor` + `InstanceofFlowBindings` (internal analysis) |
Declare names into scopes; attach **`InstanceofPathLiveNames`** (path-live
*name* sets) as AST metadata |
| classgen.asm | Read metadata only; drive CompileStack hide/push/pop —
**no** `InstanceofFlowBindings.of(...)` |
`InstanceofFlowBindings` remains a private analysis helper of the visitor
(and its
unit tests). It is not a cross-phase concept that asm re-executes. Classgen
only
consumes the **enriched** name sets on the AST — the same idea as using
`VariableScope` after the visitor, adapted to flow-sensitive bindings.
---
## 4. No SlotPublisher in VariableScopeVisitor
> *Mentioning InstanceofFlowSlotPublisher in VariableScopeVisitor is imho
not good*
**Done.** That class is gone; VSV Javadoc no longer names any classgen.asm
type.
The visitor documents only its own products: scopes +
`InstanceofPathLiveNames`.
---
## 5. Lazy allocation
> *how does the lazy approach solve the problem? … as long as this is a
break of
> the design it should not wait*
Agreed that a design break should not be deferred. The redesign above is
meant to
**remove the layering break now**, not postpone it.
Lazy define (slot only after the condition, on the taken path) could still
simplify `evaluateInstanceof` later, but it is **not** required to restore
CompileStack’s push/pop ownership. Early define remains for short-circuit
`&&`
RHS; path visibility is administered by CompileStack hide/push/pop +
metadata.
Lazy allocation is therefore an optional micro-optimisation, not a fix for a
broken design.
---
## 6. Tests you asked for
> *(1) shouldNotCompile where s is visible*
> *(2) ScopeTest: declare s where not visible, assert local*
> *(3) same for classgen / InstanceofTest*
**Added:**
- **`InstanceofScopeTest`:** redeclare where pattern is *not* live → new
local
(`accessedVariable` local and ≠ pattern); where pattern *is* live →
`already contains a variable of the name s`.
- **`InstanceofTest`:** matching runtime/classgen cases; successive ifs
reusing
the same pattern name; isolated pattern expression then later if.
Visibility matrix and flow-bindings tests remain green; main-module `:test`
is green
on this change set.
---
## 7. Point → change (summary)
| Your point | Response |
|------------|----------|
| Explain *why*, not only *what* | Free name for redeclare + correct load vs
dynamic access |
| CompileStack entry; hide + push/pop; drop publisher | Done; publisher
deleted |
| put/remove bend the model | Removed; no put-back API |
| asm must not redo visitor work | Metadata only; no re-analysis |
| Don’t name later asm types from VSV | Done |
| Design break must not wait on “lazy” | Layering fixed without lazy |
| Redeclare tests (visible / not) | Scope + runtime tests added |
Happy to adjust further if any of the above still looks like a strong bend
of the
CompileStack model from your point of view.
> 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
>
> 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)
