rust-itertools / itertools

Extra iterator adaptors, iterator methods, free functions, and macros.
https://docs.rs/itertools/
Apache License 2.0
2.64k stars 299 forks source link

Removed lifetimes in "process_results" in favour of using NonNull #897

Closed w-utter closed 4 months ago

w-utter commented 4 months ago

There were some lifetime issues that I was running into trying to directly raise an impl Iterator<Item = Result<T, E>> to a Result<impl Iterator<Item = T>, E> saying something along the lines that the lifetimes were incompatible. However when changing to NonNull these errors went away, and works the same way as before.

This will coincide with a wrapper fn that handles doing iterator.into_iter().process_results(|iter| iter.map(|item| item)) which I will work on shortly.

Just as a note, In the future this api should probably take in all T who implement Try just so that this would be possible with options and other user generated objects, but that is out of the scope of this pr.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.65%. Comparing base (6814180) to head (b78f6b4). Report is 31 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #897 +/- ## ========================================== + Coverage 94.38% 94.65% +0.26% ========================================== Files 48 48 Lines 6665 6754 +89 ========================================== + Hits 6291 6393 +102 + Misses 374 361 -13 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Philippe-Cholet commented 4 months ago

In #710, I said that process_results "should" take &mut self instead of self, in which case the lifetime you remove here is useful I think. I'm gonna think about this.

And as an iterator of results, you might want to take a look at #844.

scottmcm commented 4 months ago

trying to directly raise an impl Iterator<Item = Result<T, E>> to a Result<impl Iterator<Item = T>, E>

Note that while this is an obvious wish, it's fundamentally not possible while being lazy.

No matter how much of the iterator you've looked at and only seen Oks, there always might be an Err later, but there also might not. So you need to look at unboundedly much of the iterator to decide whether to return Ok or Err, and that's fundamentally not lazy -- you'd need to save all the values from those Oks you're seeing somewhere in order to be able to return them later.

And that's called Result<Vec<T>, E>: FromIterator.

w-utter commented 4 months ago

Ah, wrote this a little hastily while trying to get collect_into::<Result<Vec<T>>() and similar to work without thinking of the ramifications of removing the associated lifetime. Currently the std library requires the iterator being collected into to implement extend which i thought could be bypassed by using this kind of adapter. Looking at some other material I realize that there are some other ways of doing it. Thanks for the explanation!