Skip to content

Rename Rng -> RngExt, RngCore -> Rng #1288

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
dhardy wants to merge 1 commit into from
Closed

Rename Rng -> RngExt, RngCore -> Rng #1288

dhardy wants to merge 1 commit into from

Conversation

@dhardy
Copy link
Member

@dhardy dhardy commented Feb 20, 2023

For review, as mentioned in #1273. Effects:

  • RNG implementations get nicer trait names: Rng, CryptoRng, BlockRngCore, CryptoBlockRng. Eh, this still isn't quite right. But we have struct BlockRng, thus must keep trait BlockRngCore.
  • User code now uses a mix of (optionally) Rng and (at least sometimes) RngExt.

My opinion is now against this change:

  • Previously, most usage of RNGs only needed to use one trait, Rng. (In part this is because it's rarely necessary to call RngCore methods directly; in part it's because any R: Rng also satisfies R: RngCore and vice-versa.) Now, user-code tends to use both (generics over R: Rng and you need RngExt::gen or RngExt::gen_range or ..).
  • It's significant amounts of breakage for both RNG impls and users.

Still, I'll leave it here in case anyone wishes to review the changes. (Note that a couple of simplifications could be made without the trait rename; e.g. replacing R: RngCore with R: Rng in some places.)

@dhardy
Copy link
Member Author

dhardy commented Mar 23, 2023

R: Rng + Sized is exactly equivalent to R: RngCore + Sized (prior to this rename).

This is not the case when using virtual dispatch: dyn RngCore implies a vtable over methods next_u32, next_u64, fill_bytes, try_fill_bytes (intended usage). But dyn Rng implies a vtable over gen_bool, gen_ratio` (all other methods are generic and hence excluded).

So if we do not use this rename:

  • RNG impls only use RngCore
  • RNG users can mostly only use Rng
  • RNG users may need dyn RngCore

If we do use this rename:

  • RNG impls only use RngCore
  • RNG users only use R: Rng and dyn Rng, but...
  • RNG users very often need to import RngExt (from prelude or explicitly)

@dhardy
Copy link
Member Author

dhardy commented Oct 31, 2023

Does anyone have further thoughts on this, or shall we just reject it? (Primary rationale: it's a big breaking change for little gain.)

@vks @newpavlov @tarcieri and anyone else who wishes to comment (open discussion)

@dhardy dhardy mentioned this pull request Oct 31, 2023
Closed
24 tasks
@tarcieri
Copy link
Contributor

tarcieri commented Oct 31, 2023

I think I'm happy with just #1273 which will allow us to migrate from CryptoRngCore -> CryptoRng everywhere

@dhardy dhardy closed this Nov 19, 2023
@dhardy dhardy mentioned this pull request Apr 3, 2024
Merged
This was referenced Oct 18, 2025
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dhardy @tarcieri