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