From ac2af7c537201df6d6ddc43ddc83155b301815f2 Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sun, 20 Sep 2026 17:24:35 +0700 Subject: [PATCH] The diagnostics worklist, ranked, so it stops floating --- docs/DIAGNOSTICS-AUDIT.md | 225 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 225 insertions(+) create mode 100644 docs/DIAGNOSTICS-AUDIT.md diff --git a/docs/DIAGNOSTICS-AUDIT.md b/docs/DIAGNOSTICS-AUDIT.md new file mode 100644 index 0000000..d1c138b --- /dev/null +++ b/docs/DIAGNOSTICS-AUDIT.md @@ -0,0 +1,225 @@ +# Diagnostics audit — Flan against Elm + +Read-only audit, 2026-09-20. Feeds the fix pass that starts once the two live +lanes merge. Untracked on purpose: this is a worklist, not a spec. + +## The contract being graded against + +Elm's, plus this repo's own two additions: + +- **(S) Show / locate** — the exact code, a caret, at the *most specific* span. +- **(U) Understood vs conflicted** — say what the compiler took the code to + mean, and what that collides with. +- **(F) Fix by name** — name the thing to write instead, not the category. +- **(R) Register** — plain language. FIX.org's literary voice is banned here. + +Grades are per-dimension letters. A = Elm-class, B = good with one gap, +C = states the fact and stops, D = misleading or actively unhelpful. + +## The machinery that already exists (lib/loc.ml) + +Worth stating up front, because most of the worklist below is *underuse*, not +absence. + +- Spans, not points (`Loc.t` carries `eline`/`ecol`), and `squiggle` draws a + multi-column underline from them. +- **Secondary notes with their own locations and severities** (`Loc.note`, + rendered by `report` as their own `file:line:col: info:` entry, marked `-` + rather than `^`, sorted into source order). This is Elm's "this here… but + that there" and it is fully built. +- `kind` — a stable id per diagnostic (`"check/unknown-field"`), deliberately + never printed. +- `expansion` — names the macro a form came out of, automatically, off the + location. +- GNU `file:line:col:` first line, so `M-x compile` and `next-error` work. + +What it does **not** have: any notion of "expected because of *that* +signature" beyond a free-form note; anything a runtime trap can use (the +runtime has only preformatted loc strings, no access to source text). + +--- + +## The worst 20, ranked by (badness × how often a user hits it) + +| # | Message | Site | Trigger (one-liner) | S | U | F | R | Fix direction | +|---|---------|------|---------------------|---|---|---|---|---------------| +| 1 | `unknown name p.x` | check.ml:2732, 4046 | `(defstruct P [x i32])` then `p.x` anywhere | B | D | D | C | The dot-infix habit from C/Go/Odin. The checker can see the head `p` is bound and is a struct with field `x` — split on `.`, and say "field access is written `(.x p)`". The repo's own comment in examples/core-2d-camera.flan:20 writes `camera.rotation` in prose while the code is `(.rotation camera)`, so the author trips it too. | +| 2 | `dyn +: int and text, and it takes two numbers — (+ 3 "hi")` | flan_dyn.c:724 (`trap2`), :734 (`trap1`), :746 (`trap_range`), :814 (`trap_oom`) | `(defn add [x y] dyn (+ x y))` + `(add 3 "hi")`, then `flan run` | D | B | D | B | **No location at all** — no file, no line, nothing. In a dynamic-first language these *are* the type errors. flan_rt.c's bounds and arith traps already take an emitter-threaded `(loc, loclen)` pair, so the ABI precedent exists; the dyn entry points simply were never given one. Highest-leverage runtime fix. | +| 3 | `expected i32, found string` | check.ml:1866 (`expect`), 6292 (call args) | `(defn add [a i32 b i32] i32 …)` + `(add 1 "two")` | B | C | D | A | The most-hit message in the compiler. Caret is right; it never says *which* argument of *which* function, and never points at the parameter that wanted it. Elm's whole hallmark. `Loc.note` on the `Ast.field`'s `floc` is a five-line change: "the 2nd argument of `add`" + "`b` is declared `i32` here". | +| 4 | `unknown function prinltn` / `unknown name n` | check.ml:6259, 2732 | `(prinltn "hi")` | B | C | D | B | No did-you-mean, although `near_miss` (check.ml:670) is written, tested and wired — to **types only**. Point it at `env.fns`, `env.globals` and the local scope. Cheapest structural win on the list. | +| 5 | `expected bool, found i32` | check.ml:1866 via `check_truthy` (3596) | `(let [x 1] (if x …))` | A | C | D | A | Caret is exactly right (the `check_truthy` loc work paid off). But the message never states Flan's truthiness rule — bool or dyn, nothing else — and never names the fix (`(not= x 0)`). Special-case the condition position. | +| 6 | `binding 5 has no value — let takes name/value pairs` | parse.ml:670 | `(let [x i32 5] …)` | C | D | D | B | A type annotation in `let` is the single most natural thing for someone arriving from a typed language, and `let` has none. The message reads as if the user miscounted. Detect "middle form names a type" and say so: "`let` bindings take no type annotation — write `[x 5]`". | +| 7 | `get takes 2 arguments, given 1` against the **user's own** `(defn get [p P] …)` | check.ml:5296 (builtin dispatch) + 6233 | `(defn get [p P] i32 …)` + `(get p)` | D | D | D | B | A user defn whose name collides with a builtin is silently shadowed, and then the arity refusal is measured against the *builtin*, pointing at the user's call. Either refuse the shadowing definition at its `dloc` with a note, or report the arity against the definition the user can see. | +| 8 | `unhandled Boom` | flan_rt.c:646 | `(defstruct Boom [why i32])` + `(error (Boom {.why 7}))`, `flan run` | D | C | D | B | Three words. No location (not even the `error` site, which the emitter knows), no field values, no list of the handlers that were in scope. The condition system is a headline feature and this is its failure mode. | +| 9 | `the collection nosuch: is a directory named nosuch somewhere above /…/. , and there is none` | load.ml:120 | `(import zz "nosuch:thing")` | B | C | C | D | Reads as an assertion immediately contradicted. Also emits a bare `/.` on the path. Rewrite as a plain statement of the search ("no directory named `nosuch` between here and the root") and list what collections *were* found. | +| 10 | `and`'s last operand gets the previous operand's caret | parse.ml `shortcircuit`, via check.ml:3632 | `(println (and true true (vec-new i32)))` | D | B | C | A | Already diagnosed in FIX.org:1036 with three rejected fixes; the accepted one — `check_if` preferring the arm that is not a compiler temp when choosing which to blame — is a check.ml change nobody owned. This pass owns check.ml. | +| 11 | `unterminated string` | reader.ml:92 | `(println "oops` | C | C | C | A | Caret is one column on the opening quote, and there is **no** "the input ends here" note — unlike `reader/unclosed` (241) and `reader/mismatched-closer` (250), which both have one. Copy their shape. | +| 12 | `unknown type i — did you mean i8? A parameter with no type is dyn, so this would otherwise be read as a second parameter called i` | check.ml:850 | `(defn idx [v i] dyn …)` | A | B | C | C | Locates and explains well, but the did-you-mean fires on a *lowercase* name the user plainly meant as a parameter, so the suggestion is a false accusation. Suppress `near_miss` when the name is lowercase and in a parameter vector; lead with the dyn-parameter reading instead. | +| 13 | `expected a type, found 1. This is the return type, which every defn states -- a function that returns nothing writes ()` | parse.ml:1156 | `(defn f [x i32] (+ x 1))` | C | B | B | C | Says the fix, which is good. Two warts: the caret lands on the `1` deep inside the body rather than on the position where the return type belongs; and the literal `--` where the house uses `—` everywhere else. | +| 14 | `f64 is not a struct, so it has no fields` | check.ml:4028 | `(match s (Circle c) (.r c))` — single-field case binds the payload directly | B | C | D | A | The user wrote what looks like a destructuring pattern and got a type fact. Say what the pattern bound (`c` is the payload, an `f64`) and that the field is already in hand. | +| 15 | `% is a constant` | check.ml:4043 | `(defconst k 3)` + `(set k 4)` | A | C | D | A | Four words. Needs a `Loc.note` at the `defconst` and the named fix (`defvar`). `no_container_defconst` (7447) already shows how the house writes this well — imitate it. | +| 16 | `% is a parameter, and parameters are not assignable places (spec-memory.md) — bind a local with let` | check.ml:4037 | `(defn f [x i32] i32 (set x 1) x)` | A | B | B | C | Names the fix. Register wart: a diagnostic should not cite a spec filename at the user; put the rule in words and drop `(spec-memory.md)`. Same for `(plan.org, Types)` at 4609/4614 and `(see plan.org)` at 423. | +| 17 | `% is not implemented yet — milestone %d (see plan.org)` | check.ml:423 (`unimplemented`), used widely | `(Result i32 string)` in a type position | B | B | D | D | Sends the user to a planning document. Say what is missing in one clause and what to write in the meantime, or nothing. | +| 18 | `% takes numbers, found string` | check.ml:4219 (`binary`) | `(+ "a" "b")` | C | B | C | A | Caret covers the whole form rather than the offending operand — the exact "whole form vs operand" regression class this repo already fixed once in `check_truthy`. The operand's `Tast.eloc` is right there. Also: no mention of `str-cat`/the concatenation route, which is what the user wanted. | +| 19 | `expanding this declaration produced %d of them` / `expanding this expression produced %d forms, and an expression is one` | parse.ml:1541, 1569 | a `defmacro` returning two forms | B | C | D | C | "of them" has no antecedent. The `expansion` field on the diagnostic can name the macro automatically; use it, and suggest `do`. | +| 20 | `this fn has %d parameters and %s was wanted here` / `nothing here says what this fn's parameters are` | check.ml:2801, 2809 | passing an `fn` literal where no signature is in view | B | B | C | A | Correct and readable; the arity one should note the parameter list it was measured against, which is the one thing the reader cannot see from the caret. | + +### Also seen, not ranked + +- `an index is an i32, and %s is wider — write (i32 …), because …` (4113) is + **fine** — S/U/F/R ≈ A/A/A/B, listed only so the fix pass does not touch it. +- The `--warn-memory` / `check/no-gc` sites (8566, 8723) are warnings rather + than refusals and were not exercised; they build `Loc.diag` directly and + carry no notes. + +--- + +## The house's best — imitate these, not just Elm + +These are internal precedent and they already satisfy the contract. The fix +pass should copy **their shape**, so the corpus converges on one voice. + +**1. Unknown field, with the declaration shown.** check.ml:3706 / 2433 / +3793, via `declared_note` (169). This is the model. + +``` +v6.flan:3:23: P has no field z + 3 | (let [p (P {.x 1 .z 9})] (println (.x p)))) + | ^ +v6.flan:1:1: info: P is declared here, with x + 1 | (defstruct P [x i32]) + | --------------------- +``` + +Primary span on the exact offending key; secondary span on the declaration; +the available names enumerated. All four dimensions, in five lines. + +**2. Non-exhaustive match.** check.ml:3983. + +``` +w4.flan:3:3: this match is not exhaustive — Shape.Tri has no arm. Add it, or a _ arm for the rest + 3 | (match s + | ^^^^^^^^ +w4.flan:1:1: info: Shape is declared here, with Circle, Square, Tri +``` + +Names the missing case *and* both fixes, with the declaration alongside. +Elm-class. + +**3. The defconst pair**, landed 2026-09-20. `const_defconst_init` (7527) +and `no_container_defconst` (7447). No secondary span, but the prose does the +whole contract: what was understood ("the constant `n` is computed"), what it +conflicts with ("a defconst is what the linker writes into the image and has +nowhere to run"), and two named ways out (`defvar`, or a folded literal). The +best *prose-only* message in the tree. + +**4. The dyn view-lifetime refusal.** `view_not_permanent` (1570). Explains +the rule, gives the one shape that does work (`defvar g …`), and enumerates +what is refused. Borderline long — it is the closest thing in the tree to the +banned essay register, and a fix pass should cut it by a third rather than +lengthen anything toward it. + +**5. Reader's unclosed / mismatched-closer.** reader.ml:241, 250. The only +two-span diagnostics outside check.ml, and both are right: + +``` +u4.flan:2:25: expected ')' to close '(', found ']' +u4.flan:2:12: info: '(' is opened here +``` + +**6. `flan_arith_fail`'s overflow sentence** (flan_rt.c:911). A runtime trap +that explains *why* one pair of operands overflows a division. Good register, +good detail — it just lacks nothing except company. + +--- + +## Structural gaps — things Elm does that no Flan message does + +**1. Two-span "this here… but that there" as a habit, not an exception.** +*Not absent — underused.* `Loc.note` is fully built and rendered, and roughly +eight of ~250 sites use it (`check/duplicate-field`, `check/unknown-field`, +`check/defined-twice`, `check/duplicate-parameter`, +`parse/enum-autoincrement-collision`, `reader/unclosed`, +`reader/mismatched-closer`, `check/non-exhaustive-match`). **Feasible today, +no machinery needed** — the gap is that `expect` (1866) and the call-argument +path (6292) don't have the wanting-side location threaded to them. `Ast.field` +already carries `floc`; the defn record is reachable from the call site. This +is the single highest-value structural change and it is plumbing, not design. + +**2. Did-you-mean on anything but types.** `near_miss` (670) is a real +one-edit matcher with a real candidate set, wired at exactly one call site +(771). **Feasible today**: the candidate sets for functions, globals and +locals are all in `ctx.env` / the scope stack at the raise points +(check.ml:6259, 2732, 4046). Field names are already gathered by +`declared_note`, so field-typo suggestions are nearly free too. + +**3. Error titles / anchors / doc links.** Elm prints `-- TYPE MISMATCH ---` +and links a hint page. Flan has the ingredient — every diagnostic carries a +stable `kind` — and loc.ml:109 says explicitly that it is "never printed as +the reason". **Feasible today** at zero cost: print it as a trailing +`[check/unknown-field]` or route it to a docs anchor. Needs a decision, not +work. (Caveat: the first line is load-bearing for compilation-mode, so the id +belongs at the end of the line or on an indented continuation.) + +**4. Any caret at all at runtime.** Runtime traps carry a preformatted +`file:line:col` string and nothing else — flan_rt.c has no access to the +source text, and flan_dyn.c has not even the string. **Partly feasible**: the +`flan_trap_hook` (flan_rt.c:598) already hands control to the dev session, +which *is* in-process with the compiler and can read the file. Rendering a +squiggle there is the natural home — but dev.ml belongs to another lane, so +this is a hand-off, not a row in this pass. The cheap half — threading a loc +into flan_dyn.c's four trap functions — is entirely within this pass. + +**5. "What I understood" as structure rather than prose.** No message in the +tree separates the two halves the way Elm's body does ("This function expects +… / But you gave it …"). The good ones (defconst, view-lifetime) achieve it +with a paragraph. **Feasible as convention**, not machinery: the `diag` +record has `dmsg` plus notes and nothing between, so the shape would have to +be a writing rule — "first clause is what was read, second clause is the +collision, last clause names the fix" — enforced by review rather than types. + +**6. Multiple errors without cascade.** `Loc.sink` exists and finishes at +phase boundaries only, which loc.ml:178 admits is "deliberately crude". Not +graded here (no program was written to exercise cascade), but it is the +remaining Elm behaviour with machinery that only half exists. + +--- + +## Register notes for the fix pass + +- **Never cite a repo file at the user.** `(spec-memory.md)`, `(plan.org, + Types)`, `(see plan.org)` appear in at least six messages. Say the rule. +- **One dash convention.** parse.ml:1156 uses `--`; every other message uses + `—`. +- **"%s of them" / "this one"** — pronouns with no antecedent once the + message is read cold. parse.ml:1541 is the clearest case. +- **Length.** The tree's long messages are mostly *earning* their length + (defconst, runaway-instantiation, predicate-not-carried). The one to watch + is `view_not_permanent`, which is a paragraph with a subordinate clause + nested three deep. The rule the repo wants — plain language, no essay + register — bites there and nowhere else so far. + +--- + +## Coverage — honest statement + +- **Harvested**: every `Loc.fail` / `Loc.failk` / `Loc.diag` / `Loc.note` + call site in lib/check.ml, lib/parse.ml, lib/reader.ml and lib/load.ml — + roughly 250 sites — plus every `fprintf(stderr, …)` trap in + runtime/flan_rt.c and runtime/flan_dyn.c. +- **Rendered by hand**: about 30 messages, by writing the triggering program + and running `_build/default/bin/main.exe check` or `run` on it, and reading + the full output including carets and secondary lines. Scratch programs are + in the session scratchpad, not the repo. +- **Graded from source only**: the remaining ~220. Their S grade is inferred + from which location value they are raised against, which is reliable for + "is it the operand or the whole form" only where the code makes it obvious. + Treat those as provisional. +- **Out of scope**: lib/emit.ml, lib/x86.ml, lib/cimport.ml, lib/session.ml + and lib/macro.ml internals; the wasm32 and JS backends; anything printed by + `flan dev`'s break loop. +- **Not touched**: lib/dev.ml, which belongs to another lane. Gap 4's dev-side + half is written up as a hand-off for that reason. +- **Not exercised**: the `--warn-memory` warning path, macro-expansion + diagnostics with a non-trivial `expansion` chain, and multi-error cascade + behaviour through `Loc.sink`.