From dd239d6cd5806453a7ab9c5a2299feb29fd76fa6 Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sun, 20 Sep 2026 13:00:55 +0700 Subject: [PATCH] Two review findings this lane could not fix, written down --- FIX.org | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/FIX.org b/FIX.org index 6804dfd..2769a5f 100644 --- a/FIX.org +++ b/FIX.org @@ -858,3 +858,61 @@ skipping its stores on a re-run is safe against all of that because the pushes take the global's slot address, never its value, and they sit before the startup call on both backends — nothing in the bracket depends on an initialiser having run. + +* 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-/= — 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.