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

Implement FromIterator for the unit type #1130

Closed
wants to merge 3 commits into from

Conversation

fenhl
Copy link

@fenhl fenhl commented May 20, 2015

Implement FromIterator<()> for the empty tuple type () aka unit, allowing Result::from_iter to be used with Result<(), E>.

Rendered


Implement the trait `std::iter::FromIterator<()>` for the primitive type `()`.

The implementation is very short:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this should run the iterator, rather than discard it, e.g.

fn from_iter<T>(iter: T) -> () where T: IntoIterator<Item = ()> {
    for _ in iter {}
}

I also wonder if it could be used as a general purpose discard:

impl<X> FromIterator<X> for () {
     fn from_iter<T>(iter: T) -> () where T: IntoIterator<Item = X> {
          for _ in iter {}
     }
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. If the exhausting implementation is chosen, the generic version seems strictly better as far as I can tell. It could be used, for example, to collect any iterable of Result<T, E> into a Result<(), E>.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need another general-purpose discard for iterators.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @huonw that this probably wants to exhaust the iterator at least, and we can always leave the X type parameter (exhausting all kinds of iterators) to a future extension.

@Kimundi
Copy link
Member

Kimundi commented May 20, 2015

Seeing how iterators are lazy in general, I think FromIterator implementations should be too, so a impl for () should not exhaust one.

This also ties in with the future possibility of other fixed-sized containers:

let v: FixedCapVec<X, 4> = iter.collect(); // Expected not to iterate more than 4 times.
let v: OptionLike<X>     = iter.collect(); // Expected not to iterate more than 1 times.
let v: ()                = iter.collect(); // Expected not to iterate more than 0 times.

@Stebalien
Copy link
Contributor

What about find?

fn forward_values<T>(src: Receiver<T>, dst: Sender<T>) -> Result<(), SendError<T>> {
    src.iter().map(|val| dst.send(val)).find(|v|.is_err()).or(Ok(()))
}

@fenhl
Copy link
Author

fenhl commented May 20, 2015

@Kimundi I agree with your point. A generic implementation would still be useful.

@bluss
Copy link
Member

bluss commented May 20, 2015

This is better written with std::result::fold I think or even std::result::fold_ (gone!) 💨

and yes, I'd propose moving std::result::fold to be a method on Iterator<Item=Result<_,_>> instead.

@nrc nrc added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label May 21, 2015
@lilyball
Copy link
Contributor

Should the iterator be exhausted rather than discarded?

I think this question right here is actually a serious problem for this proposal. There's valid arguments both ways, and I'm inclined to think that the unclear obvious answer to this means we shouldn't do it at all.

As @bluss has said, you can implement this with std::result::fold, as in

let value = result::fold(src, (), |_,_| ());

It might be worth adding a function std::result::all() that just takes an IntoIterator<Result<T,E>> and returns a Result<(),E> (this would let it be used with any result type, by discarding the values). This would just be shorthand for that fold() expression above, intended primarily as a convenience to let the user know that this is a thing they can do, without having to think through the fold().

@Gankra Gankra self-assigned this Jun 5, 2015
@Gankra
Copy link
Contributor

Gankra commented Jun 5, 2015

I'd personally find it pretty surprising for collect to not exhaust the iterator.

I'm also personally a fan of "just write the loop" which is IMO much clearer and easier to maintain than a pile of iterator adaptors. Particularly when you're running the code for the side-effects.

@withoutboats
Copy link
Contributor

Collect ought always to exhaust the iterator; collecting an iterator consumes it. No std impl of FromIterator is lazy. (i'm not clear on what it would mean for them to be lazy, either).

@bluss
Copy link
Member

bluss commented Jun 6, 2015

Consuming an iterator or not doesn't matter: consider passing by-ref iterators, where the owner of the iterator can still access it after collect. Taking iterators by value is just the most universal interface, because it allows us to pass iterators either by value or as &mut T.

When collecting or extending into a fixed size data structure, I've for now chosen to not exhaust the iterator, but instead take only as many elements that fits.

@withoutboats
Copy link
Contributor

I suppose I wouldn't expect a data structure with a fixed len to exhaust the remainder of the iterator, and a () could be conceived of as a data structure with a fixed len of 0. I agree with @kballard that this is a real problem: whichever way this RFC would be decided, there will be contexts in which its behavior seems arbitrary and surprising.

@ftxqxd
Copy link
Contributor

ftxqxd commented Jun 6, 2015

No std impl of FromIterator is lazy.

Result and Option’s impls are lazy. (Playpen demonstration.) To me this is enough to make it pretty clear that ()’s impl should also be lazy (i.e., not exhaust the iterator).

@lilyball
Copy link
Contributor

lilyball commented Jun 7, 2015

At first glance that seems like a reasonable argument, except fixed-length data structures in general cannot implement FromIterator at all, because there would be no valid behavior for if the iterator did not have enough elements. Only the degenerate case of a zero-length data structure can do that, which kind of defeats the point of considering () as a fixed-length data structure of length 0.

@Kimundi
Copy link
Member

Kimundi commented Jun 7, 2015

@kballard: For fixed length data structures there is always the option of panic!(), or implementing FromIterator for Option<FixedLengthStructure<N>>.

And fixed capacity structures wouldn't have the this problem at all.

@alexcrichton
Copy link
Member

This RFC is now entering its final comment period.

@alexcrichton alexcrichton added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Jun 16, 2015
@ruuda
Copy link

ruuda commented Jun 18, 2015

The special-case implementation for Result sidesteps the issue of exhaustion, right? I agree with the earlier points that there is no natural preference for exhaustion or not, so we should abstain from picking one altogether.

@tbu-
Copy link
Contributor

tbu- commented Jun 21, 2015

I'm not sure a non-lazy behavior is the right choice here.

In the future we might want to implement lazy FromIterator implementations for tuples of arity n, including n=0. If we implement it this (non-lazy) way now, things will become inconsistent.

@Gankra
Copy link
Contributor

Gankra commented Jun 22, 2015

It seems like there's no agreement on what the behaviour of this code should be. It's not clear to me that it can be agreed on. As such I'd suggest not going forward with this RFC, to avoid constant surprise over this behaviour.

@Aatch
Copy link
Contributor

Aatch commented Jun 23, 2015

I agree with Gankro here. If it's lazy, that seems to contradict the motivation of the RFC, if it's strict, then it's inconsistent with the general behaviour of iterators. Since both interpretations are equally intuitive, I can see this being a source of confusion going forwards.

@alexcrichton
Copy link
Member

The consensus of the libs team is to not merge this RFC at this time, so I'm going to close this. Thanks regardless for the RFC though @fenhl!

@fenhl fenhl deleted the unit-from-iterator branch July 1, 2015 05:12
@fenhl fenhl restored the unit-from-iterator branch July 1, 2015 05:13
@fenhl
Copy link
Author

fenhl commented Nov 15, 2017

The exhausting implementation of this ended up getting implemented in rust-lang/rust#45379.

@fenhl fenhl deleted the unit-from-iterator branch November 15, 2017 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.