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

Choose should panic #924

Closed
mgostIH opened this issue Dec 30, 2019 · 7 comments
Closed

Choose should panic #924

mgostIH opened this issue Dec 30, 2019 · 7 comments

Comments

@mgostIH
Copy link

mgostIH commented Dec 30, 2019

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
Copy link
Member

dhardy commented Dec 30, 2019

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
Copy link
Author

mgostIH commented Dec 30, 2019

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
Copy link
Contributor

kazcw commented Jan 13, 2020

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
Copy link
Contributor

kazcw commented Jan 13, 2020

@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
Copy link
Collaborator

vks commented Jan 14, 2020

@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
Copy link
Member

newpavlov commented Jan 14, 2020

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 dhardy closed this as completed Jan 14, 2020
@dhardy
Copy link
Member

dhardy commented Jan 14, 2020

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.

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

No branches or pull requests

5 participants