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