Skip to content

reject builtin-named user functions at parse time#245

Merged
danieljohnmorris merged 3 commits into
mainfrom
fix/lst-shadow-verify
May 13, 2026
Merged

reject builtin-named user functions at parse time#245
danieljohnmorris merged 3 commits into
mainfrom
fix/lst-shadow-verify

Conversation

@danieljohnmorris
Copy link
Copy Markdown
Collaborator

Summary

Defining a user function whose name collides with a builtin (lst, sum, clamp, avg, …) used to fail with a misleading ILO-T006 arity error from the builtin's signature. Personas had no signal their name was already taken and debugged the arity error down the wrong path.

This extends the existing ILO-P011 reserved-name precedent (parse_stmt already does this for fld=5/cnt=/brk= bindings) into parse_fn_decl. One-location fix in the parser, so tree/VM/Cranelift all surface the same friendly diagnostic before any engine dispatch runs.

Manifesto framing: replaces a silent shadow + misleading error with an explicit named, fixable error the first time round. Less retry tax, less debugging into the wrong codepath.

Repro

Before:

$ ilo 'lst>n;42
main>n;lst()' main
{"code":"ILO-T006","message":"arity mismatch: 'lst' expects 3 args, got 0", ...}

After:

$ ilo 'lst>n;42
main>n;lst()' main
{"code":"ILO-P011","message":"`lst` is a builtin and cannot be used as a function name",
 "suggestion":"rename to something like `mylst` or `lstof` — builtins shadow user functions in calls, so reusing the name silently breaks dispatch"}

Renaming to anything non-builtin (e.g. show) compiles and runs cleanly.

What's in the diff

  • f51735e parser: check Builtin::is_builtin(&name) right after expect_ident() in parse_fn_decl, emit ILO-P011 with a rename hint. Also updates two graph.rs unit tests that had used inv and sum as fixture function names (both are builtins); renamed to not and total.
  • 4e8fe44 tests: new tests/regression_builtin_fn_name.rs covering ten builtin names (lst, hd, tl, slc, cat, has, len, str, num, map) across tree/VM/Cranelift, plus the original repro asserting ILO-P011 precedes any downstream ILO-T006 cascade, plus a rename-still-works sanity test, plus a lst builtin call sanity test. New examples/builtin-fn-name-rename.ilo gives agents an in-context demo of the correct pattern.
  • 1a21e67 docs: renamed three stock examples that previously taught the wrong pattern (examples/arithmetic.ilo avg→mean3, examples/builtins.ilo and examples/guards.ilo clamp→clmp), and the same shape in SPEC.md's multi-function-files section. ai.txt regenerates from SPEC.md.

Test plan

  • cargo test --release --features cranelift full suite green (2866 lib + all integration)
  • cargo fmt --check clean
  • cargo clippy --release --features cranelift --all-targets -- -D warnings clean
  • New regression suite covers tree/VM/Cranelift for 10 builtin names + the original lst() repro
  • New example file examples/builtin-fn-name-rename.ilo runs to 42 on every engine via tests/examples_engines.rs
  • All updated stock examples still pass tests/examples.rs
  • fld=5 reserved-name precedent (tests/regression_fld_reserved.rs) still green

Follow-ups

None tracked. The rename hint is intentionally generic (my{name} / {name}of) to cover any builtin without a hand-curated dictionary; could be made name-specific later if persona feedback shows the generic form isn't sticky enough.

When a user defines a function whose name collides with a builtin (lst, sum, clamp, etc), verify's call-dispatch resolves the builtin first and reports its signature, producing a misleading ILO-T006 arity error against a function the agent thought they had just defined. Repro:

  lst>n;42
  main>n;lst()
  # ILO-T006: arity mismatch: 'lst' expects 3 args, got 0

The agent has no signal that 'lst' is already taken; debugging the arity error sends them down the wrong path.

Extend the existing ILO-P011 reserved-name precedent (parse_stmt already does this for fld=5/cnt=/brk= bindings) into parse_fn_decl: if the function name is a builtin, error with a clear rename hint. One-location fix, runs before any engine dispatch, so tree/VM/Cranelift all surface the same friendly diagnostic.

Updates two graph.rs unit tests that had used 'inv' and 'sum' as fixture function names (both are builtins); renamed to 'not' and 'total'.
Covers ten builtin names personas reach for (lst, hd, tl, slc, cat, has, len, str, num, map), across tree/VM/Cranelift, asserting ILO-P011 fires with a clear rename hint. Also pins:

- the original repro (lst as fn name + main calling it) and asserts ILO-P011 precedes any downstream ILO-T006 cascade so the agent's first action is the rename
- the rename workaround actually compiles on every engine
- the lst builtin still works as a 3-arg list-constructor after the parser change

examples/builtin-fn-name-rename.ilo gives agents an in-context demonstration of the now-correct pattern and acts as a higher-level regression test the examples_engines harness exercises across every engine.
The new ILO-P011 reserved-name check catches three pre-existing examples that defined user functions over builtin names:

- examples/arithmetic.ilo: avg -> mean3 (avg is a list-mean builtin)
- examples/builtins.ilo:   clamp -> clmp (clamp is a builtin)
- examples/guards.ilo:     clamp -> clmp

SPEC.md's multi-function-files section had the same clamp shape; renamed to clmp and noted the reason inline. ai.txt regenerates from SPEC.md so it picks the change up too.

No behaviour change to the examples; the new names work identically and the comments now teach the right pattern (don't reuse builtin names for user functions).
@codecov
Copy link
Copy Markdown

codecov Bot commented May 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@danieljohnmorris danieljohnmorris merged commit 4d120ce into main May 13, 2026
5 checks passed
@danieljohnmorris danieljohnmorris deleted the fix/lst-shadow-verify branch May 13, 2026 17:11
danieljohnmorris added a commit that referenced this pull request May 13, 2026
The frq.ilo example (added by PR #238) had a `chars>n;...` helper
function. Introducing the `chars` builtin in this PR triggers the
ILO-P011 builtin-shadow check from PR #245 and the existing example
stops parsing. Renaming the helper to `cfrq` (char-freq) preserves
the example's intent without weakening the new shadow check.
danieljohnmorris added a commit that referenced this pull request May 16, 2026
A local binding like `flat=cat ls " "` was silently accepted, then any
later use of `flat` in operand position resolved to the 1-arg flatten
builtin (the verifier checks is_builtin before locals), surfacing as a
misleading `ILO-T006 'flat' expects 1 args, got 0`. The agent has no
signal that the local binding is being shadowed.

Mirrors the PR #245 precedent for builtin-named user functions: intercept
at parse time with ILO-P011 and a rename hint (`myflat`, `flatv`) in
both parse_decl (top-level) and parse_stmt (in-function) binding sites.
Existing `fld=` per-name block keeps its specific fold-builtin message
since it runs before the generic check.

Cross-engine regression test covers twelve builtin names across tree, VM,
and Cranelift, plus the exact pdf-analyst rerun3 repro and a sanity check
that the renamed binding compiles and runs. examples/builtin-binding-name-rename.ilo
demonstrates the correct shape (`body=cat ls " "`) for the
`spl body ". "` pipeline.
danieljohnmorris added a commit that referenced this pull request May 16, 2026
A local binding like `flat=cat ls " "` was silently accepted, then any
later use of `flat` in operand position resolved to the 1-arg flatten
builtin (the verifier checks is_builtin before locals), surfacing as a
misleading `ILO-T006 'flat' expects 1 args, got 0`. The agent has no
signal that the local binding is being shadowed.

Mirrors the PR #245 precedent for builtin-named user functions: intercept
at parse time with ILO-P011 and a rename hint (`myflat`, `flatv`) in
both parse_decl (top-level) and parse_stmt (in-function) binding sites.
Existing `fld=` per-name block keeps its specific fold-builtin message
since it runs before the generic check.

Cross-engine regression test covers twelve builtin names across tree, VM,
and Cranelift, plus the exact pdf-analyst rerun3 repro and a sanity check
that the renamed binding compiles and runs. examples/builtin-binding-name-rename.ilo
demonstrates the correct shape (`body=cat ls " "`) for the
`spl body ". "` pipeline.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant