Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixes wrong unreachable_pub lints on nested and glob public reexport #87487

Merged
merged 1 commit into from
Jan 10, 2022
Merged

Fixes wrong unreachable_pub lints on nested and glob public reexport #87487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 0 additions & 3 deletions compiler/rustc_errors/src/snippet.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,6 @@ pub enum AnnotationType {
/// Annotation under a single line of code
Singleline,

/// Annotation enclosing the first and last character of a multiline span
Multiline(MultilineAnnotation),

// The Multiline type above is replaced with the following three in order
// to reuse the current label drawing code.
//
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_lint/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,8 @@ use unused::*;

/// Useful for other parts of the compiler / Clippy.
pub use builtin::SoftLints;
pub use context::{CheckLintNameResult, EarlyContext, LateContext, LintContext, LintStore};
pub use context::{CheckLintNameResult, FindLintError, LintStore};
pub use context::{EarlyContext, LateContext, LintContext};
pub use early::check_ast_crate;
pub use late::check_crate;
pub use passes::{EarlyLintPass, LateLintPass};
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/middle/privacy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ pub enum AccessLevel {
}

/// Holds a map of accessibility levels for reachable HIR nodes.
#[derive(Debug)]
#[derive(Debug, Clone)]
pub struct AccessLevels<Id = LocalDefId> {
pub map: FxHashMap<Id, AccessLevel>,
}
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_middle/src/ty/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ pub use generics::*;
pub use vtable::*;

use crate::metadata::ModChild;
use crate::middle::privacy::AccessLevels;
use crate::mir::{Body, GeneratorLayout};
use crate::traits::{self, Reveal};
use crate::ty;
Expand Down Expand Up @@ -123,6 +124,7 @@ pub struct ResolverOutputs {
pub definitions: rustc_hir::definitions::Definitions,
pub cstore: Box<CrateStoreDyn>,
pub visibilities: FxHashMap<LocalDefId, Visibility>,
pub access_levels: AccessLevels,
pub extern_crate_map: FxHashMap<LocalDefId, CrateNum>,
pub maybe_unused_trait_imports: FxHashSet<LocalDefId>,
pub maybe_unused_extern_crates: Vec<(LocalDefId, Span)>,
Expand Down
220 changes: 39 additions & 181 deletions compiler/rustc_privacy/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ use rustc_data_structures::fx::FxHashSet;
use rustc_errors::struct_span_err;
use rustc_hir as hir;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::def_id::{DefId, LocalDefId, LocalDefIdSet};
use rustc_hir::def_id::{CRATE_DEF_ID, CRATE_DEF_INDEX, LOCAL_CRATE};
use rustc_hir::def_id::{DefId, LocalDefId, LocalDefIdSet, CRATE_DEF_ID};
use rustc_hir::intravisit::{self, DeepVisitor, NestedVisitorMap, Visitor};
use rustc_hir::{AssocItemKind, HirIdSet, Node, PatKind};
use rustc_middle::bug;
Expand All @@ -26,7 +25,7 @@ use rustc_middle::ty::subst::InternalSubsts;
use rustc_middle::ty::{self, Const, GenericParamDefKind, TraitRef, Ty, TyCtxt, TypeFoldable};
use rustc_session::lint;
use rustc_span::hygiene::Transparency;
use rustc_span::symbol::{kw, sym, Ident};
use rustc_span::symbol::{kw, Ident};
use rustc_span::Span;
use rustc_trait_selection::traits::const_evaluatable::{self, AbstractConst};

Expand Down Expand Up @@ -436,6 +435,15 @@ impl<'tcx> EmbargoVisitor<'tcx> {
self.access_levels.map.get(&def_id).copied()
}

fn update_with_hir_id(
&mut self,
hir_id: hir::HirId,
level: Option<AccessLevel>,
) -> Option<AccessLevel> {
let def_id = self.tcx.hir().local_def_id(hir_id);
self.update(def_id, level)
}

/// Updates node level and returns the updated level.
fn update(&mut self, def_id: LocalDefId, level: Option<AccessLevel>) -> Option<AccessLevel> {
let old_level = self.get(def_id);
Expand Down Expand Up @@ -623,54 +631,6 @@ impl<'tcx> EmbargoVisitor<'tcx> {
| DefKind::Generator => (),
}
}

/// Given the path segments of an `ItemKind::Use`, then we need
/// to update the visibility of the intermediate use so that it isn't linted
/// by `unreachable_pub`.
///
/// This isn't trivial as `path.res` has the `DefId` of the eventual target
/// of the use statement not of the next intermediate use statement.
///
/// To do this, consider the last two segments of the path to our intermediate
/// use statement. We expect the penultimate segment to be a module and the
/// last segment to be the name of the item we are exporting. We can then
/// look at the items contained in the module for the use statement with that
/// name and update that item's visibility.
///
/// FIXME: This solution won't work with glob imports and doesn't respect
/// namespaces. See <https://github.com/rust-lang/rust/pull/57922#discussion_r251234202>.
fn update_visibility_of_intermediate_use_statements(
&mut self,
segments: &[hir::PathSegment<'_>],
) {
if let [.., module, segment] = segments {
if let Some(item) = module
.res
.and_then(|res| res.mod_def_id())
// If the module is `self`, i.e. the current crate,
// there will be no corresponding item.
.filter(|def_id| def_id.index != CRATE_DEF_INDEX || def_id.krate != LOCAL_CRATE)
.and_then(|def_id| def_id.as_local())
.map(|module_hir_id| self.tcx.hir().expect_item(module_hir_id))
{
if let hir::ItemKind::Mod(m) = &item.kind {
for &item_id in m.item_ids {
let item = self.tcx.hir().item(item_id);
if !self.tcx.hygienic_eq(
segment.ident,
item.ident,
item_id.def_id.to_def_id(),
) {
continue;
}
if let hir::ItemKind::Use(..) = item.kind {
self.update(item.def_id, Some(AccessLevel::Exported));
}
}
}
}
}
}
}

impl<'tcx> Visitor<'tcx> for EmbargoVisitor<'tcx> {
Expand All @@ -683,120 +643,22 @@ impl<'tcx> Visitor<'tcx> for EmbargoVisitor<'tcx> {
}

fn visit_item(&mut self, item: &'tcx hir::Item<'tcx>) {
let inherited_item_level = match item.kind {
let item_level = match item.kind {
hir::ItemKind::Impl { .. } => {
Option::<AccessLevel>::of_impl(item.def_id, self.tcx, &self.access_levels)
}
// Only exported `macro_rules!` items are public, but they always are.
hir::ItemKind::Macro(MacroDef { macro_rules: true, .. }) => {
let def_id = item.def_id.to_def_id();
let is_macro_export = self.tcx.has_attr(def_id, sym::macro_export);
if is_macro_export { Some(AccessLevel::Public) } else { None }
}
// Foreign modules inherit level from parents.
hir::ItemKind::ForeignMod { .. } => self.prev_level,
// Other `pub` items inherit levels from parents.
hir::ItemKind::Const(..)
| hir::ItemKind::Enum(..)
| hir::ItemKind::ExternCrate(..)
| hir::ItemKind::GlobalAsm(..)
| hir::ItemKind::Fn(..)
| hir::ItemKind::Macro(..)
| hir::ItemKind::Mod(..)
| hir::ItemKind::Static(..)
| hir::ItemKind::Struct(..)
| hir::ItemKind::Trait(..)
| hir::ItemKind::TraitAlias(..)
| hir::ItemKind::OpaqueTy(..)
| hir::ItemKind::TyAlias(..)
| hir::ItemKind::Union(..)
| hir::ItemKind::Use(..) => {
if item.vis.node.is_pub() {
self.prev_level
} else {
None
}
let impl_level =
Option::<AccessLevel>::of_impl(item.def_id, self.tcx, &self.access_levels);
self.update(item.def_id, impl_level)
}
_ => self.get(item.def_id),
};

// Update level of the item itself.
let item_level = self.update(item.def_id, inherited_item_level);

// Update levels of nested things.
match item.kind {
hir::ItemKind::Enum(ref def, _) => {
for variant in def.variants {
let variant_level =
self.update(self.tcx.hir().local_def_id(variant.id), item_level);
if let Some(ctor_hir_id) = variant.data.ctor_hir_id() {
self.update(self.tcx.hir().local_def_id(ctor_hir_id), item_level);
}
for field in variant.data.fields() {
self.update(self.tcx.hir().local_def_id(field.hir_id), variant_level);
}
}
}
hir::ItemKind::Impl(ref impl_) => {
for impl_item_ref in impl_.items {
if impl_.of_trait.is_some()
|| self.tcx.visibility(impl_item_ref.id.def_id) == ty::Visibility::Public
{
self.update(impl_item_ref.id.def_id, item_level);
}
}
}
hir::ItemKind::Trait(.., trait_item_refs) => {
for trait_item_ref in trait_item_refs {
self.update(trait_item_ref.id.def_id, item_level);
}
}
hir::ItemKind::Struct(ref def, _) | hir::ItemKind::Union(ref def, _) => {
if let Some(ctor_hir_id) = def.ctor_hir_id() {
self.update(self.tcx.hir().local_def_id(ctor_hir_id), item_level);
}
for field in def.fields() {
if field.vis.node.is_pub() {
self.update(self.tcx.hir().local_def_id(field.hir_id), item_level);
}
}
}
hir::ItemKind::Macro(ref macro_def) => {
self.update_reachability_from_macro(item.def_id, macro_def);
}
hir::ItemKind::ForeignMod { items, .. } => {
for foreign_item in items {
if self.tcx.visibility(foreign_item.id.def_id) == ty::Visibility::Public {
self.update(foreign_item.id.def_id, item_level);
}
}
}

hir::ItemKind::OpaqueTy(..)
| hir::ItemKind::Use(..)
| hir::ItemKind::Static(..)
| hir::ItemKind::Const(..)
| hir::ItemKind::GlobalAsm(..)
| hir::ItemKind::TyAlias(..)
| hir::ItemKind::Mod(..)
| hir::ItemKind::TraitAlias(..)
| hir::ItemKind::Fn(..)
| hir::ItemKind::ExternCrate(..) => {}
}

// Mark all items in interfaces of reachable items as reachable.
match item.kind {
// The interface is empty.
hir::ItemKind::Macro(..) | hir::ItemKind::ExternCrate(..) => {}
hir::ItemKind::ExternCrate(..) => {}
// All nested items are checked by `visit_item`.
hir::ItemKind::Mod(..) => {}
// Re-exports are handled in `visit_mod`. However, in order to avoid looping over
// all of the items of a mod in `visit_mod` looking for use statements, we handle
// making sure that intermediate use statements have their visibilities updated here.
hir::ItemKind::Use(path, _) => {
if item_level.is_some() {
self.update_visibility_of_intermediate_use_statements(path.segments.as_ref());
}
}
hir::ItemKind::Use(..) => {}
// The interface is empty.
hir::ItemKind::GlobalAsm(..) => {}
hir::ItemKind::OpaqueTy(..) => {
Expand Down Expand Up @@ -847,6 +709,14 @@ impl<'tcx> Visitor<'tcx> for EmbargoVisitor<'tcx> {
}
// Visit everything except for private impl items.
hir::ItemKind::Impl(ref impl_) => {
for impl_item_ref in impl_.items {
if impl_.of_trait.is_some()
|| self.tcx.visibility(impl_item_ref.id.def_id) == ty::Visibility::Public
{
self.update(impl_item_ref.id.def_id, item_level);
}
}

if item_level.is_some() {
self.reach(item.def_id, item_level).generics().predicates().ty().trait_ref();

Expand All @@ -861,15 +731,21 @@ impl<'tcx> Visitor<'tcx> for EmbargoVisitor<'tcx> {
}
}
}

// Visit everything, but enum variants have their own levels.
hir::ItemKind::Enum(ref def, _) => {
if item_level.is_some() {
self.reach(item.def_id, item_level).generics().predicates();
}

let enum_level = self.get(item.def_id);
for variant in def.variants {
let variant_level = self.get(self.tcx.hir().local_def_id(variant.id));
let variant_level = self.update_with_hir_id(variant.id, enum_level);

if variant_level.is_some() {
if let Some(ctor_id) = variant.data.ctor_hir_id() {
self.update_with_hir_id(ctor_id, variant_level);
}

for field in variant.data.fields() {
self.reach(self.tcx.hir().local_def_id(field.hir_id), variant_level)
.ty();
Expand All @@ -880,6 +756,9 @@ impl<'tcx> Visitor<'tcx> for EmbargoVisitor<'tcx> {
}
}
}
hir::ItemKind::Macro(ref macro_def) => {
self.update_reachability_from_macro(item.def_id, macro_def);
}
// Visit everything, but foreign items have their own levels.
hir::ItemKind::ForeignMod { items, .. } => {
for foreign_item in items {
Expand Down Expand Up @@ -920,27 +799,6 @@ impl<'tcx> Visitor<'tcx> for EmbargoVisitor<'tcx> {
intravisit::walk_block(self, b);
self.prev_level = orig_level;
}

fn visit_mod(&mut self, m: &'tcx hir::Mod<'tcx>, _sp: Span, id: hir::HirId) {
// This code is here instead of in visit_item so that the
// crate module gets processed as well.
if self.prev_level.is_some() {
let def_id = self.tcx.hir().local_def_id(id);
if let Some(exports) = self.tcx.module_reexports(def_id) {
for export in exports.iter() {
if export.vis.is_public() {
if let Some(def_id) = export.res.opt_def_id() {
if let Some(def_id) = def_id.as_local() {
self.update(def_id, Some(AccessLevel::Exported));
}
}
}
}
}
}

intravisit::walk_mod(self, m, id);
}
}

impl ReachEverythingInTheInterfaceVisitor<'_, '_> {
Expand Down Expand Up @@ -2166,11 +2024,12 @@ fn privacy_access_levels(tcx: TyCtxt<'_>, (): ()) -> &AccessLevels {
// items which are reachable from external crates based on visibility.
let mut visitor = EmbargoVisitor {
tcx,
access_levels: Default::default(),
access_levels: tcx.resolutions(()).access_levels.clone(),
macro_reachable: Default::default(),
prev_level: Some(AccessLevel::Public),
changed: false,
};

loop {
tcx.hir().walk_toplevel_module(&mut visitor);
if visitor.changed {
Expand All @@ -2179,7 +2038,6 @@ fn privacy_access_levels(tcx: TyCtxt<'_>, (): ()) -> &AccessLevels {
break;
}
}
visitor.update(CRATE_DEF_ID, Some(AccessLevel::Public));

tcx.arena.alloc(visitor.access_levels)
}
Expand Down
Loading