Skip to content

Commit 3e26b44

Browse files
committed
fix(codegen-deno): truncate Int/Int division instead of float divide (#478)
JS `/` is floating-point, so the type-erased Deno-ESM backend lowered integer `a / b` to a non-integer (255 / 16 -> 15.9375). Lower a provably `Int / Int` to `Math.trunc(a / b)` (truncate toward zero, matching the interpreter's OCaml `/` and wasm's i32.div_s); every other `/` stays plain float division, so Float/Float is untouched. Since codegen_deno is type-erased, a conservative classifier (expr_is_int / expr_is_int_array) reports Int only when provable: - int literals; Int-typed params; let/assign-tracked Int locals - integer-closed arithmetic (+ - * / %); JS bitwise results (int32) - calls to Int-returning fns/builtins - `for x in xs` loop variable when xs is a provable Array<Int> - `xs[i]` element read from a provable Array<Int> Unknown operands keep `/`, so float division is never silently truncated. int_vars/int_array_vars are reset per function (enter_fn_scope); the let-binding and for-loop scopes are restored after their body so a same-named outer Float is never affected; `x /= y` over ints truncates. Fixes stdlib/math.affine integer fns (pow, sum_naturals, binomial, lcm, div_floor) and `for x in xs { ... x / k ... }` over int arrays on the Deno-ESM backend. Verified locally (apt toolchain): dune build bin/main.exe clean; tests/codegen-deno/int_div regression passes (truncation, toward-zero on negatives, float preserved incl. Array<Float> loops, loop-mutated locals, Int-returning-call operands, for-loop var + indexed Array<Int>, /=); full codegen-deno + codegen-wasm suites and dune runtest green. (Full `dune build`/@fmt need js_of_ocaml+ocamlformat, absent in this sandbox but present in CI.) Refs #478.
1 parent 72ea59a commit 3e26b44

3 files changed

Lines changed: 341 additions & 22 deletions

File tree

lib/codegen_deno.ml

Lines changed: 220 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,25 @@ type codegen_ctx = {
6868
`await` it (e.g. `get(u).status` would otherwise read `.status` off
6969
a pending Promise). Populated in {!generate}. *)
7070
async_fns : (string, unit) Hashtbl.t;
71+
(* Top-level functions whose declared return type is [Int]. Used by
72+
{!expr_is_int} so a call like [gcd(a, b)] counts as an integer
73+
operand — needed to truncate e.g. [abs(a*b) / gcd(a,b)]. Populated
74+
in {!generate}. *)
75+
int_fns : (string, unit) Hashtbl.t;
76+
(* Names bound to a provably-[Int] value in the *current function*:
77+
[Int]-typed params plus [let]/assignments whose value is an integer
78+
expression. Mutated in source order as statements are emitted (the
79+
existing per-statement {!List.iter} makes this order-correct), and
80+
reset per function so names never leak across functions. Drives the
81+
[Int / Int -> Math.trunc(a / b)] lowering (issue #478); an unknown
82+
operand keeps plain [/], so float division is never affected. *)
83+
mutable int_vars : (string, unit) Hashtbl.t;
84+
(* Names provably bound to an [Array<Int>] (i.e. `[Int]`) in the
85+
*current function*: array-typed params. Lets a `for x in xs` over an
86+
int array seed [x] as [Int], and an `xs[i]` element read count as an
87+
integer operand, so divisions over them truncate (#478). Reset per
88+
function alongside {!int_vars}. *)
89+
int_array_vars : (string, unit) Hashtbl.t;
7190
(* True while emitting a synthesised `async` method body. The
7291
expression-position IIFE wrappers (block/try/match/let/return) must
7392
then be `async` and awaited, because they may contain an awaited
@@ -85,6 +104,9 @@ let create_ctx symbols = {
85104
assoc = Hashtbl.create 32;
86105
local_fns = Hashtbl.create 64;
87106
async_fns = Hashtbl.create 32;
107+
int_fns = Hashtbl.create 64;
108+
int_vars = Hashtbl.create 16;
109+
int_array_vars = Hashtbl.create 16;
88110
in_async = false;
89111
}
90112

@@ -632,6 +654,104 @@ let resolve_var ctx (name : string) : string =
632654
| Some s when s = name -> "this"
633655
| _ -> mangle name
634656

657+
(* ============================================================================
658+
Integer-operand inference for division (issue #478)
659+
660+
The Deno-ESM backend is type-erased, but JS `/` is floating-point, so a
661+
naive `OpDiv -> "/"` turns `255 / 16` into `15.9375`. We lower an
662+
`Int / Int` to `Math.trunc(a / b)` (truncate-toward-zero, matching the
663+
interpreter's OCaml `/` and wasm's `i32.div_s`) and leave every other
664+
`/` as plain float division. The classifier below is deliberately
665+
conservative: it reports [true] only when an operand is *provably* an
666+
integer, so a value of unknown type keeps `/` and float division is
667+
never silently truncated.
668+
============================================================================ *)
669+
670+
(* [Int] head of a (possibly ref/own/mut) type expression. Only the
671+
nominal [Int] constructor counts; type variables / applications do not. *)
672+
let rec type_head_is_int : type_expr -> bool = function
673+
| TyCon id -> id.name = "Int"
674+
| TyOwn t | TyRef t | TyMut t -> type_head_is_int t
675+
| _ -> false
676+
677+
(* Is [t] an [Array<Int>] (surface `[Int]`, which the parser desugars to
678+
`Array[Int]`)? Used to seed for-loop variables and recognise indexed
679+
element reads as integers (#478). *)
680+
let rec type_is_int_array : type_expr -> bool = function
681+
| TyApp (id, [ TyArg elem ]) -> id.name = "Array" && type_head_is_int elem
682+
| TyOwn t | TyRef t | TyMut t -> type_is_int_array t
683+
| _ -> false
684+
685+
(* Simple-variable pattern name, if [pat] binds exactly one name. *)
686+
let pat_var_name : pattern -> string option = function
687+
| PatVar id -> Some id.name
688+
| _ -> None
689+
690+
(* Builtins whose return type is unambiguously [Int]. Calls to these count
691+
as integer operands. (Excludes e.g. [parse_int], which is Option<Int>.) *)
692+
let int_returning_builtins =
693+
[ "len"; "string_find"; "string_char_code_at"; "char_to_int";
694+
"string_length" ]
695+
696+
(* Conservative "is this expression provably an [Int]?" Used only to decide
697+
whether a `/` should truncate; a [false] is always safe (keeps `/`). *)
698+
let rec expr_is_int ctx (e : expr) : bool =
699+
match e with
700+
| ExprLit (LitInt _) -> true
701+
| ExprLit _ -> false
702+
| ExprSpan (e, _) -> expr_is_int ctx e
703+
| ExprVar id -> Hashtbl.mem ctx.int_vars id.name
704+
(* Integer-closed arithmetic: result is [Int] iff both operands are.
705+
[OpDiv] is included because the emission below makes `Int / Int`
706+
truncate, so it too yields an [Int]. *)
707+
| ExprBinary (a, (OpAdd | OpSub | OpMul | OpDiv | OpMod), b) ->
708+
expr_is_int ctx a && expr_is_int ctx b
709+
(* JS bitwise operators coerce to a 32-bit integer regardless of input,
710+
so the result is always an integer. *)
711+
| ExprBinary (_, (OpBitAnd | OpBitOr | OpBitXor | OpShl | OpShr), _) -> true
712+
| ExprUnary (OpNeg, e) -> expr_is_int ctx e
713+
| ExprUnary (OpBitNot, _) -> true
714+
| ExprIf { ei_then; ei_else = Some e; _ } ->
715+
expr_is_int ctx ei_then && expr_is_int ctx e
716+
| ExprApp (ExprVar id, _) ->
717+
Hashtbl.mem ctx.int_fns id.name
718+
|| List.mem id.name int_returning_builtins
719+
(* An element read from a provably-[Array<Int>] value is an [Int]
720+
(covers `xs[i] / 2` where xs: [Int]) — issue #478 finding 2. *)
721+
| ExprIndex (arr, _) -> expr_is_int_array ctx arr
722+
| _ -> false
723+
724+
(* Conservative "is this expression provably an [Array<Int>]?" Recognises
725+
int-array params/locals and array literals whose every element is an
726+
integer. As with {!expr_is_int}, a [false] is always safe. *)
727+
and expr_is_int_array ctx (e : expr) : bool =
728+
match e with
729+
| ExprSpan (e, _) -> expr_is_int_array ctx e
730+
| ExprVar id -> Hashtbl.mem ctx.int_array_vars id.name
731+
| ExprArray elems -> elems <> [] && List.for_all (expr_is_int ctx) elems
732+
| _ -> false
733+
734+
(* Record/forget whether [name] currently holds an [Int], after a binding
735+
or assignment of [value] to it. Keeps {!ctx.int_vars} in sync as a
736+
function body is emitted top-to-bottom. *)
737+
let track_int_binding ctx (name : string) (value : expr) : unit =
738+
if expr_is_int ctx value then Hashtbl.replace ctx.int_vars name ()
739+
else Hashtbl.remove ctx.int_vars name
740+
741+
(* Reset [int_vars] / [int_array_vars] for a new function body and seed
742+
them from [Int]- and [Array<Int>]-typed params. Returns [ctx] with the
743+
fresh tables installed. *)
744+
let enter_fn_scope ctx (params : param list) : codegen_ctx =
745+
let tbl = Hashtbl.create 16 in
746+
let arr_tbl = Hashtbl.create 16 in
747+
List.iter
748+
(fun (p : param) ->
749+
if type_head_is_int p.p_ty then Hashtbl.replace tbl p.p_name.name ()
750+
else if type_is_int_array p.p_ty then
751+
Hashtbl.replace arr_tbl p.p_name.name ())
752+
params;
753+
{ ctx with int_vars = tbl; int_array_vars = arr_tbl }
754+
635755
(* ============================================================================
636756
Expression code generation
637757
@@ -690,6 +810,11 @@ let rec gen_expr ctx (expr : expr) : string =
690810
(* `++` is string- OR array-concat; dispatch on shape at runtime so
691811
`a ++ b` (string) and `acc ++ [x]` (array) are both correct. *)
692812
"__as_concat(" ^ gen_expr ctx e1 ^ ", " ^ gen_expr ctx e2 ^ ")"
813+
| ExprBinary (e1, OpDiv, e2) when expr_is_int ctx e1 && expr_is_int ctx e2 ->
814+
(* Issue #478: integer `/` truncates toward zero; JS `/` is float.
815+
Both operands are provably [Int] here, so emit a truncating
816+
divide that matches the interpreter and wasm backends. *)
817+
"Math.trunc((" ^ gen_expr ctx e1 ^ ") / (" ^ gen_expr ctx e2 ^ "))"
693818
| ExprBinary (e1, op, e2) ->
694819
let op_str = match op with
695820
| OpAdd -> "+" | OpSub -> "-" | OpMul -> "*" | OpDiv -> "/"
@@ -716,13 +841,28 @@ let rec gen_expr ctx (expr : expr) : string =
716841
let pat_str = gen_pattern ctx el_pat in
717842
let val_str = gen_expr ctx el_value in
718843
let kw = if el_mut then "let" else "const" in
719-
(match el_body with
720-
| Some body ->
721-
iife ctx (kw ^ " " ^ pat_str ^ " = " ^ val_str ^ "; return "
722-
^ gen_expr ctx body ^ ";")
723-
| None ->
724-
iife ctx (kw ^ " " ^ pat_str ^ " = " ^ val_str
725-
^ "; return Unit;"))
844+
(* `let x = v in body` is lexically scoped, so track [x]'s int-ness
845+
only for [body] and restore afterward — a leaked binding could
846+
wrongly truncate a same-named outer Float (#478). *)
847+
let body_str =
848+
match el_body with
849+
| Some body ->
850+
let restore =
851+
match pat_var_name el_pat with
852+
| Some n ->
853+
let had = Hashtbl.mem ctx.int_vars n in
854+
track_int_binding ctx n el_value;
855+
fun () ->
856+
if had then Hashtbl.replace ctx.int_vars n ()
857+
else Hashtbl.remove ctx.int_vars n
858+
| None -> fun () -> ()
859+
in
860+
let b = gen_expr ctx body in
861+
restore ();
862+
kw ^ " " ^ pat_str ^ " = " ^ val_str ^ "; return " ^ b ^ ";"
863+
| None -> kw ^ " " ^ pat_str ^ " = " ^ val_str ^ "; return Unit;"
864+
in
865+
iife ctx body_str
726866
| ExprTuple exprs | ExprArray exprs ->
727867
"[" ^ String.concat ", " (List.map (gen_expr ctx) exprs) ^ "]"
728868
| ExprIndex (arr, idx) ->
@@ -993,26 +1133,73 @@ and gen_stmt ctx (stmt : stmt) : string =
9931133
match stmt with
9941134
| StmtLet { sl_pat; sl_value; sl_mut; sl_quantity = _; sl_ty = _ } ->
9951135
let kw = if sl_mut then "let" else "const" in
996-
kw ^ " " ^ gen_pattern ctx sl_pat ^ " = " ^ gen_expr ctx sl_value ^ ";"
1136+
let js = kw ^ " " ^ gen_pattern ctx sl_pat ^ " = "
1137+
^ gen_expr ctx sl_value ^ ";" in
1138+
(* Track an [Int]-bound name for the rest of this body (#478). A
1139+
block statement's scope is the enclosing function, which
1140+
{!enter_fn_scope} already bounds, so no restore is needed here. *)
1141+
(match pat_var_name sl_pat with
1142+
| Some n -> track_int_binding ctx n sl_value
1143+
| None -> ());
1144+
js
9971145
| StmtExpr e -> gen_stmt_expr ctx e
9981146
| StmtAssign (lhs, op, rhs) ->
999-
let op_str = match op with
1000-
| AssignEq -> "=" | AssignAdd -> "+=" | AssignSub -> "-="
1001-
| AssignMul -> "*=" | AssignDiv -> "/="
1147+
(* #478: `x /= y` over integers must truncate, like `x = x / y`. *)
1148+
let div_int =
1149+
op = AssignDiv && expr_is_int ctx lhs && expr_is_int ctx rhs in
1150+
let js =
1151+
if div_int then
1152+
let t = gen_expr ctx lhs in
1153+
t ^ " = Math.trunc(" ^ t ^ " / (" ^ gen_expr ctx rhs ^ "));"
1154+
else
1155+
let op_str = match op with
1156+
| AssignEq -> "=" | AssignAdd -> "+=" | AssignSub -> "-="
1157+
| AssignMul -> "*=" | AssignDiv -> "/="
1158+
in
1159+
gen_expr ctx lhs ^ " " ^ op_str ^ " " ^ gen_expr ctx rhs ^ ";"
10021160
in
1003-
gen_expr ctx lhs ^ " " ^ op_str ^ " " ^ gen_expr ctx rhs ^ ";"
1161+
(* Keep int-tracking current across reassignment of a simple var. *)
1162+
(match lhs, op with
1163+
| ExprVar id, AssignEq -> track_int_binding ctx id.name rhs
1164+
| ExprVar id, AssignDiv ->
1165+
if div_int then Hashtbl.replace ctx.int_vars id.name ()
1166+
else Hashtbl.remove ctx.int_vars id.name
1167+
| ExprVar id, (AssignAdd | AssignSub | AssignMul) ->
1168+
if not (expr_is_int ctx rhs) then
1169+
Hashtbl.remove ctx.int_vars id.name
1170+
| _ -> ());
1171+
js
10041172
| StmtWhile (cond, body) ->
10051173
"while (" ^ gen_expr ctx cond ^ ") { "
10061174
^ String.concat " " (List.map (gen_stmt ctx) body.blk_stmts)
10071175
^ (match body.blk_expr with
10081176
| Some e -> " " ^ gen_stmt_expr ctx e | None -> "")
10091177
^ " }"
10101178
| StmtFor (pat, iter, body) ->
1011-
"for (const " ^ gen_pattern ctx pat ^ " of " ^ gen_expr ctx iter ^ ") { "
1012-
^ String.concat " " (List.map (gen_stmt ctx) body.blk_stmts)
1013-
^ (match body.blk_expr with
1014-
| Some e -> " " ^ gen_stmt_expr ctx e | None -> "")
1015-
^ " }"
1179+
(* The iterable is evaluated in the outer scope, so emit it first. *)
1180+
let iter_str = gen_expr ctx iter in
1181+
let pat_str = gen_pattern ctx pat in
1182+
(* #478: a `for x in xs` over a provably-[Array<Int>] binds [x] to an
1183+
[Int] each iteration, so `x / 2` in the body must truncate. The
1184+
loop variable is loop-scoped, so save/restore its [int_vars] entry
1185+
to avoid leaking onto a same-named outer binding after the loop. *)
1186+
let restore =
1187+
match pat_var_name pat with
1188+
| Some n when expr_is_int_array ctx iter ->
1189+
let had = Hashtbl.mem ctx.int_vars n in
1190+
Hashtbl.replace ctx.int_vars n ();
1191+
fun () ->
1192+
if had then Hashtbl.replace ctx.int_vars n ()
1193+
else Hashtbl.remove ctx.int_vars n
1194+
| _ -> fun () -> ()
1195+
in
1196+
let body_js =
1197+
String.concat " " (List.map (gen_stmt ctx) body.blk_stmts)
1198+
^ (match body.blk_expr with
1199+
| Some e -> " " ^ gen_stmt_expr ctx e | None -> "")
1200+
in
1201+
restore ();
1202+
"for (const " ^ pat_str ^ " of " ^ iter_str ^ ") { " ^ body_js ^ " }"
10161203

10171204
(* ============================================================================
10181205
Top-level declarations + class emission
@@ -1046,7 +1233,7 @@ let gen_function ctx (fd : fn_decl) : unit =
10461233
else async_kw ^ "function" in
10471234
emit_line ctx
10481235
(Printf.sprintf "%s %s(%s) {" kw name (String.concat ", " params));
1049-
let body_ctx = increase_indent ctx in
1236+
let body_ctx = enter_fn_scope (increase_indent ctx) fd.fd_params in
10501237
let body_ctx =
10511238
if is_async then { body_ctx with in_async = true } else body_ctx in
10521239
gen_body body_ctx fd.fd_body;
@@ -1115,7 +1302,7 @@ let gen_method ctx ~(recv_name : string) ~(js_name : string)
11151302
emit_line ctx_m
11161303
(Printf.sprintf "async %s(%s) {" (mangle js_name)
11171304
(String.concat ", " params));
1118-
gen_body (increase_indent ctx_m) fd.fd_body;
1305+
gen_body (enter_fn_scope (increase_indent ctx_m) fd.fd_params) fd.fd_body;
11191306
emit_line ctx_m "}";
11201307
emit ctx_m ""
11211308

@@ -1126,7 +1313,7 @@ let gen_constructor ctx (fd : fn_decl) : unit =
11261313
List.map (fun (p : param) -> mangle p.p_name.name) fd.fd_params in
11271314
emit_line ctx
11281315
(Printf.sprintf "constructor(%s) {" (String.concat ", " params));
1129-
let body_ctx = increase_indent ctx in
1316+
let body_ctx = enter_fn_scope (increase_indent ctx) fd.fd_params in
11301317
let rec assign_record e =
11311318
match e with
11321319
| ExprSpan (inner, _) -> assign_record inner
@@ -1223,12 +1410,23 @@ let generate (program : program) (symbols : Symbol.t) : string =
12231410
| TopFn fd ->
12241411
Hashtbl.replace ctx.local_fns fd.fd_name.name ();
12251412
if fd_is_async fd then
1226-
Hashtbl.replace ctx.async_fns fd.fd_name.name ()
1413+
Hashtbl.replace ctx.async_fns fd.fd_name.name ();
1414+
(* Record [Int]-returning fns so calls to them count as integer
1415+
operands for the truncating-division lowering (#478). *)
1416+
(match fd.fd_ret_ty with
1417+
| Some t when type_head_is_int t ->
1418+
Hashtbl.replace ctx.int_fns fd.fd_name.name ()
1419+
| _ -> ())
12271420
| TopConst { tc_name; _ } ->
12281421
Hashtbl.replace ctx.local_fns tc_name.name ()
12291422
| TopImpl ib ->
12301423
List.iter (function
1231-
| ImplFn fd -> Hashtbl.replace ctx.local_fns fd.fd_name.name ()
1424+
| ImplFn fd ->
1425+
Hashtbl.replace ctx.local_fns fd.fd_name.name ();
1426+
(match fd.fd_ret_ty with
1427+
| Some t when type_head_is_int t ->
1428+
Hashtbl.replace ctx.int_fns fd.fd_name.name ()
1429+
| _ -> ())
12321430
| ImplType _ -> ()) ib.ib_items
12331431
| _ -> ()) program.prog_decls;
12341432

0 commit comments

Comments
 (0)