From c604911ecb4973fff8fe854201c4c75f12208a65 Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sat, 12 Sep 2026 16:46:30 +0700 Subject: [PATCH] A ring of imports is refused by name, not swallowed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Loading a package kept one table, keyed by real path, and used it for two different questions. Already loaded meant "skip", which is right for the second route of a diamond and wrong for a ring: a package that imported itself round a chain met its own entry, contributed nothing, and appeared to work. The comment said so and called it a feature. It is not one. A ring has no package order, and a definite package order is what the macro expander needs — every defmacro has to be compiled before anything that calls it. So the chain currently being read is now carried separately from the set already finished. A directory found in the first is a cycle and is refused; a directory found only in the second is still the diamond's second route and still a no-op. The refusal names the ring — a -> b -> c -> a — and only the ring, not the route that led to it. "There is a cycle" leaves the reader to find which three imports it was. pkgs now comes back dependencies-first, which is the topological order the acyclic rule buys. The declaration list is left alone: check.ml collects every top-level name before it checks any body, so declarations are order-independent by construction and sorting them would be churn in the field every test reads. The tests are a real tree rather than a second copy of pkg-shared. pkg-diamond builds a shape/Box inside area/ and hands it to a function declared inside draw/, which only type-checks if the bottom package was read once — two copies of one struct are two types. What proves it is the numbers, not the compile. --- NEXT.md | 33 +++++++++++-- lib/load.ml | 68 ++++++++++++++++++++++----- test/dune | 10 ++++ test/programs/pkg-alias-clash.flan | 15 ++++++ test/programs/pkg-cycle.flan | 16 +++++++ test/programs/pkg-diamond.flan | 24 ++++++++++ test/programs/pkgs/area/area.flan | 12 +++++ test/programs/pkgs/draw/draw.flan | 9 ++++ test/programs/pkgs/ring-a/ring-a.flan | 6 +++ test/programs/pkgs/ring-b/ring-b.flan | 3 ++ test/programs/pkgs/ring-c/ring-c.flan | 7 +++ test/programs/pkgs/shape/shape.flan | 12 +++++ test/test_acceptance.ml | 33 +++++++++++++ test/test_sanitize.ml | 5 +- 14 files changed, 235 insertions(+), 18 deletions(-) create mode 100644 test/programs/pkg-alias-clash.flan create mode 100644 test/programs/pkg-cycle.flan create mode 100644 test/programs/pkg-diamond.flan create mode 100644 test/programs/pkgs/area/area.flan create mode 100644 test/programs/pkgs/draw/draw.flan create mode 100644 test/programs/pkgs/ring-a/ring-a.flan create mode 100644 test/programs/pkgs/ring-b/ring-b.flan create mode 100644 test/programs/pkgs/ring-c/ring-c.flan create mode 100644 test/programs/pkgs/shape/shape.flan diff --git a/NEXT.md b/NEXT.md index ef6fc4f..eef6d32 100644 --- a/NEXT.md +++ b/NEXT.md @@ -630,9 +630,28 @@ a definite package order is what the macro expander will need later, since every anything that calls it. Nested import paths not being real nesting — Odin's `core:math/bits` is a separate package rather than a submodule of `math`, with no re-exporting — was reviewed and accepted as fine. -**Still missing, and this is the real gap:** `load.ml` does not support **a package importing a package**. It is -listed among the milestone-4 loose ends. Until it lands a project is an entry file plus one flat layer of libraries, -with no library depending on another. +**A package importing a package has landed**, so a project is no longer an entry file plus one flat layer of +libraries. Four things were settled doing it: + +- **A name imported *through* a package keeps the inner alias.** If `area/` imports `shape`, the type is `shape/Box` + in the finished program and never `area/shape/Box`. This is forced rather than chosen: a directory reached along two + routes has to arrive under one set of names, or the checker sees every declaration twice and two copies of one + struct fail to unify. It is also what makes the dedupe coherent, and what keeps a qualified name the resolvable + identity the `layout` op and the break loop depend on. +- **The same directory under two aliases is refused**, including when one of the two aliases is a package's own and + pages away from the other. That is the price of the rule above and the refusal names both aliases. +- **A diamond loads its bottom once**, keyed by the real path. +- **A ring is refused and named** — `a -> b -> c -> a`, not "there is a cycle". Tolerating one was the earlier + behaviour and looked like it worked; what it cost is a definite package order, which is the thing the macro expander + needs, since every `defmacro` must be compiled before anything that calls it. + +`Load.t.pkgs` now comes back in topological order, dependencies first. The *declaration* list is deliberately not +sorted and does not need to be — `check.ml` collects every top-level name before it checks any body — so the order +exists for the expander, which cannot work that way. + +**Still missing: package visibility.** `rl/get-color-raw` is callable. The blocker is surface syntax, not `load.ml`: +`exported` and the refusal machinery already exist and take a second rule in one line, but there is no way for a +package to *mark* a name private, and adding one means a parser change. ## Blocked and unfinished @@ -1227,8 +1246,12 @@ cannot tell a return type from the first expression. Only the parameter position ## Loose ends from milestone 4 -None of them blocking: block-scoped `defer`; package visibility, so `rl/get-color-raw` is not callable; a package -importing a package; imported unions. +None of them blocking: block-scoped `defer`; package visibility, so `rl/get-color-raw` is not callable; imported +unions. + +A package importing a package was on this list and is off it. It loads, a diamond shares one copy of the bottom +package, the alias clash is refused through a chain as well as inside one file, and a ring is refused by name. What is +left of the item is visibility, which is listed above and needs a marker the parser does not have. ## Macros — the reader and the declaration are in, the expander is not diff --git a/lib/load.ml b/lib/load.ml index fbb044c..17b5e1d 100644 --- a/lib/load.ml +++ b/lib/load.ml @@ -592,17 +592,47 @@ let real dir = try Unix.realpath dir with Unix.Unix_error _ -> dir dedupe below coherent. [seen] is that dedupe, keyed by the real path, so raylib imported by the - program and again by a package it imports is loaded once. It is also what - terminates a cycle, and there is nothing else to do about one: a directory - is entered before it is read, so a package that imports itself — directly or - round a ring — meets its own entry and contributes nothing the second time. - The namespace is flat, so mutually dependent packages then simply work. *) -let rec import ~seen ~loc alias dir = + program and again by a package it imports is loaded once. + + [open_] is the separate question, and the two must not be confused. It is + the chain currently being read — the packages entered and not yet finished — + innermost last. A directory already in [seen] but not in [open_] is the + second route of a diamond and is the no-op that makes a diamond work; a + directory found in [open_] is an import that has come back round to a + package still waiting on it, which is a cycle. + + Cycles are refused rather than tolerated. An earlier version let [seen] + swallow them — a directory is entered before it is read, so the second + arrival contributed nothing and mutually dependent packages appeared to + work — but "appeared to work" is the problem. Acyclic imports are the thing + that makes a package order definite, and a definite order is what the macro + expander needs, since every [defmacro] has to be compiled before anything + that calls it. A ring has no such order, so it is named and refused here + rather than resolved arbitrarily by whichever package happened to be read + first. Odin forbids cycles for the same reason. *) +let rec import ~seen ~open_ ~loc alias dir = let dir' = real dir in + (* Checked before [seen], because a cycle's second arrival is also a repeat + visit and [seen] would otherwise call it a diamond and say nothing. *) + (match List.find_index (fun (d, _) -> String.equal d dir') open_ with + | Some i -> + (* The ring itself, and only the ring: the chain from the entry that is + being re-entered onwards, closed by naming it again. Anything before + that entry is the route *to* the cycle and not part of it. *) + let ring = List.filteri (fun j _ -> j >= i) open_ in + let names = + List.map (fun (_, a) -> a) ring @ [ alias ] + in + fail loc + "%s imports itself round a ring: %s. Imports have to be acyclic — a \ + definite package order is what lets a package be compiled before the \ + ones that use it — so one of these imports has to go" + (snd (List.nth open_ i)) (String.concat " -> " names) + | None -> ()); match Hashtbl.find_opt seen dir' with | Some previous when String.equal previous alias -> - (* Already in, under the same name. Importing it again is a no-op, which - is what lets two packages both depend on a third. *) + (* Already in, under the same name, and not still open. Importing it again + is a no-op, which is what lets two packages both depend on a third. *) { decls = []; csrcs = []; lflags = []; pkgs = [] } | Some previous -> fail loc @@ -610,6 +640,7 @@ let rec import ~seen ~loc alias dir = of names, so the two cannot both be true" dir alias previous | None -> Hashtbl.replace seen dir' alias; + let open_ = open_ @ [ (dir', alias) ] in let one_file = is_package_file dir in let files = if one_file then [ dir ] else entries dir ".flan" in if files = [] then fail loc "the package at %s has no .flan file" dir; @@ -637,7 +668,7 @@ let rec import ~seen ~loc alias dir = directory, for a single file the one it sits in. *) let file = if one_file then dir else Filename.concat dir "." in let sub = resolve_dir ~file d.Ast.dloc path in - Some (import ~seen ~loc:d.Ast.dloc a sub) + Some (import ~seen ~open_ ~loc:d.Ast.dloc a sub) | _ -> None) ds in @@ -791,13 +822,28 @@ let rec import ~seen ~loc alias dir = pkgs = [ { alias; dir; owns = owned; pcsrcs = csrcs; plflags = lflags; phidden } ] } in + (* Dependencies first. [pkgs] comes back in topological order — a package + appears after everything it imports — which is what the acyclic rule + above is worth: the recursion has already finished every nested import + before this line runs, so concatenating them ahead of [here] is the + topological sort, and the dedupe in [seen] keeps each package at its + first, deepest position. + + Only [pkgs] is ordered. The declaration list is deliberately not, and + does not need to be: [check.ml] collects every top-level name in one + pass before it checks any body, so top-level names are order-independent + by construction and a package may be declared after the one that uses + it. What will need the order is the macro expander, which cannot work + that way — a [defmacro] has to be compiled before the call it expands — + and it will read [pkgs]. *) List.fold_left (fun acc p -> { decls = acc.decls @ p.decls; csrcs = acc.csrcs @ p.csrcs; lflags = acc.lflags @ p.lflags; pkgs = acc.pkgs @ p.pkgs }) - here nested + { decls = []; csrcs = []; lflags = []; pkgs = [] } + (nested @ [ here ]) (* What an import did *not* bring: the names an importer might reasonably write and that are not there, each with the reason it is not. *) @@ -828,7 +874,7 @@ let program ~file (decls : Ast.decl list) : t = match d.Ast.d with | Ast.Import (alias, path) -> let dir = resolve_dir ~file d.Ast.dloc path in - let p = import ~seen ~loc:d.Ast.dloc alias dir in + let p = import ~seen ~open_:[] ~loc:d.Ast.dloc alias dir in { decls = acc.decls @ p.decls; csrcs = acc.csrcs @ p.csrcs; lflags = acc.lflags @ p.lflags; diff --git a/test/dune b/test/dune index 89f7b30..4d7a689 100644 --- a/test/dune +++ b/test/dune @@ -31,6 +31,16 @@ ; examples/digits.flan, so the directory has to be here whole. (glob_files %{workspace_root}/examples/*) (glob_files programs/*.flan) + ; The package tree the multi-level cases import: pkg-diamond reaches shape + ; through area and draw, and pkg-cycle reaches a ring. Each directory is a + ; package, so each comes whole — a glob per directory rather than one over + ; programs/pkgs/*, because dune's glob does not descend. + (glob_files programs/pkgs/shape/*) + (glob_files programs/pkgs/area/*) + (glob_files programs/pkgs/draw/*) + (glob_files programs/pkgs/ring-a/*) + (glob_files programs/pkgs/ring-b/*) + (glob_files programs/pkgs/ring-c/*) ; The synthetic C header the importer's table reads. Committed rather than ; reached for on the machine: the raylib case needs raylib installed, at the ; right version, with a variable set, so it skips everywhere and covers diff --git a/test/programs/pkg-alias-clash.flan b/test/programs/pkg-alias-clash.flan new file mode 100644 index 0000000..e6d39de --- /dev/null +++ b/test/programs/pkg-alias-clash.flan @@ -0,0 +1,15 @@ +;;;; One directory, two names — where one of the two is a package's. +;;;; +;;;; pkg-two-aliases.flan has both import forms in the entry file, where the +;;;; clash is written down in one place and easy to see. This is the case that +;;;; only exists once a package may import a package: area/ imports shape as +;;;; [shape], this file imports the same directory as [sh], and the two names +;;;; are pages apart. The rule is the same one — a directory reached twice +;;;; arrives under one set of names, or every declaration in it exists twice — +;;;; and it has to hold through a chain, because that is where nobody can see +;;;; both halves at once. + +(import sh "pkgs/shape") +(import area "pkgs/area") + +(defn main [] i32 (area/of (sh/box 2 2))) diff --git a/test/programs/pkg-cycle.flan b/test/programs/pkg-cycle.flan new file mode 100644 index 0000000..3103ec9 --- /dev/null +++ b/test/programs/pkg-cycle.flan @@ -0,0 +1,16 @@ +;;;; An import ring, which is refused and named. +;;;; +;;;; ring-a imports ring-b imports ring-c imports ring-a. Nothing about the +;;;; flat namespace makes this impossible — an earlier version let the +;;;; already-loaded check swallow the second arrival and the three packages +;;;; appeared to work — but a ring has no package order, and a definite +;;;; package order is what the macro expander will need, since a defmacro must +;;;; be compiled before anything that calls it. So it is refused, and the +;;;; refusal names the ring rather than the one import that happened to close +;;;; it last. + +(import a "pkgs/ring-a") + +(defn main [] i32 + (print (a/step 1)) (println "") + 0) diff --git a/test/programs/pkg-diamond.flan b/test/programs/pkg-diamond.flan new file mode 100644 index 0000000..90306bc --- /dev/null +++ b/test/programs/pkg-diamond.flan @@ -0,0 +1,24 @@ +;;;; A diamond, with a type crossing it. +;;;; +;;;; The tree is pkg-diamond -> {area, draw} -> shape, which is the case that +;;;; only works if a package may import a package and if the shared bottom is +;;;; loaded once. The proof is not that it compiles but that a shape/Box built +;;;; inside area/ is accepted by a function declared inside draw/: two copies +;;;; of one struct would be two types, and passing one to the other would fail +;;;; to unify with a message naming shape/Box twice and explaining nothing. +;;;; +;;;; shape is never imported here directly. It arrives only through the two +;;;; middles, so its names are in scope because a package's import qualified +;;;; them, not because this file did — which is the inner-alias rule, visible. + +(import area "pkgs/area") +(import draw "pkgs/draw") + +(defn main [] i32 + (let [b (area/unit)] + ;; Built in area/, measured in draw/ — one type or no program. + (print (draw/describe b)) (println "") + (print (area/of b)) (println "") + ;; And the bottom package's own constructor, reached through the chain. + (print (area/of (shape/box 4 5))) (println "")) + 0) diff --git a/test/programs/pkgs/area/area.flan b/test/programs/pkgs/area/area.flan new file mode 100644 index 0000000..dfcf442 --- /dev/null +++ b/test/programs/pkgs/area/area.flan @@ -0,0 +1,12 @@ +;;;; One middle of the diamond. Imports shape, and hands shape/Box back out. +;;;; +;;;; The alias is the *inner* one: this file says shape, and the finished +;;;; program calls the type shape/Box, not area/shape/Box. A package imported +;;;; through an intermediate arrives under the name its own importer chose. + +(import shape "../shape") + +(defn of [b shape/Box] i32 (* (.w b) (.h b))) + +;;; A shape/Box made on this side of the diamond, to be read on the other. +(defn unit [] shape/Box (shape/box 3 2)) diff --git a/test/programs/pkgs/draw/draw.flan b/test/programs/pkgs/draw/draw.flan new file mode 100644 index 0000000..b0b1d67 --- /dev/null +++ b/test/programs/pkgs/draw/draw.flan @@ -0,0 +1,9 @@ +;;;; The other middle. Imports the same shape, under the same alias. +;;;; +;;;; Reached along a second route, shape must already be loaded and must not +;;;; be loaded again. + +(import shape "../shape") + +(defn describe [b shape/Box] i32 + (if (shape/wide? b) (.w b) (.h b))) diff --git a/test/programs/pkgs/ring-a/ring-a.flan b/test/programs/pkgs/ring-a/ring-a.flan new file mode 100644 index 0000000..79f854a --- /dev/null +++ b/test/programs/pkgs/ring-a/ring-a.flan @@ -0,0 +1,6 @@ +;;;; One third of a ring: a -> b -> c -> a. Three rather than two, so the +;;;; refusal has to name a chain and not just a pair. + +(import b "../ring-b") + +(defn step [n i32] i32 (b/step n)) diff --git a/test/programs/pkgs/ring-b/ring-b.flan b/test/programs/pkgs/ring-b/ring-b.flan new file mode 100644 index 0000000..8b9603a --- /dev/null +++ b/test/programs/pkgs/ring-b/ring-b.flan @@ -0,0 +1,3 @@ +(import c "../ring-c") + +(defn step [n i32] i32 (c/step n)) diff --git a/test/programs/pkgs/ring-c/ring-c.flan b/test/programs/pkgs/ring-c/ring-c.flan new file mode 100644 index 0000000..ac8349b --- /dev/null +++ b/test/programs/pkgs/ring-c/ring-c.flan @@ -0,0 +1,7 @@ +;;;; And the edge that closes the ring. + +(import a "../ring-a") + +(defn step [n i32] i32 (+ n 1)) + +(defn twice [n i32] i32 (a/step n)) diff --git a/test/programs/pkgs/shape/shape.flan b/test/programs/pkgs/shape/shape.flan new file mode 100644 index 0000000..d8c7a39 --- /dev/null +++ b/test/programs/pkgs/shape/shape.flan @@ -0,0 +1,12 @@ +;;;; The bottom of the diamond: the one package both middles depend on. +;;;; +;;;; Box has to be one type. If this directory were read twice — once along +;;;; area/, once along draw/ — there would be two defstructs both qualified +;;;; shape/Box, and the checker would either refuse the collision or, worse, +;;;; unify nothing. That is the failure this tree exists to catch. + +(defstruct Box [w i32 h i32]) + +(defn box [w i32 h i32] Box (Box {.w w .h h})) + +(defn wide? [b Box] bool (> (.w b) (.h b))) diff --git a/test/test_acceptance.ml b/test/test_acceptance.ml index 8ca995a..7779c2c 100644 --- a/test/test_acceptance.ml +++ b/test/test_acceptance.ml @@ -1032,6 +1032,20 @@ let () = raylib itself. Loading it twice would declare every binding twice. *) outputs "a package reached along two routes" "programs/pkg-shared.flan" "ok\n"; + (* And the diamond with a type crossing it, which is the case the dedupe + exists for rather than a restatement of the one above. pkg-diamond + imports area and draw; both import shape; a shape/Box is built inside + area and handed to a function declared inside draw. Read shape twice + and there are two structs both called shape/Box, which do not unify — + so the numbers are the test and compiling is not. 3 is draw/describe + picking .w of a 3x2 box it never constructed, 6 is area/of on the same + value, 20 is shape's own constructor reached through the chain. + + It is also where the inner-alias rule is visible: shape is imported by + area and by draw and never by the program, and the name is still + shape/Box and not area/shape/Box. *) + outputs "a diamond, with a type crossing it" "programs/pkg-diamond.flan" + "3\n6\n20\n"; (* A local shadows an imported name. Qualification rewrites a package's own names wherever they are used, and a binding is where it has to stop — in an expression and in a place, which are two separate lines of the @@ -1095,6 +1109,25 @@ let () = "sand/main is not a name"; refuses "one directory under two aliases" "programs/pkg-two-aliases.flan" "one directory is one set of names"; + (* A ring is refused and the ring is named. The needle is the chain, not + the word "cycle": what a person needs is which three imports, and the + refusal that says only "there is a cycle" leaves them to find it. The + ring is a -> b -> c -> a, and the message closes it by repeating the + package it came back to. *) + (* And the same rule through a chain, which is the case that only exists + once a package may import a package: area imports shape as [shape] and + the program imports the same directory as [sh]. The refusal has to point + at the package's own import line, since that is the half the person + reading the entry file cannot see. *) + refuses "one directory under two aliases, through a package" + "programs/pkg-alias-clash.flan" "one directory is one set of names"; + (* A ring is refused and the ring is named. The needle is the chain, not + the word "cycle": what a person needs is which three imports, and the + refusal that says only "there is a cycle" leaves them to find it. The + ring is a -> b -> c -> a, and the message closes it by repeating the + package it came back to. *) + refuses "an import ring" "programs/pkg-cycle.flan" + "round a ring: a -> b -> c -> a"; refuses "two mains in one program" "programs/pkg-two-mains.flan" "main is defined twice"; (* nth is gone, not renamed: it has to fail as a name nobody defined. If it diff --git a/test/test_sanitize.ml b/test/test_sanitize.ml index dc3e52b..933b0df 100644 --- a/test/test_sanitize.ml +++ b/test/test_sanitize.ml @@ -101,8 +101,8 @@ let reported text = List.exists (contains text) markers harness. The reload path is half-covered at best in any case: a redefinition module is built by llc and ld, not by clang, so nothing instruments it — one more thing this sweep does not prove. - - nth-gone, pkg-hidden-main, pkg-two-aliases, pkg-two-mains, which are - negative cases and are expected not to compile. + - nth-gone, pkg-hidden-main, pkg-two-aliases, pkg-two-mains, pkg-cycle, + which are negative cases and are expected not to compile. calc-me is here and is not in test/programs: it is the one string parser in the corpus, which makes it the likeliest to push [scratch] or [escaped] @@ -128,6 +128,7 @@ let corpus = "programs/error.flan", []; "programs/machine.flan", []; "programs/math.flan", []; + "programs/pkg-diamond.flan", []; "programs/pkg-return.flan", []; "programs/pkg-shared.flan", []; "programs/pkg-unused.flan", [];