privacy: Stabilize lint unnameable_types

[petrochenkov]

This is the last piece of "RFC #2145: Type privacy and private-in-public lints".

Having unstable lints is not very useful because you cannot even dogfood them in the compiler/stdlib in this case (#113284).
The worst thing that may happen when a lint is removed are some removed_lints warnings, but I haven't heard anyone suggesting removing this specific lint.

This lint is allow-by-default and is supposed to be enabled explicitly.
Some false positives are expected, because sometimes unnameable types are a legitimate pattern.
This lint also have some unnecessary false positives, that can be fixed - see #120146 and #120149.

Closes #48054.

[rustbot]

r? @davidtwco

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

[petrochenkov]

cc @Bryanskiy

[davidtwco]

Implementation looks good to me, r=me after relevant sign-offs.

[joshtriplett]

We talked about this in today's @rust-lang/lang meeting. We're in favor of shipping this. Some of us were favorable towards making it warn-by-default in the future.

We had one suggestion for naming and application of this lint: if we fired the lint specifically at the point where you are applying a non-pub visibility to a module with pub items, and called the lint something like suppressed_pub, then that would allow for an explicit opt-in for "I'm intentionally suppressing public items" by writing #[allow(suppressed_pub)] on the point where you're applying the non-pub visibility to the module with pub items.

[scottmcm]

We discussed this in the lang triage meeting today. There was lots of discussion about the exact details of the lint, but general agreement that as allow-by-default we should let people start trying this so that they can give feedback about what it reports and how.

@rfcbot fcp merge

Some of the topics that came up were about potentially linting on the module that "hides" the things inside it instead of on the items hidden, what the right way to suppress this when needed would be, and why macros might be generating things as pub expecting that they'd be hidden later like this.

But as far as I could tell, none of those seemed like blockers for this PR making the lint available to people, just that people might potentially have concerns should a future PR propose moving it to warn-by-default, depending on details.

[rfcbot]

Team member @scottmcm has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[joshtriplett]

I do think we should consider renaming this to suppressed_pub (or something that similarly focused on the fact that a type was pub in a module but an outer module prevents it from being exposed) before stabilizing it, personally.

[traviscross]

@rustbot labels -I-lang-nominated

As evidenced by the discussion above, we discussed this on the lang call today, and while there was discussion about where to go from here, no one seemed opposed to stabilizing an allow-by-default lint. The outcome is the proposed FCP merge, so we can unnominate this.

Thanks to @petrochenkov for pushing this forward.

[petrochenkov]

@joshtriplett

I do think we should consider renaming this to suppressed_pub (or something that similarly focused on the fact that a type was pub in a module but an outer module prevents it from being exposed) before stabilizing it, personally.

What you are describing here looks like another already existing allow-by-default lint - unreachable_pub.
unnameable_types focuses on a different things - types that are indeed public, but cannot be explicitly named by users of the crate.

unreachable_pub - something is locally marked as pub, but not actually reachable from other crates.
unnameable_types - something that is reachable from other crates (which implies pub), but cannot be named.

[tmandry]

@petrochenkov It sounds like you are using a definition of "reachable" that is different from what I understood the term to mean (which is the same as how you are using "nameable"). Explaining the terms in error messages and docs would help.

My feeling is that we should just redefine unreachable_pub to include this lint. The distinction "can create a value of unnameable type" does not seem important enough to create a separate lint, when we can already do that with closures and async blocks. But this has already been RFC'd, so I will not block stabilization.

[davidtwco]

Implementation looks good to me, r=me once lang discussions resolved.

[petrochenkov]

@tmandry

It sounds like you are using a definition of "reachable" that is different from what I understood the term to mean (which is the same as how you are using "nameable"). Explaining the terms in error messages and docs would help.

I used the naming used in the compiler (struct EffectiveVisibility)

• directly visible - both the item and all its parent modules are public, so the item can be referred by a direct path from the outside - my_crate::my_mod::Item (this is only used by rustdoc)
• reexported aka nameable - the item is either directly visible, or has a chain of public reexports leading to it, even if some of its direct parent modules are private - my_crate::my_mod_reexport::Item
• reachable - the item is either reexported, or is leaked from the crate in some other way - is returned by a public function, or by a public field, or occurs inside of some public type alias (basically it appears in a signature of something already reachable)

Also

• pub - something that has a local pub annotation, without considering any global properties like reexports or reachability

[petrochenkov]

So, summarizing again:

unreachable_pub

An item is locally pub, but it is neither reexported nor leaks to other crates in any way.

Recommended action: remove pub, or replace it with pub(crate)/pub(super)/etc.

This lint is reported for all entities because pub can be removed from anything.
(Although we probably need to tweak how this lint to not fire on fields of non-pub types, that's its main point of annoyance right now.)

unnameable_types

An item is reachable (leaks to other crates, this implies pub), but cannot be named because it's not reexported.

Recommended action: either reexport the item properly to make it nameable, or document that users are not supposed to be able to name it for one reason or another. In this case pub cannot be removed or changed to some more restricted visibility.

This lint is only reported for types and traits, because reporting it for other entities is redundant / not useful in nearly all cases.

This behavior is what many people expected from the old private_in_public lint, and were surprised because that lint didn't work like this.

• How private_in_public lint really worked: non-pub type/trait appears in a signature of a pub function.
• How some people expected it to work: "private" (actually unreachable) type/trait appears in a signature of a "public" (actually reachable/reexported) function. This matches unnameable_types.

[petrochenkov]

I've updated some lint docs.
Also, re-nominating to make progress.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[davidtwco]

Already reviewed the implementation and the FCP has finished, so will approve this now.

@bors r+

[bors]

📌 Commit 95ec17a has been approved by davidtwco

It is now in the queue for this repository.

[nnethercote]

I have been playing with adding #![deny(unreachable_pub)] to the compiler. I find visibilities very important when reading and navigating the codebase, and I often make commits that reduce unneeded visibilities. So any tools that help automate that are very welcome.

Having said that, I do have one suggestion: unreachable_pub always suggests converting a pub to a pub(crate), but sometimes it is possible to remove the visibility entirely. It would be much nicer if it was able to distinguish these two cases. I just went through the entire rustc_codegen_llvm crate doing the following:

• Add #![unreachable_pub]
• For every pub it complains about, just remove the pub. (Even though it suggests changing to pub(crate).)
• Then recompile repeatedly, adding pub(crate) to all the cases that the compiler says need it.

A bit tedious, and would have been a lot easier if the lint could distinguish the two cases.