From b20d567c2b9b279fff049f087b725b6c75126156 Mon Sep 17 00:00:00 2001 From: Jeffrey Seyfried Date: Thu, 25 Feb 2016 04:40:46 +0000 Subject: [PATCH] Privacy check paths in resolve and typeck --- src/librustc_resolve/build_reduced_graph.rs | 2 +- src/librustc_resolve/lib.rs | 66 +++++++++++++++++++-- src/librustc_resolve/resolve_imports.rs | 32 ++++++++-- src/librustc_typeck/check/method/mod.rs | 11 +++- 4 files changed, 98 insertions(+), 13 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index e59960ed4bd0..042b77e42c4e 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -499,7 +499,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { debug!("(building reduced graph for external crate) building external def {}, priv {:?}", final_ident, vis); - let is_public = vis == hir::Public; + let is_public = vis == hir::Public || new_parent.is_trait(); let mut modifiers = DefModifiers::empty(); if is_public { diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 47e4da53f334..a758008e807b 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -934,6 +934,15 @@ impl<'a> ModuleS<'a> { } } + fn is_ancestor_of(&self, module: Module<'a>) -> bool { + if self.def_id() == module.def_id() { return true } + match module.parent_link { + ParentLink::BlockParentLink(parent, _) | + ParentLink::ModuleParentLink(parent, _) => self.is_ancestor_of(parent), + _ => false, + } + } + pub fn inc_glob_count(&self) { self.glob_count.set(self.glob_count.get() + 1); } @@ -1000,9 +1009,14 @@ enum NameBindingKind<'a> { Import { binding: &'a NameBinding<'a>, id: NodeId, + // Some(error) if using this imported name causes the import to be a privacy error + privacy_error: Option>>, }, } +#[derive(Clone, Debug)] +struct PrivacyError<'a>(Span, Name, &'a NameBinding<'a>); + impl<'a> NameBinding<'a> { fn create_from_module(module: Module<'a>, span: Option) -> Self { let modifiers = if module.is_public { @@ -1145,6 +1159,7 @@ pub struct Resolver<'a, 'tcx: 'a> { // The intention is that the callback modifies this flag. // Once set, the resolver falls out of the walk, preserving the ribs. resolved: bool, + privacy_errors: Vec>, arenas: &'a ResolverArenas<'a>, } @@ -1209,6 +1224,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { callback: None, resolved: false, + privacy_errors: Vec::new(), arenas: arenas, } @@ -1255,12 +1271,15 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { self.used_crates.insert(krate); } - let import_id = match binding.kind { - NameBindingKind::Import { id, .. } => id, + let (import_id, privacy_error) = match binding.kind { + NameBindingKind::Import { id, ref privacy_error, .. } => (id, privacy_error), _ => return, }; self.used_imports.insert((import_id, ns)); + if let Some(error) = privacy_error.as_ref() { + self.privacy_errors.push((**error).clone()); + } if !self.make_glob_map { return; @@ -1352,6 +1371,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { // Check to see whether there are type bindings, and, if // so, whether there is a module within. if let Some(module_def) = binding.module() { + self.check_privacy(search_module, name, binding, span); search_module = module_def; } else { let msg = format!("Not a module `{}`", name); @@ -2911,7 +2931,10 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { let name = segments.last().unwrap().identifier.name; let result = self.resolve_name_in_module(containing_module, name, namespace, false, true); - result.success().map(|binding| binding.def().unwrap()) + result.success().map(|binding| { + self.check_privacy(containing_module, name, binding, span); + binding.def().unwrap() + }) } /// Invariant: This must be called only during main resolution, not during @@ -2958,7 +2981,10 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { let name = segments.last().unwrap().identifier.name; let result = self.resolve_name_in_module(containing_module, name, namespace, false, true); - result.success().map(|binding| binding.def().unwrap()) + result.success().map(|binding| { + self.check_privacy(containing_module, name, binding, span); + binding.def().unwrap() + }) } fn resolve_identifier_in_local_ribs(&mut self, @@ -3570,6 +3596,37 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { } } } + + fn is_visible(&self, binding: &'a NameBinding<'a>, parent: Module<'a>) -> bool { + binding.is_public() || parent.is_ancestor_of(self.current_module) + } + + fn check_privacy(&mut self, + module: Module<'a>, + name: Name, + binding: &'a NameBinding<'a>, + span: Span) { + if !self.is_visible(binding, module) { + self.privacy_errors.push(PrivacyError(span, name, binding)); + } + } + + fn report_privacy_errors(&self) { + if self.privacy_errors.len() == 0 { return } + let mut reported_spans = HashSet::new(); + for &PrivacyError(span, name, binding) in &self.privacy_errors { + if !reported_spans.insert(span) { continue } + if binding.is_extern_crate() { + // Warn when using an inaccessible extern crate. + let node_id = binding.module().unwrap().extern_crate_id.unwrap(); + let msg = format!("extern crate `{}` is private", name); + self.session.add_lint(lint::builtin::INACCESSIBLE_EXTERN_CRATE, node_id, span, msg); + } else { + let def = binding.def().unwrap(); + self.session.span_err(span, &format!("{} `{}` is private", def.kind_name(), name)); + } + } + } } @@ -3726,6 +3783,7 @@ pub fn resolve_crate<'a, 'tcx>(session: &'a Session, resolver.resolve_crate(krate); check_unused::check_crate(&mut resolver, krate); + resolver.report_privacy_errors(); CrateMap { def_map: resolver.def_map, diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 47f4fc82ad1c..f6d23c8caa2c 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -13,7 +13,7 @@ use self::ImportDirectiveSubclass::*; use DefModifiers; use Module; use Namespace::{self, TypeNS, ValueNS}; -use {NameBinding, NameBindingKind}; +use {NameBinding, NameBindingKind, PrivacyError}; use ResolveResult; use ResolveResult::*; use Resolver; @@ -78,7 +78,9 @@ impl ImportDirective { // Given the binding to which this directive resolves in a particular namespace, // this returns the binding for the name this directive defines in that namespace. - fn import<'a>(&self, binding: &'a NameBinding<'a>) -> NameBinding<'a> { + fn import<'a>(&self, + binding: &'a NameBinding<'a>, + privacy_error: Option>>) -> NameBinding<'a> { let mut modifiers = match self.is_public { true => DefModifiers::PUBLIC | DefModifiers::IMPORTABLE, false => DefModifiers::empty(), @@ -91,7 +93,11 @@ impl ImportDirective { } NameBinding { - kind: NameBindingKind::Import { binding: binding, id: self.id }, + kind: NameBindingKind::Import { + binding: binding, + id: self.id, + privacy_error: privacy_error, + }, span: Some(self.span), modifiers: modifiers, } @@ -219,7 +225,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { span: None, }); let dummy_binding = - self.resolver.new_name_binding(e.import_directive.import(dummy_binding)); + self.resolver.new_name_binding(e.import_directive.import(dummy_binding, None)); let _ = e.source_module.try_define_child(target, ValueNS, dummy_binding); let _ = e.source_module.try_define_child(target, TypeNS, dummy_binding); @@ -419,6 +425,8 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { _ => {} } + let mut privacy_error = None; + let mut report_privacy_error = true; for &(ns, result) in &[(ValueNS, &value_result), (TypeNS, &type_result)] { if let Success(binding) = *result { if !binding.defined_with(DefModifiers::IMPORTABLE) { @@ -426,10 +434,22 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { span_err!(self.resolver.session, directive.span, E0253, "{}", &msg); } - self.define(module_, target, ns, directive.import(binding)); + privacy_error = if !self.resolver.is_visible(binding, target_module) { + Some(Box::new(PrivacyError(directive.span, source, binding))) + } else { + report_privacy_error = false; + None + }; + + self.define(module_, target, ns, directive.import(binding, privacy_error.clone())); } } + if report_privacy_error { // then all successful namespaces are privacy errors + // We report here so there is an error even if the imported name is not used + self.resolver.privacy_errors.push(*privacy_error.unwrap()); + } + // Record what this import resolves to for later uses in documentation, // this may resolve to either a value or a type, but for documentation // purposes it's good enough to just favor one over the other. @@ -472,7 +492,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> { build_reduced_graph::populate_module_if_necessary(self.resolver, target_module); target_module.for_each_child(|name, ns, binding| { if !binding.defined_with(DefModifiers::IMPORTABLE | DefModifiers::PUBLIC) { return } - self.define(module_, name, ns, directive.import(binding)); + self.define(module_, name, ns, directive.import(binding, None)); if ns == TypeNS && directive.is_public && binding.defined_with(DefModifiers::PRIVATE_VARIANT) { diff --git a/src/librustc_typeck/check/method/mod.rs b/src/librustc_typeck/check/method/mod.rs index 74ee1579229e..31e233447348 100644 --- a/src/librustc_typeck/check/method/mod.rs +++ b/src/librustc_typeck/check/method/mod.rs @@ -337,9 +337,16 @@ pub fn resolve_ufcs<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>, { let mode = probe::Mode::Path; let pick = try!(probe::probe(fcx, span, mode, method_name, self_ty, expr_id)); - Ok(pick.item.def()) -} + let def = pick.item.def(); + if let probe::InherentImplPick = pick.kind { + if pick.item.vis() != hir::Public && !fcx.private_item_is_visible(def.def_id()) { + let msg = format!("{} `{}` is private", def.kind_name(), &method_name.as_str()); + fcx.tcx().sess.span_err(span, &msg); + } + } + Ok(def) +} /// Find item with name `item_name` defined in `trait_def_id` /// and return it, or `None`, if no such item.