[
https://issues.apache.org/jira/browse/GROOVY-12399?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18114148#comment-18114148
]
ASF GitHub Bot commented on GROOVY-12399:
-----------------------------------------
Copilot commented on code in PR #2922:
URL: https://github.com/apache/groovy/pull/2922#discussion_r3987321373
##########
src/main/java/org/codehaus/groovy/classgen/asm/SwitchExpressionWriter.java:
##########
@@ -213,12 +215,31 @@ protected void writeDefaultOrThrow(final SwitchExpression
expression, final int
if (defaultStatement != null && !defaultStatement.isEmpty()) {
defaultStatement.visit(controller.getAcg());
if (maybeFallsThrough(defaultStatement)) {
- throwUnmatchedSelector(selectorIndex, selectorType);
+ writeUnmatchedSelector(expression, selectorIndex,
selectorType);
}
return;
}
if (completeEnum) {
throwIncompatibleClassChangeError();
+ } else {
+ writeUnmatchedSelector(expression, selectorIndex, selectorType);
Review Comment:
This branch takes precedence over `UNMATCHED_YIELDS_NULL`. Consequently, a
complete enum switch in implicit-return position still fails under
`@CompileStatic`: an unmatched new enum constant reaches
`IncompatibleClassChangeError`, and the static enum writer's null-selector path
directly calls `throwUnmatchedSelector`. Both paths must consult the new
metadata so implicit-return switches consistently yield `null`.
##########
src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java:
##########
@@ -2449,18 +2450,101 @@ public Statement visitBlock(final BlockContext ctx) {
@Override
public Statement visitCommandExprAlt(final CommandExprAltContext ctx) {
Expression expr = this.visitCommandExpression(ctx.commandExpression());
- // Statement-position `switch` with `->` is parsed as a
SwitchExpression
- // (switchStatement is colon-only). If an arrow block does not yield,
- // rewrite it to SwitchStatement so the block need not yield (JEP 361
- // statement rules). Expression-position incomplete arms already
errored.
- if (expr instanceof SwitchExpression se
- && ctx.getParent() instanceof ExpressionStmtAltContext
- && switchExpressionHasIncompleteArm(se)) {
- return configureAST(switchExpressionAsStatement(se), ctx);
+ // A `switch` with `->` is parsed as a SwitchExpression
(switchStatement is
+ // colon-only), but as in Java (JEP 361) the arrow only decides
fall-through;
+ // the position decides whether a value is produced. A switch whose
value is
+ // not used, or whose arrow block arms do not yield, is a
SwitchStatement.
+ // In implicit-return position (the last statement of a method,
closure or
+ // script body) the value is used, but an unmatched selector yields
null
+ // rather than throwing, as it did in 4.x/5.x (GROOVY-12399).
+ if (expr instanceof SwitchExpression se && ctx.getParent() instanceof
ExpressionStmtAltContext statement) {
+ if (switchExpressionHasIncompleteArm(se) ||
!isSwitchValueUsed(statement)) {
+ return configureAST(switchExpressionAsStatement(se), ctx);
Review Comment:
The statement/expression decision occurs only after `visitCommandExpression`
has visited every arm. During that visit, `visitReturnStmtAlt` still rejects
`return` whenever the parser stack contains a `SwitchExpression`, so a genuine
statement such as `switch (i) { case 1 -> { return 'one' } }` remains a
compilation error. Determine statement position before visiting the arms, or
make the control-transfer checks consult that position.
This issue also appears in the following locations of the same file:
- line 2478
- line 2506
> 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)
