Choose should panic

[mgostIH]

I think the API of SliceRandom::choose should panic instead of returning None to provide a cleaner API, since the case of having an empty slice seems very unlikely. Often times, even in other languages, I've seen random sampling used with already known arrays, but I can understand that there the model of error handling may be different, so I'd be happy to see more discussion about this.

[dhardy]

So you're saying that slice.choose(&mut rng).unwrap() is excessively verbose? Maybe a little, but I also don't find special-casing an iterator/slice algorithm for empty input is that rare.

Since this concerns a breaking change to an existing API, such a change would need quite a significant justification to be enacted.

[mgostIH]

Sure, but rand is still far from 1.0, so I thought about expressing this opinion before it'd be too late. I'd be up to check out what most crates that use choose treat the None case as, if the majority simply unwrap I don't see this as an unreasonable change.
Moreover even without implementation details, randomly choosing from an empty array seems completely a logic error, but even when you need it writing a match pattern on a None case doesn't really express anything about what is being checked.
Basically I think that:

match slice.choose(&mut rng){
    Some(v) => do_things_with_sample(v),
    None => do_other_thing();
}

Is much less expressive than

if !slice.is_empty() {
    do_things_with_sample(slice.choose(&mut rng));
}
else{
    do_other_thing();
}

[kazcw]

Even if the more subtle API were better (it's not), it wouldn't be worth a breaking change to a crate with 3000 direct dependents; SemVer doesn't enter into it.

[kazcw]

@mgostIH BTW, you can do this:

if let Some(choice) = slice.choose(&mut rng) {
    do_things_with_sample(choice);
}
else{
    do_other_thing();
}

Although the idiomatic way to do that example would be:

slice
    .choose(&mut rng)
    .map(do_things_with_sample)
    .unwrap_or_else(|| do_other_thing());

[vks]

@mgostIH I think your example is an argument for the current API, not against it. In my opinion, the example given by @kazcw using the current API is preferable, because it does not generate any panicking code. The advantage of the API you proposed is saving an unwrap in cases where you don't care about the failure case.

I would prefer to keep the API as is, for the following reasons:

• As already mentioned, changing it would introduce a lot of churn.
• The current API allows writing panic-free code.
• The current API is arguably more idiomatic Rust, mimicking corresponding APIs in std (see [T]::{first, last} and similar).

In my opinion, any of these outweigh the advantage of saving an unwrap in some cases.

[newpavlov]

I think we should look into how choose is used across ecosystem. If use of unwrap or expect is prevalent, then I think there is a certain merit in making choose to panic on empty slices. It can be done as part of #889. Of course such change should be accompanied by addition of try_choose method which will work as the current choose.

[dhardy]

Somehow @newpavlov commented just as I clicked the "close" button. However, I believe the above arguments are sufficient not to re-open this; in particular:

1. [@vks:] The current API is arguably more idiomatic Rust, mimicking corresponding APIs in std (see [T]::{first, last} and similar).
2. [myself:] Since this concerns a breaking change to an existing API, such a change would need quite a significant justification to be enacted.