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

use composition in std::option #9355

Closed
thestinger opened this issue Sep 20, 2013 · 14 comments
Closed

use composition in std::option #9355

thestinger opened this issue Sep 20, 2013 · 14 comments
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup.

Comments

@thestinger
Copy link
Contributor

Rather than having 3 versions of every utility (get, map, chain, etc.), we can have a single by-value version.

Conversion to Option<&T> and Option<&mut T> from Option<T> can be composed with the regular combinators. References are first-class values, so there's no need to have extra methods for them.

map_move(...) -> map(...)
map(...) -> as_imm().map(...)
map_mut(...) -> as_mut().map(...)

map_move_default(...) -> map_default(...)
map_default(...) -> as_imm().map_default(...)
map_mut_default(...) -> as_mut().map_default(...)

and_then(...) -> and_then(...)
and_then_ref(...) -> as_imm().and_then(...)
and_then_mut_ref(...) -> as_mut().and_then(...)

unwrap() -> unwrap()
get_ref() -> as_imm().unwrap()
get_mut_ref() -> as_mut().unwrap()

@ghost ghost assigned thestinger Sep 20, 2013
@lilyball
Copy link
Contributor

I'm all for reducing the API. I'm a bit worried about the naming though, as I'm not aware of consensus being reached on #7887. And as @blake2-ppc pointed out in IRC, .unwrap() may fail and .get() doesn't carry the same connotations as .unwrap() (at least, for the moment. I suppose if we did a mass-rename across all collections people would get used to it, but that would require consensus on #7887).

@thestinger
Copy link
Contributor Author

There's not really any consensus about any of the names and the discussion would need to start over if there were 1/3 of the number of methods. I'll try to keep the names as close to what they already are though.

@Kimundi
Copy link
Member

Kimundi commented Sep 20, 2013

1+ for better composability!

@brson
Copy link
Contributor

brson commented Sep 20, 2013

This looks pretty compelling to me. If we did this, then renaming unwrap to get also makes a lot of sense.

@brson
Copy link
Contributor

brson commented Sep 20, 2013

How could this pattern be applied to other containers?

@thestinger
Copy link
Contributor Author

@brson: In general, the std::iter module is able to accomplish this. The algorithms are all implemented on a generic Iterator<A> and then individual iterators can yield references, mutable references or values - which are all just regular A elements as far as std::iter is concerned.

The only algorithm with a specific need is reverse_ as it requires &mut T to do an in-place reverse.

@thestinger
Copy link
Contributor Author

Option is just a bit special, since it always has 0 or 1 element, while iterators are variable-length (possibly infinite). So these utility methods can be much more specialized than generic variable-length versions.

@erickt
Copy link
Contributor

erickt commented Sep 22, 2013

While I love composability and think most people will use Option.map_move over Option.map, I added Option.map_move to make Option more symmetrical with std::iter, which is what I thought was decided in #7887. I thought we had the plan to ad an Iterable trait, as in:

trait Iterable<T> {
    fn iter(&self) -> Iterator<T>;
    fn map(&self, f: fn(&T) -> U) -> MapIterator<U> { self.iter().map(f) }
    ...
}

impl<T> Iterable<T> for Option<T> { ... }

How can we resolve the conflict between the Iterable.map and this new Option.map? Should Option just not derive from Iterable? Or maybe not have Option have an iterator interface?

@Kimundi
Copy link
Member

Kimundi commented Sep 22, 2013

Seems to me like Options default iterator should be an move_iter() anyway.

@thestinger: Would it be possible to have more an one Iterable trait?
RefIterable, ValueIterable, MutRefIterable each providing imm_iter(), iter() and mut_iter(), and have the for syntax try all four in order? (Iterator > RefIterable > ValueIterable > MutRefIterable)

You'd then for example implement all three for vector, but only ValueIterable for Option, and the for syntax would default to .imm_iter() for vectors, and .iter() for Option.

@bluss
Copy link
Member

bluss commented Sep 22, 2013

erickt, with map being either of these (with pseudocode):

map(Option<T>, fn(T) -> U) -> Option<U>    // this is in This proposal
map(Iterator<T>, fn(T) -> U) -> Iterator<U>   // this is Iterator::map today

It does seem consistent to me. I don't see the need to treat Option as an Iterable or container itself, it is a far more basic building block than that.

@Kimundi
Copy link
Member

Kimundi commented Sep 22, 2013

@blake2-ppc The idea behind the Iterable trait is that you'd not need to write .iter() explicitly, but could directly do for e in [1,2,3].filter(|&e| e % 2 == 0) { ... }, which would mean the names map filter, ... etc would be taken as methods.

HOWEVER, I just realized we can just solve this in an easy way:
Make Option implement Iterator directly.
So, instead of opt.iter().map(...) or opt.move_iter().map(...) and the confusion about how to solve the Iterable situation, we'd just have opt.map(...) and opt.as_imm().map(...).

@bluss
Copy link
Member

bluss commented Sep 22, 2013

My suggestion was: Option should not be Iterable or an Iterator.

@lilyball
Copy link
Contributor

I agree with @blake2-ppc. Having Option be an Iterable or Iterator just feels weird. If I call .map() on an Option I damn well expect to get an Option back, not a MapIterator<T>.

bors added a commit that referenced this issue Oct 9, 2013
@thestinger
Copy link
Contributor Author

Implemented.

@thestinger thestinger removed their assignment Jun 16, 2014
Jarcho pushed a commit to Jarcho/rust that referenced this issue Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup.
Projects
None yet
Development

No branches or pull requests

6 participants