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