[
https://issues.apache.org/jira/browse/GROOVY-12242?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18103046#comment-18103046
]
ASF GitHub Bot commented on GROOVY-12242:
-----------------------------------------
blackdrag commented on code in PR #2773:
URL: https://github.com/apache/groovy/pull/2773#discussion_r3740971027
##########
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:
So I hope I read the table right, I assume "-" means not visible and is
existing behavior.
Case 1b is incomplete. if the if-block also has an abrupt return, then -
well technically the code after is unreachable - but we pass that in
compilation. Which means normally `s` should not be visible. Maybe it does not
matter and can be ignored.
But I think this case here is missing:
```
if (!(o instanceof String s)) {
println "not String"
} else {
println "String"
return
}
println "still not String"
println s // invalid
```
It is not case 2,3,6, 7 or 9. Case 8 has the same behavior, but I doubt it
covers this variant. I know I did not mention this before, I had not enough
time to ensure the list is complete.
Then on TypeChecked... the comment made me think that the approach has a
major problem I did see a bit before already, but it becomes more clear to me.
I think there should be no reason as of why TypeChecked should enable more
cases. Especially not because of an implementation detail that exists in the
dynamic compiler.
My wish would be now the following... yes I am aware that this is a big wish:
Have the scoping logic completely in or through VariableScopeVisitor. Have
AST level tests that check the scope for correctness for all combinations of
instanceof (negated or not), with condition after or not, with return/throw in
if-block, in else-block. And for each case of that matrix we need to check and
define is the variable visible in the condition if it exists, in the if-block,
in the else-block or after. The code here goes in the right direction but I
think we need this on the AST level right after VariableScopeVisitor to ensure
downstream transforms do this right. Or maybe a simple VariableScope is not
enough anymore. The original purpose is to determine if a variable is
referencing something outside the scope or not.
And what does it mean for a Variable to be in the scope:
```
{ // block1
....
def s = ...
...
}
```
`s` is added to the scope of `block1`. This does not mean s is available in
all of block1. Declaring another variable s in that block can be easily checked
as not allowed, since the active scope already defines it.
```
{ // block1
if (!(o instanceof String s) {
return
}
println s
}
```
Now we have again our block1, but where is c declared? If we stay with the
VariableScope system we would then have to make a scope for block1, that define
`s` and remove it from the scope in the if-block. I think we currently have no
remove. Well or we do not add it the the child scope
```
{ // block1
if (o instanceof String s) {
return
}
println s // different s
}
```
If `block1` declares `s`, then the if-block references it, but after the if
we are back in block1 and now `s` is supposed to be invalid. The problem could
maybe solved by adding an invisible block containing the if-else for such a
case, but that means to conditionally rewrite a potentially big AST part, which
I also feel not so well about. I mean in principle the AST is an "abstract"
tree, not a parser tree, so I think technically it would be ok, just weird
because we are normally not doing that. (the primitive optimization part did
copy large parts of the AST, but mostly by visiting it twice)
So I cannot right away suggest a good solution, just that the actions in
StatementWriter raise warning flags for me and two phases using the same logic
on an AST that is supposed to have been enhanced for not requiring that logic
anymore, well that raises a red flag to me.
> 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)
