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

cargo fmt makes correct code incorrect when using pub with auto #2880

Closed
droundy opened this issue Jul 31, 2018 · 6 comments
Closed

cargo fmt makes correct code incorrect when using pub with auto #2880

droundy opened this issue Jul 31, 2018 · 6 comments
Labels
bug Panic, non-idempotency, invalid code, etc. good first issue Issues up for grabs, also good candidates for new rustfmt contributors

Comments

@droundy
Copy link

droundy commented Jul 31, 2018

This bug can be seen when formatting the dimensioned crate. Here is a screenshot:

cargo build
   Compiling dimensioned v0.6.0 (file:///home/droundy/src/dimensioned)
    Finished dev [unoptimized + debuginfo] target(s) in 6.06s
bennet:dimensioned$ cargo fmt
bennet:dimensioned$ cargo fmt
error: expected one of `!` or `::`, found `pub`
   --> /home/droundy/src/dimensioned/src/traits.rs:123:6
    |
123 | auto pub trait NotDim {}
    |      ^^^ expected one of `!` or `::` here

bennet:dimensioned$ git diff /home/droundy/src/dimensioned/src/traits.rs
diff --git a/src/traits.rs b/src/traits.rs
index 1464479..d5e94bb 100644
--- a/src/traits.rs
+++ b/src/traits.rs
@@ -120,7 +120,7 @@ pub trait Map<ValueOut>: Dimensionless {
 
 #[cfg(feature = "oibit")]
 /// Everything that is not a quantity implements this trait
-pub auto trait NotDim {}
+auto pub trait NotDim {}
 
 macro_rules! impl_unary {
     ($Type:ty, $Trait:ident, $fun:ident) => {

It looks like all it's doing to break the code is swapping the order of auto and pub. I have no knowledge of what auto does, as this isn't my code. But clearly formatting the code should not make it fail to compile.

@nrc nrc added bug Panic, non-idempotency, invalid code, etc. good first issue Issues up for grabs, also good candidates for new rustfmt contributors labels Jul 31, 2018
@isaacazuelos
Copy link

I’d like to tackle this.

@nrc
Copy link
Member

nrc commented Jul 31, 2018

@isaacazuelos great, thanks! The best way to start is to look at items.rs. There you'll find functions for reformatting module-level items, including traits. If you look at how the 'signature' of a trait is formatted you should be able to see where the pub and auto come from and why the order is wrong. I think the fix should be easy. You'll also need to add a test, testing is described in Contributing.md. You'll need to find an appropriate source and target test file and add a trait like the one in the OP, then run cargo test to ensure it passes.

@nrc
Copy link
Member

nrc commented Jul 31, 2018

Let me know if anything is unclear

@isaacazuelos
Copy link

I'm not able to reproduce the issue. It looks like this is already tested too. I think this issue would happen at about items.rs:990.

Is it possible the issue is coming from an older version of rustfmt, prior to this commit?

@nrc
Copy link
Member

nrc commented Aug 1, 2018

Ah, that looks likely. Thanks for investigating @isaacazuelos

If you want another issue to look into, #2848 might be good if it still reproduces. If you're interested in performance, then I'm sure there are lots of possible optimisation wins possible and that would be awesome!

@max-sixty
Copy link
Contributor

I confirmed by running cargo fmt on https://github.com/paholg/dimensioned. A couple of changes, but no crash. Can this be closed?

@nrc nrc closed this as completed Aug 30, 2018
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. good first issue Issues up for grabs, also good candidates for new rustfmt contributors
Projects
None yet
Development

No branches or pull requests

4 participants