An import cycle is refused and named, and packages are ordered dependencies first

BUILT.md

@@ -346,10 +346,28 @@ all four. A file carries no `.c` and no `link` file; those belong to a directory
 
 **A package may import a package.** The qualification flattens to the *inner* alias — raylib imported by a package that
 is itself imported is still `rl/…`, never `sand/rl/…` — because a directory reached along two routes has to arrive under
-one set of names or the checker sees every declaration twice. A directory is keyed by its real path and read once, which
-is also what ends a cycle: a package that imports itself meets its own entry and contributes nothing the second time,
-and the namespace being flat, mutually dependent packages simply work. The same directory under two *different* aliases
-is refused.
+one set of names or the checker sees every declaration twice. It is the one decision here that would be hardest to
+change later, because a qualified name is an identity the `layout` op and the break loop both resolve by.
+
+A directory is keyed by its real path and read once, so a **diamond** shares one copy of its bottom package.
+`programs/pkg-diamond.flan` is the case that proves it at the level that matters: `area/` and `draw/` both import
+`shape/`, a `shape/Box` is built inside `area/` and passed to a function declared inside `draw/`. Read `shape/` twice
+and there are two structs both named `shape/Box` which do not unify, so what proves the dedupe is that the program
+prints `3 6 20` and not that it compiles.
+
+The same directory under two *different* aliases is refused, including when one of the two is a package's own import
+and the other is pages away in the entry file — `programs/pkg-alias-clash.flan`.
+
+**A ring is refused and named.** Earlier it was not: the read-once table swallowed the second arrival, so mutually
+dependent packages appeared to work, and BUILT.md said so. What that cost is a definite package order, which is what
+the macro expander needs — every `defmacro` has to be compiled before anything that calls it — so it is now a refusal
+that names the ring, `a -> b -> c -> a`, rather than the single import that happened to close it. The two questions are
+kept apart by two pieces of state: the chain currently being read, and the set already finished. Found in the first is
+a cycle; found only in the second is the diamond's second route and still a no-op.
+
+`Load.t.pkgs` comes back **dependencies first** — the topological order the acyclic rule buys. The declaration list is
+deliberately unsorted: `check.ml` collects every top-level name in one pass before it checks any body, so top-level
+names are order-independent by construction. The order exists for the expander, which cannot work that way.
 
 **Visibility is one rule: `main` is not exported.** A package carrying one would collide with the importer's the moment
 anything imported it, so a program could never be a package; and `main` is a reachability root, so an imported one would
@@ -357,6 +375,9 @@ keep everything it calls alive. Writing `sand/main` is refused at the line that
 checker it would be "unknown name", which is true and useless.
 
 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
+name private, and inventing one is a reader and parser change.
 
 ## The link follows the program
 

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
 

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;

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

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)))

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)

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)

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))

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)))

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))

test/programs/pkgs/ring-b/ring-b.flan

@@ -0,0 +1,3 @@
+(import c "../ring-c")
+
+(defn step [n i32] i32 (c/step n))

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))

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)))

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,28 @@ 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]. Which of the two lines
+       the message names is whichever one arrived second, so it depends on the
+       order the entry file writes its imports in; both are correct refusals
+       and the needle deliberately does not pin it down. What is being tested
+       is that the clash is caught at all when the two halves are a page and a
+       directory apart, rather than side by side as in the case above. *)
+    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

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", [];