Auto merge of #31824 - jseyfried:privacy_in_resolve, r=nikomatsakis

This PR privacy checks paths as they are resolved instead of in `librustc_privacy` (fixes #12334 and fixes #31779). This removes the need for the `LastPrivate` system introduced in PR #9735, the limitations of which cause #31779.

This PR also reports privacy violations in paths to intra- and inter-crate items the same way -- it always reports the first inaccessible segment of the path.

Since it fixes #31779, this is a [breaking-change]. For example, the following code would break:
```rust
mod foo {
    pub use foo::bar::S;
    mod bar { // `bar` should be private to `foo`
        pub struct S;
    }
}

impl foo::S {
    fn f() {}
}

fn main() {
    foo::bar::S::f(); // This is now a privacy error
}
```

r? @alexcrichton

src/librustc/front/map/mod.rs

@@ -495,6 +495,30 @@ impl<'ast> Map<'ast> {
         }
     }
 
+    /// Returns the NodeId of `id`'s nearest module parent, or `id` itself if no
+    /// module parent is in this map.
+    fn get_module_parent(&self, id: NodeId) -> NodeId {
+        match self.walk_parent_nodes(id, |node| match *node {
+            NodeItem(&Item { node: Item_::ItemMod(_), .. }) => true,
+            _ => false,
+        }) {
+            Ok(id) => id,
+            Err(id) => id,
+        }
+    }
+
+    pub fn private_item_is_visible_from(&self, item: NodeId, block: NodeId) -> bool {
+        // A private item is visible from everything in its nearest module parent.
+        let visibility = self.get_module_parent(item);
+        let mut block_ancestor = self.get_module_parent(block);
+        loop {
+            if block_ancestor == visibility { return true }
+            let block_ancestor_parent = self.get_module_parent(block_ancestor);
+            if block_ancestor_parent == block_ancestor { return false }
+            block_ancestor = block_ancestor_parent;
+        }
+    }
+
     /// Returns the nearest enclosing scope. A scope is an item or block.
     /// FIXME it is not clear to me that all items qualify as scopes - statics
     /// and associated types probably shouldn't, for example. Behaviour in this

src/librustc/middle/def.rs

@@ -9,7 +9,6 @@
 // except according to those terms.
 
 use middle::def_id::DefId;
-use middle::privacy::LastPrivate;
 use middle::subst::ParamSpace;
 use util::nodemap::NodeMap;
 use syntax::ast;
@@ -65,7 +64,6 @@ pub enum Def {
 #[derive(Copy, Clone, Debug)]
 pub struct PathResolution {
     pub base_def: Def,
-    pub last_private: LastPrivate,
     pub depth: usize
 }
 
@@ -84,12 +82,10 @@ impl PathResolution {
     }
 
     pub fn new(base_def: Def,
-               last_private: LastPrivate,
                depth: usize)
                -> PathResolution {
         PathResolution {
             base_def: base_def,
-            last_private: last_private,
             depth: depth,
         }
     }
@@ -152,4 +148,29 @@ impl Def {
             _ => None
         }
     }
+
+    pub fn kind_name(&self) -> &'static str {
+        match *self {
+            Def::Fn(..) => "function",
+            Def::Mod(..) => "module",
+            Def::ForeignMod(..) => "foreign module",
+            Def::Static(..) => "static",
+            Def::Variant(..) => "variant",
+            Def::Enum(..) => "enum",
+            Def::TyAlias(..) => "type",
+            Def::AssociatedTy(..) => "associated type",
+            Def::Struct(..) => "struct",
+            Def::Trait(..) => "trait",
+            Def::Method(..) => "method",
+            Def::Const(..) => "const",
+            Def::AssociatedConst(..) => "associated const",
+            Def::TyParam(..) => "type parameter",
+            Def::PrimTy(..) => "builtin type",
+            Def::Local(..) => "local variable",
+            Def::Upvar(..) => "closure capture",
+            Def::Label(..) => "label",
+            Def::SelfTy(..) => "self type",
+            Def::Err => "unresolved item",
+        }
+    }
 }

src/librustc/middle/privacy.rs

@@ -12,11 +12,6 @@
 //! outside their scopes. This pass will also generate a set of exported items
 //! which are available for use externally when compiled as a library.
 
-pub use self::PrivateDep::*;
-pub use self::ImportUse::*;
-pub use self::LastPrivate::*;
-
-use middle::def_id::DefId;
 use util::nodemap::{DefIdSet, FnvHashMap};
 
 use std::hash::Hash;
@@ -64,39 +59,3 @@ impl<Id: Hash + Eq> Default for AccessLevels<Id> {
 /// A set containing all exported definitions from external crates.
 /// The set does not contain any entries from local crates.
 pub type ExternalExports = DefIdSet;
-
-#[derive(Copy, Clone, Debug)]
-pub enum LastPrivate {
-    LastMod(PrivateDep),
-    // `use` directives (imports) can refer to two separate definitions in the
-    // type and value namespaces. We record here the last private node for each
-    // and whether the import is in fact used for each.
-    // If the Option<PrivateDep> fields are None, it means there is no definition
-    // in that namespace.
-    LastImport{value_priv: Option<PrivateDep>,
-               value_used: ImportUse,
-               type_priv: Option<PrivateDep>,
-               type_used: ImportUse},
-}
-
-#[derive(Copy, Clone, Debug)]
-pub enum PrivateDep {
-    AllPublic,
-    DependsOn(DefId),
-}
-
-// How an import is used.
-#[derive(Copy, Clone, PartialEq, Debug)]
-pub enum ImportUse {
-    Unused,       // The import is not used.
-    Used,         // The import is used.
-}
-
-impl LastPrivate {
-    pub fn or(self, other: LastPrivate) -> LastPrivate {
-        match (self, other) {
-            (me, LastMod(AllPublic)) => me,
-            (_, other) => other,
-        }
-    }
-}

src/librustc/middle/ty/mod.rs

@@ -173,6 +173,14 @@ impl<'tcx> ImplOrTraitItem<'tcx> {
         }
     }
 
+    pub fn def(&self) -> Def {
+        match *self {
+            ConstTraitItem(ref associated_const) => Def::AssociatedConst(associated_const.def_id),
+            MethodTraitItem(ref method) => Def::Method(method.def_id),
+            TypeTraitItem(ref ty) => Def::AssociatedTy(ty.container.id(), ty.def_id),
+        }
+    }
+
     pub fn def_id(&self) -> DefId {
         match *self {
             ConstTraitItem(ref associated_const) => associated_const.def_id,

src/librustc_metadata/astencode.rs

@@ -32,7 +32,6 @@ use middle::ty::cast;
 use middle::const_qualif::ConstQualif;
 use middle::def::{self, Def};
 use middle::def_id::DefId;
-use middle::privacy::{AllPublic, LastMod};
 use middle::region;
 use middle::subst;
 use middle::ty::{self, Ty};
@@ -254,7 +253,7 @@ trait def_id_encoder_helpers {
 }
 
 impl<S:serialize::Encoder> def_id_encoder_helpers for S
-    where <S as serialize::serialize::Encoder>::Error: Debug
+    where <S as serialize::Encoder>::Error: Debug
 {
     fn emit_def_id(&mut self, did: DefId) {
         did.encode(self).unwrap()
@@ -268,7 +267,7 @@ trait def_id_decoder_helpers {
 }
 
 impl<D:serialize::Decoder> def_id_decoder_helpers for D
-    where <D as serialize::serialize::Decoder>::Error: Debug
+    where <D as serialize::Decoder>::Error: Debug
 {
     fn read_def_id(&mut self, dcx: &DecodeContext) -> DefId {
         let did: DefId = Decodable::decode(self).unwrap();
@@ -1161,8 +1160,6 @@ fn decode_side_tables(dcx: &DecodeContext,
                         let def = decode_def(dcx, val_dsr);
                         dcx.tcx.def_map.borrow_mut().insert(id, def::PathResolution {
                             base_def: def,
-                            // This doesn't matter cross-crate.
-                            last_private: LastMod(AllPublic),
                             depth: 0
                         });
                     }

src/librustc_privacy/lib.rs

@@ -41,9 +41,6 @@ use rustc::lint;
 use rustc::middle::def::{self, Def};
 use rustc::middle::def_id::DefId;
 use rustc::middle::privacy::{AccessLevel, AccessLevels};
-use rustc::middle::privacy::ImportUse::*;
-use rustc::middle::privacy::LastPrivate::*;
-use rustc::middle::privacy::PrivateDep::*;
 use rustc::middle::privacy::ExternalExports;
 use rustc::middle::ty;
 use rustc::util::nodemap::{NodeMap, NodeSet};
@@ -692,32 +689,7 @@ impl<'a, 'tcx> PrivacyVisitor<'a, 'tcx> {
     /// whether the node is accessible by the current module that iteration is
     /// inside.
     fn private_accessible(&self, id: ast::NodeId) -> bool {
-        let parent = *self.parents.get(&id).unwrap();
+        self.tcx.map.private_item_is_visible_from(id, self.curitem)
-        debug!("privacy - accessible parent {}", self.nodestr(parent));
-
-        // After finding `did`'s closest private member, we roll ourselves back
-        // to see if this private member's parent is anywhere in our ancestry.
-        // By the privacy rules, we can access all of our ancestor's private
-        // members, so that's why we test the parent, and not the did itself.
-        let mut cur = self.curitem;
-        loop {
-            debug!("privacy - questioning {}, {}", self.nodestr(cur), cur);
-            match cur {
-                // If the relevant parent is in our history, then we're allowed
-                // to look inside any of our ancestor's immediate private items,
-                // so this access is valid.
-                x if x == parent => return true,
-
-                // If we've reached the root, then we couldn't access this item
-                // in the first place
-                ast::DUMMY_NODE_ID => return false,
-
-                // Keep going up
-                _ => {}
-            }
-
-            cur = *self.parents.get(&cur).unwrap();
-        }
     }
 
     fn report_error(&self, result: CheckResult) -> bool {
@@ -743,7 +715,6 @@ impl<'a, 'tcx> PrivacyVisitor<'a, 'tcx> {
                      source_did: Option<DefId>,
                      msg: &str)
                      -> CheckResult {
-        use rustc_front::hir::Item_::ItemExternCrate;
         debug!("ensure_public(span={:?}, to_check={:?}, source_did={:?}, msg={:?})",
                span, to_check, source_did, msg);
         let def_privacy = self.def_privacy(to_check);
@@ -765,20 +736,6 @@ impl<'a, 'tcx> PrivacyVisitor<'a, 'tcx> {
         let def_id = source_did.unwrap_or(to_check);
         let node_id = self.tcx.map.as_local_node_id(def_id);
 
-        // Warn when using a inaccessible extern crate.
-        if let Some(node_id) = self.tcx.map.as_local_node_id(to_check) {
-            match self.tcx.map.get(node_id) {
-                ast_map::Node::NodeItem(&hir::Item { node: ItemExternCrate(_), name, .. }) => {
-                    self.tcx.sess.add_lint(lint::builtin::INACCESSIBLE_EXTERN_CRATE,
-                                           node_id,
-                                           span,
-                                           format!("extern crate `{}` is private", name));
-                    return None;
-                }
-                _ => {}
-            }
-        }
-
         let (err_span, err_msg) = if Some(id) == node_id {
             return Some((span, format!("{} is private", msg), None));
         } else {
@@ -835,7 +792,7 @@ impl<'a, 'tcx> PrivacyVisitor<'a, 'tcx> {
             }
             UnnamedField(idx) => &v.fields[idx]
         };
-        if field.vis == hir::Public || self.local_private_accessible(field.did) {
+        if field.vis == hir::Public || self.local_private_accessible(def.did) {
             return;
         }
 
@@ -867,100 +824,6 @@ impl<'a, 'tcx> PrivacyVisitor<'a, 'tcx> {
                                                      name)));
     }
 
-    // Checks that a path is in scope.
-    fn check_path(&mut self, span: Span, path_id: ast::NodeId, last: ast::Name) {
-        debug!("privacy - path {}", self.nodestr(path_id));
-        let path_res = *self.tcx.def_map.borrow().get(&path_id).unwrap();
-        let ck = |tyname: &str| {
-            let ck_public = |def: DefId| {
-                debug!("privacy - ck_public {:?}", def);
-                let origdid = path_res.def_id();
-                self.ensure_public(span,
-                                   def,
-                                   Some(origdid),
-                                   &format!("{} `{}`", tyname, last))
-            };
-
-            match path_res.last_private {
-                LastMod(AllPublic) => {},
-                LastMod(DependsOn(def)) => {
-                    self.report_error(ck_public(def));
-                },
-                LastImport { value_priv,
-                             value_used: check_value,
-                             type_priv,
-                             type_used: check_type } => {
-                    // This dance with found_error is because we don't want to
-                    // report a privacy error twice for the same directive.
-                    let found_error = match (type_priv, check_type) {
-                        (Some(DependsOn(def)), Used) => {
-                            !self.report_error(ck_public(def))
-                        },
-                        _ => false,
-                    };
-                    if !found_error {
-                        match (value_priv, check_value) {
-                            (Some(DependsOn(def)), Used) => {
-                                self.report_error(ck_public(def));
-                            },
-                            _ => {},
-                        }
-                    }
-                    // If an import is not used in either namespace, we still
-                    // want to check that it could be legal. Therefore we check
-                    // in both namespaces and only report an error if both would
-                    // be illegal. We only report one error, even if it is
-                    // illegal to import from both namespaces.
-                    match (value_priv, check_value, type_priv, check_type) {
-                        (Some(p), Unused, None, _) |
-                        (None, _, Some(p), Unused) => {
-                            let p = match p {
-                                AllPublic => None,
-                                DependsOn(def) => ck_public(def),
-                            };
-                            if p.is_some() {
-                                self.report_error(p);
-                            }
-                        },
-                        (Some(v), Unused, Some(t), Unused) => {
-                            let v = match v {
-                                AllPublic => None,
-                                DependsOn(def) => ck_public(def),
-                            };
-                            let t = match t {
-                                AllPublic => None,
-                                DependsOn(def) => ck_public(def),
-                            };
-                            if let (Some(_), Some(t)) = (v, t) {
-                                self.report_error(Some(t));
-                            }
-                        },
-                        _ => {},
-                    }
-                },
-            }
-        };
-        // FIXME(#12334) Imports can refer to definitions in both the type and
-        // value namespaces. The privacy information is aware of this, but the
-        // def map is not. Therefore the names we work out below will not always
-        // be accurate and we can get slightly wonky error messages (but type
-        // checking is always correct).
-        match path_res.full_def() {
-            Def::Fn(..) => ck("function"),
-            Def::Static(..) => ck("static"),
-            Def::Const(..) => ck("const"),
-            Def::AssociatedConst(..) => ck("associated const"),
-            Def::Variant(..) => ck("variant"),
-            Def::TyAlias(..) => ck("type"),
-            Def::Enum(..) => ck("enum"),
-            Def::Trait(..) => ck("trait"),
-            Def::Struct(..) => ck("struct"),
-            Def::Method(..) => ck("method"),
-            Def::Mod(..) => ck("module"),
-            _ => {}
-        }
-    }
-
     // Checks that a method is in scope.
     fn check_method(&mut self, span: Span, method_def_id: DefId,
                     name: ast::Name) {
@@ -1036,7 +899,7 @@ impl<'a, 'tcx, 'v> Visitor<'v> for PrivacyVisitor<'a, 'tcx> {
                         _ => expr_ty
                     }.ty_adt_def().unwrap();
                     let any_priv = def.struct_variant().fields.iter().any(|f| {
-                        f.vis != hir::Public && !self.local_private_accessible(f.did)
+                        f.vis != hir::Public && !self.local_private_accessible(def.did)
                     });
                     if any_priv {
                         span_err!(self.tcx.sess, expr.span, E0450,
@@ -1102,25 +965,6 @@ impl<'a, 'tcx, 'v> Visitor<'v> for PrivacyVisitor<'a, 'tcx> {
         intravisit::walk_foreign_item(self, fi);
         self.in_foreign = false;
     }
-
-    fn visit_path(&mut self, path: &hir::Path, id: ast::NodeId) {
-        if !path.segments.is_empty() {
-            self.check_path(path.span, id, path.segments.last().unwrap().identifier.name);
-            intravisit::walk_path(self, path);
-        }
-    }
-
-    fn visit_path_list_item(&mut self, prefix: &hir::Path, item: &hir::PathListItem) {
-        let name = if let hir::PathListIdent { name, .. } = item.node {
-            name
-        } else if !prefix.segments.is_empty() {
-            prefix.segments.last().unwrap().identifier.name
-        } else {
-            self.tcx.sess.bug("`self` import in an import list with empty prefix");
-        };
-        self.check_path(item.span, item.node.id(), name);
-        intravisit::walk_path_list_item(self, prefix, item);
-    }
 }
 
 ////////////////////////////////////////////////////////////////////////////////

src/librustc_resolve/build_reduced_graph.rs

@@ -291,10 +291,8 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
                     self.external_exports.insert(def_id);
                     let parent_link = ModuleParentLink(parent, name);
                     let def = Def::Mod(def_id);
-                    let local_def_id = self.ast_map.local_def_id(item.id);
+                    let module = self.new_extern_crate_module(parent_link, def, is_public, item.id);
-                    let external_module =
+                    self.define(parent, name, TypeNS, (module, sp));
-                        self.new_extern_crate_module(parent_link, def, is_public, local_def_id);
-                    self.define(parent, name, TypeNS, (external_module, sp));
 
                     if is_public {
                         let export = Export { name: name, def_id: def_id };
@@ -304,7 +302,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
                         }
                     }
 
-                    self.build_reduced_graph_for_external_crate(external_module);
+                    self.build_reduced_graph_for_external_crate(module);
                 }
                 parent
             }
@@ -494,7 +492,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 {

src/librustc_resolve/check_unused.rs

@@ -23,7 +23,6 @@ use Resolver;
 use Namespace::{TypeNS, ValueNS};
 
 use rustc::lint;
-use rustc::middle::privacy::{DependsOn, LastImport, Used, Unused};
 use syntax::ast;
 use syntax::codemap::{Span, DUMMY_SP};
 
@@ -69,45 +68,6 @@ impl<'a, 'b, 'tcx> UnusedImportCheckVisitor<'a, 'b, 'tcx> {
                                   span,
                                   "unused import".to_string());
         }
-
-        let mut def_map = self.def_map.borrow_mut();
-        let path_res = if let Some(r) = def_map.get_mut(&id) {
-            r
-        } else {
-            return;
-        };
-        let (v_priv, t_priv) = match path_res.last_private {
-            LastImport { value_priv, type_priv, .. } => (value_priv, type_priv),
-            _ => {
-                panic!("we should only have LastImport for `use` directives")
-            }
-        };
-
-        let mut v_used = if self.used_imports.contains(&(id, ValueNS)) {
-            Used
-        } else {
-            Unused
-        };
-        let t_used = if self.used_imports.contains(&(id, TypeNS)) {
-            Used
-        } else {
-            Unused
-        };
-
-        match (v_priv, t_priv) {
-            // Since some items may be both in the value _and_ type namespaces (e.g., structs)
-            // we might have two LastPrivates pointing at the same thing. There is no point
-            // checking both, so lets not check the value one.
-            (Some(DependsOn(def_v)), Some(DependsOn(def_t))) if def_v == def_t => v_used = Unused,
-            _ => {}
-        }
-
-        path_res.last_private = LastImport {
-            value_priv: v_priv,
-            value_used: v_used,
-            type_priv: t_priv,
-            type_used: t_used,
-        };
     }
 }
 

src/librustc_resolve/lib.rs

@@ -54,7 +54,7 @@ use rustc::middle::cstore::{CrateStore, DefLike, DlDef};
 use rustc::middle::def::*;
 use rustc::middle::def_id::DefId;
 use rustc::middle::pat_util::pat_bindings;
-use rustc::middle::privacy::*;
+use rustc::middle::privacy::ExternalExports;
 use rustc::middle::subst::{ParamSpace, FnSpace, TypeSpace};
 use rustc::middle::ty::{Freevar, FreevarMap, TraitMap, GlobMap};
 use rustc::util::nodemap::{NodeMap, DefIdSet, FnvHashMap};
@@ -757,8 +757,8 @@ enum AssocItemResolveResult {
 
 #[derive(Copy, Clone)]
 enum BareIdentifierPatternResolution {
-    FoundStructOrEnumVariant(Def, LastPrivate),
+    FoundStructOrEnumVariant(Def),
-    FoundConst(Def, LastPrivate, Name),
+    FoundConst(Def, Name),
     BareIdentifierPatternUnresolved,
 }
 
@@ -807,9 +807,9 @@ pub struct ModuleS<'a> {
     def: Option<Def>,
     is_public: bool,
 
-    // If the module is an extern crate, `def` is root of the external crate and `extern_crate_did`
+    // If the module is an extern crate, `def` is root of the external crate and `extern_crate_id`
-    // is the DefId of the local `extern crate` item (otherwise, `extern_crate_did` is None).
+    // is the NodeId of the local `extern crate` item (otherwise, `extern_crate_id` is None).
-    extern_crate_did: Option<DefId>,
+    extern_crate_id: Option<NodeId>,
 
     resolutions: RefCell<HashMap<(Name, Namespace), NameResolution<'a>>>,
     unresolved_imports: RefCell<Vec<ImportDirective>>,
@@ -856,7 +856,7 @@ impl<'a> ModuleS<'a> {
             parent_link: parent_link,
             def: def,
             is_public: is_public,
-            extern_crate_did: None,
+            extern_crate_id: None,
             resolutions: RefCell::new(HashMap::new()),
             unresolved_imports: RefCell::new(Vec::new()),
             module_children: RefCell::new(NodeMap()),
@@ -920,16 +920,6 @@ impl<'a> ModuleS<'a> {
         self.def.as_ref().map(Def::def_id)
     }
 
-    // This returns the DefId of the crate local item that controls this module's visibility.
-    // It is only used to compute `LastPrivate` data, and it differs from `def_id` only for extern
-    // crates, whose `def_id` is the external crate's root, not the local `extern crate` item.
-    fn local_def_id(&self) -> Option<DefId> {
-        match self.extern_crate_did {
-            Some(def_id) => Some(def_id),
-            None => self.def_id(),
-        }
-    }
-
     fn is_normal(&self) -> bool {
         match self.def {
             Some(Def::Mod(_)) | Some(Def::ForeignMod(_)) => true,
@@ -944,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);
     }
@@ -1010,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 {
@@ -1040,14 +1044,6 @@ impl<'a> NameBinding<'a> {
         }
     }
 
-    fn local_def_id(&self) -> Option<DefId> {
-        match self.kind {
-            NameBindingKind::Def(def) => Some(def.def_id()),
-            NameBindingKind::Module(ref module) => module.local_def_id(),
-            NameBindingKind::Import { binding, .. } => binding.local_def_id(),
-        }
-    }
-
     fn defined_with(&self, modifiers: DefModifiers) -> bool {
         self.modifiers.contains(modifiers)
     }
@@ -1056,15 +1052,8 @@ impl<'a> NameBinding<'a> {
         self.defined_with(DefModifiers::PUBLIC)
     }
 
-    fn def_and_lp(&self) -> (Def, LastPrivate) {
-        let def = self.def().unwrap();
-        if let Def::Err = def { return (def, LastMod(AllPublic)) }
-        let lp = if self.is_public() { AllPublic } else { DependsOn(self.local_def_id().unwrap()) };
-        (def, LastMod(lp))
-    }
-
     fn is_extern_crate(&self) -> bool {
-        self.module().and_then(|module| module.extern_crate_did).is_some()
+        self.module().and_then(|module| module.extern_crate_id).is_some()
     }
 
     fn is_import(&self) -> bool {
@@ -1170,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>,
 }
@@ -1234,6 +1224,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
 
             callback: None,
             resolved: false,
+            privacy_errors: Vec::new(),
 
             arenas: arenas,
         }
@@ -1262,10 +1253,10 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                                parent_link: ParentLink<'a>,
                                def: Def,
                                is_public: bool,
-                               local_def: DefId)
+                               local_node_id: NodeId)
                                -> Module<'a> {
         let mut module = ModuleS::new(parent_link, Some(def), false, is_public);
-        module.extern_crate_did = Some(local_def);
+        module.extern_crate_id = Some(local_node_id);
         self.arenas.modules.alloc(module)
     }
 
@@ -1280,12 +1271,15 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             self.used_crates.insert(krate);
         }
 
-        let import_id = match binding.kind {
+        let (import_id, privacy_error) = match binding.kind {
-            NameBindingKind::Import { id, .. } => id,
+            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;
@@ -1313,9 +1307,8 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                                      module_: Module<'a>,
                                      module_path: &[Name],
                                      index: usize,
-                                     span: Span,
+                                     span: Span)
-                                     lp: LastPrivate)
+                                     -> ResolveResult<Module<'a>> {
-                                     -> ResolveResult<(Module<'a>, LastPrivate)> {
         fn search_parent_externals(needle: Name, module: Module) -> Option<Module> {
             match module.resolve_name(needle, TypeNS, false) {
                 Success(binding) if binding.is_extern_crate() => Some(module),
@@ -1331,7 +1324,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
         let mut search_module = module_;
         let mut index = index;
         let module_path_len = module_path.len();
-        let mut closest_private = lp;
 
         // Resolve the module part of the path. This does not involve looking
         // upward though scope chains; we simply resolve names directly in
@@ -1379,15 +1371,8 @@ 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;
-
-                        // Keep track of the closest private module used
-                        // when resolving this import chain.
-                        if !binding.is_public() {
-                            if let Some(did) = search_module.local_def_id() {
-                                closest_private = LastMod(DependsOn(did));
-                            }
-                        }
                     } else {
                         let msg = format!("Not a module `{}`", name);
                         return Failed(Some((span, msg)));
@@ -1398,7 +1383,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             index += 1;
         }
 
-        return Success((search_module, closest_private));
+        return Success(search_module);
     }
 
     /// Attempts to resolve the module part of an import directive or path
@@ -1411,9 +1396,9 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                            module_path: &[Name],
                            use_lexical_scope: UseLexicalScopeFlag,
                            span: Span)
-                           -> ResolveResult<(Module<'a>, LastPrivate)> {
+                           -> ResolveResult<Module<'a>> {
         if module_path.len() == 0 {
-            return Success((self.graph_root, LastMod(AllPublic))) // Use the crate root
+            return Success(self.graph_root) // Use the crate root
         }
 
         debug!("(resolving module path for import) processing `{}` rooted at `{}`",
@@ -1425,7 +1410,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
 
         let search_module;
         let start_index;
-        let last_private;
         match module_prefix_result {
             Failed(None) => {
                 let mpath = names_to_string(module_path);
@@ -1459,7 +1443,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                         // resolution process at index zero.
                         search_module = self.graph_root;
                         start_index = 0;
-                        last_private = LastMod(AllPublic);
                     }
                     UseLexicalScope => {
                         // This is not a crate-relative path. We resolve the
@@ -1478,7 +1461,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                                 Some(containing_module) => {
                                     search_module = containing_module;
                                     start_index = 1;
-                                    last_private = LastMod(AllPublic);
                                 }
                                 None => return Failed(None),
                             }
@@ -1489,16 +1471,13 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             Success(PrefixFound(ref containing_module, index)) => {
                 search_module = containing_module;
                 start_index = index;
-                last_private = LastMod(DependsOn(containing_module.local_def_id()
-                                                                  .unwrap()));
             }
         }
 
         self.resolve_module_path_from_root(search_module,
                                            module_path,
                                            start_index,
-                                           span,
+                                           span)
-                                           last_private)
     }
 
     /// Invariant: This must only be called during main resolution, not during
@@ -1851,8 +1830,8 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                             match self.resolve_crate_relative_path(prefix.span,
                                                                    &prefix.segments,
                                                                    TypeNS) {
-                                Some((def, lp)) =>
+                                Some(def) =>
-                                    self.record_def(item.id, PathResolution::new(def, lp, 0)),
+                                    self.record_def(item.id, PathResolution::new(def, 0)),
                                 None => {
                                     resolve_error(self,
                                                   prefix.span,
@@ -2406,7 +2385,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
 
                     match self.resolve_bare_identifier_pattern(ident.unhygienic_name,
                                                                pattern.span) {
-                        FoundStructOrEnumVariant(def, lp) if const_ok => {
+                        FoundStructOrEnumVariant(def) if const_ok => {
                             debug!("(resolving pattern) resolving `{}` to struct or enum variant",
                                    renamed);
 
@@ -2416,7 +2395,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                             self.record_def(pattern.id,
                                             PathResolution {
                                                 base_def: def,
-                                                last_private: lp,
                                                 depth: 0,
                                             });
                         }
@@ -2429,18 +2407,17 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                             );
                             self.record_def(pattern.id, err_path_resolution());
                         }
-                        FoundConst(def, lp, _) if const_ok => {
+                        FoundConst(def, _) if const_ok => {
                             debug!("(resolving pattern) resolving `{}` to constant", renamed);
 
                             self.enforce_default_binding_mode(pattern, binding_mode, "a constant");
                             self.record_def(pattern.id,
                                             PathResolution {
                                                 base_def: def,
-                                                last_private: lp,
                                                 depth: 0,
                                             });
                         }
-                        FoundConst(def, _, name) => {
+                        FoundConst(def, name) => {
                             resolve_error(
                                 self,
                                 pattern.span,
@@ -2462,7 +2439,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                             self.record_def(pattern.id,
                                             PathResolution {
                                                 base_def: def,
-                                                last_private: LastMod(AllPublic),
                                                 depth: 0,
                                             });
 
@@ -2680,10 +2656,10 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                     // considered as not having a private component because
                     // the lookup happened only within the current module.
                     Some(def @ Def::Variant(..)) | Some(def @ Def::Struct(..)) => {
-                        return FoundStructOrEnumVariant(def, LastMod(AllPublic));
+                        return FoundStructOrEnumVariant(def);
                     }
                     Some(def @ Def::Const(..)) | Some(def @ Def::AssociatedConst(..)) => {
-                        return FoundConst(def, LastMod(AllPublic), name);
+                        return FoundConst(def, name);
                     }
                     Some(Def::Static(..)) => {
                         resolve_error(self, span, ResolutionError::StaticVariableReference);
@@ -2764,7 +2740,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
         let span = path.span;
         let segments = &path.segments[..path.segments.len() - path_depth];
 
-        let mk_res = |(def, lp)| PathResolution::new(def, lp, path_depth);
+        let mk_res = |def| PathResolution::new(def, path_depth);
 
         if path.global {
             let def = self.resolve_crate_relative_path(span, segments, namespace);
@@ -2777,14 +2753,14 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             let unqualified_def = self.resolve_identifier(last_ident, namespace, check_ribs, true);
             return unqualified_def.and_then(|def| self.adjust_local_def(def, span))
                                   .map(|def| {
-                                      PathResolution::new(def, LastMod(AllPublic), path_depth)
+                                      PathResolution::new(def, path_depth)
                                   });
         }
 
         let unqualified_def = self.resolve_identifier(last_ident, namespace, check_ribs, false);
         let def = self.resolve_module_relative_path(span, segments, namespace);
         match (def, unqualified_def) {
-            (Some((ref d, _)), Some(ref ud)) if *d == ud.def => {
+            (Some(d), Some(ref ud)) if d == ud.def => {
                 self.session
                     .add_lint(lint::builtin::UNUSED_QUALIFICATIONS,
                               id,
@@ -2931,7 +2907,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                                     span: Span,
                                     segments: &[hir::PathSegment],
                                     namespace: Namespace)
-                                    -> Option<(Def, LastPrivate)> {
+                                    -> Option<Def> {
         let module_path = segments.split_last()
                                   .unwrap()
                                   .1
@@ -2940,7 +2916,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                                   .collect::<Vec<_>>();
 
         let containing_module;
-        let last_private;
         let current_module = self.current_module;
         match self.resolve_module_path(current_module, &module_path, UseLexicalScope, span) {
             Failed(err) => {
@@ -2957,22 +2932,17 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                 return None;
             }
             Indeterminate => return None,
-            Success((resulting_module, resulting_last_private)) => {
+            Success(resulting_module) => {
                 containing_module = resulting_module;
-                last_private = resulting_last_private;
             }
         }
 
         let name = segments.last().unwrap().identifier.name;
         let result = self.resolve_name_in_module(containing_module, name, namespace, false, true);
-        let def = match result {
+        result.success().map(|binding| {
-            Success(binding) => {
+            self.check_privacy(containing_module, name, binding, span);
-                let (def, lp) = binding.def_and_lp();
+            binding.def().unwrap()
-                (def, last_private.or(lp))
+        })
-            }
-            _ => return None,
-        };
-        return Some(def);
     }
 
     /// Invariant: This must be called only during main resolution, not during
@@ -2981,7 +2951,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                                    span: Span,
                                    segments: &[hir::PathSegment],
                                    namespace: Namespace)
-                                   -> Option<(Def, LastPrivate)> {
+                                   -> Option<Def> {
         let module_path = segments.split_last()
                                   .unwrap()
                                   .1
@@ -2992,12 +2962,10 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
         let root_module = self.graph_root;
 
         let containing_module;
-        let last_private;
         match self.resolve_module_path_from_root(root_module,
                                                  &module_path,
                                                  0,
-                                                 span,
+                                                 span) {
-                                                 LastMod(AllPublic)) {
             Failed(err) => {
                 let (span, msg) = match err {
                     Some((span, msg)) => (span, msg),
@@ -3014,20 +2982,17 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
 
             Indeterminate => return None,
 
-            Success((resulting_module, resulting_last_private)) => {
+            Success(resulting_module) => {
                 containing_module = resulting_module;
-                last_private = resulting_last_private;
             }
         }
 
         let name = segments.last().unwrap().identifier.name;
-        match self.resolve_name_in_module(containing_module, name, namespace, false, true) {
+        let result = self.resolve_name_in_module(containing_module, name, namespace, false, true);
-            Success(binding) => {
+        result.success().map(|binding| {
-                let (def, lp) = binding.def_and_lp();
+            self.check_privacy(containing_module, name, binding, span);
-                Some((def, last_private.or(lp)))
+            binding.def().unwrap()
-            }
+        })
-            _ => None,
-        }
     }
 
     fn resolve_identifier_in_local_ribs(&mut self,
@@ -3116,10 +3081,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                                                .and_then(NameBinding::module)
                 }
             } else {
-                match this.resolve_module_path(root, &name_path, UseLexicalScope, span) {
+                this.resolve_module_path(root, &name_path, UseLexicalScope, span).success()
-                    Success((module, _)) => Some(module),
-                    _ => None,
-                }
             }
         }
 
@@ -3431,7 +3393,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
                         self.record_def(expr.id,
                                         PathResolution {
                                             base_def: def,
-                                            last_private: LastMod(AllPublic),
                                             depth: 0,
                                         })
                     }
@@ -3624,12 +3585,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
 
     fn record_def(&mut self, node_id: NodeId, resolution: PathResolution) {
         debug!("(recording def) recording {:?} for {}", resolution, node_id);
-        assert!(match resolution.last_private {
-                    LastImport{..} => false,
-                    _ => true,
-                },
-                "Import should only be used for `use` directives");
-
         if let Some(prev_res) = self.def_map.borrow_mut().insert(node_id, resolution) {
             let span = self.ast_map.opt_span(node_id).unwrap_or(codemap::DUMMY_SP);
             self.session.span_bug(span,
@@ -3652,6 +3607,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));
+            }
+        }
+    }
 }
 
 
@@ -3767,7 +3753,6 @@ fn module_to_string(module: Module) -> String {
 fn err_path_resolution() -> PathResolution {
     PathResolution {
         base_def: Def::Err,
-        last_private: LastMod(AllPublic),
         depth: 0,
     }
 }
@@ -3809,6 +3794,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,

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;
@@ -25,7 +25,6 @@ use build_reduced_graph;
 
 use rustc::lint;
 use rustc::middle::def::*;
-use rustc::middle::privacy::*;
 
 use syntax::ast::{NodeId, Name};
 use syntax::attr::AttrMetaMethods;
@@ -79,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(),
@@ -92,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,
         }
@@ -220,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);
@@ -296,7 +301,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
                                  &import_directive.module_path,
                                  UseLexicalScopeFlag::DontUseLexicalScope,
                                  import_directive.span)
-            .and_then(|(containing_module, lp)| {
+            .and_then(|containing_module| {
                 // We found the module that the target is contained
                 // within. Attempt to resolve the import within it.
                 if let SingleImport(target, source) = import_directive.subclass {
@@ -304,10 +309,9 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
                                                containing_module,
                                                target,
                                                source,
-                                               import_directive,
+                                               import_directive)
-                                               lp)
                 } else {
-                    self.resolve_glob_import(module_, containing_module, import_directive, lp)
+                    self.resolve_glob_import(module_, containing_module, import_directive)
                 }
             })
             .and_then(|()| {
@@ -333,26 +337,14 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
                              target_module: Module<'b>,
                              target: Name,
                              source: Name,
-                             directive: &ImportDirective,
+                             directive: &ImportDirective)
-                             lp: LastPrivate)
                              -> ResolveResult<()> {
-        debug!("(resolving single import) resolving `{}` = `{}::{}` from `{}` id {}, last \
+        debug!("(resolving single import) resolving `{}` = `{}::{}` from `{}` id {}",
-                private {:?}",
                target,
                module_to_string(&target_module),
                source,
                module_to_string(module_),
-               directive.id,
+               directive.id);
-               lp);
-
-        let lp = match lp {
-            LastMod(lp) => lp,
-            LastImport {..} => {
-                self.resolver
-                    .session
-                    .span_bug(directive.span, "not expecting Import here, must be LastMod")
-            }
-        };
 
         // If this is a circular import, we temporarily count it as determined so that
         // it fails (as opposed to being indeterminate) when nothing else can define it.
@@ -433,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) {
@@ -440,38 +434,34 @@ 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.
         module_.decrement_outstanding_references_for(target, ValueNS);
         module_.decrement_outstanding_references_for(target, TypeNS);
 
-        let def_and_priv = |binding: &NameBinding| {
+        let def = match type_result.success().and_then(NameBinding::def) {
-            let last_private =
+            Some(def) => def,
-                if binding.is_public() { lp } else { DependsOn(binding.local_def_id().unwrap()) };
+            None => value_result.success().and_then(NameBinding::def).unwrap(),
-            (binding.def().unwrap(), last_private)
         };
-        let value_def_and_priv = value_result.success().map(&def_and_priv);
+        let path_resolution = PathResolution { base_def: def, depth: 0 };
-        let type_def_and_priv = type_result.success().map(&def_and_priv);
+        self.resolver.def_map.borrow_mut().insert(directive.id, path_resolution);
-
-        let import_lp = LastImport {
-            value_priv: value_def_and_priv.map(|(_, p)| p),
-            value_used: Used,
-            type_priv: type_def_and_priv.map(|(_, p)| p),
-            type_used: Used,
-        };
-
-        let write_path_resolution = |(def, _)| {
-            let path_resolution =
-                PathResolution { base_def: def, last_private: import_lp, depth: 0 };
-            self.resolver.def_map.borrow_mut().insert(directive.id, path_resolution);
-        };
-        value_def_and_priv.map(&write_path_resolution);
-        type_def_and_priv.map(&write_path_resolution);
 
         debug!("(resolving single import) successfully resolved import");
         return Success(());
@@ -484,8 +474,7 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
     fn resolve_glob_import(&mut self,
                            module_: Module<'b>,
                            target_module: Module<'b>,
-                           directive: &ImportDirective,
+                           directive: &ImportDirective)
-                           lp: LastPrivate)
                            -> ResolveResult<()> {
         // We must bail out if the node has unresolved imports of any kind (including globs).
         if target_module.pub_count.get() > 0 {
@@ -503,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) {
@@ -521,7 +510,6 @@ impl<'a, 'b:'a, 'tcx:'b> ImportResolver<'a, 'b, 'tcx> {
             self.resolver.def_map.borrow_mut().insert(directive.id,
                                                       PathResolution {
                                                           base_def: Def::Mod(did),
-                                                          last_private: lp,
                                                           depth: 0,
                                                       });
         }

src/librustc_typeck/astconv.rs

@@ -54,7 +54,6 @@ use middle::const_eval::EvalHint::UncheckedExprHint;
 use middle::def::{self, Def};
 use middle::def_id::DefId;
 use middle::resolve_lifetime as rl;
-use middle::privacy::{AllPublic, LastMod};
 use middle::subst::{FnSpace, TypeSpace, SelfSpace, Subst, Substs, ParamSpace};
 use middle::traits;
 use middle::ty::{self, Ty, ToPredicate, TypeFoldable};
@@ -1650,7 +1649,6 @@ pub fn ast_ty_to_ty<'tcx>(this: &AstConv<'tcx>,
                 // Create some fake resolution that can't possibly be a type.
                 def::PathResolution {
                     base_def: Def::Mod(tcx.map.local_def_id(ast::CRATE_NODE_ID)),
-                    last_private: LastMod(AllPublic),
                     depth: path.segments.len()
                 }
             } else {
@@ -1674,7 +1672,6 @@ pub fn ast_ty_to_ty<'tcx>(this: &AstConv<'tcx>,
                 // Write back the new resolution.
                 tcx.def_map.borrow_mut().insert(ast_ty.id, def::PathResolution {
                     base_def: def,
-                    last_private: path_res.last_private,
                     depth: 0
                 });
             }

src/librustc_typeck/check/_match.rs

@@ -12,7 +12,6 @@ use middle::def::{self, Def};
 use middle::infer::{self, TypeOrigin};
 use middle::pat_util::{PatIdMap, pat_id_map, pat_is_binding};
 use middle::pat_util::pat_is_resolved_const;
-use middle::privacy::{AllPublic, LastMod};
 use middle::subst::Substs;
 use middle::ty::{self, Ty, TypeFoldable, LvaluePreference};
 use check::{check_expr, check_expr_has_type, check_expr_with_expectation};
@@ -219,7 +218,6 @@ pub fn check_pat<'a, 'tcx>(pcx: &pat_ctxt<'a, 'tcx>,
                 let sentinel = fcx.tcx().map.local_def_id(ast::CRATE_NODE_ID);
                 def::PathResolution {
                     base_def: Def::Mod(sentinel),
-                    last_private: LastMod(AllPublic),
                     depth: path.segments.len()
                 }
             } else {

src/librustc_typeck/check/method/mod.rs

@@ -14,7 +14,6 @@ use astconv::AstConv;
 use check::FnCtxt;
 use middle::def::Def;
 use middle::def_id::DefId;
-use middle::privacy::{AllPublic, DependsOn, LastPrivate, LastMod};
 use middle::subst;
 use middle::traits;
 use middle::ty::{self, ToPredicate, ToPolyTraitRef, TraitRef, TypeFoldable};
@@ -334,28 +333,21 @@ pub fn resolve_ufcs<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
                               method_name: ast::Name,
                               self_ty: ty::Ty<'tcx>,
                               expr_id: ast::NodeId)
-                              -> Result<(Def, LastPrivate), MethodError<'tcx>>
+                              -> Result<Def, MethodError<'tcx>>
 {
     let mode = probe::Mode::Path;
     let pick = try!(probe::probe(fcx, span, mode, method_name, self_ty, expr_id));
-    let def_id = pick.item.def_id();
+    let def = pick.item.def();
-    let mut lp = LastMod(AllPublic);
+
     if let probe::InherentImplPick = pick.kind {
-        if pick.item.vis() != hir::Public {
+        if pick.item.vis() != hir::Public && !fcx.private_item_is_visible(def.def_id()) {
-            lp = LastMod(DependsOn(def_id));
+            let msg = format!("{} `{}` is private", def.kind_name(), &method_name.as_str());
+            fcx.tcx().sess.span_err(span, &msg);
         }
     }
-    let def_result = match pick.item {
+    Ok(def)
-        ty::ImplOrTraitItem::MethodTraitItem(..) => Def::Method(def_id),
-        ty::ImplOrTraitItem::ConstTraitItem(..) => Def::AssociatedConst(def_id),
-        ty::ImplOrTraitItem::TypeTraitItem(..) => {
-            fcx.tcx().sess.span_bug(span, "resolve_ufcs: probe picked associated type");
-        }
-    };
-    Ok((def_result, lp))
 }
 
-
 /// Find item with name `item_name` defined in `trait_def_id`
 /// and return it, or `None`, if no such item.
 fn trait_item<'tcx>(tcx: &ty::ctxt<'tcx>,

src/librustc_typeck/check/mod.rs

@@ -91,7 +91,6 @@ use middle::def_id::DefId;
 use middle::infer;
 use middle::infer::{TypeOrigin, type_variable};
 use middle::pat_util::{self, pat_id_map};
-use middle::privacy::{AllPublic, LastMod};
 use middle::subst::{self, Subst, Substs, VecPerParamSpace, ParamSpace};
 use middle::traits::{self, report_fulfillment_errors};
 use middle::ty::{GenericPredicates, TypeScheme};
@@ -2013,6 +2012,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             Err(errors) => { report_fulfillment_errors(self.infcx(), &errors); }
         }
     }
+
+    fn private_item_is_visible(&self, def_id: DefId) -> bool {
+        match self.tcx().map.as_local_node_id(def_id) {
+            Some(node_id) => self.tcx().map.private_item_is_visible_from(node_id, self.body_id),
+            None => false, // Private items from other crates are never visible
+        }
+    }
 }
 
 impl<'a, 'tcx> RegionScope for FnCtxt<'a, 'tcx> {
@@ -3348,7 +3354,6 @@ fn check_expr_with_unifier<'a, 'tcx, F>(fcx: &FnCtxt<'a, 'tcx>,
                 // Create some fake resolution that can't possibly be a type.
                 def::PathResolution {
                     base_def: Def::Mod(tcx.map.local_def_id(ast::CRATE_NODE_ID)),
-                    last_private: LastMod(AllPublic),
                     depth: path.segments.len()
                 }
             } else {
@@ -3787,12 +3792,11 @@ pub fn resolve_ty_and_def_ufcs<'a, 'b, 'tcx>(fcx: &FnCtxt<'b, 'tcx>,
         let item_segment = path.segments.last().unwrap();
         let item_name = item_segment.identifier.name;
         match method::resolve_ufcs(fcx, span, item_name, ty, node_id) {
-            Ok((def, lp)) => {
+            Ok(def) => {
                 // Write back the new resolution.
                 fcx.ccx.tcx.def_map.borrow_mut()
                        .insert(node_id, def::PathResolution {
                    base_def: def,
-                   last_private: path_res.last_private.or(lp),
                    depth: 0
                 });
                 Some((Some(ty), slice::ref_slice(item_segment), def))

src/test/auxiliary/ambig_impl_2_lib.rs

@@ -8,7 +8,7 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-trait me {
+pub trait me {
     fn me(&self) -> usize;
 }
 impl me for usize { fn me(&self) -> usize { *self } }

src/test/auxiliary/struct_field_privacy.rs

@@ -8,7 +8,7 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-struct A {
+pub struct A {
     a: isize,
     pub b: isize,
 }

src/test/compile-fail/blind-item-block-middle.rs

@@ -8,7 +8,7 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-mod foo { struct bar; }
+mod foo { pub struct bar; }
 
 fn main() {
     let bar = 5;

src/test/compile-fail/const-pattern-irrefutable.rs

@@ -9,8 +9,8 @@
 // except according to those terms.
 
 mod foo {
-    const b: u8 = 2; //~ NOTE constant defined here
+    pub const b: u8 = 2; //~ NOTE constant defined here
-    const d: u8 = 2; //~ NOTE constant defined here
+    pub const d: u8 = 2; //~ NOTE constant defined here
 }
 
 use foo::b as c; //~ NOTE constant imported here

src/test/compile-fail/double-import.rs

@@ -14,11 +14,11 @@
 // when reporting the error.
 
 mod sub1 {
-    fn foo() {} // implementation 1
+    pub fn foo() {} // implementation 1
 }
 
 mod sub2 {
-    fn foo() {} // implementation 2
+    pub fn foo() {} // implementation 2
 }
 
 use sub1::foo; //~ NOTE previous import of `foo` here

src/test/compile-fail/export-tag-variant.rs

@@ -14,4 +14,4 @@ mod foo {
     enum y { y1, }
 }
 
-fn main() { let z = foo::y::y1; } //~ ERROR: is inaccessible
+fn main() { let z = foo::y::y1; } //~ ERROR: enum `y` is private

src/test/compile-fail/issue-11680.rs

@@ -14,8 +14,8 @@ extern crate issue_11680 as other;
 
 fn main() {
     let _b = other::Foo::Bar(1);
-    //~^ ERROR: variant `Bar` is private
+    //~^ ERROR: enum `Foo` is private
 
     let _b = other::test::Foo::Bar(1);
-    //~^ ERROR: variant `Bar` is private
+    //~^ ERROR: enum `Foo` is private
 }

src/test/compile-fail/issue-13407.rs

@@ -16,4 +16,5 @@ fn main() {
     A::C = 1;
     //~^ ERROR: invalid left-hand side expression
     //~| ERROR: mismatched types
+    //~| ERROR: struct `C` is private
 }

src/test/compile-fail/issue-13641.rs

@@ -17,9 +17,7 @@ mod a {
 
 fn main() {
     a::Foo::new();
-    //~^ ERROR: method `new` is inaccessible
+    //~^ ERROR: struct `Foo` is private
-    //~^^ NOTE: struct `Foo` is private
     a::Bar::new();
-    //~^ ERROR: method `new` is inaccessible
+    //~^ ERROR: enum `Bar` is private
-    //~^^ NOTE: enum `Bar` is private
 }

src/test/compile-fail/issue-16538.rs

@@ -9,11 +9,11 @@
 // except according to those terms.
 
 mod Y {
-    type X = usize;
+    pub type X = usize;
     extern {
-        static x: *const usize;
+        pub static x: *const usize;
     }
-    fn foo(value: *const X) -> *const X {
+    pub fn foo(value: *const X) -> *const X {
         value
     }
 }

src/test/compile-fail/issue-21221-2.rs

@@ -13,7 +13,7 @@ pub mod foo {
         // note: trait T is not public, but being in the current
         // crate, it's fine to show it, since the programmer can
         // decide to make it public based on the suggestion ...
-        trait T {}
+        pub trait T {}
     }
     // imports should be ignored:
     use self::bar::T;

src/test/compile-fail/issue-25396.rs

@@ -32,6 +32,6 @@ mod foo {
 mod bar {
     pub mod baz {}
     pub type Quux = i32;
-    struct blah { x: i8 }
+    pub struct blah { x: i8 }
     pub const WOMP: i8 = -5;
 }

src/test/compile-fail/issue-29161.rs

@@ -13,7 +13,6 @@ mod a {
 
     impl Default for A {
         pub fn default() -> A {
-            //~^ ERROR E0449
             A;
         }
     }
@@ -22,5 +21,5 @@ mod a {
 
 fn main() {
     a::A::default();
-    //~^ ERROR method `default` is inaccessible
+    //~^ ERROR struct `A` is private
  }

src/test/compile-fail/privacy-in-paths.rs

@@ -0,0 +1,40 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+mod foo {
+    pub use self::bar::S;
+    mod bar {
+        pub struct S;
+        pub use baz;
+    }
+
+    trait T {
+        type Assoc;
+    }
+    impl T for () {
+        type Assoc = S;
+    }
+}
+
+impl foo::S {
+    fn f() {}
+}
+
+pub mod baz {
+    fn f() {}
+
+    fn g() {
+        ::foo::bar::baz::f(); //~ERROR module `bar` is private
+        ::foo::bar::S::f(); //~ERROR module `bar` is private
+        <() as ::foo::T>::Assoc::f(); //~ERROR trait `T` is private
+    }
+}
+
+fn main() {}

src/test/compile-fail/privacy-ns2.rs

@@ -25,14 +25,13 @@ pub mod foo1 {
 }
 
 fn test_single1() {
-    // In an ideal world, these would be private instead of inaccessible.
+    use foo1::Bar;  //~ ERROR function `Bar` is private
-    use foo1::Bar;  //~ ERROR `Bar` is inaccessible
 
     Bar();
 }
 
 fn test_list1() {
-    use foo1::{Bar,Baz};  //~ ERROR `Bar` is inaccessible
+    use foo1::{Bar,Baz};  //~ ERROR `Bar` is private
 
     Bar();
 }
@@ -47,7 +46,7 @@ pub mod foo2 {
 }
 
 fn test_single2() {
-    use foo2::Bar;  //~ ERROR `Bar` is private
+    use foo2::Bar;  //~ ERROR trait `Bar` is private
 
     let _x : Box<Bar>;
 }

src/test/compile-fail/privacy-ufcs.rs

@@ -19,6 +19,5 @@ mod foo {
 }
 
 fn main() {
-    <i32 as ::foo::Bar>::baz(); //~ERROR method `baz` is inaccessible
+    <i32 as ::foo::Bar>::baz(); //~ERROR trait `Bar` is private
-                                //~^NOTE: trait `Bar` is private
 }

src/test/compile-fail/privacy1.rs

@@ -72,7 +72,6 @@ mod bar {
         self::baz::A::foo();
         self::baz::A::bar(); //~ ERROR: method `bar` is private
         self::baz::A.foo2();
-        self::baz::A.bar2(); //~ ERROR: method `bar2` is private
 
         // this used to cause an ICE in privacy traversal.
         super::gpub();
@@ -91,7 +90,6 @@ fn lol() {
     bar::A::foo();
     bar::A::bar(); //~ ERROR: method `bar` is private
     bar::A.foo2();
-    bar::A.bar2(); //~ ERROR: method `bar2` is private
 }
 
 mod foo {
@@ -99,19 +97,14 @@ mod foo {
         ::bar::A::foo();
         ::bar::A::bar();        //~ ERROR: method `bar` is private
         ::bar::A.foo2();
-        ::bar::A.bar2();        //~ ERROR: method `bar2` is private
+        ::bar::baz::A::foo();   //~ ERROR: module `baz` is private
-        ::bar::baz::A::foo();   //~ ERROR: method `foo` is inaccessible
+        ::bar::baz::A::bar();   //~ ERROR: module `baz` is private
-                                //~^ NOTE: module `baz` is private
+                                //~^ ERROR: method `bar` is private
-        ::bar::baz::A::bar();   //~ ERROR: method `bar` is private
+        ::bar::baz::A.foo2();   //~ ERROR: module `baz` is private
-        ::bar::baz::A.foo2();   //~ ERROR: struct `A` is inaccessible
+        ::bar::baz::A.bar2();   //~ ERROR: module `baz` is private
-                                //~^ NOTE: module `baz` is private
-        ::bar::baz::A.bar2();   //~ ERROR: struct `A` is inaccessible
-                                //~^ ERROR: method `bar2` is private
-                                //~^^ NOTE: module `baz` is private
 
         let _: isize =
-        ::bar::B::foo();        //~ ERROR: method `foo` is inaccessible
+        ::bar::B::foo();        //~ ERROR: trait `B` is private
-                                //~^ NOTE: trait `B` is private
         ::lol();
 
         ::bar::Enum::Pub;
@@ -126,19 +119,14 @@ mod foo {
 
         ::bar::gpub();
 
-        ::bar::baz::foo(); //~ ERROR: function `foo` is inaccessible
+        ::bar::baz::foo(); //~ ERROR: module `baz` is private
-                           //~^ NOTE: module `baz` is private
+        ::bar::baz::bar(); //~ ERROR: module `baz` is private
-        ::bar::baz::bar(); //~ ERROR: function `bar` is inaccessible
-                           //~^ NOTE: module `baz` is private
     }
 
     fn test2() {
         use bar::baz::{foo, bar};
-        //~^ ERROR: function `foo` is inaccessible
+        //~^ ERROR: module `baz` is private
-        //~| NOTE: module `baz` is private
+        //~| ERROR: module `baz` is private
-        //~| ERROR: function `bar` is inaccessible
-        //~| NOTE: module `baz` is private
-
 
         foo();
         bar();
@@ -169,8 +157,7 @@ pub mod mytest {
     // Even though the inner `A` struct is a publicly exported item (usable from
     // external crates through `foo::foo`, it should not be accessible through
     // its definition path (which has the private `i` module).
-    use self::foo::i::A; //~ ERROR: struct `A` is inaccessible
+    use self::foo::i::A; //~ ERROR: module `i` is private
-                         //~^ NOTE: module `i` is private
 
     pub mod foo {
         pub use self::i::A as foo;

src/test/compile-fail/privacy2.rs

@@ -16,7 +16,7 @@
 mod bar {
     pub use self::glob::*;
 
-    mod glob {
+    pub mod glob {
         use foo;
     }
 }

src/test/compile-fail/privacy4.rs

@@ -28,7 +28,7 @@ mod bar {
 pub fn foo() {}
 
 fn test2() {
-    use bar::glob::gpriv; //~ ERROR: function `gpriv` is private
+    use bar::glob::gpriv; //~ ERROR: module `glob` is private
     gpriv();
 }
 

src/test/compile-fail/private-impl-method.rs

@@ -18,7 +18,14 @@ mod a {
     }
 }
 
+fn f() {
+    impl a::Foo {
+        fn bar(&self) {} // This should be visible outside `f`
+    }
+}
+
 fn main() {
     let s = a::Foo { x: 1 };
+    s.bar();
     s.foo();    //~ ERROR method `foo` is private
 }

src/test/compile-fail/struct-field-privacy.rs

@@ -17,7 +17,7 @@ struct A {
 }
 
 mod inner {
-    struct A {
+    pub struct A {
         a: isize,
         pub b: isize,
     }
@@ -28,9 +28,6 @@ mod inner {
 }
 
 fn test(a: A, b: inner::A, c: inner::B, d: xc::A, e: xc::B) {
-    //~^ ERROR: struct `A` is private
-    //~^^ ERROR: struct `A` is private
-
     a.a;
     b.a; //~ ERROR: field `a` of struct `inner::A` is private
     b.b;

src/test/compile-fail/struct-variant-privacy-xc.rs

@@ -13,7 +13,7 @@ extern crate struct_variant_privacy;
 
 fn f(b: struct_variant_privacy::Bar) { //~ ERROR enum `Bar` is private
     match b {
-        struct_variant_privacy::Bar::Baz { a: _a } => {} //~ ERROR variant `Baz` is private
+        struct_variant_privacy::Bar::Baz { a: _a } => {} //~ ERROR enum `Bar` is private
     }
 }
 

src/test/compile-fail/struct-variant-privacy.rs

@@ -15,8 +15,7 @@ mod foo {
 
 fn f(b: foo::Bar) { //~ ERROR enum `Bar` is private
     match b {
-        foo::Bar::Baz { a: _a } => {} //~ ERROR variant `Baz` is inaccessible
+        foo::Bar::Baz { a: _a } => {} //~ ERROR enum `Bar` is private
-        // ^~ ERROR enum `Bar` is private
     }
 }
 

src/test/compile-fail/task-rng-isnt-sendable.rs

@@ -10,7 +10,7 @@
 
 // ensure that the ThreadRng isn't/doesn't become accidentally sendable.
 
-use std::rand;
+use std::rand; //~ ERROR: module `rand` is private
 
 fn test_send<S: Send>() {}
 

src/test/compile-fail/use-mod-3.rs

@@ -12,8 +12,7 @@ use foo::bar::{
     self //~ ERROR module `bar` is private
 };
 use foo::bar::{
-    Bar //~ ERROR type `Bar` is inaccessible
+    Bar //~ ERROR module `bar` is private
-    //~^ NOTE module `bar` is private
 };
 
 mod foo {

src/test/compile-fail/xcrate-private-by-default.rs

@@ -43,13 +43,13 @@ fn main() {
 
     // public items in a private mod should be inaccessible
     static_priv_by_default::foo::a;
-    //~^ ERROR: static `a` is private
+    //~^ ERROR: module `foo` is private
     static_priv_by_default::foo::b;
-    //~^ ERROR: function `b` is private
+    //~^ ERROR: module `foo` is private
     static_priv_by_default::foo::c;
-    //~^ ERROR: struct `c` is private
+    //~^ ERROR: module `foo` is private
     foo::<static_priv_by_default::foo::d>();
-    //~^ ERROR: enum `d` is private
+    //~^ ERROR: module `foo` is private
     foo::<static_priv_by_default::foo::e>();
-    //~^ ERROR: type `e` is private
+    //~^ ERROR: module `foo` is private
 }