diff --git a/BUILT.md b/BUILT.md index 241e1c4..2a14e59 100644 --- a/BUILT.md +++ b/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 diff --git a/NEXT.md b/NEXT.md index ad15709..a131af4 100644 --- a/NEXT.md +++ b/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 diff --git a/lib/load.ml b/lib/load.ml index 9e690d6..87e4ff9 100644 --- a/lib/load.ml +++ b/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 = diff --git a/lib/session.ml b/lib/session.ml index e8f1f92..97dc426 100644 --- a/lib/session.ml +++ b/lib/session.ml @@ -334,8 +334,14 @@ let eval ?(origin = "") ?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 diff --git a/test/test_session.ml b/test/test_session.ml index b42fa68..562132d 100644 --- a/test/test_session.ml +++ b/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