Tracking Issue for bool_to_option · Issue #80967 · rust-lang/rust · GitHub

iago-lito commented on Jan 13, 2021 •

I personally like bool.then_some(value) better than bool.then(|| value) because it's less circuit in my head (althought I do expect both to produce same machine code). I would also favour it if it's more consistent with the rest of std.

Otherwise, bool.then(|| value) is fine and I could definitely live with that, so the stakes aren't high IMO. What's the overhead of having both methods in std?

I'm in favor of deprecating then_some for the following reasons:

• I still don't like the name
  • Since it is the conceptually simpler version of then, I think it should have the shorter/simpler name of the two functions, similar to and vs and_then, or vs or_else and unwrap_or vs unwrap_or_else and because then is already one word (and we don't want to call it th) this won't be possible.
  • The std crate uses the word then (and else) in function names almost exclusively to imply lazy evaluation (see again and vs and_then). then_some would only be the second function in the entire std crate to break this pattern.
  • The word some makes it sound like there is a then_none or then_ok counterpart to then_some, which does not exist. For the same reason, I believe being exposed to then_some first makes the name of the then function less intuitive and harder to find.
• then can already do everything then_some can do (and more!)
  • Every use of the then_some function can be trivially translated to use then instead, by adding || in front of the argument.
  • then_some can't even be called shorthand for then, because b.then(|| 5) is 2 bytes shorter b.then_some(5).
  • There is no decrease in performance to using then instead of then_some, because the compiler is smart enough to optimize closures away if they're unnecessary (see this comment in the original issue). Depending on how heavy the computation of the argument is, then might even give you a performance benefit, because the computation is skipped entirely if the bool evaluates to false.

nagisa commented on Jan 13, 2021

Every use of the then_some function can be trivially translated to use then instead, by adding || in front of the argument.

That wouldn't necessarily have the same semantics. then evaluates the argument, thus executing all of the code necessary to construct the T regardless of the bool's value.

Boscop commented on Jan 15, 2021 •

So instead of cond.then_some(foo()) you'd have to write cond.then({ let v = foo(); move || v })
or write it in multiple lines :/

iago-lito commented on Jan 15, 2021

The following two versions are not strictly identical indeed:

#![feature(bool_to_option)]

fn create_value() -> u32 {
    println!("creating");
    16
}

fn main() {
    let cond = false;
    println!("{:?}", cond.then_some(create_value()));
    println!("----");
    println!("{:?}", cond.then(|| create_value())); // or cond.then(create_value)
}

prints

creating
None
----
None

So it's not just a matter of syntax.

Not sure whether this particular behaviour of then_some is useful though.

x87 commented on Jan 20, 2021 •

With .then_some I can do the following:

    fn f() -> Option<usize> {
        let x: Option<Vec<u32>> = Some(vec![]);
        true.then_some(x?.len())
    }

how do I rewrite this example using .then?

    fn f() -> Option<usize> {
        let x: Option<Vec<u32>> = Some(vec![]);
        true.then(|| x?.len())
    }

gives an error that a closure should return either a Result or Option.

I can think of an extracting the closure expression into a variable:

    fn f() -> Option<usize> {
        let x: Option<Vec<u32>> = Some(vec![]);
        let len = x?.len();
        true.then(|| len)
    }

But in my opinion the first example with .then_some looks more concise and does not require a block {...}

jplatte commented on Jan 20, 2021

how do I rewrite this example using .then? [...]

If you use ? on your Option<Vec<u32>> anyways, might as well do it before assigning to a local:

fn f() -> Option<usize> {
    let x: Vec<u32> = Some(vec![])?;
    true.then(|| x.len())
}

If you insist on eagerly evaluating the value inside the option, you can also make use of Option::filter.

@iago-lito

#![feature(bool_to_option)]

fn create_value() -> u32 {
    println!("creating");
    16
}

fn main() {
    let cond = false;
    println!("{:?}", cond.then_some(create_value()));
    println!("----");
    println!("{:?}", Some(create_value()).filter(|_| cond));
}

@x87

fn f() -> Option<usize> {
    let x: Option<Vec<u32>> = Some(vec![]);
    Some(x?.len()).filter(|_| true)
}

iago-lito commented on Jan 21, 2021

@slerpyyy Well, this does work, but it becomes really hard to read IMO. I would argue that this being the only workaround to "eager evaluation" would be a good reason to actually include then_some ;)

Note that I'm still unsure whether "eager evaluation" is useful anyway, has anyone ever needed this?

To summarize my understanding of the advantage of then_some against then so far:

• permits early evaluation (but I'm not sure whether it's useful, is it?)
• permits early return (but @jplatte offered a convincing workaround IMO)
• .. anything else?

varkor commented on Jan 22, 2021

I prefer to use then_some than then when possible: needlessly creating a closure feels inelegant, even if in practice it makes no difference to generated code. It is also more consistent with existing APIs. I agree the name is unfortunate, but I don't view this as a reason not to have both. There's no way to make the names consistent with existing APIs now, so we could always go with a shorter name like some that is unrelated to then, which would at least address the length concern. I don't think the two names have to be related: it helps a little with memorisation, but if they're both useful (which in my experience, and others', they are), then it doesn't matter so much.

You're right, this is a function that users want and there is no reason not to add it to the std. I'm still annoyed that after more than a year of discussion about what we want to call these two functions in the original issue, we went with the only combination of names I actively dislike. Yet, I see how having this function with a somewhat unfortunate name is still better than not having this functionality at all.

coolshaurya commented on Feb 12, 2021 •

With regards to naming, I personally think then/then_do would have been ideal, but that ship has sailed.

then_some sounds a bit awkward, but I can't think of something better ¯\_(ツ)_/¯

varkor commented on Feb 12, 2021

I think it's fair to say that few people are happy with the names as they stand, but there was no general consensus, and we'd rather have the feature than not :)

slerpyyy commented on Feb 12, 2021 •

With regards to naming, I personally think then/then_do would have been ideal, but that ship has sailed.

I would have preferred (written as value / closure)

• then/then_with
• and/and_then
• some/then_some
• and_some/and_then_some
• to_option/to_option_with
• on_true/on_true_then
• if_true/if_true_then

but we went with

• then_some/then

SoniEx2 commented on Mar 8, 2021 •

reap/then? (or sail/then)

@SoniEx2 How does reap/sail relate to turning a (bool, T) into an Option<T>?

nah it's just a reference to that ship having sailed ^^

(side note: and/and_then we'd expect to return a bool.)

(I also proposed the names and_some/and_then_some in the original tracking issue for the same reason, but I listed those lower because the comment didn't get any reactions)

I have a different proposal. We should:

• Drop then_some
• Wait for #31844 to stabilize
• Create a specialization for plain values (which would be converted to Option) and another one for callables.

I think that dictating the behavior according to the type will let us eat the cake and keep it.

I usually prefer starting a statement with the variable which is being assigned, checked, or iterated on. This can increase readability, once you get used to it.

I have to say that I even prefer
players.iter().for_each(|p| p.take_turn) (I would like to get rid of .iter(), though)
over
for p in players { p.take_turn } (here, readers have to mentally skip three symbols (for p in) until they reach the point where it is getting interessing (players...))

Therefore, the following combination of methods for bool would be my favorite solution:

• bool.if_true( || foo() ) (alternative to, or even obsoleting then)
• bool.if_false( || bar() )
• and maybe: bool.if_else( || foo(), || bar() )
• finally, this is my contribution to this issue: bool.to_option( expr )

(disclaimer: I have my roots in Smalltalk, where if_true, and if_false is the only way to write an IF-condition.)

I have a different proposal. We should:

* Drop `then_some`

* Wait for #31844 to stabilize

* Create a specialization for plain values (which would be converted to `Option`) and another one for callables.

I think that dictating the behavior according to the type will let us eat the cake and keep it.

What do you guys think? I think it's the most ergonomic option as of yet.
It's very common to do these type of things in C++ for example.

jhpratt commented on Apr 5, 2021

@thedrow that wouldn't be unambiguous in all cases. What if you wanted to obtain Some(closure)?

@thedrow that wouldn't be unambiguous in all cases. What if you wanted to obtain Some(closure)?

Can you demonstrate how that would be ambiguous?
In the case you're proposing, you'll get a closure just like with any other Some(closure) you'd instantiate, no?

jhpratt commented on Apr 6, 2021

If I understand what you said correctly, you were suggesting that bool.then(val) would be permitted via specialization, yes? If this is the case, what if val was a closure? This would be equivalent to bool.then_some(|| || ret) (note that the closure returns a closure). From my understanding of what you said, this would be replaced by bool.then(|| ret), which is not the same thing.

The specialization would only handle types of Some<T> and None.
bool.then(|| || ret) accepts a closure as an argument which means it should be handled by another specialization (or possibly the default implementation) that expects a callback. The result of bool.then(|| || ret) would be Some(|| ret) if bool is true and None if it is false.

I don't see a reason why we'd need both a version that returns an Option and another that doesn't. In my opinion, then should always return an Option.

ExpHP commented on May 3, 2021

bool::then_some still links to the old tracking issue in the documentation...

dhardy commented on Jul 18, 2021

• Create a specialization for plain values (which would be converted to Option) and another one for callables.

I think that dictating the behavior according to the type will let us eat the cake and keep it.

What do you guys think? I think it's the most ergonomic option as of yet.
It's very common to do these type of things in C++ for example.

This requires replacing

pub fn then<T, F>(self, f: F) -> Option<T> where
    F: FnOnce() -> T, 

pub fn then<T, R>(self, f: R) -> Option<T> where
    R: ResolveTo<T>,

But with impls of ResolveTo<T> for both T and FnOnce() -> T this breaks type inference.

This is a breaking change and also in my opinion undesirable (explicit typing is often easier to work with).

I honestly am in favour of bool::then_some; many arguments here are more criticisms of bool::then which has already been stabilised.

It would be possible with overlapping traits.

dhardy commented on Jul 18, 2021

@SoniEx2 if you are referring to https://rust-lang.github.io/rfcs/1268-allow-overlapping-impls-on-marker-traits.html, this is not applicable since the ResolveTo used above is not a marker trait:

pub trait ResolveTo<T> {
    fn resolve(self) -> T;
}

nah, we mean our rejected proposal for marking traits as overlapping, and having both preferred and overlapping impls of those traits. you can then explicitly choose impls in the overlapping case, or default to the preferred impls only.

you'd write

// uhh not sure how you'd make an overlapping trait "sealed".
pub overlapping trait ResolveTo<T> {
  fn resolve(self) -> T;
}

overlapping impl<T> ResolveTo<T> for T {
  fn resolve(self) -> T {
    self
  }
}

impl<T, F> ResolveTo<T> for F where F: FnOnce() -> T {
  fn resolve(self) -> T {
    self()
  }
}

and then calls would be one of these:

b.then(|| bar): Option<Bar>;
b.then::<fn() -> Bar>(|| bar): Option<Bar>;
b.then::<_: (overlapping impl<T> ResolveTo<T> for T)>(|| bar): Option<fn() -> Bar>;
b.then::<_: (overlapping impl<T> ResolveTo<T> for T)>(bar): Option<Bar>; // with future possibility of not requiring the <_: (overlapping impl)> syntax if Bar just so happens to not implement FnOnce() -> T. 

jhpratt commented on Jul 18, 2021

You've proposed this three times with negative reactions; there's no need to hijack a tracking issue with what you acknowledge is a rejected proposal.

To summarize my understanding of the advantage of then_some against then so far:

* permits early evaluation (but I'm not sure whether it's useful, is it?)

* permits early return (but @jplatte offered a convincing workaround IMO)

* .. anything else?

I find it useful in the following context:

impl<T> Foo<T> {
    ...
    fn data(&self) -> Option<&T> {
         self.some_condition().then(|| self.data) //works
    }

    fn data_mut(&mut self) -> Option<&mut T> {
        self.some_condition().then(|| self.data) // Doesn't work, closure may outlive the current function, but it borrows `self`, which is owned by the current function
        self.some_condition().then_some(self.data) // Works
    }
}

I just ran into this today, and found myself wanting then_some; I had an iterator of bools and an iterator of corresponding values, zipped together, and I wanted to do .filter_map(|(has, thing)| has.then_some(thing)). I found .filter_map(|(has, thing)| has.then(|| thing)) confusing to read, because it nests a closure within a closure.

I'd love to revive the question of stabilizing this.

mbartelsm commented on Jan 2

Agree with @joshtriplett. We already have value/eager and closure/lazy evaluators for Option and Result, having just one version for bool feels off.

Regarding the name, it's probably too late to fix it but maybe there's a chance to introduce a new method that does the same as then and deprecate then next edition? that is, only if the naming issue is that big of a deal for people.

Anyway, what I mean to say is that the name is already chosen, and even though it's unstable plenty of people already use this. I think it would be beneficial to go ahead and stabilize it.

joshtriplett commented 9 days ago

It does seem like the rough consensus in this thread is that while nobody is thrilled with any particular choice of naming at this point, many people would like to have the method.

Let's see if we have some level of consensus on that. Shall we stabilize the method bool::then_some, under its current name?

@rfcbot merge

rfcbot commented 9 days ago •

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

No concerns currently listed.

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 commented 8 days ago

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

SoniEx2 commented 8 days ago •

so python has this convention of list.sorted() and sort(list). rust has option.filter(closure) how about bool.filtered(option)?

(we know this is mostly silly but, eh, might as well put it out there y'know?)