feat: replace numeric subtyping with explicit modes 🔢 - #210
Merged
Conversation
timfennis
commented
Aug 27, 2026
timfennis
commented
Aug 27, 2026
timfennis
commented
Aug 27, 2026
timfennis
commented
Aug 27, 2026
timfennis
force-pushed
the
feat/explicit-numeric-modes
branch
2 times, most recently
from
September 2, 2026 13:51
f84329c to
b8214eb
Compare
timfennis
force-pushed
the
feat/explicit-numeric-modes
branch
2 times, most recently
from
September 2, 2026 14:41
94652c8 to
1ed6ebc
Compare
timfennis
added a commit
that referenced
this pull request
Sep 3, 2026
## Context Noticed while rebasing `feat/explicit-numeric-modes` (#210) onto the as-cast work. That branch dropped a commit which, among other things, had fixed this comment — so the inaccuracy stays on master until someone corrects it on its own. ## Change `Object::static_type`'s doc comment claimed container element types are reported as `Any`, with `Tuple` as the sole exception. The body says otherwise: lists, deques, maps, and `Some` all derive element types from their contents via `lub`. Iterators and heaps are the variants that report `Any`. The `# Performance` note on `Value::static_type`, immediately above, already documents the O(n) element scan for `List`, `Map`, and `Deque` — so the two comments contradicted each other. Comment only, no behaviour change. 🤖 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Dispatch keeps master's no-scan policy, so a value the analyser only knows as `List<Any>` no longer reaches the typed `sum` overloads. An `as` cast is the supported spelling: it scans once, at a site the author wrote. `016_casts/024` pins that a cast selects a *typed* container overload — `002_list_downcast_from_any` only reaches `sorted(Sequence<Any>)`, so nothing covered that. It includes the empty case, since `[].sum()` cannot be written without a cast. `007_numeric_hierarchy` was written to change here — with Int, Float, and Number siblings, `1 as Number` can never hold, so it moves to an error test and the file keeps only the casts that recover a numeric type from Any. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The troubleshooting page claimed every standard library container overload uses `<Any>` elements, so the element-type limitation was invisible. The typed `sum` overloads make it visible, and a cast is the way through it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Introduce a borrowed NumericRef and operational NumericMode without changing the public sibling types or Value representation. Reuse them for promotion, comparison, hashing, constructors, and conversions, while keeping primitive fast paths and exact overload validation. Borrow Number payloads in generated native adapters so reference parameters no longer clone AdvancedNumber values.
Pick the evaluator per overload at registration instead of rediscovering the output mode on every call, and give each integer operation its own precondition so add, subtract and multiply test nothing but overflow. Collapse the fixed-signature natives onto one declare_typed! macro so a type is named once instead of three times, and report operand mismatches from a single cold path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
timfennis
force-pushed
the
feat/explicit-numeric-modes
branch
from
September 3, 2026 13:42
1f4ceb5 to
888b9a0
Compare
timfennis
marked this pull request as ready for review
September 3, 2026 13:42
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 888b9a0cb1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`checked_rem` and `checked_rem_euclid` report the overflow of the quotient they imply, so `i64::MIN % -1` failed even though the remainder is 0. The divisor is already screened for zero, which leaves no way for a remainder to overflow, so wrap instead. Division keeps reporting the real overflow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`cmp_to_zero` enumerated the exact Number variants and let the rest fall through to a rejection, so a comparator returning a float-backed Number failed with "must return a number, got Number". Handle the float with the same NaN check the primitive branch uses. The match is now exhaustive, so a new variant is a compile error rather than a silent rejection, and the complex arm says which half of "number" is missing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Widening the declared type of `&BigRational` and `Complex64` parameters to `StaticType::Number` left their runtime extraction matching one variant, so such a native advertised every Number and then rejected most of them. Route both through the existing conversions instead. Nothing declares these parameters today, so this is a trap disarmed rather than a bug fixed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Number overloads converted through an i64 conversion that accepted only exact BigInt values, so `randi(0, 10.0n)` and `randi(0, 20n/2n)` failed at runtime with "cannot convert Number to int". Dropping them moves the rejection to resolve time and matches randi already having no Float overloads. That conversion now has no callers, so it goes too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d29e7f665
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Four changes to AdvancedNumber, all reachable from ordinary programs: `0 ^ negative` only guarded the integer base, so a zero rational reached num-rational directly and panicked the process. One check before the match covers every exact operand pair and lets int_pow drop its own copy. A non-finite exponent has a NaN `fract()`, which compares unequal to zero and sent a negative base into the complex plane: `(-2n) ^ Inf` answered a complex NaN where the float path answers Inf. Found by the Codex review. Division and the remainders build their result as a fraction, and a denominator of one never collapsed, so `4n / 2n` stayed a Rational that printed as `2` but no longer matched the Int arm serde switches on. The constructor now normalizes, which fixes every caller at once. Hashing a value no i64 could hold built an exact BigRational, so float map keys allocated twice per insert. Values an f64 represents exactly now hash from their bits; only the rest reach the canonical form. 200k float keys go from 176ms to 48ms. Equal values still share a bucket in every mode, which the new test pins down across floats, dyadic rationals and big integers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`checked_shl` only rejects a shift amount of 64 or more, never a result that does not fit, so `1 << 63` wrapped to a negative Int in silence while every other Int operation reported its overflow — and the manual already claimed shifts were checked. Verify the shift is reversible instead. Three other repairs in the same file: The error helpers formatted operands with the unbounded `static_type()`, so comparing a self-referential list aborted the process with a stack overflow while building the message. They use the bounded `diagnostic_type()` the same PR introduced for this hazard. `convert_to_int` described its argument before the numeric fast path that discards the description, walking whole containers to do it. It now only describes on the error paths. Restores the per-function documentation on ceil, floor, round, abs, signum, real, imag, numerator, denominator, gcd and lcm, which the rewrite had replaced with one shared placeholder per group; the same strings feed LSP hover. Registration of the unary helpers is reversed to match register_binary_arithmetic, so a dynamically dispatched abs(1) tests the Int candidate first rather than last. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Publishes `diagnostic_type()` so ndc_stdlib can reach the bounded description its error helpers now use; it was pub(crate), which left the unbounded `static_type()` as the only option outside this crate. Every non-Int range bound claimed the integer was too large. Now that `n` literals are the only way to write a big integer, a small Number bound is the common case, and the message sent you looking for an overflow that was not there. Only an integer that genuinely does not fit reports a size problem. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`float_to_i64` allocated a BigInt to range-test a finite f64. Truncating first makes it exactly the conversion `exact_f64_to_i64` already performs with two comparisons, so it delegates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`5n % 0n` is NaN where `5 % 0` is an error. The page covered division but left the remainders implicit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a6a1afbc5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ion 🔢 Collapsing a denominator of one inside `rational()` only helps callers that go through it, and the power branches built the variant directly: `1n ^ -1n` and `(1n/2n) ^ -1n` are whole numbers that stayed wrapped as rationals, so json_encode refused them. Reported by the Codex review. The `From<BigRational>` conversion had the same hole and is the wider one, since every `.into()` on a computed fraction went through it. Both now route to the normalizing constructor, as does `abs`, which cannot produce a whole value today but would be the next place to leak one. Documents the invariant on the variant itself: a `Rational` never holds a whole number, and `rational()` is how you get one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46b2e3ee9d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Third report of one bug shape: `AdvancedNumber::Rational` must never hold a
whole number, and every construction site that forgot leaked a value that is
an integer in every sense except the variant holding it. Normalizing the
constructor fixed division and the remainders, routing the power branches
fixed those, and Codex then found the macro's return path. A doc comment was
not going to hold the line.
The payload is now an `ExactFraction` whose field is private to this module,
so `AdvancedNumber::rational` is the only way to build the variant and the
rule holds by construction. Both routes are compile errors from outside:
AdvancedNumber::Rational(Box::new(r)) // mismatched types
AdvancedNumber::Rational(ExactFraction(Box::new(r)))
// private field
Reading is unchanged — it derefs to the fraction, and Unbox/AsRef/Display
bridge the sites that took a Box before, so the 35 match sites stay as they
were. The macro's `BigRational` return arm is fixed here rather than by the
compiler, because nothing returns one yet and the arm never expands; anyone
who adds such a native now gets an error instead of a silent leak.
Two tests built a whole fraction on purpose to check it hashed like an
integer. That state no longer exists, so they assert the guarantee that
replaced it: the constructor hands back `Int`, and a genuine fraction is
left alone.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Int,Float, andNumbersibling public typesNumber, backed by internal bigint, rational, float, or complex representationsRationalandComplextypes without compatibility shimsnliterals, plus migration diagnostics for oversized primitive integersCloses #191
Semantics to review
Intuses checkedi64arithmetic;/truncates,\floors,%uses truncating remainder, and%%uses Euclidean remainder.Floatkeeps IEEE 754 behavior.Numberkeeps exact arithmetic where possible and permits internal bigint, rational, float, and complex results.Int-only.Numbersupport complex continuation.Out of scope
Specialized numeric bytecode from #110 remains separate from this semantic cutover.
Verification
cargo fmt --allcargo test --workspacecargo build --no-default-featurescargo clippy -p ndc_core -p ndc_lexer -p ndc_vm -p ndc_stdlib --all-targetsgit diff --checkAI disclosure
OpenAI Codex using GPT-5 assisted across the complete patch, including the runtime and type-system refactor, lexer and standard-library changes, tests, and documentation. The resulting implementation was validated with the commands listed above.