An edited package macro reloads as its new body, not its old one

Both unions kept the wrong side. macro_union keeps the left on a name
collision, and both callers had the older set on the left: Load.program
put the ambient set ahead of the packages it had just resolved, and
Session put the copy it had been holding since creation ahead of what
Load handed back. So editing a macro in a package and reloading the file
that imports it went on expanding the old body -- and said nothing,
which in this area is the failure that costs the most to find.

Two tests, because the two unions are reached by different paths: the
reload itself, and the C-c C-c after it, which reads the set the session
kept rather than the one Load just supplied. Each fails on its own if
only the other order is put back.

Also written down, and not fixed: C-x C-e expands no macros at all.
Parse.expr never calls the expander, so (unless ...) as a bare
expression is as much an unknown name as (mac/twice 4) -- the prelude
fails there too, which is what says it is an older gap. Changing it
changes what an expression evaluation means.

And the cost note is cut back to what was measured. Four macro modules
where the file's own macros leave two is what the cache shows; why four
is not settled and no longer claimed.

BUILT.md

@@ -665,6 +665,16 @@ before desugaring, where `` `(begin) `` and `(begin)` are still the same shape.
 replacing it — a session that replaced would expand `mac/twice` on the build and answer "unknown function" on the
 reload.
 
+Both unions put **what was just read off disk first**, because `macro_union` keeps the left on a name collision. Edit a
+macro in a package and reload the file that imports it: the session has been holding that macro since it was created,
+`Load` has just re-read it, and the other order goes on expanding the old body and says nothing about it. That is the
+quietest failure in this area and `test_session` pins it in both places — the reload itself, and the `C-c C-c` after
+the reload, which reads the set the session kept rather than the one `Load` handed it.
+
+Not covered: `C-x C-e`. `Parse.expr` does not run the expander at all, so a macro call typed as a bare expression is an
+unknown name — a package's and the prelude's alike, which is what says this is an older gap and not this feature's.
+`Session.eval_expr` is the only path that reaches it.
+
 Still missing: a package-private marker for anything other than `main`, which is why `rl/get-color-raw` is callable.
 The gap is surface syntax and not `load.ml` — `exported` is one predicate and the refusal machinery that points at the
 line which tried already exists, so a second rule is a line. What does not exist is any way for a package to *mark* a
@@ -3029,12 +3039,14 @@ were already expanded before the build was entered.
   under the object cache and keyed by a digest of the prelude's source plus the file's `defmacro` forms, so it is paid
   once per change rather than once per build. Every `flan build` is a fresh process, which is what makes the on-disk
   cache rather than a memo the right shape.
-- **Importing a package that declares macros moved the cold number and not the warm one.** A macro module is built per
-  *round*, so a package whose macros have a compile-order dependency among them costs its own rounds on top of the
-  importer's: `test/programs/pkg-macro.flan` leaves four `flan-macros-*.so` in the cache where `macros.flan` leaves two,
-  and cold it is about twice the wait. Warm it is the same 70ms, because every one of those modules is keyed and cached
-  like the first. A program that imports a package declaring **no** macro pays nothing new — the extra work is reading
-  the import forms, which is a scan of the top level.
+- **Importing a package that declares macros moved the cold number and not the warm one.** What the cache shows is
+  that more than one module is built: `test/programs/pkg-macro.flan` leaves four `flan-macros-*.so` behind where
+  `macros.flan` leaves two, and cold it is about twice the wait. Why four rather than three or two has not been
+  pinned down — the package's macros are expanded once for the package's own parse and again under the importer's
+  qualification, and the rounds within each of those are also separate modules; nothing here discriminates the two.
+  Warm it is the same 70ms, because every one of those modules is keyed and cached like the first. A program that
+  imports a package declaring **no** macro pays nothing new — the extra work is reading the import forms, which is a
+  scan of the top level.
 
   Measured on this machine, with the cache warm for the runtime and cold for every macro module, so the numbers are
   comparable to each other rather than to the two above: no macro 78ms, imports but no macro 110ms, the file's own

NEXT.md

@@ -108,6 +108,13 @@ import resolver at the Form level. It did not. The file being compiled is parsed
 too, so no shape of the feature could have left import resolution where it was — `Load.program` takes
 forms now and uses the one resolver that always existed.
 
+One gap left, and it is older than this work: **`C-x C-e` expands no macros at all.** `Parse.expr`
+never calls the expander, so `(unless ...)` typed as a bare expression is as much an unknown name as
+`(mac/twice 4)` is — the prelude's macros fail there too, which is what says this is not the package
+feature's doing. `Session.eval_expr` is the only path that reaches it and the fix is the wrap
+`Parse.decl` already has. Left alone deliberately: it changes what an expression evaluation means,
+which is a decision and not a repair.
+
 Two things found while finishing it. `Session` held the imported macro set but **replaced** it on
 every evaluation, and the one form `C-c C-c` sends carries no import — so a package macro worked on
 the build and was an unknown name on the first reload. It unions now, and `test_session` drives two

lib/load.ml

@@ -1179,7 +1179,13 @@ let hidden_of (t : t) = List.concat_map (fun (p : pkg) -> p.phidden) t.pkgs
    own set when it calls this, and an evaluation may add an import without
    losing what the file imported. Restored because a compiler process builds
    more than one program and a macro left ambient is a name that works until
-   somebody reorders the tests. *)
+   somebody reorders the tests.
+
+   What was just resolved comes first, because [macro_union] keeps the left on
+   a name collision. The ambient set is a session's, held since the session was
+   created; the packages were read off disk a line ago. Editing a macro in a
+   package and reloading the file that imports it has to expand the new body,
+   and the other order would expand the old one and say nothing. *)
 let program ?(parse = Parse.program) ~file (forms : Form.t list) : t =
   let seen = Hashtbl.create 8 in
   let imported =
@@ -1196,7 +1202,7 @@ let program ?(parse = Parse.program) ~file (forms : Form.t list) : t =
       (imports_of forms)
   in
   let decls =
-    Parse.with_imported (macro_union !Parse.imported_macros imported.macros)
+    Parse.with_imported (macro_union imported.macros !Parse.imported_macros)
       (fun () -> parse forms)
   in
   let t =

lib/session.ml

@@ -334,8 +334,14 @@ let eval ?(origin = "<eval>") ?pause t src : change =
        handed, and the one form C-c C-c sends has no import in it, so
        replacing would empty the set on the first re-evaluation of a defn —
        the macro would work on the build and be an unknown name on the
-       reload. *)
-    t.macros <- Load.macro_union t.macros l.Load.macros;
+       reload.
+
+       The incoming set comes first because [macro_union] keeps the left on a
+       name collision, and what [Load] just read off disk is newer than what
+       the session has been holding: editing a macro in a package and
+       reloading the file that imports it has to expand the new body. The
+       other order would keep the stale one and say nothing. *)
+    t.macros <- Load.macro_union l.Load.macros t.macros;
     let ds = l.Load.decls in
     match package_of t origin with
     | None -> ds

test/test_session.ml

@@ -237,6 +237,65 @@ let () =
      if not (has m "twice") then
        fail "an unqualified package macro said %S" m);
 
+  (* And editing the macro itself, in the package, then reloading the file
+     that imports it. The session is holding a copy of that macro from when it
+     was created, so the union has to prefer what [Load] has just read off
+     disk — keeping the held one would go on expanding the old body and say
+     nothing about it, which is the quietest failure in this whole area.
+
+     Written into a temporary package rather than into the corpus because the
+     point is the *second* read of a file that changed underneath. [+] first
+     and [*] after, because the expansion is visible in the IR: what is
+     asserted is the operator the macro chose, not that the reload succeeded.
+     *)
+  let tmp = Filename.temp_file "flan-macro" "" in
+  Sys.remove tmp;
+  Unix.mkdir tmp 0o755;
+  let pkg = Filename.concat tmp "p" in
+  Unix.mkdir pkg 0o755;
+  let write path text =
+    Out_channel.with_open_bin path (fun oc -> Out_channel.output_string oc text)
+  in
+  let macro op =
+    Printf.sprintf "(defmacro grow [args]
+  `(%s ~(at args 0) ~(at args 0)))
+" op
+  in
+  write (Filename.concat pkg "p.flan") (macro "+");
+  let entry = Filename.concat tmp "use.flan" in
+  let text = "(import p \"p\")
+
+(defn grown [] i32 (p/grow 21))
+
+(defn main [] i32 0)
+" in
+  write entry text;
+  let te, _ = Session.create ~file:entry () in
+  (match Session.eval ~origin:entry te text with
+   | c ->
+     if not (has c.Session.ir "add i32 21, 21") then
+       fail "a package macro's first expansion was not the one it declared"
+   | exception Loc.Error { Loc.dmsg = m; _ } ->
+     fail "reloading a file importing a macro package: %s" m);
+  write (Filename.concat pkg "p.flan") (macro "*");
+  (match Session.eval ~origin:entry te text with
+   | c ->
+     if not (has c.Session.ir "mul i32 21, 21") then
+       fail "an edited package macro reloaded as its old body";
+     if has c.Session.ir "add i32 21, 21" then
+       fail "an edited package macro kept the session's stale copy"
+   | exception Loc.Error { Loc.dmsg = m; _ } ->
+     fail "reloading an edited package macro: %s" m);
+  (* And the C-c C-c after that reload, which is the one that reads the set the
+     session *kept* rather than the one [Load] just handed it. Both unions have
+     to prefer the new copy or this is where the old body reappears. *)
+  (match Session.eval ~origin:entry te "(defn grown [] i32 (p/grow 21))" with
+   | c ->
+     if not (has c.Session.ir "mul i32 21, 21") then
+       fail "a form evaluated after an edited package reloaded used the old macro"
+   | exception Loc.Error { Loc.dmsg = m; _ } ->
+     fail "a form evaluated after an edited package reloaded: %s" m);
+
   (* A form typed into a file that is *imported as a package* has to be
      qualified the way the import qualified it, or it splices as a brand-new
      unrelated name: the evaluation reports success and the running program