Introduce RandomGeneratorNext{Double,Int,Long}WithOrigin Refaster rules#2087
Introduce RandomGeneratorNext{Double,Int,Long}WithOrigin Refaster rules#2087Stephan202 merged 7 commits intomasterfrom
RandomGeneratorNext{Double,Int,Long}WithOrigin Refaster rules#2087Conversation
ac2cbe7 to
bd4c84d
Compare
|
Suggested commit message: |
|
Looks good. No mutations were possible for these changes. |
1 similar comment
|
Looks good. No mutations were possible for these changes. |
| static final class RandomGeneratorNextIntWithOrigin { | ||
| @BeforeTemplate | ||
| int before(RandomGenerator random, int a, int b) { | ||
| return Refaster.anyOf(a + random.nextInt(b)); |
There was a problem hiding this comment.
[javac] reported by reviewdog 🐶
[RefasterAnyOfUsage] Refaster#anyOf should be passed at least two parameters
There was a problem hiding this comment.
AKA: No need to use this API if we're only writing from one before template variant.
mohamedsamehsalah
left a comment
There was a problem hiding this comment.
Make sure we run mvn fmt:format and run the full build ./run-full-build.sh
| static final class RandomGeneratorNextIntWithOrigin { | ||
| @BeforeTemplate | ||
| int before(RandomGenerator random, int a, int b) { | ||
| return Refaster.anyOf(a + random.nextInt(b)); |
There was a problem hiding this comment.
AKA: No need to use this API if we're only writing from one before template variant.
|
Looks good. No mutations were possible for these changes. |
mohamedsamehsalah
left a comment
There was a problem hiding this comment.
Suggesting:
Introduce `RandomGeneratorNextIntWithOrigin` refaster rule
| } | ||
|
|
||
| ImmutableSet<Integer> testRandomGeneratorNextIntWithOrigin() { | ||
| return ImmutableSet.of(20 + new Random().nextInt(80)); |
There was a problem hiding this comment.
The return type can be an int.
There was a problem hiding this comment.
updated this and the test output 🫡
|
Looks good. No mutations were possible for these changes. |
rickie
left a comment
There was a problem hiding this comment.
Makes sense but I think we can do even more :)
|
|
||
| @AfterTemplate | ||
| int after(RandomGenerator random, int a, int b) { | ||
| return random.nextInt(a, a + b); |
There was a problem hiding this comment.
Can't we also apply this to the nextLong and nextDouble?
There was a problem hiding this comment.
Indeed, I think we can!
|
Looks good. No mutations were possible for these changes. |
📝 WalkthroughWalkthroughIntroduces three new refaster rules to RandomGeneratorRules that replace arithmetic expressions with origin-and-bound variant calls for RandomGenerator.nextInt(), nextDouble(), and nextLong() methods. Includes corresponding test cases for both input and expected output scenarios. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/RandomGeneratorRules.java`:
- Around line 82-123: The after-templates in RandomGeneratorNextIntWithOrigin,
RandomGeneratorNextDoubleWithOrigin, and RandomGeneratorNextLongWithOrigin
duplicate the expression `a` (risking double evaluation) and can cause overflow
for `a + b` (changing wrapping semantics to exceptions); add Refaster guards so
these rules only apply when `a` is a stable/pure expression (no side effects or
method calls) and when `a + b` cannot overflow: for
RandomGeneratorNextIntWithOrigin require b >= 0 and a <= Integer.MAX_VALUE - b;
for RandomGeneratorNextLongWithOrigin require b >= 0 and a <= Long.MAX_VALUE -
b; for RandomGeneratorNextDoubleWithOrigin require Double.isFinite(a) &&
Double.isFinite(b) && b >= 0 && Double.isFinite(a + b); implement these as guard
methods (e.g., isStable(a), nonNegative(b), noOverflowInt(a,b),
noOverflowLong(a,b), finiteDoubleBounds(a,b)) referenced from the rule classes
so the refactoring is only applied when safe.
| static final class RandomGeneratorNextIntWithOrigin { | ||
| @BeforeTemplate | ||
| int before(RandomGenerator random, int a, int b) { | ||
| return a + random.nextInt(b); | ||
| } | ||
|
|
||
| @AfterTemplate | ||
| int after(RandomGenerator random, int a, int b) { | ||
| return random.nextInt(a, a + b); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Prefer {@link RandomGenerator#nextDouble(double origin, double bound)} over more contrived | ||
| * alternatives. | ||
| */ | ||
| static final class RandomGeneratorNextDoubleWithOrigin { | ||
| @BeforeTemplate | ||
| double before(RandomGenerator random, double a, double b) { | ||
| return a + random.nextDouble(b); | ||
| } | ||
|
|
||
| @AfterTemplate | ||
| double after(RandomGenerator random, double a, double b) { | ||
| return random.nextDouble(a, a + b); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Prefer {@link RandomGenerator#nextLong(long origin, long bound)} over more contrived | ||
| * alternatives. | ||
| */ | ||
| static final class RandomGeneratorNextLongWithOrigin { | ||
| @BeforeTemplate | ||
| long before(RandomGenerator random, long a, long b) { | ||
| return a + random.nextLong(b); | ||
| } | ||
|
|
||
| @AfterTemplate | ||
| long after(RandomGenerator random, long a, long b) { | ||
| return random.nextLong(a, a + b); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file in question to see the complete rules with any guards/constraints
fd -type f -name "RandomGeneratorRules.java" -exec wc -l {} \;Repository: PicnicSupermarket/error-prone-support
Length of output: 251
🏁 Script executed:
# Search for the RandomGenerator interface to understand its contract
fd -type f -name "*.java" | xargs grep -l "interface RandomGenerator" | head -5Repository: PicnicSupermarket/error-prone-support
Length of output: 251
🏁 Script executed:
# Look for nextInt(int origin, int bound) signature documentation
rg "nextInt.*origin.*bound" -A 5 -B 5Repository: PicnicSupermarket/error-prone-support
Length of output: 1475
🏁 Script executed:
# Check if there are any guards or constraints in the Refaster rules
cat error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/RandomGeneratorRules.javaRepository: PicnicSupermarket/error-prone-support
Length of output: 4466
🏁 Script executed:
# Search for RandomGenerator usage in tests to understand the expected behavior
fd -name "*RandomGenerator*Test*" -o -name "*Test*RandomGenerator*"Repository: PicnicSupermarket/error-prone-support
Length of output: 311
🏁 Script executed:
# Look for any documentation or tests that show the contract for nextInt(origin, bound)
rg "nextInt.*origin.*bound" -B 3 -A 10Repository: PicnicSupermarket/error-prone-support
Length of output: 1842
🏁 Script executed:
# Search for RandomGenerator interface in the repository
rg "interface RandomGenerator"Repository: PicnicSupermarket/error-prone-support
Length of output: 63
🌐 Web query:
java.util.random.RandomGenerator nextInt origin bound contract documentation
💡 Result:
java.util.random.RandomGenerator#nextInt(int origin, int bound) returns a pseudorandomly chosen int in the range [origin, bound) — i.e., origin is inclusive, bound is exclusive. It is specified to produce a uniform distribution over that range (assuming the generator’s nextInt() is uniform). Throws IllegalArgumentException if origin >= bound. [1]
Related contract points (same interface):
nextInt(int bound)returns a value in [0, bound) and throwsIllegalArgumentExceptionifbound <= 0. [2]- The interface’s default implementations may call
nextInt()multiple times (“rejection sampling”) to achieve uniformity. [1]
Sources: Oracle JDK API docs for RandomGenerator (Java SE 25) [1] and (Java SE 21) [2].
🏁 Script executed:
# Look for test files to understand how these rules are used and tested
fd "RandomGenerator" --extension java | grep -i testRepository: PicnicSupermarket/error-prone-support
Length of output: 303
🏁 Script executed:
# Check for any Refaster documentation or guards in the codebase
rg "Precondition|Guard" error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ -B 2 -A 2 | head -40Repository: PicnicSupermarket/error-prone-support
Length of output: 5073
🏁 Script executed:
# Look for how parameters are used in other Refaster rules to understand evaluation semantics
rg "@BeforeTemplate" -A 5 error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ | head -60Repository: PicnicSupermarket/error-prone-support
Length of output: 6559
🌐 Web query:
Refaster parameter evaluation multiple occurrences semantics double evaluation
💡 Result:
In Refaster, a @BeforeTemplate method parameter is an expression variable: it binds to (i.e., captures) the matched AST expression from user code, and Refaster generates the replacement by substituting that captured expression into the @AfterTemplate. [1]
As a result, if you reference the same parameter multiple times in @AfterTemplate, Refaster will duplicate the captured expression multiple times in the generated code—so any side effects or expensive work in that expression can be evaluated multiple times after the rewrite (macro-like behavior), unless you structure the @AfterTemplate to avoid that. [1][2]
Example (illustrative):
- If
xmatchescompute()and your@AfterTemplateusesxtwice, the output will containcompute()twice.
Sources: Refaster substitution description and “expression variables” semantics [1], Refaster template parameter-as-“any expression” explanation [2].
[1] https://research.google.com/pubs/archive/41876.pdf
[2] https://errorprone.info/docs/refaster
🏁 Script executed:
# Check the test files to see what cases are tested
cat error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/RandomGeneratorRulesTestInput.javaRepository: PicnicSupermarket/error-prone-support
Length of output: 1507
🏁 Script executed:
# Check the test output to see expected transformations
cat error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/RandomGeneratorRulesTestOutput.javaRepository: PicnicSupermarket/error-prone-support
Length of output: 1408
🏁 Script executed:
# Look for any discussion about these rules or overflow concerns
rg "nextInt.*origin.*bound|nextLong.*origin.*bound|nextDouble.*origin.*bound" -B 5 -A 5 | head -100Repository: PicnicSupermarket/error-prone-support
Length of output: 4244
🌐 Web query:
RandomGenerator nextInt(int origin, int bound) overflow IllegalArgumentException int addition wraparound
💡 Result:
In Java’s RandomGenerator.nextInt(int origin, int bound), an IllegalArgumentException is thrown only when origin >= bound (a plain signed int comparison), not because of arithmetic overflow in computing the range. The spec for RandomGenerator.nextInt(origin, bound) states it checks that origin is less than bound, then generates a uniformly distributed value in [origin, bound). [1]
The usual pitfall is computing the range as int n = bound - origin and then doing nextInt(n) + origin: if bound - origin overflows (wraps negative/zero), n is no longer a valid positive bound. The JDK’s reference algorithm accounts for this: it computes int n = bound - origin; and only uses nextInt(n) + origin when n > 0; otherwise (the “range not representable as int” case caused by overflow), it falls back to rejection sampling (nextInt() until origin <= r < bound), avoiding the wraparound-add bug while still honoring the requested interval. [2]
Sources:
[1] Java SE RandomGenerator API docs (nextInt(int origin, int bound) contract and exception) (docs.oracle.com)
[2] Java SE Random docs showing the reference algorithm with the n > 0 / “range not representable as int” fallback (docs.oracle.com)
Citations:
- 1: https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/util/random/RandomGenerator.html?utm_source=openai
- 2: https://docs.oracle.com/en/java/javase/20/docs/api/java.base/java/util/Random.html
Both parameter double-evaluation and overflow behavior concerns are valid; add guards or constraints.
In the after templates, a is referenced twice (a and a + b). If a matches an expression with side effects, Refaster will duplicate it, causing the expression to evaluate multiple times—changing program semantics.
Additionally, a + b can overflow for int/long types. In the before template, this naturally wraps; in the after template, if a + b overflows and violates the origin < bound precondition, nextInt(origin, bound) throws IllegalArgumentException instead of wrapping, changing behavior.
Prevent these issues by guarding a and b to be stable, non-side-effect expressions (or introduce placeholder/guard restrictions) and ensure a + b cannot overflow (e.g., constrain ranges or use explicit bounds checks).
🤖 Prompt for AI Agents
In
`@error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/RandomGeneratorRules.java`
around lines 82 - 123, The after-templates in RandomGeneratorNextIntWithOrigin,
RandomGeneratorNextDoubleWithOrigin, and RandomGeneratorNextLongWithOrigin
duplicate the expression `a` (risking double evaluation) and can cause overflow
for `a + b` (changing wrapping semantics to exceptions); add Refaster guards so
these rules only apply when `a` is a stable/pure expression (no side effects or
method calls) and when `a + b` cannot overflow: for
RandomGeneratorNextIntWithOrigin require b >= 0 and a <= Integer.MAX_VALUE - b;
for RandomGeneratorNextLongWithOrigin require b >= 0 and a <= Long.MAX_VALUE -
b; for RandomGeneratorNextDoubleWithOrigin require Double.isFinite(a) &&
Double.isFinite(b) && b >= 0 && Double.isFinite(a + b); implement these as guard
methods (e.g., isStable(a), nonNegative(b), noOverflowInt(a,b),
noOverflowLong(a,b), finiteDoubleBounds(a,b)) referenced from the rule classes
so the refactoring is only applied when safe.
|
Looks good. No mutations were possible for these changes. |
rickie
left a comment
There was a problem hiding this comment.
Congrats on your PR @szumal ! Looks good to me :)
Added a small commit with a reordering of the rules. One thing we could still consider is moving the int related ones as first ones. I think that's slightly more inline with how we usually have that. CC: @Stephan202
| } | ||
|
|
||
| int testRandomGeneratorNextIntWithOrigin() { | ||
| return 20 + new Random().nextInt(80); |
There was a problem hiding this comment.
We usually use even simpler test values :)
|
Looks good. No mutations were possible for these changes. |
52e450c to
f69848f
Compare
|
Looks good. No mutations were possible for these changes. |
f69848f to
0ae77e0
Compare
There was a problem hiding this comment.
Rebased and added a commit.
Suggested commit message:
Introduce `RandomGeneratorNext{Double,Int,Long}WithOrigin` Refaster rules (#2087)
Resolves #2047.
I'll let @rickie have a final look at the changes and then merge.
One thing we could still consider is moving the
intrelated ones as first ones. I think that's slightly more inline with how we usually have that. CC: @Stephan202
Perhaps we should, but the current order matches what is already there, which is lexicographical. Let's keep it as-is for now; we may even settle on this in more place.
| /** | ||
| * Prefer {@link RandomGenerator#nextDouble(double origin, double bound)} over more contrived | ||
| * alternatives. | ||
| */ |
There was a problem hiding this comment.
I would not say that the alternative is more contrived. But in case of large origin or bound, the replacement code is more likely to yield a correct value distribution, or throw an IllegalArgumentException in case of illogical bounds.
|
|
||
| @AfterTemplate | ||
| double after(RandomGenerator random, double a, double b) { | ||
| return random.nextDouble(a, a + b); |
There was a problem hiding this comment.
Let's document CodeRabbit's feedback about a having side-effects or being expensive to compute.
| /** | ||
| * Prefer {@link RandomGenerator#nextInt(int origin, int bound)} over more contrived alternatives. | ||
| */ |
There was a problem hiding this comment.
Here too there are more substantial benefits: the replacement code won't silently cause overflow.
| import com.sun.source.tree.UnaryTree; | ||
|
|
||
| /** A matcher of expressions that may a non-trivial amount of computation. */ | ||
| /** A matcher of expressions that may require a non-trivial amount of computation. */ |
There was a problem hiding this comment.
I noticed this while briefly considering adding @NotMatches(RequiresComputation.class).
RandomGeneratorNext{Double,Int,Long}WithOrigin Refaster rules
|
Looks good. No mutations were possible for these changes. |
rickie
left a comment
There was a problem hiding this comment.
Second time approval, nice changes @Stephan202 !
0ae77e0 to
1fddfbd
Compare
|
Looks good. No mutations were possible for these changes. |
|



#2047