Skip to content

consider for new epoch: rand::sample's API should not be infalible #46163

Closed
@vitiral

Description

@vitiral

I'm not sure where to put this, as it would be a breaking change and clearly it would need something like a new epoch to make happen, but I thought it would be worth mentioning.

Most rust operations are fallible if they don't make sense, either through a panic or by returning an Option or Result. However, rand::sample has the following API:

pub fn sample<T, I, R>(rng: &mut R, iterable: I, amount: usize) -> Vec<T> where
    I: IntoIterator<Item = T>,
    R: Rng, 

Randomly sample up to amount elements from a finite iterator.

This function will silently return less than amount if the size of I is less than amount. I would say this is very odd, and returning Option<Vec<T>> would have been a better design choice. You can imagine the confusion if you did rand::sample(rng, v, 100) but then only had 2 elements!

If it were changed to returning None if I.len() < amount then the current behavior could still be accomplished with:

rand::sample(&mut rng, vec.iter(), usize::min(amount, vec.len())).unwrap()

With the current behavior, user has to put guards around their code (which is not required by the compiler) to prevent getting less than amount, which is unusual in rust.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions