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

Enforce alphabetical sorting with tidy #102719

Merged
merged 3 commits into from
Oct 12, 2022
Merged

Conversation

Noratrieb
Copy link
Member

@Noratrieb Noratrieb commented Oct 5, 2022

We have many places where things are supposed to be sorted alphabetically. For the smaller and more recent size assertions, this is mostly upheld, but in other more... alive places it's very messy.

This introduces a new tidy directive to check that a section of code is sorted alphabetically and fixes all places where sorting has gone wrong.

@rustbot
Copy link
Collaborator

rustbot commented Oct 5, 2022

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 5, 2022
@rust-highfive
Copy link
Collaborator

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 5, 2022
@rust-log-analyzer

This comment has been minimized.

@jackh726
Copy link
Member

jackh726 commented Oct 5, 2022

This is nice. My only thought here is that, because it's a very manual annotation, it will be forgotten. Is there any way we could maybe even get rid of the "end" annotation?

@saethlin
Copy link
Member

saethlin commented Oct 5, 2022

because it's a very manual annotation, it will be forgotten

I agree, but seeing how this has tidied up some sorting errors already, it's hard to argue this isn't a step forward.

Is there any way we could maybe even get rid of the "end" annotation?

Don't know if we want to get this advanced, but there are places in ui tests where we use regular expressions. The begin comment could contain a regular expression, then sort all nonempty lines that match that, perhaps sorting by a capture group. The downside would be that any change which causes one line to not match (like a comment in the middle of an argument list) would cause the elements below to not be sorted. So maybe we want a way to diagnose that... But now we're talking about tooling for the tools. Maybe a simple end delimiter is better.

@jackh726
Copy link
Member

jackh726 commented Oct 5, 2022

I mean, we could be a bit stricter and put all the items that we want to be in alphabetical order in their own block, then annotate that block.

@faptc
Copy link

faptc commented Oct 6, 2022

What about making rustfmt works with feature attributes?

@davidtwco
Copy link
Member

Could you add this annotatation here as well?

fluent_messages! {
ast_lowering => "../locales/en-US/ast_lowering.ftl",
ast_passes => "../locales/en-US/ast_passes.ftl",
attr => "../locales/en-US/attr.ftl",
borrowck => "../locales/en-US/borrowck.ftl",
builtin_macros => "../locales/en-US/builtin_macros.ftl",
compiletest => "../locales/en-US/compiletest.ftl",
const_eval => "../locales/en-US/const_eval.ftl",
codegen_gcc => "../locales/en-US/codegen_gcc.ftl",
driver => "../locales/en-US/driver.ftl",
expand => "../locales/en-US/expand.ftl",
hir_analysis => "../locales/en-US/hir_analysis.ftl",
infer => "../locales/en-US/infer.ftl",
interface => "../locales/en-US/interface.ftl",
lint => "../locales/en-US/lint.ftl",
metadata => "../locales/en-US/metadata.ftl",
middle => "../locales/en-US/middle.ftl",
mir_dataflow => "../locales/en-US/mir_dataflow.ftl",
monomorphize => "../locales/en-US/monomorphize.ftl",
parser => "../locales/en-US/parser.ftl",
passes => "../locales/en-US/passes.ftl",
plugin_impl => "../locales/en-US/plugin_impl.ftl",
privacy => "../locales/en-US/privacy.ftl",
query_system => "../locales/en-US/query_system.ftl",
save_analysis => "../locales/en-US/save_analysis.ftl",
session => "../locales/en-US/session.ftl",
symbol_mangling => "../locales/en-US/symbol_mangling.ftl",
trait_selection => "../locales/en-US/trait_selection.ftl",
ty_utils => "../locales/en-US/ty_utils.ftl",
}

@Noratrieb
Copy link
Member Author

Seeing hir_analysis being moved in a follow-up PR in this macro was actually what originally gave me the idea to do this, so it's funny that I forgot about it :D

@rustbot rustbot added the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Oct 6, 2022
@rustbot
Copy link
Collaborator

rustbot commented Oct 6, 2022

rustc_error_messages was changed

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

@Noratrieb
Copy link
Member Author

Noratrieb commented Oct 6, 2022

This is nice. My only thought here is that, because it's a very manual annotation, it will be forgotten. Is there any way we could maybe even get rid of the "end" annotation?

I think we could get rid of the end annotation with more heuristics, but I think it's quite complex already and I don't think the end annotation is particularly annoying, so I'd rather not do this.

Our best chance for not forgetting to add it to new places is to use it in enough places so that many people become aware of it, and then the assigned reviewer or people that randomly come across can remind the author of this. I'll also ping @nnethercote here since you've been adding a lot of size assertions which are particularly impacted by this.

@Noratrieb
Copy link
Member Author

What about making rustfmt works with feature attributes?
#[rustfmt::sorted] might be an interesting attribute, but adding that is gonna be harder and require more effort than this simple tidy check, which is quite nice already.

@nnethercote
Copy link
Contributor

This is great. A+, 10/10, would recommend. Can you use it for dependencies in Cargo.toml files? 😀

I agree the tidy-alphabetical-end is a little clumsy, but certainly not a showstopper. If you leave it off will tidy complain?

@Noratrieb
Copy link
Member Author

It will complain about lines after the section not being sorted (unless you do this at the end of the file, in which case it will probably work)

@nnethercote
Copy link
Contributor

Could it be made to complain if it reaches either the end of file or another start annotation, without hitting an intervening end annotation?

@Noratrieb
Copy link
Member Author

I can implement this tomorrow, yes.

I could also add it to all the Cargo.toml but I'd rather not do this right now. I think it should check those implicitly, which takes some more code, which I'd rather do in a follow up PR. The big diff in the unstable options is making me feel very uneasy and I'd like to see this merged rather soon :D

@Noratrieb
Copy link
Member Author

Oh, and I just noticed that I have a bad typo in the commit message, it should be "directive"

@Noratrieb
Copy link
Member Author

I implemented that error message and CI is green now

@bors
Copy link
Contributor

bors commented Oct 11, 2022

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

It can be used to ensure that a list of things is sorted alphabetically.
It goes off lines, but contains several heuristics to work with normal
Rust code (looking at indentation, ignoring comments and attributes).
@Noratrieb
Copy link
Member Author

Thank you @Dylan-DPC for doing the rebase as I requested, as I can't do it right now and would like to get this merged quickly

@jackh726
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 12, 2022

📌 Commit ce35609 has been approved by jackh726

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 12, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 12, 2022
Rollup of 7 pull requests

Successful merges:

 - rust-lang#102623 (translation: eager translation)
 - rust-lang#102719 (Enforce alphabetical sorting with tidy)
 - rust-lang#102830 (Unify `tcx.constness` query and param env constness checks)
 - rust-lang#102883 (Fix stabilization of `feature(half_open_range_patterns)`)
 - rust-lang#102927 (Fix `let` keyword removal suggestion in structs)
 - rust-lang#102936 (rustdoc: remove unused CSS `nav.sum`)
 - rust-lang#102940 (Update books)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 40deece into rust-lang:master Oct 12, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 12, 2022
@Noratrieb Noratrieb deleted the tidy-alphabetical branch October 13, 2022 05:29
@joshtriplett
Copy link
Member

Great to have this available and checked in CI!

I would love to see this available in rustfmt, as an attribute on a block or declaration. #[rustfmt::sort] could keep the block or declaration in sorted order.

Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
Rollup of 7 pull requests

Successful merges:

 - rust-lang#102623 (translation: eager translation)
 - rust-lang#102719 (Enforce alphabetical sorting with tidy)
 - rust-lang#102830 (Unify `tcx.constness` query and param env constness checks)
 - rust-lang#102883 (Fix stabilization of `feature(half_open_range_patterns)`)
 - rust-lang#102927 (Fix `let` keyword removal suggestion in structs)
 - rust-lang#102936 (rustdoc: remove unused CSS `nav.sum`)
 - rust-lang#102940 (Update books)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.