Skip to content
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.

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

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

Closed
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
23 tasks
@tarcieri
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants