Repository navigation
Test a mutable field again after a guard that may have changed it - #9131
Merged
Merged
Conversation
Why:
A guard can change the value being matched. The cases after it read the
mutable field again, but the match compiler kept what its tests had found
on the old value, so a `switch` could take a case that no longer matches
and bind a value of the wrong type (#9129, upstream ocaml/ocaml#7241).
Upstream fixed it in OCaml 5.3; this takes over its erasure of contexts
and replaces its per-position totality with a simpler criterion.
Matching modified (contexts at a mutable read):
Example:
type t = {a: bool, mutable b: (option<int>, option<int>)}
let f = r =>
switch r {
| {a: true, b: (None, _)} => 1
| {a: true, b: (Some(_), _)} if swap(r) => 0
| {a: false} => 3
| {b: (_, None)} => 4
| {b: (Some(x), Some(_))} => x
}
Before (matrix): context of the exit taken when the guard fails, Matching.compile_match
{a: true, b: (Some(_), _)}
After (matrix): the column of the mutable read is forgotten
{a: true, b: _}
Before (js):
let match$1 = r.b;
if (match$1[1] !== undefined) {
return match$1[0];
} else {
return 4;
}
After (js):
let match$1 = r.b;
let x = match$1[0];
if (match$1[1] === undefined) {
return 4;
}
if (x !== undefined) {
return x;
}
throw {
RE_EXN_ID: "Match_failure",
…
};
Matching modified (exhaustiveness after a guard):
Example:
type flagged = {d: bool, mutable c: option<int>}
let flagged = r =>
switch r {
| {c: None} => 0
| {d: true} if clear(r) => 1
| {c: Some(x)} => x
}
Before (text): Matching.check_partial
Partial when one case both has a guard and reads a mutable field
After (text): Matching.check_partial
Partial when a case has a guard and a case reads a mutable field
Before (js):
if (r.d && (r.c = undefined, false)) {
return 1;
} else {
return r.c;
}
After (js):
if (r.d && (r.c = undefined, false)) {
return 1;
}
let x = r.c;
if (x !== undefined) {
return x;
}
throw {
RE_EXN_ID: "Match_failure",
…
};
Review: Why not upstream's per-position totality (a mut flag on each argument and a First/Following tempo)? Without lazy patterns, and on one thread, only a guard can change the value during a match, so marking the whole match Partial when it has a guard and reads a mutable field is enough. It needs no new state in the compiler; the cost is a failure branch at immutable positions of such matches too. No committed output in the stdlib, Belt or tests changes.
Review: Why are array elements not counted in check_partial? A Total match on arrays has a case matching any array, and the cases after a guard fall through to it; the contexts are still erased after an element is read.
Review: Why are dict patterns left alone? A dict entry is read once into a variable and the cases after the guard keep using it, so they see one consistent value.
Fixes: #9129
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kbf3gLpf7RN1MiRcubPgRn
Signed-off-by: Christoph Knittel <ck@cca.io>
Changelog🐛 Bug fix
Semantic Diff1 commit · Full page Test a mutable field again after a guard that may have changed it51592af · cknitt + Claude · Oct 29, 2026, 19:47 UTC
Matching modified (contexts at a mutable read) type t = {a: bool, mutable b: (option<int>, option<int>)}
let f = r =>
switch r {
| {a: true, b: (None, _)} => 1
| {a: true, b: (Some(_), _)} if swap(r) => 0
| {a: false} => 3
| {b: (_, None)} => 4
| {b: (Some(x), Some(_))} => x
}
-{a: true, b: (Some(_), _)}
+{a: true, b: _}
let match$1 = r.b;
-if (match$1[1] !== undefined) {
- return match$1[0];
-} else {
+let x = match$1[0];
+if (match$1[1] === undefined) {
return 4;
}
+if (x !== undefined) {
+ return x;
+}
+throw {
+ RE_EXN_ID: "Match_failure",
+ …
+};Matching modified (exhaustiveness after a guard) type flagged = {d: bool, mutable c: option<int>}
let flagged = r =>
switch r {
| {c: None} => 0
| {d: true} if clear(r) => 1
| {c: Some(x)} => x
}
-Partial when one case both has a guard and reads a mutable field
+Partial when a case has a guard and a case reads a mutable field
if (r.d && (r.c = undefined, false)) {
return 1;
-} else {
- return r.c;
}
+let x = r.c;
+if (x !== undefined) {
+ return x;
+}
+throw {
+ RE_EXN_ID: "Match_failure",
+ …
+};Review
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9131 +/- ##
==========================================
- Coverage 81.71% 81.70% -0.01%
==========================================
Files 460 460
Lines 64761 64767 +6
==========================================
+ Hits 52918 52920 +2
- Misses 11843 11847 +4
🚀 New features to boost your workflow:
|
Member
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-9131 |
cknitt
marked this pull request as ready for review
October 29, 2026 20:24
cristianoc
approved these changes
Oct 29, 2026
cristianoc
left a comment
cristianoc
left a comment
Collaborator
There was a problem hiding this comment.
Looks good to me. Thanks for the fix.
aspeddro
pushed a commit
to aspeddro/rescript-compiler
that referenced
this pull request
Oct 31, 2026
…script-lang#9131) Signed-off-by: Pedro Castro <aspeddro@gmail.com>
This was referenced Nov 1, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #9129.
A guard can change the value being matched. The cases after a failed guard read the mutable field again, but the match compiler still trusted the tests it had made on the old value, so a
switchcould take a case that no longer matches and bindundefinedat typeint. This is upstream OCaml's #7241, fixed in OCaml 5.3.Two changes in
compiler/ml/matching.ml:bind_match_arg): exits that leave the binding of a mutable field or an array element forget what the tests on it found, so the code they jump to tests the value it reads again.check_partialmakes a match Partial when a case has a guard and a case reads a mutable field. Before, both had to be the same case, so{c: None} => 0 | {d: true} if f() => 1 | {c: Some(x)} => xstayed Total and its last case readr.cwithout testing it. A Partial match has a default exit, so the cases after the guard can fail withMatch_failure, as they do upstream.Upstream tracks instead whether each argument lies below a mutable field, and whether a jump has happened, and marks only those switches Partial. Without lazy patterns, and on one thread, only a guard can change the value during a match, so the coarser rule is enough. Its cost is a failure branch at immutable positions of matches that have both a guard and a mutable field; no committed output in the stdlib, Belt or the tests changes.
Dict patterns are not affected: an entry is read once and the cases after the guard keep using that value.
New test:
tests/tests/src/match_mutation_guard_test.rescovers the #9129 shape and a guarded case that reads no mutable field. Both now throwMatch_failurewhen the guard mutates the value into no case. It also checks that guards which mutate into a later case, or mutate nothing, still pick the right case.🤖 Generated with Claude Code
https://claude.ai/code/session_01Kbf3gLpf7RN1MiRcubPgRn