Skip to content

privacy: Type privacy lints fixes and cleanups #112670

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

Merged
merged 4 commits into from
Jun 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions compiler/rustc_feature/src/active.rs
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,8 @@ declare_features! (
/// Allows creation of instances of a struct by moving fields that have
/// not changed from prior instances of the same struct (RFC #2528)
(active, type_changing_struct_update, "1.58.0", Some(86555), None),
/// Allows using type privacy lints (`private_interfaces`, `private_bounds`, `unnameable_types`).
(active, type_privacy_lints, "CURRENT_RUSTC_VERSION", Some(48054), None),
/// Enables rustc to generate code that instructs libstd to NOT ignore SIGPIPE.
(active, unix_sigpipe, "1.65.0", Some(97889), None),
/// Allows unsized fn parameters.
Expand Down
8 changes: 2 additions & 6 deletions compiler/rustc_lint/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -665,9 +665,7 @@ declare_lint_pass!(MissingCopyImplementations => [MISSING_COPY_IMPLEMENTATIONS])

impl<'tcx> LateLintPass<'tcx> for MissingCopyImplementations {
fn check_item(&mut self, cx: &LateContext<'_>, item: &hir::Item<'_>) {
if !(cx.effective_visibilities.is_reachable(item.owner_id.def_id)
&& cx.tcx.local_visibility(item.owner_id.def_id).is_public())
{
if !cx.effective_visibilities.is_reachable(item.owner_id.def_id) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the local visibility check just redundant because effective visibility is always stricter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, with 95a24c6 reachable to other crates implies pub again, so we revert various "adaptation" changes from #111801 too.

return;
}
let (def, ty) = match item.kind {
Expand Down Expand Up @@ -786,9 +784,7 @@ impl_lint_pass!(MissingDebugImplementations => [MISSING_DEBUG_IMPLEMENTATIONS]);

impl<'tcx> LateLintPass<'tcx> for MissingDebugImplementations {
fn check_item(&mut self, cx: &LateContext<'_>, item: &hir::Item<'_>) {
if !(cx.effective_visibilities.is_reachable(item.owner_id.def_id)
&& cx.tcx.local_visibility(item.owner_id.def_id).is_public())
{
if !cx.effective_visibilities.is_reachable(item.owner_id.def_id) {
return;
}

Expand Down
10 changes: 8 additions & 2 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4263,6 +4263,7 @@ declare_lint! {
/// ### Example
///
/// ```rust,compile_fail
/// # #![feature(type_privacy_lints)]
/// # #![allow(unused)]
/// # #![allow(private_in_public)]
/// #![deny(private_interfaces)]
Expand All @@ -4287,6 +4288,7 @@ declare_lint! {
pub PRIVATE_INTERFACES,
Allow,
"private type in primary interface of an item",
@feature_gate = sym::type_privacy_lints;
}

declare_lint! {
Expand All @@ -4297,6 +4299,7 @@ declare_lint! {
/// ### Example
///
/// ```rust,compile_fail
/// # #![feature(type_privacy_lints)]
/// # #![allow(private_in_public)]
/// # #![allow(unused)]
/// #![deny(private_bounds)]
Expand All @@ -4316,7 +4319,8 @@ declare_lint! {
/// the item actually provides.
pub PRIVATE_BOUNDS,
Allow,
"private type in secondary interface of an item"
"private type in secondary interface of an item",
@feature_gate = sym::type_privacy_lints;
}

declare_lint! {
Expand All @@ -4326,6 +4330,7 @@ declare_lint! {
/// ### Example
///
/// ```rust,compile_fail
/// # #![feature(type_privacy_lints)]
/// # #![allow(unused)]
/// #![deny(unnameable_types)]
/// mod m {
Expand All @@ -4344,5 +4349,6 @@ declare_lint! {
/// you can name the type `T` as well, this lint attempts to enforce this rule.
pub UNNAMEABLE_TYPES,
Allow,
"effective visibility of a type is larger than the area in which it can be named"
"effective visibility of a type is larger than the area in which it can be named",
@feature_gate = sym::type_privacy_lints;
}
35 changes: 19 additions & 16 deletions compiler/rustc_middle/src/middle/privacy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
use crate::ty::{TyCtxt, Visibility};
use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
use rustc_hir::def::DefKind;
use rustc_macros::HashStable;
use rustc_query_system::ich::StableHashingContext;
use rustc_span::def_id::{LocalDefId, CRATE_DEF_ID};
Expand Down Expand Up @@ -148,13 +149,12 @@ impl EffectiveVisibilities {
};
}

pub fn check_invariants(&self, tcx: TyCtxt<'_>, early: bool) {
pub fn check_invariants(&self, tcx: TyCtxt<'_>) {
if !cfg!(debug_assertions) {
return;
}
for (&def_id, ev) in &self.map {
// More direct visibility levels can never go farther than less direct ones,
// neither of effective visibilities can go farther than nominal visibility,
// and all effective visibilities are larger or equal than private visibility.
let private_vis = Visibility::Restricted(tcx.parent_module_from_def_id(def_id));
let span = tcx.def_span(def_id.to_def_id());
Expand All @@ -175,17 +175,20 @@ impl EffectiveVisibilities {
ev.reachable_through_impl_trait
);
}
let nominal_vis = tcx.visibility(def_id);
// FIXME: `rustc_privacy` is not yet updated for the new logic and can set
// effective visibilities that are larger than the nominal one.
if !nominal_vis.is_at_least(ev.reachable_through_impl_trait, tcx) && early {
span_bug!(
span,
"{:?}: reachable_through_impl_trait {:?} > nominal {:?}",
def_id,
ev.reachable_through_impl_trait,
nominal_vis
);
// All effective visibilities except `reachable_through_impl_trait` are limited to
// nominal visibility. For some items nominal visibility doesn't make sense so we
// don't check this condition for them.
if !matches!(tcx.def_kind(def_id), DefKind::Impl { .. }) {
let nominal_vis = tcx.visibility(def_id);
if !nominal_vis.is_at_least(ev.reachable, tcx) {
span_bug!(
span,
"{:?}: reachable {:?} > nominal {:?}",
def_id,
ev.reachable,
nominal_vis
);
}
}
}
}
Expand All @@ -212,7 +215,7 @@ impl<Id: Eq + Hash> EffectiveVisibilities<Id> {
pub fn update(
&mut self,
id: Id,
nominal_vis: Option<Visibility>,
max_vis: Option<Visibility>,
lazy_private_vis: impl FnOnce() -> Visibility,
inherited_effective_vis: EffectiveVisibility,
level: Level,
Expand All @@ -236,8 +239,8 @@ impl<Id: Eq + Hash> EffectiveVisibilities<Id> {
if !(inherited_effective_vis_at_prev_level == inherited_effective_vis_at_level
&& level != l)
{
calculated_effective_vis = if let Some(nominal_vis) = nominal_vis && !nominal_vis.is_at_least(inherited_effective_vis_at_level, tcx) {
nominal_vis
calculated_effective_vis = if let Some(max_vis) = max_vis && !max_vis.is_at_least(inherited_effective_vis_at_level, tcx) {
max_vis
} else {
inherited_effective_vis_at_level
}
Expand Down
Loading