Skip to content

Relax Sized requirements for blanket impls#1593

Merged
dhardy merged 2 commits into
rust-random:masterfrom
fjarri:sized
Feb 20, 2025
Merged

Relax Sized requirements for blanket impls#1593
dhardy merged 2 commits into
rust-random:masterfrom
fjarri:sized

Conversation

@fjarri

@fjarri fjarri commented Feb 18, 2025

Copy link
Copy Markdown
Contributor

Relaxes Sized bound on blanket impls for TryRngCore, TryCryptoRng, UnwrapErr, and UnwrapMut.

This came up when trying to pass a RngCore + ?Sized-bound argument to a method from a third-party library requiring TryRngCore + ?Sized.

I think this is not a breaking change, but not 100% sure.

@fjarri

fjarri commented Feb 18, 2025

Copy link
Copy Markdown
Contributor Author

Oh wait, I feel dumb, there's already #1592. But I add the UnwrapErr and UnwrapMut part.

@baloo

baloo commented Feb 18, 2025

Copy link
Copy Markdown
Contributor

I don't believe this a breaking change either, and a 0.9.2 would be appreciated if possible :)
Thanks a lot!

@dhardy dhardy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than UnwrapErr this looks fine... though possibly not very useful.

I release now there is something missing from #1589: reborrow support. Basically this method (see the RFC issue):

    /// Reborrow with a new lifetime
    ///
    /// Rust allows references like `&T` or `&mut T` to be "reborrowed" through
    /// coercion: essentially, the pointer is copied under a new, shorter, lifetime.
    /// Until rfcs#1403 lands, reborrows on user types require a method call.
    #[inline(always)]
    pub fn re<'b>(&'b mut self) -> UnwrapMut<'b, R>
    where
        'r: 'b,
    {
        UnwrapMut(self.0)
    }

We may also (or instead) want to publicly export UnwrapErr and UnwrapMut from rand.

@newpavlov thoughts?

Comment thread rand_core/src/lib.rs Outdated
Comment thread rand_core/CHANGELOG.md Outdated
@fjarri

fjarri commented Feb 18, 2025

Copy link
Copy Markdown
Contributor Author

Applied RFCs

though possibly not very useful.

My use case is passing an external RNG through a dyn-safe trait API. Then I might have a dyn RngCore argument that I want to pass to a method that takes dyn TryRngCore.

@newpavlov

Copy link
Copy Markdown
Member

This looks fine to me. Could you add tests which would cover your use case?

@fjarri

fjarri commented Feb 19, 2025

Copy link
Copy Markdown
Contributor Author

Added a test for RngCore -> TryRngCore. Should I add the same for CryptoRng and UnwrapMut?

@newpavlov

Copy link
Copy Markdown
Member

Yes, I think it's worth to have them to prevent potential future regressions.

@fjarri

fjarri commented Feb 19, 2025

Copy link
Copy Markdown
Contributor Author

Added the remaining tests

@dhardy dhardy merged commit 775b05b into rust-random:master Feb 20, 2025
@fjarri fjarri deleted the sized branch February 20, 2025 17:55
@baloo

baloo commented Feb 21, 2025

Copy link
Copy Markdown
Contributor

Could I ask for a 9.0.2 release of rand_core?

We're consuming the fixes here in crypto-bigint (RustCrypto/crypto-bigint#770)

Thanks a lot!

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.

4 participants