JosephFerano/flan
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

The review batch: honest headers, named temps, the seqlock's odd window

Browse Source 
# Conflicts:
#	FIX.org
This commit is contained in:
Joseph Ferano 2026-09-20 13:11:36 +07:00
commit 0e9707c9ef
7 changed files with 215 additions and 58 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

58
FIX.org
View File

@ -959,3 +959,61 @@ reserve, with a typed vec-new and map-new silent between them); and dyn
arithmetic answering nothing at all. test/test_dev.ml drives [(:op "memory")]
over the socket against programs/dev-dyn-global.flan, whose one line is two
gc crossings at two columns and no native allocation anywhere.
* Review-batch findings, 2026-09-20
Two things found in review that this lane could not fix in the files it owned.
Written down here so they are not lost with the branch.
** The daemon leaves its temp directory behind, forever
Every session makes =/tmp/flan-dev-<pid>/= — lib/dev.ml:3516 for the
two-process daemon and lib/dev.ml:4415 for the merged one — and nothing ever
removes it. It holds the built =program=, the host's =host.ll= or =host.s=,
the reload modules and =agent.sock=: about 7MB a session. The review counted
873 of them, 5GB, on the morning of 2026-09-19; this lane counted 68 and 457MB
on 2026-09-20. Whatever removed the difference, nothing in the tree did, and
the count climbs again with every =M-x flan=.
The fix is small and the merged daemon already has the one place for it. Its
session ends at lib/dev.ml:4395: [accept_loop] returns, the listening socket
closes, the editor socket is unlinked, and [Unix._exit 0] follows. A recursive
remove of [dir] belongs between the unlink and the flush — that one site
covers all three ways a session ends cleanly, because all three come back
through [accept_loop]:
- =close= from the editor (lib/dev.ml:3359, which returns [true] and ends the
loop);
- no editor connected for the grace period (lib/dev.ml:3472);
- the program finished and the parked process is let go.
The two-process daemon needs the same thing at its own session end.
Two deliberate non-goals, and they are the reason this is worth spelling out
rather than just doing:
- *Not on a crash.* The sibling branch at lib/dev.ml:4393 is [accept_loop]
raising, and the directory is the post-mortem — the binary and the exact IR
it was built from. Only the clean return cleans up.
- *Not other sessions' directories.* A sweep of =/tmp/flan-dev-*= would delete
the working directory of a daemon that is still running, and a stale pid is
not proof of anything. Each session removes its own and no more.
Not done here because lib/dev.ml belongs to another lane that has not merged.
** and's last operand gets a misdirected caret in a want-free position
[shortcircuit] in lib/parse.ml documents this at the site; the summary is that
=(println (and true true (vec-new i32)))= reports "expected (Vec i32), found
bool" with the caret on the second =true=. The last operand of an =and= is the
then arm, check_if types the then arm first, and the mismatch is therefore
reported against the else arm, which carries the *previous* operand's loc.
Every other operand position is right, because an operand anywhere but last is
a condition and check_truthy blames it at its own loc; =or= is right
everywhere, because there the chain and not the sentinel sits in the else arm.
Compiled on the tree at every operand position of both forms, want-free and
want-ful; want-ful is right everywhere too, because the want reaches each arm
instead of the arms being unified against each other.
Not fixed. Three candidate fixes were considered and rejected: giving the else
arm the last operand's loc makes the sentence read backwards ("expected (Vec
i32)" under a caret on the thing that is the (Vec i32)); answering a bool
sentinel again reverts the fix that made =(or nil "x")= answer ="x"=; and
inverting the condition to move the last operand into the else arm costs a
[not] per operand and worse locs than it buys. What would fix it is check_if
preferring the arm that is not a compiler temp when it decides which one to
blame — a change in check.ml, which this lane did not own.

7
NEXT.md
View File

@ -2395,8 +2395,11 @@ plan.org's single line on it (831) names a `for` the language does not have and
What is missing is the half only an editor can do — the leverage SBCL lacks. `eval` already compiles and runs an
expression inside the live program and the daemon already holds the struct layouts, so "ask the human, type-check
the answer, hand it over" is a short hop, and it is the one path the runtime today *refuses*: a restart with
parameters taken from the break loop traps, because `flan_break_resume` and `flan_restart_take` aim the channel at
a frame and have nothing to fill its buffer with. What it needs, end to end:
parameters taken from the break loop traps, because the pair a break loop resumes through — `flan_restart_frame`,
which hands back the frame the human picked, and `flan_restart_take`, which aims the transfer channel at it —
reaches the frame and has nothing to fill its buffer with. (`flan_break_resume` is what this paragraph named
before; there is no such function, and `flan_break_hook` is the unrelated thing that gets the break loop *into*
the agent in the first place.) What it needs, end to end:
- the frame already carries the arity and the signature as a string — `flan_restart_arity` and `flan_restart_sig`
beside `flan_restart_name`, the same walk, so `restarts` can say what each one takes;
- `:restarts` on the wire carries the signature per entry, so the minibuffer can show `use-value (i32)` rather

33
docs/SPIKE-DUPLICITY.md
View File

@ -425,23 +425,28 @@ and reload semantics are working." Under this reading the gating item changed. C
## 9. Print, hash, sort — one family already duplicated, two not yet
**Printing is duplicated three ways on one side, and it is not the dyn/typed split.**
`runtime/flan_dyn.c:308-313` says so itself:
**Printing was duplicated three ways on one side, and it is not the dyn/typed split. Two of the three are
now one.** This section found three escape tables; the runtime dedupe lane took the two that were on the
same side and made them one function with two framings, which is what this section asked for. What is left
is the split it argued was correct.
> A text inside a structure, quoted and escaped. The same table as `flan_rt.c`'s `flan_escape_bytes` and
> `flan_dev.c`'s `flan_dev_emit_str`...
The table lives once, as `flan_escape_char` (`runtime/flan_rt.c:388`), and both typed-side printers call
it: `flan_escape_bytes` (`:422`), which builds a capped slice for `println`, and `put_str`
(`runtime/flan_dev.c:302`), which streams into the inspector's fixed buffer so the REPL can parse the
printed form back. The framing is each caller's and is the part that is genuinely theirs; the switch is
shared, because two printers disagreeing about what a string looks like is two wire formats.
Three escape tables: `runtime/flan_rt.c:383` (`flan_escape_bytes`, with `:366-372` noting it is already "the
same escape table as `flan_dev_emit_str`"), `runtime/flan_dev.c:237` and again at `:551`, and
`runtime/flan_dyn.c:314` (`emit_escaped`). `SPIKE-DYNAMIC.md:329-332` calls its own "the third copy" and
leaves the instruction "If that table changes, change all three."
The dyn copy stays, as `emit_escaped` (`runtime/flan_dyn.c:472`), and says why at `:465-470`:
Two of those three are on the same side. `flan_rt.c`'s is `println`'s; `flan_dev.c`'s is the inspector's, so
the REPL can parse the printed form back. That pair predates dyn and is same-side duplication that dyn has
now made three-way. The *dyn* copy is the one with a defence, and `SPIKE-DYNAMIC.md:334-338` gives it: a
typed `Vec` prints as `<vec>` because the typed printer will not walk storage it does not own, while a dyn
vec's storage belongs to the collector and the printer is inside the runtime that owns it. That is correct
duplicity. Three tables is not.
> This copy is deliberate, and the argument for it is docs/SPIKE-DUPLICITY.md §9's: the dyn printer lives
> inside the runtime that owns the storage it walks, which is why it prints a dyn vec structurally where
> the typed printer answers `<vec>`. The whole printer is this side's; the table it shares with the other
> side is the part that must not drift. If that table changes, change this one.
That is the defence `SPIKE-DYNAMIC.md` gives and this section accepted: a typed `Vec` prints as `<vec>`
because the typed printer will not walk storage it does not own, while a dyn vec's storage belongs to the
collector and the printer is inside the runtime that owns it. One copy per *side*, which is the doctrine.
Two tables is the answer; three was not.
**Hashing is typed-only.** `flan_hash_fn`, `flan_hash_flat`, `flan_hash_str`, `flan_hash_combine`
(`runtime/flan_rt.c:1807, 1911, 1939, 1954`), reached through `Types.keyable` and the checker's `key_pair`

10
docs/SPIKE-DYNAMIC.md
View File

@ -326,10 +326,12 @@ running program**, not read off `lib/render.ml` — that file is the REPL's insp
| nil | — | `nil` |
The leading space before every element is what `lib/render.ml`'s slice loop emits and what a Flan program
prints today; a tidier answer would diverge from an acceptance test. The escape table is the third copy
of the one in `flan_rt.c`'s `flan_escape_bytes` and `flan_dev.c`'s `flan_dev_emit_str` — those two are
already kept identical because the REPL parses the printed form back, and this one joins them. If that
table changes, change all three.
prints today; a tidier answer would diverge from an acceptance test. The escape table is the dyn side's
own copy of the typed side's. The typed side used to have two and now has one — `flan_escape_char`
(`runtime/flan_rt.c:388`), which `flan_escape_bytes` calls for `println` and `flan_dev.c`'s `put_str`
calls for the inspector, each framing the result its own way — so there are two tables in the tree, not
three, and the split that is left is the dyn/typed one argued just below. If that table changes, change
this one too.
**A typed `Vec` prints as `<vec>` and a dyn vec does not.** That looks like a mismatch and is not. The
typed printer's refusal is about walking storage it does not own — `render.ml` says so, and directs you to

43
lib/parse.ml
View File

@ -14,15 +14,21 @@ let sym (f : Form.t) =
| Sym s -> s
| _ -> fail f "expected a name, found %s" (Form.to_string f)
(* Names for the temporaries a destructuring binding needs — the value is bound
once and every name in the pattern reads *that*, so a pattern over a call
calls it once. [~] is a delimiter in the reader, so no symbol anyone can
write contains one: these cannot collide with a source name and a source
name cannot shadow one. Reset per program so the names, and therefore the
slot numbering downstream, are the same every run. *)
(* Names for the temporaries this file mints — the value is bound once and
everything that needs it reads *that*, so a destructuring pattern over a
call calls it once and a short-circuit operand is evaluated once. [~] is a
delimiter in the reader, so no symbol anyone can write contains one: these
cannot collide with a source name and a source name cannot shadow one.
Reset per program so the names, and therefore the slot numbering
downstream, are the same every run.
The purpose is part of the name because these names are shown: the
inspector lists a frame's locals by name, and a short-circuit temp called
[destructure~3] is a plain lie about where it came from. One counter across
all purposes, so a name is still unique whatever minted it. *)
let temps = ref 0
let fresh_temp () = incr temps; Printf.sprintf "destructure~%d" !temps
let fresh_temp what = incr temps; Printf.sprintf "%s~%d" what !temps
(* Destructuring binds in [let] and nowhere else. Every other binding position —
a [defn] parameter, a [defstruct] field, an [fn] parameter, a [dotimes]
@ -673,7 +679,7 @@ and bindings f (items : Form.t list) : Ast.binding list =
the reference as well as the binding is what makes the two impossible to
separate by accident. *)
and temp (p : Form.t) (v : Ast.expr) : Ast.expr * Ast.binding =
let t = fresh_temp () in
let t = fresh_temp "destructure" in
({ Ast.e = Ast.Var t; loc = p.loc },
{ Ast.bname = t; bty = None; bval = v; bloc = p.loc })
@ -915,7 +921,24 @@ and cond f (args : Form.t list) : Ast.expr =
because the sentinel it answered was a bool literal that boxed to fit
whatever the real branch was; neither is now, and they are at least
symmetric about it. Making bool and dyn arms join as dyn is a check_if
question, noted in FIX.org under item 7 and not decided here. *)
question, noted in FIX.org under item 7 and not decided here.
One known wart, measured rather than guessed, and left alone deliberately.
In a want-free position — [(println (and true true (vec-new i32)))] — the
caret lands on the *second* [true] and not on the vec: the last operand is
the then arm, check_if types the then arm first, and the mismatch is
therefore reported against the else arm, which is the previous operand's
temp. An operand anywhere but last is a condition instead, so check_truthy
blames it at its own loc and the caret is right; [or] is right everywhere,
because there the chain and not the sentinel sits in the else arm. Giving
the else arm's [Var] node the *last* operand's loc moves the caret onto the
vec and makes the sentence read backwards — "expected (Vec i32), found
bool" under a caret on the thing that is the (Vec i32) — so it is not an
improvement; answering a bool literal again would revert the paragraph
above; and inverting the condition to move the last operand into the else
arm buys a [not] per operand and worse locs than it fixes. What would
actually fix it is check_if preferring the arm that is not a compiler temp
when it reports, which is check.ml's call. Written up in FIX.org. *)
and shortcircuit f (args : Form.t list) ~is_and : Ast.expr =
let mk e = { Ast.e; loc = f.loc } in
let rec go = function
@ -923,7 +946,7 @@ and shortcircuit f (args : Form.t list) ~is_and : Ast.expr =
| [ last ] -> expr last
| x :: rest ->
let ex = expr x in
let t = fresh_temp () in
let t = fresh_temp (if is_and then "and" else "or") in
let tvar = { Ast.e = Ast.Var t; loc = ex.Ast.loc } in
let bind = { Ast.bname = t; bty = None; bval = ex; bloc = ex.Ast.loc } in
let rest = go rest in

101
runtime/flan_dev.c
View File

@ -246,12 +246,6 @@ static void put_i64(sink out, int64_t x) {
* level rather than on the number. */
extern int flan_f64_format(double x, char *buf, size_t cap);
static void put_f64(sink out, double x) {
char buf[64];
flan_f64_format(x, buf, sizeof buf);
put(out, buf);
}
/* Quoted and escaped, in C, because doing it in the generated IR would be a
* loop per string and the language has no allocator to build the result in.
* A string whose content is not escaped does not round-trip and reads as a
@ -259,11 +253,55 @@ static void put_f64(sink out, double x) {
* string would become a second row of the table on the wire.
*
* The table is flan_rt.c's [flan_escape_char], which is also what println's
* [flan_escape_bytes] escapes through. One table, three framings of it. */
* [flan_escape_bytes] escapes through: one table, and the two callers differ
* only in how they frame what comes out of it. flan_dyn.c has a copy of the
* table rather than a call to it, deliberately and for a reason argued there
* and in docs/SPIKE-DUPLICITY.md §9. */
extern int flan_escape_char(unsigned char c, char *out);
/* The two [extern]s above are this file's hand copies of prototypes flan_rt.c
* owns, and nothing in the build compares the two: each translation unit is
* compiled on its own with no include path (see [Build.compile_c]), and the
* link that joins them matches names and not types. A parameter added on one
* side, a [size_t] cap that becomes an [int], a return that stops being a
* length — all of those build clean and then go wrong here, inside a render,
* with nothing pointing at the cause.
*
* So they are checked the way the tree checks its other cross-file agreement
* it cannot #include its way out of — by value, at run time, loudly. That is
* [flan_vec_layout] and test/dyn_ops.c's "layout" mode for the vec header;
* this is the same idea one function wide. Two calls with known answers, once
* per process, on the first value this file renders: if the callee is not the
* function these declarations describe, the answers do not come back right
* and the process stops here rather than emitting a wrong wire format.
*
* Not a constructor, because flan_dev.c is linked into every build and not
* only a dev one, and nothing should run in a release image that its program
* did not ask for. On the first render instead: the branch is one predictable
* test per value, and it runs in the dev paths only, which are the only paths
* that reach these two functions from here. */
static void check_shared(void) {
static int checked;
char buf[64];
char e[4];
if (checked) return;
checked = 1;
if (flan_f64_format(1.5, buf, sizeof buf) != 3 || strcmp(buf, "1.5") != 0)
die("flan_f64_format is not the function this file declares", "1.5");
if (flan_escape_char('\n', e) != 2 || e[0] != '\\' || e[1] != 'n')
die("flan_escape_char is not the function this file declares", "newline");
}
static void put_f64(sink out, double x) {
char buf[64];
check_shared();
flan_f64_format(x, buf, sizeof buf);
put(out, buf);
}
static void put_str(sink out, const uint8_t *bytes, int64_t len) {
size_t n = len < 0 ? 0 : (size_t)len;
check_shared();
put(out, "\"");
for (size_t i = 0; i < n; i++) {
char e[4];
@ -273,19 +311,20 @@ static void put_str(sink out, const uint8_t *bytes, int64_t len) {
put(out, "\"");
}
/* Closing a value: the ellipsis a full buffer earns, and the counter back to
* even. Shared by [flan_dev_result_end] and [flan_dev_watch_end], which is
* the same seqlock twice over two different buffers.
/* Closing a value, in the two halves the seqlock needs it in. Shared by
* [flan_dev_result_end] and [flan_dev_watch_end], which is the same seqlock
* twice over two different buffers.
*
* Room for the ellipsis is made rather than assumed: the buffer is full by
* definition when [full] is set.
* The first half is the ellipsis a full buffer earns. Room for it is made
* rather than assumed: the buffer is full by definition when [full] is set.
*
* The counter goes last, and back to even, so a reader that sees the new
* generation sees the whole value. [| 1] first for the same reason [begin]
* sets rather than increments: this must land on an even count whatever state
* an abandoned write left behind. */
static void end_value(char *buf, size_t *len, size_t cap, int full,
uint64_t *gen) {
* They are two functions and not one because everything a reader will look at
* has to be stored before the generation is, and a caller may have a length
* of its own to write back — the watch slot keeps its length in 32 bits and
* so cannot pass its own field here. Splitting lets that write-back land
* inside the odd window where it belongs, rather than after the release
* store, where it would be a tear the day a length actually changed. */
static void truncate_value(char *buf, size_t *len, size_t cap, int full) {
if (full) {
const char *ell = "...";
size_t k = strlen(ell);
@ -293,6 +332,14 @@ static void end_value(char *buf, size_t *len, size_t cap, int full,
memcpy(buf + *len, ell, k);
*len += k;
}
}
/* The second half: the counter last, and back to even, so a reader that sees
* the new generation sees the whole value. [| 1] first for the same reason
* [begin] sets rather than increments: this must land on an even count
* whatever state an abandoned write left behind. Nothing a reader reads may
* be written after this returns. */
static void close_value(uint64_t *gen) {
__atomic_store_n(gen, (*gen | 1) + 1, __ATOMIC_RELEASE);
}
@ -305,7 +352,8 @@ void flan_dev_emit_str(const uint8_t *bytes, int64_t len) {
}
void flan_dev_result_end(void) {
end_value(result, &result_len, RESULT_MAX, result_full, &generation);
truncate_value(result, &result_len, RESULT_MAX, result_full);
close_value(&generation);
}
/* Copy the current value out, with the counter that says which one it is.
@ -563,12 +611,21 @@ void flan_dev_watch_end(void) {
watch_slot *s = watch_cur;
watch_cur = NULL;
if (s == NULL) return;
/* [len] widened and narrowed around the shared close, which counts in
/* [len] widened and narrowed around the shared truncate, which counts in
* size_t because the result buffer does; a slot's own length is 32 bits
* and [WATCH_VAL] is 192, so neither conversion can lose anything. */
* and [WATCH_VAL] is 192, so neither conversion can lose anything.
*
* The narrowing write-back goes before [close_value] and not after it: a
* reader takes [len] and [val] together under the generation, so a length
* stored after the release store is a length the reader is entitled to
* have missed. It happens to write back the same bit pattern today — only
* a [full] slot is truncated, and a full slot's length is already
* [WATCH_VAL] — but that is a fact about the current cap arithmetic and
* not a rule anyone reading this would keep. */
size_t len = s->len;
end_value(s->val, &len, WATCH_VAL, s->full, &s->gen);
truncate_value(s->val, &len, WATCH_VAL, s->full);
s->len = (uint32_t)len;
close_value(&s->gen);
}
/* ── Watching one scalar, with no compiler change ───────────────────── */

21
runtime/flan_dyn.h
View File

@ -3,12 +3,21 @@
* This header is not compiled into a program. The build embeds the runtime's
* .c files as strings and hands each one to clang on its own, with no include
* path (see [Build.compile_c]), so runtime/flan_dyn.c declares everything it
* defines and this file declares it a second time. That second copy is not
* held honest by hand the way a repeated escape table would be: it is made
* mechanical, because test/dyn_ops.c includes this header and names every
* function below, so a signature that drifts from the implementation is a
* link error in `dune test` rather than a surprise at someone else's call
* site.
* defines and this file declares it a second time. What keeps that second
* copy honest is partial, and the exact shape of it is worth writing down
* rather than overclaiming: test/dyn_ops.c includes this header and calls
* most of what it declares, so a name that is spelled one way here and
* another way in flan_dyn.c fails to link in `dune test`.
*
* Two holes in that. A signature that drifts while the name stays is not
* caught at all — C links on names and not on types, so a changed parameter
* or return type compiles on both sides and goes wrong at the call site
* instead of failing the build. And five of the names below are never
* referenced by dyn_ops.c — [flan_dyn_map_get], [flan_dyn_map_set],
* [flan_dyn_map_contains], [flan_dyn_is_nil] and [flan_dyn_need_not_nil] —
* so not even the rename check reaches them. Calling those five from
* dyn_ops.c would close the second hole; nothing available from here closes
* the first.
*
* Who reads it: the compiler lane, which emits calls to these names, and the
* C tests. The whole of the boundary is here. What is behind it — the value