[
https://issues.apache.org/jira/browse/GROOVY-12399?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18114277#comment-18114277
]
ASF GitHub Bot commented on GROOVY-12399:
-----------------------------------------
daniellansun commented on PR #2922:
URL: https://github.com/apache/groovy/pull/2922#issuecomment-5636301822
I’d hesitate to land this as-is. The unmatched throw is fixed and the tests
for that path are good. The encoding feels heavier than the language rule
needs, and statement-position `return` / `break` / `continue` still fail.
## The rule looks right
After GROOVY-12255, an arrow `switch` became a `SwitchExpression` whenever
the arms looked like expressions:
```groovy
def f(int i) {
def r = 'none'
switch (i) { case 1 -> r = 'one' }
r
}
f(42) // IllegalStateException; type-checker error under @TypeChecked
```
Java (JEP 361) decides statement vs expression by **position**, not by arrow
vs colon. The spec in this commit matches that. The implementation is the
part I’d like to revisit.
## Three modes, cloned into the parser
| Position | AST | Unmatched |
|---|---|---|
| Value unused | `SwitchStatement` | no-op |
| Implicit return | `SwitchExpression` + `_SWITCH_UNMATCHED_YIELDS_NULL` |
`null` |
| Real expression (`return switch`, assignment, argument, arrow-arm value) |
`SwitchExpression` | throw / exhaustiveness |
The third mode is driven by `AstBuilder.isImplicitReturnPosition`, an ANTLR
parent walk over labelled statements, `if`/`else`, `try`/`catch`, method /
closure / lambda / script bodies, colon-switch groups, trailing `break`, and
constructor vs method.
That is `ReturnAdder` on a different tree. `ReturnAdder` already knows those
sites; STC already uses it; classgen already emits `return null` when a body
`maybeFallsThrough`. None of that is reused, and the walks already disagree:
for a non-last colon case without `break`, the parse-tree walk still treats
the arm as an implicit return; `ReturnAdder` does not.
It also adds ~90 lines to `AstBuilder`, which is already very large.
## Simpler encodings
Two options; both look simpler than what we have.
**Prefer: make statement-position arrow `switch` a `SwitchStatement`**,
including implicit return. GROOVY-12255 already does this for incomplete
block arms.
- Matched: `ReturnAdder` already turns `case 1 -> 'one'` into `return 'one'`.
- Unmatched: the switch falls through; `AsmClassGenerator` already does
`ACONST_NULL; ARETURN` for a reference return.
- Exhaustiveness, ISE, and ICCE never apply.
- The flag, `writeUnmatchedSelector`, the STC early-return, and the
`complete && !unmatchedYieldsNull(...)` guard can go.
`visitCommandExprAlt` stays close to GROOVY-12255: rewrite unless the
statement is an arrow arm of an enclosing switch expression.
`isSwitchValueUsed`’s arrow-vs-colon check still belongs there; implicit
return does not.
Caveat: for a primitive return type, fall-through yields `0` / `false`, not
unbox-of-null. 4.x returned `null` and NPEd on unbox. If that matters,
document it — or keep a `SwitchExpression` only for primitives. I wouldn’t
keep the third mode for `def` / `Object` / `String` just to preserve AST
shape. `voidMethodAndConstructorEndWithAStatement` asserting
`SwitchExpression` tests the encoding, not the language.
**If the third mode stays, stamp it with `ReturnAdder`.** Parse time keeps
only the incomplete-arm rewrite. After the AST exists, run `ReturnAdder`
(`doAdd = false`) and stamp the flag when the implicit-return expression is
a `SwitchExpression`; rewrite leftover
`ExpressionStatement(SwitchExpression)`
nodes to `SwitchStatement`. Arrow-arm values are already wrapped in `yield`
by then, so they stay strict. Closures need a small walk (or a helper pulled
out of `ReturnAdder`). Same concept, right layer, no second ReturnAdder.
## `return` / `break` / `continue` still see an expression
`visitCommandExprAlt` rewrites only after the arms have been visited.
`visitReturnStmtAlt` still rejects `return` whenever a
`SwitchExpressionContext` is on the stack, so this Java-legal statement
does not compile:
```groovy
def f(int i) {
switch (i) {
case 1 -> { return 'one' }
}
'none'
}
```
Same for unlabeled `break` / `continue`. JEP 361 allows those in statements.
The GROOVY-12255 tests only cover `def r = switch ... { return }`. This was
raised on the PR and is not addressed here.
Decide statement vs expression **before** visiting arms
(`isSwitchUsedAsStatement` / `isArrowGroup` already exist for yield rules)
and consult that in `visitReturnStmtAlt` / `visitBreakStatement` /
`visitContinueStatement`. `isEnclosingSwitchUsedAsStatement()` is close;
`visitReturnStmtAlt` does not use it.
## The flag is on the wrong type
`UNMATCHED_YIELDS_NULL` means “this `SwitchExpression` is lenient.” It lives
on `SwitchExpressionUtils`, which classifies labels for tableswitch /
lookupswitch / enum-name dispatch. If the flag stays, it belongs on
`SwitchExpression`.
`writeDefaultOrThrow` still prefers `completeEnum` over the flag. Callers
were updated (`complete = false` when the flag is set), so behaviour looks
fine today; the method itself still has an implicit contract. Safer if the
flag wins inside `writeDefaultOrThrow`.
Dropping `OperandStack.doGroovyCast` in `writeUnmatchedSelector` is right:
STC wraps result types, dynamic compilation sees `Object`, `null` needs no
cast.
## Tests
`Groovy12399` is in the right shape and covers a lot: unused statement,
class labels, loops, `if` not in return position, colon-arm nesting,
implicit return through `if` / `try` / colon switch, closures, scripts,
complete-enum `null` and ICCE-vs-null, void/ctor vs value method.
Gaps for the new rules:
- `return` / `break` / `continue` in a statement-position arrow block.
- Nested `case 1 -> switch (y) { case 2 -> 'two' }` as implicit return of
the outer. The inner stays strict (unmatched throws). May be intended;
4.x returned `null`.
- Primitive implicit return: `int f(int i) { switch (i) { case 1 -> 1 } }` —
NPE vs `0` is the `SwitchExpression` vs `SwitchStatement` fork.
- `@CompileStatic` on the exhaustiveness-in-expression-position cases
(only `@TypeChecked` today).
The `return switch` edits in `Groovy12255` follow from lenient implicit
return; a one-line comment on the enum test (as on the exhaustiveness test)
would make that obvious.
`COMPATIBILITY.md` now describes the implicit-return exception, then **Who
is affected** still says every dynamic switch expression throws on unmatched.
## What looks good
- `isArrowGroup`: a `switch` in a colon arm of a switch expression is a
statement; the value comes from `yield`. The nested-colon test matches.
- Complete-enum ICCE and null-selector ISE consult the flag on the optimized
path; the two complete-enum tests lock that in.
- `core-semantics.adoc` matches the JIRA: `return switch (...)` opts into
the strict rules.
## Suggestion
Keep the language rule; consider dropping this encoding.
Either rewrite statement-position arrow switches (including implicit return)
to `SwitchStatement` and let `ReturnAdder` plus fall-through do the work, or
stamp lenient expressions with `ReturnAdder` after the AST exists. Either
way we can delete `isImplicitReturnPosition` / `returnsValue` /
`isLastStatement` from `AstBuilder`. Decide statement vs expression before
visiting arms so `return` / `break` / `continue` follow JEP 361.
As it stands this is a working patch that puts a compiler concept in the
parser, and it doesn’t quite finish the Java alignment the issue argues for.
Happy to be talked out of that.
> Arrow switch statement without default throws IllegalStateException when no
> arm matches
> ---------------------------------------------------------------------------------------
>
> Key: GROOVY-12399
> URL: https://issues.apache.org/jira/browse/GROOVY-12399
> Project: Groovy
> Issue Type: Bug
> Affects Versions: 6.0.0-RC-2
> Reporter: Paul King
> Priority: Major
>
> Since GROOVY-12255 (first-class switch expressions), a {{switch}} used as a
> *statement* is only rewritten to a {{SwitchStatement}} when an arrow arm is a
> block that does not {{yield}}. A single expression arm, or a block containing
> exactly one expression statement, becomes an implicit {{yield}} and the
> switch stays a {{SwitchExpression}} even though its value is discarded. Such
> a switch now inherits the expression rules: an unmatched selector throws
> {{IllegalStateException}} in dynamic code and a missing {{default}} is a
> compile error under {{@TypeChecked}} / {{@CompileStatic}}.
> {code:groovy}
> def f(int i) {
> def r = 'none'
> switch (i) {
> case 1 -> r = 'one'
> }
> r
> }
> f(42)
> {code}
> Groovy 5.1.2: returns {{'none'}}. Groovy 6.0.0-RC-1:
> {noformat}
> java.lang.IllegalStateException: the switch expression does not cover the
> value 42
> {noformat}
> With {{@TypeChecked}} on {{f}}:
> {noformat}
> the switch expression does not cover all possible input values
> {noformat}
> The equivalent Java compiles and runs, returning {{"none"}}: an arrow-form
> switch *statement* needs no {{default}} and does nothing when unmatched.
> {code:java}
> static String f(int i) {
> String r = "none";
> switch (i) {
> case 1 -> r = "one";
> }
> return r;
> }
> f(42); // "none"
> {code}
> The boundary in Groovy depends on arm shape rather than position, which shows
> it is not intended:
> {code:groovy}
> switch (i) { case 1 -> r = 'one' } // throws when unmatched
> switch (i) { case 1 -> { r = 'one' } } // throws when unmatched
> switch (i) { case 1 -> { r = 'one'; r += '!' } } // fine
> switch (i) { case 1 -> { if (i) r = 'one' } } // fine
> switch (i) { case 1: r = 'one' } // fine
> {code}
> h3. Background
> In Java (JEP 361) the arrow is not what makes a switch an expression: both
> switch statements and switch expressions may use either the arrow or the
> colon form. The arrow controls fall-through; the position controls whether a
> value is produced. A Java switch statement with arrow arms needs no
> {{default}} and does nothing when unmatched (exhaustiveness is only required
> for statements that use patterns or {{case null}}, JEP 441).
> Groovy's implementation grew up with the other reading. GROOVY-9272 (4.0)
> added the arrow form _as_ the switch expression: the grammar rule is
> {{switchExpression}}, it accepts only arrows, and a switch in statement
> position was simply a switch expression whose value nobody read. That was
> harmless while the lowering was a closure call that evaluated to {{null}}
> when nothing matched. GROOVY-12255 kept "arrow means expression", gave
> expressions real semantics (exhaustiveness, the unmatched throw) and added a
> statement rewrite only for the one shape that visibly broke, blocks without
> {{yield}}. The GROOVY-12255 compatibility notes describe the new behaviour
> for switch _expressions_ only and state that a switch used as a statement is
> a {{SwitchStatement}}; the statement-position tests in {{Groovy12255.groovy}}
> all carry a {{default}} or always match, so the unmatched case was never
> exercised.
> h3. Fix
> Classify by position rather than syntax, as Java does: a switch whose value
> is not used always builds a {{SwitchStatement}}, whatever the arm shape. This
> applies to dynamic and statically compiled code alike since the rewrite
> happens in the parser. The exhaustiveness error and the unmatched-selector
> throw stay reserved for switches whose value is used ({{def x = switch ...}},
> {{return switch ...}}, an argument, an arm of an enclosing switch expression).
> Implicit-return position, which Java has no equivalent for, is the one
> Groovy-specific case: the last statement of a method, closure or script body
> (followed through nested blocks, if/else branches, try/catch blocks and
> colon-switch arms, as the compiler's {{ReturnAdder}} does). Both idioms below
> were valid in 4.x/5.x and pull in opposite directions, so neither "statement"
> nor "strict expression" fits:
> {code:groovy}
> list.collect { switch (it) { case 1 -> 'a'; default -> 'b' } } // the
> closure's value is the switch
> switch (args[0]) { case 'run' -> run() } // last
> statement of a script; no match must not throw
> {code}
> There the switch keeps its value, but an unmatched selector yields {{null}}
> rather than throwing and the type checker does not require exhaustiveness,
> exactly as 4.x/5.x behaved. Code that wants the strict expression rules in
> that position writes "return switch (...) \{...}".
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
