Skip to content

Add Iterator::single #55355

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 3 commits into from
Closed

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Oct 25, 2018

Tracking issue: #55354

Suggested and discussed in https://internals.rust-lang.org/t/what-do-you-think-about-iterator-single/8608.

Alternatives

Designs

Alternative designs could use a dedicated enum:

enum IteratorSingleError<T> {
    MoreThanOneItem(T),
    EmptyIterator,
}

but I think the common case doesn't need that.
This addition is also cheaper complexity wise this way.

Bikeshed

  • singleton
  • one

I picked single since it strikes a balance between brevity and clarity.

r? @alexcrichton

@Centril Centril added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 25, 2018
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 25, 2018
@rust-lang rust-lang deleted a comment from rust-highfive Oct 25, 2018
@rust-lang rust-lang deleted a comment from rust-highfive Oct 25, 2018
@CAD97
Copy link
Contributor

CAD97 commented Oct 25, 2018

Docs should probably mention that in the too-many-items case, two elements are consumed by the Iterator.

Actually, it can probably be overridden by Peekable to only consume the one, but given it doesn't hand it back, that seems more likely to be surprising than useful.

Prior art:

C#: Enumerable.Single
C#: Enumerable.SingleOrDefault
Kotlin: Iterable.single
Kotlin: Iterable.singleOrNull
crate: single Single::single (version 1.0.0, <1000 downloads, author=me)

@Centril
Copy link
Contributor Author

Centril commented Oct 25, 2018

@CAD97

Docs should probably mention that in the too-many-items case, two elements are consumed by the Iterator.

Hmm... I debated with myself whether I should include such a note but ultimately I decided it was "too much information" / detail; but I don't feel strongly. Do you have some good phrasing I copy into the docs perhaps?

Prior art:

Thanks!

@scottmcm
Copy link
Member

I'm really not a fan of making "0 elements" and "more than one element" indistinguishable. I feel like the whole point of single is that it's a coding error for there to be multiple -- if it's going to be handled, one would just use .next() instead.

@Centril
Copy link
Contributor Author

Centril commented Oct 25, 2018

I feel like the whole point of single is that it's a coding error for there to be multiple

If by "coding error" you mean logic bug, then I disagree. In the cases (proc macros) I wanted .single() the source iterator was from user data and I the relevant condition was whether there was a single element...

if it's going to be handled, one would just use .next() instead.

Idk what that means... elaborate?

I feel strongly that the Result<T, IteratorSingleError<T>> encoding is the wrong default; I think the common case works better with Option<T>. If you need the extra control and want to distinguish between zero and 2+, then you can just use match or if let, i.e. the manual approach.
Even worse would be to panic in this case, that would be essentially useless for me.

@clarfonthey
Copy link
Contributor

Seems silly, but why not Result<T, Option<T>>?

@clarfonthey
Copy link
Contributor

Actually, it would be Option<[T; 2]> ideally.

@Centril
Copy link
Contributor Author

Centril commented Oct 27, 2018

@clarcharr

Seems silly, but why not Result<T, Option<T>>?

That's not so bad actually... I could be convinced in favor of it since it doesn't invent new types...
I just don't think it's so common to care about the extra value if you call .single()... why do you believe the contrary?

Actually, it would be Option<[T; 2]> ideally.

So Result<T, Option<[T; 2]>>? I suppose... if you really insist on getting all the information...
It's starting to get more complex now tho... why not: Result<T, Option<(T, T)>>?
What makes one better than the other? I think we should think about what a good default is...

@CAD97
Copy link
Contributor

CAD97 commented Oct 27, 2018

I'm not sure how best the docs could mention that two elements of the iterator are consumed.
Returning the elements would make this clear by showing rather than telling, though.

But I'm pretty certain that most uses of this fn would be when the author is reasonably certain that the iterator should have a single element, and otherwise is a logic error or input error.

There is benefit in returning the second item in the most common case I've seen for ::single (proc-macro derive attributes), though: emitting an error for duplicate attribute that points to the duplicate attribute.

In conclusion, I have confused myself out of what would be the ideal, canonical signature for this fn. I think I lean very weakly towards -> Result<T, Option<[T; 2]>> over -> Result<T, Option<(T, T)>>, as it communicates the order.

@Centril
Copy link
Contributor Author

Centril commented Oct 28, 2018

I'm not sure how best the docs could mention that two elements of the iterator are consumed.

I guess we could just say "the method consumes at most two elements from the iterator and does not exhaustively consume the entire iterator" or something.

But I'm pretty certain that most uses of this fn would be when the author is reasonably certain that the iterator should have a single element, and otherwise is a logic error or input error.

Logic errors or input errors are very different and shouldn't be handled the same way.
I cannot speak for anyone else, but this was never the case for me that it was a logic error...
I always wanted to know explicitly with a value when there was not exactly one element left so I could handle it properly; but I didn't care if there was zero or 2+.

In conclusion, I have confused myself out of what would be the ideal, canonical signature for this fn. I think I lean very weakly towards -> Result<T, Option<[T; 2]>> over -> Result<T, Option<(T, T)>>, as it communicates the order.

I buy that I guess; I could go with -> Result<T, Option<[T; 2]>> over -> Option<T> since it isn't too cumbersome to call .ok() or just let Ok(x) = iter.single() instead...

Should we keep it as is?, change it?, wait for someone on T-libs before landing?

@Centril
Copy link
Contributor Author

Centril commented Oct 30, 2018

cc @alexcrichton any chance you might get to this (there are some minor open design questions here for you to resolve...)?

@alexcrichton
Copy link
Member

Er well I don't think I'm personally on the hook to solve any design questions here. If this isn't ready to land then I think we should close this PR until those design questions are fleshed out, or they can be hashed out on thread as well.

My personal gut reaction to this method is that it's not worth it to add to the standard library, so I don't really have many opinions about the precise pieces here. If signatures like Result<T, Option<[T; 2]>> are being considered though that sounds like this belongs not in libstd.

@Centril
Copy link
Contributor Author

Centril commented Nov 10, 2018

@alexcrichton

Er well I don't think I'm personally on the hook to solve any design questions here.

Eh well... The language team is on the hook to resolve (not solve) design questions about the language so it's not too crazy for y'all to resolve libs questions and pick between designs. :)

If this isn't ready to land then I think we should close this PR until those design questions are fleshed out, or they can be hashed out on thread as well.

Perhaps we can land the PR as-is and get some experience on nightly then?
I personally am happy with the design as-is and N.B. the other PR proposed the exact same signature.

My personal gut reaction to this method is that it's not worth it to add to the standard library, so I don't really have many opinions about the precise pieces here.

Can you elaborate on why perhaps? I'd like to note that numerous people noted that this method was useful to them on internals (see https://internals.rust-lang.org/t/what-do-you-think-about-iterator-single/8608).

If signatures like Result<T, Option<[T; 2]>> are being considered though that sounds like this belongs not in libstd.

OK but then it seems like you do have some opinions about the design (namely that the signature as-is in the PR belongs more in libcore than other signatures?).

@alexcrichton
Copy link
Member

@Centril you singled me out specifically as specifically getting to this and solving open design problems here. It's not really my job, nor the libs team's, to solve any arbitrary bikeshed presented to them. We're all busy folks and are balancing contributing and reviewing with other tasks, and if something doesn't grab anyone's attention or doesn't seem like it should go in libstd, then it ends up not doing so.

Modifying core traits like Iterator can pretty easy lead to compilation breakage in downstream crates due to multiple methods that apply (especially short names like single), so we're hesitant to experiment with methods like this on nightly unless we're quite sure we want to land it.

Iterators are used by practically everyone who uses Rust, so no matter what method you propose for an iterator some subset of folks will want to see it included in libstd and standardized. To me, however, this seems like very niche functionality where you want a guarantee that there's only one element in the iterator rather than just the next element. Start talking about recovering the iterator and item in erroneous cases and that's just way oer the top for me to be included in libstd.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants