You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "alhudz (via GitHub)" <gi...@apache.org> on 2026/10/13 15:32:18 UTC
[PR] keep randomDouble and randomFloat below the exclusive upper bound [commons-lang]
alhudz opened a new pull request, #1817:
URL: https://github.com/apache/commons-lang/pull/1817
Repro: `RandomUtils.nextDouble(10, Math.nextUp(10))`; expected a value strictly below `endExclusive`, actual `10.000000000000002` (the exclusive bound) on 4953 of 10000 calls. `nextFloat` and the instance `randomDouble`/`randomFloat` behave the same.
Cause: `randomDouble`/`randomFloat` scale with `startInclusive + (endExclusive - startInclusive) * nextDouble()`, and rounding in the multiply and add can land exactly on `endExclusive`, which the Javadoc documents as not included. Same mechanism as LANG-1592, which fixed `nextLong(long, long)`; the double and float paths kept the unclamped scaling.
Fix: clamp an out-of-range result to `Math.nextDown(endExclusive)`, the correction `java.util.Random#nextDouble(double, double)` applies. The regression tests use a range one ULP wide, where the unpatched code returns the bound about half of the time; they fail before the change and pass after, and the default `mvn` goal is green.
- [x] Read the [contribution guidelines](CONTRIBUTING.md) for this project.
- [x] Read the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) if you use Artificial Intelligence (AI).
- [x] I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute? Claude Code was used to find the defect and draft the fix and tests; the change was reviewed and the build and tests were run locally before submitting.
- [x] Run a successful build using the default [Maven](https://maven.apache.org/) goal with `mvn`; that's `mvn` on the command line by itself.
- [x] Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep RandomUtils's randomDouble() and randomFloat() below the exclusive upper bound [commons-lang]
Posted by "alhudz (via GitHub)" <gi...@apache.org>.
alhudz commented on PR #1817:
URL: https://github.com/apache/commons-lang/pull/1817#issuecomment-6161821597
Reviewed and applied the remaining two: the instance tests are renamed to `testRandomDoubleExclusiveUpperBound`/`testRandomFloatExclusiveUpperBound` (and moved to keep members sorted), and the `randomFloat` comment now uses the same wording you committed for `randomDouble`, so neither names the newer JDK overloads. `RandomUtilsTest` is green locally, 125 tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep RandomUtils's randomDouble() and randomFloat() below the exclusive upper bound [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1817:
URL: https://github.com/apache/commons-lang/pull/1817#issuecomment-6156649434
@alhudz
I've committed a Copilot suggestion. Please review the other comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep RandomUtils's randomDouble() and randomFloat() below the exclusive upper bound [commons-lang]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1817:
URL: https://github.com/apache/commons-lang/pull/1817#discussion_r4272431306
##########
src/test/java/org/apache/commons/lang3/RandomUtilsTest.java:
##########
@@ -245,6 +245,19 @@ void testNextDouble(final RandomUtils ru) {
assertTrue(result < 42d);
}
+ /**
+ * Test a double range one ULP wide. A previous implementation could round the scaled value up to the
+ * exclusive upper limit about half of the time.
+ */
+ @Test
+ void testNextDoubleExclusiveUpperBound() {
+ final double startInclusive = 10d;
+ final double endExclusive = Math.nextUp(startInclusive);
+ for (int i = 0; i < 1000; i++) {
+ assertEquals(startInclusive, RandomUtils.nextDouble(startInclusive, endExclusive));
+ }
+ }
Review Comment:
The new exclusive-upper-bound tests duplicate the same structure (start value, `nextUp`, and `for (int i = 0; i < 1000; i++)`) across 4 methods. Consider extracting a small private helper (and/or a shared `TRIALS` constant) to reduce duplication and make future adjustments (e.g., changing the trial count or start value) less error-prone.
##########
src/main/java/org/apache/commons/lang3/RandomUtils.java:
##########
@@ -345,7 +345,9 @@ public double randomDouble(final double startInclusive, final double endExclusiv
if (startInclusive == endExclusive) {
return startInclusive;
}
- return startInclusive + (endExclusive - startInclusive) * random().nextDouble();
+ final double result = startInclusive + (endExclusive - startInclusive) * random().nextDouble();
+ // Rounding in the scaling above can reach endExclusive; clamp to the greatest value below endExclusive.
+ return result < endExclusive ? result : Math.nextDown(endExclusive);
Review Comment:
This clamping logic is correct for the instance methods, but it’s easy for similar scaling code paths to diverge (e.g., any static `nextDouble`/`nextFloat` implementation or other internal helpers that compute `start + (end-start) * u`). To keep behavior consistent and avoid regressions, consider centralizing the scaling+clamp into a shared private helper used by all floating-point range methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep RandomUtils's randomDouble() and randomFloat() below the exclusive upper bound [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1817:
URL: https://github.com/apache/commons-lang/pull/1817#issuecomment-6179267181
@alhudz See my comment above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep RandomUtils's randomDouble() and randomFloat() below the exclusive upper bound [commons-lang]
Posted by "alhudz (via GitHub)" <gi...@apache.org>.
alhudz commented on code in PR #1817:
URL: https://github.com/apache/commons-lang/pull/1817#discussion_r4280153888
##########
src/test/java/org/apache/commons/lang3/RandomUtilsTest.java:
##########
@@ -245,6 +245,19 @@ void testNextDouble(final RandomUtils ru) {
assertTrue(result < 42d);
}
+ /**
+ * Test a double range one ULP wide. A previous implementation could round the scaled value up to the
+ * exclusive upper limit about half of the time.
+ */
+ @Test
+ void testNextDoubleExclusiveUpperBound() {
+ final double startInclusive = 10d;
+ final double endExclusive = Math.nextUp(startInclusive);
+ for (int i = 0; i < 1000; i++) {
+ assertEquals(startInclusive, RandomUtils.nextDouble(startInclusive, endExclusive));
+ }
+ }
Review Comment:
Done in f434d34. The four exclusive-bound tests now loop over `EXCLUSIVE_BOUND_TRIALS` instead of a literal `1000`. I left the existing `* 1000` in `testNextDouble`/`testNextFloat` alone because it is a separate scaling factor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep RandomUtils's randomDouble() and randomFloat() below the exclusive upper bound [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #1817:
URL: https://github.com/apache/commons-lang/pull/1817
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep RandomUtils's randomDouble() and randomFloat() below the exclusive upper bound [commons-lang]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1817:
URL: https://github.com/apache/commons-lang/pull/1817#discussion_r4264629193
##########
src/main/java/org/apache/commons/lang3/RandomUtils.java:
##########
@@ -345,7 +345,9 @@ public double randomDouble(final double startInclusive, final double endExclusiv
if (startInclusive == endExclusive) {
return startInclusive;
}
- return startInclusive + (endExclusive - startInclusive) * random().nextDouble();
+ final double result = startInclusive + (endExclusive - startInclusive) * random().nextDouble();
+ // Rounding in the scaling above can reach endExclusive; clamp like java.util.Random#nextDouble(double, double).
Review Comment:
The comment references `java.util.Random#nextDouble(double, double)` / `nextFloat(float, float)`, which only exist in newer JDKs (not in Java 8). If this project still targets older Java versions, consider rewording to avoid pointing at APIs that don’t exist for all contributors (e.g., describe the behavior without naming the overload, or reference the JDK implementation conceptually).
##########
src/test/java/org/apache/commons/lang3/RandomUtilsTest.java:
##########
@@ -245,6 +245,33 @@ void testNextDouble(final RandomUtils ru) {
assertTrue(result < 42d);
}
+ /**
+ * Test a double range one ULP wide. A previous implementation could round the scaled value up to the
+ * exclusive upper limit about half of the time.
+ */
+ @Test
+ void testNextDoubleExclusiveUpperBound() {
+ final double startInclusive = 10d;
+ final double endExclusive = Math.nextUp(startInclusive);
+ for (int i = 0; i < 1000; i++) {
+ assertEquals(startInclusive, RandomUtils.nextDouble(startInclusive, endExclusive));
+ }
+ }
Review Comment:
There are two test methods with the same name (`testNextDoubleExclusiveUpperBound`) differentiated only by their parameter list. This can make test reports and IDE navigation ambiguous. Consider renaming one (e.g., `testRandomDoubleExclusiveUpperBound`) and similarly for the float tests to clearly distinguish static vs instance behavior.
##########
src/test/java/org/apache/commons/lang3/RandomUtilsTest.java:
##########
@@ -245,6 +245,33 @@ void testNextDouble(final RandomUtils ru) {
assertTrue(result < 42d);
}
+ /**
+ * Test a double range one ULP wide. A previous implementation could round the scaled value up to the
+ * exclusive upper limit about half of the time.
+ */
+ @Test
+ void testNextDoubleExclusiveUpperBound() {
+ final double startInclusive = 10d;
+ final double endExclusive = Math.nextUp(startInclusive);
+ for (int i = 0; i < 1000; i++) {
+ assertEquals(startInclusive, RandomUtils.nextDouble(startInclusive, endExclusive));
+ }
+ }
+
+ /**
+ * Test a double range one ULP wide. A previous implementation could round the scaled value up to the
+ * exclusive upper limit about half of the time.
+ */
+ @ParameterizedTest
+ @MethodSource("randomProvider")
+ void testNextDoubleExclusiveUpperBound(final RandomUtils ru) {
+ final double startInclusive = 10d;
+ final double endExclusive = Math.nextUp(startInclusive);
+ for (int i = 0; i < 1000; i++) {
+ assertEquals(startInclusive, ru.randomDouble(startInclusive, endExclusive));
+ }
+ }
Review Comment:
There are two test methods with the same name (`testNextDoubleExclusiveUpperBound`) differentiated only by their parameter list. This can make test reports and IDE navigation ambiguous. Consider renaming one (e.g., `testRandomDoubleExclusiveUpperBound`) and similarly for the float tests to clearly distinguish static vs instance behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep RandomUtils's randomDouble() and randomFloat() below the exclusive upper bound [commons-lang]
Posted by "alhudz (via GitHub)" <gi...@apache.org>.
alhudz commented on PR #1817:
URL: https://github.com/apache/commons-lang/pull/1817#issuecomment-6187072154
Addressed in `f434d34`: the four exclusive-bound tests now loop over a `EXCLUSIVE_BOUND_TRIALS` constant instead of the literal `1000`. On the other Copilot note about a shared helper, the static `nextDouble(double, double)`/`nextFloat(float, float)` already delegate to `randomDouble`/`randomFloat`, so the clamp lives in one place per type and I left that as is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep RandomUtils's randomDouble() and randomFloat() below the exclusive upper bound [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #1817:
URL: https://github.com/apache/commons-lang/pull/1817#discussion_r4273083557
##########
src/test/java/org/apache/commons/lang3/RandomUtilsTest.java:
##########
@@ -245,6 +245,19 @@ void testNextDouble(final RandomUtils ru) {
assertTrue(result < 42d);
}
+ /**
+ * Test a double range one ULP wide. A previous implementation could round the scaled value up to the
+ * exclusive upper limit about half of the time.
+ */
+ @Test
+ void testNextDoubleExclusiveUpperBound() {
+ final double startInclusive = 10d;
+ final double endExclusive = Math.nextUp(startInclusive);
+ for (int i = 0; i < 1000; i++) {
+ assertEquals(startInclusive, RandomUtils.nextDouble(startInclusive, endExclusive));
+ }
+ }
Review Comment:
@alhudz
Use a constant for the magic number 1000.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep RandomUtils's randomDouble() and randomFloat() below the exclusive upper bound [commons-lang]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #1817:
URL: https://github.com/apache/commons-lang/pull/1817#issuecomment-6199318085
@alhudz Merged 🚀 Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org