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

Add methods for converting from bool to Option<T> #2757

Closed
wants to merge 6 commits into from

Conversation

varkor
Copy link
Member

@varkor varkor commented Sep 5, 2019

Rendered.

The following pattern is prevalent in typical Rust code.

if some_condition {
    Some(t)
} else {
    None
}

Here, I propose new methods for converting a bool to an Option<T>, abstracting this pattern, to aid in readability and reducing repetition.

impl bool {
    fn to_option<T>(self, t: T) -> Option<T>;
    fn to_option_with<T, F: FnOnce() -> T>(self, f: F) -> Option<T>;
}

This supersedes #2180, which was closed due to concerns about the suggested names and resolves #2606.

@varkor varkor added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Sep 5, 2019
@Centril Centril added A-impls-libstd Standard library implementations related proposals. A-primitive Primitive types related proposals & ideas T-lang Relevant to the language team, which will review and decide on the RFC. labels Sep 5, 2019
@Centril
Copy link
Contributor

Centril commented Sep 5, 2019

Adding the language team since the actual implementation of this requires the addition of a language item #[lang = "bool"]. This should also be noted in the reference text of the RFC.

@Centril
Copy link
Contributor

Centril commented Sep 7, 2019

@varkor I think it would also be good to add a few examples of before/after for real world use-cases, e.g. in the compiler or elsewhere, that would have benefited from this RFC.

I think it would also be worth noting that this encourages the use of more closures. This ostensibly has the drawback of longer compile-times. On the flip-side, it also is a good idea due to the restrictions imposed on imperative control flow (return & friends). Such restrictions may improve local reasoning and is helpful towards creating closures that may be refactored into stand-alone functions over time. So the RFC may be a boon for maintainability.

@varkor
Copy link
Member Author

varkor commented Sep 7, 2019

There's now a reference implementation at rust-lang/rust#64255.

@fbstj
Copy link
Contributor

fbstj commented Sep 7, 2019

I remember previous discussions on similar topics have regularly brought up the question of symmetry; aka "what about when self is false". is it something that should be mentioned/discussed in the RFC? is it future enhancement? is it worth adding the dual false.unthen() or should there be a false.not().then() or does !false.then() have the correct precedence (I believe not because it has to be looser than method calls for most things), or is (!false).then() acceptable?

@Centril
Copy link
Contributor

Centril commented Sep 7, 2019

@fbstj https://doc.rust-lang.org/nightly/std/ops/trait.Not.html -- we could add it to the prelude if necessary.

I think foo.not().then(...) or (!foo).then(...) is fine.

@varkor
Copy link
Member Author

varkor commented Sep 7, 2019

Very often you can rewrite a condition to avoid negation, and if that's not convenient (if the condition is used elsewhere, for instance), negation with ! or .not() seems no less readable, so I don't think we need extra methods for this.

@Centril
Copy link
Contributor

Centril commented Sep 7, 2019

..., so I don't think we need extra methods for this.

And on the off chance we do, we are free to add more methods later, so this feels very safe. ;)

Centril added a commit to Centril/rust that referenced this pull request Sep 7, 2019
Add methods for converting `bool` to `Option<T>`

This provides a reference implementation for rust-lang/rfcs#2757.
@troiganto
Copy link

troiganto commented Sep 7, 2019

I think the existing methods that are closest to the ones being proposed here are:

x.ok();  // Result<T, E> → Option<T>
x.err(); // Result<T, E> → Option<E>

x.ok_or(y);            // Option<T> → Result<T, E>
x.ok_or_else(closure); // Option<T> → Result<T, E>

x.and(y); x.and_then(closure); // Result<T, E> → Result<U, E>
x.or(y); x.or_else(closure);   // Result<T, E> → Result<T, F>

x.and(y); x.and_then(closure); // Option<T> → Option<U>
x.or(y); x.or_else(closure);   // Option<T> → Option<T>

I've noticed that the first four methods, which transform between different "monad kinds", explicitly mention the variant to transform to. The rest (and/and_then/or/or_else), on the other hand, stays within Option and Result respectively.

Based on this precedent, I think that methods which convert from bool to Option<T> should also contain the word "some" in their name, whatever the concrete choice is.

@varkor
Copy link
Member Author

varkor commented Sep 8, 2019

I've noticed that the first four methods, which transform between different "monad kinds", explicitly mention the variant to transform to.

Unfortunately, they're not that consistent. Both the methods from Option to Result and Result to Option are named after the Result (i.e. ok and err). This naming scheme in general does not extend well to other type combinations. I read some people state that some would be confusing, because it's either Some or None.

@ahicks92
Copy link

ahicks92 commented Sep 8, 2019

Seeing the name debate, I just want to ask why not to_option and to_option_with? I imagine this was discussed elsewhere, but in terms of "what does this do" when you don't know them, my_bool.then(5) is cryptic, my_bool.to_option(5) isn't. The former might have nice systemic math-like implications but the latter is very clear about what it does at least in my opinion.

@nugend
Copy link

nugend commented Sep 11, 2019

Sorry, I skimmed through the discussion and didn't see it, but has something along the the following lines been considered? I realize that this goes back to putting the method on the Option rather than the bool and I didn't see if that had been ruled out entirely as unworkable in the earlier RFC.

Some(x).test(boolvar) // Some(x) or None

I don't know if there's a general rule against using test as a function name though (didn't see it in the keyword list). I was thinking ok might work as well, but then there's a small chance of confusion with ok_or and ok_or_else due to the different return types.

Presumably a lazy version would be a bit verbose though, which might make it a non-starter

Some(()).test(boolvar).and_then(|_| expensive_expression)

@joshtriplett
Copy link
Member

I'm not sure why this is tagged T-lang; this is entirely a libs decision.

@joshtriplett joshtriplett removed the T-lang Relevant to the language team, which will review and decide on the RFC. label Sep 11, 2019
@joshtriplett
Copy link
Member

Ah, just saw @Centril's comment (which was marked as resolved).

For the sake of simplicity, I'd suggest treating the questions orthogonally; can we just ask if anyone on @rust-lang/lang objects to the creation of this lang item for bool, and if not, leave the rest of the decision entirely to T-libs?

[boolinator](https://docs.rs/boolinator/2.4.0/boolinator/)), but this functionality is commonly
desired and an obvious candidate for the standard library, where an external crate for such simple
functionality is not convenient.

Choose a reason for hiding this comment

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

Might as well mention if some_condition { t } as an alternative.

Copy link
Member Author

Choose a reason for hiding this comment

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

Inlining a method body is always a possible alternative, so mentioning it specifically here isn't useful.

Copy link
Member

@kennytm kennytm Nov 9, 2019

Choose a reason for hiding this comment

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

@varkor that does not mean inlining the method body, but to make if x { y } evaluates to Some(y) if x is true and None if x is false (basically an even more controversial version of https://internals.rust-lang.org/t/pre-rfc-break-with-value-in-for-while-loops/11208)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see. Unless there are serious proponents of that approach, I don't think it's worth including, as it seems like an unviable alternative to me.

@Centril
Copy link
Contributor

Centril commented Nov 11, 2019

@dtolnay I believe @varkor addressed your concerns, so ping. :)

I tend to agree with @SimonSapin that lang team already "authorized" adding inherent methods to primitive types; I think that the libs team ought to be generally trusted to add appropriate methods. If this requires adding a lang item (I think that would be for the inherent impl?), that doesn't seem like a big deal in and of itself to me.

Well I agree that this lang item in particular is not a big deal (because as @SimonSapin notes, we already have a bunch of those for primitive types, and so in terms of spec complexity there's nothing novel here). However, I think it's important that institutional hindrances exist against the addition of lang items, and the language team is a good check on that. That said, as for adding a non-method, I didn't make a point about teams -- just that I personally didn't think it was worth it -- and it seems like there's agreement on that, so that's great. :)

@dtolnay
Copy link
Member

dtolnay commented Nov 11, 2019

@rfcbot fcp cancel

@rfcbot
Copy link
Collaborator

rfcbot commented Nov 11, 2019

@dtolnay proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Nov 11, 2019
@dtolnay
Copy link
Member

dtolnay commented Nov 11, 2019

Proposing FCP to introduce unstable methods on bool for conditionally creating an Option::Some or running a closure.

This RFC has been unusual compared to other libs RFCs in that whether we accept these methods hinges on whether we can settle on sufficiently clear names for the behavior; it isn't like e.g. MaybeUninit where we know we need the functionality and we'll pick a name one way or another. As such, mostly the FCP is to confirm that we are interested in gathering experience using this set of names—if it seems to work out then we'll follow up in the tracking issue with a separate FCP to stabilize later.

impl bool {
    fn then_some<T>(self, t: T) -> Option<T> {
        if self {
            Some(t)
        } else {
            None
        }
    }

    fn then<T, F: FnOnce() -> T>(self, f: F) -> Option<T> {
        if self {
            Some(f())
        } else {
            None
        }
    }
}

This is the same functionality as from the earlier FCP in #2757 (comment), but new names as motivated in #2757 (comment).

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Nov 11, 2019

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Nov 11, 2019
@dtolnay
Copy link
Member

dtolnay commented Nov 11, 2019

@rfcbot concern :((

Registering withoutboats's concern from #2757 (comment).

@dtolnay
Copy link
Member

dtolnay commented Nov 11, 2019

@rfcbot resolve :((

@hadronized
Copy link
Contributor

hadronized commented Nov 12, 2019

Just to note, currently, I’m using the try-guard crate to do exactly that. I was happy to see that coming to std but it looks like it will not.

Just a thought to contribute. I think there are broadly two types of programmers. Let's call them combinator programmers and imperative programmers (I don't want to use the word functional here because it's not exactly functional programming that I mean). I think that code in one style seems bad to people in the other camp but that neither style is bad as such in itself.

Yes, I agree with that statement. However, as you said it, both styles are useful. I’m a Haskeller and use lots of combinators, but I also use lots of imperative statements. It’s not a “pick one; stick to it” decision. Especially, I use bool -> Option<T> / bool -> Result<T, E> to guard and verify pre-conditions and early-returns in functions. But for other common check, I tend to use if / else statements.

After reading the pro and cons arguments from this thread, I’m not sure what the best decision should be (I’m not saying that I’m against the decision of closing, I just haven’t made up my mind about how I feel about it). After all, we can still have those combinators in lib crates. The best comparison to me is, again, Haskell, because I know it a lot, but Haskell has that kind of functions in base but the Haskell ecosystem is also using a lot libraries such as either, errors, etc.

@SimonSapin
Copy link
Contributor

Proposing FCP to introduce unstable methods on bool for conditionally creating an Option::Some or running a closure.

Landing unstable methods has already happened in rust-lang/rust#64255 (tracked in rust-lang/rust#64260) so I’m not sure what this FCP is for. Deciding on names (or removal) is basically the only thing left to do.

@jplatte
Copy link
Contributor

jplatte commented Nov 19, 2019

As a datapoint, this feature turns out pretty useful in proc macros, and IMHO the current naming reads nicely: ruma/ruma-api@cb00188

@Coder-256
Copy link

Another use that would benefit from this is iterators (especially in long chains):

let x = iter.filter_map(|(foo, condition)| condition.then(foo));

The alternatives include:

let x = iter.filter_map(|(foo, condition)| {
    if condition {
        Some(foo)
    } else {
        None
    }
);

and:

let x = iter
    .filter(|(_, condition)| condition)
    .map(|(foo, _)| foo)

@dtolnay
Copy link
Member

dtolnay commented Dec 2, 2019

Landing unstable methods has already happened in rust-lang/rust#64255 (tracked in rust-lang/rust#64260) so I’m not sure what this FCP is for. Deciding on names (or removal) is basically the only thing left to do.

The most recent nightly still has then/then_with naming which I wouldn't want to stabilize (#2757 (comment)). @varkor: could you send a PR to rust-lang/rust with the then_some/then naming suggested in #2757 (comment)? I'll cancel the proposed FCP here and we can do an FCP to stabilize in the tracking issue.

@Centril
Copy link
Contributor

Centril commented Dec 6, 2019

The renaming PR has been sent to the bors queue. :)

bors added a commit to rust-lang/rust that referenced this pull request Dec 6, 2019
Rename `bool::then_*` to `bool::to_option_*` and use where appropriate

Name change following rust-lang/rfcs#2757. Also try it out throughout the compiler in places I think makes the code more readable.
@Centril
Copy link
Contributor

Centril commented Dec 6, 2019

...and @bors has merged the renaming PR. :)

@dtolnay
Copy link
Member

dtolnay commented Dec 11, 2019

Looks good to me. :) Thanks @varkor! We can follow up with an FCP to stabilize in rust-lang/rust#64260 after a few releases once we have some real world use cases outside of rustc to look at.

@dtolnay dtolnay closed this Dec 11, 2019
@dtolnay
Copy link
Member

dtolnay commented Dec 11, 2019

@rfcbot cancel

@rfcbot rfcbot removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Dec 11, 2019
@pnkfelix
Copy link
Member

I tried using these combinators for some code that looked like this:

let prev = if path.exists() {
    ThinLTOImportMaps::load_from_file(&path).ok()
} else {
    None
};

Unfortunately, since the closure fed to for then has the signature FnOnce() -> T instead of FnOnce() -> Option<T>, I had to resort to code that looked like this:

let prev = path.exists().then(|| ThinLTOImportMaps::load_from_file(&path).ok()).flatten();

and at that point I decided that was obscure enough that it wasn't worth trying to adopt these combinators.

Given that the signatures of and_then in the standard library (as well as std::cmp::Ordering::then) always take a closure, is it worth considering having fn then_some here also always take a closure, and have the difference between fn then_some and fn then not be about whether it takes a closure or not, but instead about what that closure's signature is?

That is, I'm suggesting this API:

impl bool {
    pub fn then_some<T, F: FnOnce() -> T>(self, f: F) -> Option<T> {
        if self { Some(f()) } else { None }
    }
    pub fn then<T, F: FnOnce() -> Option<T>>(self, f: F) -> Option<T> {
        if self { f() } else { None }
    }
}

If people want to keep a closure-less method in the API, I suggest fn and_some, so that then consistently means "invoke a higher order function"

pub fn and_some<T>(self, t: T) -> Option<T> {
    if self { Some(t) } else { None }
}

@pnkfelix
Copy link
Member

(oh, whoops; I guess the current conversation regarding the names is in rust-lang/rust#64260 )

@ibraheemdev
Copy link
Member

ibraheemdev commented Feb 18, 2021

For anyone coming from a search engine, bool::then is stable as of rust 1.50, and bool::then_some is available on nightly.

@djc
Copy link
Contributor

djc commented Feb 18, 2021

Actually, bool::then() became stable in 1.50:

https://blog.rust-lang.org/2021/02/11/Rust-1.50.0.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-impls-libstd Standard library implementations related proposals. A-primitive Primitive types related proposals & ideas 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.

Idea: Built-in way to go from bool to Option