[
https://issues.apache.org/jira/browse/GROOVY-12255?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18104853#comment-18104853
]
ASF GitHub Bot commented on GROOVY-12255:
-----------------------------------------
daniellansun commented on PR #2784:
URL: https://github.com/apache/groovy/pull/2784#issuecomment-5296013229
> I am still reviewing but an initial AI assessment below:
>
> > Gaps I'd close before merge:
> >
> > * **Labeled break/continue escaping an arm**: `outer: while (...) { def
r = switch (x) { case 1 -> { for (;;) { break outer } } } }`. The parser's
peek-based check sees the loop frame and allows it, and the new `LabelVerifier`
code fences closures for `yield` but doesn't fence _labels_ at the expression
boundary. Java rejects this ("attempt to break out of a switch expression");
here it likely compiles to a jump that abandons the expression mid-evaluation.
Needs a check plus a fail test.
> > * **Colon group that can complete normally despite containing a yield**:
`case 'a': if (cond) yield 1` as the last group passes the parser's
contains-yield check, then at runtime falls into the "does not cover" ISE even
though the selector _did_ match — Java makes this a compile error, and the
runtime message is misleading. Verify and decide.
> > * **Null selector under `@CS`** — no test for any of the three fast
paths (see above).
> > * **`@CS` fast paths + yield inside try/finally** — the finally-block
stashing logic is only exercised dynamically.
> > * **Switch expressions in field initializers / constructors / static
initializers under `@CS`** — the writer leans on controller state; untested.
> > * **GINQ**: the `LabelVerifier` javadoc explicitly calls out
switch-expressions-inside-GINQ-queries as allowed, but no GINQ test was added.
> > * **`SwitchExpression.transformExpression`** is a design smell: for a
plain `ExpressionTransformer` it only rewrites top-level
yield/throw/expression-statement expressions inside arms (if-conditions and
loop conditions are missed); when the transformer is also a `GroovyCodeVisitor`
it additionally re-visits the arm code, risking double transformation. It also
mutates the original arm statements while claiming to produce a copy. Worth
tightening before third-party transforms depend on it.
Thank you — including for
[`9a0c682`](https://github.com/apache/groovy/commit/9a0c682afdaaed2ae706f7ca52973cead801a8db),
which already closed labeled break/continue, the `@CS` null-selector paths,
and several bytecode cases. The remaining items are handled as follows.
| Gap | Response |
|---|---|
| Labeled `break` / `continue` escaping an arm | Kept your `LabelVerifier`
isolation of loop/switch/label state. Added a `@CompileStatic` for-loop-in-arm
case with an implicit-this call so the copied `ForStatement` keeps its
`VariableScope` after `transformExpression`. |
| Colon group that contains a `yield` but can still complete | Last group is
now rejected unless every path yields or throws
(`GeneralUtils.mayCompleteNormally`, which uses the existing statement-flow
analysis). Intermediate colon groups may still fall through. Covered by
`lastColonArmIfWithoutElseIsError`, `fail/SwitchExpression_14x.groovy`, and
`colonArmIfFallsThroughToCompletingDefault`. |
| Null selector under `@CS` | Your three fast-path tests remain. |
| `@CS` fast paths + `yield` in `try`/`finally` | Added int / `String` /
enum variants (`compileStaticYieldInsideTryFinally*`). |
| Field / constructor / static initializer under `@CS` | Added
`compileStaticSwitchExpressionInFieldInitializer`, `…InConstructor`,
`…InStaticInitializer`. |
| GINQ | Arrow-form coverage already lived in `GinqTest` (`testGinq - switch
- 1`…`6`). Added `testGinq - switch - yield block` so an explicit `yield`
inside `GQ { }` is covered as well. |
| `transformExpression` | It now returns a structural copy: selector and
case labels are transformed, arm statements are copied (not mutated), and the
transformer is not also applied as a `GroovyCodeVisitor`. `AssertStatement` and
`ForStatement.variableScope` are preserved.
`transformExpressionCopiesArmsAndDoesNotMutateOriginal` checks that a constant
inside an `if` / `yield` / `assert` is rewritten only on the copy. |
Happy to adjust further if any of these should take a different shape.
> Compile switch expressions as first-class AST (no closure desugar)
> ------------------------------------------------------------------
>
> Key: GROOVY-12255
> URL: https://issues.apache.org/jira/browse/GROOVY-12255
> Project: Groovy
> Issue Type: Improvement
> Reporter: Daniel Sun
> Priority: Major
> Labels: breaking
>
> h3. Problem
> GROOVY-9272 added switch expressions. The 4.0 implementation rewrites them in
> {{AstBuilder}} to an immediately-called closure around a switch
> {{{}statement{}}}:
> {code:groovy}
> // source
> def r = switch (x) {
> case 0, 1 -> 'a'
> default -> 'z'
> }
> // compiled as
> def r = { ->
> switch (x) {
> case 0:
> case 1: return 'a'
> default: return 'z'
> }
> }.call()
> {code}
> That is a simulation, not a JEP 361 switch expression:
> * every evaluation allocates a closure and an extra call frame
> * an unmatched selector completes with {{null}} instead of throwing
> * {{return}} / {{break}} / {{continue}} are interpreted against the
> synthetic closure, not the enclosing method
> * locals assigned in an arm are closure-shared, not method locals
> * {{@CompileStatic}} cannot emit {{tableswitch}} / {{lookupswitch}} the way
> javac does
> h3. Goal
> Compile a switch expression as a first-class {{SwitchExpression}} whose arms
> {{yield}} (or throw). Emit the result on the operand stack. Keep Groovy
> {{isCase}} matching (Class, regex, Collection, Closure). Align control flow
> and exhaustiveness with [JEP 361|https://openjdk.org/jeps/361] for both
> dynamic Groovy and {{@TypeChecked}} / {{{}@CompileStatic{}}}.
> h3. Proposed shape
> * Parser builds {{SwitchExpression}} / {{{}YieldStatement{}}}; arrow
> expressions become implicit {{{}yield{}}}. No closure wrapper.
> * Codegen: join all completing arms at one label with the value on the
> stack. When the selector and labels allow it, emit {{tableswitch}} /
> {{{}lookupswitch{}}}, the Java string-switch (hash + {{equals}} + second
> switch), or {{{}Enum.ordinal(){}}}; otherwise sequential {{{}isCase{}}}.
> * Exhaustiveness: unmatched dynamic selector throws
> {{{}IllegalStateException{}}}; a complete enum may omit {{default}}
> (synthetic {{IncompatibleClassChangeError}} if a new constant appears at
> runtime). {{@TypeChecked}} / {{@CompileStatic}} reject a provably
> non-exhaustive expression at compile time.
> * Control flow: {{return}} must not leave the enclosing method through a
> switch expression; {{yield}} must not jump through a nested closure/lambda.
> An arrow arm must {{yield}} or throw on every path.
> {code:groovy}
> int n = switch (day) {
> case MONDAY, FRIDAY -> 6
> case TUESDAY -> 7
> default -> {
> int len = day.toString().length()
> yield len
> }
> }
> {code}
> h3. Compatibility
> ||topic||4.0-5.x (closure rewrite)||after this change||
> |unmatched selector (dynamic)|{{null}}|{{IllegalStateException}}|
> |non-exhaustive under STC / CS|often accepted|compile error (unless a
> complete enum)|
> |arrow block with no {{yield}}|last expression is the closure result|compile
> error unless every path yields or throws|
> |Groovy {{isCase}} cases|works|still works (fast path only when labels are
> int / String / enum constants)|
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
