A ring of imports is refused by name, not swallowed

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

@@ -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,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

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