Make the evaluation core reliable (Stage 1) - #30
Merged
Conversation
Documents the four defects reproduced against the 0.2.0 binary — factorial rejecting integral decimals, Number violating the PartialEq/PartialOrd contract, factorial and power never terminating on large operands, and function arity never being validated — together with the four components that close them and the order they land in. Declares what is deliberately left out: undefined variables keep evaluating to 0, and prefix factorial stays accepted until the grammar pass in stage 2. Declares one behaviour change beyond the defects: parentheses become mandatory after a function name, which argument counting requires.
A bit budget bounds memory directly and running time only indirectly: a factorial is a loop of n bignum multiplications, so a limit calibrated on memory alone can still admit a multi-second computation. The default must be lowered until the slowest expression it accepts stays well under a second, and that timing recorded.
resolve_decimal! asserts the enum variant through matches!, across roughly eighty call sites, so every integral result flips to NaturalNumber and the assertion fails. The repair is one line: the macro drops the variant check, which was incidental, and the invariant gains two tests that state it outright.
Four tasks in the order the spec fixes: extract function evaluation, give every numeric value one representation, refuse oversized results, validate function arity. Each task is test-first and ends at a commit.
resolve() was 243 lines and clippy flagged it. The MathFunction dispatch and the numeric conversions it needs now live in src/functions.rs, leaving the shunting-yard translation and the evaluation loop in rpn_resolver. Pure move: no behaviour change, no test edited, same 64 tests green.
Task 1 required both that resolve() end up around 140 lines and that clippy's too-many-lines warning disappear. Those contradict: the threshold is 100, and splitting a 243-line function into 141 plus 102 leaves both over it. The criterion is now that the warning total must not grow, checked per category, since a stable total hid one regression offsetting one improvement when the split landed.
NaturalNumber(2) and DecimalNumber(2/1) meant the same number, and code branched on the tag rather than the value. Two defects followed: factorial rejected abs(-3), floor(2.5) and max(3,2), because those functions forced the decimal tag onto integral results; and Number violated the PartialOrd contract, with 2 == 2/1 false while 2 >= 2/1 was true. Number::decimal is now the only way to build a decimal and degrades an integral rational to NaturalNumber, Number::as_integer replaces the two copies of the is-this-a-whole-number test, and PartialEq compares values.
999999999! and 10^100000000 never returned: the only guard on the factorial was that its operand fit in a u64, and none at all on exponentiation. Both are now predicted before being computed - Stirling for the factorial, base size times exponent for the power - and the four arithmetic operators check the result they produced, so growth through repeated multiplication is caught too. The budget is one knob, Limits::max_value_bits, on by default and configurable through Session::with_limits.
predicted_factorial_bits used only the two leading Stirling terms, which underestimate log2(n!) by close to ten bits at n in the hundreds of thousands - enough for the guard to admit a factorial whose actual size was over the configured budget. Adding the omitted 0.5*log2(2*pi*n) correction term brings the prediction within a bit of exact, so it now errs toward refusing like the power prediction does. test_growth_through_multiplication_is_caught used a budget the power's own predictive check already rejected the first operand under, so it never reached the multiplication it claimed to be testing. The budget is now the exact ceiling that check admits for its expression, so failure can only come from the post-hoc Mul check - confirmed by disabling that check and watching the test fail, then restoring it. Re-measured the boundary factorial the default limit still admits: 71421!, ~0.43s on a release build, comfortably under one second, so the default is unchanged.
…te bases The factorial correction term added in the previous fix had no test that would fail without it: the existing ballpark range admitted both the two-term and three-term predictions. Replaced it with an exact value cross-checked against lgamma(1001)/ln(2). An exponent too large to fit a u64 was reported as "Invalid power operation", the same message already used for an unrelated powf failure. It now gets its own EXPONENT_TOO_LARGE_ERR. predicted_power_bits clamped a degenerate base's zero or one bit up to one and multiplied by the exponent, so 1^10000000, 0^10000000 and (-1)^10000000 - each free to compute and previously instant - were wrongly refused for "needing" ten million bits. The magnitude of a power of 0, 1 or -1 doesn't depend on the exponent, so these are now special-cased ahead of the multiplication. Also gave check_size its own wording instead of borrowing check_predicted_size's "would need" phrasing for a value that was already computed, and corrected two comments that overclaimed the factorial prediction as a guaranteed lower bound - it is accurate to about a bit, not exact, since ceil() of a value just under an integer can still round one bit low.
max(1), max(1,2,3), sin(1,2) and max() all reported the same generic 'malformed expression'. The shunting yard now keeps one frame per open bracket, recording whether it opens a call and how many arguments it has seen, and reports the function by name with the counts it expected and received. A comma outside a call is diagnosed on its own terms. Parentheses after a function name become mandatory: 'sin 5' used to work by accident, and argument counting has no meaning without a bracket to count within.
Two gaps let malformed input slip past the new diagnostics into the old
generic error: an unclosed bracket left its frame on the stack across a
';', desyncing it from the operator stack and surfacing as a misleading
arity mismatch on a later close; and has_content was tracked per bracket
rather than per argument slot, so an empty slot next to a comma (max(,1),
max(1,)) or an empty nested group (sin(())) passed the arity check with a
phantom argument. Both are now diagnosed on their own terms.
Also: dropped a doc link to a private item, extracted the duplicated
"must be followed by '('" message into one function, made arity's match
exhaustive over MathFunction so a new function forces its author to state
its arity, and added regression tests for the previously hand-verified
edge cases.
An unclosed bracket fell through to the generic malformed-expression message while a stray closing one already had a named diagnosis, so "max(1,2" never reached the arity check at all. Reject a non-empty bracket stack at end of input with the same named error. The degenerate-base short-circuit sat behind the exponent's narrowing to a u64, so "1^99999999999999999999" was refused as having an exponent too large to evaluate under any size limit - false for a base of magnitude 1. Move the narrowing into predicted_power_bits, behind the degenerate test, so the two cannot drift apart again: the function now declines an exponent it cannot use rather than being handed one it never needed. max_value_bits is documented as bounding every intermediate and final result, but a literal was pushed onto the stack unchecked and returned above the budget. Check it like any other value.
Cross-variant equality means NaturalNumber(2) == DecimalNumber(2/1), so these five assertions passed whichever variant came back. They named the decimal, which read as a claim that max returns a decimal - the belief this branch exists to remove - while in fact asserting nothing about the representation in either direction. Name the natural number and assert the variant alongside the value: the value assertion alone stays blind to the tag. Verified by making resolve() hand back DecimalNumber(n/1) for every integral result: all five now fail, and each fails on the variant assertion, the value one having passed. test_growth_through_multiplication_is_caught asserted only "size limit", which both wordings contain. It now asserts "occupies", so it pins the post-hoc check rather than trusting its own comment.
Only Limits and Session::with_limits are the contract an embedder needs. size_in_bits, check_size, check_predicted_size, predicted_factorial_bits and predicted_power_bits are how that contract is enforced, and exporting them from a published crate promotes their caveats - the factorial prediction's own "the remaining exposure is about a bit, not zero" among them - into API guarantees Stage 2 would have to keep or break again. parser.rs also carried two copies of the malformed-expression text while MALFORMED_ERR was already reachable from it; use the static at both sites.
The crate's front page and its docs.rs landing page both still described
the pre-branch rule - a decimal literal or a trigonometric function yields
a decimal - and both of the examples they used to explain it are now
counterexamples: 2.5+2.5 is 5 and 1/cos(0) is 1. State the invariant that
actually holds instead, in the same words on both pages.
setf's doc promised a Number::DecimalNumber it can no longer produce; it
goes through Number::decimal, so setf("x", 4.0) stores 4.
Document the canonicalisation invariant on the Number enum itself, where a
consumer meets the two publicly constructible variants, rather than only on
the constructor that upholds it.
Carry the design spec's factorial timing onto max_value_bits, so an
embedder raising the one public knob sees that its effect on worst-case
time is superlinear.
Gather the three declared behaviour changes in the design spec, phrased to
be lifted into the 0.3.0 release notes. Only one of the three was recorded
anywhere before; there is no CHANGELOG.md until Stage 3.
A variable is a value on the stack like any other, but the Token::Variable
arm pushed it without check_size while the operand arm beside it now has
one. set and setf write straight into the heap without passing through any
checked operator, so "x" alone returned whatever setf had stored: under a
64-bit budget, setf("x", 1e308) came back as a 1024-bit result. Undefined
variables still read as zero.
The two !found_open branches in the shunting-yard pass are unreachable, so
no test can cover them: bracket_stack and operators_stack gain and lose
open brackets together. Both arms now state that invariant and assert it
with debug_assert! instead of returning an error that cannot happen.
The README's variables example did not compile - set takes an i64 and was
handed 0.001, and resolve() returns a Result, which has no Display. Use
setf and unwrap, matching the same example in lib.rs.
Restore the two !found_open branches in the shunting-yard pass. Replacing them with debug_assert! was the wrong trade, on a wrong belief about the failure mode: falling through does not produce a malformed-RPN error. The loop above has by then drained the operator stack into the output in exactly the order the end-of-expression drain uses, so the postfix sequence stays evaluable and resolve() returns a number with the bracket grouping dissolved. Measured on a build with the invariant deliberately broken: 2*(3+4) gives 10, 2*(3+4)+5 gives 15, 1+(2+3)*4 gives 24. Two uncoverable branches are an annoyance; silently wrong arithmetic in a maths library is not. The invariant comments stay, and now say why the branch is kept and what it rests on. max_value_bits was documented as bounding any intermediate or final result, and the spec's release-note paragraph said it applied to every value on the stack. Neither is true: the function arm pushes its result unchecked, so with an 8-bit budget sin(1) returns 113 bits successfully. Narrow both to what is enforced - every literal, variable and arithmetic result - and name the exclusion where an embedder will read it. Size-checking the variable push made a small budget reject the built-in constants, which are f64s held exactly as rationals: 99 bits for pi, 107 for gamma. Name that floor on with_limits and pin it from both sides. The degenerate-base short-circuit also gave the size budget a new worst case: 1^n now runs a repeated-squaring loop over every bit of n instead of being refused at the u64 gate. At the default budget the largest such exponent is 315,652 digits and takes about 1.57s, against the 0.43s factorial the note quoted. Record it.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/limits.rs">
<violation number="1" location="src/limits.rs:149">
P2: Negative powers of integer bases can return a value above `Limits::max_value_bits` after passing this prediction. For example, with a 2-bit limit, `2^-1` predicts 2 here but produces `1/2` occupying 3 counted bits; accounting for the reciprocal numerator or checking the computed power with `check_size` would preserve the budget guarantee.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if base_bits <= 1 { | ||
| return Some(1); | ||
| } | ||
| Some(u128::from(base_bits) * u128::from(exponent_magnitude.to_u64()?)) |
There was a problem hiding this comment.
P2: Negative powers of integer bases can return a value above Limits::max_value_bits after passing this prediction. For example, with a 2-bit limit, 2^-1 predicts 2 here but produces 1/2 occupying 3 counted bits; accounting for the reciprocal numerator or checking the computed power with check_size would preserve the budget guarantee.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/limits.rs, line 149:
<comment>Negative powers of integer bases can return a value above `Limits::max_value_bits` after passing this prediction. For example, with a 2-bit limit, `2^-1` predicts 2 here but produces `1/2` occupying 3 counted bits; accounting for the reciprocal numerator or checking the computed power with `check_size` would preserve the budget guarantee.</comment>
<file context>
@@ -0,0 +1,219 @@
+ if base_bits <= 1 {
+ return Some(1);
+ }
+ Some(u128::from(base_bits) * u128::from(exponent_magnitude.to_u64()?))
+}
+
</file context>
Owner
Author
There was a problem hiding this comment.
Addressed at 9559aff, via the second remedy this comment offers rather than the first.
The prediction still under-estimates for negative exponents, and deliberately so: predicted_power_bits predicts the magnitude of base^|exponent|, while a negative exponent returns the reciprocal, whose denominator size_in_bits also counts. 2^-1 predicts 2 bits and yields 1/2, which measures 3. Teaching the prediction about the reciprocal would buy nothing, because the prediction is no longer what guarantees the budget.
What changed is the division of labour. The prediction is an optimisation — it buys the right to refuse 10^100000000 without computing it — and check_size on the value actually built is the guarantee. RpnResolver::power now measures its result on both paths, integer and powf, so a negative-exponent result over budget is refused by measurement rather than by estimate. Same change closed the two sibling findings on this review: the powf path returning unchecked, and the factorial pushing its result unchecked.
The same reasoning is now recorded in src/limits.rs's module doc, so the next person to tighten a prediction knows correctness does not rest on it: "check_size runs on the value that was actually built, and is the guarantee. It is exact by construction, because it measures rather than estimates. Predictions may be tightened or added freely; correctness does not rest on them."
Covered by tests in tests/integration_tests.rs for both paths — a powf result and a negative-exponent result, each refused under a tight budget.
The size prediction was doing two jobs and only ever fit one. Its job is to make a hopeless computation cheap to refuse: 999999999! declined in milliseconds instead of run. It cannot also be the guarantee, and three ways past it were reachable. power's non-integer path never consults a prediction at all, so under a 16-bit budget 2^0.5 returned a 106-bit rational. A negative exponent is predicted on the magnitude of base^|exponent| while the value returned is the reciprocal, whose denominator size_in_bits also counts: 2^-1 predicts 2 bits and yields 1/2, which measures 3. And the factorial pushed its result on the strength of a three-term Stirling series rounded up, which is a bit short of the truth at n=2 - the only n up to 60000 where it is, verified by computing them. Reaching that one needs the unchecked function arm to smuggle the operand in, since 2 is itself a 2-bit operand: floor(exp(1))! returned 2 under a 1-bit budget. Apply check_size to the materialised value on both power paths and to the factorial before it is pushed. Every predictive check stays exactly as it was - they are what keep the refusals fast. Predict to avoid the work, verify to be correct; the module doc and the predicted_* comments now say which of the two each check is, and predicted_power_bits no longer claims to be an upper bound on a value it does not describe. eval's contract comment had its operands backwards: the caller pops the top of the stack, which postfix order makes the second argument, so what eval pops is the first.
Both were fixed in the implementation during Task 3's fix rounds and never carried back into the plan, so the committed document still specifies the broken versions. test_growth_through_multiplication_is_caught was specified with a 4096-bit budget and x=2^3000. The power prediction for 2^3000 is bits(2)*3000 = 6000, already over 4096, so the expression is refused before x*x runs and the post-hoc Mul check the test exists to cover never executes. Verified: the specified version errors with "would need about 6000 bits", the predictive wording. Use the merged version - budget 4000, x=2^2000, where the power prediction is exactly 4000 and admitted - and assert "occupies" so a regression to the predictive path fails rather than passing quietly. predicted_factorial_bits was specified with the two-term Stirling series and the constant written as a literal that is bit-for-bit LOG2_E, which clippy's deny-by-default approx_constant rejects. Two terms also omits a correction worth about ten bits at the scale the default budget works at, which is how an earlier calibration admitted a value over its own budget. The companion test's (8000..=9200) range is why that survived: the two-term formula predicts 8524 for 1000!, inside the range, so the test could not tell the formulas apart. Pin the exact value instead. Both amendments say what was corrected and why, as 16fb791 did.
The function arm pushed its result unchecked. The justification for leaving it was that a function result is bounded by construction - every built-in routes through f64 - and that justification was wrong, not because a function returns something huge but because an unchecked value goes on to feed guards whose correctness assumes their inputs were checked. floor(exp(1))! is the proof: the factorial's predictive guard falls a bit short at n=2, and 2 is a 2-bit operand no checked arm would have admitted, so the function arm was the only way it could get there. Apply check_size like every other arm. Verified nothing legitimate is refused: sin, cos, sqrt, cdf, pdf, atan, ln and log all evaluate unchanged at sane budgets. That makes "every value on the evaluation stack is measured" true with no exceptions, so the carve-outs written while it was false come back out of the max_value_bits doc and the spec's release-note paragraph. Documenting an exception is what you do when you cannot close it. Closing it also shadows the factorial's own post-hoc check: every route to an operand now measures it first, so no input reaches that check with a prediction short of the truth. The check stays - n=2 being the only shortfall up to 60000 is an empirical bound, not a proof - but the test that used to cover it would now be exercising the function arm instead. It asserts the function arm and says so, rather than staying green while meaning something else. Correct predicted_power_bits' doc in the stage 1 plan, the third snippet describing a prediction as a bound, folded into the existing amendment note.
The reliable-core work produced a list of things reviewers raised and we chose not to fix: panicking public impls that need fallible signatures, four functions over the line threshold, untested edge cases, and diagnostics that are still generic. That list lived only in a gitignored scratch file, which is the same as not having it. Every entry is verified against the source at this commit and names files and symbols rather than line numbers, since half the line numbers recorded during the work were stale within a day.
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.
Stage 1 of making yarer production-ready: a core an embedder can rely on. No new features — four verified reliability defects fixed, plus the documentation and tests that make the new behaviour honest.
Breaking changes are declared and intentional; the target is 0.3.0. The version in
Cargo.tomlis deliberately left at 0.2.0 — Stage 2 (typed errors, source positions,Send/Sync) will add more breaking changes, so the bump belongs with the release, not here.What was wrong
Four defects, each reproduced against the built binary before being fixed:
1/0.0panicked. The derivedPartialEqcomparedNumberby enum tag, so the divide-by-zero guard never matchedDecimalNumber(0/1)andBigRational::new(v, 0)panicked. The same derive madePartialEqandPartialOrddisagree, which violates their contract.999999999!and10^100000000never returned. They ran until memory was exhausted.max(1),sin(1,2)andsin 5all producedRuntime Error: The mathematical expression is malformed., from the evaluation stack, where the function's name no longer exists.2.0,4/2andmax(1,2)could each come back asNaturalNumber(2)orDecimalNumber(2/1)depending on the route taken.What changed
Every value has exactly one representation.
Number::decimalis now the only constructor for a decimal, and it degrades an integral rational toNaturalNumber.PartialEqis hand-written and value-based, consistent withPartialOrd. The1/0.0panic closes as a side effect: the guard now sees the zero it was always meant to see.Oversized results are refused instead of computed. New
Limits { max_value_bits }, 1 Mibit by default — about 315,000 decimal digits — configurable per session throughSession::with_limits. Arithmetic is checked after the fact; powers and factorials are checked before any work happens, using a size prediction rather than a measurement:log2(n!) ≈ n·log2(n) − n·log2(e) + 0.5·log2(2πn), which matcheslgamma(n+1)/ln(2)to 0.00 bits at the boundary;bits(base) × |exponent|, a provable upper bound, which is why powers need no post-hoc check.999999999!and10^100000000now return a diagnostic in milliseconds. The largest factorial admitted at the default budget is71421!, about 0.43 s in release.Argument counts are validated while the RPN form is built, where the function's name and the bracket structure still exist. A per-bracket frame stack tracks the owning function, the separator count and whether each argument slot has content:
max(1,2,3)The mathematical expression is malformed.Function 'max' expects 2 argument(s), 3 given.sin 5sin(5)Function 'sin' must be followed by '('.max(,1)The mathematical expression is malformed.A function argument cannot be empty.(1,2)The mathematical expression is malformed.',' is only valid between the arguments of a function call.(1+2The mathematical expression is malformed.Unbalanced brackets.max(1; 2)A bracket must be closed before ';'.Function evaluation moved to its own module (
src/functions.rs), which shrankresolve()from 243 lines to 141 and gave the following three changes somewhere coherent to land.Declared behaviour changes
Gathered verbatim in
docs/superpowers/specs/2026-08-04-reliable-core-design.mdso a release note can be written from them:Number::NaturalNumber.2.5+2.5is5,1/cos(0)is1,6/3is2, andsetf("x", 4.0)stores4.DecimalNumberappears only when the value genuinely has a fractional part. Code matching on the variant to decide rendering may need adjusting; code comparing or converting values is unaffected, since equality and ordering work across variants.Limits::max_value_bits, applying to every literal, every variable read and every arithmetic result.(.sin 5andsqrt 16were previously accepted; they are now parse errors that name the function.Undefined variables still evaluate to
0, matchingbc. Prefix!5is still accepted alongside postfix5!. The interactive REPL is unchanged.Documentation
README.mdand the crate-level docs described the old return-type rule, using two examples that this branch turns into counterexamples. Both were rewritten.setf's doc comment promised aDecimalNumberit can no longer produce. Four README examples did not compile (E0277formatting aResult,E0308passing a float toset, which takesi64) — verified by extracting the blocks into a throwaway test, then fixed.Verification
cargo fmt --checkclean; no new clippy lint category, measured per-lint against the merge-base with a cleared cache;cargo doc --no-depsat the same 5 warnings as before the branch.Cargo.tomlandCargo.lockare untouched.DecimalNumberfor now-integral values were made sensitive again. Changing the expected variant was not enough: cross-variant equality meansNaturalNumber(2) == DecimalNumber(2/1), so each site also asserts the enum tag. Verified by de-canonicalising the constructor and confirming each fails.Known gaps, deliberately left
anyhow; typed errors with source positions are Stage 2.max(1,*2)passes the arity check and fails at evaluation.[and]remain bracket aliases, sosin[5]evaluates although the error text says parentheses.f64— and documented as such on the field rather than left implicit.Summary by cubic
Stage 1 makes the evaluation core reliable. It fixes panics and non-termination, enforces a size budget with both predictions and exact checks on every stack value (including function results), validates function arity with clear errors, canonicalizes numeric values, extracts function evaluation into
src/functions.rs, and documents known tech debt indocs/tech-debt.md.Bug Fixes
1/0.0;Numberequality is now value-based and consistent with ordering.n!via Stirling anda^bvia bit bounds, then verify the materialized value against the budget. Every value pushed to the stack is measured — literals, variables, arithmetic, powers, factorials, and function results — closing holes like2^0.5,2^-1, andfloor(exp(1))!under tiny budgets.sin 5now errors and must besin(5).Migration
Number::NaturalNumber;Number::DecimalNumberonly when the value has a fractional part. Update code that matches on the variant.((e.g.,sin(5)).Limits::max_value_bitsare refused (default ~1 Mibit); enforcement now covers all values, including function results. Configure per session viaSession::with_limits.Written for commit f8ede7e. Summary will update on new commits.