Skip to content

Commit 17e4cdc

Browse files
cristianocclaude
andcommitted
Keep a case's guard as data until its fallthrough is known
Matching encoded a guard as a term and recovered it afterwards by shape: translcore emitted [if cond then body else Lstaticraise (0, [])], and is_guarded / patch_guarded recognised that shape to substitute the real fallthrough. Exit zero was a sentinel, not an exit. Normalization cannot know a shape is a message. Fold the condition of [x if 1 > 2] and if_ correctly returns the else branch, so the action becomes a bare raise to an exit that has no catch, is_guarded stops recognising it, nothing patches it, and the raise reaches codegen alone. That is what broke the analysis corpus when mk_builtin started folding. Carry the guard as data instead. A case's right-hand side is now type action = { binds: (let_kind * Ident.t * Lambda.t) list; guard: Lambda.t option; body: Lambda.t; } lowered at the single point where the fallthrough exists - the row that matches in compile_match. The bindings are there because simplification brings pattern variables into scope with lets that must cover the guard as well as the body; keeping them as data preserves that without a term to recurse through. staticfail, is_guarded and patch_guarded are deleted, and exit zero is no longer magic. Comparing actions needs a stand-in for the fallthrough, and it must be a fresh variable rather than a constant: with unit, [when g => e] and [_ => if g then e else ()] produce the same key, and merging them loses one evaluation of g. The variable is created per comparison, so its freshness does not depend on ident stamps surviving Ident.reinit between units. guard_action_test pins both: a guard that folds to false is not a missing guard, and two actions that differ only in where the condition sits are not the same action. Each fails on the unfixed compiler. Generated JavaScript is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
1 parent 7875e74 commit 17e4cdc

7 files changed

Lines changed: 158 additions & 49 deletions

File tree

compiler/ml/lambda.ml

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1457,19 +1457,6 @@ let next_negative_raise_count () =
14571457
!negative_raise_count
14581458

14591459
(* Anticipated staticraise, for guards *)
1460-
let staticfail = Lstaticraise (0, [])
1461-
1462-
let rec is_guarded = function
1463-
| Lifthenelse (_cond, _body, Lstaticraise (0, [])) -> true
1464-
| Llet (_str, _id, _lam, body) -> is_guarded body
1465-
| _ -> false
1466-
1467-
let rec patch_guarded patch = function
1468-
| Lifthenelse (cond, body, Lstaticraise (0, [])) ->
1469-
Lifthenelse (cond, body, patch)
1470-
| Llet (str, id, lam, body) -> Llet (str, id, lam, patch_guarded patch body)
1471-
| _ -> assert false
1472-
14731460
(* Translate an access path *)
14741461

14751462
let rec transl_normal_path = function

compiler/ml/lambda.mli

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -599,9 +599,3 @@ val next_negative_raise_count : unit -> int
599599
exception x -> ...'. This disabled some simplifications
600600
performed by the Simplif module that assume that static raises
601601
are in tail position in their handler. *)
602-
603-
val staticfail : t (* Anticipated static failure *)
604-
605-
(* Check anticipated failure, substitute its final value *)
606-
val is_guarded : t -> bool
607-
val patch_guarded : t -> t -> t

compiler/ml/matching.ml

Lines changed: 71 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -345,8 +345,56 @@ let jumps_map f env = List.map (fun (i, pss) -> (i, f pss)) env
345345

346346
(* Pattern matching before any compilation *)
347347

348+
(* A case's right-hand side, kept as data until the fallthrough it may need
349+
is known. Encoding the guard as a term and recognizing it by shape
350+
afterwards would let normalization erase it: a guard that folds to false
351+
is not a missing guard. Simplification brings pattern variables into
352+
scope with lets, which must cover the guard as well as the body, so those
353+
accumulate here too, outermost first. *)
354+
type action = {
355+
binds: (let_kind * Ident.t * Lambda.t) list;
356+
guard: Lambda.t option;
357+
body: Lambda.t;
358+
}
359+
360+
let unguarded body = {binds = []; guard = None; body}
361+
let guarded ~guard body = {binds = []; guard = Some guard; body}
362+
363+
(* Bring [id = e] into scope over the whole right-hand side. Like
364+
[Lambda.bind], an alias of a variable to itself is dropped. *)
365+
let bind_action kind id e (a : action) =
366+
match e with
367+
| Lvar v when Ident.same v id -> a
368+
| _ -> {a with binds = (kind, id, e) :: a.binds}
369+
370+
let action_body_with ~fail {binds; guard; body} =
371+
let core =
372+
match guard with
373+
| None -> body
374+
| Some g -> if_ g body fail
375+
in
376+
List.fold_right (fun (k, id, e) acc -> let_ k id e acc) binds core
377+
378+
(* For comparing actions by key. The stand-in for the fallthrough is a fresh
379+
variable, shared by both sides so the keys stay comparable, and impossible
380+
for a real action to mention. A constant such as unit would not do: it
381+
would make [when g => e] and [_ => if g then e else ()] compare equal, and
382+
merging those loses one evaluation of [g]. *)
383+
let action_key_term ~fail a = action_body_with ~fail a
384+
385+
let action_free_variables {binds; guard; body} =
386+
let inner =
387+
match guard with
388+
| None -> free_variables body
389+
| Some g -> Set_ident.union (free_variables body) (free_variables g)
390+
in
391+
List.fold_right
392+
(fun (_, id, e) acc ->
393+
Set_ident.union (free_variables e) (Set_ident.remove acc id))
394+
binds inner
395+
348396
type pattern_matching = {
349-
mutable cases: (pattern list * Lambda.t) list;
397+
mutable cases: (pattern list * action) list;
350398
args: (Lambda.t * let_kind) list;
351399
default: (matrix * int) list;
352400
}
@@ -471,10 +519,9 @@ let same_actions = function
471519

472520
(* Test for swapping two clauses *)
473521

474-
let up_ok_action act1 act2 =
475-
try
476-
let raw1 = tr_raw act1 and raw2 = tr_raw act2 in
477-
raw1 = raw2
522+
let up_ok_action (a1 : action) (a2 : action) =
523+
let fail = Lambda.var (Ident.create "fallthrough") in
524+
try tr_raw (action_key_term ~fail a1) = tr_raw (action_key_term ~fail a2)
478525
with Exit -> false
479526

480527
let up_ok (ps, act_p) l =
@@ -514,7 +561,7 @@ let simplify_or p =
514561
try simpl_rec p with Var p -> p
515562

516563
let bind_record_rest loc arg rest action =
517-
let_ Strict rest.rest_ident
564+
bind_action Strict rest.rest_ident
518565
(prim ~primitive:(Precord_rest rest.excluded_runtime_labels) ~args:[arg] loc)
519566
action
520567

@@ -527,10 +574,10 @@ let simplify_cases args cls =
527574
| ((pat :: patl, action) as cl) :: rem -> (
528575
match pat.pat_desc with
529576
| Tpat_var (id, _) ->
530-
(omega :: patl, bind Alias id arg action) :: simplify rem
577+
(omega :: patl, bind_action Alias id arg action) :: simplify rem
531578
| Tpat_any -> cl :: simplify rem
532579
| Tpat_alias (p, id, _) ->
533-
simplify ((p :: patl, bind Alias id arg action) :: rem)
580+
simplify ((p :: patl, bind_action Alias id arg action) :: rem)
534581
| Tpat_record ([], _, rest) ->
535582
let action =
536583
match rest with
@@ -643,7 +690,7 @@ let rec explode_or_pat arg patl mk_action rem vars aliases = function
643690

644691
let pm_free_variables {cases} =
645692
List.fold_right
646-
(fun (_, act) r -> Set_ident.union (free_variables act) r)
693+
(fun (_, act) r -> Set_ident.union (action_free_variables act) r)
647694
cases Set_ident.empty
648695

649696
(* Basic grouping predicates *)
@@ -698,7 +745,7 @@ let is_or p =
698745
(* Conditions for appending to the Or matrix *)
699746
let conda p q = not (may_compat p q)
700747

701-
and condb act ps qs = (not (is_guarded act)) && Parmatch.le_pats qs ps
748+
and condb (act : action) ps qs = act.guard = None && Parmatch.le_pats qs ps
702749

703750
let or_ok p ps l =
704751
List.for_all
@@ -1046,7 +1093,7 @@ and precompile_or argo cls ors args def k =
10461093
let new_patl = Parmatch.omega_list patl in
10471094

10481095
let mk_new_action vs =
1049-
staticraise or_num (List.map (fun v -> var v) vs)
1096+
unguarded (staticraise or_num (List.map (fun v -> var v) vs))
10501097
in
10511098

10521099
let body, handlers = do_cases rem in
@@ -2416,11 +2463,15 @@ let arg_to_var arg cls =
24162463
let rec compile_match repr partial ctx m =
24172464
match m with
24182465
| {cases = []; args = []} -> comp_exit ctx m
2419-
| {cases = ([], action) :: rem} ->
2420-
if is_guarded action then
2421-
let lambda, total = compile_match None partial ctx {m with cases = rem} in
2422-
(patch_guarded lambda action, total)
2423-
else (action, jumps_empty)
2466+
| {cases = ([], action) :: rem} -> (
2467+
(* The row matches. An unguarded action is the result; a guarded one
2468+
falls through to the remaining rows when the guard fails, so those
2469+
are compiled first and become the alternative. *)
2470+
match action.guard with
2471+
| None -> (action_body_with ~fail:lambda_unit action, jumps_empty)
2472+
| Some _ ->
2473+
let fail, total = compile_match None partial ctx {m with cases = rem} in
2474+
(action_body_with ~fail action, total))
24242475
| {args = (arg, str) :: argl} ->
24252476
let v, newarg = arg_to_var arg m.cases in
24262477
let first_match, rem =
@@ -2568,7 +2619,7 @@ let check_partial is_mutable pat_act_list = function
25682619
||
25692620
(* allow empty case list *)
25702621
List.exists
2571-
(fun (pats, lam) -> is_mutable pats && is_guarded lam)
2622+
(fun (pats, (act : action)) -> is_mutable pats && act.guard <> None)
25722623
pat_act_list
25732624
then Partial
25742625
else Total
@@ -2641,7 +2692,9 @@ let for_trywith param pat_act_list =
26412692
param pat_act_list Partial
26422693

26432694
let simple_for_let loc param pat body =
2644-
compile_matching None (partial_function loc) param [(pat, body)] Partial
2695+
compile_matching None (partial_function loc) param
2696+
[(pat, unguarded body)]
2697+
Partial
26452698

26462699
(* Optimize binding of immediate tuples
26472700

compiler/ml/matching.mli

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -33,22 +33,27 @@ val make_test_sequence_variant_constant :
3333
(Lambda.t option -> Lambda.t -> (int * (string * Lambda.t)) list -> Lambda.t)
3434
ref
3535

36+
(* A case's right-hand side. The guard is kept apart from the body until the
37+
match compiler knows what it falls through to; it is not encoded as a term
38+
to be recognized by shape later. *)
39+
type action
40+
41+
val unguarded : Lambda.t -> action
42+
43+
val guarded : guard:Lambda.t -> Lambda.t -> action
44+
3645
(* Entry points to match compiler *)
3746
val for_function :
3847
Location.t ->
3948
int ref option ->
4049
Lambda.t ->
41-
(pattern * Lambda.t) list ->
50+
(pattern * action) list ->
4251
partial ->
4352
Lambda.t
44-
val for_trywith : Lambda.t -> (pattern * Lambda.t) list -> Lambda.t
53+
val for_trywith : Lambda.t -> (pattern * action) list -> Lambda.t
4554
val for_let : Location.t -> Lambda.t -> pattern -> Lambda.t -> Lambda.t
4655
val for_multiple_match :
47-
Location.t ->
48-
Lambda.t list ->
49-
(pattern * Lambda.t) list ->
50-
partial ->
51-
Lambda.t
56+
Location.t -> Lambda.t list -> (pattern * action) list -> partial -> Lambda.t
5257

5358
exception Cannot_flatten
5459

compiler/ml/translcore.ml

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1300,10 +1300,10 @@ and transl_exp0 (e : Typedtree.expression) : Lambda.t =
13001300
and transl_list expr_list = List.map transl_exp expr_list
13011301

13021302
and transl_guard guard rhs =
1303-
let expr = transl_exp rhs in
1303+
let body = transl_exp rhs in
13041304
match guard with
1305-
| None -> expr
1306-
| Some cond -> if_ (transl_exp cond) expr staticfail
1305+
| None -> Matching.unguarded body
1306+
| Some cond -> Matching.guarded ~guard:(transl_exp cond) body
13071307

13081308
and transl_case {c_lhs; c_guard; c_rhs} = (c_lhs, transl_guard c_guard c_rhs)
13091309

@@ -1384,13 +1384,15 @@ and transl_function loc (params : function_param list) body =
13841384
| [{fp_param; fp_pat; fp_partial}] ->
13851385
( [fp_param],
13861386
Matching.for_function loc None (var fp_param)
1387-
[(fp_pat, transl_exp body)]
1387+
[(fp_pat, Matching.unguarded (transl_exp body))]
13881388
fp_partial,
13891389
is_base_type body.exp_env body.exp_type Predef.path_unit )
13901390
| {fp_param; fp_pat; fp_partial} :: rest ->
13911391
let lparams, lbody, return_unit = transl_function loc rest body in
13921392
( fp_param :: lparams,
1393-
Matching.for_function loc None (var fp_param) [(fp_pat, lbody)] fp_partial,
1393+
Matching.for_function loc None (var fp_param)
1394+
[(fp_pat, Matching.unguarded lbody)]
1395+
fp_partial,
13941396
return_unit )
13951397

13961398
and transl_let ~js_hoist rec_flag pat_expr_list body =
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
// Generated by ReScript, PLEASE EDIT WITH CARE
2+
3+
4+
let calls = {
5+
contents: 0
6+
};
7+
8+
function guard() {
9+
calls.contents = calls.contents + 1 | 0;
10+
return false;
11+
}
12+
13+
function both_guards_run() {
14+
calls.contents = 0;
15+
if (guard()) {
16+
17+
} else {
18+
guard();
19+
}
20+
return calls.contents;
21+
}
22+
23+
let constant_guard = "right";
24+
25+
export {
26+
constant_guard,
27+
calls,
28+
guard,
29+
both_guards_run,
30+
}
31+
/* No side effect */
Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
// A guard is kept apart from its body until the match compiler knows what it
2+
// falls through to. Two things must hold.
3+
4+
// 1. Folding must not erase the fact that a case is guarded: a guard that
5+
// folds to false is not a missing guard.
6+
let constant_guard = switch true {
7+
| true if false => "wrong"
8+
| _ => "right"
9+
}
10+
11+
// 2. A guarded case and a case whose body happens to be the same conditional
12+
// are different actions. Comparing them through a stand-in fallthrough must
13+
// not equate them, or one evaluation of the guard is lost.
14+
type value = A | B | C
15+
16+
let calls = ref(0)
17+
18+
let guard = () => {
19+
calls := calls.contents + 1
20+
false
21+
}
22+
23+
let both_guards_run = () => {
24+
calls := 0
25+
switch B {
26+
| A => ()
27+
| _ if guard() => ()
28+
| B =>
29+
if guard() {
30+
()
31+
} else {
32+
()
33+
}
34+
| _ => ()
35+
}
36+
calls.contents
37+
}

0 commit comments

Comments
 (0)