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

Add future::pinned combinator #1023

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cramertj
Copy link
Member

This is a preliminary attempt at a massively unsafe manual self-borrowing future to allow working with borrowing futures combinators without relying on async/await. I'm uncomfortable about the number of different crazy hacks happening here-- hopefully we can whittle those down. Ideally this wouldn't be replaced entirely by async/await someday, at which point we can remove this, but it could allow moving stable code over to the new borrowing versions of e.g. Stream::next and AsyncRead::read.

I'm not even sure that this is possible to express correctly in Rust's surface syntax, since the Self type contains a value as well as a live field with a live &mut inside of it. UnsafeCell isn't enough here, and I don't know what is. Perhaps it's enough to know that we never create an &mut reference to self.data while self.fn_or_fut holds a future containing a reference to self.data? I wouldn't have thought so, but I don't know what to do otherwise. cc @Zoxc and @alexcrichton, who know how self-referential generators are implemented-- do they rely on special magic for this? cc also @RalfJung, primarily so they're aware of the crazy pinning shenanigans happening here.

@cramertj cramertj requested a review from aturon May 23, 2018 01:36
/// Created by the `borrowed` function.
#[must_use = "futures do nothing unless polled"]
#[allow(missing_debug_implementations)]
pub struct PinnedFut<'any, Data: 'any, Output, F: PinnedFn<Data, Output> + 'any> {
Copy link
Member Author

Choose a reason for hiding this comment

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

I can't actually make this work because I get stuck on that PinnedFut isn't allowed to outlive 'any no matter what I do. This seems like I need #[may_dangle], but somehow stronger. I don't even need a lifetime to be part of the type-- I just need a lifetime that does not outlive Data (can't always use 'static) but which will unify with other lifetimes.

Copy link
Member Author

@cramertj cramertj May 23, 2018

Choose a reason for hiding this comment

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

Even using 'static doesn't work-- then the implied WF checks for HRTB aren't kicking in for some reason: i get errors that T: for<'a> PinnedFnLt<'a, Data, Output> isn't satisfied if I add a where Self: 'a bound to PinnedFnLt.

@cramertj
Copy link
Member Author

cramertj commented May 23, 2018

Okay, I've got a minimal reproduction of the issue here. It seems like higher ranked bounds for traits implemented by closures like this don't work.

@cramertj
Copy link
Member Author

Opened an issue: rust-lang/rust#51004

// TODO:
// marker: Pinned,
// Data, which may be borrowed by `fn_or_fut`, must be dropped last
data: Data,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should use ManuallyDrop so as to not rely on drop order?

(AFAIK drop order is actually "first field is dropped last", so this may even be wrong right now.)

@RalfJung
Copy link
Member

I don't think I really understand what is going on here... what is the crazy stuff that's going on here, other than some funny shenanigans with for<'a> ... as a super trait bound?

@cramertj
Copy link
Member Author

@RalfJung One of the fields is being allowed to hold a PinMut reference into another field, and it may mutate that field.

@MajorBreakfast MajorBreakfast changed the base branch from 0.3 to master July 19, 2018 20:03
@taiki-e taiki-e added the S-blocked Status: Blocked on something else label Nov 15, 2020
@taiki-e taiki-e added A-future Area: futures::future and removed futures-util labels Dec 17, 2020
@cramertj
Copy link
Member Author

Well, you can get a little bit further with this nowadays on nightly, but still the actual usage fails due to the way elision inside closures works: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=b480eb854734a23aa525cf60dc71a40d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-future Area: futures::future S-blocked Status: Blocked on something else
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants