fix(expr): align arithmetic, rounding, paths, and float display with the spec/Python reference - #2
Draft
crowecawcaw wants to merge 2 commits into
Draft
fix(expr): align arithmetic, rounding, paths, and float display with the spec/Python reference#2crowecawcaw wants to merge 2 commits into
crowecawcaw wants to merge 2 commits into
Conversation
…the spec/Python reference
Fixes a set of expression-evaluator behaviors where openjd-expr diverged from
the OpenJD spec (RFC 0005/0006) and its Python reference implementation. Each
fix comes with regression tests asserting the concrete expected value.
Numeric semantics:
- round(float, ndigits) returned float; the spec (RFC 0006) says int when
ndigits <= 0, float when ndigits > 0. round(float) likewise returns int.
- round(int, -k) now uses exact integer arithmetic, so large ints keep every
digit above the rounding position (an f64 round-trip corrupted them).
- round(float, ndigits>0) preserves trailing zeros in the display
(round(3.5, 2) -> "3.50"), including for zero (round(0.0, 7) -> "0.0000000").
- Float modulo now uses CPython's fmod-based algorithm instead of
l - r*floor(l/r), which lost all precision for large quotients
(9.2e18 % 0.1 was 0.0, should be 2.842170943040401e-14).
- Float floor-division reproduces CPython's float_divmod (205 // 0.1 is 2049,
not 2050) and reports an integer-overflow error instead of saturating when
the quotient exceeds i64.
- int ** (negative exponent) is computed with powf, not powi, avoiding a
last-ulp error from repeated squaring.
- The float->i64 conversions (floor/ceil/round/floordiv) use an exact 2^63
bound check; the previous `> i64::MAX as f64` let exactly 2^63 through and
saturated silently.
Conversions and display:
- float(x) is a true identity that preserves the original float literal
(RFC 0005 float pass-through), so float(1e308) stays "1e308" rather than
reformatting to "1e+308".
- format_float emits C/Python-style exponents (sign + min two digits: e+18,
e-07) instead of Rust's bare e18 / e-7.
- Float64::with_str only discards a negative-zero string, so a zero value keeps
legitimate trailing zeros.
Strings and paths:
- center/ljust/rjust clamp negative widths to 0 (Python semantics) instead of
casting -1 to usize and wrapping to a huge value.
- center's odd-padding bias matches CPython (marg & width & 1).
- path join treats an empty right component as a no-op (no trailing separator),
and on Windows a root-relative right with no drive/UNC left replaces the left
(matching ntpath.join).
- Mixed path/string comparison preserves operand order (path('/tmp/x') < 'ab'
was returning the wrong boolean).
A few tests that asserted the pre-alignment behavior are updated to the
spec/Python-correct values (round(2.5,0) -> 2 not 2.0; path join empty right
-> "/a/b" not "/a/b/").
6 tasks
Per the report-driven-development convention, reflect this change's fixes in reports/expr-quality-evaluation-report.md. The report previously claimed "Behavioral differences: None observed" and "Bugs found: None" — true for the original one-shot probes, but a later differential pass against the Python reference found 15 silent-value/wrong-type divergences (now all fixed). Mark both claims superseded and add the findings table; note the String↔Path ordering bug specifically contradicted the old "String↔Path equality matches Python" observation.
Owner
Author
|
Updated
Let me know if you'd prefer the findings recorded differently (e.g. as §8 Recommendations items instead), or if the comment was pointing at a different report. |
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
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
Fixes a set of
openjd-exprexpression-evaluator behaviors that diverged from the OpenJD spec (RFC 0005 / RFC 0006) and its Python reference implementation. Each fix ships with regression tests asserting the concrete expected value, matching the crate's existing test-quality standard.These divergences were found by differential-testing
openjd-expragainst the Python reference. That harness is a developer tool with an out-of-tree Python dependency, so it is not included here — it lives on a branch and is linked below. This PR is self-contained: fixes + unit tests only, no new external dependencies, no CI changes.🔧 Differential tool (for reference / re-running):
differential-toolingbranchFixes (each spec-referenced and test-covered)
Numeric semantics
round(float, ndigits)returned float; per RFC 0006 it returns int whenndigits ≤ 0, float when> 0.round(float)returns int. (spec-confirmed)round(int, -k)now rounds via exact integer arithmetic, so large ints keep every digit above the rounding position (an f64 round-trip corrupted them).round(float, ndigits>0)preserves trailing zeros (RFC 0006):round(3.5, 2)→"3.50",round(0.0, 7)→"0.0000000".l - r*floor(l/r), which lost all precision for large quotients (9.2e18 % 0.1was0.0, should be2.842170943040401e-14).float_divmod(205 // 0.1is2049, not2050) and reports integer overflow instead of saturating when the quotient exceeds i64.int ** (negative exponent)usespowf, notpowi— avoids a last-ulp error from repeated squaring.floor/ceil/round/floordiv) use an exact 2^63 bound; the old> i64::MAX as f64check let exactly 2^63 through and saturated silently. (RFC 0005 §3: out-of-range integer results are errors, not silent wraps.)Conversions & display
float(x)is a true identity that preserves the original float literal (RFC 0005 float pass-through):float(1e308)stays"1e308", not"1e+308".format_floatemits C/Python-style exponents — sign + min two digits (e+18,e-07) — instead of Rust's baree18/e-7.Float64::with_stronly discards a negative-zero string, so a zero value keeps legitimate trailing zeros.Strings & paths
center/ljust/rjustclamp negative widths to 0 (Python semantics) instead of casting-1tousizeand wrapping to a huge value (which tripped the op limit / attempted a giant allocation).center's odd-padding bias matches CPython (marg & width & 1).pathjoin treats an empty right component as a no-op (no trailing separator); on Windows a root-relative right with no drive/UNC left replaces the left (matchingntpath.join).path('/tmp/x') < 'ab'was returning the wrong boolean).Note on the oracle
Python is the OpenJD spec's named reference implementation (RFC 0005), so matching it is the default — but the spec only says implementations "should follow it to the extent the language supports," not that Python is normatively correct in every case. Most fixes here are directly spec-confirmed. A few (float floor-div/mod tie-breaking, exponent spelling, negative-width padding) are spec-silent edge cases where the Python reference is the de-facto definition; those are reasonable readings and are called out as such in code comments. They'd be good candidates to raise as spec clarifications separately.
Test updates
A few existing tests asserted the pre-alignment behavior and are updated to the spec/Python-correct values:
round(2.5, 0)→2(int), not2.0; and theround(3.5,0)/round(-2.5,0)/round(-3.5,0)siblings.join_posix_empty_right→"/a/b", not"/a/b/".Test plan
cargo test --workspace --exclude openjd-for-js— all suites pass (verified locally; 3,068 inopenjd-expr).cargo clippy --all-features --all-targets --workspace -- -D warnings— clean.cargo fmt --all --check— clean.