Skip to content

[codec] Reject incomparable values in RangeCfg::contains - #4529

Open
memosr wants to merge 1 commit into
commonwarexyz:mainfrom
memosr:fix-rangecfg-partial-ord
Open

[codec] Reject incomparable values in RangeCfg::contains#4529
memosr wants to merge 1 commit into
commonwarexyz:mainfrom
memosr:fix-rangecfg-partial-ord

Conversation

@memosr

@memosr memosr commented Aug 19, 2026

Copy link
Copy Markdown

The inherent RangeCfg::contains is written as a pair of exclusion tests (value < s, value > e). Under a partial order the negation of "is less than" is not "is greater or equal", so a value incomparable to both bounds escapes every arm and is reported as contained.

Since the inherent method shadows the one from the RangeBounds impl, the same RangeCfg gives two different answers for the same value:

let cfg = RangeCfg::new(0.0f64..=1.0f64);
cfg.contains(&f64::NAN);                // true
RangeBounds::contains(&cfg, &f64::NAN); // false

cores RangeBounds::contains uses inclusion tests, which is why it gets this right.

The bound on RangeCfg is Copy + PartialOrd, so RangeCfg<f64> compiles today. I grepped the workspace and every current instantiation is an integer or NonZero type, where the two definitions coincide, so this is latent rather than live. Filing it because the divergence is silently wrong for any future PartialOrd-only type, and because the fix costs nothing for existing users.

Fix: use partial_cmp and treat None as out of range. This is also what clippy::neg_cmp_op_on_partial_ord recommends; the naive rewrite to !(s <= value) trips that lint, which exists for exactly this class of bug.

Behavior is unchanged for totally ordered types. No wire format change.

Added test_range_cfg_partial_ord_nan, which fails on main and passes here. cargo test, cargo clippy --all-targets -- -D warnings, cargo fmt, and cargo check --no-default-features are all clean for commonware-codec.

RangeCfg::contains used exclusion tests (value < s, value > e). Under a
partial order the negation of these is not inclusion, so a value that is
incomparable to both bounds escaped every arm and was reported as
contained.

Because the inherent method shadows RangeBounds::contains, the same
RangeCfg gave two different answers for the same value:

    let cfg = RangeCfg::new(0.0f64..=1.0f64);
    cfg.contains(&f64::NAN);                // true
    RangeBounds::contains(&cfg, &f64::NAN); // false

Switch to partial_cmp so incomparable values are explicitly rejected,
matching core's RangeBounds::contains. Behavior is unchanged for totally
ordered types, which covers every RangeCfg instantiation in the
workspace today.

Adds a regression test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant