Skip to content

Commit 9d25b40

Browse files
cknittcknitt
authored
Test a mutable field again after a guard that may have changed it (#9131)
1 parent d389139 commit 9d25b40

4 files changed

Lines changed: 247 additions & 7 deletions

File tree

‎changes/match-guard-mutation.fix.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
- Test a mutable field again after a guard that may have changed it, instead of taking a `switch` case that no longer matches.

‎compiler/ml/matching.ml‎

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,15 @@ let rshift {left; right} =
103103

104104
let ctx_rshift ctx = List.map rshift ctx
105105

106+
(* Forget what the contexts know about the first column *)
107+
let ctx_erase_first_col ctx =
108+
List.map
109+
(fun {left; right} ->
110+
match right with
111+
| _ :: right -> {left; right = omega :: right}
112+
| [] -> fatal_error "Matching.ctx_erase_first_col")
113+
ctx
114+
106115
let rec nchars n ps =
107116
if n <= 0 then ([], ps)
108117
else
@@ -2788,6 +2797,15 @@ let rec compile_match repr partial ctx m =
27882797
((if dbg then do_compile_matching_pr else do_compile_matching) repr)
27892798
partial ctx newarg first_match rem
27902799
in
2800+
(* A mutable field or array element is read once into [v] here. The code
2801+
that the exits jump to reads it again and may see another value, as a
2802+
guard may have changed it, so what the tests on [v] found doesn't hold
2803+
there. *)
2804+
let total =
2805+
match str with
2806+
| StrictOpt -> jumps_map ctx_erase_first_col total
2807+
| Strict | Alias | Variable -> total
2808+
in
27912809
(bind_check str v arg lam, total)
27922810
| _ -> assert false
27932811

@@ -2916,10 +2934,12 @@ let have_mutable_field p =
29162934

29172935
let is_mutable p = find_in_pat have_mutable_field p
29182936

2919-
(* Downgrade Total when
2920-
1. Matching accesses some mutable fields;
2921-
2. And there are guards or lazy patterns.
2922-
*)
2937+
(* Downgrade Total when the matching reads some mutable field and has a guard.
2938+
The guard may change the field, and the cases after it read it again: the
2939+
value they see may match none of them. The guarded case need not read the
2940+
field, e.g. [{c: None} => 0 | {d: true} if f() => 1 | {c: Some(x)} => x].
2941+
Array elements are mutable too, but a Total matching on arrays has a case
2942+
that matches any array, and the cases after a guard fall through to it. *)
29232943

29242944
let check_partial is_mutable pat_act_list = function
29252945
| Partial -> Partial
@@ -2928,9 +2948,8 @@ let check_partial is_mutable pat_act_list = function
29282948
pat_act_list = []
29292949
||
29302950
(* allow empty case list *)
2931-
List.exists
2932-
(fun (pats, (act : action)) -> is_mutable pats && act.guard <> None)
2933-
pat_act_list
2951+
List.exists (fun (_, (act : action)) -> act.guard <> None) pat_act_list
2952+
&& List.exists (fun (pats, _) -> is_mutable pats) pat_act_list
29342953
then Partial
29352954
else Total
29362955

‎tests/tests/src/match_mutation_guard_test.mjs‎

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,153 @@
1+
// Generated by ReScript, PLEASE EDIT WITH CARE
2+
3+
import * as Mocha from "mocha";
4+
import * as Test_utils from "./test_utils.mjs";
5+
6+
let nextB = {
7+
contents: undefined
8+
};
9+
10+
function setB(r) {
11+
let b = nextB.contents;
12+
if (b !== undefined) {
13+
r.b = b;
14+
}
15+
return false;
16+
}
17+
18+
function pair(r) {
19+
if (!r.a) {
20+
return 3;
21+
}
22+
let match = r.b;
23+
if (match[0] === undefined) {
24+
return 1;
25+
}
26+
if (setB(r)) {
27+
return 0;
28+
}
29+
let match$1 = r.b;
30+
let x = match$1[0];
31+
if (match$1[1] === undefined) {
32+
return 4;
33+
}
34+
if (x !== undefined) {
35+
return x;
36+
}
37+
throw {
38+
RE_EXN_ID: "Match_failure",
39+
_1: [
40+
"match_mutation_guard_test.res",
41+
21,
42+
2
43+
],
44+
Error: new Error()
45+
};
46+
}
47+
48+
function pairWith(b, next) {
49+
nextB.contents = next;
50+
return pair({
51+
a: true,
52+
b: b
53+
});
54+
}
55+
56+
function clear(r) {
57+
r.c = undefined;
58+
return false;
59+
}
60+
61+
function flagged(r) {
62+
let match = r.c;
63+
if (match === undefined) {
64+
return 0;
65+
}
66+
if (r.d && (r.c = undefined, false)) {
67+
return 1;
68+
}
69+
let x = r.c;
70+
if (x !== undefined) {
71+
return x;
72+
}
73+
throw {
74+
RE_EXN_ID: "Match_failure",
75+
_1: [
76+
"match_mutation_guard_test.res",
77+
43,
78+
2
79+
],
80+
Error: new Error()
81+
};
82+
}
83+
84+
Mocha.describe("Match_mutation_guard_test", () => {
85+
Mocha.test("guards that mutate nothing", () => {
86+
Test_utils.eq("File \"match_mutation_guard_test.res\", line 51, characters 7-14", 1, pairWith([
87+
undefined,
88+
undefined
89+
], undefined));
90+
Test_utils.eq("File \"match_mutation_guard_test.res\", line 52, characters 7-14", 4, pairWith([
91+
1,
92+
undefined
93+
], undefined));
94+
Test_utils.eq("File \"match_mutation_guard_test.res\", line 53, characters 7-14", 7, pairWith([
95+
7,
96+
2
97+
], undefined));
98+
Test_utils.eq("File \"match_mutation_guard_test.res\", line 54, characters 7-14", 3, pair({
99+
a: false,
100+
b: [
101+
undefined,
102+
undefined
103+
]
104+
}));
105+
Test_utils.eq("File \"match_mutation_guard_test.res\", line 55, characters 7-14", 5, flagged({
106+
d: false,
107+
c: 5
108+
}));
109+
});
110+
Mocha.test("a guard that mutates into a later case", () => {
111+
Test_utils.eq("File \"match_mutation_guard_test.res\", line 59, characters 7-14", 8, pairWith([
112+
1,
113+
undefined
114+
], [
115+
8,
116+
2
117+
]));
118+
Test_utils.eq("File \"match_mutation_guard_test.res\", line 60, characters 7-14", 4, pairWith([
119+
1,
120+
2
121+
], [
122+
undefined,
123+
undefined
124+
]));
125+
});
126+
Mocha.test("a guard that mutates into no case", () => {
127+
Test_utils.throws("File \"match_mutation_guard_test.res\", line 64, characters 11-18", () => {
128+
pairWith([
129+
1,
130+
undefined
131+
], [
132+
undefined,
133+
2
134+
]);
135+
});
136+
Test_utils.throws("File \"match_mutation_guard_test.res\", line 65, characters 11-18", () => {
137+
flagged({
138+
d: true,
139+
c: 5
140+
});
141+
});
142+
});
143+
});
144+
145+
export {
146+
nextB,
147+
setB,
148+
pair,
149+
pairWith,
150+
clear,
151+
flagged,
152+
}
153+
/* Not a pure module */

‎tests/tests/src/match_mutation_guard_test.res‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
open Mocha
2+
open Test_utils
3+
4+
// A guard may change the value being matched. The cases after it read the
5+
// changed value, so they must test it again and may all fail to match it.
6+
7+
type pair = {a: bool, mutable b: (option<int>, option<int>)}
8+
9+
let nextB = ref(None)
10+
11+
let setB = r => {
12+
switch nextB.contents {
13+
| Some(b) => r.b = b
14+
| None => ()
15+
}
16+
false
17+
}
18+
19+
// The tests on b.0 before the guard must not be trusted after it (#9129).
20+
let pair = r =>
21+
switch r {
22+
| {a: true, b: (None, _)} => 1
23+
| {a: true, b: (Some(_), _)} if setB(r) => 0
24+
| {a: false} => 3
25+
| {b: (_, None)} => 4
26+
| {b: (Some(x), Some(_))} => x
27+
}
28+
29+
let pairWith = (b, next) => {
30+
nextB := next
31+
pair({a: true, b})
32+
}
33+
34+
type flagged = {d: bool, mutable c: option<int>}
35+
36+
let clear = r => {
37+
r.c = None
38+
false
39+
}
40+
41+
// The guarded case reads no mutable field, but the case after it does.
42+
let flagged = r =>
43+
switch r {
44+
| {c: None} => 0
45+
| {d: true} if clear(r) => 1
46+
| {c: Some(x)} => x
47+
}
48+
49+
describe(__MODULE__, () => {
50+
test("guards that mutate nothing", () => {
51+
eq(__LOC__, 1, pairWith((None, None), None))
52+
eq(__LOC__, 4, pairWith((Some(1), None), None))
53+
eq(__LOC__, 7, pairWith((Some(7), Some(2)), None))
54+
eq(__LOC__, 3, pair({a: false, b: (None, None)}))
55+
eq(__LOC__, 5, flagged({d: false, c: Some(5)}))
56+
})
57+
58+
test("a guard that mutates into a later case", () => {
59+
eq(__LOC__, 8, pairWith((Some(1), None), Some((Some(8), Some(2)))))
60+
eq(__LOC__, 4, pairWith((Some(1), Some(2)), Some((None, None))))
61+
})
62+
63+
test("a guard that mutates into no case", () => {
64+
throws(__LOC__, () => ignore(pairWith((Some(1), None), Some((None, Some(2))))))
65+
throws(__LOC__, () => ignore(flagged({d: true, c: Some(5)})))
66+
})
67+
})

0 commit comments

Comments
 (0)