Conversation
szumal
force-pushed
the
szumal/random-generator-rule
branch
from
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. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| 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
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
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
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?
|
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 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
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. |
3 participants
#2047