"cfg-if"-included modules ignored if formatted after parsing a top-level macro in another file

[smklein]

Describe the bug

Rustfmt generally tries to follow cfg-if-conditional modules, which is great -- this allows parsing modules which are e.g. arch/OS specific. However, this logic operates incorrectly when parsed on the Nth file, for N > 1 where some Mth file (M < N) contains a top-level macro.

This is a bit quirky, and I'm not sure I'm using the best terminology, so I'll use an example:

To Reproduce

format_me_please.rs

// Any invalid formatting will do
pub fn hello   (    ) {                            }

lib.rs:

cfg_if! {
  if #[cfg(target_family = "unix")] {
    mod format_me_please;
  }
}

lib2.rs:

its_a_me_a_macro! {
  // Any contents; this comment will suffice
}

With this setup, the bug can be reproduced on command-line using rustfmt, though it is also possible to repro via cargo fmt:

// This works as expected; lib.rs + format_me_please.rs are formatted.
// The "-v" lets us see that the `format_me_please.rs` file is detected + fixed.
$ rustfmt -v lib.rs

// BUG: This case is unexpected. Basically, lib2.rs and lib.rs are formatted,
// but the `format_me_please.rs` file is not even considered.
$ rustfmt -v lib2.rs lib.rs

Meta

• rustfmt version: 1.33
• From where did you install rustfmt?: Rustup, but also compiled from source.

[smklein]

I actually have a patch for this bug, but I wanted to get an issue number first so I could reference it in my PR. Will upload shortly!

TL;DR: Interned symbols have different indices when crossing files; the check for cfg_if is currently a lazily-initialized global variable, but when it becomes a local, this issue is resolved.

[smklein]

Also, I think this is related to #3253

[smklein]

Looking at rustfmt 2.0, it seems this issue was resolved by 3b7fd26 , but this PR is still relevant; as this bug currently exists in the 1.x branch

[calebcartwright]

Have pulled this into the 1.4.33 release. Would you have any interest in adding your test case against the master branch? If not we can go ahead and close this

[ytmimi]

@calebcartwright as you mentioned you included this in the 1.4.33 release so I'm assuming this can be closed now

[calebcartwright]

@calebcartwright as you mentioned you included this in the 1.4.33 release so I'm assuming this can be closed now

I still think a test case would be worthwhile, but sure; test case can be added independently of the open/closed status of this issue.

[ytmimi]

@calebcartwright I think we've already got issue 4656 tests in the code base. Was there another test you had in mind? I can work to get a PR ready, I'm just not sure which test you had in mind.

[calebcartwright]

Nope that works! Just bad memory on my part then 😅