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 1.7.0-nightly inconsistent output across targets #5964

Closed
kyle-mccarthy opened this issue Nov 20, 2023 · 26 comments · Fixed by Sajjon/RadixWalletKit#20
Closed

rustfmt 1.7.0-nightly inconsistent output across targets #5964

kyle-mccarthy opened this issue Nov 20, 2023 · 26 comments · Fixed by Sajjon/RadixWalletKit#20

Comments

@kyle-mccarthy
Copy link

kyle-mccarthy commented Nov 20, 2023

Our CI started failing at the formatting check step after updating to 1.7.0-nightly (9a66e4471 2023-11-19); however, when run locally cargo +nightly fmt --check passes. I verified that they both are using the same version of rustfmt, the only differences being the platform/target.

CI: nightly-x86_64-unknown-linux-gnu installed - rustc 1.76.0-nightly (9a66e4471 2023-11-19)
Local: nightly-aarch64-apple-darwin - Up to date : 1.76.0-nightly (9a66e4471 2023-11-19)

Note that rustfmt 1.7.0-nightly (28317017 2023-11-17) produces the same output regardless of platform, so I think that the issue only exists in the most recent nightly (rustfmt 1.7.0-nightly (9a66e447 2023-11-19)) on the nightly-aarch64-apple-darwin target.

Example

CI Output (nightly-x86_64-unknown-linux-gnu)

use serde_json::json;

fn main() {
    let map = json!({ "key": "value" });

    let _value: String = map
        .get("key")
        .map(|v| v.as_str())
        .flatten()
        .unwrap_or_default()
        .to_string();
}

Local Output (nightly-aarch64-apple-darwin)

use serde_json::json;

fn main() {
    let map = json!({ "key": "value" });

    let _value: String =
        map.get("key")
            .map(|v| v.as_str())
            .flatten()
            .unwrap_or_default()
            .to_string();
}

Output Diff

diff --git a/src/main.rs b/src/main.rs
index 789b018..771d6ca 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -3,10 +3,10 @@ use serde_json::json;
 fn main() {
     let map = json!({ "key": "value" });

-    let _value: String =
-        map.get("key")
-            .map(|v| v.as_str())
-            .flatten()
-            .unwrap_or_default()
-            .to_string();
+    let _value: String = map
+        .get("key")
+        .map(|v| v.as_str())
+        .flatten()
+        .unwrap_or_default()
+        .to_string();
 }
@lukesneeringer
Copy link

As a note, the output on Darwin is alarmingly weird. The formatting differences are annoying enough, but it also eats comments after a single blank line, e.g.

// This is a comment.
//
// This is more information.

^-- The second and third line of the above get removed.

@ytmimi
Copy link
Contributor

ytmimi commented Nov 21, 2023

@kyle-mccarthy thanks for reaching out. I'm not quite sure what would have changed recently to cause the formatting diff. Our last subtree sync with the rust-lang/rust repo was about a month ago rust-lang/rust#117066 so we haven't released anything for a while.

Note that rustfmt 1.7.0-nightly (28317017 2023-11-17) produces the same output regardless of platform, so I think that the issue only exists in the most recent nightly (rustfmt 1.7.0-nightly (9a66e447 2023-11-19)).

Thanks for taking the time to do a little investigating on this.

I checked rustfmt 1.7.0-nightly (4cb3beec 2023-11-18) and rustfmt 1.7.0-nightly (9a66e447 2023-11-19) and here are the results:

rustfmt 1.7.0-nightly (4cb3beec 2023-11-18) does not produce a diff, and I can confirm that rustfmt 1.7.0-nightly (9a66e447 2023-11-19) does.

ytmimi ~ $: rustfmt +nightly-2023-11-19 --version
rustfmt 1.7.0-nightly (4cb3beec 2023-11-18)
ytmimi ~ $: rustfmt +nightly-2023-11-19 --check  
use serde_json::json;

fn main() {
    let map = json!({ "key": "value" });

    let _value: String = map
        .get("key")
        .map(|v| v.as_str())
        .flatten()
        .unwrap_or_default()
        .to_string();
}
ytmimi ~ $: rustfmt +nightly-2023-11-20 --version
rustfmt 1.7.0-nightly (9a66e447 2023-11-19)
ytmimi ~ $: rustfmt +nightly-2023-11-20 --check  
use serde_json::json;

fn main() {
    let map = json!({ "key": "value" });

    let _value: String = map
        .get("key")
        .map(|v| v.as_str())
        .flatten()
        .unwrap_or_default()
        .to_string();
}
Diff in <stdin> at line 3:
 fn main() {
     let map = json!({ "key": "value" });
 
-    let _value: String = map
-        .get("key")
-        .map(|v| v.as_str())
-        .flatten()
-        .unwrap_or_default()
-        .to_string();
+    let _value: String =
+        map.get("key")
+            .map(|v| v.as_str())
+            .flatten()
+            .unwrap_or_default()
+            .to_string();
 }

@calebcartwright do you think something weird could have happened with changes made to rustfmt directly from rust-lang/rust?

Edit: rustfmt 1.7.0-nightly (3a85a5cf 2023-11-20) also produces a diff

@ytmimi
Copy link
Contributor

ytmimi commented Nov 21, 2023

@lukesneeringer Are you sure that your issue is related? I just double check and I can't reproduce. It would probably be best for you to open up a separate issue fully explaining the issue that you're having as well as any configuration options that you're using.

ytmimi ~ $: rustfmt +nightly-2023-11-19 --check  
// This is a comment.
//
// This is more information.
ytmimi ~ $: rustfmt +nightly-2023-11-20 --check
// This is a comment.
//
// This is more information.

@lukesneeringer
Copy link

lukesneeringer commented Nov 21, 2023

I can do that. Although I'm fairly certain it's related, and this comment seems to suggest the same.

I'll open a separate issue in a little bit when I get a moment. :-)

P. S. I didn't know about the +nightly-{date} syntax, thanks for the tip!

@ytmimi
Copy link
Contributor

ytmimi commented Nov 21, 2023

@calebcartwright it Also doesn't look like there were any changes made to rustfmt in the rust-lang/rust repo within the last two weeks: https://github.com/rust-lang/rust/commits/master/src/tools/rustfmt

@ytmimi
Copy link
Contributor

ytmimi commented Nov 22, 2023

For what it's worth this doesn't seem to be an issue on the Rust Playground which as of today is using 1.7.0-nightly (2023-11-20 3a85a5c)

Screen Shot 2023-11-21 at 7 24 55 PM

Though when I ran with rustfmt 1.7.0-nightly (3a85a5cf 2023-11-20) locally with the nightly-2023-11-21-aarch64-apple-darwin toolchain I got a diff:

Diff in <stdin> at line 3:
 fn main() {
     let map = json!({ "key": "value" });
 
-    let _value: String = map
-        .get("key")
-        .map(|v| v.as_str())
-        .flatten()
-        .unwrap_or_default()
-        .to_string();
+    let _value: String =
+        map.get("key")
+            .map(|v| v.as_str())
+            .flatten()
+            .unwrap_or_default()
+            .to_string();
 }

@kyle-mccarthy
Copy link
Author

kyle-mccarthy commented Nov 22, 2023

For what it's worth this doesn't seem to be an issue on the Rust Playground which as of today is using 1.7.0-nightly (2023-11-20 3a85a5c)

The rust playground is actually how I produced the nightly-x86_64-unknown-linux-gnu output in my original bug report (it has the same output as our CI). Note that the playground is currently running linux and the architecture is x86_64.

The output that it is producing right now (2023-11-20 3a85a5c), is the same output that it was producing yesterday (9a66e4471 2023-11-19). I think that this is the correct/intended output.

The incorrect formatting seems to happen on nightly-aarch64-apple-darwin starting with 9a66e4471 2023-11-19.

I don't have access to test on other targets so I am not sure if the wrong output is limited to nightly-aarch64-apple-darwin or if it extends to others.

@xxchan
Copy link
Contributor

xxchan commented Nov 22, 2023

Similar issue here. Also aarch64-apple-darwin.

image

@xxchan
Copy link
Contributor

xxchan commented Nov 22, 2023

Is it possible that this is caused by sth like rustup or rust nightly release pipeline?

@lukesneeringer
Copy link

lukesneeringer commented Nov 22, 2023

@ytmimi I've come up with a fairly simple repro case. I'm posting it here rather than making it a new ticket because I've proven that it only occurs on aarch64-apple-darwin (not Linux). I'm happy to open a new issue if appropriate.

Here's my src/lib.rs file (this is documentation from one of my actual crates, but you can repro with literally just this):

//! Market calendar aware utilities
//!
//! This crate primarily provides the [`MarketCalendar`] trait, which adds methods for traversing
//! trading days, determining whether the markets are open on a given date, and so on. Currently
//! the trait is only applied to [`chrono::NaiveDate`], and when brought into scope, allows you to
//! ask about the market calendar for a given date, or traverse by trading day count.
//!
//! ```
//! use chrono::NaiveDate;
//! use market_cal::MarketCalendar;
//!
//! let dt = NaiveDate::from_ymd_opt(2020, 12, 24).unwrap();
//! assert_eq!(dt.next_trading_day(), NaiveDate::from_ymd_opt(2020, 12, 28).unwrap());
//! ```

Also the following rustfmt.toml file:

format_code_in_doc_comments = true
wrap_comments = true

Expected Behavior (Linux)

On Linux, using nightly-2023-11-21 I get this diff (the diffs are expected as I use other rustfmt options, which I removed to make the repro case simple):

 //! Market calendar aware utilities
 //!
-//! This crate primarily provides the [`MarketCalendar`] trait, which adds methods for traversing
-//! trading days, determining whether the markets are open on a given date, and so on. Currently
-//! the trait is only applied to [`chrono::NaiveDate`], and when brought into scope, allows you to
-//! ask about the market calendar for a given date, or traverse by trading day count.
+//! This crate primarily provides the [`MarketCalendar`] trait, which adds
+//! methods for traversing trading days, determining whether the markets are
+//! open on a given date, and so on. Currently the trait is only applied to
+//! [`chrono::NaiveDate`], and when brought into scope, allows you to
+//! ask about the market calendar for a given date, or traverse by trading day
+//! count.
 //!
 //! ```
 //! use chrono::NaiveDate;
Diff in /home/luke/Code/testme/src/lib.rs at line 10:
 //! use market_cal::MarketCalendar;
 //!
 //! let dt = NaiveDate::from_ymd_opt(2020, 12, 24).unwrap();
-//! assert_eq!(dt.next_trading_day(), NaiveDate::from_ymd_opt(2020, 12, 28).unwrap());
+//! assert_eq!(
+//!     dt.next_trading_day(),
+//!     NaiveDate::from_ymd_opt(2020, 12, 28).unwrap()
+//! );
 //!

Unexpected Behavior (Darwin)

On Mac/Darwin only, on nightly-2023-10-21 (and this started only on nightly-2023-10-19), I get this diff:

 //! Market calendar aware utilities
-//!
-//! This crate primarily provides the [`MarketCalendar`] trait, which adds methods for traversing
-//! trading days, determining whether the markets are open on a given date, and so on. Currently
-//! the trait is only applied to [`chrono::NaiveDate`], and when brought into scope, allows you to
-//! ask about the market calendar for a given date, or traverse by trading day count.
-//!
-//! ```
-//! use chrono::NaiveDate;
-//! use market_cal::MarketCalendar;
-//!
-//! let dt = NaiveDate::from_ymd_opt(2020, 12, 24).unwrap();
-//! assert_eq!(dt.next_trading_day(), NaiveDate::from_ymd_opt(2020, 12, 28).unwrap());
-//! ```

I'm happy to post another issue if you still believe it's unrelated, although since it popped up at the same time and appears to be isolated to Macs, I really do think it's a symptom of the same issue.

@ytmimi
Copy link
Contributor

ytmimi commented Nov 22, 2023

@lukesneeringer thanks for providing a small reproducible test case. I think a new issue would be overkill at this point. I couldn't tell you why these issues are related, but something definitely seems off on aarch64-apple-darwin and the issues start with the nightly-2023-11-20-aarch64-apple-darwin toolchain.

Confirmation Screenshot Screen Shot 2023-11-21 at 10 40 54 PM

@xxchan thank you for also providing some confirmation on this.

Is it possible that this is caused by sth like rustup or rust nightly release pipeline?

At this point I'm not too sure what the underlying issue is, but I'll be sure to report back if I figure anything out.

@ytmimi
Copy link
Contributor

ytmimi commented Nov 22, 2023

Also want to highlight that neither issue is reproducible when build rustfmt from source:

rustfmt 1.7.0-nightly (37489e4 2023-11-01)

Screenshot Screen Shot 2023-11-21 at 11 06 38 PM

@calebcartwright
Copy link
Member

@calebcartwright do you think something weird could have happened with changes made to rustfmt directly from rust-lang/rust?

@ytmimi no, though I think this has already been borne out in subsequent discussion, both because of the seeming platform dependency and the git history.


We aren't involved with the packaging and distribution aspects of rustfmt, as is the typical pattern for the dev tools that ship with the toolchains. There shouldn't be any saliant (see: behavioral, commit, etc.) differences between the rustfmt binaries per platform, so I'd guess we're looking at an upstream packaging issue, or somehow execution is occurring with a different config.

I'd like to start with the most absolute minimal repro possible, and then do a platform-specific bisect, essentially walking back through each nightly version on nightly-aarch64-apple-darwin, making sure the version of cargo-fmt & rustfmt is as expected for each toolchain, and then running in verbose mode for each tool chain so that the relevant config info is displayed.

If nothing jumps out from that, then next I think we'd want to take a look at the actual binary that rustup is grabbing from the upstream buckets so that we have more info when we talk to them and/or infra

@ehuss
Copy link
Contributor

ehuss commented Nov 22, 2023

FWIW, bisecting showed the change appeared with rust-lang/rust#117500.

@ehuss
Copy link
Contributor

ehuss commented Nov 22, 2023

CC @RalfJung, perhaps you have a theory why rust-lang/rust#117500 might have changed rustfmt to behave differently on aarch64? I tested aarch64-apple-darwin, aarch64-unknown-linux-gnu, and aarch64-pc-windows-msvc, and they all behave differently from x86_64 builds (the aarch64 hosts now have different formatting behavior).

@calebcartwright
Copy link
Member

I'll add that the diffs seem to be related to rustfmt now thinking the width/length isn't the same across the platforms

jqnatividad added a commit to jqnatividad/qsv that referenced this issue Nov 22, 2023
This reverts commit 4e590b2.

Reverting because of problem in rustfmt +nightly on aarch64 architectures

rust-lang/rustfmt#5964
@RalfJung
Copy link
Member

RalfJung commented Nov 22, 2023

Wow, that's wild. Could this be a symptom of rust-lang/rust#118124, where data gets corrupted on some intrinsic calls? Then rust-lang/rust#118127 would help. Otherwise there's some other use of the "Direct" ABI somewhere else [which is a bug but can sometimes produce the right results] that got uncovered by my PR, but I have no idea how to figure out where... (our current ABI code is super fragile, sadly)

@ehuss
Copy link
Contributor

ehuss commented Nov 22, 2023

I confirmed that rust-lang/rust#118127 fixed rustfmt for me on aarch64. I haven't tested it heavily, though.

@ytmimi
Copy link
Contributor

ytmimi commented Nov 22, 2023

@ehuss thanks for the confirmation. Would you mind quickly outlining what steps you took to verify that rust-lang/rust#118127 fixed rustfmt for you

@ytmimi
Copy link
Contributor

ytmimi commented Nov 22, 2023

What I did to verify that the changes in rust-lang/rust#118127 resolved the issue is that I checked out Ralph's branch, built rustfmt 1.7.0-dev (ebfb95a3571 2023-11-22), and then ran it against the snippets posted here and found that no diffs were produced.

@ehuss
Copy link
Contributor

ehuss commented Nov 22, 2023

Yea, that's what I did. Essentially:

  1. ./x.py build rusfmt
  2. Verified it formatted incorrectly.
  3. gh pr checkout 118127
  4. ./x.py build rustfmt
  5. Verified it formatted correctly.

@xfbs
Copy link

xfbs commented Nov 25, 2023

Hey just to chip in here, I ran into the same issue.

  • CI runs nightly Rust 1.74 with rustfmt 1.7.0-nightly (37b2813 2023-11-24) on x86_64-unknown-linux-gnu
  • Locally, I run nightly Rust 1.74 with rustfmt 1.7.0-nightly (37b2813a 2023-11-24) on aarch64-unknown-linux-gnu

I'm working on an M2 MacBook Air, but I'm coding inside a Debian VM (hence aarch64 but Linux).

Running cargo +nightly fmt, I get inconsistent output. It appears like rustfmt has a different idea of what the line width should be on my local machine. I thought I was going crazy, as I'm using the exact same build, until I realized that I am using it on a different platform, which lead to me finding this issue here.

@ytmimi
Copy link
Contributor

ytmimi commented Nov 26, 2023

I just checked and I can't reproduced the issue when using the nightly-2023-11-26-aarch64-apple-darwin toolchain. @xfbs when you have a moment could you confirm whether you're still seeing issues with nightly-2023-11-26.

@xfbs
Copy link

xfbs commented Nov 26, 2023

Can confirm this is resolved. I just updated with rustup to the latest nightly:

nightly-aarch64-unknown-linux-gnu updated - rustc 1.76.0-nightly (f5dc2653f 2023-11-25)

This is the rustfmt version I have now:

rustfmt 1.7.0-nightly (f5dc2653 2023-11-25)

And formatting my (correctly formatted) code from my repository no longer crates a diff.

@kaos
Copy link

kaos commented Nov 29, 2023

+1, seems to be resolved now (since ~3 days back).

@calebcartwright
Copy link
Member

Agreed this is sorted so going to close. Thanks for the reports, feedback, and assistance troubleshooting & resolving.

I am curious about the absence of preemptive test failures, but that's something for us to dig into separately moving forward

jsirois added a commit to jsirois/jump that referenced this issue Dec 9, 2023
These skips were added in a-scie#173 to work around this issue:
  rust-lang/rustfmt#5964

It has been resolved; so formatting should now work on all
architectures.

Fixes a-scie#174
jsirois added a commit to a-scie/jump that referenced this issue Dec 10, 2023
These skips were added in #173 to work around this issue:
  rust-lang/rustfmt#5964

It has been resolved; so formatting should now work on all
architectures.

Fixes #174
Sajjon added a commit to Sajjon/RadixWalletKit that referenced this issue Jan 21, 2024
…ference in rustfmt output between Mac Intel and Mac Silicon: rust-lang/rustfmt#5964
Vinnstah pushed a commit to Vinnstah/RadixWalletKit that referenced this issue Jan 21, 2024
…ference in rustfmt output between Mac Intel and Mac Silicon: rust-lang/rustfmt#5964
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants