Skip to content

Commit

Permalink
Rollup merge of #100147 - Bryanskiy:private-in-public, r=petrochenkov
Browse files Browse the repository at this point in the history 
optimization of access level table construction

Refactoring which was mentioned in #87487
  • Loading branch information
@GuillaumeGomez
GuillaumeGomez authored Sep 2, 2022
2 parents 0e82dc9 + 0111fb0 commit 1aaf9ae
Show file tree
Hide file tree
Showing 10 changed files with 326 additions and 125 deletions.
2 changes: 2 additions & 0 deletions compiler/rustc_error_messages/locales/en-US/privacy.ftl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ privacy_in_public_interface = {$vis_descr} {$kind} `{$descr}` in public interfac
.label = can't leak {$vis_descr} {$kind}
.visibility_label = `{$descr}` declared as {$vis_descr}
privacy_report_access_level = {$descr}
privacy_from_private_dep_in_public_interface =
{$kind} `{$descr}` from private dependency '{$krate}' in public interface
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_feature/src/builtin_attrs.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -758,6 +758,7 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
// Internal attributes, Testing:
// ==========================================================================

rustc_attr!(TEST, rustc_access_level, Normal, template!(Word), WarnFollowing),
rustc_attr!(TEST, rustc_outlives, Normal, template!(Word), WarnFollowing),
rustc_attr!(TEST, rustc_capture_analysis, Normal, template!(Word), WarnFollowing),
rustc_attr!(TEST, rustc_insignificant_dtor, Normal, template!(Word), WarnFollowing),
Expand Down
8 changes: 8 additions & 0 deletions compiler/rustc_privacy/src/errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,14 @@ pub struct InPublicInterface<'a> {
pub vis_span: Span,
}

#[derive(SessionDiagnostic)]
#[diag(privacy::report_access_level)]
pub struct ReportAccessLevel {
#[primary_span]
pub span: Span,
pub descr: String,
}

#[derive(LintDiagnostic)]
#[diag(privacy::from_private_dep_in_public_interface)]
pub struct FromPrivateDependencyInPublicInterface<'a> {
Expand Down
62 changes: 60 additions & 2 deletions compiler/rustc_privacy/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ use rustc_middle::ty::{self, Const, DefIdTree, GenericParamDefKind};
use rustc_middle::ty::{TraitRef, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitor};
use rustc_session::lint;
use rustc_span::hygiene::Transparency;
use rustc_span::symbol::{kw, Ident};
use rustc_span::symbol::{kw, sym, Ident};
use rustc_span::Span;

use std::marker::PhantomData;
Expand All @@ -42,7 +42,8 @@ use std::{cmp, fmt, mem};

use errors::{
FieldIsPrivate, FieldIsPrivateLabel, FromPrivateDependencyInPublicInterface, InPublicInterface,
InPublicInterfaceTraits, ItemIsPrivate, PrivateInPublicLint, UnnamedItemIsPrivate,
InPublicInterfaceTraits, ItemIsPrivate, PrivateInPublicLint, ReportAccessLevel,
UnnamedItemIsPrivate,
};

////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -907,6 +908,60 @@ impl<'tcx> DefIdVisitor<'tcx> for ReachEverythingInTheInterfaceVisitor<'_, 'tcx>
}
}

////////////////////////////////////////////////////////////////////////////////
/// Visitor, used for AccessLevels table checking
////////////////////////////////////////////////////////////////////////////////
pub struct TestReachabilityVisitor<'tcx, 'a> {
tcx: TyCtxt<'tcx>,
access_levels: &'a AccessLevels,
}

impl<'tcx, 'a> TestReachabilityVisitor<'tcx, 'a> {
fn access_level_diagnostic(&mut self, def_id: LocalDefId) {
if self.tcx.has_attr(def_id.to_def_id(), sym::rustc_access_level) {
let access_level = format!("{:?}", self.access_levels.map.get(&def_id));
let span = self.tcx.def_span(def_id.to_def_id());
self.tcx.sess.emit_err(ReportAccessLevel { span, descr: access_level });
}
}
}

impl<'tcx, 'a> Visitor<'tcx> for TestReachabilityVisitor<'tcx, 'a> {
fn visit_item(&mut self, item: &'tcx hir::Item<'tcx>) {
self.access_level_diagnostic(item.def_id);

match item.kind {
hir::ItemKind::Enum(ref def, _) => {
for variant in def.variants.iter() {
let variant_id = self.tcx.hir().local_def_id(variant.id);
self.access_level_diagnostic(variant_id);
for field in variant.data.fields() {
let def_id = self.tcx.hir().local_def_id(field.hir_id);
self.access_level_diagnostic(def_id);
}
}
}
hir::ItemKind::Struct(ref def, _) | hir::ItemKind::Union(ref def, _) => {
for field in def.fields() {
let def_id = self.tcx.hir().local_def_id(field.hir_id);
self.access_level_diagnostic(def_id);
}
}
_ => {}
}
}

fn visit_trait_item(&mut self, item: &'tcx hir::TraitItem<'tcx>) {
self.access_level_diagnostic(item.def_id);
}
fn visit_impl_item(&mut self, item: &'tcx hir::ImplItem<'tcx>) {
self.access_level_diagnostic(item.def_id);
}
fn visit_foreign_item(&mut self, item: &'tcx hir::ForeignItem<'tcx>) {
self.access_level_diagnostic(item.def_id);
}
}

//////////////////////////////////////////////////////////////////////////////////////
/// Name privacy visitor, checks privacy and reports violations.
/// Most of name privacy checks are performed during the main resolution phase,
Expand Down Expand Up @@ -2045,6 +2100,9 @@ fn privacy_access_levels(tcx: TyCtxt<'_>, (): ()) -> &AccessLevels {
}
}

let mut check_visitor = TestReachabilityVisitor { tcx, access_levels: &visitor.access_levels };
tcx.hir().visit_all_item_likes_in_crate(&mut check_visitor);

tcx.arena.alloc(visitor.access_levels)
}

Expand Down
158 changes: 53 additions & 105 deletions compiler/rustc_resolve/src/access_levels.rs
View file
Original file line number Diff line number Diff line change
@@ -1,25 +1,21 @@
use crate::imports::ImportKind;
use crate::NameBinding;
use crate::NameBindingKind;
use crate::Resolver;
use rustc_ast::ast;
use rustc_ast::visit;
use rustc_ast::visit::Visitor;
use rustc_ast::Crate;
use rustc_ast::EnumDef;
use rustc_ast::ForeignMod;
use rustc_ast::NodeId;
use rustc_hir::def_id::LocalDefId;
use rustc_hir::def_id::CRATE_DEF_ID;
use rustc_middle::middle::privacy::AccessLevel;
use rustc_middle::ty::Visibility;
use rustc_middle::ty::DefIdTree;
use rustc_span::sym;

use crate::imports::ImportKind;
use crate::BindingKey;
use crate::NameBinding;
use crate::NameBindingKind;
use crate::Resolver;

pub struct AccessLevelsVisitor<'r, 'a> {
r: &'r mut Resolver<'a>,
prev_level: Option<AccessLevel>,
changed: bool,
}

Expand All @@ -28,11 +24,10 @@ impl<'r, 'a> AccessLevelsVisitor<'r, 'a> {
/// For now, this doesn't resolve macros (FIXME) and cannot resolve Impl, as we
/// need access to a TyCtxt for that.
pub fn compute_access_levels<'c>(r: &'r mut Resolver<'a>, krate: &'c Crate) {
let mut visitor =
AccessLevelsVisitor { r, changed: false, prev_level: Some(AccessLevel::Public) };
let mut visitor = AccessLevelsVisitor { r, changed: false };

visitor.set_access_level_def_id(CRATE_DEF_ID, Some(AccessLevel::Public));
visitor.set_exports_access_level(CRATE_DEF_ID);
visitor.set_bindings_access_level(CRATE_DEF_ID);

while visitor.changed {
visitor.reset();
Expand All @@ -44,15 +39,17 @@ impl<'r, 'a> AccessLevelsVisitor<'r, 'a> {

fn reset(&mut self) {
self.changed = false;
self.prev_level = Some(AccessLevel::Public);
}

/// Update the access level of the exports of the given module accordingly. The module access
/// Update the access level of the bindings in the given module accordingly. The module access
/// level has to be Exported or Public.
/// This will also follow `use` chains (see PrivacyVisitor::set_import_binding_access_level).
fn set_exports_access_level(&mut self, module_id: LocalDefId) {
fn set_bindings_access_level(&mut self, module_id: LocalDefId) {
assert!(self.r.module_map.contains_key(&&module_id.to_def_id()));

let module_level = self.r.access_levels.map.get(&module_id).copied();
if !module_level.is_some() {
return;
}
// Set the given binding access level to `AccessLevel::Public` and
// sets the rest of the `use` chain to `AccessLevel::Exported` until
// we hit the actual exported item.
Expand All @@ -72,28 +69,20 @@ impl<'r, 'a> AccessLevelsVisitor<'r, 'a> {
}
};

let module_level = self.r.access_levels.map.get(&module_id).copied();
assert!(module_level >= Some(AccessLevel::Exported));

if let Some(exports) = self.r.reexport_map.get(&module_id) {
let pub_exports = exports
.iter()
.filter(|ex| ex.vis == Visibility::Public)
.cloned()
.collect::<Vec<_>>();

let module = self.r.get_module(module_id.to_def_id()).unwrap();
for export in pub_exports.into_iter() {
if let Some(export_def_id) = export.res.opt_def_id().and_then(|id| id.as_local()) {
self.set_access_level_def_id(export_def_id, Some(AccessLevel::Exported));
}

if let Some(ns) = export.res.ns() {
let key = BindingKey { ident: export.ident, ns, disambiguator: 0 };
let name_res = self.r.resolution(module, key);
if let Some(binding) = name_res.borrow().binding() {
set_import_binding_access_level(self, binding, module_level)
}
let module = self.r.get_module(module_id.to_def_id()).unwrap();
let resolutions = self.r.resolutions(module);

for (.., name_resolution) in resolutions.borrow().iter() {
if let Some(binding) = name_resolution.borrow().binding() && binding.vis.is_public() && !binding.is_ambiguity() {
let access_level = match binding.is_import() {
true => {
set_import_binding_access_level(self, binding, module_level);
Some(AccessLevel::Exported)
},
false => module_level,
};
if let Some(def_id) = binding.res().opt_def_id().and_then(|id| id.as_local()) {
self.set_access_level_def_id(def_id, access_level);
}
}
}
Expand Down Expand Up @@ -127,97 +116,59 @@ impl<'r, 'a> AccessLevelsVisitor<'r, 'a> {

impl<'r, 'ast> Visitor<'ast> for AccessLevelsVisitor<'ast, 'r> {
fn visit_item(&mut self, item: &'ast ast::Item) {
let inherited_item_level = match item.kind {
let def_id = self.r.local_def_id(item.id);
// Set access level of nested items.
// If it's a mod, also make the visitor walk all of its items
match item.kind {
// Resolved in rustc_privacy when types are available
ast::ItemKind::Impl(..) => return,

// Only exported `macro_rules!` items are public, but they always are
ast::ItemKind::MacroDef(ref macro_def) if macro_def.macro_rules => {
let is_macro_export =
item.attrs.iter().any(|attr| attr.has_name(sym::macro_export));
if is_macro_export { Some(AccessLevel::Public) } else { None }
}

// Foreign modules inherit level from parents.
ast::ItemKind::ForeignMod(..) => self.prev_level,

// Other `pub` items inherit levels from parents.
ast::ItemKind::ExternCrate(..)
| ast::ItemKind::Use(..)
| ast::ItemKind::Static(..)
| ast::ItemKind::Const(..)
| ast::ItemKind::Fn(..)
| ast::ItemKind::Mod(..)
| ast::ItemKind::GlobalAsm(..)
| ast::ItemKind::TyAlias(..)
| ast::ItemKind::Enum(..)
| ast::ItemKind::Struct(..)
| ast::ItemKind::Union(..)
| ast::ItemKind::Trait(..)
| ast::ItemKind::TraitAlias(..)
| ast::ItemKind::MacroDef(..) => {
if item.vis.kind.is_pub() {
self.prev_level
} else {
None
}
}

// Should be unreachable at this stage
ast::ItemKind::MacCall(..) => panic!(
"ast::ItemKind::MacCall encountered, this should not anymore appear at this stage"
),
};

let access_level = self.set_access_level(item.id, inherited_item_level);
// Foreign modules inherit level from parents.
ast::ItemKind::ForeignMod(..) => {
let parent_level =
self.r.access_levels.map.get(&self.r.local_parent(def_id)).copied();
self.set_access_level(item.id, parent_level);
}

// Set access level of nested items.
// If it's a mod, also make the visitor walk all of its items
match item.kind {
ast::ItemKind::Mod(..) => {
if access_level.is_some() {
self.set_exports_access_level(self.r.local_def_id(item.id));
// Only exported `macro_rules!` items are public, but they always are
ast::ItemKind::MacroDef(ref macro_def) if macro_def.macro_rules => {
if item.attrs.iter().any(|attr| attr.has_name(sym::macro_export)) {
self.set_access_level(item.id, Some(AccessLevel::Public));
}
}

let orig_level = std::mem::replace(&mut self.prev_level, access_level);
ast::ItemKind::Mod(..) => {
self.set_bindings_access_level(def_id);
visit::walk_item(self, item);
self.prev_level = orig_level;
}

ast::ItemKind::ForeignMod(ForeignMod { ref items, .. }) => {
for nested in items {
if nested.vis.kind.is_pub() {
self.set_access_level(nested.id, access_level);
}
}
}
ast::ItemKind::Enum(EnumDef { ref variants }, _) => {
self.set_bindings_access_level(def_id);
for variant in variants {
let variant_level = self.set_access_level(variant.id, access_level);
if let Some(ctor_id) = variant.data.ctor_id() {
self.set_access_level(ctor_id, access_level);
}

let variant_def_id = self.r.local_def_id(variant.id);
let variant_level = self.r.access_levels.map.get(&variant_def_id).copied();
for field in variant.data.fields() {
self.set_access_level(field.id, variant_level);
}
}
}
ast::ItemKind::Struct(ref def, _) | ast::ItemKind::Union(ref def, _) => {
if let Some(ctor_id) = def.ctor_id() {
self.set_access_level(ctor_id, access_level);
}

ast::ItemKind::Struct(ref def, _) | ast::ItemKind::Union(ref def, _) => {
let inherited_level = self.r.access_levels.map.get(&def_id).copied();
for field in def.fields() {
if field.vis.kind.is_pub() {
self.set_access_level(field.id, access_level);
self.set_access_level(field.id, inherited_level);
}
}
}
ast::ItemKind::Trait(ref trait_kind) => {
for nested in trait_kind.items.iter() {
self.set_access_level(nested.id, access_level);
}

ast::ItemKind::Trait(..) => {
self.set_bindings_access_level(def_id);
}

ast::ItemKind::ExternCrate(..)
Expand All @@ -229,9 +180,6 @@ impl<'r, 'ast> Visitor<'ast> for AccessLevelsVisitor<'ast, 'r> {
| ast::ItemKind::TraitAlias(..)
| ast::ItemKind::MacroDef(..)
| ast::ItemKind::Fn(..) => return,

// Unreachable kinds
ast::ItemKind::Impl(..) | ast::ItemKind::MacCall(..) => unreachable!(),
}
}
}
27 changes: 9 additions & 18 deletions compiler/rustc_resolve/src/imports.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1133,24 +1133,15 @@ impl<'a, 'b> ImportResolver<'a, 'b> {
if let Some(def_id) = module.opt_def_id() {
let mut reexports = Vec::new();

module.for_each_child(self.r, |_, ident, _, binding| {
// FIXME: Consider changing the binding inserted by `#[macro_export] macro_rules`
// into the crate root to actual `NameBindingKind::Import`.
if binding.is_import()
|| matches!(binding.kind, NameBindingKind::Res(_, _is_macro_export @ true))
{
let res = binding.res().expect_non_local();
// Ambiguous imports are treated as errors at this point and are
// not exposed to other crates (see #36837 for more details).
if res != def::Res::Err && !binding.is_ambiguity() {
reexports.push(ModChild {
ident,
res,
vis: binding.vis,
span: binding.span,
macro_rules: false,
});
}
module.for_each_child(self.r, |this, ident, _, binding| {
if let Some(res) = this.is_reexport(binding) {
reexports.push(ModChild {
ident,
res,
vis: binding.vis,
span: binding.span,
macro_rules: false,
});
}
});

Expand Down
Loading

0 comments on commit 1aaf9ae

Please sign in to comment.