Readability improvements to rust code in this repo

[umbrageodotus]

Please double check everything as I might have done something wrong. Please draft as this is not complete. Satisfies clippy a lot more (from ~400 warnings to ~150)
(also haha back to back PRs go BRRRRRRRRRRRRR :P)

[rustbot]

Some changes occurred in src/tools/opt-dist

cc @Kobzol

tidy extra checks were modified.

cc @lolbinarycat

The run-make-support library was changed

cc @jieyouxu

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

Some changes occurred in src/tools/compiletest

cc @jieyouxu

[rustbot]

r? @Zalathar

rustbot has assigned @Zalathar.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[umbrageodotus]

Also, please check if I broke MSRV.

[Noratrieb]

Please don't make such large PRs that touch everything, it's quite hard to review. We generally welcome cleanups to different parts of the tree (though avoid touching subtrees (rust-analzer, clippy, rustfmt, Miri)), but you always have to consider the review effort of these changes. Split them up, don't open it all at once, and we're happy :).

[umbrageodotus]

Please don't make such large PRs that touch everything, it's quite hard to review. We generally welcome cleanups to different parts of the tree (though avoid touching subtrees (rust-analzer, clippy, rustfmt, Miri)), but you always have to consider the review effort of these changes. Split them up, don't open it all at once, and we're happy :).

Hey! Sorry, I don't know how to split it up anymore... is this one semi-fine or would I need to redo it all?

[umbrageodotus]

Do not review the PR yet. I'm planning to add much more.

[kpreid]

Sorry, I don't know how to split it up anymore...

Here is one approach to splitting into many commits (there are other ways, but this one will suit "break up one large commit").

1. Make a new branch from the parent of your PR branch:

   git branch smaller-change $(git merge-base master readability-perf)
   git checkout smaller-change

2. Copy the contents of individual files/directories from the old branch: git checkout readability-perf compiler/rustc-arena/

   Or to interactively select individual changes from a directory or file, use the same git checkout command but with the -p option.

3. Commit those copied changes: git commit -av

4. To create more commits on the same branch, if desired, go to step 2.

5. To create more branches for separate PRs, go to step 1.

[Noratrieb]

Hey! Sorry, I don't know how to split it up anymore... is this one semi-fine or would I need to redo it all?

I'd say that this one is already too big

[jieyouxu]

I'll add some context on why this PR is difficult to review, and why it basically has no chance to merge as is, let alone with even more changes. Hopefully, this context can help you to split the set of changes into reviewable and thus mergeable chunks, before you expend even more effort on creating an even larger set of changes in the same PR (that also has no chance to merge as is).

What qualities of this PR makes it difficult to review?

Firstly,

Please double check everything as I might have done something wrong. Please draft as this is not complete. Satisfies clippy a lot more (from ~400 warnings to ~150)

Does not inspire confidence for two reasons:

1. Firstly, the "direction". It is a non-goal for all code in this repository to pass all clippy lints.¹ Certain tools or helper crates might be okay with trying to pass under clippy::all at all times, but this is definitely not true for the compiler/ tree or the library/ tree. For more elaboration on this, refer to discussions in [1], [2] and specifically [3]. See also the Design principle: Satisfying all clippy lints are a non-goal section below.
2. This suggests that you have not reviewed the changes carefully yourself. I apologize if you did, but that's what the description suggests to me. If this is the case, then it means the reviewer has to do an unusually large amount of work to heavily scrutinize all your changes. This is a lot of work for the reviewer, and the most likely outcome is that the reviewer will find the effort-required-vs-utility-anticipated tradeoff scale leans heavily towards the "effort required fails to match utility anticipated" side, and refuse to review your PR.

Secondly, this PR is already too large, as Nora said above. "Too large" is relative, and there's of course nuance to this. Before enumerating how you might split this PR to make it reviewable, let me just demonstrate what I mean by an easier to-review (sub-) PR:

Note

Example. If you picked only a single clippy lint, applied it to the compiler/ tree only, then reviewed the changes yourself and think that the changes do improve the codebase in some way, then even if the diff touches a bunch of compiler files, they can still be mechanical. Even if the diff has quite a few files, the changes would be easy for the reviewer to (1) run the specific clippy lint themselves, and compare the warnings to your diff to verify for themselves, and (2) actually meaningfully review the diff, because the reviewer needs only to be concerned with the applicability of this one clippy lint and the correctness/desirability of the corresponding changes.

In contrast, this PR does several things that make it effectively-impossible to review:

• It does a "shotgun" surgery over the entire rust-lang/rust codebase. This is undesirable, because different maintenance teams maintain different parts of the codebase, and no single reviewer will be comfortable or able to review this PR in its entirety. Finding multiple reviewers from all affected teams is a lot of extra work.
• It mixes "native" in-tree code like compiler/ or library/ versus "external" subtree code that are primarily maintained in their own repos (e.g. rustfmt). This is strongly discouraged unless out of necessity², because doing so can introduce unnecessary/additional merge conflicts during subtree syncs. For instance, I will not accept the src/tools/rustfmt/ changes of this PR against this repository because it does not need to accompany a compiler/ tree change, and on the rustfmt side we are already facing difficulties trying to get the rust-lang/rustfmt <-> rust-lang/rust sync directions unblocked.
• It mixes corresponding changes for many different clippy lints, so that it's very difficult to tell which change is for which clippy lint.
• It touches unsafe {} code, mixed in a lot of changes to safe code. This makes it super easy to accidentally introduce unsoundness.

Now, I've written about what makes this PR very difficult to review. You may reasonably ask, well then, how do I make this set of changes easier to review and easier to merge?

Making this set of changes possible to review and merge

A few tips:

• Plan out smaller, more focused PRs.
• I would recommend a "boring" recipe of {1 subtree, 1-2 specific clippy lints, a brief discussion on why the lint and its corresponding changes seem desirable} for the split PRs. Bonus points if you can structure your commits such that each commit corresponds to a specific clippy lint (and please name which clippy lints!).
  • If you feel like one of such clippy lint and its corresponding change seems like a "no-brainer" or "seems almost always better", you can discuss with the relevant maintenance team on maybe even enabling and thus enforcing that lint for the specific tree (or across the entire repo).

Recommended subtree splits

On the subtree-split axis (the other axis being the which clippy lint(s) as I discussed previously), I think a reasonable split is:

Subtree	Relevant maintenance team / maintainer
compiler/	For compiler team
library/	For library team
src/build_helper/, src/tools/{build-manifest,cargotest,collect-license-metadata,features-status-dump,generate-copyright,generate-windows-sys,html-checker,linkchecker,llvm-bitcode-linker,opt-dist,remote-test-client,remote-test-server,replace-version-placeholder,tidy}	For bootstrap team
src/librustdoc/, src/tools/{jsondocck,jsondoclint,rustdoc-gui-test,rustdoc-themes}	For rustdoc and/or rustdoc-frontend team
src/tools/coverage-dump	@Zalathar if they want to, or bootstrap team as fallback
src/tools/{compiletest,run-make-support}	You can roll me for those (@jieyouxu)
src/tools/test-float-parse	@tgross35 if they want to, or bootstrap team as fallback

Caution

src/tools/rustfmt changes need to be submitted against rust-lang/rustfmt separately because it's a subtree, and we do not want to accrue more rustfmt-related merge conflicts than absolutely necessarily.

---

Design principle: Satisfying all clippy lints is a non-goal

As Boxy puts it (which I find is a very good characterization) in [2]:

fwiw clippy is not supposed to be used as a blanket "turn every lint on and listen to everything". you're supposed to pick and choose which lints you care about and selectively enable/disable them

This is particularly the case for the compiler/ and library/ trees because we are the language implementation details. For instance, a clippy lint warning against a manual implementation of a method might be completely nonsense for the library/ if we necessarily have to be the one defining and providing that manual impl so standard library consumers don't have to.

Now, clippy and its clippy::all default lint group tries to be a very sensible default for the "average" or "usual" Rust crate -- and that default I find quite reasonable³. What this does not mean is that users should unconditionally apply all changes recommended by the various clippy lints. Users should evaluate, for each instance, if the recommended change causes the code base to be actually better, in some way.

Basically, carefully evaluate if a given clippy lint has a very good signal-to-noise ratio (and seems almost always an improvement in some property, such as clarity or correctness), then we can try to evaluate to enable (and thus enforce) that specifically for compiler//library//other tree in CI. For instance, we run subsets of clippy lints for CI, configured differently for specific subtrees. See

rust/src/bootstrap/src/core/build_steps/clippy.rs

Lines 541 to 619 in 4422885

	builder.ensure(Bootstrap {
	// This will be the stage 1 compiler
	build_compiler: prepare_compiler_for_check(builder, self.target, Mode::ToolTarget),
	target: self.target,
	config: self.config.merge(&LintConfig {
	allow: vec![],
	warn: vec![],
	deny: vec!["warnings".into()],
	forbid: vec![],
	}),
	});
	let library_clippy_cfg = LintConfig {
	allow: vec!["clippy::all".into()],
	warn: vec![],
	deny: vec![
	"clippy::correctness".into(),
	"clippy::char_lit_as_u8".into(),
	"clippy::four_forward_slashes".into(),
	"clippy::needless_bool".into(),
	"clippy::needless_bool_assign".into(),
	"clippy::non_minimal_cfg".into(),
	"clippy::print_literal".into(),
	"clippy::same_item_push".into(),
	"clippy::single_char_add_str".into(),
	"clippy::to_string_in_format_args".into(),
	"clippy::unconditional_recursion".into(),
	],
	forbid: vec![],
	};
	builder.ensure(Std::from_build_compiler(
	// This will be the stage 1 compiler, to avoid building rustc stage 2 just to lint std
	builder.compiler(1, self.target),
	self.target,
	self.config.merge(&library_clippy_cfg),
	vec![],
	));
	let compiler_clippy_cfg = LintConfig {
	allow: vec!["clippy::all".into()],
	warn: vec![],
	deny: vec![
	"clippy::correctness".into(),
	"clippy::char_lit_as_u8".into(),
	"clippy::clone_on_ref_ptr".into(),
	"clippy::format_in_format_args".into(),
	"clippy::four_forward_slashes".into(),
	"clippy::needless_bool".into(),
	"clippy::needless_bool_assign".into(),
	"clippy::non_minimal_cfg".into(),
	"clippy::print_literal".into(),
	"clippy::same_item_push".into(),
	"clippy::single_char_add_str".into(),
	"clippy::to_string_in_format_args".into(),
	"clippy::unconditional_recursion".into(),
	],
	forbid: vec![],
	};
	// This will lint stage 2 rustc using stage 1 Clippy
	builder.ensure(Rustc::new(
	builder,
	self.target,
	self.config.merge(&compiler_clippy_cfg),
	vec![],
	));
	let rustc_codegen_gcc = LintConfig {
	allow: vec![],
	warn: vec![],
	deny: vec!["warnings".into()],
	forbid: vec![],
	};
	// This will check stage 2 rustc
	builder.ensure(CodegenGcc::new(
	builder,
	self.target,
	self.config.merge(&rustc_codegen_gcc),
	));
	}

.

Footnotes

1. Specifically, clippy::all. ↩

2. Such as if a change to a rustc_private-tool like rust-analyzer must be accompanied a compiler/ tree change to even bootstrap ↩

3. Well, I'm not a fan of clippy::all including clippy::style, but that's another story. ↩

[jieyouxu]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[Kivooeo]

I checked some of this changes, and I'd say that clippy is doing a good job here, I'd love to see just a compiler/ commit (or a separate PR) for that kind of clean up!

[umbrageodotus]

@jieyouxu Thanks for the feedback. I did not blindly follow clippy: I only did the easiest changes like expect with a fn call -> unwrap_or_else, unnecessary references, un-nest useless branches, etc. Those alone were most the errors.

[umbrageodotus]

Also, I did it all manually - I didn't just clippy --fix, as to manually see what fits best.

[umbrageodotus]

Also also, does anyone know why the goddamn PR isn't working? It's just so next time I avoid the issue.

[jieyouxu]

Thanks for the feedback. I did not blindly follow clippy: I only did the easiest changes like expect with a fn call -> unwrap_or_else, unnecessary references, un-nest useless branches, etc. Those alone were most the errors.

Also, I did it all manually - I didn't just clippy --fix, as to manually see what fits best.

Sounds good 👍 Then, we would only need to make this set of changes easier to review, by breaking it up into more reviewable chunks.

[jieyouxu]

Also also, does anyone know why the goddamn PR isn't working? It's just so next time I avoid the issue.

Usually there will be a bot @rust-log-analyzer that would post a summary snippet of which job failed and what failed, but sometimes github webhooks can be missed or for whatever other reasons. In this case of failed PR CI, you can check the list of failed checks

and look at the log for the failing job, aarch64-gnu-llvm-20-1. If you look inside the logs (and scroll to the bottom), it has a bunch of coverage-map test failures

2025-10-11T15:11:51.6310078Z ------coverage-dump stderr------------------------------
2025-10-11T15:11:51.6310196Z Error: couldn't resolve global file: 5014445792341297956, 1

which might be from your compiler/bootstrap/coverage-dump tool changes.

[umbrageodotus]

I looked at it, it was just... absolutely incomprehensible...

[Kivooeo]

stage 2 coverage tests (tests/coverage) failed, something with raw bytes comparing, seems like it needs to be reblessed, but I never touched a stage 2 tests before, so I'm not sure how to do this

Can you try something like ./x test coverage --bless maybe

[jieyouxu]

That does not look like a snapshot difference, that looks like a genuine failure. This is also why we recommended the splits, so that it's easier to see what changed and less work to rule out what isn't involved. I do not have the bandwidth to determine why this failure is happening.

[Kivooeo]

hm, can I do this? seems like it works, @jieyouxu mind to delegate me rights on this one sooner or later?

r? Kivooeo

[Kivooeo]

This change is breaking coverage tests (I test it locally)

[jieyouxu]

hm, can I do this?

I don't think it is particularly productive to review this PR in its current shape as-is. Even if you review it, I do not want to land this PR like this. Again, see split discussion above.

[Kivooeo]

I don't think it is particularly productive to review this PR in its current shape as-is

I don't either and waiting on split