Fix Uniform::new_inclusive overflow on large finite float ranges - #1809
Closed
teddytennant wants to merge 3 commits into
Closed
Fix Uniform::new_inclusive overflow on large finite float ranges#1809teddytennant wants to merge 3 commits into
teddytennant wants to merge 3 commits into
Conversation
dhardy
reviewed
Jul 12, 2026
Comment on lines
136
to
143
| if !scale.all_finite() { | ||
| return Err(Error::NonFinite); | ||
| // The division above may overflow to infinity even though | ||
| // `range` is finite (e.g. `low = 0.0`, `high = f64::MAX`). | ||
| // Replace infinite lanes with the largest finite value; | ||
| // `new_bounded` reduces `scale` as required to ensure that | ||
| // samples can never exceed `high`. | ||
| scale = scale.decrease_masked(scale.gt_mask(<$ty>::splat($f_scalar::MAX))); | ||
| } |
Member
There was a problem hiding this comment.
We shouldn't need this if branch at all assuming we change let mask = (scale * max_rand + low).gt_mask(high) (in new_bounded) to handle NaNs by replacing gt_mask with not_le_mask.
1 task
teddytennant
force-pushed
the
uniform-float-inclusive-overflow
branch
from
August 8, 2026 16:25
ef5b8ab to
2b1d0eb
Compare
Dividing high - low by 1 - EPSILON can round to infinity even when the range itself is finite (e.g. 0.0..=f64::MAX), yielding a spurious NonFinite error while Uniform::new and sample_single_inclusive both accept the same range. Clamp infinite lanes to the largest finite value and let new_bounded reduce scale as usual, so that samples still cannot exceed high. Noted by dhardy in rust-random#1603.
Per review: drop the explicit non-finite scale branch in new_inclusive and let new_bounded reduce the scale instead. gt_mask compares false for a non-finite product, so the loop would accept it silently; not_le_mask treats it as out of bounds and reduces the scale, which decrease_masked already handles for infinity. new_inclusive still rejects a non-finite range up front, so f64::MIN..=f64::MAX remains an error, matching the exclusive range.
not_le_mask replaced its only call site, so gt_mask was dead code and failed the -D warnings clippy job.
Contributor
Author
|
Good call, that's much cleaner. Dropped the special case in |
Member
|
Sorry but I already opened a replacement PR (#1821); I just forgot to close this one. |
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.
CHANGELOG.mdentrySummary
Fix
Uniform::new_inclusivereturning a spuriousError::NonFinitefor large finite float ranges such as0.0..=f64::MAX, as noted by @dhardy in #1603.Motivation
new_inclusivecomputesscale = (high - low) / (1 - EPSILON). The division can round to infinity even whenhigh - lowis finite (e.g.low = 0.0,high = f64::MAX), so theall_finite()check rejects the range. MeanwhileUniform::newandsample_single_inclusiveboth accept the same bounds, which is the inconsistency reported in #1603.Details
new_inclusivenow checkshigh - lowfor finiteness first (preserving theNonFiniteerror for genuinely infinite ranges likef64::MIN..=f64::MAX), then clamps any infinite lanes ofscaledown to the largest finite value using the existingdecrease_maskedhelper.new_boundedthen reducesscaleas usual, so samples still cannot exceedhigh.Tests:
0.0..=MAXand-MAX..=0.0are added to thetest_floatsrange list (checking bounds and value ordering for both scalar and SIMD lanes), andtest_float_overflowgains an assertion thatf64::MIN..=f64::MAXstill errors.