Skip to content

Add impl to collect an iterator of Result<T,E> into (Collection<T>, Collection<E>) #87047

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Add impl to collect an iterator of Result<T,E> into (Collection<T>, Collection<E>) #87047

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions library/core/src/result.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1626,6 +1626,25 @@ impl<A, E, V: FromIterator<A>> FromIterator<Result<A, E>> for Result<V, E> {
}
}

//FIXME: Figure out how to mark this as unstable without x.py refusing to run any tests
#[stable(feature = "tuple_from_result_iter", since = "1.53.0")]
Copy link
Author

@Svetlitski Svetlitski Jul 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This obviously should be marked as unstable, but every time I did so, it would cause x.py to refuse any attempt I made to run the tests, instead exiting with the message:

error: an `#[unstable]` annotation here has no effect
    --> library/core/src/result.rs:1630:1
     |
1630 | #[unstable(feature = "tuple_from_result_iter", issue = "87047")]
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: `#[deny(ineffective_unstable_trait_impl)]` on by default
     = note: see issue #55436 <https://github.com/rust-lang/rust/issues/55436> for more information

Thus I temporarily marked it as stable as a kludge to get the tests to run. I realize there is probably just a mistake I'm making as a result of this being my first contribution here, so I'd appreciate some help resolving this small detail.

Copy link
Member

Choose a reason for hiding this comment

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

Impls can't be unstable, see the linked issue. They're always insta-stable.

Copy link
Author

Choose a reason for hiding this comment

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

I see. I read the issue before but was confused because there is an impl just a few lines below where I inserted my code (line 1649, impl<T, E> ops::TryV2 for Result<T, E>) which is marked as unstable, but upon re-reading the issue I realize the difference is that there the trait itself is unstable.

impl<T, E, U, V> FromIterator<Result<T, E>> for (U, V)
where
U: Default + Extend<T>,
V: Default + Extend<E>,
{
fn from_iter<I: IntoIterator<Item = Result<T, E>>>(iter: I) -> (U, V) {
let (mut oks, mut errs) = (U::default(), V::default());
for result in iter {
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't this use 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.

Just wondering, why did this got a negative feedback? Internal iteration should always be either as fast as or faster than external iteration, so you should always try to use it if you can, hence why I would have used for_each instead of a normal for loop here.

Copy link
Member

Choose a reason for hiding this comment

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

Internal iteration should always be either as fast as or faster than external iteration, so you should always try to use it if you can

I disagree with this. for_each introduces a lot more syntactic noise and does the same thing as a normal for loop. If this happens to be super slow for some reason it seems fine to use for_each, but I don't think it should be the default.

Copy link
Member

Choose a reason for hiding this comment

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

Also, you didn't mention at first that this was for performance reasons, so I thought it was just personal preference.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this happens to be super slow for some reason it seems fine to use for_each,

It's already pretty slow for iterators like RangeInclusive and VecDeque, and probably many iterator adapters that need to do additional checks in each iteration.

but I don't think it should be the default.

But isn't it already the default in pretty much all the stdlib? Iterator's utility methods and any implementation of FromIterator, Extend, Sum and Product all use internal iteration methods. The only occurences of plain for loops I can find are where performance doesn't matter (for example tests) or when the iterator doesn't gain anything with internal iteration (for example exclusive ranges, slices and vecs).

Also, you didn't mention at first that this was for performance reasons, so I thought it was just personal preference.

Yeah my bad for that one, I was assuming that the difference between for and for_each is only performance.

match result {
Ok(ok) => oks.extend_one(ok),
Err(err) => errs.extend_one(err),
}
}
(oks, errs)
}
}

#[unstable(feature = "try_trait_v2", issue = "84277")]
impl<T, E> ops::TryV2 for Result<T, E> {
type Output = T;
Expand Down
18 changes: 18 additions & 0 deletions library/core/tests/result.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,3 +402,21 @@ fn result_try_trait_v2_branch() {
assert_eq!(Ok::<NonZeroU32, ()>(one).branch(), Continue(one));
assert_eq!(Err::<NonZeroU32, ()>(()).branch(), Break(Err(())));
}

#[test]
fn tuple_from_result_iter() {
let results = [Ok(1), Err(false), Ok(3), Ok(4), Err(true)];
let (oks, errs) = IntoIterator::into_iter(results).collect::<(Vec<_>, Vec<bool>)>();
assert_eq!(oks, [1, 3, 4]);
assert_eq!(errs, [false, true]);
// All `Ok`s
let results = [Ok(5), Ok(6), Ok(7)];
let (oks, errs) = IntoIterator::into_iter(results).collect::<(Vec<_>, Vec<String>)>();
assert_eq!(oks, [5, 6, 7]);
assert_eq!(errs, [] as [String; 0]);
// All `Errs`s
let results: [Result<i32, _>; 2] = [Err("hello"), Err("world")];
let (oks, errs) = IntoIterator::into_iter(results).collect::<(Vec<i32>, Vec<_>)>();
assert_eq!(oks, [] as [i32; 0]);
assert_eq!(errs, ["hello", "world"]);
}