Tracking issue for bool::then_some

[varkor]

Tracking issue for the bool::then_some method, which abstract the following pattern:

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

bool:then has previously been stabilised as part of this feature gate: #79299

RFC: rust-lang/rfcs#2757
Implementation PR: #64255

[U007D]

In trying this out, I was puzzled why the eager/lazy argument evaluation versions are then/then_with and not and/and_then, which seems more consistent with established patterns in std?

[varkor]

There's a long discussion in the RFC about the naming. The current names are essentially placeholders.

[jplatte]

As a datapoint, this feature turns out pretty useful in proc macros, and I do think the current naming reads nicely: https://github.com/ruma/ruma-api/commit/cb00188e0371c544c4b8622d5a32731f2f6c54b3

[varkor]

@jplatte: that would be helpful to share on the RFC!

[c410-f3r]

It's been sometime since the PR implementation.
Is there anything preventing stabilization? If not, I can write a stabilization report for this feature.

[varkor]

@c410-f3r: the names were only decided upon quite recently. It was decided that we would wait a few versions before deciding to stabilise: rust-lang/rfcs#2757 (comment).

[U007D]

Just came back to give this a test drive in a library I am working on. If we look at common current established patterns in std:

Option:
map<U, F> whereF: FnOnce(T) -> U and
and_then<U, F> whereF: FnOnce(T) -> Option<U>

Result:
map<U, F> whereF: FnOnce(T) -> U and
and_then<U, F> whereF: FnOnce(T) -> Option<U>

And looking at the current implementation of the bool_to_option feature:

bool:
then_some whereF: FnOnce() -> T

The inconsistency is pretty jarring/unexpected, to my mind. Specifically,

1. for consistency shouldn't bool's F: FnOnce() -> T be called map, and F: FnOnce() -> Option<T> be some then derivative (and_then, then_some, etc.)?
2. shouldn't bool_to_option also provide both an F: FnOnce() -> T and an F: FnOnce() -> Option<T> implementation?

This is not just hypothetical; I am working on a library with a type that has a two-step fallible constructor. The intent is to be able to write something along the lines of:

...
impl Foo {
    fn new() -> Option<Self> {
        step_1_check().and_then(|| step_2_check().map(|| Self { /* ... */ }))
    }
}

but with the current plan developers would still have to resort to writing a custom combinator or pull in an external crate like boolinator for this simple case.

Thoughts?

[RustyYato]

You can call bool::then followed by Option::flatten to get the same effect. But it does make sense to have a fallible version of bool::then

[jplatte]

The inconsistency is pretty jarring/unexpected, to my mind.

@U007D: Your comparison is between bool → Option<T> conversion functions and Option<T> → Option<U> (+ Result<T, E> → Result<U, E>) conversion functions. Personally I find that given the input and output types are completely distinct in the new functions when they're not in the ones you're comparing them with is a good reason to consider different names.

[Boscop]

@U007D I completely agree. It was already tripping me up many times when using the boolinator crate, which also has inconsistent naming!

IMO these functions should be named consistently with Option/Result.
And not just map and and_then, but it would also make sense to have and, or and or_else.

Or (if that's too much to include into std):
Just have a method on bool that turns a bool into an option fn opt(self) -> Option<()>; (should have a short name), so that we can then use all the above option methods:
b.opt().map(|_| 42), b.opt().and_then(|_| Some(42)), b.opt().and(Some(42)), b.opt().or(Some(42)), b.opt().or_else(|| Some(42)).
Although I'd also prefer to have these as direct methods on bool (or maybe we can let bool be borrowable as &Option<()>, so that we could call Option's methods on a bool?).
But please use consistent naming!

Currently, according to the docs, b.then_some(..) corresponds to b.opt().map(..) whereas b.then(..) corresponds to b.opt().and_then(..), which is not consistent at all.

[djc]

How is the libs team feeling about this? Just ran into another case where I'd have liked to use this (on stable).

[djc]

@rustbot ping libs

[rustbot]

Error: Only Rust team members can ping teams.

Please let @rust-lang/release know if you're having trouble with this bot.

[varkor]

cc @rust-lang/libs regarding naming and stabilisation: #64260 (comment).

[SimonSapin]

@varkor Did you mean to link some other comment? This one does not discuss naming.

I honestly don’t remember the current status of this API.

The current names are essentially placeholders.

Is this still the case or has some consensus been found since?

[varkor]

@SimonSapin: sorry, I should have also linked to the comments above. I think the status is that, modulo outstanding concerns from some users about the naming, these methods are probably suitable for stabilisation.

Is this still the case or has some consensus been found since?

The current names were suggested by @dtolnay. I'm personally happy with the existing names, but there have been some complaints: #64260 (comment), #64260 (comment). However, I think considering how much previous discussion we had on these names, without coming to a clear consensus, I think it's probably worth stabilising with the existing names, so these methods can be used.

Perhaps starting an FCP would be the best way forward with these methods now: if any libs team members decide there are better alternatives, these could be raised during the FCP — otherwise, it would be good to be able to use these on stable.

[SimonSapin]

As a reminder, this issue currently tracks:

impl bool {
    pub fn then_some<T>(self, t: T) -> Option<T> {…}
    pub fn then<T, F: FnOnce() -> T>(self, f: F) -> Option<T> {…}
}

I can live with this, and more time likely won’t bring new information or ideas at this point.

@rfcbot fcp merge

[rfcbot]

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

• @Amanieu
• @KodrAus
• @SimonSapin
• @dtolnay
• @sfackler
• @withoutboats

Concerns:

• name (Tracking issue for bool::then_some #64260 (comment))

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.

[withoutboats]

@rfcbot resolve stabilize-only-then

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

[varkor]

I've opened a stabilisation PR for then: #79299.

[jplatte]

Stabilized; I think this tracking issue can be closed 🙂

[CryZe]

Only then got stabilized, then_some is still being tracked.

[m-ou-se]

It might be a good idea to move then_some to a new tracking issue and close this one.

[varkor]

I've updated the title and description. I don't think there's any need to create a new issue: that just causes unnecessary churn.

[slerpyyy]

Does that mean we're still considering stabilizing then_some after all?
I thought we decided that adding then_some was redundant, since then can already do everything then_some can and more.

[varkor]

@slerpyyy: yes, see #64260 (comment). There was a desire to see how useful then_some was in practice as a shorthand. This pattern (taking a value vs taking a closure) is already common throughout the standard library.

[KodrAus]

Closing this one in favour of #80967, so that we'll get a chance to issue a fresh FCP if we decide to in the future.

[matklad]

As an experience report, I've recently found roughly the following code in the wild:

(!is_uppercase)
    .then(|| ())
    .ok_or_else(|| ParseError(ParseErrorKind::Invalid, value.to_string()))

It takes some mental puzzle-solving to figure out whether is_uppercase results in error or not.

Don't think this is actionable in any way, but it's a useful real-world specimen to ponder.

[thomaseizinger]

I think that is a good example that not everything is better with combinators :)

I haven't decipered 100% it but I would write this as:

if !is_uppercase {
    return Err(ParseError(...));
}

Personally, I've been using .then() primarily with .filter_map. I think it is a fantasic fit for that.

[Lucretiel]

Agree, I've seen some similar code in the wild. I think that usually .then() is only appropriate in cases where an Option (or Result, via ok_or_else) is actually in play; .then() followed immediately by *_or_else is probably an antipattern (because you're needlessly round-tripping through an Option). I'd love to see a clippy lint to this effect.