Privacy check paths in resolve and typeck

jseyfried · jseyfried · commit b20d567c2b9b · 2016-02-26T00:37:27.000Z
diff --git 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
@@ -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<Box<PrivacyError<'a>>>,
     },
 }
 
+#[derive(Clone, Debug)]
+struct PrivacyError<'a>(Span, Name, &'a NameBinding<'a>);
+
 impl<'a> NameBinding<'a> {
     fn create_from_module(module: Module<'a>, span: Option<Span>) -> 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<PrivacyError<'a>>,
 
     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
@@ -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<Box<PrivacyError<'a>>>) -> 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,17 +425,31 @@ 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) {
                     let msg = format!("`{}` is not directly importable", target);
                     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
@@ -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.
