From ad0f1fbd6a8aa1d0a97e50ece53dabb1ef19ea20 Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sun, 20 Sep 2026 09:56:34 +0700 Subject: [PATCH] or hands back its deciding operand instead of a bare bool, review pass on item 7 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dyn if truthiness review turned up that or's answer position, unlike and's, still traps on a non-bool dyn value: or's short-circuit sentinel sat in the then arm of its own if, the one check_if types first, so that sentinel decided the whole expression's type and a later non-bool dyn answer hit the strict bool boundary and unboxed itself into a trap rather than surviving as itself. (or nil "x") — the canonical Clojure (or x default) idiom — crashed instead of answering "x", identical on all three backends. or now binds its test to a temp and answers the temp itself, exactly the way Clojure's own or macro expands: (or a b) becomes (let [t a] (if t t b)), not (if a true b). The temp evaluates a once and lets the answer be a without writing it a second time as the then arm; it is the temp's own type check_if sees first, so or hands back the actual truthy operand the same way and always has. Verified real output, unchanged, on LLVM, -O0 and --x86, and the survey program now exercises the case its own header used to exclude for being unsafe: a non-bool value stopping or and being handed back as-is. check_truthy also gets three corrections a closer look found. Its own [loc] used to come from the enclosing if/while/not rather than from the condition itself, so the rt call and cast it builds carried the wrong column in an --x86 disassembly or the dev inspector whenever the condition was not the form's first token; it now takes loc from the scrutinee's own AST node, confirmed against a real --x86 dump. A comment now names the precondition its exception-swallowing retry rests on: none of check.ml's save-restore sites (barrier, in_frames, in_defer, loops, scope) are exception-safe, which is harmless only because the retry always either succeeds cleanly or re-raises and aborts the compile before ctx is read again — and would stop being harmless the day some want-sensitive elaboration on this path could succeed differently on retry. And a bare keyword condition, which used to be checked with want:Bool from the start and refused by the keyword arm's enum-or-refuse case, now resolves as the dyn keyword instead and is unconditionally truthy — a deliberate loss of that diagnostic, the author's call, pinned in test_flan.ml so it does not regress by accident. The two typed-refusal messages captured before this pass (a float literal condition, an i32 while condition) are unchanged, checked again against the same baseline. test_flan.ml's parser test for or's shape is updated to match the new let-bound desugaring. --- lib/check.ml | 69 ++++++++++++++++++++++++++++---- lib/parse.ml | 35 ++++++++++------ test/programs/dyn-if-truthy.flan | 28 ++++++++----- test/test_acceptance.ml | 18 +++++---- test/test_flan.ml | 19 ++++++++- 5 files changed, 130 insertions(+), 39 deletions(-) diff --git a/lib/check.ml b/lib/check.ml index d62177a..788f917 100644 --- a/lib/check.ml +++ b/lib/check.ml @@ -2032,7 +2032,7 @@ let rec check ctx ?want (e : Ast.expr) : Tast.expr = top of every trip — but it stays outside [in_loop], because a [break] in a condition still means the enclosing loop and a [defer] there is still the outer block's. *) - let c = check_truthy ctx loc c in + let c = check_truthy ctx c in let body = in_loop ctx ?label (fun () -> scoped ctx (fun () -> map_lr (fun b -> check ctx b) body)) in @@ -3177,9 +3177,10 @@ and check_recur ctx ~tail loc args = (* Every boolean-position test in the language funnels through here: [if]'s own condition, [while]'s, and [not]'s argument. ([when] and [cond] are sugar built out of [Ast.If] in parse.ml, so they get this for free - without a separate case. [and] and [or] are sugar too, and their *tests* - get this for free the same way — see [shortcircuit] in parse.ml for why - [or]'s answer position does not.) + without a separate case. [and] and [or] are sugar too, and both their + tests and their answers get this for free the same way — see + [shortcircuit] in parse.ml for how [or] carries its deciding operand + through, the same way [and] always has.) A dyn scrutinee is tested for truthiness, Clojure's rule: nil and false are the only falsey values, and everything else — 0, "", an empty vec, an @@ -3199,8 +3200,60 @@ and check_recur ctx ~tail loc args = found None". Asking the first way first is what makes the dyn case work, so the refusal path — success or exception both — asks the second way again, after the fact, purely to get the sentence a typed if has always - given. *) -and check_truthy ctx loc c = + given. + + [loc] comes from [c] itself, not from the caller's [if]/[while]/[not] — + the built-in rt call and cast have to sit at the condition's own position + or an --x86 disassembly and the dev inspector point at the wrong column + when the two differ (an [if] whose test is not its first token). + + The [exception Loc.Error _] arm swallows a rejection from arbitrary depth + inside [c] and re-runs the whole of [check ctx c] a second time, which is + sound only because every one of [ctx]'s save-restore sites — [barrier], + and the [in_frames]/[in_defer]/[loops]/[scope] plumbing [check] itself + uses — is not exception-safe: a failure mid-walk can leave one of those + pushed without its pop. Today that is harmless, because the second call + always either succeeds outright or raises again and this function's own + caller then aborts the compile — nothing downstream ever reads [ctx] + again on that path. It would stop being harmless the day some later + want-sensitive elaboration on this path can *succeed* by yielding a + concrete [Bool] on retry rather than failing a second time: then the + first, swallowed pass's half-restored state and any name or slot it + registered before raising would both still be live. + + That same retry-on-failure is also, deliberately, not made cheaper by + only retrying at the leaf that actually needs a nicer message (an int or + float literal, or [None]) and re-raising everywhere else: the shorter + path was tried and shelved, because "everywhere else" is not safe to + generalise past [not]'s one level of nesting — a condition that is + itself a compound form carrying its own literals arbitrarily deep (an + [if] or [let] standing where a condition is expected) would need [want] + threaded through exactly as far as this function's own second call + already threads it, and stopping short changes which of *those* + literals gets the nicer message, not just the speed. The cost that + buys is real: nested [not] on a program that does not type-check re-runs + this whole function once per level of nesting inside the level above it, + which is exponential in how deep the nesting goes — moot for a program + that compiles, since neither retry ever fires, and moot for ordinary + nesting depths, but visible within a second or so around twenty levels + of a [not] wrapped in a [not] wrapped in .... The dev daemon is the one + caller that could feel this, recompiling a half-typed form on every + edit; nobody has hit it in practice and it is not fixed here. + + Keywords are a separate, deliberate loss rather than a bug: a bare + [:kw] used to be checked here with [want:Types.Bool] from the start, so + it hit the keyword arm's [Some other] case and refused by name — "is an + enum member where an enum is expected and a dyn keyword elsewhere, but + bool is expected here". Checking it here with no expectation first, as + every other scrutinee now is, resolves it as the dyn keyword instead + (there being no enum in play), and dyn keywords are unconditionally + truthy — so [(if :kw a b)] now takes [a], where it used to refuse + outright. The author's call: lispy truthiness wins wherever it can, so + this refusal is given up on purpose and not special-cased back in; + test_flan.ml pins the new answer down so it is not lost again by + accident. *) +and check_truthy ctx c = + let loc = c.Ast.loc in match check ctx c with | c0 when c0.Tast.ty = Types.Dyn -> widen loc Types.Bool (rt loc (Types.Int Types.I32) "flan_dyn_truthy" [ c0 ]) @@ -3209,7 +3262,7 @@ and check_truthy ctx loc c = | exception Loc.Error _ -> check ctx ~want:Types.Bool c and check_if ctx ?(tail = false) ?want loc c t e = - let c = check_truthy ctx loc c in + let c = check_truthy ctx c in (* Both arms are the tail, and a one-armed [if] counts: [(when c (recur ...))] is how nearly every loop is written, and the branch is still the last thing the body does. *) @@ -4249,7 +4302,7 @@ and named_call ctx ~want loc name args = arity loc name 1 args; (* Same truthiness as [if]: a dyn argument is negated on nil/false vs. everything else, not narrowed to a strict bool first. *) - prim Tast.Not Types.Bool [ check_truthy ctx loc (List.hd args) ] + prim Tast.Not Types.Bool [ check_truthy ctx (List.hd args) ] (* Bitwise operators are integers-only, and the shift count has the same type as the value shifted — there is no implicit widening anywhere else either. *) | "bit-and" | "bit-or" | "bit-xor" -> diff --git a/lib/parse.ml b/lib/parse.ml index d2bf07b..aab0b9e 100644 --- a/lib/parse.ml +++ b/lib/parse.ml @@ -882,17 +882,24 @@ and cond f (args : Form.t list) : Ast.expr = if args = [] then Loc.fail f.loc "cond needs at least one clause" else go args (* Every test here is an [if]'s condition, so a dyn operand is truthy-tested - (check.ml's check_truthy) exactly the way a bare [if]'s is — that much is - symmetric between [and] and [or]. What is not symmetric is the *answer*: - [and]'s "false" sentinel sits in the else arm, so check_if picks the real - branch's type first and boxes "false" to match it, which is what lets - [and] hand back the actual last dyn value, Clojure-style, rather than a - bare bool. [or]'s "true" sentinel sits in the then arm instead — the one - check_if types *first* — so it is what decides the whole expression's - type when nothing upstream already demanded one, and a later non-bool dyn - answer then hits the strict bool boundary instead of surviving as itself. - Reordering [or] to match [and] is a real fix and a separate decision; not - this item's to make. *) + (check.ml's check_truthy) exactly the way a bare [if]'s is, for both + [and] and [or]. The *answer* used to be asymmetric between them: [and]'s + "false" sentinel sits in the else arm, so check_if picks the real + branch's type first and boxes "false" to match it, which lets [and] hand + back the actual last dyn value, Clojure-style. [or] used to put its + "true" sentinel in the then arm instead — the one check_if types first — + so that sentinel decided the whole expression's type, and a later + non-bool dyn answer hit the strict bool boundary instead of surviving as + itself: (or nil "x") traps rather than answering "x", exactly the + canonical (or x default) idiom Clojure is reached for. + + [or] now binds its test to a temp and asks the temp itself, the way + Clojure's own or expands: [(or a b)] is [(let [t a] (if t t b))], not + [(if a true b)]. The temp is what lets the answer be [a] itself without + writing [a] a second time as the then arm — [(if a a b)] would evaluate + it twice, once for the test and again for the answer — and it is the + temp's own type check_if sees first, so [or] hands back the actual + truthy value the same way [and] hands back its own. *) and shortcircuit f (args : Form.t list) ~is_and : Ast.expr = let mk e = { Ast.e; loc = f.loc } in let rec go = function @@ -900,7 +907,11 @@ and shortcircuit f (args : Form.t list) ~is_and : Ast.expr = | [ last ] -> expr last | x :: rest -> if is_and then mk (Ast.If (expr x, go rest, Some (mk (Ast.Var "false")))) - else mk (Ast.If (expr x, mk (Ast.Var "true"), Some (go rest))) + else + let t = fresh_temp () in + let tvar = mk (Ast.Var t) in + let bind = { Ast.bname = t; bty = None; bval = expr x; bloc = f.loc } in + mk (Ast.Let ([ bind ], [ mk (Ast.If (tvar, tvar, Some (go rest))) ])) in go args diff --git a/test/programs/dyn-if-truthy.flan b/test/programs/dyn-if-truthy.flan index b7b9bc0..1d0d34e 100644 --- a/test/programs/dyn-if-truthy.flan +++ b/test/programs/dyn-if-truthy.flan @@ -10,13 +10,15 @@ ;;;; own syntax rather than by inspecting the desugaring. [and]'s *answer* ;;;; carries a non-bool dyn value through too, Clojure-style, because its ;;;; short-circuit sentinel is the else arm and the real value's type wins. -;;;; [or]'s sentinel is the then arm instead, so it is the one that decides -;;;; the whole expression's type when nothing else does, and [or]'s answer -;;;; stays a strict bool -- a pre-existing asymmetry (parse.ml, shortcircuit) -;;;; this item leaves alone; only [or]'s tests are exercised here, not a -;;;; non-bool value in its answer position. [not] and [while] are not [if] -;;;; in disguise, so check_truthy is called at their own sites by hand, and -;;;; get their own coverage too. +;;;; [or] used to put its sentinel in the then arm instead, so a non-bool +;;;; dyn answer hit the strict bool boundary and traps -- (or nil "x"), the +;;;; canonical Clojure (or x default) idiom, used to crash. [or] now binds +;;;; its test to a temp and answers the temp itself, Clojure's own +;;;; expansion, so its answer carries a non-bool dyn value through exactly +;;;; the way [and]'s does; both are exercised below, including the case +;;;; that used to be excluded here for being unsafe. [not] and [while] are +;;;; not [if] in disguise, so check_truthy is called at their own sites by +;;;; hand, and get their own coverage too. ;;;; ;;;; A typed if keeps needing a strict bool -- that refusal, and its message, ;;;; is a checker test in test_flan.ml, not a row here, since a program that @@ -66,14 +68,18 @@ ;; and/or: also if in disguise, so each test along the chain is ;; truthy-tested the same way if's own is -- 0 and "" do not stop and, ;; only nil and false do; 0 does stop or, the way any truthy value does. - ;; and's answer is the last truthy operand itself (:kw here), Clojure's - ;; and. or's answer stays a plain bool -- "true" below is or's own - ;; sentinel, not the operand that made it truthy; that asymmetry is - ;; parse.ml's, not this item's to close. + ;; Both hand back the actual operand that decided them, Clojure-style -- + ;; and's answer is the last truthy operand itself (:kw here); or's is the + ;; first truthy one (0, then "x") rather than a bare true. (println (and (box 1) (box "") (box :kw))) (println (and (box 1) (box false) (box "unreached"))) (println (or (box 0) (box false))) (println (or (box nil) (box false))) + ;; The case excluded before the fix: a non-bool value stopping or and + ;; being handed back as-is -- the canonical (or x default) idiom, which + ;; used to trap trying to unbox "x" as a strict bool. + (println (or (box nil) (box "x"))) + (println (or (box 5) (box "unreached"))) ;; not: truthiness, negated -- true only for nil and false. (println (not (box nil))) diff --git a/test/test_acceptance.ml b/test/test_acceptance.ml index 6af66f3..45216d4 100644 --- a/test/test_acceptance.ml +++ b/test/test_acceptance.ml @@ -1252,16 +1252,20 @@ let () = nil/false vs. everything else, Clojure's rule, on both backends; a typed scrutinee stays strictly bool, which is a checker test (test_flan.ml) and not a row here since a typed if given a non-bool - scrutinee would not compile. [when], [cond] and [and]'s and [or]'s - *tests* ride along for free because they desugar to [if] in - parse.ml; [not] and [while] get the same treatment by hand in - check.ml's check_truthy. [and]'s answer carries a non-bool dyn value - through, Clojure-style; [or]'s answer stays a plain bool, a - pre-existing asymmetry in its desugaring this item leaves alone. *) + scrutinee would not compile. [when], [cond], [and] and [or] ride + along for free because their tests desugar to [if] in parse.ml; + [not] and [while] get the same treatment by hand in check.ml's + check_truthy. [and] and [or] both hand back the actual operand that + decided them, Clojure-style -- [or] used to answer a bare bool + instead, its short-circuit sentinel sitting in the arm check_if + types first, and a non-bool dyn value reaching that position (the + canonical (or x default) idiom) used to trap rather than survive as + itself; [or] now binds its test to a temp and answers the temp, + fixing that. *) let dyn_if_truthy_out = "falsey\nfalsey\ntruthy\ntruthy\ntruthy\ntruthy\ntruthy\ntruthy\ntruthy\n\ truthy\ntruthy\ntruthy\nwhen 0 ran\nwhen empty-string ran\nb\nb\n:kw\n\ - false\ntrue\nfalse\ntrue\ntrue\nfalse\nfalse\nfalse\n3\n2\n1\n" + false\n0\nfalse\nx\n5\ntrue\ntrue\nfalse\nfalse\nfalse\n3\n2\n1\n" in outputs "dyn if truthiness" "programs/dyn-if-truthy.flan" dyn_if_truthy_out; outputs ~opt:"-O0" "dyn if truthiness, -O0" "programs/dyn-if-truthy.flan" diff --git a/test/test_flan.ml b/test/test_flan.ml index 0ac4409..51bedcb 100644 --- a/test/test_flan.ml +++ b/test/test_flan.ml @@ -354,8 +354,13 @@ let () = (match (parse1 "(and a b)").e with | If (_, _, Some { e = Var "false"; _ }) -> () | _ -> check "and short-circuits" false); + (* or binds its test to a temp and answers the temp itself -- Clojure's + own expansion, and what lets or hand back the actual truthy operand + rather than a bare true (M2 queue item 7's review pass). *) (match (parse1 "(or a b)").e with - | If (_, { e = Var "true"; _ }, Some _) -> () + | Let ([ _ ], + [ { e = If ({ e = Var t1; _ }, { e = Var t2; _ }, Some _); _ } ]) + when t1 = t2 -> () | _ -> check "or short-circuits" false); (* ── Forms that bind or alter control are never calls ──────────── *) @@ -875,6 +880,18 @@ let () = "(defn f [x] i32 (let [y x] (if (and y true) 0 1)))"; accepts "dyn or accepts a non-bool dyn operand" "(defn f [x] i32 (let [y x] (if (or y true) 0 1)))"; + (* A bare keyword condition used to be checked with want:Bool from the + start, landing on the keyword arm's enum-or-refuse case and refusing + by name -- ":kw is an enum member where an enum is expected ... but + bool is expected here", since there was no enum in play. Checked with + no expectation first, as every scrutinee now is, it resolves as the + dyn keyword instead, and a dyn keyword is unconditionally truthy: a + typed if with a bare keyword condition now compiles, and always takes + the then branch. The author's call, recorded at check_truthy: lispy + truthiness wins here, the lost diagnostic is not brought back, and + this pins the new answer down so it does not regress by accident. *) + accepts "a typed if with a bare keyword condition now compiles" + "(defn f [] i32 (if :kw 1 2))"; (* ── Unknown types ─────────────────────────────────────────────── *) (* A lowercase name is a type variable (plan.org, Types), so a mistyped