cargo fmt and cargo fmt --check don't agree on bindgen

[emilio]

Will put concrete STR in a sec.

[emilio]

STR: Check out rust-lang/rust-bindgen@dd21b21

$ rustup run nightly cargo --version                                                                                                                                                                                                       
cargo 1.39.0-nightly (9655d70af 2019-09-10)
$ rustup run nightly cargo fmt                                                                                                                                                                                                             
$ git diff # empty
$ rustup run nightly cargo fmt -- --check                                                                                                                                                                                                  
Diff in /home/emilio/src/moz/rust-bindgen/src/options.rs at line 1:
-use bindgen::{builder, Builder, CodegenConfig, EnumVariation, RustTarget, RUST_TARGET_STRINGS};
+use bindgen::{
+    builder, Builder, CodegenConfig, EnumVariation, RustTarget,
+    RUST_TARGET_STRINGS,
+};
 use clap::{App, Arg};
 use std::fs::File;
 use std::io::{self, stderr, Error, ErrorKind, Write};
Diff in /home/emilio/src/moz/rust-bindgen/src/options.rs at line 6:
 use std::str::FromStr;
 
 /// Construct a new [`Builder`](./struct.Builder.html) from command line flags.
-pub fn builder_from_flags<I>(args: I) -> Result<(Builder, Box<dyn io::Write>, bool), io::Error>
+pub fn builder_from_flags<I>(
+    args: I,
+) -> Result<(Builder, Box<dyn io::Write>, bool), io::Error>
 where

[calebcartwright]

I can reproduce this one as well, though I've no idea what the cause is 🤔

[calebcartwright]

When running with the list file names option -l, it looks like rustfmt is formatting options.rs twice, but with different config files causing the issue.

$ cargo +nightly fmt -- -l
rust-bindgen/src/options.rs
rust-bindgen/tests/../src/options.rs

There is a rustfmt.toml file in the root of the project directory:

max_width = 80
binop_separator = "back"

but also one in the tests directory:

normalize_doc_attributes = true

Since rustfmt will run for each target with cargo fmt, my guess is that it's doing one pass against options.rs with the root level config and reformatting it correctly, but it then is doing another pass against options.rs since it is included via in tests.rs, but with the tests config that doesn't have the same max_width value and basically reverting the change.

In the interim, I believe you can get this resolved by adding a #[rustfmt::skip] on the options mod import in tests.rs

https://github.com/rust-lang/rust-bindgen/blob/master/tests/tests.rs#L16

#[rustfmt::skip]
#[path = "../src/options.rs"]
mod options;

Alternatively, you could use the same max_width setting in the config in the tests directory too:

max_width = 80

[calebcartwright]

@topecongiro @scampi - is this indeed a bug in rustfmt? Personally I feel the issue is that the same file is being formatted twice, but with two different conflicting config files (one with max_width = 80 and the other with max_width = 100)

If it is a bug, is the expectation that rustfmt would somehow know while processing the options mod for the test target that it should use the other config file for just that one mod?

[emilio]

Yeah, that's a good point (sorry, I missed the replies before).

I think this can be closed as invalid if you want, though probably a warning or some sort of detection would be useful for this (probably lower priority? Idk).

[scampi]

I am not really familiar with that part of the code, but that said:

• Is it intentional that rustfmt picks up a new config file tests/rustfmt.toml ? Maybe this behavior is documented and I never paid attention to that.
• if that is intentional, maybe the logic should be that code in tests gets reformatted with a config that is a merge of rustfmt.toml at the root and tests/rustfmt.toml.

[topecongiro]

Sorry for the late reply. @calebcartwright @scampi This seems like a bug to me. IIRC, the rule to pick the correct rustfmt.toml should be something like the following:

1. If --config is passed, then always use it.
2. If not, then use the config file that is in the same directory as the input file.
3. If the config file is not present, then look for the parent directory recursively.
4. If there is no config file in any parents, then look for global config file

So with respect to this issue, the options.rs should be always formatted using the config file at the project's root directory.

Also we should not format the same file more than once. Looks like when a module has #[path = "..."] attribute the path to the module does not get canonicalized.

[calebcartwright]

@scampi -

Is it intentional that rustfmt picks up a new config file tests/rustfmt.toml ? Maybe this behavior is documented

I actually hadn't noticed it before either, but I think the docs here accurately reflect the rules @topecongiro enumerated

IIRC, the rule to pick the correct rustfmt.toml should be something like the following:

That makes sense. IMO, rustfmt is locating and using the correct config files. For the bin target (which includes options.rs getting formatted), rustfmt is correctly using the rustfmt file in the root of the project directory because the main input file is src/main.rs, and then when rustfmt is invoked for the test target (main input file being tests/tests.rs) it is correctly using the rustfmt file in the tests directory

Also we should not format the same file more than once. Looks like when a module has #[path = "..."] attribute the path to the module does not get canonicalized.

Sounds like that's the actual bug here 🐞 So IIUC, because src/options.rs is formatted with the invocation of rustfmt for the bin target, rustfmt should not attempt to format src/options.rs again when it's invoked for the tests target? However, because the paths are not canonicalized, rustfmt thinks it is seeing two different files and erroneously formatting twice?

rust-bindgen/src/options.rs
rust-bindgen/tests/../src/options.rs

@scampi - any chance your #3590 PR solves that (Asking based on the PR title)? 🤞

[scampi]

@calebcartwright that PR is to handle path attribute in windows. I doubt this would have an impact here ;o(