Add #[warn(unreachable_pub)] to a bunch of compiler crates

[nnethercote]

By default unreachable_pub identifies things that need not be pub and tells you to make them pub(crate). But sometimes those things don't need any kind of visibility. So they way I did these was to remove the visibility entirely for each thing the lint identifies, and then add pub(crate) back in everywhere the compiler said it was necessary. (Or occasionally pub(super) when context suggested that was appropriate.) Tedious, but results in more pub removal.

There are plenty more crates to do but this seems like enough for a first PR.

r? @compiler-errors

[rustbot]

Some changes occurred in coverage instrumentation.

cc @Zalathar

[Kobzol]

Just my 2 cents: I prefer the style where you just use pub in the internal modules, and then limit visibility in the parent (for multiple reasons, one of them being that pub(crate) is IMO too verbose). It seems to be more uniform to me, in the sense that you export things from the module, and then decide in the parent what to export further and what not to export, rather than hard-coding the visibility to the whole crate in nested modules.

For example, in rustc_ast_lowering/src/lib.rs, all modules are actually private, and the crate selectively exports (or declares as pub in the root library module) just a few selected items. It doesn't seem to me that adding pub(crate) to many items in the nested modules (which are not reexported from the crate anyway) adds a lot of value.

Given the concerns expressed by others, I withdraw my approval.

[compiler-errors]

Yeah, this is far more reaching than what I was asking for in the original PR (diagnostic structs). I don't have the energy to review this and also don't think we're gaining much -- this really seems like churn.

r? compiler

[Nadrieril]

r? @Urgau since you seem interested in solving this

[nnethercote]

I personally hate having to look at some outer level (sometimes the immediate struct, sometimes the module, sometimes a use item in an entirely different file) to learn that a seemingly pub thing is not actually pub. But I can see I'm in the minority.

[nnethercote]

I also don't understand why this change would be desirable on diagnostic structs but not on other things.

[Kobzol]

I personally hate having to look at some outer level (sometimes the immediate struct, sometimes the module, sometimes a use item in an entirely different file) to learn that a seemingly pub thing is not actually pub.

It would be nice if IDEs could show the computed crate visibility on the struct/field. That way we could see both the final visibility, without having to specify pub(crate) everywhere. I created a ticket for RustRover :)

But otherwise, this is just a matter of taste, of course.

[petrochenkov]

I personally hate having to look at some outer level (sometimes the immediate struct, sometimes the module, sometimes a use item in an entirely different file) to learn that a seemingly pub thing is not actually pub. But I can see I'm in the minority.

I also don't like to look elsewhere, but in case of nearly all structs and many impls the outer visibility is in the immediate proximity of the fields/methods, or at worst on the same page. In those cases it's better to avoid the noise and keep just pub.

[bors]

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

[Urgau]

While I like this change, the concerns expressed by others makes me realize that this change is probably to big to just be approved without further discussion.

The major change proposal page on the forge quotes changing coding conventions as a potential reason for an MCP. @nnethercote if you are interested in pursuing this change further, could you open an MCP for it.

@rustbot author

[nnethercote]

I have updated this PR for the new unreachable_pub checking implemented in #126040. The diff has reduced from +946/-917 to +506/-467. Better?

[nnethercote]

@Urgau: the MCP has passed, so this is ready for final review. Thanks!

[Urgau]

@bors r+ rollup

[bors]

📌 Commit cc84442 has been approved by Urgau

It is now in the queue for this repository.