From a983a46ea5ae10a7190bcba89b9b15faabcfa6cb Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sun, 20 Sep 2026 19:51:52 +0700 Subject: [PATCH] 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 | 95 ++++++++++++++++++----- lib/check.ml | 90 ++++++++++++++------- test/programs/pkgs/shadowed/shadowed.flan | 8 ++ test/programs/shadow-builtin.flan | 33 ++++++-- test/test_acceptance.ml | 22 ++++-- test/test_flan.ml | 40 ++++++++++ 6 files changed, 225 insertions(+), 63 deletions(-) diff --git a/FIX.org b/FIX.org index 62bc377..b0d73ca 100644 --- a/FIX.org +++ b/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 -(the two tables and the scope), is it a builtin's (otherwise there is no -order to change, and the enum/type-variable/machine-type cast arms keep -theirs), and is the definition visible here. +~shadows_builtin~ asks two questions, in this order. Is the name a +builtin's: one lookup in ~builtin_set~, false for every call to an ordinary +function, and asking it first is also what keeps the arms that are not calls +— 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~ -answers false when the enclosing function's name is qualified, which inside -an imported package is always. The prelude is excluded the same way, by its -file: it is the language's own source and means the builtin wherever it -writes one. ~programs/shadow-builtin.flan~ is both halves in one program: 7 -is the program's own one-argument ~(get p)~, 4 is the builtin ~get~ called -inside the package it imports. +That call must keep meaning the builtin, and it does: the shadow reaches +exactly the file the definition was written in, which is the same visibility +a defn has everywhere else. The prelude falls out of the same rule rather +than needing one of its own — it is a file, and not the one the program is +in. + +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 -with no file behind it (origin ~~) is checked with an unqualified -owner and so does shadow, which is right; a call written inside an imported -package's *global initialiser* — no enclosing function, so no qualified owner -— would not be excluded. Nothing in the corpus does that, and the fix if it -ever matters is to carry the package's file rather than the owner's name. +*What the file rule costs.* A bare REPL expression — ~C-x C-e~ on a form, +evaluated with origin ~~ and no file behind it — is not the file the +defn was written in, so it reaches the builtin. ~C-c C-c~ sends the buffer's +own path and is unaffected, which is the case the dev loop is actually made +of. It is the conservative direction: a REPL line meaning the builtin is a +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 - the ~@x86~ sweep compares both backends over the same file. +- ~test_flan.ml~, from review: a shadowed operator warns with the same + 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. diff --git a/lib/check.ml b/lib/check.ml index e0f8fcc..3f7b21d 100644 --- a/lib/check.ml +++ b/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 - false for every call to an ordinary function, which is most calls in most - programs, and it is the question that stops the other two from running. - Asking it also keeps the arms that are not calls — an enum cast, a cast to + Is the name a builtin's at all. One lookup in [builtin_set], false for + every call to an ordinary function — which is most calls in most programs + — and the question that stops the second from being asked at all. Asking + 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 - local of function type. Two hash lookups and, only if both miss, the walk - down the scope, which is a list. + Is there a definition of it that reaches this call: a local of function + type, or a defn — ordinary or generic — written in this same file. - And is the definition visible here. Shadowing follows a defn's visibility - like anything else: the prelude is the language's own source and means the - builtin wherever it writes one, and an imported package's names are - qualified ([rl/get]) so a call written [get] inside one is the builtin - too — recognised by the owner's name, which the import qualified along - with everything else. Neither is shadowed by a definition in the file - being compiled, which is the point: a package that defines [get] does not - change what [get] means to its importer, and an importer that defines one - does not change what it means inside the package. *) + And is the definition visible here, which is asked of the two files: the + one the definition was written in and the one this call is written in. A + definition shadows the builtin through its own file and no further, which + is the same visibility a defn has everywhere else — the prelude is the + language's own source and means the builtin wherever it writes one, and an + imported package keeps the builtin it was written against no matter what + the program importing it decides to call [get]. + + 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 () = - Hashtbl.mem ctx.env.fns name - || Hashtbl.mem ctx.env.gsigs name - || (match lookup ctx name with - | Some b -> (match b.bty with Types.Fn _ -> true | _ -> false) - | None -> false) + (* Where the definition was written, if this name has one. A generic is in + [generics] and nowhere near [fn_locs], so both tables are asked. *) + let declared_in () = + match Hashtbl.find_opt ctx.env.fn_locs name with + | Some at -> Some at.Loc.file + | None -> + (match Hashtbl.find_opt ctx.env.generics name with + | Some fn -> Some fn.Ast.nloc.Loc.file + | None -> None) in - let visible () = - not (String.equal loc.Loc.file Prelude.file) - && not (String.contains ctx.owner '/') + (* A local of function type is lexical: it cannot be in scope anywhere but + the file that bound it, so there is no file to compare. *) + 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 diff --git a/test/programs/pkgs/shadowed/shadowed.flan b/test/programs/pkgs/shadowed/shadowed.flan index fa3e16a..1e67901 100644 --- a/test/programs/pkgs/shadowed/shadowed.flan +++ b/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) diff --git a/test/programs/shadow-builtin.flan b/test/programs/shadow-builtin.flan index 0ede1a4..9f9e316 100644 --- a/test/programs/shadow-builtin.flan +++ b/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 -;;;; the builtin get on a dyn map. The shadow does not follow it there: a -;;;; package's own calls mean what they meant when the package was written. +;;;; - 4: (shadowed/field m) reaches into the imported package, whose body +;;;; calls the builtin get on a dyn map. The shadow does not follow it +;;;; 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))) diff --git a/test/test_acceptance.ml b/test/test_acceptance.ml index 7c69c6d..36d7393 100644 --- a/test/test_acceptance.ml +++ b/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 - by the other reading: 7 is the program's own one-argument (get p), - which the builtin get has no arity for at all, and 4 is the builtin - get called inside the imported package on a dyn map — the same name, - in one program, meaning two things because the package's names were - qualified at the import. + The numbers are the whole claim and none of them could be printed by + the other reading: 7 is the program's own one-argument (get p), which + the builtin get has no arity for at all; 4 is the builtin get called + inside the imported package on a dyn map; 99 is an operator shadowed + like any other name; 999 is this program's len. - 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 diff --git a/test/test_flan.ml b/test/test_flan.ml index 69e776c..8bb569a 100644 --- a/test/test_flan.ml +++ b/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:"" "(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