The review's follow-ups: a file decides the shadow, and a set answers the membership

The leak review found: an importer's (defn len ...) reached inside an
imported package's (defvar sz i32 (len "abcd")) and made it 999. A global
initialiser is checked with no enclosing function, so the qualified name the
first cut asked about was not there to ask. The file the definition was
written in is what the shadow follows now, which is what FIX.org had already
named as the fix if it ever mattered. It mattered.

builtin_set beside builtin_names: the guard is the first arm of the dispatch
and ran a linear walk of eighty-odd strings at every named call. The list
stays for the did-you-mean, whose order is its order.

Pinned: a shadowed operator warns and lowers to a Call, and a call carrying
another file's name reaches the builtin. The corpus program grew both cases
and the package grew the initialiser that demonstrated the leak.

And the int/float section's sentence about "the arity precedent, where the
builtin wins" now says that the precedent was deleted the same day, since
this lane is what deleted it.

FIX.org

@@ -2485,10 +2485,18 @@ way a reader would guess: local of function type, then generic signature,
 then the function table, then the builtins, then the struct and the
 did-you-mean refusals.
 
-~shadows_builtin~ asks three questions, cheapest first: is the name defined
+~shadows_builtin~ asks two questions, in this order. Is the name a
-(the two tables and the scope), is it a builtin's (otherwise there is no
+builtin's: one lookup in ~builtin_set~, false for every call to an ordinary
-order to change, and the enum/type-variable/machine-type cast arms keep
+function, and asking it first is also what keeps the arms that are not calls
-theirs), and is the definition visible here.
+— an enum cast, a cast to a type variable, a machine-type cast — exactly
+where they were. Then, and only then, is there a definition that reaches
+this call: a local of function type, or a defn written in this same file.
+
+~builtin_set~ is a ~Hashtbl~ and is new. The guard is the first arm of the
+dispatch, so it runs at every named call, and the list ~builtin_names~ that
+already existed is walked linearly — about a third of check time on a
+program of twenty thousand calls, measured in review. The list stays for the
+did-you-mean, whose order is its order; the set answers the membership.
 
 ** The warning, verbatim
 : shadow-builtin.flan:20:7: warning: get shadows the builtin get — every call in this program now reaches your definition
@@ -2514,13 +2522,26 @@ it.* ~Load~ qualifies every name an imported package declares to ~alias/name~,
 including its own uses of them, so a package's ~get~ is ~rl/get~ and cannot
 collide with a builtin at all. What is left is the other direction: a program
 that defines ~get~ and imports a package whose body calls the builtin ~get~.
-That call must keep meaning the builtin, and it does — ~shadows_builtin~
+That call must keep meaning the builtin, and it does: the shadow reaches
-answers false when the enclosing function's name is qualified, which inside
+exactly the file the definition was written in, which is the same visibility
-an imported package is always. The prelude is excluded the same way, by its
+a defn has everywhere else. The prelude falls out of the same rule rather
-file: it is the language's own source and means the builtin wherever it
+than needing one of its own — it is a file, and not the one the program is
-writes one. ~programs/shadow-builtin.flan~ is both halves in one program: 7
+in.
-is the program's own one-argument ~(get p)~, 4 is the builtin ~get~ called
+
-inside the package it imports.
+The file and not the enclosing function's name, which is what this first
+shipped with and was wrong. A package's functions are qualified at the
+import, so "does the owner's name carry a slash" answers correctly wherever
+a call sits inside a function — and wrongly in the one place a call does
+not. Review demonstrated it: a program defining ~(defn len ...)~ reached
+inside an imported package's ~(defvar sz i32 (len "abcd"))~, which is
+checked with no owner at all, and made it 999. A global initialiser has no
+enclosing name; it does have a file.
+
+~programs/shadow-builtin.flan~ is every half in one program: 7 is the
+program's own one-argument ~(get p)~, 4 is the builtin ~get~ called inside
+the package it imports, 99 is a shadowed ~+~, 999 is the program's own
+~len~, and the last 4 is that same ~len~ inside the package's global
+initialiser, where the builtin still means the builtin.
 
 *Prelude macros.* No rule was needed: the namespace is already one.
 ~(defn comment [x i32] i32 ...)~ against the prelude's ~(defmacro comment
@@ -2533,20 +2554,49 @@ so if the redefinition check were ever relaxed the macro would win and the
 defn would be unreachable; that is not a state this compiler can reach, and
 nothing was written to handle it.
 
-*The one place the rule is conservative.* A bare REPL expression evaluated
+*What the file rule costs.* A bare REPL expression — ~C-x C-e~ on a form,
-with no file behind it (origin ~<eval>~) is checked with an unqualified
+evaluated with origin ~<eval>~ and no file behind it — is not the file the
-owner and so does shadow, which is right; a call written inside an imported
+defn was written in, so it reaches the builtin. ~C-c C-c~ sends the buffer's
-package's *global initialiser* — no enclosing function, so no qualified owner
+own path and is unaffected, which is the case the dev loop is actually made
-— would not be excluded. Nothing in the corpus does that, and the fix if it
+of. It is the conservative direction: a REPL line meaning the builtin is a
-ever matters is to carry the package's file rather than the owner's name.
+surprise, a REPL line silently meaning a definition somewhere else is a
+worse one. If it ever bites, the fix is for the session to evaluate with the
+buffer's path as origin, which it already knows.
+
+*A macro named after a builtin warns too, and that is right.* ~(defmacro get
+[args] ...)~ is an ~Ast.Defn~ like any other by the time the declaration
+list is collected — a macro is a function the compiler runs — so
+~shadowed_builtins~ names it and the warning reads the same. The macro also
+wins, and by a different mechanism: expansion runs before checking and keys
+on the head name, so the call never becomes a call at all. The one wrinkle
+is that a file carrying macros is checked twice, the macro module first, so
+its warning is printed twice. Disclosed rather than suppressed: dropping a
+duplicate means keeping state across the two checks, and the second line is
+the same line.
+
+*The dead end: a shadowed builtin has no remaining spelling.* Nothing in
+this language qualifies a name — there is no ~core/get~, no ~(builtin get)~
+— so a file that defines ~get~ has given up the builtin ~get~ for the whole
+file, and a definition that wants to *wrap* the builtin cannot. ~(defn len
+[s string] i32 (+ 1 (len s)))~ is not a wrapper, it is unbounded recursion:
+the inner call reaches the definition being written, and the program
+stack-overflows at run time with no diagnostic from the compiler, which has
+nothing to object to. The warning says the name is taken over; it does not
+say this. An escape hatch is a language decision and is with the author.
 
 ** Pins
 - ~test_flan.ml~: the warning's kind, line and column; its message, matched
   whole and not by needle; that it carries no notes; that the source which
   used to be refused now checks; and that a program shadowing nothing warns
   not at all.
-- ~test_acceptance.ml~: ~programs/shadow-builtin.flan~ outputs ~7\n4\n~, and
+- ~test_flan.ml~, from review: a shadowed operator warns with the same
-  the ~@x86~ sweep compares both backends over the same file.
+  sentence and lowers to a ~Call~ to the definition rather than the ~Add~
+  prim; and a call read with another file's name, against the same
+  declaration list, reaches the builtin and is refused at the builtin's
+  arity — the global-initialiser case at its smallest.
+- ~test_acceptance.ml~: ~programs/shadow-builtin.flan~ outputs
+  ~7\n4\n99\n999\n4\n~, and the ~@x86~ sweep compares both backends over
+  the same file.
 - Removed: the ~check/builtin-arity~ kind, its message ("this is the builtin
   get, which a defn of the same name does not replace"), its note ("is also
   defined here, and this call is not reaching it — rename it to call it"),
@@ -2565,5 +2615,10 @@ at ..." and prints no FAIL line, only "1 failure(s)" at the end of the log:
 worth knowing, because a grep for FAIL says green over it.
 
 ** What was run
-~dune test --root .~ in the lane's worktree, forced: exit 0.
+~dune test --root .~ in the lane's worktree, forced: exit 0. Rebased onto
+dev-loop before the review follow-ups, so the ~arity~ signature this lane
+cuts down is the one the byte-fill lane had just given a ~ctx~ argument, and
+the ~int~/~float~ section's paragraph about "the ~arity~ precedent, where
+the builtin wins" is revised in place — that precedent is what this lane
+deleted.
 The heavy sweeps (~@x86~, ~@sanitize~, ~@valgrind~) were left to the batch.

lib/check.ml

@@ -297,6 +297,15 @@ let foreign_spelling = function
    and letting the two drift. Filled once, immediately after that table. *)
 let builtin_names : string list ref = ref []
 
+(* The same names as a set, and the two are not one because they are asked
+   two different questions. The list above is read once, at a refusal, and
+   its order is the order the did-you-mean walks. This is asked at *every*
+   named call — [shadows_builtin] is the first arm of the dispatch — and a
+   linear walk of eighty-odd strings per call is a cost a whole-program check
+   pays in full: measured at about a third of check time on a program of
+   twenty thousand calls. Filled beside the list. *)
+let builtin_set : (string, unit) Hashtbl.t = Hashtbl.create 128
+
 (* What a [break] or a [continue] may be talking about, innermost first.
 
    [Lloop] is a loop it is lexically inside, carrying its label if it was given
@@ -7257,41 +7266,62 @@ and ordinary_call ctx ~want loc name args =
           else Loc.failk "check/unknown-function" loc "unknown function %s" name
 
 (* Does the program's own definition of this name take this call over?
-   Three questions, and the order is the order that asks the fewest of them.
+   Two questions, in this order, and the order is what makes the guard cheap
+   enough to be the first arm of the dispatch.
 
-   Is the name a builtin's at all. This is asked first and not second: it is
+   Is the name a builtin's at all. One lookup in [builtin_set], false for
-   false for every call to an ordinary function, which is most calls in most
+   every call to an ordinary function — which is most calls in most programs
-   programs, and it is the question that stops the other two from running.
+   — and the question that stops the second from being asked at all. Asking
-   Asking it also keeps the arms that are not calls — an enum cast, a cast to
+   it first also keeps the arms that are not calls — an enum cast, a cast to
    a type variable, a machine-type cast — exactly where they were, since a
    name that reaches one of those is not a builtin's either.
 
-   Is it defined — by the function table, by a generic signature, or by a
+   Is there a definition of it that reaches this call: a local of function
-   local of function type. Two hash lookups and, only if both miss, the walk
+   type, or a defn — ordinary or generic — written in this same file.
-   down the scope, which is a list.
 
-   And is the definition visible here. Shadowing follows a defn's visibility
+   And is the definition visible here, which is asked of the two files: the
-   like anything else: the prelude is the language's own source and means the
+   one the definition was written in and the one this call is written in. A
-   builtin wherever it writes one, and an imported package's names are
+   definition shadows the builtin through its own file and no further, which
-   qualified ([rl/get]) so a call written [get] inside one is the builtin
+   is the same visibility a defn has everywhere else — the prelude is the
-   too — recognised by the owner's name, which the import qualified along
+   language's own source and means the builtin wherever it writes one, and an
-   with everything else. Neither is shadowed by a definition in the file
+   imported package keeps the builtin it was written against no matter what
-   being compiled, which is the point: a package that defines [get] does not
+   the program importing it decides to call [get].
-   change what [get] means to its importer, and an importer that defines one
+
-   does not change what it means inside the package. *)
+   The file and not the enclosing function's name. A package's functions are
+   qualified at the import ([rl/get]), so asking whether the owner's name
+   carries a slash answers correctly everywhere a call sits inside a
+   function — and wrongly in the one place a call does not: a package's
+   global initialiser, which is checked with no owner at all. An importer
+   defining [len] reached inside an imported [(defvar sz i32 (len "abcd"))]
+   and changed what it computed. The files were never wrong about it.
+
+   What it costs is the REPL: an expression evaluated with no file behind it
+   is not the file the defn was written in, so it reaches the builtin. That
+   is the conservative direction, and C-c C-c — which sends the buffer's own
+   path — is not affected. *)
 and shadows_builtin ctx loc name =
-  let defined () =
+  (* Where the definition was written, if this name has one. A generic is in
-    Hashtbl.mem ctx.env.fns name
+     [generics] and nowhere near [fn_locs], so both tables are asked. *)
-    || Hashtbl.mem ctx.env.gsigs name
+  let declared_in () =
-    || (match lookup ctx name with
+    match Hashtbl.find_opt ctx.env.fn_locs name with
-        | Some b -> (match b.bty with Types.Fn _ -> true | _ -> false)
+    | Some at -> Some at.Loc.file
-        | None -> false)
+    | None ->
+      (match Hashtbl.find_opt ctx.env.generics name with
+       | Some fn -> Some fn.Ast.nloc.Loc.file
+       | None -> None)
   in
-  let visible () =
+  (* A local of function type is lexical: it cannot be in scope anywhere but
-    not (String.equal loc.Loc.file Prelude.file)
+     the file that bound it, so there is no file to compare. *)
-    && not (String.contains ctx.owner '/')
+  let local_fn () =
+    match lookup ctx name with
+    | Some b -> (match b.bty with Types.Fn _ -> true | _ -> false)
+    | None -> false
   in
-  List.mem name !builtin_names && defined () && visible ()
+  Hashtbl.mem builtin_set name
+  && (local_fn ()
+      || (match declared_in () with
+          | Some file -> String.equal file loc.Loc.file
+          | None -> false))
 
 (* ── A call to a generic function ───────────────────────────────────────
    The whole of instantiation, and it is at the call site because the call
@@ -7849,7 +7879,9 @@ let builtins : (string * string * string) list =
 (* The forward reference declared beside [nearest], filled the moment the table
    it names exists. Nothing reads it before a call is checked, and no call is
    checked before this module is loaded. *)
-let () = builtin_names := List.map (fun (n, _, _) -> n) builtins
+let () =
+  builtin_names := List.map (fun (n, _, _) -> n) builtins;
+  List.iter (fun (n, _, _) -> Hashtbl.replace builtin_set n ()) builtins
 
 (* ── The one thing shadowing owes the reader ────────────────────────────
    A defn named after a builtin is legal and it wins ([shadows_builtin]), and
@@ -7872,7 +7904,7 @@ let shadowed_builtins (decls : Ast.decl list) : Loc.diag list =
     (fun (d : Ast.decl) ->
        match d.Ast.d with
        | Ast.Defn fn
-         when List.mem fn.Ast.name !builtin_names
+         when Hashtbl.mem builtin_set fn.Ast.name
               && not (String.contains fn.Ast.name '/')
               && not (String.equal fn.Ast.nloc.Loc.file Prelude.file) ->
          Some

test/programs/pkgs/shadowed/shadowed.flan

@@ -8,3 +8,11 @@
 ;;;; dyn map holds under a keyword.
 
 (defn field [m] dyn (get m :b))
+
+;;; The same question asked where there is no enclosing function to be
+;;; qualified: a global's initialiser, which runs at startup and is checked
+;;; with no owner at all. The importer below defines a len of its own; this
+;;; one is the builtin's and this global is 4.
+(defvar size i32 (len "abcd"))
+
+(defn stored-size [] i32 size)

test/programs/shadow-builtin.flan

@@ -4,14 +4,20 @@
 ;;;; at every call site in this file, and the compiler says so once at the
 ;;;; definition — the warning is on stderr and the exit status does not move.
 ;;;;
-;;;; Two calls, and between them the whole rule:
+;;;; Five lines printed, and between them the whole rule. In order:
 ;;;;
-;;;; - (get p) is one argument, which the builtin get does not take. It
+;;;; - 7: (get p) is one argument, which the builtin get does not take. It
 ;;;;   compiles, and it prints the field, because the name resolves to the
 ;;;;   definition below and the builtin is not consulted about its arity.
-;;;; - (shadowed/field m) reaches into the imported package, whose body calls
+;;;; - 4: (shadowed/field m) reaches into the imported package, whose body
-;;;;   the builtin get on a dyn map. The shadow does not follow it there: a
+;;;;   calls the builtin get on a dyn map. The shadow does not follow it
-;;;;   package's own calls mean what they meant when the package was written.
+;;;;   there: a package's calls mean what they meant when it was written.
+;;;; - 99: an operator is a builtin like any other and shadows like one.
+;;;; - 999: this file's own len, which is what len means in this file.
+;;;; - 4 again, and it is the one that needed the work: the package's global
+;;;;   initialiser (defvar size i32 (len "abcd")) is checked with no
+;;;;   enclosing function at all, so there is no qualified name on it to say
+;;;;   it belongs to a package. The file it was written in says so instead.
 
 (import shadowed "pkgs/shadowed")
 
@@ -19,6 +25,21 @@
 
 (defn get [p P] i32 (.x p))
 
+;;; An operator is a builtin like any other, and shadows like any other: (+ 1
+;;; 2) below is this definition and answers 99. Nothing else in the program
+;;; adds anything, and the prelude's own additions are untouched — the
+;;; prelude is a different file.
+(defn + [a i32 b i32] i32 99)
+
+;;; And a builtin the imported package uses in a *global initialiser*, which
+;;; is the one place there is no enclosing function to carry a package's
+;;; qualified name. The package's (defvar size i32 (len "abcd")) is 4; this
+;;; definition answers 999 and is reached only here.
+(defn len [s string] i32 999)
+
 (defn main [] ()
   (println (get (P {.x 7})))
-  (println (shadowed/field {:a 1 :b 4})))
+  (println (shadowed/field {:a 1 :b 4}))
+  (println (+ 1 2))
+  (println (len "abcd"))
+  (println (shadowed/stored-size)))

test/test_acceptance.ml

@@ -2476,19 +2476,25 @@ let () =
     outputs "a diamond, with a type crossing it" "programs/pkg-diamond.flan"
       "3\n6\n20\n";
     (* A defn named after a builtin, and the boundary the shadow stops at.
-       The numbers are the whole claim and neither of them could be printed
+       The numbers are the whole claim and none of them could be printed by
-       by the other reading: 7 is the program's own one-argument (get p),
+       the other reading: 7 is the program's own one-argument (get p), which
-       which the builtin get has no arity for at all, and 4 is the builtin
+       the builtin get has no arity for at all; 4 is the builtin get called
-       get called inside the imported package on a dyn map — the same name,
+       inside the imported package on a dyn map; 99 is an operator shadowed
-       in one program, meaning two things because the package's names were
+       like any other name; 999 is this program's len.
-       qualified at the import.
 
-       The warning the definition earns is on stderr and is pinned in
+       The last 4 is the one that was a bug. It is the package's global
+       initialiser, (defvar size i32 (len "abcd")), which is the one place a
+       call sits inside no function and so carries no package-qualified name
+       — the importer's len reached into it and made it 999. The shadow is
+       decided by the file the definition was written in, and a file is
+       something a global initialiser has.
+
+       The warning the definitions earn is on stderr and is pinned in
        test_flan.ml, where the line and column can be asked about directly.
        What is asserted here is that it changes nothing else: the program
        runs and its status is zero. *)
     outputs "a defn shadows a builtin, and the package it imports does not"
-      "programs/shadow-builtin.flan" "7\n4\n";
+      "programs/shadow-builtin.flan" "7\n4\n99\n999\n4\n";
     (* A data type crossing the same boundary, which was a refusal by name
        until vendor/edn needed one. The rename has two halves and the second
        is the one that is easy to do by accident only: the type's name is a

test/test_flan.ml

@@ -4343,6 +4343,46 @@ let () =
      | exception Loc.Error _ -> false);
   check "a program that shadows nothing is warned at not at all"
     (Check.shadowed_builtins (program "(defn f [] i32 1)") = []);
+  (* An operator is a builtin like any other and shadows like any other.
+     Pinned in both halves because it is the case most likely to be thought
+     of as special and quietly excepted later: the warning is the same
+     sentence, and the call is a [Call] to the definition rather than the
+     [Add] prim it would otherwise have lowered to. *)
+  let plus_src = "(defn + [a i32 b i32] i32 99)\n(defn f [] i32 (+ 1 2))" in
+  (match Check.shadowed_builtins (program plus_src) with
+   | [ d ] ->
+     check "an operator shadowed by a defn warns like any other builtin"
+       (d.Loc.kind = "check/shadows-builtin"
+        && d.Loc.dmsg
+           = "+ shadows the builtin + — every call in this program now \
+              reaches your definition")
+   | _ -> check "a shadowed operator warns exactly once" false);
+  (match checked plus_src with
+   | p ->
+     (match List.find_opt (fun (f : Tast.fn) -> f.Tast.name = "f") p.Tast.fns with
+      | Some { Tast.body = [ { Tast.e = Tast.Call ("+", _); _ } ]; _ } -> ()
+      | _ -> check "a shadowed operator's call reaches the defn" false)
+   | exception _ -> check "a shadowed operator's call reaches the defn" false);
+  (* And the file the definition was written in is what the shadow follows.
+     Same declaration list, a call whose location is another file: the
+     builtin, whose arity this call does not satisfy. This is the package
+     global-initialiser case at its smallest — an initialiser is checked with
+     no enclosing function, so the enclosing name cannot be what decides.
+
+     The shadowing defn takes *two* parameters and the builtin takes one, so
+     the two readings cannot produce the same sentence: reaching the builtin
+     is a refusal measured at one, and reaching the defn is no refusal at
+     all. With both at one argument this check passed under either
+     resolution, which is a check that cannot fail — found in review. *)
+  (match
+     Check.program
+       (program "(defn len [a string b string] i32 999)"
+        @ Parse.program (read ~file:"<elsewhere>" "(defn g [] i32 (len \"a\" \"b\"))"))
+   with
+   | _ -> check "a call in another file does not reach the shadow" false
+   | exception Loc.Error d ->
+     check "a call in another file reaches the builtin, at the builtin's arity"
+       (contains d.Loc.dmsg "len takes 1 argument, given 2"));
 
   (* and's last operand is the then arm and the sentinel carrying the previous
      operand's location is the else arm, so with no expectation in hand the