Poor formatting of expressions with dots / extra spaces

[chifflier]

Hi,

After upgrading rustfmt stable (using rustup), rusftmt add unexpected spaces in expressions containing many dots.
This is a problem in der-parser and oid-registry, since the notation of a OID is a path with digits and dots.
These spaces seem to be added always after the third group (see examples).

Input

let oid = oid!(1.2.840.113549.1.1.5);
let oid2 = oid!(2.5.4.3);

Output

let oid = oid!(1.2.840 .113549 .1 .1 .5);
let oid2 = oid!(2.5.4 .3);

Expected output

let oid = oid!(1.2.840.113549.1.1.5);
let oid2 = oid!(2.5.4.3);

Meta

• rustfmt version: rustfmt 1.4.24-stable (eb894d5 2020-11-05)
• From where did you install rustfmt?: rustup

[calebcartwright]

Thanks for reaching out @chifflier, what version of rustc were you using before your update, and what version have you updated to? Guessing 1.46.0 (or earlier) and 1.48.0 respectively

What I think you'll find is that prior to your update, rustfmt was actually failing to format those macro calls entirely and simply leaving your original code in place. Due to the incorporation of some upstream rustc changes, rustfmt is now able to work with and format this, and the formatting you are seeing is the result of some legacy formatting styles for nested tuple access.

I've included some additional context/background for you below, followed by some available options you can take

Background

To demonstrate what has actually been happening with the code you provided, try running rustfmt from stable 1.46.0 using this intentionally misformatted version of your code:

fn foo() {
    let oid = oid!(
                1.2.840.113549.1.1.5       );
let oid2 = oid!(2.      5                      .4                 .     3         );
}

$ rustc -V
rustc 1.46.0 (04488afe3 2020-08-24)

$ cat foo.rs
fn foo() {
    let oid = oid!(
                1.2.840.113549.1.1.5       );
let oid2 = oid!(2.      5                      .4                 .     3         );
}

$ rustfmt foo.rs --emit stdout
/........./foo.rs:

fn foo() {
    let oid = oid!(
                1.2.840.113549.1.1.5       );
    let oid2 = oid!(2.      5                      .4                 .     3         );
}

From 1.46.0 to 1.47.0 there were some notable upstream changes in rustc to support nested tuple access which actually caused issues for us in rustfmt due to a rustc parser bug in the associated spans

$ rustc -V
rustc 1.47.0 (18bf6b4f0 2020-10-07)

$ cat foo.rs
fn foo() {
    let oid = oid!(
                1.2.840.113549.1.1.5       );
let oid2 = oid!(2.      5                      .4                 .     3         );
}

$ rustfmt foo.rs --emit stdout
thread 'main' panicked at 'bad span: `.`: ``', src/tools/rustfmt/src/source_map.rs:52:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'bad span: `.`: ``', src/tools/rustfmt/src/source_map.rs:52:13
/........./foo.rs:

fn foo() {
    let oid = oid!(
                1.2.840.113549.1.1.5       );
    let oid2 = oid!(2.      5                      .4                 .     3         );
}

What you're now seeing in the version of rustfmt that ships with 1.48.0 is that the rustc parser bug has been fixed, and rustfmt is now, finally able to apply formatting rules. For legacy/historical reasons (which I described in more detail in #4355 (comment)), rustfmt has inserted the extra space because until the recent upstream changes, not doing so would've resulted in the formatted code causing compilation issues in many cases.

Due to rustfmt's stability guarantee, we cannot change that default behavior as it would cause issues for folks that formatted their code (successfully) with the older versions of rustfmt

$ rustc -V
rustc 1.46.0 (04488afe3 2020-08-24)
$ echo "fn foo() { oid!(  2  .5  .4); }" | rustfmt 
fn foo() {
    oid!(2.5 .4);
}

Options

If the default behavior of those extra spaces is problematic for you then you can either:

• add version="Two" to your rustfmt config file (or use the cli override --config version=Two)
• leverage the skip attributes to have rustfmt leave them alone, either holistically via #[rustfmt::skip::macros(oid)] or individually via #[rustfmt::skip]

$ rustc -V
rustc 1.48.0 (7eac88abb 2020-11-16)
$ cat bar.rs
fn bar() {
let oid = oid!( 1 . 2   .840 .113549 .1 .1 .5);
let oid2 = oid!(  2.5.4 .3);
}
$ rustfmt bar.rs --emit stdout --config version=Two
/........./bar.rs:

fn bar() {
    let oid = oid!(1.2.840.113549.1.1.5);
    let oid2 = oid!(2.5.4.3);
}

[chifflier]

Hi @calebcartwright ,
Thanks for the reply and the explanations!
I confirm your thoughts on rustc versions: it "worked" (left unchanged content) using 1.46, raised errors using 1.47, and current version (from my issue) is 1.48.

I am testing the solutions you proposed

• I tested the skip attribute to the entire crate. Unfortunately, the skip macro cannot be applied as an inner attribute (error[E0658]: non-builtin inner attributes are unstable)
• The version="Two" config value seems to work, with very little side effects (other lines changed). I'm curious why this affects the formatting?

[chifflier]

Additional question: is version=Two considered stable, and usable in CI? The option is accepted when added to the command line of cargo fmt, but not if I add it to .rustfmt.yml:

Warning: can't set `version = Two`, unstable features are only available in nightly channel.`

Both are using the same version:

cargo fmt -- --version
rustfmt 1.4.24-stable (eb894d5 2020-11-05)
rustfmt --version
rustfmt 1.4.24-stable (eb894d5 2020-11-05)

[calebcartwright]

Unfortunately, the skip macro cannot be applied as an inner attribute (error[E0658]: non-builtin inner attributes are unstable)

Hmm, I thought tool attributes were fine as inner attributes. Are you sure you're using 1.48.0? https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=823b9832c37480c3edce895afd2b900a

I'm curious why this affects the formatting?

Docs on the config option can be found here (including the in-source link since the anchor for version seems to be broken on the site)
https://github.com/rust-lang/rustfmt/blob/rustfmt-1.4.28/Configurations.md#version
https://rust-lang.github.io/rustfmt/?version=v1.4.27&search=#version

The stability guarantee that was put in place as part of the initial launch of rustfmt places some pretty stringent constraints that make it tricky to change anything about the formatted code rustfmt emits. There's a constraint that, except in cases of genuine bug fixes within rustfmt (like this one), new syntax, etc., that newer versions of rustfmt can't change the formatting of code formatted with previous versions of rustfmt (i.e. a rustup update ... && cargo fmt -- --check can never fail).

So even in cases like nested tuples where syntax/parser changes enable us to provide objectively better formatting, we can't make that new/better formatting the default due to that constraint, and have to gate the default formatting changes behind a feature-flag of sorts, which is the version config option.

Folks that opt in to this version = "Two" are agreeing to get the newer/improved formatting but also accepting that when they update to a newer version of rustfmt there is a chance it could make some (typically minor) formatting changes.

Additional question: is version=Two considered stable, and usable in CI?

No that option is still unstable. I should've remembered that given you shared you're working stable, sorry about that.

[chifflier]

Edit: fix comments/tests results

Unfortunately, the skip macro cannot be applied as an inner attribute (error[E0658]: non-builtin inner attributes are unstable)

Hmm, I thought tool attributes were fine as inner attributes. Are you sure you're using 1.48.0? https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=823b9832c37480c3edce895afd2b900a

I was trying to apply the attribute to the entire crate using an inner attribute (#![rustfmt::skip::macros(oid)], note the !) to src/lib.rs, which gave the error message.

It seems the skip attribute cannot be applied globally to a file or crate (unless I missed something?).

After more tests, I think the stable solution is to add the skip attribute to every function calls to oid. This solution is fine, I'll use that until version 2 becomes stable.

[...]

Additional question: is version=Two considered stable, and usable in CI?

No that option is still unstable. I should've remembered that given you shared you're working stable, sorry about that.

Thanks for all the details and explanations!

[calebcartwright]

I was trying to apply the attribute to the entire crate using an inner attribute (#![rustfmt::skip::macros(oid)], note the !) to src/lib.rs, which gave the error message.

Nuts, sorry about that, typo 😄

After more tests, I think the stable solution is to add the skip attribute to every function calls to oid. This solution is fine, I'll use that until version 2 becomes stable.

Going to go ahead and close this one as there's no real action for us to take, but thanks again for reaching out!