Skip to content

Let Fill target element types; move to rand_core; support min_specialization #1651

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

Closed
wants to merge 11 commits into from

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Jul 30, 2025

  • Added a CHANGELOG.md entry

Summary

Motivation

This trait was added to fill a capability gap: a safe interface for fast filling of slices like [i16]. The impls for [bool], [f32] etc. are extra complexity beyond this and unnecessary since they offer no benefit over element-wise generation in user-code.

Further, we can now support specialization like this:

struct MyRng;

impl RngCore for MyRng {
    // [method impls omitted]
}

#[cfg(feature = "min_specialization")]
impl Fill<MyRng> for u64 {
    fn fill_slice(this: &mut [Self], rng: &mut MyRng) {
        todo!()
    }
}

Alternatives

The specialization option motivated removal of support for element-wise types ([f32] etc.), moving to rand_core and moving the generics to the trait. I don't think any of these are bad changes however.

Via RngCore

We could add fill_u32_slice, fill_u64_slice to RngCore. Adding these methods would be more disruptive, but possibly more useful overall (dyn trait support, no dependence on unstable features).

Forget these specializations

... there's no strong evidence we need them.

These impls contradict the doc that fn fill is implemented
for types which may be reinterpreted as [u8].
@dhardy
Copy link
Member Author

dhardy commented Jul 30, 2025

Note: it might seem more natural to reverse the generics and parameters of Fill to this:

pub trait Fill<T> {
    fn fill_slice(&mut self, slice: &mut [T]);
}

Indeed, it is more natural. Unfortunately it does not support external impls for externally-defined types like MyInt over any R: RngCore + ?Sized: orphan rules would require that R be covered by another type. This is due to parameter ordering (see orphan rules).

@newpavlov
Copy link
Member

Wouldn't it be better to rely on zerocopy::FromBytes for this? On the first glance, moving Fill to rand_core does not look like a good solution to me.

@dhardy dhardy force-pushed the push-wvwwyutpkynn branch from 8867221 to c13c5a2 Compare July 30, 2025 15:37
@dhardy
Copy link
Member Author

dhardy commented Jul 30, 2025

Wouldn't it be better to rely on zerocopy::FromBytes for this? On the first glance, moving Fill to rand_core does not look like a good solution to me.

Aside from #1574, that wouldn't support RNG specializations. It also would require unsafe code to implement for user-defined types (though some impls will require that anyway).

@dhardy
Copy link
Member Author

dhardy commented Jul 30, 2025

What do you not like about moving Fill to rand_core? This would prevent (or make it harder to) add element-wise impls for f32 etc. in the future, but I don't think we want that (at least, so far we haven't bothered with float-specific RNGs like xoshiro256+).

@newpavlov
Copy link
Member

newpavlov commented Jul 30, 2025

I don't think RNG specialization is that important. The only difference between filling [u8; 256] instead of [u32; 64] for an RNG which internally generates u32s is a matter of unaligned stores and even that in most cases should be easily optimized out by the compiler.

It also would require unsafe code to implement for user-defined types

No, if user has successfully derived FromBytes for a custom type, then no unsafe code would be needed (obviously, in user code, not in general).

What do you not like about moving Fill to rand_core?

This feels like a wrong place for it. It goes against the goal of providing the fundamental RNG APIs.

@dhardy
Copy link
Member Author

dhardy commented Jul 30, 2025

This feels like a wrong place for it. It goes against the goal of providing the fundamental RNG APIs.

I agree except that I've been reconsidering what the fundamental RNG APIs should be. This is sort-of a part of that. The "sort-of" part is where this proposal falls flat though IMO since trait Fill doesn't (and shouldn't) replace RngCore::fill_bytes.


Do you think this Fill trait should replace the current one in rand?

@dhardy dhardy added the B-API Breakage: API label Jul 30, 2025
@newpavlov
Copy link
Member

Do you think this Fill trait should replace the current one in rand?

I haven't used Fill much in practice, so it's hard for me to say. One potential problem with a FromBytes-based Fill is that it could be somewhat error-prone, e.g. FromBytes is implemented for f32 and re-interpreting random bytes as f32 is probably not what most users want.

@dhardy
Copy link
Member Author

dhardy commented Aug 12, 2025

#1652 was merged instead.

@dhardy dhardy closed this Aug 12, 2025
@dhardy dhardy deleted the push-wvwwyutpkynn branch August 12, 2025 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-API Breakage: API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CHANGE: Allow Fill to be implemented for third-party types
2 participants