diff --git a/FIX.org b/FIX.org index 9cdda86..b0d73ca 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,174 @@ 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 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 +: 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: 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 +...)~ 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. + +*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_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"), + 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. 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/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..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 @@ -4932,35 +4941,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 +5342,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 +7168,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 +7265,64 @@ 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? + 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. 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 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, 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 = + (* 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 + (* 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 + 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 site is the only place the concrete types exist. Odin does the same thing @@ -7807,7 +7879,41 @@ 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 + 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 Hashtbl.mem builtin_set fn.Ast.name + && 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 ─────────────────────────────────── *) @@ -9414,6 +9520,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..1e67901 --- /dev/null +++ b/test/programs/pkgs/shadowed/shadowed.flan @@ -0,0 +1,18 @@ +;;;; 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)) + +;;; 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 new file mode 100644 index 0000000..9f9e316 --- /dev/null +++ b/test/programs/shadow-builtin.flan @@ -0,0 +1,45 @@ +;;;; 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. +;;;; +;;;; Five lines printed, and between them the whole rule. In order: +;;;; +;;;; - 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. +;;;; - 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") + +(defstruct P [x i32]) + +(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 (+ 1 2)) + (println (len "abcd")) + (println (shadowed/stored-size))) diff --git a/test/test_acceptance.ml b/test/test_acceptance.ml index e982445..36d7393 100644 --- a/test/test_acceptance.ml +++ b/test/test_acceptance.ml @@ -2475,6 +2475,26 @@ 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 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 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\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 0790e2d..8bb569a 100644 --- a/test/test_flan.ml +++ b/test/test_flan.ml @@ -4311,25 +4311,78 @@ 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)") = []); + (* 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 @@ -4559,10 +4612,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 +4651,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