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