df6e830bec
The issue-0076 reserved-type-name binding diagnostic only ran over main-file decls, so an imported module (or the stdlib) could still declare `s2 := ...` and reach lowering, where the address-of family loads the whole aggregate and passes it by value to a `ptr` param — LLVM verifier abort. Extend coverage to every compiled module: a dedicated `checkBindingNames` walk (in semantic_diagnostics.zig) visits every var/`:=`/typed-local binding name and function/lambda/struct-method parameter at any depth, with NO main-file filter, descending the `namespace_decl` that a `mod :: #import` wraps so imported-module decls are reached. It tracks each module's source_file (save/restore per node) so the diagnostic renders against the imported module's text. Rejection still defers to the parser's `Type.fromName` classifier; the unknown-type check (0064) stays main-file-only. No lowering special-case; `.identifier`-only address-of paths are unchanged. Stdlib audit: the only reserved-name bindings under library/ were two `u1` locals in ui/renderer.sx (UV coords) — renamed to u_min/u_max/v_min/v_max. Regression test: examples/1120-diagnostics-imported-reserved-type-name.sx (+ companion mod.sx) — an imported `s2 := ...` now emits the clean diagnostic at the import's declaration site (exit 1), not an LLVM abort. Resolves issues 0076 (coverage extension) and 0077.
136 lines
7.2 KiB
Markdown
136 lines
7.2 KiB
Markdown
# 0076 — builtin/reserved type name wrongly accepted as an identifier
|
||
|
||
> **Status: RESOLVED.**
|
||
>
|
||
> **Root cause:** the language accepted a value binding (local/global `var` or a
|
||
> parameter) spelled as a reserved/builtin type name. The parser turns such a
|
||
> spelling into a `.type_expr` rather than an `.identifier` (`parser.zig`, via
|
||
> `Type.fromName`), so the address-of family in `src/ir/lower.zig` never saw a
|
||
> scoped local and fell through to value lowering — loading the whole aggregate
|
||
> and passing it by value to a `ptr` parameter (LLVM verifier abort, or a silent
|
||
> `*self`-mutation-losing copy).
|
||
>
|
||
> **Fix:** a declaration-site diagnostic in the existing semantic pass
|
||
> `src/ir/semantic_diagnostics.zig` (`UnknownTypeChecker`). New
|
||
> `checkBindingName` rejects any parameter name or `var` binding name (local or
|
||
> global, `:=` / typed-local forms) whose spelling collides with a reserved type
|
||
> name; `isReservedTypeName` defers to the parser's own classifier
|
||
> (`types.Type.fromName`) so the rejected set never drifts from the set that
|
||
> would parse as a type — the named builtins (`bool`, `string`, `void`, `f32`,
|
||
> `f64`, `usize`, `isize`, `Any`) and `[su]N` over sx's 1–64 range. Bare value
|
||
> names (`s`, `self`, `index`) are untouched. No lowering special-case is added;
|
||
> the `.identifier`-only address-of paths are correct once type-shaped names can
|
||
> never be bound. The rejected `bareVarName` approach was never landed.
|
||
>
|
||
> **Regression tests:**
|
||
> - `examples/0125-types-type-named-var-rejected.sx` — `:=` form (`s2`) rejected.
|
||
> - `examples/1119-diagnostics-reserved-type-name-as-identifier.sx` — parameter
|
||
> (`u8`), typed-local (`s64`, `bool`), and `:=` (`string`) forms rejected.
|
||
> - `examples/0135-types-self-streaming-nonreserved.sx` — positive: `*self`
|
||
> streaming with non-reserved names (`hasher`, `ctx`) accumulates correctly via
|
||
> both `update(@h, …)` and `h.update(…)`.
|
||
>
|
||
> Pre-existing example `examples/0904-...` declared locals `s1`/`s2` (incidental
|
||
> names); renamed to `filled`/`empty`.
|
||
>
|
||
> **Coverage extension (issue 0077).** The first landing scoped the binding
|
||
> check to main-file decls (matching the unknown-type check's trusted-imports
|
||
> convention); an imported module could still declare `s2 := …` and hit the
|
||
> original LLVM verifier abort. The reserved-name binding diagnostic now runs
|
||
> over EVERY compiled module — imported user modules (descending the
|
||
> `namespace_decl` an `mod :: #import` wraps) AND the stdlib `library/` — and
|
||
> the two `u1` locals in `library/modules/ui/renderer.sx` were renamed
|
||
> accordingly. The unknown-type check (issue 0064) stays main-file-only. See
|
||
> issue 0077 for the imported-module facet and its pinned regression test
|
||
> `examples/1120-diagnostics-imported-reserved-type-name.sx`.
|
||
|
||
## Symptom (how it first surfaced)
|
||
|
||
A local variable whose name is lexically a type — e.g. `s2` (the `sN`
|
||
arbitrary-width signed-int syntax: `Type.fromName("s2")` → `s(2)`), or `u8`,
|
||
`s64`, etc. — is accepted as a variable. Because such a name parses as a
|
||
`.type_expr` (not `.identifier`), the address-of family of lowering sites
|
||
(`@s2`, the autoref `s2.update(...)` receiver, a bare `f(s2)` at a `*T` param,
|
||
global function-pointer args) does NOT recognize it as a scoped local and falls
|
||
through to value lowering — loading the whole aggregate and passing it **by
|
||
value** to a `ptr` parameter:
|
||
|
||
```
|
||
LLVM verification failed: Call parameter type does not match function signature!
|
||
call void @update(ptr @__sx_default_context,
|
||
{ [8 x i64], [64 x i8], i64, i64 } %load, ...)
|
||
```
|
||
|
||
For some struct shapes it compiles but silently passes a **copy** (callee
|
||
`*self` mutations lost). A non-type-shaped name (`hasher`, `ctx`) never triggers
|
||
any of this — the `.identifier` paths already work correctly.
|
||
|
||
## Root cause
|
||
|
||
The language is **accepting reserved/builtin type names as identifiers** in the
|
||
first place. `sN`/`uN` (arbitrary-width ints) and the named builtins
|
||
(`bool`, `string`, `void`, `f32`, `f64`, `s8`/`s16`/`s32`/`s64`,
|
||
`u8`/`u16`/`u32`/`u64`, …) are reserved type names; declaring a variable with
|
||
such a name is meaningless and produces the mis-lowering above. Patching each
|
||
address-of site to tolerate the name (the rejected `bareVarName` approach) is
|
||
whack-a-mole — there is always another site, and it entrenches a name that
|
||
should never have been allowed.
|
||
|
||
## Proper fix (the required direction)
|
||
|
||
Emit a **diagnostic error** when an identifier is declared with a name that
|
||
collides with a **builtin/reserved type name** — including the arbitrary-width
|
||
`[su][0-9]+` (`sN`/`uN`) family AND the named builtins (`bool`, `string`,
|
||
`void`, `f32`, `f64`, the fixed-width int types, etc.). Scope ruling (Agra):
|
||
**all builtin/reserved type names** are rejected as identifiers. (User-defined
|
||
struct/type-name shadowing, if intentionally supported elsewhere, is out of
|
||
scope for this issue — this is specifically about builtin/reserved type names.)
|
||
|
||
Diagnostic at the declaration site, e.g.:
|
||
`error: 'u8' is a reserved type name and cannot be used as an identifier`
|
||
with the declaration's span.
|
||
|
||
Suspected area: name binding / declaration handling — where a `:=` / typed
|
||
local / parameter name is introduced. Reject the name there, before it ever
|
||
reaches lowering. Do NOT add lowering special-cases for type-shaped names; the
|
||
`.identifier`-only checks at the address-of sites are then correct as-is (no
|
||
type-shaped name can reach them).
|
||
|
||
## Reproduction
|
||
|
||
```sx
|
||
#import "modules/std.sx";
|
||
Sha256 :: struct { h:[8]u64; block:[64]u8; block_len:s64=0; total_len:u64=0; }
|
||
init :: () -> Sha256 { s:Sha256=---; s.block_len=0; s.total_len=0; s }
|
||
update :: (self:*Sha256, data:string) { self.total_len += data.len; }
|
||
main :: () -> s32 { s2 := init(); update(@s2, "."); print("total_len={}\n", s2.total_len); return 0; }
|
||
```
|
||
|
||
`./zig-out/bin/sx run <file>` today → LLVM verifier abort.
|
||
**Expected after fix:** a clean compile-time diagnostic that `s2` is a reserved
|
||
type name and cannot be an identifier (exit non-zero, readable error — NOT an
|
||
LLVM abort, NOT a silent copy). The same program with a non-reserved name
|
||
(`hasher := init(); update(@hasher, ".")`) must compile and print `total_len=1`.
|
||
|
||
## Verification
|
||
|
||
1. Pinned diagnostics test(s) asserting the error for representative reserved
|
||
names used as identifiers: `s2`, `u8`, `s64`, `bool`, `string` (declaration
|
||
forms: `:=`, typed local, and a parameter name). Capture the diagnostic text
|
||
in `expected/`.
|
||
2. A positive test: the same `*self` streaming pattern with NON-reserved names
|
||
(`hasher`, `ctx`) compiles and accumulates state correctly via both
|
||
`update(@h, ...)` and `h.update(...)` — proving the `.identifier` paths are
|
||
correct and no lowering special-case is needed.
|
||
3. `zig build && zig build test && bash tests/run_examples.sh` all green. If any
|
||
existing example/test declares a variable with a reserved type name, it is now
|
||
illegal — fix the test's variable name (do NOT weaken the diagnostic). Report
|
||
how many such sites existed.
|
||
|
||
## Provenance
|
||
|
||
Discovered by the `distribution` flow (P1.2 pure-sx SHA-256), whose minimal repro
|
||
happened to name a local `s2`. Real SHA-256 code with names like `hasher`/`ctx`
|
||
is unaffected on the current compiler — so the P1.2 "blocker" was a
|
||
naming artifact, and this issue is really a missing-diagnostic correctness bug.
|