fix(randx): use upper bound in Between - #512
Merged
Merged
Conversation
Between passed lower as the exclusive maximum to crypto/rand.Int and never read upper, so it returned values in [lower, 2*lower) instead of [lower, upper). With lower == 0 the call panics in crypto/rand. Compute the range as upper-lower and shift the result by lower, and return an error when upper <= lower. Extend tests to assert both bounds tightly over 1000 samples, cover lower == 0, and cover invalid bounds.
adamdecaf
approved these changes
Aug 12, 2026
Contributor
Author
|
Thanks for the merge. |
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
2 participants
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.
Problem
randx.Between(lower, upper)never readsupper. It passesloweras the exclusive maximum tocrypto/rand.Intand then addslowerto the result, so the returned value falls in[lower, 2*lower)instead of[lower, upper).The existing test only passes because
[100, 200)loosely overlaps the asserted range of[100, 250].Additionally, when
lower == 0,crypto/rand.Intis called with a maximum of 0 and panics, so any caller doingBetween(0, n)crashes the process.Fix
upper - lower, draw from[0, upper-lower), and shift the result bylower, yielding a uniform value in[lower, upper).upper <= lowerinstead of panicking insidecrypto/rand. This matches the package's existing style whereBetweenreturns errors andMustpanics on them.Tests
TestBetweenandTestMustnow assert both bounds tightly over 1000 samples (lower <= n < upper).TestBetween_ZeroLowercoverslower == 0, which previously panicked.TestBetween_InvalidBoundsandTestMust_Panicscover theupper <= lowererror path.Verified that the new tests fail against the old implementation (the zero-lower test panics in
crypto/rand.Int).Made with Cursor