fix(eo-runtime): return fallback when no switch case evaluates to true (#5726)#5731
fix(eo-runtime): return fallback when no switch case evaluates to true (#5726)#5731arimu1 wants to merge 2 commits into
Conversation
|
Ran the eo-maven-plugin formatter (-Deo.autoFix) so switch.eo passes the MjFormat canonical-format check that was failing CI.
3c04e41 to
fbc6b76
Compare
|
morphqdd
left a comment
morphqdd
left a comment
There was a problem hiding this comment.
Verified by checking out the branch and running mvn -pl eo-runtime test -Dtest='org.eolang.EOswitchTest' (8/8 green). Core fix is correct and well-scoped. Left inline notes on a few points, the main one being the asymmetry between how the two error paths treat fallback (see the comment on line 36).
| # | ||
| # This object returns the value of the first true statement. | ||
| # This object returns the value of the first true statement, | ||
| # or `fallback` if no condition is true. |
There was a problem hiding this comment.
Good, accurate doc update. Matches the new behavior exactly.
| true.eq found.match | ||
| found.head | ||
| true | ||
| fallback |
There was a problem hiding this comment.
This is the actual fix for #5726, and it's correct: true is gone, fallback is returned instead. But note this returns fallback bare/unapplied, while the empty-cases branch just below (cases.length.eq 0 -> fallback "Switch cases are empty") explicitly calls fallback with a message. I tried to build a repro where a fallback that actually reads its msg parameter would observably differ between the two paths, using EO's msg > [msg] free-attribute idiom, but ran out of time to get a clean one (the semantics of that idiom made it hard to isolate). I'm not confident this is a live bug, but the asymmetry is real and worth a second look, or at least a comment explaining why fallback isn't invoked with a message here the way it is a few lines up.
| * | ||
| false | ||
| "false2" | ||
| eq. ++> tests-switch-with-all-false-cases |
There was a problem hiding this comment.
Good, this is the right regression test for the actual reported bug (previously this called switch with two false cases and no fallback at all, and silently returned true).
| * | ||
| * false "false1" | ||
| * false "false2" | ||
| "fallback" |
There was a problem hiding this comment.
Worth noting: fallback here is a plain string, not a callable (unlike tests-empty-switch-returns-provided-fallback a few lines up, which passes "recovered" > [msg]). That's fine for this test's own purpose, but it means this test can't distinguish "fallback was properly resolved" from "fallback happened to be a non-callable value that dataizes to itself regardless of how it's touched" -- see my note on line 36.
| "TRUE1" | ||
|
|
||
| switch --> on-empty-switch | ||
| switch * --> on-all-false-cases-without-fallback |
There was a problem hiding this comment.
Good new edge-case test: non-empty cases, all false, fallback never bound at all. Confirms the fix raises a real error here instead of silently returning something, which is exactly what #5726 asked for.
| * | ||
| * false "false1" | ||
| * false "false2" | ||
| "fallback" |
There was a problem hiding this comment.
Separate minor point: neither this test nor tests-empty-switch-returns-provided-fallback (nor the two bottom-tests) ever assert what message string actually reaches fallback. "Switch cases are empty" in the empty-cases branch is never checked against anything, since both existing callable-fallback examples ignore their msg parameter. Not a blocker, just a coverage gap if fallback's message content is ever meant to be part of the contract.
| false | ||
| "false2" | ||
|
|
||
| switch * --> on-empty-switch |
There was a problem hiding this comment.
Confirmed this pre-existing test still passes with the fix (empty cases, no fallback bound, correctly still raises).
| true | ||
| fallback | ||
| @. > found | ||
| rec-case cases |
There was a problem hiding this comment.
Confirmed found/rec-case (the recursive matcher that finds the first true case) is untouched by this PR and still behaves correctly for the several-true-cases test -- the fix is isolated to what happens after found comes back empty, which is the right scope for this bug.
| @@ -32,7 +33,7 @@ | |||
| if. | |||
There was a problem hiding this comment.
Confirmed the branch structure here is still canonically formatted (mvn -pl eo-runtime install with the format check enabled reports no issues on this file), so no -Deo.autoFix follow-up needed for this change specifically.
2 participants
Closes #5726
What was changed
In
eo-runtime/src/main/eo/switch.eo, updatedswitchlogic so that when no case evaluates to true in a non-emptycasesarray, it returnsfallbackinstead of returning literaltrue.Updated documentation and unit tests in
switch.eo:switchreturnsfallbackwhen all cases evaluate tofalse.switchwithoutfallbackfails when all cases evaluate tofalse.