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.
--
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]
