[
https://issues.apache.org/jira/browse/GROOVY-12242?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18102988#comment-18102988
]
ASF GitHub Bot commented on GROOVY-12242:
-----------------------------------------
codecov-commenter commented on PR #2773:
URL: https://github.com/apache/groovy/pull/2773#issuecomment-5224879654
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/2773?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
Report
:x: Patch coverage is `90.49587%` with `23 lines` in your changes missing
coverage. Please review.
:white_check_mark: Project coverage is 70.0243%. Comparing base
([`5ec4b4a`](https://app.codecov.io/gh/apache/groovy/commit/5ec4b4adfbeb7659395ed4164ae6c5ecdd42aba2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`0644776`](https://app.codecov.io/gh/apache/groovy/commit/0644776d6ea405719859bec5c35b4123254bb93c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2773?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Patch % | Lines |
|---|---|---|
|
[...dehaus/groovy/classgen/InstanceofFlowBindings.java](https://app.codecov.io/gh/apache/groovy/pull/2773?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2FInstanceofFlowBindings.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL0luc3RhbmNlb2ZGbG93QmluZGluZ3MuamF2YQ==)
| 82.9268% | [9 Missing and 5 partials :warning:
](https://app.codecov.io/gh/apache/groovy/pull/2773?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
|
[...oovy/classgen/asm/InstanceofFlowSlotPublisher.java](https://app.codecov.io/gh/apache/groovy/pull/2773?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2Fasm%2FInstanceofFlowSlotPublisher.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL2FzbS9JbnN0YW5jZW9mRmxvd1Nsb3RQdWJsaXNoZXIuamF2YQ==)
| 84.4444% | [2 Missing and 5 partials :warning:
](https://app.codecov.io/gh/apache/groovy/pull/2773?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
|
[...codehaus/groovy/classgen/VariableScopeVisitor.java](https://app.codecov.io/gh/apache/groovy/pull/2773?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2FVariableScopeVisitor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL1ZhcmlhYmxlU2NvcGVWaXNpdG9yLmphdmE=)
| 97.6190% | [0 Missing and 1 partial :warning:
](https://app.codecov.io/gh/apache/groovy/pull/2773?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
|
[...org/codehaus/groovy/classgen/asm/CompileStack.java](https://app.codecov.io/gh/apache/groovy/pull/2773?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2Fasm%2FCompileStack.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL2FzbS9Db21waWxlU3RhY2suamF2YQ==)
| 75.0000% | [0 Missing and 1 partial :warning:
](https://app.codecov.io/gh/apache/groovy/pull/2773?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2773?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2773 +/- ##
==================================================
+ Coverage 69.9945% 70.0243% +0.0298%
- Complexity 35540 35615 +75
==================================================
Files 1557 1559 +2
Lines 131696 131897 +201
Branches 24174 24212 +38
==================================================
+ Hits 92180 92360 +180
- Misses 31171 31180 +9
- Partials 8345 8357 +12
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2773?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[...odehaus/groovy/ast/expr/DeclarationExpression.java](https://app.codecov.io/gh/apache/groovy/pull/2773?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fast%2Fexpr%2FDeclarationExpression.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2FzdC9leHByL0RlY2xhcmF0aW9uRXhwcmVzc2lvbi5qYXZh)
| `82.7586% <ø> (ø)` | |
|
[...us/groovy/classgen/asm/BinaryExpressionHelper.java](https://app.codecov.io/gh/apache/groovy/pull/2773?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2Fasm%2FBinaryExpressionHelper.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL2FzbS9CaW5hcnlFeHByZXNzaW9uSGVscGVyLmphdmE=)
| `89.1396% <100.0000%> (+0.2028%)` | :arrow_up: |
|
[.../codehaus/groovy/classgen/asm/StatementWriter.java](https://app.codecov.io/gh/apache/groovy/pull/2773?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2Fasm%2FStatementWriter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL2FzbS9TdGF0ZW1lbnRXcml0ZXIuamF2YQ==)
| `99.2857% <100.0000%> (+0.0320%)` | :arrow_up: |
|
[...codehaus/groovy/classgen/VariableScopeVisitor.java](https://app.codecov.io/gh/apache/groovy/pull/2773?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2FVariableScopeVisitor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL1ZhcmlhYmxlU2NvcGVWaXNpdG9yLmphdmE=)
| `95.0644% <97.6190%> (+0.2166%)` | :arrow_up: |
|
[...org/codehaus/groovy/classgen/asm/CompileStack.java](https://app.codecov.io/gh/apache/groovy/pull/2773?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2Fasm%2FCompileStack.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL2FzbS9Db21waWxlU3RhY2suamF2YQ==)
| `86.4608% <75.0000%> (-0.1099%)` | :arrow_down: |
|
[...oovy/classgen/asm/InstanceofFlowSlotPublisher.java](https://app.codecov.io/gh/apache/groovy/pull/2773?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2Fasm%2FInstanceofFlowSlotPublisher.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL2FzbS9JbnN0YW5jZW9mRmxvd1Nsb3RQdWJsaXNoZXIuamF2YQ==)
| `84.4444% <84.4444%> (ø)` | |
|
[...dehaus/groovy/classgen/InstanceofFlowBindings.java](https://app.codecov.io/gh/apache/groovy/pull/2773?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2FInstanceofFlowBindings.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL0luc3RhbmNlb2ZGbG93QmluZGluZ3MuamF2YQ==)
| `82.9268% <82.9268%> (ø)` | |
... and [5 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/2773/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests,
report on failures, and find test suite problems.
- :package: [JS Bundle
Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save
yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
> 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)
