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

Breaking changes in rustfmt with 1.44.0 #73078

Closed
Elrendio opened this issue Jun 6, 2020 · 17 comments
Closed

Breaking changes in rustfmt with 1.44.0 #73078

Elrendio opened this issue Jun 6, 2020 · 17 comments
Labels
A-attributes Area: #[attributes(..)]

Comments

@Elrendio
Copy link
Contributor

Elrendio commented Jun 6, 2020

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
Copy link
Member

Xanewok commented Jun 6, 2020

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
Copy link
Contributor Author

Elrendio commented Jun 6, 2020

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

@Xanewok
Copy link
Member

Xanewok commented Jun 6, 2020

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
Copy link
Member

calebcartwright commented Jun 7, 2020

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
Copy link
Contributor

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
Copy link
Contributor Author

Elrendio commented Jun 7, 2020

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
Copy link
Member

Xanewok commented Jun 7, 2020

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

@Mark-Simulacrum
Copy link
Member

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
Copy link
Contributor Author

Elrendio commented Jun 7, 2020

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
Copy link
Member

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
Copy link
Contributor Author

Elrendio commented Jun 7, 2020

Alright, leaving it to a rustfmt team member 🙂

@calebcartwright
Copy link
Member

@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
Copy link
Member

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
Copy link
Contributor

@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
Copy link
Contributor

@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.

@crlf0710 crlf0710 added the A-attributes Area: #[attributes(..)] label Jun 11, 2020
@Xanewok
Copy link
Member

Xanewok commented Jun 19, 2020

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

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Jul 7, 2020
Pkgsrc changes:
 * None.

Upstream changes:

Version 1.44.1 (2020-06-18)
===========================

* [rustfmt accepts rustfmt_skip in cfg_attr again.][73078]
* [Don't hash executable filenames on apple platforms, fixing backtraces.]
  [cargo/8329]
* [Fix crashes when finding backtrace on macOS.][71397]
* [Clippy applies lint levels into different files.][clippy/5356]

[71397]: rust-lang/rust#71397
[73078]: rust-lang/rust#73078
[cargo/8329]: rust-lang/cargo#8329
[clippy/5356]: rust-lang/rust-clippy#5356
@Daniel-B-Smith
Copy link
Contributor

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.

Daniel-B-Smith pushed a commit to Daniel-B-Smith/protoc-grpcio that referenced this issue Jul 17, 2020
The biggest thing I'm looking for in the newer versions is moving off of the deprecated `cfg_attr` to disable rustfmt which has broken multiple times recently (rust-lang/rust#73078, rust-lang/rust#74443).
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Aug 6, 2020
While here clean up all pkglint warnings.  Changes since 1.44.1:

Version 1.45.2 (2020-08-03)
==========================

* [Fix bindings in tuple struct patterns][74954]
* [Fix track_caller integration with trait objects][74784]

[74954]: rust-lang/rust#74954
[74784]: rust-lang/rust#74784

Version 1.45.1 (2020-07-30)
==========================

* [Fix const propagation with references.][73613]
* [rustfmt accepts rustfmt_skip in cfg_attr again.][73078]
* [Avoid spurious implicit region bound.][74509]
* [Install clippy on x.py install][74457]

[73613]: rust-lang/rust#73613
[73078]: rust-lang/rust#73078
[74509]: rust-lang/rust#74509
[74457]: rust-lang/rust#74457

Version 1.45.0 (2020-07-16)
==========================

Language
--------
- [Out of range float to int conversions using `as` has been defined as a saturating
  conversion.][71269] This was previously undefined behaviour, but you can use the
   `{f64, f32}::to_int_unchecked` methods to continue using the current behaviour, which
   may be desirable in rare performance sensitive situations.
- [`mem::Discriminant<T>` now uses `T`'s discriminant type instead of always
  using `u64`.][70705]
- [Function like procedural macros can now be used in expression, pattern, and  statement
  positions.][68717] This means you can now use a function-like procedural macro
  anywhere you can use a declarative (`macro_rules!`) macro.

Compiler
--------
- [You can now override individual target features through the `target-feature`
  flag.][72094] E.g. `-C target-feature= avx2 -C target-feature= fma` is now
  equivalent to `-C target-feature= avx2, fma`.
- [Added the `force-unwind-tables` flag.][69984] This option allows
  rustc to always generate unwind tables regardless of panic strategy.
- [Added the `embed-bitcode` flag.][71716] This codegen flag allows rustc
  to include LLVM bitcode into generated `rlib`s (this is on by default).
- [Added the `tiny` value to the `code-model` codegen flag.][72397]
- [Added tier 3 support\* for the `mipsel-sony-psp` target.][72062]
- [Added tier 3 support for the `thumbv7a-uwp-windows-msvc` target.][72133]

\* Refer to Rust's [platform support page][forge-platform-support] for more
information on Rust's tiered platform support.


Libraries
---------
- [`net::{SocketAddr, SocketAddrV4, SocketAddrV6}` now implements `PartialOrd`
  and `Ord`.][72239]
- [`proc_macro::TokenStream` now implements `Default`.][72234]
- [You can now use `char` with
  `ops::{Range, RangeFrom, RangeFull, RangeInclusive, RangeTo}` to iterate over
  a range of codepoints.][72413] E.g.
  you can now write the following;
  ```rust
  for ch in 'a'..='z' {
      print!("{}", ch);
  }
  println!();
  // Prints "abcdefghijklmnopqrstuvwxyz"
  ```
- [`OsString` now implements `FromStr`.][71662]
- [The `saturating_neg` method as been added to all signed integer primitive
  types, and the `saturating_abs` method has been added for all integer
  primitive types.][71886]
- [`Arc<T>`, `Rc<T>` now implement  `From<Cow<'_, T>>`, and `Box` now
  implements `From<Cow>` when `T` is `[T: Copy]`, `str`, `CStr`, `OsStr`,
  or `Path`.][71447]
- [`Box<[T]>` now implements `From<[T; N]>`.][71095]
- [`BitOr` and `BitOrAssign` are implemented for all `NonZero`
  integer types.][69813]
- [The `fetch_min`, and `fetch_max` methods have been added to all atomic
  integer types.][72324]
- [The `fetch_update` method has been added to all atomic integer types.][71843]

Stabilized APIs
---------------
- [`Arc::as_ptr`]
- [`BTreeMap::remove_entry`]
- [`Rc::as_ptr`]
- [`rc::Weak::as_ptr`]
- [`rc::Weak::from_raw`]
- [`rc::Weak::into_raw`]
- [`str::strip_prefix`]
- [`str::strip_suffix`]
- [`sync::Weak::as_ptr`]
- [`sync::Weak::from_raw`]
- [`sync::Weak::into_raw`]
- [`char::UNICODE_VERSION`]
- [`Span::resolved_at`]
- [`Span::located_at`]
- [`Span::mixed_site`]
- [`unix::process::CommandExt::arg0`]

Cargo
-----

Misc
----
- [Rustdoc now supports strikethrough text in Markdown.][71928] E.g.
  `~~outdated information~~` becomes "~~outdated information~~".
- [Added an emoji to Rustdoc's deprecated API message.][72014]

Compatibility Notes
-------------------
- [Trying to self initialize a static value (that is creating a value using
  itself) is unsound and now causes a compile error.][71140]
- [`{f32, f64}::powi` now returns a slightly different value on Windows.][73420]
  This is due to changes in LLVM's intrinsics which `{f32, f64}::powi` uses.
- [Rustdoc's CLI's extra error exit codes have been removed.][71900] These were
  previously undocumented and not intended for public use. Rustdoc still provides
  a non-zero exit code on errors.

Internals Only
--------------
- [Make clippy a git subtree instead of a git submodule][70655]
- [Unify the undo log of all snapshot types][69464]

[73420]: rust-lang/rust#73420
[72324]: rust-lang/rust#72324
[71843]: rust-lang/rust#71843
[71886]: rust-lang/rust#71886
[72234]: rust-lang/rust#72234
[72239]: rust-lang/rust#72239
[72397]: rust-lang/rust#72397
[72413]: rust-lang/rust#72413
[72014]: rust-lang/rust#72014
[72062]: rust-lang/rust#72062
[72094]: rust-lang/rust#72094
[72133]: rust-lang/rust#72133
[71900]: rust-lang/rust#71900
[71928]: rust-lang/rust#71928
[71662]: rust-lang/rust#71662
[71716]: rust-lang/rust#71716
[71447]: rust-lang/rust#71447
[71269]: rust-lang/rust#71269
[71095]: rust-lang/rust#71095
[71140]: rust-lang/rust#71140
[70655]: rust-lang/rust#70655
[70705]: rust-lang/rust#70705
[69984]: rust-lang/rust#69984
[69813]: rust-lang/rust#69813
[69464]: rust-lang/rust#69464
[68717]: rust-lang/rust#68717
[`Arc::as_ptr`]: https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#method.as_ptr
[`BTreeMap::remove_entry`]: https://doc.rust-lang.org/stable/std/collections/struct.BTreeMap.html#method.remove_entry
[`Rc::as_ptr`]: https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#method.as_ptr
[`rc::Weak::as_ptr`]: https://doc.rust-lang.org/stable/std/rc/struct.Weak.html#method.as_ptr
[`rc::Weak::from_raw`]: https://doc.rust-lang.org/stable/std/rc/struct.Weak.html#method.from_raw
[`rc::Weak::into_raw`]: https://doc.rust-lang.org/stable/std/rc/struct.Weak.html#method.into_raw
[`sync::Weak::as_ptr`]: https://doc.rust-lang.org/stable/std/sync/struct.Weak.html#method.as_ptr
[`sync::Weak::from_raw`]: https://doc.rust-lang.org/stable/std/sync/struct.Weak.html#method.from_raw
[`sync::Weak::into_raw`]: https://doc.rust-lang.org/stable/std/sync/struct.Weak.html#method.into_raw
[`str::strip_prefix`]: https://doc.rust-lang.org/stable/std/primitive.str.html#method.strip_prefix
[`str::strip_suffix`]: https://doc.rust-lang.org/stable/std/primitive.str.html#method.strip_suffix
[`char::UNICODE_VERSION`]: https://doc.rust-lang.org/stable/std/char/constant.UNICODE_VERSION.html
[`Span::resolved_at`]: https://doc.rust-lang.org/stable/proc_macro/struct.Span.html#method.resolved_at
[`Span::located_at`]: https://doc.rust-lang.org/stable/proc_macro/struct.Span.html#method.located_at
[`Span::mixed_site`]: https://doc.rust-lang.org/stable/proc_macro/struct.Span.html#method.mixed_site
[`unix::process::CommandExt::arg0`]: https://doc.rust-lang.org/std/os/unix/process/trait.CommandExt.html#tymethod.arg0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: #[attributes(..)]
Projects
None yet
Development

No branches or pull requests

7 participants