impl Add for Option<T>

[zcesur]

Implement Add for Option<T> for all T that also impl Add. The resulting Option type forms a semigroup as well as a monoid given that it also implements Default.

Motivation

Fold iterators containing optional values, concatenating the values along the way.

Example

fn fizzbuzz(n: u32) -> String {
    [(3, "Fizz"), (5, "Buzz"), (7, "Bazz")]
        .iter()
        .map(|(m, s)| Some(n).filter(|i| i % m == 0).map(|_| s.to_string()))
        .fold(None, Option::add)
        .unwrap_or(n.to_string())
}

assert_eq!("1", fizzbuzz(1));
assert_eq!("Fizz", fizzbuzz(3));
assert_eq!("FizzBuzzBazz", fizzbuzz(105));

To make the above possible, I've also added an implementation of Add for String along the lines of the below since the existing string impl didn't work. I've just started learning Rust so I don't understand why that is the case. I'd love to learn more if anyone has an explanation!

impl Add for String {
    type Output = String;

    fn add(self, other: String) -> String {
        format!("{}{}", self, other)
    }
}

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[kennytm]

We have discussed about this in https://internals.rust-lang.org/t/lifting-binary-operations-e-g-t-t-option-t-option-t/6618 before, and there's no consensus that we want this impl. It is not natural that Some(x) + None == Some(x), because we would also apply the applicative rule (liftA2) and conclude that Some(x) + None == None (it is like (2 + NULL) IS NULL in SQL). I suggest we close this PR.

(Also, even if we have this impl, your example doesn't work since &str does not impl Add)

[pickfire]

Potential footgun, it may not be obvious that None is ignored, I thought None should become None at first if either one is None.

[scottmcm]

Procedurally, I think this needs an RFC:

• This is insta-stable so needs at least an FCP no matter what
• This is setting a new precedent -- if this is accepted, then it seems clear that similar implementations should also be added for Sub, Mul, Div, etc, as well as possibly ones on Result (as our other monad)
• There are two reasonable implementations for such a method, so it's a place where the "let's think about it and document the tradeoffs and why we picked what we did" of an RFC would be valuable (unlike many libs additions where there's a naming bikeshed but no deep semantic questions)

But I'm not on T-libs so we'll see what they say. And I'd suggest waiting to get some sort of straw poll from them before putting work into an RFC, since what @kennytm mentioned sounds persuasive to me.

[zcesur]

@kennytm Thanks for the link, that was an insightful discussion.

I've found out that there has also been some back and forth on implementing this exact behavior way back in 2013:
#5328 (merged PR), #6002 (proposal to revert), #8772 (reverted PR)

I was initially convinced that this would be the natural and obvious implementation by drawing a parallel to list/vector concatenation (i.e., [] ++ [x] ++ [y] = [x,y] hence None + Some(x) + Some(y) = Some(x + y)) given that Option can be viewed as a collection with either one or zero elements. But your feedback along with the above issues makes it evident that committing to either implementation is not at all natural. I can see how overloading an especially ubiquitous operator like + can result in unintended bugs and/or counter-intuitive behavior in either implementation.

Despite that, I feel like the monadic semantics (i.e. None as the absorbing element representing a failed computation) for + (as well as any other binary function) is already fairly straightforward to achieve:

fn lift<T0, T1, T2>(f: fn(T0, T1) -> T2) -> impl Fn(Option<T0>, Option<T1>) -> Option<T2> {
    move |x, y| Some(f(x?, y?))
}

let add_opt = &lift(std::ops::Add::add);

assert_eq!(add_opt(Some(5), Some(8)), Some(13));
assert_eq!(add_opt(Some(13), None), None);

assert_eq!(vec![Some(3), Some(5), Some(8)].into_iter().fold(Some(0), add_opt), Some(16));
assert_eq!(vec![Some(3), None, Some(8)].into_iter().fold(Some(0), add_opt), None);

We can even leverage FromIterator impl of Option in some cases:

let xs = vec![Some("he"), Some("llo")];
assert_eq!(xs.into_iter().collect::<Option<String>>(), Some(String::from("hello")));

let xs = vec![Some("he"), None, Some("llo")];
assert_eq!(xs.into_iter().collect::<Option<String>>(), None);

Given that the monadic instance of Option is already covered by the std lib, I think the instance with None as the identity element is practically speaking more useful to add. And so my follow-up proposal is to introduce a new trait

pub trait Accum<Rhs = Self> {
    type Output;
    fn concat(self, rhs: Rhs) -> Self::Output;
}

so that Option<T> has the semantics by which it takes any T: Accum<Output = T> and turns it into a monoid by adjoining None as the identity element.

impl<T: Accum<Output = T>> Accum for Option<T> {
    type Output = Self;

    fn concat(self, other: Self) -> Self::Output {
        match (self, other) {
            (a, None) => a,
            (None, b) => b,
            (Some(a), Some(b)) => Some(a.concat(b)),
        }
    }
}

I will close this PR and make a new one if you guys think that's a good idea.

[scottmcm]

@zcesur Note that the bar for a new trait in the standard library is quite high. That Accum trait probably hits the same "setting a new convention" level that warrants an RFC I'd mentioned earlier.

This PR needed to be to core because it's an impl of a core trait on a core type. If you can go through a new trait, I'd suggest trying it out as a crate first -- you can still implement your trait for String and friends that way.

Good sleuthing for the old issues. Given that I don't think anything obvious that has changed since then, closing this PR probably sounds like the way to go.

[zcesur]

Sounds good