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

rustfmt can't understand configured module path attributes #2407

Closed
fitzgen opened this issue Feb 2, 2018 · 10 comments · Fixed by #3604
Closed

rustfmt can't understand configured module path attributes #2407

fitzgen opened this issue Feb 2, 2018 · 10 comments · Fixed by #3604
Labels
bug Panic, non-idempotency, invalid code, etc. p-high
Milestone

Comments

@fitzgen
Copy link
Member

fitzgen commented Feb 2, 2018

I have a module that has different implementations depending on what the target is. I use #[cfg_attr(..., path = "...")] to choose the correct implementation.

https://github.com/fitzgen/wee_alloc/blob/master/wee_alloc/src/lib.rs#L237-L246

#[cfg(all(not(unix), not(target_arch = "wasm32")))]
compile_error! {
    "There is no `wee_alloc` implementation for this target; want to send a pull request? :)"
}

#[cfg_attr(target_arch = "wasm32",
           path = "./imp_wasm32.rs")]
#[cfg_attr(all(unix, not(target_arch = "wasm32")),
           path = "./imp_unix.rs")]
mod imp;

rustfmt seems unable to handle this:

$ cargo fmt --all
error[E0583]: file not found for module `imp`
   --> /home/fitzgen/wee_alloc/wee_alloc/src/lib.rs:246:5
    |
246 | mod imp;
    |     ^^^
    |
    = help: name the file either imp.rs or imp/mod.rs inside the directory "/home/fitzgen/wee_alloc/wee_alloc/src"

I would expect that it would use the host target by default, or otherwise maybe hit the compile_error or something.

$ cargo fmt --version
0.3.6-nightly (e0e3e22 2018-01-18)

Full Steps to Reproduce

@fitzgen
Copy link
Member Author

fitzgen commented Feb 2, 2018

For anyone else who hits this issue, as a workaround, if I change my code to

#[cfg(all(not(unix), not(target_arch = "wasm32")))]
compile_error! {
    "There is no `wee_alloc` implementation for this target; want to send a pull request? :)"
}

#[cfg(target_arch = "wasm32")]
mod imp_wasm32;
#[cfg(target_arch = "wasm32")]
use imp_wasm32 as imp;

#[cfg(all(unix, not(target_arch = "wasm32")))]
mod imp_unix;
#[cfg(all(unix, not(target_arch = "wasm32")))]
use imp_unix as imp;

then cargo fmt --all works.

@topecongiro
Copy link
Contributor

cc #1208.

@topecongiro topecongiro added the bug Panic, non-idempotency, invalid code, etc. label Feb 3, 2018
@topecongiro
Copy link
Contributor

I think we need to mimic some setups of rustc (ones in librustc_driver). I am not sure how, though.

@phansch
Copy link
Member

phansch commented Feb 6, 2018

fwiw, clippy is also having issues with cfg_attr: rust-lang/rust-clippy#1522

I guess it's the same cause? Unfortunately I don't know enough about the compiler internals yet, to be sure.

@mjbshaw
Copy link

mjbshaw commented Mar 2, 2019

Edit: On reflection, I filed a separate issue for this (#3427) as it seems related but separate.

This is now an issue with Rust 2018's path clarity, where submodules no longer require mod.rs. Example:

$ mkdir foo
$ touch foo/bar.rs
$ echo 'mod bar;' > foo.rs
$ rustfmt --edition 2018 foo.rs
error[E0583]: file not found for module `bar`
 --> /private/tmp/demo/foo.rs:1:5
  |
1 | mod bar;
  |     ^^^
  |
  = help: name the file either bar.rs or bar/mod.rs inside the directory "/private/tmp/demo"

For clarity, the file structure is:

$ tree
.
├── foo
│   └── bar.rs
└── foo.rs

Here, foo.rs is the Rust 2018 equivalent of Rust 2015's foo/mod.rs.

ashleygwilliams added a commit to cloudflare/boringtun that referenced this issue Mar 22, 2019
this PR updates the way mods are imported in src/device/mod to
avoid a bug in cargo fmt that struggles with cfg_attr(path)
(see: rust-lang/rustfmt#2407).

once that bug was avoided, that allowed us to run cargo fmt.
this PR includes those formatting updates and adds a cargo
fmt check to CI.
ashleygwilliams pushed a commit to cloudflare/boringtun that referenced this issue Mar 22, 2019
this PR updates the way mods are imported in src/device/mod to
avoid a bug in cargo fmt that struggles with cfg_attr(path)
(see: rust-lang/rustfmt#2407).

once that bug was avoided, that allowed us to run cargo fmt.
this PR includes those formatting updates and adds a cargo
fmt check to CI.
christophermaier added a commit to habitat-sh/habitat that referenced this issue May 15, 2019
Looks like `path` attributes are invisible to rustfmt:

- rust-lang/rustfmt#1208
- rust-lang/rustfmt#2407

Ideally, it would be nice to not use the `path` attribute at all, and
we can actually formulate this in a way that doesn't use `path`, but
it seems to involve a lot more repetition. It's not at all clear that
that's a win here, particularly given that we're only going to have
two implementations for a short period of time.

Signed-off-by: Christopher Maier <[email protected]>
christophermaier added a commit to habitat-sh/habitat that referenced this issue May 17, 2019
Looks like `path` attributes are invisible to rustfmt:

- rust-lang/rustfmt#1208
- rust-lang/rustfmt#2407

Ideally, it would be nice to not use the `path` attribute at all, and
we can actually formulate this in a way that doesn't use `path`, but
it seems to involve a lot more repetition. It's not at all clear that
that's a win here, particularly given that we're only going to have
two implementations for a short period of time.

Signed-off-by: Christopher Maier <[email protected]>
@crlf0710
Copy link
Member

crlf0710 commented Jun 1, 2019

Just meet this on my code, my code is:

#[cfg_attr(feature = "winit", path = "ui_winit.rs")]
#[cfg_attr(not(feature = "winit"), path = "ui_apiw.rs")]
mod ui;

@carllerche
Copy link
Member

I have hit this issue as well here.

@topecongiro topecongiro added p-high and removed p-low labels Jun 6, 2019
@topecongiro
Copy link
Contributor

Note that we cannot completely support the cases like #2407 (comment), given the current design of rustfmt, whose main data structure is the AST from the rust compiler. Which files to be formatted will be feature/platform dependent.

@topecongiro
Copy link
Contributor

@carllerche With respect to your project (loom), are these two files ([1] [2]) part of your crate? IIUC they aren't referenced at all in the crate.

Asking because when testing with #3604 these two files weren't formatted when running cargo fmt. I would like to know if this is a bug or not.

[1] https://github.com/carllerche/loom/blob/981e6f60db2b71c515ab46c4c68c1d975220ce6e/src/rt/scheduler/fringe.rs
[2] https://github.com/carllerche/loom/blob/981e6f60db2b71c515ab46c4c68c1d975220ce6e/src/rt/scheduler/gen.rs

@carllerche
Copy link
Member

@topecongiro fringe is not currently (commented out), but gen is enabled via feature flag and cfg guard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Panic, non-idempotency, invalid code, etc. p-high
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants