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

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

Conversation

w-utter
Copy link

@w-utter w-utter commented Mar 8, 2024

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.

@Philippe-Cholet Philippe-Cholet added the fallible-iterator Iterator of results/options label Mar 8, 2024
Copy link

codecov bot commented Mar 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Philippe-Cholet
Copy link
Member

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.

let mut error = Ok(());
let mut err = Ok(());

//SAFETY: the pointer to err will always be valid thoughout the fns lifetime
Copy link
Contributor

@scottmcm scottmcm Mar 15, 2024

Choose a reason for hiding this comment

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

I'm pretty sure this is incorrect.

By removing the lifetime you decoupled its lifetime from the closure context, which means I could move it out of the FnOnce closure to something that lives longer.

Try something like this https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7d22b4472f094ec36a45b879d6b8e7df

use itertools::*;

fn main() {
    let mut foo = None;
    
    let it = std::iter::repeat(Err::<String, String>("hello".to_string()));
    
    itertools::process_results(it, |x| {
        foo = Some(x);
        Err::<String, String>("world".to_string())
    });
    
    dbg!(foo.unwrap().next());
}

today and you get "error: borrowed data escapes outside of closure".

But if you take the lifetime off of the ProcessResults struct, it'd be allowed, and would be holding a NonNull to a local from the function you've already left, which is definitely not ok if it uses it.

@scottmcm
Copy link
Contributor

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
Copy link
Author

w-utter commented Mar 16, 2024

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!

@w-utter w-utter closed this Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fallible-iterator Iterator of results/options
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants