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