diff --git a/docs/REVIEW-production-readiness.md b/docs/REVIEW-production-readiness.md new file mode 100644 index 0000000..e71c50d --- /dev/null +++ b/docs/REVIEW-production-readiness.md @@ -0,0 +1,207 @@ +# Production-readiness review — 2026-09-17 + +Three review lanes (C runtime, compiler robustness, tooling/UX) plus a suite run on the +merged tree (`e9d0b99`, 232 checks, 0 failures, `@x86` 104 MATCH / 0 DIFFER). This file is +written to be implemented from: each item says what is wrong, where, and what the fix is. +Items marked **(known)** are already recorded in NEXT.md/FIX.org and are listed so the +ranking is complete, not because they are news. + +**The verdict in one paragraph.** The development loop is the most finished part of the +project and the shipping loop is the least. A solo developer in Emacs on this machine can +build a small raylib game today and the experience is genuinely good — the module system, +the Emacs client's failure handling, the `import-c` header diffing, the diagnostics (a real +span-and-notes system, ~365 located refusal sites, zero warning suppressions), and full +LLVM/x86 backend parity are production-grade. Nearly every gap sits at the boundary where a +second person, a second machine, or a shipped binary appears — plus three memory-safety +holes in the runtime that nothing currently mentions. + +--- + +## Tier 1 — correctness blockers + +These produce silent wrong values or memory corruption in programs that look fine. + +### 1.1 Every number→string conversion aliases one static buffer **(known, unenforced)** +`runtime/flan_rt.c:217-218` — `flan_i64_to_bytes` / `flan_f64_to_bytes` / `flan_u64_to_bytes` +all return `{scratch, len}` into one shared `static char scratch[64]`, and `emit.ml:2266-2270` +never copies the bytes out. Holding two results — `(vec-push! v (str i))` in a loop, a +`(str n)` stored in a struct — silently reads clobbered bytes. No crash, no diagnostic, so +sanitizers never see it. NEXT.md's "Sharp edges" records it; the prelude's `append-i64!` +family works around it; nothing *enforces* it. +**Fix to decide, then do:** either make the runtime conversions allocate from +`context/temp` (semantic change, kills the hazard everywhere), or have the checker type +these results as a distinct short-lived slice that may not be stored or outlive the next +conversion. The first is simpler and matches the Odin idiom already adopted for arenas. + +### 1.2 `cap * size` overflow at container growth +`runtime/flan_rt.c:1336` (Vec), `:1574` (Pool, `cap*size + cap*sslot`). `flan_vec_reserve` +(`:1374`) forwards arbitrary `n` and the `1<<40` clamp at `:1333` is bypassed when `want` +exceeds it (`cap = want; break;`). A wrap to a small positive allocates a tiny block while +`v->cap` stores the unwrapped value; the next push memcpys far past the block (`:1386`). +Same family, lower reach: `flan_over_budget` (`:785`), `flan_map_block_size` (`:2080`). +**Fix:** `__builtin_mul_overflow` (or `size > INT64_MAX/cap`) at every grow/reserve site; +overflow reports as `StorageExhausted` like any other allocation failure. Small, mechanical. + +### 1.3 `Map` has no removal +`runtime/flan_rt.c:1788`, `:2439` — deferred, no tombstones. A `Map` you cannot delete a +key from is a daily-use gap, not an edge case (entity tables, caches). +**Fix:** Robin Hood backward-shift deletion (no tombstones needed with the existing +cache-line-run layout), a `map-remove!` builtin through check/emit/x86, tests on both +backends. Medium-sized, self-contained. + +### 1.4 The dev allocation registry is read cross-thread with no synchronisation +Writer: game thread inside every alloc/free (`runtime/flan_dev.c:1130`, `:1180`). Reader: +the agent's listener thread (`vendor/agent/flan_agent.c:1081`, `:1146`) on a *running* +program — the `reg` verb has no stopped-gate, unlike the frame chain. No seqlock, no +atomics, and `flan_reg_compact` (`flan_dev.c:1099`) memsets and reinserts the whole table +mid-scan. A torn `(type, typelen)` pair is an out-of-bounds read in the listener. +Ranked Tier 1 because the dev loop is the project's priority. +**Fix:** either gate the `reg` verb on stopped (matching the frame chain — smallest change), +or give the registry the same per-slot seqlock the watch table already has +(`flan_dev.c:380-870` is the template in the same file). + +### 1.5 `(addr (.field x))` on an `Option` — confirm, then fix +FIX.org records `Tast.Addr (Tast.Pfield ...)` failing on both backends (working route: +`Prim (AddrOf, [Field ...])`). `check.ml:4715` builds `Tast.Addr` from user-writable +`(addr )` and `check.ml:2968` builds `Pfield` from `(.field x)`, so the combination +is expressible today; neither `emit.ml:1371-1375` nor `x86.ml:2070` has an Option arm. +**Fix:** first write the failing program to confirm reachability from source; then either +lower `Addr(Pfield)` through the AddrOf route in `check.ml`, or add the Option arm to both +backends. If unreachable from source, add the refusal-by-name that the house rule requires. + +--- + +## Tier 2 — the install and shipping story + +One coherent problem: the compiler runs only from its checkout, and its output runs only on +this machine. This is the single largest thing between "the author's language" and +"a language someone else can try." + +### 2.1 There is no install path +`dune-project` is two stanzas — no `(package ...)`, so `dune install` cannot work; the +documented way to run the compiler is `dune exec`. Worse, the README's suggested workaround +(copy the binary onto PATH) silently breaks the flagship feature: the merged `flan dev` +needs `flan.cmxa` + `flan.a` *beside the binary* (`lib/dev.ml:3198-3203`) and `ocamlfind` +on PATH at runtime (`lib/dev.ml:2898`). The failure message names the escape hatches +(`FLAN_LIBDIR`, `--two-process`) but neither README nor `emacs/MANUAL.md` mentions them. +**Fix:** a `(package)` stanza with install rules that place `flan.cmxa`/`flan.a` where the +binary's own lookup finds them; document `FLAN_LIBDIR`; make the README's install section +truthful about what `flan dev` needs. + +### 2.2 A built game runs only on machines configured like this one +No `-static` anywhere in `lib/build.ml`; `vendor/raylib/link` names `-l:libraylib.so.550` +by exact soname (a Fedora-ism — no unversioned symlink). The binary is otherwise genuinely +standalone (the C runtime is embedded in the compiler, `lib/dune:25-41` — right design). +**Fix:** a `flan build --static` (or bundled-raylib) option, and per-target link lines that +do not hardcode one distro's soname. + +### 2.3 The env-var surface is real and entirely undocumented +`FLAN_CLANG`, `FLAN_EMCC`, `FLAN_LD`, `FLAN_LLC`, `FLAN_WASM_SYSROOT`, `FLAN_WASM_BUILTINS`, +`FLAN_CACHE_DIR`, `FLAN_OCAMLFIND`, `FLAN_LIBDIR`, `FLAN_RAYLIB_WEB` — none in the README. +Also undocumented: the live loop needs `llc`/`ld` at an LLVM version matching clang +(`lib/build.ml:886-962`), and a mismatch breaks `C-c C-c` while `flan build` keeps working. +**Fix:** an env-var table in the README, and a sentence about the llc/clang version coupling. + +--- + +## Tier 3 — the standard library + +The containers, strings/UTF-8, sequences, random, and printing layers are decent. The gaps: + +- **No clock of any kind** — no `now`, no monotonic time, no `sleep`, not in prelude or + runtime. A raylib game gets time from raylib; a non-graphical tool cannot time anything. + Blocker for the "or tool" half of day-to-day use. A `time`/`sleep` builtin pair backed by + `clock_gettime` is small. +- **Math is five f32 functions** (`lib/prelude.ml:624-677`: sqrt, sin, cos, atan2, pow). + No tan/asin/acos/log/exp/fmod/abs/hypot, no f64 variants, no PI constant. +- **File IO is whole-file only** (`slurp`/`barf`). No streaming, stdin, directory listing, + metadata, delete/rename/mkdir. +- **No env vars, no process spawn.** `argv` and `exit` are the whole OS surface. +- **The prelude is an OCaml string literal** (`lib/prelude.ml:32`) — cannot be read as Flan, + extended, or replaced without rebuilding the compiler. Its own docstring promises a + `core:` package "at milestone 3" that does not exist. The `import-c` header-diffing is a + strong mitigation (users can bind libc and be told when they get it wrong), but the + promised `core:` migration is the structural fix. + +--- + +## Tier 4 — robustness and polish (small, high-value) + +- **`Sys_error` uncaught in the CLI** — `flan check nosuch.flan` → + `Fatal error: exception Sys_error(...)`. The daemon already has the arm + (`lib/dev.ml:2613-2621`); copy it into `bin/main.ml:8-32`. One line. Add a `Not_found` + backstop arm at the same time — nothing reaches it today, but the failure would be a + message-less `Fatal error: exception Not_found`. +- **Three tests leave `Fatal error: exception Flan.Loc.Error(_)` on stderr** + (test_acceptance, test_session, test_web; suite still passes). Some spawned compiler + process dies without going through the error printer — locate the spawn (grep the dune + log attribution), and either wrap it or assert on the formatted message instead. +- **`abort()` in the dev runtime** — `flan_dev.c:47-50`, `:103`: reload-name-table + exhaustion, intern OOM, and "global changed size" kill the game instead of signalling. + Against the grain of everything else in the runtime; route through `flan_error`. +- **Six trap paths bypass `flan_exit_hook`** and end a merged `flan dev` session + (`flan_restart_fail`, `flan_restart_args_fail`, `flan_restart_unarmed`, + `flan_transfer_fail`, `flan_null_alloc_fail`, `flan_free_all_fail` → `rt_die`). Bounds + and arithmetic already park via the break-loop hook; these should too. +- **Unchecked `malloc` in `flan_argv`** (`flan_rt.c:162`) — the one in the file. +- **`v->gen` stale-slice word is maintained and never consulted** (`flan_rt.c:1347`) — + either implement the check or delete the field before someone trusts it. +- **`flan_slurp_into` conflates elements and bytes and skips `flan_vec_check`** + (`flan_rt.c:2746`) — safe only because check.ml pins slurp to `(Vec u8)`; a latent trap. +- **`flan run` swallows build flags as program arguments** (`bin/main.ml:634-651`) — + `flan run game.flan --debug` hands `--debug` to the game. Filter or refuse by name like + every other subcommand. Also: no `-O` control anywhere (`Build.default` pins `-O2`; + `--debug` is the only route to `-O0`). +- **`main` signature errors print `:0:0`** (`check.ml:6033`, `:6038`) — + `env.locs` already holds the decl location; use the existing `find_opt` idiom. +- **`flan_shim_cstr` accepts embedded NULs** that `flan_path_cstr` refuses + (`lib/shim.ml:310` vs `flan_rt.c:2639`) — pick one policy. +- **No `-Wall -Wextra` on the runtime's C compile** (`lib/build.ml:827`, `:1069`). +- **Package visibility** — everything in a package is public except `main` + (`lib/load.ml:25-31`); `rl/get-color-raw` is the recorded symptom. +- **Emacs client**: 30s hard deadline with no retry on long builds (`flan-dev.el:155-170`); + `accept-process-output` loops can freeze Emacs up to 60s on a hung daemon (`:521`, + `:576-600`); no package headers, so not installable off MELPA or by path alone. +- **No CI** — README states it openly and records two silent-failure incidents. `@checks` + exists; a workflow that runs `dune build @checks` on push is the whole job. Note `@x86` + parity is only under `@checks`, so routine `dune test` does not protect parity. + +## Documentation corrections (cheap, decision-relevant) + +- **`docs/DISCUSS.md:762` is stale and argues the opposite of the truth**: it lists the + condition/restart family under "no plan" for the x86 backend; `x86.ml:1587-1615` lowers + all of it and the survey shows 104/104 parity. Anyone reading it for a production + decision concludes wrongly. +- README documents 4 of 11 subcommands — `import-c`/`generate-c`, the most valuable + undocumented feature, are missing (`README.md:105-119` vs `bin/main.ml`). +- `lib/prelude.ml:8-10` promises the nonexistent `core:` package. +- Root clutter: working artifacts (`MY-NOTES.org`, `plan.org`, committed binaries, + `sand.js`/`sand.wasm`, `old-ocaml/`) a newcomer must ignore. + +--- + +## Deliberately out of scope — do not pick these up from this report + +- **`drop` / recursive teardown** — parked on `worktree-agent-a18e9e62485eaedb5` with + `docs/handoffs/HANDOFF-drop.md`; the arena route replaced it (FIX.org item 4, merged). +- **JavaScript backend** — held (FIX.org item 6). +- **wasm32/browser** — explicitly deprioritised; the ILP32 `(size_t)` truncations and the + emscripten gaps are recorded here but not queued. +- **macOS/Windows portability** (`aligned_alloc`, `MSG_NOSIGNAL`, `__atomic_*` vs + `stdatomic.h`, `long ftell` 2 GiB cap) — real, recorded, not current-machine problems. +- **Known and accepted**: seqlock memcpy formal-UB (correctly fenced, retry-bounded); + one-`.so`-leak-per-reload (cells hold module text addresses by design); the arena route's + compile-time→runtime-trap trade (stated in FIX.org); string-literal write-through + (waiting on provenance, plan.org decision #3). + +## Suggested order + +1. **Quick wins, one sitting**: Sys_error + Not_found arms; `flan_argv` malloc check; + `main`-signature locations; `flan run` flag filtering; DISCUSS.md:762 correction; + README subcommand + env-var tables. +2. **Runtime correctness**: mul-overflow guards (1.2), registry sync (1.4), + scratch-buffer decision + fix (1.1), `map-remove!` (1.3), dev-runtime aborts → signals. +3. **Confirm and fix** `Addr(Pfield)` on Option (1.5); locate the stderr `Loc.Error` fatals. +4. **Install story** (2.1, 2.3), then **shipping** (2.2). +5. **Stdlib**: clock first, then math, then IO/env — each is independent. +6. **CI**: `dune build @checks` on push.