[
https://issues.apache.org/jira/browse/GROOVY-12428?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18131904#comment-18131904
]
ASF GitHub Bot commented on GROOVY-12428:
-----------------------------------------
paulk-asert commented on PR #3003:
URL: https://github.com/apache/groovy/pull/3003#issuecomment-6400964192
Thanks for the review. Taking your points in turn:
1. Narrowing a *different* variable and falling through is already covered
by the
"'integer' may be entered directly" test: the body's narrowing is popped
with
the case frame, and only the selector's own narrowing is carried forward.
2. Good call on the fall-through test. The private helper only looked at the
last
statement, so `if (c) { ...; break } else { ...; break }` and
`try { ... } finally { break }` were treated as falling through and
produced a
spurious `(String | Integer)` union at the next type label. I've replaced
it
with `GeneralUtils.mayCompleteNormally`, the same flow analysis
`StatementWriter`
uses to decide whether a case falls through, so STC and the bytecode now
agree.
`yield` was already fine (abrupt in that analysis), as are `continue`,
labelled
`break`, and a nested switch whose inner `break` does fall through.
3. Agreed, `default` is fine: GROOVY-8411 adds the declared type there, so
the
union is a safe over-approximation. The existing GROOVY-10702 test still
expects `(Class | File | Object)` and passes unchanged.
4. #3002 merges cleanly with this branch; the changes are complementary.
While adding the tests I found one more gap, pre-existing but now easy to
close:
falling through into a *constant* label kept the previous narrowing, so
case 'a':
assert x instanceof String
case 1:
x.toUpperCase() // compiled, but 1 enters this case directly
Only a class label refines the selector, so on fall-through the selector's
narrowing is now dropped unless the next label is a `ClassExpression`. Tests
added for the expression-form leak, the two constant-label shapes, and the
if/else-break and try/finally-break shapes. STC suite and full core `:test`
green.
> STC: instanceof narrowing leaks from one switch case into subsequent cases
> --------------------------------------------------------------------------
>
> Key: GROOVY-12428
> URL: https://issues.apache.org/jira/browse/GROOVY-12428
> Project: Groovy
> Issue Type: Bug
> Components: Static Type Checker
> Affects Versions: 5.0.3, 5.1.3, 6.0.0
> Reporter: Jochen Kemnade
> Priority: Major
> Labels: patch, patch-available
> Attachments:
> 0001-GROOVY-12428-STC-flow-typing-narrowing-leaks-from-on.patch
>
>
> h3. Reproducer 1
> {code:groovy}
> import groovy.transform.CompileStatic
> @CompileStatic
> void check(String type, Object value) {
> switch (type) {
> case 'text':
> assert value instanceof String
> break
> case 'integer':
> assert value instanceof Integer
> break
> case 'boolean':
> assert value instanceof Boolean
> break
> }
> }
> check('integer', 42)
> {code}
> With Groovy 6.0.0 this fails to compile:
> {noformat}
> Incompatible instanceof types: java.lang.String and java.lang.Integer
> @ line 10, column 20.
> Incompatible instanceof types: java.lang.String and java.lang.Boolean
> @ line 13, column 20.
> {noformat}
> h3. Reproducer 2
> {code:groovy}
> import groovy.transform.CompileStatic
> @CompileStatic
> void check(String type, Object value) {
> switch (type) {
> case 'text':
> assert value instanceof String
> break
> case 'integer':
> println value.toUpperCase()
> break
> }
> }
> check('integer', 42)
> {code}
> With Groovy 5.0.3, 5.1.3, and 6.0.0 this compiles and fails at runtime:
> {noformat}
> java.lang.ClassCastException: class java.lang.Integer cannot be cast to class
> java.lang.String
> {noformat}
> h3. Expected result
> Each case ends with {{{}break{}}}, so the narrowing of {{value}} to
> {{String}} in the first case should not apply to the other cases. Reproducer
> 1 should compile. Reproducer 2 should be rejected with "Cannot find matching
> method java.lang.Object#toUpperCase()".
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
