[
https://issues.apache.org/jira/browse/GROOVY-12478?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18134015#comment-18134015
]
ASF GitHub Bot commented on GROOVY-12478:
-----------------------------------------
paulk-asert commented on PR #3009:
URL: https://github.com/apache/groovy/pull/3009#issuecomment-6489752895
Thanks Daniel, I've pushed b361b9ac35 to cover your points:
1. **Placement.** The new `earlyYieldInSwitchExpressionPositions` test pins
early `yield` in `return switch …`, an assignment RHS and a method argument,
both dynamically and under `@CompileStatic`. These are expression positions, so
they go through the switch-expression path and were never affected. I also
probed the early yield in trailing position directly, inside `if`,
`try`/`finally`, `synchronized`, a closure, under `@CompileStatic`, and with a
typed `String` return. All are correct with this PR. 6.0.0 returns the late
value in all of those trailing positions, and 5.1.3 gets them all right.
`return`/`break` inside a value-producing switch are still rejected.
2. **`finally`.** Covered by `earlyYieldRunsEnclosingFinallyBlock`.
3. **#2933 / #3003.** #3003 has landed. I ran a combined smoke test before
it merged: arrow arms that narrow with `case String ->` and yield early, plus
`break`/`continue`/`return` from arrow arms in a loop, all under
`@TypeChecked`. With both changes applied, it passed along with both PRs' tests
and the stc package (3,676 tests). #2933 is still open, so I'll do the same
when it's ready.
4. **`ReturnAdder` shape match.** Good call. The lowered block is now marked
itself, not just its `break`. `ReturnAdder` finds the block by that marker and
replaces only its trailing value and `break`, so statements another transform
adds earlier in the block are kept.
`earlyYieldInReturnPositionKeepsStatementsAddedBeforeIt` uses a customizer to
insert a statement at `CANONICALIZATION`. With the previous shape match,
`pick(true)` silently returned `null`; now it returns `'first'`. If a marked
block ever stops ending with its value and `break`, it fails with a
`GroovyBugError` instead of compiling the wrong result.
Locally the groovy, bugs, gls, classgen, control and parser suites pass
(12,939 tests) and lintGate is clean. The commits also cherry-pick cleanly onto
GROOVY_6_0_X for 6.0.1.
> yield before the last statement of an arrow-switch arm is ignored when the
> switch is compiled as a statement
> ------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12478
> URL: https://issues.apache.org/jira/browse/GROOVY-12478
> Project: Groovy
> Issue Type: Bug
> Affects Versions: 6.0.0
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
> Fix For: 6.0.1
>
>
> A {{yield}} that is not the last statement of an arrow-switch block arm is
> silently ignored when the switch ends up classified as a statement: execution
> falls through to the rest of the arm instead of completing the switch.
> {code:groovy}
> def pick(boolean flag) {
> switch (1) {
> case 1 -> {
> if (flag) yield 'first'
> yield 'second'
> }
> }
> }
> assert pick(true) == 'first' // fails on 6.0.0: 'second'
> {code}
> The same thing happens when the switch's value is discarded (the code after
> the conditional {{yield}} still runs), and a {{yield}} inside a loop within
> the arm no longer compiles:
> {code:groovy}
> def pick(List xs) {
> switch (1) {
> case 1 -> {
> for (x in xs) { if (x > 2) yield "big $x" }
> yield 'none'
> }
> }
> }
> // 6.0.0: "the yield statement is only allowed inside a switch expression"
> {code}
> ||Case||5.1.2||6.0.0-RC-1||6.0.0-RC-3 / 6.0.0||
> |Trailing switch, {{pick(true)}}|'first'|'first'|*'second'*|
> |Value discarded, conditional {{yield}}|arm exits|arm exits|*falls through*|
> |{{yield}} inside a loop in the arm|works|works|*compile error*|
> |{{return switch (...)}}|'first'|'first'|'first'|
> Behaviour is the same in dynamic Groovy and under {{@CompileStatic}}.
> *Cause:* since GROOVY-12408 a trailing switch is a statement, and the parser
> rewrites statement-position arrow switches via
> {{AstBuilder.armCodeAsStatement}}, which replaces each {{yield x}} with a
> plain expression statement {{x}}. Only the last value of the arm reaches
> {{ReturnAdder}}, so a final {{yield}} still appears to work, but any earlier
> {{yield}} loses its jump. The rewrite does not descend into loops, so a
> nested {{yield}} survives as a {{YieldStatement}} and is then rejected.
> The related rejection of {{return}}, {{break}} and {{continue}} in every
> arrow arm ("switch expression does not support `return`") dates from when
> arrow switches were implemented as closures, where a {{return}} would only
> have left the closure; 6.0 compiles switches inline and makes
> statement-position ones real switch statements, so that restriction should
> now depend on position as well.
> *Expected:* {{yield}} keeps its early-exit meaning wherever the switch's
> position puts it. GROOVY-12408 was meant to change only how a trailing switch
> is encoded, and 4.x/5.x code had to use {{yield}} in block arms of exactly
> these switches. In return position, a {{yield}} should behave like
> {{return}}. Elsewhere it should evaluate its expression and exit the switch.
> *Suggested fix:* keep the {{YieldStatement}} s instead of flattening them.
> Let {{ReturnAdder}} turn the yields belonging to a return-position switch
> into returns, searching the whole arm including loops but stopping at nested
> switch expressions and closures. Compile any remaining {{yield}} in a switch
> statement as "evaluate, discard, jump to the switch's end label", which also
> makes the loop case work.
> *Workaround:* write return switch (...) { ... } so the switch stays an
> expression, or restructure the arm so its only {{yield}} is the last
> statement (e.g. if/else with a {{yield}} in each branch).
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
