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

pin-project does not interoperate well with #[cfg()] #68

Closed
jonhoo opened this issue Sep 4, 2019 · 6 comments · Fixed by #77
Closed

pin-project does not interoperate well with #[cfg()] #68

jonhoo opened this issue Sep 4, 2019 · 6 comments · Fixed by #77
Assignees
Labels
A-pin-projection Area: #[pin_project] C-bug Category: related to a bug. C-enhancement Category: A new feature or an improvement for an existing one
Milestone

Comments

@jonhoo
Copy link

jonhoo commented Sep 4, 2019

Take this struct for instance:

#[pin_project]
#[derive(Debug)]
pub struct Socket {
    #[cfg_attr(unix, pin)]
    #[cfg(unix)]
    inner: tokio::net::unix::UnixStream,
    #[cfg_attr(windows, pin)]
    #[cfg(windows)]
    inner: tokio_named_pipes::NamedPipe,
}

I get the error

error[E0124]: field `inner` is already declared
  --> src/io/socket.rs:29:5
   |
26 |     inner: tokio::net::unix::UnixStream,
   |     ----------------------------------- `inner` first declared here
...
29 |     inner: tokio_named_pipes::NamedPipe,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ field already declared

Even though, as far as I can tell, this should work. My guess here is that pin-project is run before the #[cfg()]s are evaluated, and ends up not propagating them, but I'm not sure?

@jonhoo
Copy link
Author

jonhoo commented Sep 4, 2019

For now, I'm working around this by placing the #[cfg()] on the whole struct, but that won't easily work for structs with more fields that are shared.

@taiki-e
Copy link
Owner

taiki-e commented Sep 4, 2019

@jonhoo Thanks for the reporting!

I will investigate.

@taiki-e taiki-e self-assigned this Sep 4, 2019
@taiki-e
Copy link
Owner

taiki-e commented Sep 5, 2019

This is because I missed that the timing when the cfg attributes are applied differs between proc-macro-derive and proc-macro-attribute (rust-lang/rust#44528), but, I'm not sure if this issue can be fixed immediately.

  1. pin-project collects the type of field annotated with #[pin] attributes from the structure field and generates an Unpin implementation.
  2. Since cfg is not applied at that time, pin-project collects all #[pin] fields and adds them to where-clause of the Unpin implementation.
    For example, in the example you wrote, the generated Unpin implementation would be:
impl Unpin for Socket 
where 
    tokio::net::unix::UnixStream: Unpin,
    tokio_named_pipes::NamedPipe: Unpin,
{}

To fix this, we probably need to delegate the Unpin implementation to proc-macro-derive.
(Other issues are just extracting cfg from the field and applying it, so it shouldn't be too difficult to fix: abf3700)

So, for now, it is preferable to create a type alias / new type or create structs for each cfg.

#[cfg(unix)]
type Inner = tokio::net::unix::UnixStream;
// or `struct Inner(tokio::net::unix::UnixStream);`

#[cfg(windows)]
type Inner = tokio_named_pipes::NamedPipe;
// or `struct Inner(tokio_named_pipes::NamedPipe);`

#[pin_project]
#[derive(Debug)]
pub struct Socket {
    #[pin]
    inner: Inner,
}

@taiki-e
Copy link
Owner

taiki-e commented Sep 5, 2019

However, I would like to fix this by the release of 0.4 (the other remaining blocker at this time is #26).

@taiki-e
Copy link
Owner

taiki-e commented Sep 7, 2019

Filed #77

@bors bors bot closed this as completed in 3e82552 Sep 7, 2019
@taiki-e
Copy link
Owner

taiki-e commented Sep 7, 2019

Published 0.4.0-alpha.10 which fixes this issue.

By the way, if you use cfg on fields, it works for other attributes used on that field, so you can use #[pin] directly instead of #[cfg_attr(..., pin)].

#[pin_project]
#[derive(Debug)]
pub struct Socket {
    #[pin]
    #[cfg(unix)]
    inner: tokio::net::unix::UnixStream,
    #[pin]
    #[cfg(windows)]
    inner: tokio_named_pipes::NamedPipe,
}

jonhoo added a commit to jonhoo/mysql_async that referenced this issue Sep 8, 2019
blackbeam pushed a commit to blackbeam/mysql_async that referenced this issue Sep 20, 2019
* Add rustfmt.toml for 2018 edition fmt

* Part-way there

* Closer to upstream tokio

* No more MyFuture

* Port tests

* More stuff to async fn

* Use ? in tests over unwrap

* Workaround for rust-lang/rust#46415

* All tests pass

* async/await is only on nightly for now

* Only nightly on circle as well

* CI is hard

* Prep for async named pipes

* Don't fail tests if local infiles aren't supported

* No more workaround for taiki-e/pin-project#68

* Attempt at windows support

* PollEvented in tokio_net::util

* Avoid compilation error in Transaction::new

* Fix compilation error in tls::connect_async()

* Fix benches. Add SSL env var for tests.

* Test SSL during CI

* Bump dependencies
@taiki-e taiki-e added C-bug Category: related to a bug. C-enhancement Category: A new feature or an improvement for an existing one A-pin-projection Area: #[pin_project] labels Sep 23, 2019
bors bot added a commit that referenced this issue Oct 14, 2019
135: Move most of the processing to proc-macro-derive r=taiki-e a=taiki-e

To generate the correct `Unpin` implementation and the projection methods, we need to collect the types of the pinned fields.  However, since proc-macro-attribute is applied before `#[cfg]` and `#[cfg_attr]` on fields, we cannot be collecting field types properly at this timing. So instead of generating the `Unpin` implementation and the projection methods here, delegate their processing to proc-macro-derive.

#77 moved only the automatic generation of `Unpin` implementation to proc-macro-derive but found that it could not support `#[cfg_attr]`, so this moves more processing.

See also #68 (comment).

This also adds support for the use of `#[cfg]` on tuple structs and tuple variants.

Closes #123

Co-authored-by: Taiki Endo <te316e89@gmail.com>
diseraluca added a commit to diseraluca/uavcan that referenced this issue May 31, 2021
Recently, the getters for the priority field in
`uavcan::session_id::message::MessageSessionId` were disabled from being
generated to avoid an unused warning, as they are, indeed, unused in the build
codebase.

Nonetheless, the getter for `priority` is actually used by one of the tests, such
that is needed, but only when testing.

The general way to do this, would be to add a `#[cfg_attr(...)]` for the `skip`
attribute to avoid generating it during testing.

Unfortunately, `cfg*` attributes are not expanded before `proc_macro_attributes`
expansion and the `modular-bitfield` does not manage them manually.

This is a general problem in the proc-macro environment, with an example being
taiki-e/pin-project#68.

The rust team is working on a solution to this, see, for example:

https://doc.rust-lang.org/beta/unstable-book/library-features/cfg-eval.html
https://doc.rust-lang.org/beta/unstable-book/language-features/macro-attributes-in-derive-output.html

And the related issues.

In particular, `cfg_eval` provides a way to ensure that the `cfg` attributes are
evaluated before the expansion of the following macros.

For this reason, the `cfg_eval` was activated, a `#[cfg_eval]` attribute was
added to `MessageSessionId` and a `cfg_attr` conditioned on `test` was added to
the priority field of the same structure.

As this is not yet stabilized, the crate now depends on nightly, and cannot be
compiled in stable.

It was possible to provide workarounds to avoid moving outside stable.
For example, by duplicating `MessageSessionsId` and providing a version with the
`skip` attribute and one without, conditioning the whole structure, instead of a
field attribute, on `test`.

Nonetheless, being nightly only is not considered a problem for this crate
usage and the "forward-correct" solution was thus preferred.
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] C-bug Category: related to a bug. C-enhancement Category: A new feature or an improvement for an existing one
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants