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

Enable draining iterator/stream elements into a () #94166

Closed
wants to merge 2 commits into from

Conversation

ibraheemdev
Copy link
Member

The current implementation of Extend for () was added in #50234 because:

This is useful in some generic code which wants to collect iterators of items into a result.

Making the implementation generic makes something like stream.collect::<()> possible for any stream, vs. stream.map(drop).collect::<()>.

Similarly, impl<T> FromIterator<T> for () makes iter.collect::<()> a generic replacement for iter.for_each(drop).

@rust-highfive
Copy link
Collaborator

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 19, 2022
@ibraheemdev ibraheemdev changed the title Make Enable draining iterator/stream elements into a () Feb 19, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member

compiler-errors commented Feb 19, 2022

This might affect inference in cases like:

struct Baz;

impl Into<()> for Baz {
    fn into(self) -> () { () }
}

impl Into<i32> for Baz {
    fn into(self) -> i32 { 0 }
}

fn main() {
    let _: () = vec![Baz].into_iter().map(|b| b.into()).collect();
}

iter.into_iter().for_each(|()| {})
impl<T> FromIterator<T> for () {
fn from_iter<A: IntoIterator<Item = T>>(iter: A) -> Self {
iter.into_iter().for_each(|_| {})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
iter.into_iter().for_each(|_| {})
iter.into_iter().for_each(drop)

Maybe this should also use .for_each(drop) to clarify the intent better? It doesn't matter much for (), but for generic T that seems better.

@yaahc yaahc added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 18, 2022
@yaahc
Copy link
Member

yaahc commented Apr 15, 2022

What is the motivating use case for this PR? To me iter.collect::<()>() seems like a strictly worse version of iter.for_each(drop), so I am against this addition without an explanation for how the former supports use cases that the latter does not.

@ibraheemdev
Copy link
Member Author

The use case I ran into was running an iterator completion, stopping on the first error, but not caring about the success type. iter.collect::<Result<_, Vec<_>>>? works but allocates the Oks needlessly. iter.map(|r| r.map(drop)).collect::<Result<_, Vec<()>>> is the best current solution I could come up with.

@yaahc
Copy link
Member

yaahc commented Apr 15, 2022

The use case I ran into was running an iterator completion, stopping on the first error, but not caring about the success type. iter.collect::<Result<_, Vec<_>>>? works but allocates the Oks needlessly. iter.map(|r| r.map(drop)).collect::<Result<_, Vec<()>>> is the best current solution I could come up with.

I think I understand what you mean but the example you gave is a little confusing. You mentioned stopping on the first error but then the snippet shows a Vec in the error type, which seems contradictory. I'm guessing that the Vec placement is a typo given that our FromIterator impl for Result doesn't seem to support collecting multiple errors, but please let me know if I'm missing something.

Regardless, In this case I'd still prefer to push people towards the explicit case. I don't think that collecting into () implying drop is intuitive and I am also worried this could possibly introduce footguns such as accidentally inferring (), tho this risk seems much less significant compared to the readability / discoverability issues. Is the issue here the verbosity of .map(|r| r.map(drop))? If so it seems like the better solution might be to pursue a .map_continue(drop) combinator on Result instead.

cc @scottmcm

@ibraheemdev
Copy link
Member Author

Yes, sorry, I meant iter.collect::<Result<Vec<_>, _>>?.

@yaahc
Copy link
Member

yaahc commented Apr 15, 2022

And in the case where you drop there's no need to collect it into a Vec at all right? So ideally you want to be able to write something like iter.map_continue(drop).collect::<Result<(), _>>()?; right?

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=aadf15e698726ac3465be4878315ebfd

@ibraheemdev
Copy link
Member Author

Right, yes, iter.map_ok(drop).collect::<Result<(), _>()? works currently. Ideally I would like to be able to write iter.collect::<Result<(), _>()? directly. I do think (): FromIterator<T> is a useful abstraction.

@compiler-errors
Copy link
Member

I still think that breaking inference is a concern, and possibly does not outweigh the convenience of not having to map the elements under drop.

I guess we could do crater to check if there are people actually using the Iterator<Item = _> into () collect in a way that depends on inferring _ = () currently. Maybe fallback catches this though.

@scottmcm
Copy link
Member

I've never really been a fan of this, going back to even when it was first added: #45379 (comment)

I can accept it for (), since ZSTs are special, and thus I can convince myself that "sure, a () is a container of arbitrarily-many ()s", since it's justifiable to collect zero bits of state into zero bits.

But when actually throwing stuff away, I think .for_each(drop) is better than .collect() as it emphasizes what's going on, especially since it's definitely not "collecting" anything. And we already recommend .for_each(drop) as the right way to manually exhaust an iterator if you need the side effects:

#[must_use = "if you really need to exhaust the iterator, consider `.for_each(drop)` instead"]

So like #64117 (comment), I'd propose just closing this with reference to #48945 (comment).

@yaahc
Copy link
Member

yaahc commented Apr 19, 2022

@scottmcm I think this case is slightly different from those because it is simultaneously dropping the Ok variants while wanting to short circuit and propagate out any Err variants, but I think that's still well supported by try_for_each.

iter.try_for_each(|r| {
    let _ = r?;
    Ok(())
})?;

I agree with wanting to close this though, especially given that this is an insta-stable change. @ibraheemdev I appreciate the work you've put into this, but I don't think we should add this change to the standard library. I'm going to close this for now. If additional reasons come up, or additional information changes some of these conclusions, we can always reopen this PR. Regardless, thank you for your work.

@yaahc yaahc closed this Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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.

7 participants