Breaking changes in rustfmt with 1.44.0

[Elrendio]

Important

This is not a breaking change in the compiler. Sorry if it's not the place for such issue. However this has high chance to break a lot of continuous integrations and is highly linked to rust update.

Description

Default version for rustfmt in rust 1.44.0 is 1.4.14-stable (e417356 2020-04-21) when in 1.43.1 it was 1.4.12. rustfmt removed the support for #![cfg_attr(rustfmt, rustfmt_skip)] and #[cfg_attr(rustfmt, rustfmt_skip)] in version 1.4.13 (this may be a bug, I don't know). Which means lint that was passing in 1.43.1 might not in 1.44.0.

Other implications

Moreover I believe #![rustfmt:skip] doesn't work, you need to add #[rustfmt::skip] mod my_mod; which is quite problematic with generated code. For example I have a lot of crate using rust-protobuf to generate rust structs for grpc. Before 1.44.0 all the codegen was in files my_proto.rs and my_proto_grpc.rs.

Since the rust-protobuf crate was adding #![cfg_attr(rustfmt, rustfmt_skip)] at the beginning of each file you would just have to include them with mod my_proto.rs. And now you need to change to #[rustfmt:skip] mod my_protos.rs which is less ergonomic because I'd like to have all the dirtyness of generated code isolated in the generated files.

Suggestions

Lastly I coudln't find any notes on #[cfg_attr(rustfmt, rustfmt_skip)] vs #[rustfmt:skip] in recent release notes of either rust or rustfmt. I had to scroll down to the release note of rust 1.30.0 to learn about #[rustfmt:skip] and guess #[cfg_attr(rustfmt, rustfmt_skip)] might have been deprecated recently. I think it would be nice to mention it either in some rustfmt release note or rust 1.44.0 with an explanation on how to solve the problem.

Have a excellent day !

[Xanewok]

I'm not a part of rustfmt team, but maybe #![rustfmt::skip] (notice the bang) at the top of the generated file still works just as #![cfg_attr(rustfmt, rustfmt_skip)]?

[Elrendio]

Before mentioning #![rustfmt::skip] in Other Implications I tested it and it didn't work 😞 Have you been able to make it work ?

[Xanewok]

Argh, here I thought I processed everything from the OP. Sorry for the noise, can't help :(

Maybe @topecongiro or @calebcartwright may know more about breaking changes in rustfmt

[calebcartwright]

The cfg_attr approach for skipping has been deprecated for quite some time (rust-lang/rustfmt#3150), it was not a recent deprecation in rustfmt 1.4.13/14.

#![rustfmt::skip] at the top of a file should result in rustfmt ignoring the entirety of the file. If it's not doing so then perhaps this issue could be transferred or a new one opened in the rustfmt repo with the relevant details and reproduction steps

Example on the Rust Playground https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=e677e86d39d7843a9ad2f288365a54ea

Actually this is most likely the same bug as rust-lang/rustfmt#4206 which has been fixed in source but I believe exists in 1.4.14.

@topecongiro - thoughts on the best way to get the fix from rust-lang/rustfmt#4207 backported and available for rustfmt on stable?

[topecongiro]

Changes between 1.4.12 and 1.4.14 are quite minimal; mostly bumping the parser version, so the reported breaking change is most likely caused by the breaking changes from the parser.

Indeed, there was a breaking change of the parser we missed; the AST nodes for Item no longer contains inner attributes like #![cfg_attr(rustfmt, rustfmt_skip)]. I am not sure what caused this, likely #69838?

Fortunately, the issue is fixed on the latest master branch of rustfmt, because we now use Parser::parse_mod, which correctly parses and returns inner attributes of the module. We could backport this change to 1.4.14 or 1.4.15 if necessary.

I do not know how rustup handles backporting, so I would appreciate it if anyone familiar with the process could tell me what needs to be done.

[Elrendio]

Thanks @topecongiro and @calebcartwright for the explanations.

I'm quite surprised I didn't get any warnings from rustfmt or the compiler when using #![cfg_attr(rustfmt, rustfmt_skip)], was the warning only available with clippy ? If so is that the new strategy for rust warnings or just a small mistake ? I thought warnings were integrated in the compiler and that clippy lints were only optional suggestions.

Amazing news that this bug has already been fixed on master, @topecongiro do you know who might know how to backport the changes with rustup so that we can tag him on this issue ?

I still think that it's worth mentioning the breaking change with the temporary solution in rust 1.44.0 changelog as I think most user don't follow the releases of rustfmt and only update when updating rust. For these guys (which include myself) the deprecation of #![cfg_attr(rustfmt, rustfmt_skip)] only became effective when updating to rust 1.44.0. I have no idea what's the process to update it, but since it's a file in this repo I'm guessing a PR should do the trick. Do you me to submit one with the explanation and the temporary solution ? I could also update it when the correct fix has been backported to rust.

[Xanewok]

cc @Mark-Simulacrum on possible backporting rustfmt fix to stable, not sure if this is severe enough

[Mark-Simulacrum]

We're planning on a point release for a cargo bug anyway so we can definitely include a rustfmt backport. Can someone put up a branch on rustfmt which cherry-picks the relevant commits or so? Thanks!

[Elrendio]

I can try to do it, do you want it to start from version 1.14.0 (what's currently with rust 1.44.0) or from 1.15.0 (latest rustfmt release) ?

[Mark-Simulacrum]

Currently in stable for sure, we want to backport a minimal set of changes. It might be easier for rustfmt developers to judge what that is though (I myself am not familiar with rustfmt internals).

[Elrendio]

Alright, leaving it to a rustfmt team member 🙂

[calebcartwright]

@Mark-Simulacrum - do you mean a literal branch in the rustfmt repo, or a bump of the rustfmt submod somewhere in this repo?

If the former, do you or anyone else know if the rustc-ap-* crate versions used in rustfmt need to be in sync with what's in the rls dep tree like they do with the respective submodules? rustfmt 1.4.14 and the corresponding rls version in rust 1.44 are on v654 whereas rustfmt 1.4.15 is on v659.

[calebcartwright]

For these guys (which include myself) the deprecation of #![cfg_attr(rustfmt, rustfmt_skip)] only became effective when updating to rust 1.44.0.

@Elrendio - Just to clarify, the issue is that rustfmt v1.4.14 doesn't see any of those inner attributes on those submods due to a parsing bug, regardless of whether it's #![cfg_attr(rustfmt, rustfmt_skip)] or #![rustfmt::skip]. It doesn't really have anything to do with the former being deprecated.

Yes cfg_attr(rustfmt, rustfmt_skip) is deprecated (and has been for quite some time) and we encourage folks to use rustfmt::skip, but technically cfg_attr(rustfmt, rustfmt_skip will still work once the fix is backported.

[topecongiro]

@Mark-Simulacrum Thank you for your help. I have pushed commits to https://github.com/rust-lang/rustfmt/tree/rustfmt-1.4.16.

@Elrendio Thank you for quickly catching this and raising an alert.

[topecongiro]

@calebcartwright Ah, I somehow didn't see your comment when I posted my previous comment. In any case, that is a good point.

IIUC, the version of rustc-ap-* crates should match that of the 1.44.0 release, so rustfmt-1.4.16 uses 654.0.0. If the build process builds tools with the latest compiler, then rustfmt-1.4.17 can be used instead.

[Xanewok]

@Mark-Simulacrum Can this be closed now that 1.44.1 is released?

[Daniel-B-Smith]

I'm seeing the same behavior with 1.45.0. I didn't see anything in the release notes about removing support for #![cfg_attr(rustfmt, rustfmt_skip)] but I may have just missed it.