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

StreamExt::scan: specified lifetime requirements #2046

Closed
wants to merge 8 commits into from

Conversation

olegnn
Copy link
Contributor

@olegnn olegnn commented Jan 22, 2020

As mentioned in #2044 (comment), current scan realisation doesn't allow to capture &mut state in future. This PR adds adds lifetime requirements and allows to use &mut state in async functions and blocks. @cramertj is this solution ok?

@cramertj
Copy link
Member

Unfortunately this is unsound-- it's giving the closure access to the mutable reference for the entire lifetime 'a, while it can only soundly access the mutable reference until the next time the closure is called with said mutable reference, or the structure is destroyed. This becomes clearer if you think about that if the state type (S) has no lifetime parameters, you can choose 'a to be 'static. Now the closure is being called with a &'static mut S, which is clearly nonsense.

@cramertj
Copy link
Member

The real type of the closure you'd want is something like for<'a> F: FnMut<&'a mut S, Self::Item, Output: Future<Output = Option<B>>>.

@olegnn
Copy link
Contributor Author

olegnn commented Jan 22, 2020

@cramertj but future returned by provided function is stored internally in Scan structure and can only be executed from within poll function when stream will be polled. If incomplete stream will be dropped, this future will be also. Even if future will have static lifetime, I still don’t see an issue because it’s internal.

@olegnn
Copy link
Contributor Author

olegnn commented Jan 22, 2020

I firstly tried higher rank trait bound of course, but it doesn’t work properly yet :(

@olegnn
Copy link
Contributor Author

olegnn commented Jan 22, 2020

Way to “hack” which I see is to spawn a thread from function with mutable reference or assign it to static items/other long-lived variables in scope. But I need to check if it compiles.

@cramertj
Copy link
Member

. Even if future will have static lifetime, I still don’t see an issue because it’s internal.

The reference it gets will have a 'static lifetime, so it can save that reference somewhere outside of the call, or pass it to a function like spawn which lets it continue to be used mutably forever.

I firstly tried higher rank trait bound of course, but it doesn’t work properly yet :(

Yup, unfortunately there are a bunch of bugs around HRTB that make this not work yet. See #1023 for my own struggle to make this work. Maybe we can bug @nikomatsakis or someone else to take a look at fixing rust-lang/rust#51004 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants