Skip to content

fix(expr): Match spec behavior for identified float, string, and path differences - #305

Merged
mwiebe merged 7 commits into
OpenJobDescription:mainfrom
mwiebe:fix/issue-290-expr-divergences
Aug 12, 2026
Merged

fix(expr): Match spec behavior for identified float, string, and path differences#305
mwiebe merged 7 commits into
OpenJobDescription:mainfrom
mwiebe:fix/issue-290-expr-divergences

Conversation

@mwiebe

@mwiebe mwiebe commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Fixes: #290

What was the problem/requirement? (What/Why)

The OpenJD expression evaluator is intended to produce the same results as the
reference implementation. Differential testing found six cases where the Rust
implementation behaved differently:

  1. Floating-point remainder calculations could lose the remainder when dividing
    a very large number by a small number.
  2. Floating-point floor division could be off by one because the intermediate
    division result had already been rounded.
  3. Small calculated numbers used a different scientific notation, such as
    1e-7 instead of the canonical 1e-07.
  4. Centering text with an odd number of padding spaces could put the extra space
    on the wrong side.
  5. Joining a Windows root-relative path, such as \renders, to a relative path
    incorrectly kept the relative path in front of it.
  6. Ordering a path and a string could reverse the comparison when the path was
    on the left. For example, path("/a") < "/z" incorrectly evaluated to
    false.

These differences could change calculated parameter values, rendered text,
Windows file locations, or the ordering of values in an OpenJD expression.

What was the solution? (How)

Each behavior was corrected independently:

  • Floating-point floor division and remainder now use the remainder to derive a
    consistent quotient. This avoids losing useful precision or producing an
    off-by-one result near a rounding boundary.
  • Calculated numbers in scientific notation now include an exponent sign and at
    least two exponent digits.
  • Text centering now places an unmatched padding space on the same side as the
    reference implementation.
  • A root-relative Windows path now replaces a drive-less relative path. Existing
    drive roots and network-share roots are still retained when present.
  • Path-to-string ordering now compares the values in the same left-to-right order
    in which they appear in the expression. This also applies when those values are
    inside lists.

The expression specification documents were updated alongside each behavior.

What is the impact of this change?

Expressions using these edge cases now produce results consistent with the
reference implementation. Most expressions are unaffected.

Users who depended on one of the incorrect results may observe a corrected
number, string, path, or comparison result. No public Rust API signatures were
changed.

How was this change tested?

  • Compared each reported behavior with locally installed Python 3.14.3 before
    changing the Rust implementation.
  • Added a focused regression test for each behavior and confirmed that it failed
    before the corresponding fix.
  • Performed mutation testing for every fix, including boundary mutations found
    while testing floor division and a manual operand-reversal mutation for the
    path/string comparison.
  • Ran cargo test -p openjd-expr: 308 unit tests, 3,150 integration tests, and
    8 documentation tests passed.
  • Ran cargo fmt --all -- --check.
  • Ran
    cargo clippy --all-features --all-targets --workspace -- -D warnings.
  • Ran the full workspace test suite. Two unrelated Windows credential tests
    failed locally because this machine returned the domain-unavailable error
    0x8007051F instead of the expected unknown-account error. The complete
    workspace suite passed when only those two environment-dependent tests were
    skipped.

Was this change documented?

Yes. The relevant behavior is documented in:

  • specs/expr/function-library.md
  • specs/expr/path-mapping.md
  • specs/expr/public-api.md
  • specs/expr/values.md

No code docstring changes were needed because the public function signatures did
not change.

Is this a breaking change?

No. This corrects behavior that differed from the documented reference behavior
and does not change the public API.

Does this change impact security?

No. It does not add privileges, external inputs, network access, or new
file/directory permission behavior. No threat model changes are needed.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@mwiebe
mwiebe requested a review from a team as a code owner August 10, 2026 21:47
Comment thread crates/openjd-expr/src/functions/path.rs
@leongdl

leongdl commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Reviewed this PR with mutation testing — all six fixes check out (each regression test fails by name when its fix is individually reverted; 8/8 mutants caught, suite/clippy/fmt green at the tip).

One observation for a possible follow-up, not a blocker for this PR: ExprValue::equals still contains the same combined or-pattern with swapped bindings that caused divergence #6 in compare():

(Self::String(a), Self::Path { value: b, .. })
| (Self::Path { value: b, .. }, Self::String(a)) => a == b,

In the second alternative, a binds to the right operand. Equality is symmetric, so this is harmless today — but it's exactly the pattern shape that silently reversed path/string ordering in compare(), and keeping it around invites copying it back into an ordering context. Splitting it into two position-consistent arms (mirroring the fix this PR makes in compare()) would remove the trap. Happy to leave that to a separate change since it's pre-existing code this PR doesn't touch.

@mwiebe
mwiebe force-pushed the fix/issue-290-expr-divergences branch from 4cea4dc to cca0557 Compare August 12, 2026 16:38
@mwiebe
mwiebe enabled auto-merge (squash) August 12, 2026 17:11
@mwiebe

mwiebe commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

One observation for a possible follow-up, not a blocker for this PR: ExprValue::equals still contains the same combined or-pattern with swapped bindings that caused divergence #6 in compare():

(Self::String(a), Self::Path { value: b, .. })
| (Self::Path { value: b, .. }, Self::String(a)) => a == b,

I'll look at this in a follow-up.

mwiebe added 7 commits August 12, 2026 10:31
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
@mwiebe
mwiebe force-pushed the fix/issue-290-expr-divergences branch from cca0557 to 085c26b Compare August 12, 2026 17:31
@mwiebe
mwiebe merged commit 4f04c13 into OpenJobDescription:main Aug 12, 2026
25 of 39 checks passed
@mwiebe
mwiebe deleted the fix/issue-290-expr-divergences branch August 12, 2026 18:05
@github-actions github-actions Bot mentioned this pull request Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug(expr): divergences from CPython/spec

3 participants