diff --git a/HANDOFF-f2.md b/HANDOFF-f2.md new file mode 100644 index 0000000..976c589 --- /dev/null +++ b/HANDOFF-f2.md @@ -0,0 +1,142 @@ +# Handoff: `pause` marking from Emacs (DISCUSS.md §9) + +The daemon half is built and works end to end over the wire. The Emacs half is **not** built. +Nothing half-written was left behind: `dune build` is clean and there is no new test. + +## The design, as it stands after reading the code + +`C-u` before an eval marks a form so the program stops when that form runs. The mark is **not** +spliced into the source text — that would move every line and column after the insertion, and the +error overlays, `layout`, the break loop's frame locations and DWARF all read those. Instead the +editor sends a **position** beside the code: + +``` +(:op "eval" :code "(defn step [] i64 ...)" :file "/x/y.flan" :pause (LINE COL)) +``` + +The daemon parses and `Load`s as usual, then walks the resulting `Ast.decl list` and puts a +`(pause)` call at whatever *starts* at that position. `(pause)` is an ordinary prelude function +(`error` under a `restart-case` with a `continue` clause), so an instrumented body is just a body +that calls one more function, and the break loop it lands in is the one an unhandled condition +already builds. Nothing in the compiler changes. + +It **sticks** with no extra machinery: the marked declaration is what goes into `Session.t.decls`, +so it stays marked until an evaluation replaces it — an ordinary `C-c C-c` over the same form with +no `:pause`, or `C-c C-k` over the buffer. That is §9's settled behaviour and it costs one +statement that was already there. + +### What §9 left out or got slightly wrong + +- **§9 says "send the top-level form with that span replaced by `(do (pause) )`".** That + wrapping is right for a sub-expression but *impossible* for the first of its three targets: a + whole top-level `defn` is a declaration, and `(do (pause) (defn ...))` is not an expression. + Marking a whole `defn` therefore means *stopping on entry*, and the call goes at the front of + `fbody`. `Ast.mark_pause` does both, chosen by what the position lands on. +- **§9 does not say the mark can be refused.** It has to be: a position that matches nothing must + be an error, because installing an unmarked body and answering `ok` reports a breakpoint that is + not there — the silent-success failure the session refuses everywhere else. +- **Desugaring makes locations non-unique.** `parse.ml` gives several nested nodes the same + location (`when` becomes an `If` whose branch is a `Do` at the `when`'s own position — lines 141, + 164, 651, 654). The walk is pre-order and stops at the first hit, so the outermost node at that + position wins, which is the one the editor pointed at. +- **The third target ("the form point is inside") needs no new daemon work** — it is the same + position field, computed differently in Emacs. + +## What was built, file by file + +All four are **working** (built, and exercised against a real daemon and a real running program by +hand — see "How it was checked"). + +- **`lib/ast.ml`** — new section at the end: + - `map_children : (expr -> expr) -> expr -> expr`, an exhaustive one-level rebuild. Exhaustive on + purpose: a missing constructor is a form you silently cannot stop inside. + - `pause_call : Loc.t -> expr` — `(pause)` at a given location. + - `mark_pause : line:int -> col:int -> decl list -> decl list option` — pre-order, first hit wins, + `None` when nothing is at that position. The synthesized `Do`/`Call` take the target's own + location, never `Loc.unknown`, because DWARF and the break loop's frame location read it. +- **`lib/wire.ml`** — `pos_field form key`, reading `(LINE COL)` as a pair of ints; `None` for + anything else, the same narrowness as `int_field`. +- **`lib/session.ml`** — `eval` takes `?pause:(int * int)`. It is applied **after** + `Load.qualify_decl`, so a package that defines a `pause` of its own cannot capture the + synthesized call, and a position that matches nothing is a `Loc.fail` naming the position. +- **`lib/dev.ml`** — `eval` takes `~pause`, `handle` reads `:pause` off the request, and a + successful install echoes `:pause "LINE:COL"` back so an editor marks the buffer only for a mark + the session actually applied. With no `:pause` in the request, every byte of the old behaviour is + unchanged. + +## How it was checked + +A daemon over `test/programs/dev-loop.flan`, driven by a raw socket client: + +- eval of `step` with `:pause (1 1)` → `(:status "ok" … :pause "1:1")`, and a later `describe` + came back `:stopped t :condition "Pause"` — the program stopped, on the prelude's own condition. +- `:pause (1 999)` → `(:status "error" :message "nothing to pause at line 1, column 999 of the form + sent")`. +- a plain re-eval of the same form → accepted. + +`dune build` is clean. **`dune test` was not run** (budget). The changes are additive: the new +session argument is optional and the new reply field only appears when `:pause` was sent, so no +existing path changes shape — but the suite should be run first thing next session anyway. Note +`test_dev.ml`'s first block is separately known-flaky (a socket bind race, ~1 in 4). + +## What remains, in order + +1. **`lib/dev.ml`, `eval_expr`** — accept `:pause` for `C-u C-x C-e` (§9's "last expression"). + Two parts. (a) `lib/session.ml`'s `eval_expr` should take `?(pause = false)` and wrap + `Parse.expr form` in `Do [Ast.pause_call loc; e]` before `Check.expression`. (b) `eval_expr`'s + `wait` loop in `dev.ml` returns `error "the program did not reach a frame boundary…"` after 5s, + which is exactly what a thunk that stopped in the break loop will do — so it would report the + working feature as a failure. Make `wait` three-way (`` `Value | `Stopped | `Timeout ``) and + check `state t = Stopped` **only when a pause was requested**: `test_dev.ml:519–560` already + asserts the current timeout shape for the no-pause case (`"an expression that stopped inside a + break answered anyway"`), and that must stay byte-identical. + Use `:pause t` here, not `(LINE COL)`: `flan-eval-last-sexp` sends a raw `buffer-substring` + with no line padding (unlike `flan-dev--text`), so buffer coordinates do not survive that path. +2. **`emacs/flan-dev.el`, `flan-dev--eval`** — take an optional pause position and put + `:pause (LINE COL)` on the request. The column is a **1-based byte offset**, per the comment + above `flan-dev--position`: `(1+ (- (position-bytes pos) (position-bytes (line-beginning-position))))`, + *not* `current-column`. The line is the buffer's own line, which already works because + `flan-dev--text` pads with leading newlines. +3. **`emacs/flan-dev.el`, `flan-eval-defun`** — `(interactive "P")`. `C-u` marks the innermost form + point is inside (`backward-up-list`, falling back to the defun's start when point is not nested); + `C-u C-u` marks the top-level form itself, i.e. stop on entry. That plus item 1 covers §9's three + targets with no new keybinding — `emacs/flan-mode.el` needs no change. +4. **`emacs/flan-dev.el`, the visual indication** — a `flan-dev-pause-face` overlay over the marked + form's bounds, drawn **only** when the reply carries `:pause`, tagged with a `flan-dev-pause` + property. Copy the shape of the error overlays (`flan-dev--show-error`) but *not* their lifetime: + a pause mark is an annotation on the program, not feedback about one command, so it must survive + `pre-command-hook`. Remove overlays intersecting the sent region on every accepted plain eval, + and over the whole buffer in `flan-eval-buffer` — that is the visible half of "cleared by an + ordinary `C-c C-c`". +5. **`test/programs/dev-pause.flan`** — new, shaped like `dev-break.flan`'s tail so the marked + function keeps being called: `(dotimes [i 4000] (agent/wait 5) (set ticks (step)))`. + `dev-loop.flan` calls `step` only four times, which is too tight. +6. **`test/test_dev.ml`** — a block in the style of the break-loop block (its own daemon, its own + program, its own output buffer). Assert both halves: mark `step`'s `(+ ticks 1)` sub-expression + → await `:stopped t` with a condition containing `Pause` → `break` lists `continue` → then + **re-eval `step` plainly, take `continue`, and confirm it runs on without stopping again**. The + second assertion is the one that tests the settled "it sticks until evaluated plainly" decision + and the one most likely to be skipped. +7. **`emacs/test-flan-dev.el`** — the elisp side, once items 2–4 exist. + +## Decisions made that were not already settled + +- **The mark is not a field on `Ast`.** It is an ordinary `(pause)` call spliced into the tree. A + `paused : bool` on `Ast.expr` would have to be threaded through `Check`, `Tast` and `Emit` for a + feature the prelude already implements as a function. +- **No parallel `paused` list on `Session.t`.** The spliced declaration in `t.decls` *is* the state + that makes the mark stick; a second `(name * position) list` would be a second source of truth + that drifts the first time some path replaces `decls` without touching it. Clearing then falls out + for free — any plain eval replaces the stored declaration with an unmarked one. +- **Marking a whole `defn` means stopping on entry**, since it cannot be wrapped (above). +- **A position matching nothing is refused**, rather than installed unmarked (above). +- **The splice happens after qualification**, so a package's own `pause` cannot capture it. +- **The reply echoes `:pause "LINE:COL"`** so the editor draws its overlay off the daemon's + confirmation and can never claim a mark that was refused. + +## Tried and abandoned + +- Nothing failed outright. Worth recording: the worktree this was done in was created **485 commits + behind** `dev-loop` (at `2c232dd`, before `lib/dev.ml` existed at all) and had to be + `git reset --hard` to the branch tip before any of the files named in the task existed. Check + `git log` against `dev-loop` before starting in a fresh worktree. diff --git a/lib/ast.ml b/lib/ast.ml index 9790e56..e90b2d3 100644 --- a/lib/ast.ml +++ b/lib/ast.ml @@ -175,3 +175,107 @@ let declared_name (d : decl) = | Defvar (n, _, _) | Defconst (n, _, _) -> Some n | Declare (fn, _) | DeclareC (fn, _) | Defn fn -> Some fn.name | Package _ | Import _ -> None + +(* ── Instrumenting a form with (pause) ─────────────────────────────── *) + +(* [C-u C-c C-c] marks a form so the program stops when it runs — DISCUSS.md + §9. The mark travels beside the source as a position and is applied *here*, + to the AST, rather than being spliced into the text the editor sends: text + would shift every line and column after the insertion, and the error + overlays, the layout, the break loop's frame locations and DWARF all read + those. Applied after parsing, every location is already attached and none of + them moves. + + Nothing in the compiler knows about this. [(pause)] is an ordinary prelude + function — [error] under a [restart-case] — so an instrumented body is a + body that calls one more function, and the break loop it lands in is the one + an unhandled condition already builds. *) + +(* Rebuild [e] with [f] applied to each expression written directly inside it. + Exhaustive on purpose: a constructor left out would be a form the mark + silently cannot be set inside, which is the kind of hole nobody finds + except by trying it on the one function they wanted to stop in. *) +let map_children f (e : expr) : expr = + let ex = f in + let bind (b : binding) = { b with bval = ex b.bval } in + let arm (a : arm) = { a with body = List.map ex a.body } in + let hcl (h : hclause) = { h with hbody = List.map ex h.hbody } in + let rcl (r : rclause) = { r with rbody = List.map ex r.rbody } in + let place = function + | Pvar n -> Pvar n + | Pfield (x, n) -> Pfield (ex x, n) + | Pindex (x, is) -> Pindex (ex x, List.map ex is) + | Pderef x -> Pderef (ex x) + in + let kind = + match e.e with + | Int _ | Float _ | Byte _ | Str _ | Kw _ | Quote _ | Var _ | ArrayOf _ + | Break _ | Continue _ -> e.e + | Do es -> Do (List.map ex es) + | Let (bs, es) -> Let (List.map bind bs, List.map ex es) + | If (c, a, b) -> If (ex c, ex a, Option.map ex b) + | While (l, c, es) -> While (l, ex c, List.map ex es) + | Loop (bs, es) -> Loop (List.map (fun (n, v) -> (n, ex v)) bs, List.map ex es) + | Recur es -> Recur (List.map ex es) + | Return x -> Return (Option.map ex x) + | Set (p, v) -> Set (place p, ex v) + | Field (x, n) -> Field (ex x, n) + | Call (fn, args) -> Call (ex fn, List.map ex args) + | Match (s, arms) -> Match (ex s, List.map arm arms) + | Struct (n, fs) -> Struct (n, List.map (fun (n, v) -> (n, ex v)) fs) + | Arr es -> Arr (List.map ex es) + | Fn (ps, es) -> Fn (ps, List.map ex es) + | Dotimes (l, n, c, es) -> Dotimes (l, n, ex c, List.map ex es) + | Defer es -> Defer (List.map ex es) + | Unwrap (u, x) -> Unwrap (u, ex x) + | HandlerBind (cs, es) -> HandlerBind (List.map hcl cs, List.map ex es) + | Signal (k, x) -> Signal (k, ex x) + | RestartCase (b, cs) -> RestartCase (ex b, List.map rcl cs) + | InvokeRestart (n, args) -> InvokeRestart (n, List.map ex args) + in + { e with e = kind } + +let pause_call loc = { e = Call ({ e = Var "pause"; loc }, []); loc } + +(* [mark_pause ~line ~col ds] is [ds] with a [(pause)] put in front of whatever + starts at that position, or [None] when nothing does. + + [None] rather than "leave it alone": installing an unmarked body and + answering "ok" would report a breakpoint that is not there, which is the + failure the session refuses everywhere else. + + Pre-order, and it stops at the first hit. Desugaring gives several nested + nodes the same location — [(when c a)] becomes an [If] whose else-less + branch is a [Do] at the [when]'s own position — so the outermost of those is + the one the editor pointed at. + + A whole top-level [defn] is the third target from §9 and cannot be wrapped: + [(do (pause) (defn ...))] is not an expression. Marking one means stopping + on entry, so the call goes at the front of its body. *) +let mark_pause ~line ~col (ds : decl list) : decl list option = + let at (l : Loc.t) = l.Loc.line = line && l.Loc.col = col in + let hit = ref false in + let rec walk (e : expr) = + if !hit then e + else if at e.loc then begin + hit := true; + (* The [Do] takes the target's own location, and the target keeps its + own: a wrapper at [Loc.unknown] would put the frame the break loop + reports, and the line DWARF names, nowhere. *) + { e with e = Do [ pause_call e.loc; e ] } + end + else map_children walk e + in + let body es = List.map walk es in + let decl (d : decl) = + match d.d with + | Defn f when (not !hit) && at d.dloc -> + hit := true; + { d with d = Defn { f with fbody = pause_call d.dloc :: f.fbody } } + | Defn f -> { d with d = Defn { f with fbody = body f.fbody } } + | Defvar (n, t, Init e) -> { d with d = Defvar (n, t, Init (walk e)) } + | Defconst (n, t, e) -> { d with d = Defconst (n, t, walk e) } + | _ -> d + in + let ds = List.map decl ds in + if !hit then Some ds else None diff --git a/lib/dev.ml b/lib/dev.ml index b7c2a88..f4aa5ac 100644 --- a/lib/dev.ml +++ b/lib/dev.ml @@ -418,10 +418,13 @@ let error ?loc msg = ^ (match loc with None -> "" | Some l -> " :loc " ^ Wire.quote l) ^ ")" -let eval t ~code ~origin = +(* [pause], when given, is the position of the form to stop at — §9. It rides + beside the code rather than in it, and the reply echoes it back so an editor + marks the buffer only for a mark the session actually applied. *) +let eval t ~code ~origin ~pause = if not (alive t) then error "the program exited; restart flan dev" else - match Session.eval ~origin t.session code with + match Session.eval ~origin ?pause t.session code with | c when not c.Session.installs -> (* Accepted into the session and nothing to send: a declaration the program already has, with no body and no new storage. Saying "ok" and @@ -454,10 +457,14 @@ let eval t ~code ~origin = { ogen = t.gen; oso = out; oll = ll; oloc = fn_loc t n }) c.Session.fns; ok - [ ":names " ^ Wire.strings c.Session.names; - ":fns " ^ Wire.strings c.Session.fns; - Printf.sprintf ":ms %.1f" - (timing.Build.llc_ms +. timing.Build.link_ms) ] + ([ ":names " ^ Wire.strings c.Session.names; + ":fns " ^ Wire.strings c.Session.fns; + Printf.sprintf ":ms %.1f" + (timing.Build.llc_ms +. timing.Build.link_ms) ] + @ (match pause with + | Some (l, c) -> + [ ":pause " ^ Wire.quote (Printf.sprintf "%d:%d" l c) ] + | None -> [])) | reply -> error ("the program refused the module: " ^ reply) | exception Unix.Unix_error (e, _, _) -> error @@ -1664,7 +1671,7 @@ let handle t req = let origin = match Wire.string_field req "file" with Some f -> f | None -> "" in - eval t ~code ~origin + eval t ~code ~origin ~pause:(Wire.pos_field req "pause") | None -> error "eval needs :code") | Some "eval-expr" -> (match Wire.string_field req "code" with diff --git a/lib/session.ml b/lib/session.ml index 3c18465..8d3a03e 100644 --- a/lib/session.ml +++ b/lib/session.ml @@ -270,7 +270,18 @@ type change = { installs : bool; } -let eval ?(origin = "") t src : change = +(* [pause] is [C-u C-c C-c]: the position, in the source just sent, of the form + the program should stop at — DISCUSS.md §9. It arrives as a separate field + rather than spliced into [src], because splicing text would move every + location after it, and it is applied below to the *declarations*, once + parsing has attached those locations and [Load] has qualified the names. + + Nothing here makes it stick and nothing has to: the marked declaration is + what goes into [t.decls], so it stays marked until an evaluation replaces + it — which is an ordinary [C-c C-c] over the same form, with no [:pause]. + That is §9's settled behaviour, and it is the same one statement that + accepts every other change. *) +let eval ?(origin = "") ?pause t src : change = let forms = Reader.read_all ~file:origin src in (* Through [Load] like any other source, so an evaluated (import ...) means what it means in a file. Its expansion is what gets spliced, which is also @@ -296,6 +307,20 @@ let eval ?(origin = "") t src : change = let loc = match incoming with d :: _ -> d.Ast.dloc | [] -> Loc.unknown in + (* After [qualify_decl], so a package that defines a [pause] of its own does + not capture the call this splices in. Refused when the position matches + nothing: installing an unmarked body and answering "ok" would report a + breakpoint that is not there. *) + let incoming = + match pause with + | None -> incoming + | Some (line, col) -> + (match Ast.mark_pause ~line ~col incoming with + | Some ds -> ds + | None -> + fail loc "nothing to pause at line %d, column %d of the form sent" + line col) + in let names = List.filter_map Ast.declared_name incoming in let replacement n = List.find_opt diff --git a/lib/wire.ml b/lib/wire.ml index 0a1d0ad..cabf274 100644 --- a/lib/wire.ml +++ b/lib/wire.ml @@ -103,3 +103,16 @@ let parse src = match Reader.read_all ~file:"" src with | [ f ] -> f | _ -> Loc.fail Loc.unknown "one form per message" + +(* [:pause (LINE COL)] — where in the source just sent a [(pause)] goes. A + position and not a span: the daemon matches it against the location the + reader already attached to that form, so the editor says *which* form by + saying where it starts. Anything that is not two integers is [None], and + the op says what it wanted, exactly as [int_field] does. *) +let pos_field form key = + match field form key with + | Some { Form.v = + Form.List [ { Form.v = Form.Int l; _ }; { Form.v = Form.Int c; _ } ]; + _ } -> + Some (Int64.to_int l, Int64.to_int c) + | _ -> None