Bump rand, mt19937; fix associated build failures#7800
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWorkspace RNG deps upgraded (getrandom→0.4, mt19937→3.3, rand→0.10). Stdlib switched from workspace rand_core to rand. random.rs now seeds MT19937 from the thread RNG. WASM target getrandom features added and a test import updated. ChangesRandom Number Generation Dependency Upgrade
Sequence Diagram(s)sequenceDiagram
participant PyRandom
participant ThreadRNG as thread::rng()
participant MT as MT19937
PyRandom->>ThreadRNG: request RNG
ThreadRNG-->>PyRandom: provide RNG
PyRandom->>MT: MT19937::from_rng(&mut rng)
MT-->>PyRandom: seeded MT19937
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Cargo.toml (1)
261-261: 💤 Low value
sys_rngfeature is redundant with rand 0.10 defaults.
thread_rngis a default feature ofrand0.10 and already impliessys_rng. Listingsys_rngexplicitly does not break anything, but it is unnecessary noise.🔧 Simplified dependency
-rand = { version = "0.10", features = ["sys_rng"] } +rand = { version = "0.10" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Cargo.toml` at line 261, The rand dependency declaration includes an explicit features entry for "sys_rng" which is redundant with rand 0.10 defaults; update the Cargo.toml rand entry (the dependency named "rand") by removing the features = ["sys_rng"] clause so it relies on the default features (e.g., use rand = "0.10" or remove the features array), keeping the crate name "rand" intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/stdlib/src/random.rs`:
- Line 18: Import the correct trait so SeedableRng::from_rng can use an RngCore
source: replace the current import use rand::{Rng, SeedableRng} with use
rand::{RngCore, SeedableRng} (so MT19937::from_rng / SeedableRng::from_rng has
the required RngCore bound); no changes needed for MT19937::next_u32 since that
is a concrete method.
---
Nitpick comments:
In `@Cargo.toml`:
- Line 261: The rand dependency declaration includes an explicit features entry
for "sys_rng" which is redundant with rand 0.10 defaults; update the Cargo.toml
rand entry (the dependency named "rand") by removing the features = ["sys_rng"]
clause so it relies on the default features (e.g., use rand = "0.10" or remove
the features array), keeping the crate name "rand" intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e2351995-7f8a-459f-b8a7-0eb1680752b2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlcrates/stdlib/Cargo.tomlcrates/stdlib/src/random.rs
| use mt19937::MT19937; | ||
| use num_traits::{Signed, Zero}; | ||
| use rand_core::{RngCore, SeedableRng}; | ||
| use rand::{Rng, SeedableRng}; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what rand/RngCore/Rng methods are called in random.rs
rg -n "\.next_u32\|\.next_u64\|\.fill_bytes\|\.gen\b\|\.random\b\|rand::Rng\|rand::RngCore\|rand_core" crates/stdlib/src/random.rsRepository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Check if file exists and get the imports section
head -30 crates/stdlib/src/random.rsRepository: RustPython/RustPython
Length of output: 851
🏁 Script executed:
# Search more broadly for any 'use' statements and RNG variable usage
rg -A 5 -B 5 "use rand" crates/stdlib/src/random.rsRepository: RustPython/RustPython
Length of output: 387
🏁 Script executed:
# Look for where the RNG object is actually being used
rg -n "rng\|Rng\|RngCore" crates/stdlib/src/random.rsRepository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Read the full file to understand RNG usage
cat -n crates/stdlib/src/random.rs | head -100Repository: RustPython/RustPython
Length of output: 4288
🏁 Script executed:
# Search for method calls on 'rng' variable without escaping
rg -n "\brng\." crates/stdlib/src/random.rsRepository: RustPython/RustPython
Length of output: 530
🏁 Script executed:
# Check what MT19937 implements - search documentation or trait bounds
rg -n "MT19937" crates/stdlib/src/random.rs | head -20Repository: RustPython/RustPython
Length of output: 249
🏁 Script executed:
# Check if there are any compiler errors or clippy warnings in the repo
# Try to find cargo check or build output
cd crates/stdlib && cargo check 2>&1 | grep -A 5 -B 5 "random\|Rng\|error\|warning" | head -50Repository: RustPython/RustPython
Length of output: 151
🏁 Script executed:
# Search for MT19937::from_rng usage and documentation in mt19937 crate
rg -A 2 "from_rng" crates/stdlib/src/random.rsRepository: RustPython/RustPython
Length of output: 160
🏁 Script executed:
# Check what trait MT19937 implements by searching for impl statements
rg "impl.*MT19937\|impl.*for.*MT19937" --type rustRepository: RustPython/RustPython
Length of output: 47
🌐 Web query:
mt19937 Rust crate from_rng method parameter trait
💡 Result:
In the rand_core-based Rust ecosystem, the MT19937 generator’s “from_rng” style constructor is exposed via the rand_core::SeedableRng trait as: fn from_rng<R: RngCore>(rng: R) -> Result<Self, Error> So the parameter is a type parameter R constrained by rand_core::RngCore (an RNG producing values without error), passed by value (rng: R) and returns Result<Self, Error>. This is the generic trait signature in rand_core’s SeedableRng: [1]. Common related (infallible) variant in rand_mt: rand_mt’s Mt implements a method with signature: fn from_rng(rng: &mut R) -> Self where R: Rng +?Sized Here the parameter is &mut R, where R: rand::Rng (the infallible high-level RNG trait), and the method returns Self directly. [2] For the “mt19937” crate specifically (coolreader18/hiroshima-univ translation), its source shows implementations of rand_core::TryRng and rand_core::SeedableRng for MT19937 (but the exact from_rng signature there follows the rand_core::SeedableRng trait; the trait provides from_rng generically). [3][4] Therefore, when you ask “mt19937 Rust crate from_rng method parameter trait”, the trait bound for from_rng is R: RngCore in rand_core::SeedableRng. [1] If instead you are referring to rand_mt’s Mt::from_rng convenience API, the parameter uses &mut R where R: rand::Rng +?Sized. [2]
Citations:
- 1: https://docs.rs/rand_core/0.4.0/rand_core/trait.SeedableRng.html
- 2: https://docs.rs/rand_mt/latest/rand_mt/struct.Mt.html
- 3: https://docs.rs/mt19937/latest/src/mt19937/lib.rs.html
- 4: https://docs.rs/rand_mt/latest/rand_mt/
🏁 Script executed:
# Check if RngCore is imported elsewhere or re-exported
rg "RngCore\|rand_core" crates/stdlib/src/random.rsRepository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Check if there's a re-export of RngCore in the rand crate imports
rg "use rand" crates/stdlib/src/random.rs -A 1 -B 1Repository: RustPython/RustPython
Length of output: 185
🏁 Script executed:
# Try to check Cargo.toml to see rand version and features
cat crates/stdlib/Cargo.toml | grep -A 5 -B 5 "rand"Repository: RustPython/RustPython
Length of output: 325
🏁 Script executed:
# Search for any build output or error logs
find . -name "*.log" -type f 2>/dev/null | head -5Repository: RustPython/RustPython
Length of output: 47
Replace Rng with RngCore in the import statement.
The MT19937::from_rng() call at line 62 uses the SeedableRng::from_rng() trait method, which requires a generic parameter bound by RngCore, not Rng. The current import (use rand::{Rng, SeedableRng}) is missing RngCore and cannot satisfy the trait bound for from_rng(). While next_u32() at line 82 is a concrete method on MT19937 itself (not requiring a trait import), the trait-based from_rng() method cannot be properly used without RngCore in scope.
Proposed fix
- use rand::{Rng, SeedableRng};
+ use rand_core::RngCore;
+ use rand::SeedableRng;or equivalently:
- use rand::{Rng, SeedableRng};
+ use rand::{SeedableRng};
+ use rand_core::RngCore;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/stdlib/src/random.rs` at line 18, Import the correct trait so
SeedableRng::from_rng can use an RngCore source: replace the current import use
rand::{Rng, SeedableRng} with use rand::{RngCore, SeedableRng} (so
MT19937::from_rng / SeedableRng::from_rng has the required RngCore bound); no
changes needed for MT19937::next_u32 since that is a concrete method.
d25758a to
a60c45b
Compare
e363af0 to
ef88086
Compare
|
Seems relevant to the |
149112d to
723e4ae
Compare
Supercedes: RustPython#7716 Bumping rand and mt19937 is not trivial, so Dependabot's PR causes build failures. `getrandom` cannot be bumped yet. RustPython's crate graph has several old versions of `getrandom`. Apparently, this is causing a conflict with the WebAssembly build.
723e4ae to
ee86de1
Compare
|
Uh, I give up on this for now. 😓 |
Supercedes: #7716
Bumping rand and mt19937 is not trivial so Dependabot's PR causes build failures.
Update: Draft until I figure out why the WebAssembly build is failing. The rest of it is working, though.I reverted thegetrandombump. Apparently, the new version is conflicting with version 0.3 which is pulled in by other dependencies.Summary by CodeRabbit
Chores
Bug Fixes / Behavior Change