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

[nnethercote]

trafficstars

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

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

     |
1172 |     pub fn oem_code_page() -> u32 {
     |     ---^^^^^^^^^^^^^^^^^^^^^^^^^^
     |     |
     |     help: consider restricting its visibility: `pub(crate)`
     = help: or consider exporting it for use by other crates
note: the lint level is defined here
    --> compiler/rustc_codegen_ssa/src/lib.rs:7:9
     |
     |
7    | #![deny(unreachable_pub)]
     |         ^^^^^^^^^^^^^^^

error: unreachable `pub` item
    --> compiler/rustc_codegen_ssa/src/back/link.rs:1195:5
     |
1195 |     pub fn locale_byte_str_to_string(s: &[u8], code_page: u32) -> Option<String> {
     |     ---^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |     |
     |     help: consider restricting its visibility: `pub(crate)`
     = help: or consider exporting it for use by other crates

    Checking rustc_query_impl v0.0.0 (/checkout/compiler/rustc_query_impl)
error: could not compile `rustc_codegen_ssa` (lib) due to 2 previous errors

[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]

:umbrella: 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?

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

#12 [ 5/11] RUN npm install [email protected] [email protected] -g
#12 5.038 npm WARN deprecated [email protected]: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
#12 5.107 npm WARN deprecated [email protected]: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
#12 5.113 npm WARN deprecated [email protected]: Rimraf versions prior to v4 are no longer supported
#12 5.143 npm WARN deprecated @humanwhocodes/[email protected]: Use @eslint/config-array instead
#12 5.182 npm WARN deprecated [email protected]: Glob versions prior to v9 are no longer supported
#12 5.182 npm WARN deprecated @humanwhocodes/[email protected]: Use @eslint/object-schema instead
#12 6.134 
#12 6.134 added 205 packages in 6s
#12 6.134 
#12 6.134 20 packages are looking for funding
---
#16 3.487 Building wheels for collected packages: reuse
#16 3.488   Building wheel for reuse (pyproject.toml): started
#16 3.820   Building wheel for reuse (pyproject.toml): finished with status 'done'
#16 3.821   Created wheel for reuse: filename=reuse-1.1.0-cp310-cp310-manylinux_2_35_x86_64.whl size=181117 sha256=f5f58750481f69515c2c0d1d503daf565e2565c370d07fc6aeb95fe3498b4269
#16 3.822   Stored in directory: /tmp/pip-ephem-wheel-cache-kcemqh4f/wheels/c2/3c/b9/1120c2ab4bd82694f7e6f0537dc5b9a085c13e2c69a8d0c76d
#16 3.824 Installing collected packages: boolean-py, binaryornot, setuptools, reuse, python-debian, markupsafe, license-expression, jinja2, chardet
#16 3.846   Attempting uninstall: setuptools
#16 3.846     Found existing installation: setuptools 59.6.0
#16 3.848     Not uninstalling setuptools at /usr/lib/python3/dist-packages, outside environment /usr
---

error: unreachable `pub` item
    --> compiler/rustc_codegen_ssa/src/back/link.rs:1195:5
     |
1195 |     pub fn locale_byte_str_to_string(s: &[u8], code_page: u32) -> Option<String> {
     |     ---^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |     help: consider restricting its visibility: `pub(crate)`
     |
     = help: or consider exporting it for use by other crates

[Urgau]

I'm personally fine with the PR as is, the diff looks good to me.

@Kobzol @petrochenkov How do feel about the new diff?

[Kobzol]

I'm still not a big fan of pub(crate), but the new diff looks much better to me, so fine by me.

[bors]

:umbrella: The latest upstream changes (presumably #127819) made this pull request unmergeable. Please resolve the merge conflicts.

[petrochenkov]

I'm personally fine with the PR as is, the diff looks good to me.

@Kobzol @petrochenkov How do feel about the new diff?

It looks acceptable to me now.

[Urgau]

@nnethercote could you resolve the conflicts, I will take a last look after that.

@rustbot author

[nnethercote]

I'm planning to file an MCP to use this lint for all (or almost all) compiler crate. This PR will serve as a good reference for that.

[petrochenkov]

As a side note, the compiler should have enough information to automatically suggest pub(super) instead of pub(crate) when possible in unreachable_pub (if it doesn't already do that).

[nnethercote]

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

[Urgau]

@bors r+ rollup

[bors]

:pushpin: Commit cc8444274b33ffd01fd8cb35199f02697e632852 has been approved by Urgau

It is now in the queue for this repository.