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

Overloaded Add on Option has arguably the wrong behavior #6002

Closed
bstrie opened this issue Apr 22, 2013 · 24 comments
Closed

Overloaded Add on Option has arguably the wrong behavior #6002

bstrie opened this issue Apr 22, 2013 · 24 comments
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@bstrie
Copy link
Contributor

bstrie commented Apr 22, 2013

A while ago I overloaded Add on Option types such that Some(1) + None == Some(1). I've since had a change of heart and believe that instead it should be Some(1) + None == None. An informal poll in IRC seems to agree.

@brson
Copy link
Contributor

brson commented Apr 22, 2013

+1

@isaacaggrey
Copy link

Is there any precedence of this behavior from other programming languages (e.g., Haskell, Scala, Standard ML)?

I'm not very familiar with type theory, so my novice eyes see the original behavior as correct; ultimately, I would err on the side of whatever has proven best in other languages (especially as that seems to be Rust's philosophy).

@thestinger
Copy link
Contributor

AFAIK other languages don't define + for their Option types. They have you use map/fmap with any generic function.

It's not very generic right now because it assumes that copying is the behaviour you want when one of the two is None. I would prefer just using higher-order functions to keep the behaviour clear and the API small. Are we going to add the other operators and functions in the standard library? It feels very wrong to me.

Defining Eq/Ord has precedent in other languages and allows for deriving and containers to work well.

Does it really make sense to pass Option<int> to a generic math function making use of Add/Mul?

@catamorphism
Copy link
Contributor

I think that by analogy with the Maybe monad in Haskell, it would make sense for Some(1) + None == None. The intuition is that None represents an error, so once you see an error, you want to propagate it outwards.

I'm neutral about whether we want to have an instance for Add on Option at all. Other languages don't have it AFAIK, but then, Haskell does support using monadic syntax to simplify code that passes around Options, and we don't.

@bstrie
Copy link
Contributor Author

bstrie commented May 23, 2013

@thestinger you're welcome to remove it if you think it's a poor idea. My primary concern is that increasing the friction required to make use of Options will simply result in them not being used at all. And since users can't implement this themselves, this is a decision that we have to make now.

@kud1ing
Copy link

kud1ing commented May 24, 2013

@catamorphism: Haskell's typeclass MonadPlus shows Rust's current behaviour: mplus (Just 1) Nothing gives Just 1

http://www.haskell.org/haskellwiki/MonadPlus

@catamorphism
Copy link
Contributor

@kud1ing I guess I was thinking of the Maybe instance of the >>= (bind) operator in Haskell.

@kud1ing
Copy link

kud1ing commented May 24, 2013

I think we have to decide, whether None means "no value" or "error".
For example: what would be the sum of a list of Options, when there is a single None?

I think we need to support both representations:

  • "no value" represented by Option's None
  • "error" represented by
    Result's Err.

Whatever we come up with, we should explain the decision at least in the code.

@thestinger
Copy link
Contributor

I don't think there's anything to decide, "no value" and "error" are the same thing if there's only one error that could happen and it means a value couldn't be returned. If there's more than one possible error and there's a meaningful distinction between them that the caller would care about, it should use Result.

An Option<T> return on from_str or a decompression function means "invalid input". An expanded form could return Result and indicate why it was invalid, but both are sane functions.

@huonw
Copy link
Member

huonw commented May 24, 2013

@kud1ing MonadPlus is slightly different, since mplus (Just 1) (Just 2) == Just 1 (not Just 3), i.e. it is the same as Option::or in Rust.

@kud1ing
Copy link

kud1ing commented May 24, 2013

Ask two persons how many pair of shoes they own. One does not reply, the other says 10.
Now calculate the number of pair of shoes people own. Do you say "this is an error, one value is not available" or do you mean "just add the available numbers"?

@catamorphism
Copy link
Contributor

I get where you're going with this, @kud1ing , but I don't think this question has a single right answer. The team just needs to agree on something and document it.

@kud1ing
Copy link

kud1ing commented May 24, 2013

@catamorphism: That's why i think we have to be aware of what we want to express and write that down. First so that users know what behaviour to expect and second so we don't change our implementation again and again.

@huonw
Copy link
Member

huonw commented May 24, 2013

Two questions: Which behaviour is most common? Which is most annoying to implement in "userspace"?

@kud1ing
Copy link

kud1ing commented May 24, 2013

I think Option can be seen as a Monoid: http://en.wikipedia.org/wiki/Monoid with the operation "addition".

None would be the Identity element, or empty set.

@kud1ing
Copy link

kud1ing commented May 24, 2013

Haskell can have 4 different implementations for sums: http://byorgey.wordpress.com/2011/04/18/monoids-for-maybe/

  1. return the sum of all non-None values
    • None represents no information
  2. return None when there is a single None
    • None represents failing computation
  3. return the first non-None value (this is what the MonadPlus implementation for Maybe implies)
    • None represents failing computation
  4. return the last non-None value
    • None represents failing computation

I think

  • no information should be expressed using Option, since its name implies optionality
  • failing computation should be expressed using Result, since it can indicate the failure reason

and thus suggest we

  1. keep the current Option behaviour
  2. change the places where a missing value is an error to use Result instead
  3. write down that None means "no information" and Err means "error"

@graydon
Copy link
Contributor

graydon commented May 24, 2013

Better yet: reconsider the places that use either option or result this way, and if it is a rare error, add / rewrite to a version that uses a condition.

@glaebhoerl
Copy link
Contributor

I would strongly vote for Some(1) + None == None. What we have is a function (+) over T and we want to lift it to a function over Option<T>. We can do this two ways (corresponding to 1. and 2. above):

  1. If the function forms a monoid with some identity element of T, we can replace any Nones with that identity element and then use the function to combine them, returning Some. Note that in this case it actually makes more sense to return T and not Option<T>, because we never return None.
  2. We can use the function to combine Somes returning Some, or return None otherwise.

The latter works generically for any function. It's how Haskell defines the Applicative and Monad instances for Maybe (and is in fact the only way they can be lawfully defined). Note also that with 1., to be consistent, you need to depend on another trait impl that provides the identity element, so that None + None == Some(0). I'm not sure if there's any principled formulation where None + None == None, Some(a) + None == Some(a), and Some(a) + Some(b) == Some(a + b).

Haskell doesn't overload the numerical operators like this by default because the fully generic way to do it would be:

instance (Applicative f, Num n) => Num (f n) where
    (+) = liftA2 (+)
    (*) = liftA2 (*)
    ...

and that would overlap with every other instance Num (Foo a) (because overlap checking doesn't take superclass constraints into account).

MonadPlus is unrelated to + except by name.

I think having Option and Result behave differently would be surprising.

@toddaaro
Copy link
Contributor

Visiting for triage, have there been any more thoughts on this? Also nominating for "backwards compatible".

An option I haven't seen is just not having a + operation defined on options. Has this been ruled out?

@bstrie
Copy link
Contributor Author

bstrie commented Jul 16, 2013

@toddaaro I originally overloaded + on Option as a lark. I doubt it's used anywhere, so it could probably be trivially removed. That might even be the best course of action at the moment until there's both an actual demand for it and a consensus on its semantics.

@Kimundi
Copy link
Member

Kimundi commented Jul 30, 2013

I fully agree with @glehel here: Some(n) + None == None so that None is a useful part of the operation that doesn't just do the same as Some(0).

You can always map None to Some(0) if you want a different behavior, something which is not possible the other way round without information loss.

If we compare it with @kud1ing 's list the use cases could be fulfilled like this::

  1. return the sum of all non-None values: opt1.or_zero() + opt2.or_zero()
  2. return None when there is a single None: opt1 + opt2
  3. return the first non-None value: opt1.or(opt2)
  4. return the last non-None value: Either opt2.or(opt1) or add a method that does this.

@nikomatsakis
Copy link
Contributor

My vote: do not define + on options. I see no good reason to use a numeric
operator here.

@graydon
Copy link
Contributor

graydon commented Aug 22, 2013

accepted for backwards-compatible milestone

@catamorphism
Copy link
Contributor

I agree with @nikomatsakis at this point.

bors added a commit that referenced this issue Aug 27, 2013
Closes #6002 

There is consensus that the current implementation should be changed or
removed, so removing it seems like the right decision for now.
osaut pushed a commit to osaut/rust that referenced this issue Oct 31, 2013
This is an alternative version to rust-lang#8268, where instead of transitioning to `get()` completely, I transitioned to `unwrap()` completely.

My reasoning for also opening this PR is that having two different functions with identical behavior on a common datatype is bad for consistency and confusing for users, and should be solved as soon as possible. The fact that apparently half the code uses `get()`, and the other half `unwrap()` only makes it worse.

If the final naming decision ends up different, there needs to be a big renaming anyway, but until then it should at least be consistent.

---

- Made naming schemes consistent between Option, Result and Either
- Lifted the quality of the either and result module to that of option
- Changed Options Add implementation to work like the maybe Monad (return None if any of the inputs is None)  
  See rust-lang#6002, especially my last comment.
- Removed duplicate Option::get and renamed all related functions to use the term `unwrap` instead  
  See also rust-lang#7887.

Todo: 

Adding testcases for all function in the three modules. Even without the few functions I added, the coverage wasn't complete to begin with. But I'd rather do that as a follow up PR, I've touched to much code here already, need to go through them again later.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.