From e03028819c3c3bdaac7ca477d05d958fb3df5332 Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sun, 20 Sep 2026 19:28:00 +0700 Subject: [PATCH 1/2] A defn named after a builtin now wins, and says so once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The author's rule: "allow shadowing but warn". A user (defn get ...) is legal, the user's definition wins at every call site in the file that wrote it, and the compiler warns once at the definition. Builtin-wins was never a rule anybody wrote: named_call is one match on the name, the builtin arms are string literals, and the three arms that look a name up are the last three in it. So a guard goes first, the trailing three are factored into ordinary_call, and both routes into it resolve a name the same way. The shadow stops at the file that declared it. An imported package's names were qualified at the import, so a get written inside one is the builtin's and stays the builtin's; the prelude is excluded by its file for the same reason. programs/shadow-builtin.flan is both halves at once. The warning prints from build_program, which is what every command and the dev daemon's reload go through, in the shape --warn-memory established: file:line:col, the squiggle, and an exit status that does not move. And the message that described the old world is gone — the builtin-arity note said a defn does not replace a builtin, which is no longer true and is no longer reachable. --- FIX.org | 129 +++++++++++++++++- docs/DIAGNOSTICS-AUDIT.md | 2 +- lib/check.ml | 155 +++++++++++++++++----- test/dune | 16 ++- test/programs/pkgs/shadowed/shadowed.flan | 10 ++ test/programs/shadow-builtin.flan | 24 ++++ test/test_acceptance.ml | 14 ++ test/test_flan.ml | 68 ++++++---- 8 files changed, 354 insertions(+), 64 deletions(-) create mode 100644 test/programs/pkgs/shadowed/shadowed.flan create mode 100644 test/programs/shadow-builtin.flan diff --git a/FIX.org b/FIX.org index 9cdda86..62bc377 100644 --- a/FIX.org +++ b/FIX.org @@ -2042,6 +2042,11 @@ wins every call and the definition is still unreachable; what changed is that the arity message says so and notes the definition. Refusing the shadowing is a language decision and was left to the author. +[Superseded the same day by the author's decision — see "Shadowing a builtin" +below. The builtin no longer wins, the definition is no longer unreachable, +and the arity note this paragraph describes has been removed along with the +world it described.] + * ~int~ and ~float~ as builtin aliases, 2026-09-20 The author, on the foreign-spelling list the diagnostics pass had just @@ -2098,8 +2103,12 @@ was a builtin. The rule is decided by the target, at registration: - Anything else — refused: "int is a builtin alias for i32 and cannot be redefined as i64 — delete this defalias, or give the type another name". -The alternative was the ~arity~ precedent, where the builtin wins and a note -surfaces at the error the shadowing caused. It does not transfer: a +The alternative was the ~arity~ precedent, where the builtin won and a note +surfaced at the error the shadowing caused — a precedent deleted later the +same day, when shadowing a builtin became legal and the user's definition +started winning instead (see "Shadowing a builtin" below); the reasoning +below stands either way, because neither world has anywhere to put the note. +It does not transfer: a ~(defalias int i64)~ has no later error site to hang a note on. ~resolve_name~ reaches ~ikind_of_name~ before the alias table, so the declaration would be read as ~i32~ at every use and nothing would ever say so. Silence was the one @@ -2442,3 +2451,119 @@ failures were that row and the sixth was ~dev-trap-free-all~, so what is racy is ~trap_park~ itself and every row that calls it — which is exactly what the mechanism described there predicts. Per the sweep policy the ~@x86~ and ~@sanitize~ sweeps were not run here. +* Shadowing a builtin, 2026-09-20 + +The author's decision, in the author's words: + +#+begin_quote +"allow shadowing but warn" — a user ~(defn get ...)~ colliding with a builtin +is legal, the USER'S definition wins at call sites (real shadowing, Clojure's +model: the def takes over, a warning says so), and the compiler warns once at +the definition site. +#+end_quote + +** Where builtin-wins actually lived +Not in a table and not in a precedence list. ~named_call~ is one +~match name with~ whose arms are the builtin names written out as string +literals, and the three arms that look anything up — a local of ~Fn~ type, +~gsigs~, then ~env.fns~ — are the last three in that match. So a builtin won +because OCaml tried its arm first, and for no other reason. ~env.fns~ never +outranked anything; it was simply never reached for a name spelled like a +builtin. The old comment above ~arity~ said this outright ("the dispatch +above reaches every builtin arm before it ever looks in [fns]") and is the +only place it was written down. + +** The resolution change +One guard, first arm of ~named_call~: + +: | _ when shadows_builtin ctx loc name -> ordinary_call ctx ~want loc name args + +and the three trailing arms factored into ~ordinary_call~ so that both routes +— falling past every builtin, and being sent straight there by the guard — +resolve a name by exactly the same rules. Order is now total and reads the +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. + +** The warning, verbatim +: shadow-builtin.flan:20:7: warning: get shadows the builtin get — every call in this program now reaches your definition +: 20 | (defn get [p P] i32 (.x p)) +: | ~~~ + +Rendered by ~Loc.entry ~mark:'~' ~label:"warning: "~, which is the +~--warn-memory~ precedent, so flycheck parses it exactly as it parses an +error. Nothing raises and the exit status does not move. Unlike +~--warn-memory~ it is behind no flag: there is nothing to tune, and the line +is one line and rare. + +It is printed from ~Check.build_program~ rather than from ~bin/main.ml~ +beside ~print_memory_warnings~, because every route into the compiler passes +through that function — build, check, run, and the dev daemon's reload, which +is where a defn is most likely to be written. The list itself is +~Check.shadowed_builtins~, a pure function over the declarations, which is +what the tests ask. + +** Scope, settled from the code +*Package-wide or program-wide: neither, and the mechanism already decided +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. + +*Prelude macros.* No rule was needed: the namespace is already one. +~(defn comment [x i32] i32 ...)~ against the prelude's ~(defmacro comment +...)~ is refused today as "comment is defined twice", with a note at the +prelude's definition, and the same for ~inc~ and ~dec~. Shadowing a builtin +is a different question precisely because a builtin is not a declaration — +it is an arm in the compiler, with nothing for a redefinition check to point +at. Macros expand before checking and key on the head name unconditionally, +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. + +** 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. +- 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"), + and the three checks that pinned them. The situation cannot arise: the call + reaches the user's defn, whose arity is whatever it declared. +- Changed: the builtin-arm/~Check.builtins~ cross-check reads ~named_call~'s + source down to ~ | _ ->~ rather than ~ | _~, because the new first arm is + guarded and stopping at it read the whole region as empty. + +** One thing the new package cost +A package under ~test/programs/pkgs/~ needs a ~glob_files~ line of its own in +four places in ~test/dune~ — the test stanza and the ~@valgrind~, ~@x86~ and +~@js~ sweeps — because dune's glob does not descend and the sweeps walk +~programs/*.flan~ whole. Without it the corpus row fails with "no package +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. +The heavy sweeps (~@x86~, ~@sanitize~, ~@valgrind~) were left to the batch. diff --git a/docs/DIAGNOSTICS-AUDIT.md b/docs/DIAGNOSTICS-AUDIT.md index d1c138b..1c9d7ea 100644 --- a/docs/DIAGNOSTICS-AUDIT.md +++ b/docs/DIAGNOSTICS-AUDIT.md @@ -49,7 +49,7 @@ runtime has only preformatted loc strings, no access to source text). | 4 | `unknown function prinltn` / `unknown name n` | check.ml:6259, 2732 | `(prinltn "hi")` | B | C | D | B | No did-you-mean, although `near_miss` (check.ml:670) is written, tested and wired — to **types only**. Point it at `env.fns`, `env.globals` and the local scope. Cheapest structural win on the list. | | 5 | `expected bool, found i32` | check.ml:1866 via `check_truthy` (3596) | `(let [x 1] (if x …))` | A | C | D | A | Caret is exactly right (the `check_truthy` loc work paid off). But the message never states Flan's truthiness rule — bool or dyn, nothing else — and never names the fix (`(not= x 0)`). Special-case the condition position. | | 6 | `binding 5 has no value — let takes name/value pairs` | parse.ml:670 | `(let [x i32 5] …)` | C | D | D | B | A type annotation in `let` is the single most natural thing for someone arriving from a typed language, and `let` has none. The message reads as if the user miscounted. Detect "middle form names a type" and say so: "`let` bindings take no type annotation — write `[x 5]`". | -| 7 | `get takes 2 arguments, given 1` against the **user's own** `(defn get [p P] …)` | check.ml:5296 (builtin dispatch) + 6233 | `(defn get [p P] i32 …)` + `(get p)` | D | D | D | B | A user defn whose name collides with a builtin is silently shadowed, and then the arity refusal is measured against the *builtin*, pointing at the user's call. Either refuse the shadowing definition at its `dloc` with a note, or report the arity against the definition the user can see. | +| 7 | `get takes 2 arguments, given 1` against the **user's own** `(defn get [p P] …)` | check.ml:5296 (builtin dispatch) + 6233 | `(defn get [p P] i32 …)` + `(get p)` | D | D | D | B | A user defn whose name collides with a builtin is silently shadowed, and then the arity refusal is measured against the *builtin*, pointing at the user's call. Either refuse the shadowing definition at its `dloc` with a note, or report the arity against the definition the user can see. **Settled 2026-09-20, neither way: the author chose "allow shadowing but warn" — the defn wins at every call site in its own file and the compiler warns once at the definition. See FIX.org, "Shadowing a builtin".** | | 8 | `unhandled Boom` | flan_rt.c:646 | `(defstruct Boom [why i32])` + `(error (Boom {.why 7}))`, `flan run` | D | C | D | B | Three words. No location (not even the `error` site, which the emitter knows), no field values, no list of the handlers that were in scope. The condition system is a headline feature and this is its failure mode. | | 9 | `the collection nosuch: is a directory named nosuch somewhere above /…/. , and there is none` | load.ml:120 | `(import zz "nosuch:thing")` | B | C | C | D | Reads as an assertion immediately contradicted. Also emits a bare `/.` on the path. Rewrite as a plain statement of the search ("no directory named `nosuch` between here and the root") and list what collections *were* found. | | 10 | `and`'s last operand gets the previous operand's caret | parse.ml `shortcircuit`, via check.ml:3632 | `(println (and true true (vec-new i32)))` | D | B | C | A | Already diagnosed in FIX.org:1036 with three rejected fixes; the accepted one — `check_if` preferring the arm that is not a compiler temp when choosing which to blame — is a check.ml change nobody owned. This pass owns check.ml. | diff --git a/lib/check.ml b/lib/check.ml index 329b970..e0f8fcc 100644 --- a/lib/check.ml +++ b/lib/check.ml @@ -4932,35 +4932,18 @@ and call_value ctx ~want loc (callee : Tast.expr) args = fail loc "this is a %s and not a function, so it cannot be called" (Types.to_string other) -(* A builtin's arity, and the one thing the caret cannot show: whether the - count being measured against is the builtin's or a defn of the same name. - A defn does not shadow a builtin — the dispatch above reaches every builtin - arm before it ever looks in [fns] — so a user function called [get] is - silently unreachable, and the refusal that followed measured the call - against the builtin while pointing at a call the reader had written for - their own. Said outright, with the definition alongside. *) -and arity ctx loc name n args = - if List.length args <> n then begin - let notes = - if Hashtbl.mem ctx.env.fns name then - match Hashtbl.find_opt ctx.env.fn_locs name with - | Some at -> - [ Loc.note at - (name ^ " is also defined here, and this call is not reaching \ - it — rename it to call it") ] - | None -> [] - else [] - in - let shadowed = notes <> [] in - if shadowed then - Loc.failk "check/builtin-arity" loc ~notes - "%s takes %d argument%s, given %d — this is the builtin %s, which a \ - defn of the same name does not replace" - name n (if n = 1 then "" else "s") (List.length args) name - else - fail loc "%s takes %d argument%s, given %d" name n - (if n = 1 then "" else "s") (List.length args) - end +(* A builtin's arity. The count is the builtin's and can only be the + builtin's: a defn of the same name written in the program now takes the + call over before any builtin arm is reached ([shadows_builtin] at the top + of [named_call]), so a call measured here is a call to the builtin and + there is no second signature for the reader to have meant. The note that + used to say otherwise — "this is the builtin get, which a defn of the same + name does not replace" — described a resolution order this compiler no + longer has. *) +and arity _ctx loc name n args = + if List.length args <> n then + fail loc "%s takes %d argument%s, given %d" name n + (if n = 1 then "" else "s") (List.length args) (* The operators that fold: [+ - * /], [min]/[max] and the three bitwise combining operators all take two operands or more, and mean the same thing @@ -5350,6 +5333,19 @@ and vec_at ctx loc (target : Tast.expr) (idx : Ast.expr list) = and named_call ctx ~want loc name args = let prim p ty args = expect ctx loc ~want (mk loc ty (Tast.Prim (p, args))) in match name with + (* The user's own definition, first — before every builtin arm below. + Clojure's rule: a [(defn get ...)] takes the name over, and a call + written in the program that defines it reaches that definition rather + than the builtin it is named after. The defn site is warned about once + (see [shadowed_builtins]); the call sites say nothing, because at a call + site there is nothing surprising left — the name means what the file + says it means. + + What this arm does NOT do is let one file's definition reach into + another's: [shadows_builtin] answers false for a call in the prelude and + for a call in imported package code, which is the same visibility rule a + defn has everywhere else. *) + | _ when shadows_builtin ctx loc name -> ordinary_call ctx ~want loc name args (* ── arithmetic and comparison ─────────────────────────────────── *) | "+" | "-" | "*" | "/" -> let p = match name with @@ -7163,13 +7159,22 @@ and named_call ctx ~want loc name args = | _ -> prim (Tast.Cast target) target [ a ]) (* ── ordinary calls ────────────────────────────────────────────── *) + | _ -> ordinary_call ctx ~want loc name args + +(* Everything that is not a builtin arm: a local of function type, a generic + signature, the function table, and the refusals for a name that is none of + them. Reached two ways — by falling past every arm above, and by the + shadowing guard at the very top of [named_call], which sends a call whose + name the program has defined straight here. One function so that both + routes resolve a name by exactly the same rules. *) +and ordinary_call ctx ~want loc name args = + match () with (* A local or a parameter holding a function value, called by the name it is bound to — which is what the body of [map] looks like. It is checked - before the global function table and after every builtin: a binding - shadows a defn of the same name (one namespace, ordinary lexical - scoping), and nothing shadows [+]. A local of any *other* type falls - through to the table, so a program that shadows a function name with an - i32 and then calls the function still means the function. *) + before the global function table: a binding shadows a defn of the same + name (one namespace, ordinary lexical scoping). A local of any *other* + type falls through to the table, so a program that shadows a function + name with an i32 and then calls the function still means the function. *) | _ when (match lookup ctx name with | Some b -> (match b.bty with Types.Fn _ -> true | _ -> false) | None -> false) -> @@ -7251,6 +7256,43 @@ and named_call ctx ~want loc name args = name name 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. + + 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 + 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. + + 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 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) + in + let visible () = + not (String.equal loc.Loc.file Prelude.file) + && not (String.contains ctx.owner '/') + in + List.mem name !builtin_names && defined () && visible () + (* ── A call to a generic function ─────────────────────────────────────── The whole of instantiation, and it is at the call site because the call site is the only place the concrete types exist. Odin does the same thing @@ -7809,6 +7851,38 @@ let builtins : (string * string * string) list = checked before this module is loaded. *) let () = builtin_names := List.map (fun (n, _, _) -> n) builtins +(* ── The one thing shadowing owes the reader ──────────────────────────── + A defn named after a builtin is legal and it wins ([shadows_builtin]), and + that is a large thing to have happened in silence: every [(get m k)] in + the file now means something the reader has to go and look at. So it is + said once, where the decision was made, and never again at the call sites + — a footgun notice, not a lint. + + It is a warning and it says so in the one way that matters: nothing raises + and the exit status does not move. Unlike [memory_sites] it is behind no + flag, because there is nothing to tune — a program either renamed a + builtin or it did not, and the line is one line and rare. + + The prelude is skipped: its defns are the language's own and a collision + there is a compiler bug rather than news for whoever is compiling. So are + qualified names, for the reason [shadows_builtin]'s [visible] gives — + [rl/get] is not [get] and shadows nothing. *) +let shadowed_builtins (decls : Ast.decl list) : Loc.diag list = + List.filter_map + (fun (d : Ast.decl) -> + match d.Ast.d with + | Ast.Defn fn + when List.mem fn.Ast.name !builtin_names + && not (String.contains fn.Ast.name '/') + && not (String.equal fn.Ast.nloc.Loc.file Prelude.file) -> + Some + (Loc.diag ~kind:"check/shadows-builtin" fn.Ast.nloc + (Printf.sprintf + "%s shadows the builtin %s — every call in this program now \ + reaches your definition" fn.Ast.name fn.Ast.name)) + | _ -> None) + decls + (* ── Declarations: pass 1, collect ─────────────────────────────────── *) (* Constant folding, only over integers and only for defconst — enough for an @@ -9414,6 +9488,19 @@ let build_program ~keep_going (decls : Ast.decl list) : Tast.program * env = be written anywhere in it, which is also what makes a reload rebuild every dispatch from the session's declarations — see lib/classes.ml. *) let decls = Classes.expand decls in + (* And with the declaration list in its final shape — the classes expanded, + the shims flattened, the imports already qualified by [Load] — the one + warning this compiler prints unasked. Here rather than in [bin/main.ml] + beside [print_memory_warnings] because every route into the compiler + passes through this function: build, check, run, and the dev daemon's + reload, which is where a defn is most likely to be written. Printed in + the shape [Loc] gives an error, so a checker in an editor parses it the + same way. *) + List.iter + (fun (d : Loc.diag) -> + prerr_endline + (Loc.entry ~mark:'~' ~label:"warning: " d.Loc.dloc d.Loc.dmsg)) + (shadowed_builtins decls); (* Pass one, and it stops at the first thing it refuses. That is not laziness: every name, type and signature in the file comes from here, so a declaration this pass could not make sense of leaves a hole that pass two diff --git a/test/dune b/test/dune index a3680cb..fe33b84 100644 --- a/test/dune +++ b/test/dune @@ -56,6 +56,10 @@ (glob_files programs/pkgs/mac/*) (glob_files programs/pkgs/macring/*) (glob_files programs/pkgs/macspin/*) + ; The package whose body calls the builtin get while the program importing + ; it defines a get of its own — shadow-builtin.flan, which is the pin that + ; a shadow stops at the file that declared it. + (glob_files programs/pkgs/shadowed/*) ; 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 @@ -215,7 +219,9 @@ ; And the macro-declaring packages, for the same reason. (glob_files programs/pkgs/mac/*) (glob_files programs/pkgs/macring/*) - (glob_files programs/pkgs/macspin/*)) + (glob_files programs/pkgs/macspin/*) + ; And the package shadow-builtin.flan imports. + (glob_files programs/pkgs/shadowed/*)) (action (run ./test_valgrind.exe))) ; The corpus a fourth time, through the hand-written x86-64 backend, compared @@ -272,7 +278,9 @@ (glob_files programs/pkgs/tree/*) (glob_files programs/pkgs/mac/*) (glob_files programs/pkgs/macring/*) - (glob_files programs/pkgs/macspin/*)) + (glob_files programs/pkgs/macspin/*) + ; And the package shadow-builtin.flan imports. + (glob_files programs/pkgs/shadowed/*)) (action (setenv SURVEY_STRICT 1 (setenv SURVEY_QUIET 1 @@ -435,7 +443,9 @@ (glob_files programs/pkgs/tree/*) (glob_files programs/pkgs/mac/*) (glob_files programs/pkgs/macring/*) - (glob_files programs/pkgs/macspin/*)) + (glob_files programs/pkgs/macspin/*) + ; And the package shadow-builtin.flan imports. + (glob_files programs/pkgs/shadowed/*)) (action (setenv SURVEY_STRICT 1 (setenv SURVEY_QUIET 1 diff --git a/test/programs/pkgs/shadowed/shadowed.flan b/test/programs/pkgs/shadowed/shadowed.flan new file mode 100644 index 0000000..fa3e16a --- /dev/null +++ b/test/programs/pkgs/shadowed/shadowed.flan @@ -0,0 +1,10 @@ +;;;; A package that calls the builtin get, imported by a program that defines +;;;; a get of its own. +;;;; +;;;; The importer's defn takes the name over in the importer's own file and +;;;; nowhere else: this file's names were qualified at the import (this +;;;; function is shadowed/field to everything downstream), so the get written +;;;; here is the builtin's, was compiled as the builtin's, and answers what a +;;;; dyn map holds under a keyword. + +(defn field [m] dyn (get m :b)) diff --git a/test/programs/shadow-builtin.flan b/test/programs/shadow-builtin.flan new file mode 100644 index 0000000..0ede1a4 --- /dev/null +++ b/test/programs/shadow-builtin.flan @@ -0,0 +1,24 @@ +;;;; A defn named after a builtin, and what the name means afterwards. +;;;; +;;;; "Allow shadowing but warn": this file's (defn get ...) is legal, it wins +;;;; 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: +;;;; +;;;; - (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. + +(import shadowed "pkgs/shadowed") + +(defstruct P [x i32]) + +(defn get [p P] i32 (.x p)) + +(defn main [] () + (println (get (P {.x 7}))) + (println (shadowed/field {:a 1 :b 4}))) diff --git a/test/test_acceptance.ml b/test/test_acceptance.ml index e982445..7c69c6d 100644 --- a/test/test_acceptance.ml +++ b/test/test_acceptance.ml @@ -2475,6 +2475,20 @@ let () = shape/Box and not area/shape/Box. *) 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 warning the definition earns 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"; (* 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 0790e2d..69e776c 100644 --- a/test/test_flan.ml +++ b/test/test_flan.ml @@ -4311,25 +4311,38 @@ let () = "(defn f [] i32 (let [x 1] (.r x)))" ~needle:"i32 is not a struct, so it has no fields"; - (* A defn whose name is a builtin's is silently unreachable — the dispatch - reaches every builtin arm before it looks in the function table — and the - arity refusal was measured against the builtin while pointing at a call - the reader had written for their own. *) - (match diag_of "(defstruct P [x i32])\n(defn get [p P] i32 (.x p))\n (defn f [] i32 (let [p (P {.x 1})] (get p)))" with - | Some d -> - check "a shadowed builtin's arity has a kind" - (d.Loc.kind = "check/builtin-arity"); - check "and says whose count it is" - (contains d.Loc.dmsg - "this is the builtin get, which a defn of the same name does not \ - replace"); - (match d.Loc.notes with - | [ n ] -> - check "and notes the definition that is not being reached" - (n.Loc.nloc.Loc.line = 2 - && contains n.Loc.nmsg "this call is not reaching it") - | _ -> check "a shadowed builtin has one note" false) - | None -> check "a shadowed builtin's call is refused" false); + (* "Allow shadowing but warn": a defn whose name is a builtin's is legal, + it wins at the call sites of the file that wrote it, and the compiler + says so once at the definition. + + This used to be the other way round — the dispatch reached every builtin + arm before it looked in the function table, so the defn was silently + unreachable and the arity refusal carried a note saying so. That note + described a resolution order this compiler no longer has, and the source + below, which used to be refused, is the one that proves it: (get p) is + one argument, and the builtin get takes two. *) + let shadow_src = + "(defstruct P [x i32])\n(defn get [p P] i32 (.x p))\n\ + (defn f [] i32 (let [p (P {.x 1})] (get p)))" + in + (match Check.shadowed_builtins (program shadow_src) with + | [ d ] -> + check "a defn named after a builtin is warned about, at the definition" + (d.Loc.kind = "check/shadows-builtin" + && d.Loc.dloc.Loc.line = 2 && d.Loc.dloc.Loc.col = 7); + check "and the warning says what the name now means" + (d.Loc.dmsg + = "get shadows the builtin get — every call in this program now \ + reaches your definition"); + check "and it carries no notes, being one sentence about one decision" + (d.Loc.notes = []) + | _ -> check "a shadowing defn is warned about exactly once" false); + check "and the call reaches the defn, at the defn's arity" + (match checked shadow_src with + | _ -> true + | exception Loc.Error _ -> false); + check "a program that shadows nothing is warned at not at all" + (Check.shadowed_builtins (program "(defn f [] i32 1)") = []); (* 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 @@ -4559,10 +4572,17 @@ let () = There is no way to reflect over an OCaml match, so this reads the source instead. The two regions are [named_call]'s arms and [var]'s, each from - its own [and] down to the first catch-all at the same indentation, and - the names are the string literals in the arm heads. It is a regex over - one file and costs nothing, which is why it is in the default run rather - than behind an alias. *) + its own [and] down to the catch-all at the same indentation, and the + names are the string literals in the arm heads. It is a regex over one + file and costs nothing, which is why it is in the default run rather + than behind an alias. + + The catch-all is [ | _ ->] and not [ | _], because a guarded arm is + not one: [named_call] opens with [| _ when shadows_builtin ...], which + is a name the program defined taking its own call over, and stopping + there would read the region as empty and report every builtin as + undescribed. Guarded arms in between are skipped by the same rule that + skips a comment — they carry no string literal in the head. *) let arm_names () = let src = In_channel.with_open_bin "../lib/check.ml" In_channel.input_all @@ -4591,7 +4611,7 @@ let () = let rec take = function | [] -> [] | l :: rest -> - if starts_with " | _" l then [] + if starts_with " | _ ->" l then [] else if starts_with " | \"" l then quoted l @ take rest else take rest in From a983a46ea5ae10a7190bcba89b9b15faabcfa6cb Mon Sep 17 00:00:00 2001 From: Joseph Ferano Date: Sun, 20 Sep 2026 19:51:52 +0700 Subject: [PATCH 2/2] 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