WIP: feat: migrate builtins to extension functions and add rounding (Phase 2)

claireb commented

2026-03-04 05:01:01 +00:00

(Migrated from codeberg.org)

Unify all arithmetic operators under the extension function mechanism
introduced in Phase 1. The ArithExpr enum simplifies from 6 variants
(Lit, Var, NaryOp, BinOp, UnaryOp, Call) to 3 (Lit, Var, Call), with
all 10 built-in operators (+, -, *, /, min, max, div, rem, **, abs)
dispatched through FunctionRegistry as Call nodes.

Key changes:

Add builtins.rs with ExtensionFunction impls for all 10 operators
Auto-inject prelude registry in pipeline prepare()
Switch parser to emit Call nodes for all arithmetic operators
Remove NaryArithOp, BinArithOp, UnaryArithOp enums and eval_nary/eval_bin
Add EvalError::ArithError variant to preserve error semantics
Add round (banker's rounding), floor, ceil as new builtin functions
Update all tests (~1900 passing, 0 failures)

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com

Unify all arithmetic operators under the extension function mechanism introduced in Phase 1. The ArithExpr enum simplifies from 6 variants (Lit, Var, NaryOp, BinOp, UnaryOp, Call) to 3 (Lit, Var, Call), with all 10 built-in operators (+, -, *, /, min, max, div, rem, **, abs) dispatched through FunctionRegistry as Call nodes. Key changes: - Add builtins.rs with ExtensionFunction impls for all 10 operators - Auto-inject prelude registry in pipeline prepare() - Switch parser to emit Call nodes for all arithmetic operators - Remove NaryArithOp, BinArithOp, UnaryArithOp enums and eval_nary/eval_bin - Add EvalError::ArithError variant to preserve error semantics - Add round (banker's rounding), floor, ceil as new builtin functions - Update all tests (~1900 passing, 0 failures) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

hugooconnor commented

2026-03-04 05:59:08 +00:00

(Migrated from codeberg.org)

nice one!

just a small issue;

[P2] Add runtime arity guard before extension/builtin dispatch — arith.rs (the ArithExpr::Call eval arm)

func.eval(&term_args) is called without checking func.signature().arity.accepts(term_args.len()). This is safe when the full pipeline runs (the Validate stage catches it), but public entry points like
eval_with_context and ground_theory_with_limit can reach this path directly. With builtins-as-extensions, the blast radius widens since all arithmetic ops now route through Call.

Extension authors naturally index into args (e.g. args[0]), so a wrong arg count will panic instead of returning an ArithError.

Fix: check arity after evaluating args, before func.eval(), and add an ArithError::ArityMismatch { name, expected, got } variant.

nice one! just a small issue; [P2] Add runtime arity guard before extension/builtin dispatch — arith.rs (the ArithExpr::Call eval arm) func.eval(&term_args) is called without checking func.signature().arity.accepts(term_args.len()). This is safe when the full pipeline runs (the Validate stage catches it), but public entry points like eval_with_context and ground_theory_with_limit can reach this path directly. With builtins-as-extensions, the blast radius widens since all arithmetic ops now route through Call. Extension authors naturally index into args (e.g. args[0]), so a wrong arg count will panic instead of returning an ArithError. Fix: check arity after evaluating args, before func.eval(), and add an ArithError::ArityMismatch { name, expected, got } variant.

claireb commented

2026-03-04 06:12:38 +00:00

(Migrated from codeberg.org)

@hugooconnor wrote in https://codeberg.org/anuna/spindle-rust/pulls/27#issuecomment-11181143:

nice one!

just a small issue;

[P2] Add runtime arity guard before extension/builtin dispatch — arith.rs (the ArithExpr::Call eval arm)

func.eval(&term_args) is called without checking func.signature().arity.accepts(term_args.len()). This is safe when the full pipeline runs (the Validate stage catches it), but public entry points like eval_with_context and ground_theory_with_limit can reach this path directly. With builtins-as-extensions, the blast radius widens since all arithmetic ops now route through Call.

Extension authors naturally index into args (e.g. args[0]), so a wrong arg count will panic instead of returning an ArithError.

Fix: check arity after evaluating args, before func.eval(), and add an ArithError::ArityMismatch { name, expected, got } variant.

Good catch, thanks! Addressed, and I've added notes to my planning documents about being very careful when a refactor relaxes constraints from being structural to being convention.

@hugooconnor wrote in https://codeberg.org/anuna/spindle-rust/pulls/27#issuecomment-11181143: > nice one! > > just a small issue; > > [P2] Add runtime arity guard before extension/builtin dispatch — arith.rs (the ArithExpr::Call eval arm) > > func.eval(&term_args) is called without checking func.signature().arity.accepts(term_args.len()). This is safe when the full pipeline runs (the Validate stage catches it), but public entry points like eval_with_context and ground_theory_with_limit can reach this path directly. With builtins-as-extensions, the blast radius widens since all arithmetic ops now route through Call. > > Extension authors naturally index into args (e.g. args[0]), so a wrong arg count will panic instead of returning an ArithError. > > Fix: check arity after evaluating args, before func.eval(), and add an ArithError::ArityMismatch { name, expected, got } variant. Good catch, thanks! Addressed, and I've added notes to my planning documents about being very careful when a refactor relaxes constraints from being structural to being convention.

This pull request is marked as a work in progress.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin feat/builtins-as-extensions:feat/builtins-as-extensions

git switch feat/builtins-as-extensions

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.