-
-
Notifications
You must be signed in to change notification settings - Fork 33
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
Make generated 'project' reference take an '&mut Pin<&mut Self>' #47
Conversation
Based on rust-lang/unsafe-code-guidelines#148 (comment) by @CAD97 Currently, the generated 'project' method takes a 'Pin<&mut Self>', consuming it. This makes it impossible to use the original Pin<&mut Self> after calling project(), since the 'Pin<&mut Self>' has been moved into the the 'Project' method. This makes it impossible to implement useful pattern when working with enums: ```rust enum Foo { Variant1(#[pin] SomeFuture), Variant2(OtherType) } fn process(foo: Pin<&mut Foo>) { match foo.project() { __FooProjection(fut) => { fut.poll(); let new_foo: Foo = ...; foo.set(new_foo); }, _ => {} } } ``` This pattern is common when implementing a Future combinator - an inner future is polled, and then the containing enum is changed to a new variant. However, as soon as 'project()' is called, it becoms imposible to call 'set' on the original 'Pin<&mut Self>'. To support this pattern, this commit changes the 'project' method to take a '&mut Pin<&mut Self>'. The projection types works exactly as before - however, creating it no longer requires consuming the original 'Pin<&mut Self>' Unfortunately, current limitations of Rust prevent us from simply modifiying the signature of the 'project' method in the inherent impl of the projection type. While using 'Pin<&mut Self>' as a receiver is supported on stable rust, using '&mut Pin<&mut Self>' as a receiver requires the unstable `#![feature(arbitrary_self_types)]` For compatibility with stable Rust, we instead dynamically define a new trait, '__{Type}ProjectionTrait', where {Type} is the name of the type with the `#[pin_project]` attribute. This trait looks like this: ```rust trait __FooProjectionTrait { fn project(&'a mut self) -> __FooProjection<'a>; } ``` It is then implemented for `Pin<&mut {Type}>`. This allows the `project` method to be invoked on `&mut Pin<&mut {Type}>`, which is what we want. If Generic Associated Types (rust-lang/rust#44265) were implemented and stablized, we could use a single trait for all pin projections: ```rust trait Projectable { type Projection<'a>; fn project(&'a mut self) -> Self::Projection<'a>; } ``` However, Generic Associated Types are not even implemented on nightly yet, so we need for generate a new trait per type for the forseeable future.
This ensures that we bail out early if we encounter an error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was using .as_mut()
to do that, but this is more convenient. Thanks!
@@ -7,6 +7,10 @@ pub(crate) fn proj_ident(ident: &Ident) -> Ident { | |||
format_ident!("__{}Projection", ident) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way to refer the projected-type should be improved in #46, but should this be format_ident!("{}Projection", ident)
?
Personally, I don't want to recommend relying on the internal implementation of proc-macro, but I don't plan to change this frequently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @Nemo157
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only used the type name for the example in #43 because it was the only way to implement it then, I'm not using it in any real code and wouldn't expect it to stay the same across builds (I would even be for hashing the input AST and sticking part of that hash in the generated type names to block anyone actually relying on it with any reliability).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, thanks for clarifying.
I would even be for hashing the input AST and sticking part of that hash in the generated type names to block anyone actually relying on it with any reliability
Unfortunately, I think this is difficult because #[project]
depends on the current behavior (simple naming). So for now, I will keep the current one.
bors r+ |
47: Make generated 'project' reference take an '&mut Pin<&mut Self>' r=taiki-e a=Aaron1011 Based on rust-lang/unsafe-code-guidelines#148 (comment) by @CAD97 Currently, the generated 'project' method takes a 'Pin<&mut Self>', consuming it. This makes it impossible to use the original Pin<&mut Self> after calling project(), since the 'Pin<&mut Self>' has been moved into the the 'Project' method. This makes it impossible to implement useful pattern when working with enums: ```rust enum Foo { Variant1(#[pin] SomeFuture), Variant2(OtherType) } fn process(foo: Pin<&mut Foo>) { match foo.project() { __FooProjection(fut) => { fut.poll(); let new_foo: Foo = ...; foo.set(new_foo); }, _ => {} } } ``` This pattern is common when implementing a Future combinator - an inner future is polled, and then the containing enum is changed to a new variant. However, as soon as 'project()' is called, it becoms imposible to call 'set' on the original 'Pin<&mut Self>'. To support this pattern, this commit changes the 'project' method to take a '&mut Pin<&mut Self>'. The projection types work exactly as before - however, creating it no longer requires consuming the original 'Pin<&mut Self>' Unfortunately, current limitations of Rust prevent us from simply modifying the signature of the 'project' method in the inherent impl of the projection type. While using 'Pin<&mut Self>' as a receiver is supported on stable rust, using '&mut Pin<&mut Self>' as a receiver requires the unstable `#![feature(arbitrary_self_types)]` For compatibility with stable Rust, we instead dynamically define a new trait, '__{Type}ProjectionTrait', where {Type} is the name of the type with the `#[pin_project]` attribute. This trait looks like this: ```rust trait __FooProjectionTrait { fn project(&'a mut self) -> __FooProjection<'a>; } ``` It is then implemented for `Pin<&mut {Type}>`. This allows the `project` method to be invoked on `&mut Pin<&mut {Type}>`, which is what we want. If Generic Associated Types (rust-lang/rust#44265) were implemented and stabilized, we could use a single trait for all pin projections: ```rust trait Projectable { type Projection<'a>; fn project(&'a mut self) -> Self::Projection<'a>; } ``` However, Generic Associated Types are not even implemented on nightly yet, so we need to generate a new trait per type for the foreseeable future. Co-authored-by: Aaron Hill <[email protected]>
Build succeeded
|
That is not true, you can just reborrow the Is there any other reason why double pointer-indirections are used? If not, I suggest to revert this. |
I was aware of it, but I approved this PR because I felt it would be more convenient to call it without However, in reality, I missed some of the drawbacks (#65, #21 (comment), etc.). So now I feel that this approach is not always convenient. |
Filed #89 |
Based on rust-lang/unsafe-code-guidelines#148 (comment)
by @CAD97
Currently, the generated 'project' method takes a 'Pin<&mut Self>',
consuming it. This makes it impossible to use the original Pin<&mut Self>
after calling project(), since the 'Pin<&mut Self>' has been moved into
the the 'Project' method.
This makes it impossible to implement useful pattern when working with
enums:
This pattern is common when implementing a Future combinator - an inner
future is polled, and then the containing enum is changed to a new
variant. However, as soon as 'project()' is called, it becoms imposible
to call 'set' on the original 'Pin<&mut Self>'.
To support this pattern, this commit changes the 'project' method to
take a '&mut Pin<&mut Self>'. The projection types work exactly as
before - however, creating it no longer requires consuming the original
'Pin<&mut Self>'
Unfortunately, current limitations of Rust prevent us from simply
modifying the signature of the 'project' method in the inherent impl
of the projection type. While using 'Pin<&mut Self>' as a receiver is
supported on stable rust, using '&mut Pin<&mut Self>' as a receiver
requires the unstable
#![feature(arbitrary_self_types)]
For compatibility with stable Rust, we instead dynamically define a new
trait, '__{Type}ProjectionTrait', where {Type} is the name of the type
with the
#[pin_project]
attribute.This trait looks like this:
It is then implemented for
Pin<&mut {Type}>
. This allows theproject
method to be invoked on
&mut Pin<&mut {Type}>
, which is what we want.If Generic Associated Types (rust-lang/rust#44265)
were implemented and stabilized, we could use a single trait for all pin
projections:
However, Generic Associated Types are not even implemented on nightly
yet, so we need to generate a new trait per type for the foreseeable
future.