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.
When I mentioned De Morgan, I was actually not thinking of !!. I was
thinking of for example
`(a==b && (a instanceof X x && x.isFoo())` which is... well De Morgan helps
with the escape analysis here maybe, but does not solve the problem of the
usage of x is valid, but a and b cannot reference that x. But I think you have
that one actually covered.
One more word to TypeChecked. We have basically lexical scopes for variables
in Groovy. The difference is that the scope parenting the method level is an
open scope (similar for Closure), while it is not for TypeChecked. This means a
random reference `s` is always valid in dynamic Groovy, but the semantics are
influenced by the lexical scope. That involves double declaration and also
shadowing rules. TypeChecked and not TypeChecked should behave here the same,
except for a vanilla `s` not always being valid.
--
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]
