Add tidy-alphabetical-start/end around feature lists and, in some cases, allow/warn/deny lists #114766

clarfonthey · 2023-08-12T22:16:55Z

Problem

A lot of crates in the code include a large header with feature flags and lint overrides sorted in no particular order. The tidy-alphabetical-start and tidy-alphabetical-end flags for tidy were added to solve this problem, and they're used in some key places like the std crate lib.rs file. However, most of the crates in the code do not use this. Because sorting is not enforced, merge conflicts are common because every feature is usually just appended to the end of the list, and these changes often happen concurrently.

Solution

I used a command to narrow down the scope of my search to just lib.rs files without a tidy-alphabetical- comment anywhere in the file. (using fish, kakoune, and ripgrep)

$ kak (rg -F '#![feature' --files-with-matches --glob '**/lib.rs' | xargs rg -F 'tidy-alphabetical-' --files-without-match)

In these files, I added flags based upon the following heuristic:

tidy-alphabetical tags were added around feature lists more than one feature in size, and if there were any lint overrides, I often included those in this central list as well. This has the side-effect of splitting up lint overrides between feature flags if they include both allow or deny and warn attributes, so, I made two lists in those cases (and sometimes more).
cfg_attr-based feature flags were left out of this list, both because the sorting would be mostly meaningless for these (depending on the condition, not the feature) in addition to the fact that they're likely to have comments nearby explaining why the flags are conditional.
I shoved any interwoven other attributes to the beginning or end of the list in a mostly reasonable pattern.

Is this a perfect solution? No. However, it's something I could both apply in a reasonable amount of time, could be reviewed in an easy amount of time, and could be tweaked in the future without causing too much disruption. I suspect that this PR will create a large amount of merge conflicts immediately after it's merged, but it will help reduce merge conflicts going forward.

IMHO, any aesthetic tweaks can be added on a case-by-case basis, and doing so will most likely still limit the scope of merge conflicts since the contents of the list will be already sorted and thus easy to move around in a single unit. Additionally, anyone reviewing changes made to these files would help ensure that features and lints are grouped together anyway, limiting the chance of the problem reappearing in the future.

Prior Art

I did this large, sweeping change after I did a rather localised version in #113295 for libcore tests.

I have also undertaken multiple large refactoring projects unprompted in the past (see: #42523, #56932) that were mostly well-received, but I still feel the need to apologise for dropping such a large, sudden change on reviewers without much notice. I suggest using the GitHub UI to hide files that you've reviewed so you can make sure that you both actually look at all the changes, and don't miss anything.

Final notes

I am definitely willing to help out with merge conflicts until this is merged, but would strongly prefer it being merged sooner rather than later due to the extremely high chance of merge conflicts. My goal is not to create a perfect solution, as I stated, but to objectively and immediately improve the situation first before any aesthetic improvements.

If you have any particular files you feel strongly should be removed from the list of changes, or if you have any suggestions for additions, please feel free to mention them, but beyond simply removing files from this list, additional changes will likely be done in a separate PR.

rustbot · 2023-08-12T22:17:03Z

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-08-12T22:17:07Z

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Portable SIMD is developed in its own repository. If possible, consider making this change to rust-lang/portable-simd instead.

cc @calebzulawski, @programmerjake

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred in src/tools/rust-analyzer

cc @rust-lang/rust-analyzer

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @TaKO8Ki

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in GUI tests.

cc @GuillaumeGomez

…es, allow/warn/deny lists

bjorn3 · 2023-08-13T10:15:53Z

compiler/rustc_codegen_cranelift/src/lib.rs

 #![warn(unreachable_pub)]
+#![warn(unused_lifetimes)]
+// tidy-alphabetical-end


Could you please skip this for cg_clif? There is nothing in place in https://github.com/bjorn3/rustc_codegen_cranelift to enforce this and because of this it will only cause issues when syncing between that repo and this repo.

Same applies to rust-analyzer, I'd say this should not be done for subtrees in general.

bjorn3 · 2023-08-13T10:16:43Z

compiler/rustc_codegen_cranelift/src/lib.rs

 // Note: please avoid adding other feature gates where possible
+#![feature(rustc_private)]


The comment was intentionally before the feature gate as you did likely add new feature gates after the last one, which is exactly where the comment is positioned.

RalfJung · 2023-08-13T11:13:16Z

src/tools/miri/src/lib.rs

+#![warn(rust_2018_idioms)]
+// tidy-alphabetical-end
+// We are not implementing queries here so it's fine
+#![allow(rustc::potential_query_instability)]


I don't think it is a good idea to enforce this for code in subtrees. This code will be updated by PRs from Miri, where no tidy runs. Having tidy check this code will make syncing changes back to rustc harder.

src/tools/miri is already excluded from rustfmt checking in the rustc repo. Please also don't add other tidy checks here. (The same probably goes for clippy, but I am speaking as a miri maintainer here.)

Just want to add that I strongly agree with Ralf here that subtrees should be excluded, and I wouldn't want this enforced on the rustfmt source via tidy either

Yeah, this shouldn't be enforced in clippy either. I'm not opposed to enforcing this in specific, I think forcing them to be alphabetical is a fine idea, but it's that tidy is never ran outside of rustc and this shouldn't be enforced from the rustc side.

Yeah--I don't really have anything more to add, but we have no mechanism for enforcing this in std::simd and it will probably make syncing difficult.

klensy · 2023-08-13T12:31:20Z

Isn't it better to have rustfmt sort features? Doing that manually not fun.

RalfJung · 2023-08-13T12:41:51Z

./x.py test tidy --bless can do the sorting.

However the error should really say that, since I totally did not expect that either.

compiler-errors · 2023-08-13T22:50:22Z

tests/run-make/raw-dylib-cross-compilation/lib.rs

@@ -1,4 +1,7 @@
-#![feature(no_core, lang_items)]
+// tidy-alphabetical-start


I don't think we need to enforce this on a test file.

compiler-errors · 2023-08-13T22:51:30Z

tests/run-make/raw-dylib-cross-compilation/lib.rs

@@ -8,7 +11,7 @@
 trait Sized {}

 #[link(name = "extern_1", kind = "raw-dylib")]
-extern {
+extern "C" {


And the extraneous formatting-related changes should be removed from this and the other files below.

compiler-errors · 2023-08-13T22:52:44Z

library/profiler_builtins/src/lib.rs

+// tidy-alphabetical-start
 #![allow(unused_features)]
-#![cfg_attr(not(bootstrap), allow(internal_features))]
+#![feature(profiler_runtime)]
 #![feature(staged_api)]
+// tidy-alphabetical-end


It feels somewhat arbitrary what lists are mixing allow/deny + feature and what lists are sorting them separately.

compiler-errors · 2023-08-13T22:58:25Z

I think that this PR should be broken up between compiler / library / other tools. There's no reason to do it all at once imo, and it means that all these nits above wouldn't be blocking landing fully separable parts (e.g. changes to compiler/).

bors · 2023-08-13T23:06:36Z

☔ The latest upstream changes (presumably #114786) made this pull request unmergeable. Please resolve the merge conflicts.

workingjubilee · 2023-08-14T01:10:06Z

HELLO.

clarfonthey · 2023-08-14T05:29:10Z

Okay, so, after causing a giant mess and alerting everyone, I'm going to just close this for now.

I made sure I excluded submodules, but with the presence of subtrees in the code, it's very difficult to track what is "internal code" and what isn't. So, I'm going to try and take a stab at this later with a much smaller scope, and figure out how I can sift out subtrees too.

Apologies for the excess of notifications, which… in hindsight I should have expected that, but I somehow didn't.

rustbot assigned petrochenkov Aug 12, 2023

clarfonthey force-pushed the tidy-alphabetical branch from 10ccf6e to 7c1bec2 Compare August 12, 2023 22:18

This comment has been minimized.

Sign in to view

clarfonthey mentioned this pull request Aug 12, 2023

Implement Step for ascii::Char #113295

Merged

clarfonthey force-pushed the tidy-alphabetical branch from 7c1bec2 to 1130ab4 Compare August 12, 2023 22:23

clarfonthey marked this pull request as draft August 12, 2023 22:23

clarfonthey force-pushed the tidy-alphabetical branch from 1130ab4 to eaaa89b Compare August 12, 2023 22:25

This comment has been minimized.

Sign in to view

clarfonthey force-pushed the tidy-alphabetical branch from eaaa89b to 1a23bd0 Compare August 12, 2023 22:30

This comment has been minimized.

Sign in to view

Add tidy-alphabetical-start/end around feature lists and, in some cas…

2a752c2

…es, allow/warn/deny lists

clarfonthey force-pushed the tidy-alphabetical branch from 1a23bd0 to 2a752c2 Compare August 12, 2023 22:36

clarfonthey mentioned this pull request Aug 12, 2023

Lint suggestion: Suggest separating (or combining) attributes which accept a list of parameters rust-lang/rust-clippy#11331

Open

This comment has been minimized.

Sign in to view

clarfonthey marked this pull request as ready for review August 12, 2023 23:11

Unrelated rustdoc test failed, so, this appears to be the fix

873f23e

bjorn3 reviewed Aug 13, 2023

View reviewed changes

RalfJung reviewed Aug 13, 2023

View reviewed changes

compiler-errors reviewed Aug 13, 2023

View reviewed changes

clarfonthey closed this Aug 14, 2023

clarfonthey deleted the tidy-alphabetical branch September 16, 2023 05:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tidy-alphabetical-start/end around feature lists and, in some cases, allow/warn/deny lists #114766

Add tidy-alphabetical-start/end around feature lists and, in some cases, allow/warn/deny lists #114766

clarfonthey commented Aug 12, 2023 •

edited

Loading

rustbot commented Aug 12, 2023

rustbot commented Aug 12, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bjorn3 Aug 13, 2023

Veykril Aug 13, 2023

bjorn3 Aug 13, 2023

RalfJung Aug 13, 2023 •

edited

Loading

calebcartwright Aug 13, 2023

Centri3 Aug 13, 2023

calebzulawski Aug 14, 2023

klensy commented Aug 13, 2023

RalfJung commented Aug 13, 2023 •

edited

Loading

compiler-errors Aug 13, 2023

compiler-errors Aug 13, 2023

compiler-errors Aug 13, 2023 •

edited

Loading

compiler-errors commented Aug 13, 2023

bors commented Aug 13, 2023

workingjubilee commented Aug 14, 2023

clarfonthey commented Aug 14, 2023

		// Note: please avoid adding other feature gates where possible
		#![feature(rustc_private)]

		@@ -1,4 +1,7 @@
		#![feature(no_core, lang_items)]
		// tidy-alphabetical-start

Add tidy-alphabetical-start/end around feature lists and, in some cases, allow/warn/deny lists #114766

Add tidy-alphabetical-start/end around feature lists and, in some cases, allow/warn/deny lists #114766

Conversation

clarfonthey commented Aug 12, 2023 • edited Loading

Problem

Solution

Prior Art

Final notes

rustbot commented Aug 12, 2023

rustbot commented Aug 12, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Aug 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klensy commented Aug 13, 2023

RalfJung commented Aug 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors Aug 13, 2023 • edited Loading

Choose a reason for hiding this comment

compiler-errors commented Aug 13, 2023

bors commented Aug 13, 2023

workingjubilee commented Aug 14, 2023

clarfonthey commented Aug 14, 2023

clarfonthey commented Aug 12, 2023 •

edited

Loading

RalfJung Aug 13, 2023 •

edited

Loading

RalfJung commented Aug 13, 2023 •

edited

Loading

compiler-errors Aug 13, 2023 •

edited

Loading