[
https://issues.apache.org/jira/browse/GROOVY-12242?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18103092#comment-18103092
]
ASF GitHub Bot commented on GROOVY-12242:
-----------------------------------------
daniellansun commented on code in PR #2773:
URL: https://github.com/apache/groovy/pull/2773#discussion_r3742793371
##########
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 detailed and precise feedback. All three points have been
addressed.
## 1. Missing test case
You identified this shape, which was absent from the matrix:
```groovy
if (!(o instanceof String s)) {
println "not String" // S can complete normally
} else {
println "String"
return // T cannot complete normally
}
println s // must be INVALID
```
JLS §6.3.2.2-200-C analysis:
- `e = !(o instanceof String s)`: `whenTrue = {}`, `whenFalse = {s}`
- **C-A**: `e.whenTrue = {}` — nothing to introduce even with abrupt T.
- **C-B**: requires `e.whenFalse = {s}` **and** S cannot complete normally.
Here S *can* complete normally (it falls through) — **C-B does not apply**.
- → `s` is **not** introduced after the if-else.
Added:
- `testNegatedInstanceof_abruptElseOnly_noVisibilityAfter` in
`InstanceofTest.groovy`
- `testCase7b_negatedInstanceof_abruptElseOnly_afterDynamic` in the new
`InstanceofScopeTest.groovy` (AST-level, see §3 below)
## 2. `@TypeChecked` should not be special for scoping
You are correct. Tests 5, 6, and 7 incorrectly used `@TypeChecked`, implying
it
enables enforcement that the base compiler does not provide. The real
picture:
`VariableScopeVisitor.visitIfElse` already correctly scopes pattern
variables in
all modes. For shapes where `e.whenTrue = {}` (e.g. `instanceof s || cond`,
`!(instanceof s) && cond`), `declarePatternVariables(bindings.whenTrue())`
declares
**nothing** in the if-block scope. The undeclared reference `s` therefore
resolves
via `findVariableDeclaration` to a `DynamicVariable` — in both dynamic and
`@TypeChecked` modes. In dynamic Groovy, `DynamicVariable` resolution at
runtime
yields `MissingPropertyException` — the same observable effect.
Tests 5, 6, and 7 have been rewritten to use plain
`shouldFail MissingPropertyException` without `@TypeChecked`. The comments
and the
`VariableScopeVisitor` Javadoc now state explicitly:
> References to a pattern variable outside its flow scope resolve to
> `DynamicVariable`, which at runtime produces a `MissingPropertyException` —
> the same behaviour as `@TypeChecked`'s compile-time error, **without
> requiring that annotation**.
## 3. Architecture: `VariableScopeVisitor` as single source of truth
You raised the concern that two phases independently apply the same
flow-scoping
logic on an AST that should already have been enhanced by the first.
Here is the precise split:
- **`VariableScopeVisitor.visitIfElse`** is the **authoritative source** of
scope
decisions. It declares each pattern variable only in the scope(s) where the
JLS says it is definitely assigned. All subsequent compiler phases — AST
transforms, type-checking, code generation — see the correct name bindings
without re-deriving the logic.
- **`InstanceofFlowSlotPublisher`** in `StatementWriter` solves a distinct
**bytecode-level problem**: `evaluateInstanceof` allocates a CompileStack
slot
during condition evaluation (required for `&&` short-circuit RHS). Without
the
publisher, that slot would be visible to both branches in the bytecode,
even
though `VariableScopeVisitor` has already excluded it from the
else-branch's
scope. The publisher hides and re-exposes slots to keep *CompileStack slot
visibility* consistent with the *VariableScope declarations* — it does not
independently decide which variables are in scope.
In short: `VariableScopeVisitor` says *which names are in scope*;
`InstanceofFlowSlotPublisher` says *which bytecode slots are exposed* on each
branch. Both are driven by the same `InstanceofFlowBindings` analysis to stay
in sync.
The Javadoc on `VariableScopeVisitor` (class level and `visitIfElse`) now
makes
this split explicit and is the canonical description of the design.
Longer term, if the dynamic compiler were changed to allocate pattern slots
lazily (only after the full condition has been evaluated, with initial
visibility
controlled by the taken branch), the slot publisher could be removed
entirely.
That would require changes to `CompileStack` / `evaluateInstanceof` and is
left
as a tracked future improvement.
## 4. AST-level scope tests (`InstanceofScopeTest.groovy`)
As you requested, a new `InstanceofScopeTest.groovy` has been added. It
compiles
each condition shape to `Phases.SEMANTIC_ANALYSIS` and inspects
`VariableExpression.getAccessedVariable()` on every `s` reference to verify:
- **In-scope** references resolve to the pattern variable's
`VariableExpression`
(a declared local).
- **Out-of-scope** references resolve to `DynamicVariable`.
| Test | Condition shape | Assertions |
|---|---|---|
| `testCase1_*` | `o instanceof String s` | if-block: local; else/after:
dynamic |
| `testCase1b_*` | same, abrupt else | after: local (§6.3.2.2-200-C-A) |
| `testCase2_*` | `!(o instanceof String s)` | if-block: dynamic;
else-block: local |
| `testCase3_*` | negated, abrupt if | after: local (§6.3.2.2-200-C-B) |
| `testCase4_*` | `instanceof s && cond` | && RHS: local; if-block: local;
else/after: dynamic |
| `testCase5_*` | `instanceof s \|\| cond` | if-block: dynamic; \|\| RHS
(false path): local |
| `testCase6_*` | `!(instanceof s) && cond` | if-block: dynamic |
| `testCase7a_*` | same, abrupt else | after: dynamic (C-B does not apply) |
| `testCase7b_*` | `!(instanceof s)`, abrupt-else-only | else-block: local;
after: dynamic (**new** missing case) |
| `testCase8_*` | `!(instanceof s) \|\| cond` | \|\| RHS: local; if-block:
dynamic; else-block: local |
| `testDoubleNegation_*` | `!!(instanceof s)` | if-block: local |
| `testDeMorgan_*` | `!(instanceof s && cond)` | if-block: dynamic;
else-block: local |
| `testTernaryExpression_*` | `(instanceof s) ? s : s` | true-expr: local;
false-expr: dynamic |
## Summary of changes
| File | Change |
|---|---|
| `InstanceofTest.groovy` | Tests 5/6/7 rewritten without `@TypeChecked`;
new `testNegatedInstanceof_abruptElseOnly_noVisibilityAfter` |
| `InstanceofScopeTest.groovy` | **New** — AST-level scope assertions via
`VariableExpression.getAccessedVariable()`, full visibility matrix |
| `VariableScopeVisitor.java` | Class-level and `visitIfElse` Javadoc
clarified: authoritative-source role; relationship with
`InstanceofFlowSlotPublisher`; JLS §6.3.2.2 rule labels on inline comments |
## 5. Bug fix: slot leakage for `||`-shaped conditions in dynamic Groovy
During the test rewrite, a genuine bug was uncovered and fixed:
For conditions like `o instanceof String s || cond` where
`InstanceofFlowBindings.of()` returns `EMPTY` (neither `whenTrue` nor
`whenFalse`
has any bindings), `InstanceofFlowSlotPublisher.captureAndHide` previously
returned early (`NONE`) because it only looked at `bindings.allNames()` —
which
is empty for this shape. However, `evaluateInstanceof` *still* allocates a
CompileStack slot for `s` while evaluating the condition. Without
capture-and-hide,
that slot remained visible to both branches in bytecode, allowing `s` to be
read
in the if-block even though `VariableScopeVisitor` had correctly excluded it
from
the if-block scope.
**Fix**: A new `InstanceofFlowBindings.allPatternNames(Expression)` static
method
walks the full condition expression tree to collect *all* pattern variable
names,
regardless of which flow path they are definitely assigned on. The
`captureAndHide` method now uses this to capture and hide all allocated
slots,
not just those in `bindings.allNames()`. Publishing still uses only the
path-specific sets (`whenTrueNames()`, `whenFalseNames()`), so the net
effect is:
- **Before fix**: `o instanceof String s || true` — `s` slot visible in
if-block
(bytecode leak; `VariableScopeVisitor` said dynamic but bytecode said
local).
- **After fix**: slot is captured and hidden after condition evaluation;
never
re-published for the if-block (because `whenTrue={}`) → correctly
invisible.
This fix closes the gap between `VariableScopeVisitor`'s scope decision and
the
bytecode slot visibility — the same result in both dynamic and `@TypeChecked`
modes, without needing `@TypeChecked` to enforce the rule.
## 6. Refactoring: `InstanceofFlowBindings` →
`VariableScopeVisitor.InstanceofFlowBindings`
To make the architectural intent explicit — that `VariableScopeVisitor` is
the
single authoritative source, and that the flow analysis exists **in service
of**
that visitor — `InstanceofFlowBindings` has been refactored into a `public
static
final` nested class named `InstanceofFlowBindings` inside
`VariableScopeVisitor`.
### Why a nested class?
- **Co-location signals authorship.** The class physically living inside
`VariableScopeVisitor` makes it immediately clear that this is *the*
visitor's analysis helper, not a free-standing utility that happens to
be referenced from two places.
- **Reference from the asm layer is still clean.** The code-generation layer
(`InstanceofFlowSlotPublisher`, `StatementWriter`,
`BinaryExpressionHelper`)
imports `VariableScopeVisitor.InstanceofFlowBindings` — the
`VariableScopeVisitor.`
prefix makes explicit that these classes are *consuming the scope decisions
established by `VariableScopeVisitor`*, not independently deriving them.
- **Single file to read.** Anyone reading `VariableScopeVisitor.java` now
sees
the complete picture: scope declaration logic **and** the flow analysis
that
drives it — no need to navigate to a second file.
### Changes
| File | Change |
|---|---|
| `VariableScopeVisitor.java` | Added `public static final class
InstanceofFlowBindings` (moved from the deleted file); added `@Internal`;
Javadoc updated with `@see InstanceofFlowBindings` |
| `InstanceofFlowBindings.java` | **Deleted** |
| `InstanceofFlowSlotPublisher.java` | Import updated to
`VariableScopeVisitor.InstanceofFlowBindings`; all `InstanceofFlowBindings`
usages → `FlowBindings` |
| `StatementWriter.java` | Same import + usage update |
| `BinaryExpressionHelper.java` | Same import + usage update |
| `InstanceofFlowBindingsTest.groovy` | Import updated; all
`InstanceofFlowBindings.xxx` → `InstanceofFlowBindings.xxx`; class name kept as
`InstanceofFlowBindingsTest` |
| `InstanceofScopeTest.groovy` | Same import + usage update |
> 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)
