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.

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

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
28 changes: 17 additions & 11 deletions src/process_results_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#[cfg(doc)]
use crate::Itertools;
use core::ptr::NonNull;

/// An iterator that produces only the `T` values as long as the
/// inner iterator produces `Ok(T)`.
Expand All @@ -8,12 +9,12 @@ use crate::Itertools;
/// for more information.
#[must_use = "iterator adaptors are lazy and do nothing unless consumed"]
#[derive(Debug)]
pub struct ProcessResults<'a, I, E: 'a> {
error: &'a mut Result<(), E>,
pub struct ProcessResults<I, E> {
error: NonNull<Result<(), E>>,
iter: I,
}

impl<'a, I, T, E> Iterator for ProcessResults<'a, I, E>
impl<I, T, E> Iterator for ProcessResults<I, E>
where
I: Iterator<Item = Result<T, E>>,
{
Expand All @@ -23,7 +24,10 @@ where
match self.iter.next() {
Some(Ok(x)) => Some(x),
Some(Err(e)) => {
*self.error = Err(e);
//SAFETY: the pointer is always valid while iterating over items
unsafe {
*self.error.as_mut() = Err(e);
}
None
}
None => None,
Expand All @@ -39,7 +43,9 @@ where
Self: Sized,
F: FnMut(B, Self::Item) -> B,
{
let error = self.error;
//SAFETY: the pointer is always valid while iterating over items
let error = unsafe { self.error.as_mut() };

self.iter
.try_fold(init, |acc, opt| match opt {
Ok(x) => Ok(f(acc, x)),
Expand All @@ -62,12 +68,12 @@ where
F: FnOnce(ProcessResults<I::IntoIter, E>) -> R,
{
let iter = iterable.into_iter();
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 •
edited
Loading

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.

let error = unsafe { NonNull::new_unchecked(&mut err) };

let result = processor(ProcessResults {
error: &mut error,
iter,
});
let result = processor(ProcessResults { error, iter });

error.map(|_| result)
err.map(|_| result)
}