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

RFC: Add Option::filter to the standard library #2124

Merged
merged 5 commits into from
Nov 8, 2017
Merged

RFC: Add Option::filter to the standard library #2124

merged 5 commits into from
Nov 8, 2017

Conversation

LukasKalbertodt
Copy link
Member

@LukasKalbertodt LukasKalbertodt commented Aug 24, 2017

Add the method Option::filter<P>(self, predicate: P) -> Option<T> to the standard library. This method makes it possible to easily throw away a Some value depending on a given predicate. The call opt.filter(p) is equivalent to opt.into_iter().filter(p).next().

assert_eq!(Some(3).filter(|_| true)), Some(3));
assert_eq!(Some(3).filter(|_| false)), None);
assert_eq!(None.filter(|_| true), None);

Rendered


I certainly know that there are plenty of active, more important RFCs. But since this RFC only introduces a tiny change to the standard library, I figured I should try writing it.

Fixes #1485

Copy link
Member

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

Just some minor nitpick ^_^

text/0000-option-filter.md Outdated Show resolved Hide resolved
text/0000-option-filter.md Outdated Show resolved Hide resolved
@scottmcm scottmcm added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Aug 24, 2017
@CAD97
Copy link

CAD97 commented Aug 25, 2017

One minor downside: including filter means that since we have map and filter, someone might expect filter_map as well, which is and_then for Option. (I'm still 👍 though; I want this, and didn't know about the crate.)

@ticki
Copy link
Contributor

ticki commented Aug 26, 2017

In all fairness, filter_map probably had been a better choice, but there's no way it's going to change now.

@egilburg
Copy link

egilburg commented Aug 28, 2017

@ticki

Could add filter_map as an alternatively-named function doing the same thing, and either deprecate and_then, or leave it as an alternative, depending on whether the semantics imply flow control (and_then) or functional data manipulation (filter_map).

Or is this kind of function aliasing discouraged in Rust? It's pretty common in languages like Ruby, e.g. #collect and #map do the same thing.

@psFried
Copy link

psFried commented Aug 30, 2017

There's been at least a few times that I've started to type .filter(... only to remember that Rust options don't have a filter method. I for one would really appreciate having it in the standard library.

@Ryan1729
Copy link

Ryan1729 commented Sep 1, 2017

I like this idea. If this is done then it seems like filter and filter_err should be added to Result as well.

EDIT: Thought about it a bit more and I guess in the Result<T, E> case you'd have to provide filter with an E, and analogously, you'd need to provide filter_err with an T. This suggests variants that take a closure as well (filter_else and filter_err_else?) Now I'm no longer sure the Result extension makes sense.

@LukasKalbertodt
Copy link
Member Author

@Ryan1729 I thought about a Result<T, E> equivalent, too, but it's less clear how something like that would look. As a simple example (which is very similar to code I wrote yesterday):

fn parse(input: &str) -> Result<Ast, Error> {}
fn generate(ast: Ast) -> Result<String, Error> {}

parse(input)
    .and_then(generate)

Now I wanted to add another step which verifies the AST. It doesn't produce a result, but can produce an error. So the signature looks like this:

fn check(ast: &Ast) -> Result<(), Error> {}

Today, I think there is no better way to integrate it into my code than:

parse(input)
    .and_then(|ast| {
        check(&ast).map(|_| ast)
    })
    .and_then(generate)

So that's not particularly nice, but maybe ok-ish. I think I encountered this particular situation not nearly as often as situations in which Option::filter() would be useful. So since this is a lot less clear, the RFC only talks about Option::filter().

@kennytm
Copy link
Member

kennytm commented Sep 3, 2017

@LukasKalbertodt If catch is stable, it would be clearer to write this as:

let string: Result<String, Error> = catch {
    let ast = parse(input)?;
    check(&ast)?;
    generate(ast)?
};

@de-vri-es
Copy link

de-vri-es commented Oct 8, 2017

Would it make sense to call it and_if instead of filter? That matches better with the the current naming scheme of and, and_then, or and or_else.

and_if is also much more descriptive in the context of control flow.

@Centril
Copy link
Contributor

Centril commented Oct 19, 2017

Haskell calls this method mfilter (monadic filter) - so it seems reasonable to call it filter when this is seen from the FP-perspective. When and if we get HKTs, this could be part of a MonadPlus trait.

I guess .and_if() gives more of an imperative feeling to it - which some, but not me personally, might like.

@SuperCuber
Copy link

I agree with the .and_if() suggestion - it is more consistent with what we have and it is what I tried when searching for such a method. I searched for .if() but obviously that's a keyword, so .and_if() seems like the next best thing.

@LukasKalbertodt
Copy link
Member Author

LukasKalbertodt commented Oct 22, 2017

I tried to find the name for this method in other languages:

  • Haskell: mfilter
  • Scala: filter
  • C++17: no such method (right?)
  • boost::optional: no such method (right?)
  • Idris: no such method (right?)
  • Nim: filter
  • Java8: filter
  • Swift: no such method (right?)

(if you comment with information about other languages, I will add those here)


Comparing both suggestions (with all points I could come up with):

AdvantagesDisadvantages
filter
  • Same as in most other languages
  • Same as in Iterator
and_if
  • Fits other methods of Option more closely
  • More descriptive in the context of control flow
  • Name is not well known in the FP world (right?)

(again, if you raise other points below, I'll add them)

@Centril
Copy link
Contributor

Centril commented Oct 23, 2017

@LukasKalbertodt .and_if, while being "More descriptive in the context of control flow" is less descriptive as a computation on data than .filter is.

In other words, .and_if describes how it does X, .filter describes what X is. This is the distinction between imperative and declarative programming where the .filter is declarative.

@glaebhoerl
Copy link
Contributor

We'd probably prefer "retain" over "filter" for consistency with other containers like Vec.

@scottmcm
Copy link
Member

@glaebhoerl Isn't retain in-place, though?

@glaebhoerl
Copy link
Contributor

Hmm, could be. But my assumption is that the reason it wasn't named "filter" is the ambiguity around whether that means "filter-in" or "filter-out", which applies equally here. (For the record, I find the name "retain" kind of awkward and might prefer "filter" myself despite the ambiguity, in a vacuum, but again, consistency.)

@kennytm, the downvoting of comments without explanation feels kind of obnoxious to me, could you please write a comment instead if you have a disagreement? Thank you.

@Centril
Copy link
Contributor

Centril commented Oct 23, 2017

@glaebhoerl A method/function named filter should always be "filter-in" (keep those where the predicate matches) and never "filter-out". Is there any large language this does apply to?

I think we should pick the function name most people would intuitively just write without checking any docs, and in this respect I think filter is the clear winner and retain the clear loser as @psFried shows.

@LukasKalbertodt The general function for filtering iterables is called "filter" in python. While this is not the same as Option::filter, it is the filter operation for a different instance of MonadPlus wherefore it is relevant.

@glaebhoerl
Copy link
Contributor

I agree in that personally I think "filter" should be taken to mean filter-in, but I'm also not a representative sample, because I first learned filter in a language (Haskell) where it does, in fact, mean that. So, of course I would think that. So I'm more conveying concerns from others I've heard around the web on multiple occasions about the potential for confusion with "filter" (one recent example just as a proof-of-existence), rather than stating my own opinion about it.

Does anyone know what the motivation behind the name retain actually was, historically? The "filter" ambiguity issue, in-place versus copying as @scottmcm suggests, or something else entirely?

@Centril
Copy link
Contributor

Centril commented Oct 23, 2017

@glaebhoerl I think

'filter' has had a well-defined meaning for decades. Save your rage for the people who define it wrong.

in that twitter feed is right. There might be a few anecdotal examples - but are they, put together, notable?

Haskell, is hardly alone in defining filter as filter-in. Other languages where it is filter-in:

There are a bunch of other languages where it is called grep (groovy), select (ruby) where it is still filter-in.

I'd like an example of a language where it is filter-out.

@dtolnay dtolnay self-assigned this Nov 8, 2017
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks for the RFC! This is a small enough addition that I think we can go ahead with an implementation. Please open a tracking issue in rust-lang/rust and send a PR containing the implementation, fully documented with rustdoc, and an #[unstable] attribute linking to the tracking issue -- see rust-lang/rust#44682 for an example.

@CAD97
Copy link

CAD97 commented Nov 8, 2017

@dtolnay see rust-lang/rust#44996, which has been closed on hold until further discussion of the RFC:

aidanhs commented on Oct 5

Thanks for the PR LukasKalbertodt! I've set sfackler as the reviewer for the PR but have set it as "with author" for now until the RFC gets merged - assuming it does, we can than get this reviewed and merged.

aidanhs commented on Oct 12

In fact, I think I'll close this PR for now as I don't see the RFC being merged in the next week - once it is, please ping me and I'll reopen and we can get your PR reviewed :)

(this is just so PR triagers aren't constantly visiting this PR and checking to see where the RFC is up to)

@LukasKalbertodt
Copy link
Member Author

@dtolnay As much as I'd like this RFC to be accepted immediately, I have to say: I don't think the name debate is settled, really. There are a few alternative names and I think and_if() is a serious alternative (given that Option has many methods with names different from their monadic/FP counterparts).

Or do we want to discuss the naming issue after implementing and before stabilizing it?

@dtolnay
Copy link
Member

dtolnay commented Nov 8, 2017

Thanks folks! It appears I was biased by landing on this RFC after trying to write filter on an Option.

In my experience we discuss naming before merging a new API, but only enough so that we don't get it blatantly wrong. Then we debate it more rigorously as part of FCP to stabilize. I think the logic there is that 1 month spent writing code against an API is more valuable toward deciding whether it is named correctly, compared to 1 month spent academizing about the name without writing code against it. One recent example of this is hint_core_should_pause, merged here with a brief naming discussion and fcp with rename here. Another example is cmp::Reverse, merged here with a brief naming discussion and FCP here with a rename considered but not done.

Other than the name, it seems like there is agreement that we want this functionality and the method signature in the RFC is the correct one. Is that your impression as well?

@LukasKalbertodt do you feel that there could be strong arguments in favor of either name that haven't been voiced yet? Based on the existing arguments, what name are you leaning toward now and why?

@LukasKalbertodt
Copy link
Member Author

@dtolnay

Other than the name, it seems like there is agreement that we want this functionality and the method signature in the RFC is the correct one. Is that your impression as well?

Yip.

@LukasKalbertodt do you feel that there could be strong arguments in favor of either name that haven't been voiced yet? Based on the existing arguments, what name are you leaning toward now and why?

I don't expect any more super important arguments, to be honest. And about my preference: I'm really not sure! I thought filter() would be the obvious choice (given the same method on Iterator), but and_if() would fit the existing naming scheme better (I think).

I'm absolutely fine with merging the RFC now and discussing the name on the tracking issue :)

@dtolnay
Copy link
Member

dtolnay commented Nov 8, 2017

Sounds good to me!

@dtolnay dtolnay merged commit bedc7b2 into rust-lang:master Nov 8, 2017
kennytm added a commit to kennytm/rust that referenced this pull request Nov 10, 2017
…r=dtolnay

Add `Option::filter()` according to RFC 2124

(*old PR: rust-lang#44996)

This is the implementation of [RFC "Add `Option::filter` to the standard library"](rust-lang/rfcs#2124). Tracking issue: rust-lang#45860

**Questions for code reviewers:**

- Is the documentation sufficiently long?
- Is the documentation easy enough to understand?
- Is the position of the new method (after `and_then()`) a good one?
@vitiral
Copy link

vitiral commented Nov 15, 2017

@LukasKalbertodt the rendered link is now broken, can you fix it?

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.