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

Allow using #[pin_project] type with private field types #53

Merged
merged 1 commit into from
Aug 29, 2019

Conversation

Aaron1011
Copy link
Collaborator

Previously, given code such as:

struct Private<T>;

#[pin_project]
pub struct Public<T> {
    #[pin] private: Private<T>
}

we would generate an Unpin impl like this:

impl Unpin for Public where Private: Unpin {}

Unfortunately, since Private is not a public type,
this would cause an E0446 ('private type Private in public interface)

When RFC 2145 is implemented (rust-lang/rust#48054),
this will become a lint, rather then a hard error.

In the time being, we need a solution that will work with the current
type privacy rules.

The solution is to generate code like this:

fn __private_scope() {
    pub struct __UnpinPublic<T> {
        __field0: Private<T>
    }
    impl<T> Unpin for Public<T> where __UnpinPublic<T>: Unpin {}
}

That is, we generate a new struct, containing all of the pinned
fields from our #[pin_project] type. This struct is delcared within
a function, which makes it impossible to be named by user code.
This guarnatees that it will use the default auto-trait impl for Unpin -
that is, it will implement Unpin iff all of its fields implement Unpin.
This type can be safely declared as 'public', satisfiying the privacy
checker without actually allowing user code to access it.

This allows users to apply the #[pin_project] attribute to types
regardless of the privacy of the types of their fields.

@Aaron1011 Aaron1011 force-pushed the fix/private-in-public branch from 58caa5f to 6da1242 Compare August 25, 2019 21:30
Copy link
Owner

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Looks good to me aside from one concern. Thanks!

pin-project-internal/src/pin_project/mod.rs Outdated Show resolved Hide resolved
#[allow(private_in_public)]
fn test_private_type_in_public_type() {
#[pin_project]
pub struct PublicStruct<T> {
Copy link
Owner

Choose a reason for hiding this comment

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

My concern is that this generates the following documents (due to rust-lang/rust#63281):

impl<T> Unpin for PublicStruct<T>
where
    __UnpinStructPublicStruct<T>: Unpin, 

Copy link
Owner

Choose a reason for hiding this comment

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

Even if UnsafeUnpin is used, a similar document is generated, but UnsafeUnpin impl is also documented at the same time, so we can know which type needs to be Unpin.

impl<T> Unpin for Foo<T>
where
    Wrapper<Self>: UnsafeUnpin,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this would be fine if __UnpinStructPublicStruct had documentation generated - that would allow users to use --document-private-items to seewhen it implemented Unpin. However, the fact that __UnpinStructPublicStruct is defined within a function seems to be preventing rustdoc from documenting it.

I'll investigate further.

@taiki-e taiki-e added this to the v0.4 milestone Aug 27, 2019
@Aaron1011
Copy link
Collaborator Author

Aaron1011 commented Aug 28, 2019

@taiki-e: Unfortunately, there doesn't appear to be a simple way to allow documenting the Unpin impl on stable rust. There are two ways we can make the __UnpinStruct$Ident structs inaccessible to user code (which prevents them from manually implementing Unpin):

  1. Declare the struct inside a new function: e.g. fn foo() { pub struct __UnpinStructFoo; } While this prevents user code from referencing __UnpinStructFoo, it also prevents rustdoc from generating documentation for __UnpinStructFoo.
  2. Declare the struct within nested private modules: e.g. mod outer { mod inner { pub struct __UnpinStructFoo; } } User code will be able to reference outer, but will not be able to reference __UnpinStructFoo inside inner. However, this does not work when our original type is itself declared within a function (e.g. fn foo() { #[pin_project] struct Foo }. From within inner, it is impossible to reference any type declared within the containing function.

However, a solution is possible on nightly. When compiling on nightly, we can make use of the unstable Span::def_site() constructor. When such a span is used with an identifier, that identifier opts into def-site hygiene - that is, it becomes impossible for user code to even refer to that identifier (even if it's public!).

By using a build script, we can get the best of both worlds. At compile time, we detect the current rust channel. If we're on nightly, we use the nightly-only Span::def_site. If we're on stable, we use a normal span, but wrap our generated code in a function.

Since docs.rs uses nightly rust, libraries will automatically get the nicer documentation when built on docs.rs. Users building documentation locally can use cargo +nightly doc, but still target stable rust.

@taiki-e
Copy link
Owner

taiki-e commented Aug 29, 2019

@Aaron1011: Okay, thanks for the investigation.

Since docs.rs uses nightly rust, libraries will automatically get the nicer documentation when built on docs.rs. Users building documentation locally can use cargo +nightly doc, but still target stable rust.

Should we provide a way to opt-out of this? (the documents generated this way is probably not very clean) cc @Nemo157

Copy link
Owner

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

r=me with #53 (comment) addressed.

@taiki-e
Copy link
Owner

taiki-e commented Aug 29, 2019

bors delegate+

@bors
Copy link
Contributor

bors bot commented Aug 29, 2019

✌️ Aaron1011 can now approve this pull request

Previously, given code such as:

```rust

struct Private<T>;

pub struct Public<T> {
    #[pin] private: Private<T>
}
```

we would generate an Unpin impl like this:

```rust
impl Unpin for Public where Private: Unpin {}
```

Unfortunately, since Private is not a public type,
this would cause an E0446 ('private type `Private` in public interface)

When RFC 2145 is implemented (rust-lang/rust#48054),
this will become a lint, rather then a hard error.

In the time being, we need a solution that will work with the current
type privacy rules.

The solution is to generate code like this:

```rust

fn __private_scope() {
    pub struct __UnpinPublic<T> {
        __field0: Private<T>
    }
    impl<T> Unpin for Public<T> where __UnpinPublic<T>: Unpin {}
}
```

That is, we generate a new struct, containing all of the pinned
fields from our #[pin_project] type. This struct is delcared within
a function, which makes it impossible to be named by user code.
This guarnatees that it will use the default auto-trait impl for Unpin -
that is, it will implement Unpin iff all of its fields implement Unpin.
This type can be safely declared as 'public', satisfiying the privacy
checker without actually allowing user code to access it.

This allows users to apply the #[pin_project] attribute to types
regardless of the privacy of the types of their fields.
@Aaron1011 Aaron1011 force-pushed the fix/private-in-public branch from 2974b16 to dfd7b91 Compare August 29, 2019 00:58
@Aaron1011
Copy link
Collaborator Author

Should we provide a way to opt-out of this? (the documents generated this way is probably not very clean)

I don't really see a use case for this. While the generated documentation certainly could be cleaner, I think it's always useful to know when a pin-projected type implements Unpin.

@Aaron1011
Copy link
Collaborator Author

Aaron1011 commented Aug 29, 2019

@taiki-e: I've added a comment explaining why the generated struct must be public, and squashed my commits.

@Aaron1011
Copy link
Collaborator Author

Aaron1011 commented Aug 29, 2019

@taiki-e: Note that once rust-lang/rust#48054 is implemented and stabilized, it will be possible to remove almost all of this code, and just generate normal bounds in the 'where' clause.

@taiki-e
Copy link
Owner

taiki-e commented Aug 29, 2019

@Aaron1011: That's great, I'll look forward to that!

@taiki-e
Copy link
Owner

taiki-e commented Aug 29, 2019

bors r+

bors bot added a commit that referenced this pull request Aug 29, 2019
53: Allow using #[pin_project]  type with private field types r=taiki-e a=Aaron1011

Previously, given code such as:

```rust

struct Private<T>;

#[pin_project]
pub struct Public<T> {
    #[pin] private: Private<T>
}
```

we would generate an Unpin impl like this:

```rust
impl Unpin for Public where Private: Unpin {}
```

Unfortunately, since Private is not a public type,
this would cause an E0446 ('private type `Private` in public interface)

When RFC 2145 is implemented (rust-lang/rust#48054),
this will become a lint, rather then a hard error.

In the time being, we need a solution that will work with the current
type privacy rules.

The solution is to generate code like this:

```rust

fn __private_scope() {
    pub struct __UnpinPublic<T> {
        __field0: Private<T>
    }
    impl<T> Unpin for Public<T> where __UnpinPublic<T>: Unpin {}
}
```

That is, we generate a new struct, containing all of the pinned
fields from our #[pin_project] type. This struct is delcared within
a function, which makes it impossible to be named by user code.
This guarnatees that it will use the default auto-trait impl for Unpin -
that is, it will implement Unpin iff all of its fields implement Unpin.
This type can be safely declared as 'public', satisfiying the privacy
checker without actually allowing user code to access it.

This allows users to apply the #[pin_project] attribute to types
regardless of the privacy of the types of their fields.

Co-authored-by: Aaron Hill <aa1ronham@gmail.com>
@bors
Copy link
Contributor

bors bot commented Aug 29, 2019

Build succeeded

  • taiki-e.pin-project

@bors bors bot merged commit dfd7b91 into taiki-e:master Aug 29, 2019
bors bot added a commit that referenced this pull request Aug 29, 2019
54: Detect if the proc_macro_def_site feature is not disallowed by -Zallow-features r=taiki-e a=taiki-e

Apart from what I mentioned in #53 (comment), there are cases where you want to completely avoid using the nightly function in the first place.

Refs: dtolnay/proc-macro2#175

Co-authored-by: Taiki Endo <te316e89@gmail.com>
This was referenced Sep 1, 2019
bors bot added a commit that referenced this pull request Sep 3, 2019
62: Improve document of UnpinStruct r=Aaron1011 a=taiki-e

Improve document and add a way to opt-out mentioned in #53 (comment).

The reason for needing support for ways other than `-Zallow-features` is that it also affects the use of other dependency unstable features.

Related:  #54 #56 (comment)

Generated document:

<img width="1001" alt="struct-1" src="https://user-images.githubusercontent.com/43724913/64136451-95494280-ce2c-11e9-81ce-6a02c315332f.png">
<img width="1015" alt="struct-2" src="https://user-images.githubusercontent.com/43724913/64136462-a6924f00-ce2c-11e9-9d1c-3cdca2d9b158.png">

Ways to opt-out of this:

https://github.com/taiki-e/pin-project/blob/5f7c6e35c2a9f3dfbc01a63e6206875d1bfb986b/pin-project-internal/build.rs#L21-L28

Co-authored-by: Taiki Endo <te316e89@gmail.com>
bors bot added a commit that referenced this pull request Sep 4, 2019
71: Do not display UnpinStruct in the document by default r=taiki-e a=taiki-e

There are some problems as mentioned in #70 

This causes the problem that by default the actual Unpin bounds cannot be known from the document if the original struct/enum itself is public (see #53 (comment) and rust-lang/rust#63281 for more).

This can be enabled using `--cfg pin_project_show_unpin_struct` in RUSTFLAGS.

```toml
# in Cargo.toml
[package.metadata.docs.rs]
rustdoc-args = ["--cfg", "pin_project_show_unpin_struct"]
```

cc @Aaron1011 @seanmonstar @LucioFranco

Co-authored-by: Taiki Endo <te316e89@gmail.com>
@taiki-e taiki-e added the A-pin-projection Area: #[pin_project] label Sep 24, 2019
@taiki-e taiki-e mentioned this pull request Sep 25, 2019
bors bot added a commit that referenced this pull request Sep 25, 2019
109: Release 0.4.0 r=taiki-e a=taiki-e

cc #21

### Changes since the latest 0.3 release:

* **Pin projection has become a safe operation.** In the absence of other unsafe code that you write, it is impossible to cause undefined behavior. (#18)

* `#[unsafe_project]` attribute has been replaced with `#[pin_project]` attribute. (#18, #33)

* The `Unpin` argument has been removed - an `Unpin` impl is now generated by default. (#18)

* Drop impls must be specified with `#[pinned_drop]` instead of via a normal `Drop` impl. (#18, #33, #86)

* `Unpin` impls must be specified with an impl of `UnsafeUnpin`, instead of implementing the normal `Unpin` trait. (#18)

* `#[pin_project]` attribute now determines the visibility of the projection type/method is based on the original type. (#96)

* `#[pin_project]` can now be used for public type with private field types. (#53)

* `#[pin_project]` can now interoperate with `#[cfg()]`. (#77)

* Added `project_ref` method to `#[pin_project]` types. (#93)

* Added `#[project_ref]` attribute. (#93)

* Removed "project_attr" feature and always enable `#[project]` attribute. (#94)

* `#[project]` attribute can now be used for `impl` blocks. (#46)

* `#[project]` attribute can now be used for `use` statements. (#85)

* `#[project]` attribute now supports `match` expressions at the position of the initializer expression of `let` expressions. (#51)

### Changes since the 0.4.0-beta.1 release:

* Fixed an issue that caused an error when using `#[pin_project(UnsafeUnpin)]` and not providing a manual `UnsafeUnpin` implementation on a type with no generics or lifetime. (#107)


Co-authored-by: Taiki Endo <te316e89@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-pin-projection Area: #[pin_project]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants