Auto merge of #31938 - jseyfried:autoderef_privacy, r=nikomatsakis

Integrate privacy into field and method selection

This PR integrates privacy checking into field and method selection so that an inaccessible field/method can not stop an accessible field/method from being used (fixes #12808 and fixes #22684).
r? @eddyb

src/librustc/front/map/mod.rs

@@ -581,14 +581,6 @@ impl<'ast> Map<'ast> {
         }
     }
 
-    pub fn get_foreign_vis(&self, id: NodeId) -> Visibility {
-        let vis = self.expect_foreign_item(id).vis; // read recorded by `expect_foreign_item`
-        match self.find(self.get_parent(id)) { // read recorded by `find`
-            Some(NodeItem(i)) => vis.inherit_from(i.vis),
-            _ => vis
-        }
-    }
-
     pub fn expect_item(&self, id: NodeId) -> &'ast Item {
         match self.find(id) { // read recorded by `find`
             Some(NodeItem(item)) => item,

src/librustc_privacy/lib.rs

@@ -27,9 +27,6 @@
 extern crate rustc;
 extern crate rustc_front;
 
-use self::PrivacyResult::*;
-use self::FieldName::*;
-
 use std::cmp;
 use std::mem::replace;
 
@@ -43,7 +40,7 @@ use rustc::middle::def::{self, Def};
 use rustc::middle::def_id::DefId;
 use rustc::middle::privacy::{AccessLevel, AccessLevels};
 use rustc::ty::{self, TyCtxt};
-use rustc::util::nodemap::{NodeMap, NodeSet};
+use rustc::util::nodemap::NodeSet;
 use rustc::front::map as ast_map;
 
 use syntax::ast;
@@ -58,98 +55,6 @@ type Context<'a, 'tcx> = (&'a ty::MethodMap<'tcx>, &'a def::ExportMap);
 /// optionally the same for a note about the error.
 type CheckResult = Option<(Span, String, Option<(Span, String)>)>;
 
-////////////////////////////////////////////////////////////////////////////////
-/// The parent visitor, used to determine what's the parent of what (node-wise)
-////////////////////////////////////////////////////////////////////////////////
-
-struct ParentVisitor<'a, 'tcx:'a> {
-    tcx: &'a TyCtxt<'tcx>,
-    parents: NodeMap<ast::NodeId>,
-    curparent: ast::NodeId,
-}
-
-impl<'a, 'tcx, 'v> Visitor<'v> for ParentVisitor<'a, 'tcx> {
-    /// We want to visit items in the context of their containing
-    /// module and so forth, so supply a crate for doing a deep walk.
-    fn visit_nested_item(&mut self, item: hir::ItemId) {
-        self.visit_item(self.tcx.map.expect_item(item.id))
-    }
-    fn visit_item(&mut self, item: &hir::Item) {
-        self.parents.insert(item.id, self.curparent);
-
-        let prev = self.curparent;
-        match item.node {
-            hir::ItemMod(..) => { self.curparent = item.id; }
-            // Enum variants are parented to the enum definition itself because
-            // they inherit privacy
-            hir::ItemEnum(ref def, _) => {
-                for variant in &def.variants {
-                    // The parent is considered the enclosing enum because the
-                    // enum will dictate the privacy visibility of this variant
-                    // instead.
-                    self.parents.insert(variant.node.data.id(), item.id);
-                }
-            }
-
-            // Trait methods are always considered "public", but if the trait is
-            // private then we need some private item in the chain from the
-            // method to the root. In this case, if the trait is private, then
-            // parent all the methods to the trait to indicate that they're
-            // private.
-            hir::ItemTrait(_, _, _, ref trait_items) if item.vis != hir::Public => {
-                for trait_item in trait_items {
-                    self.parents.insert(trait_item.id, item.id);
-                }
-            }
-
-            _ => {}
-        }
-        intravisit::walk_item(self, item);
-        self.curparent = prev;
-    }
-
-    fn visit_foreign_item(&mut self, a: &hir::ForeignItem) {
-        self.parents.insert(a.id, self.curparent);
-        intravisit::walk_foreign_item(self, a);
-    }
-
-    fn visit_fn(&mut self, a: intravisit::FnKind<'v>, b: &'v hir::FnDecl,
-                c: &'v hir::Block, d: Span, id: ast::NodeId) {
-        // We already took care of some trait methods above, otherwise things
-        // like impl methods and pub trait methods are parented to the
-        // containing module, not the containing trait.
-        if !self.parents.contains_key(&id) {
-            self.parents.insert(id, self.curparent);
-        }
-        intravisit::walk_fn(self, a, b, c, d);
-    }
-
-    fn visit_impl_item(&mut self, ii: &'v hir::ImplItem) {
-        // visit_fn handles methods, but associated consts have to be handled
-        // here.
-        if !self.parents.contains_key(&ii.id) {
-            self.parents.insert(ii.id, self.curparent);
-        }
-        intravisit::walk_impl_item(self, ii);
-    }
-
-    fn visit_variant_data(&mut self, s: &hir::VariantData, _: ast::Name,
-                        _: &'v hir::Generics, item_id: ast::NodeId, _: Span) {
-        // Struct constructors are parented to their struct definitions because
-        // they essentially are the struct definitions.
-        if !s.is_struct() {
-            self.parents.insert(s.id(), item_id);
-        }
-
-        // While we have the id of the struct definition, go ahead and parent
-        // all the fields.
-        for field in s.fields() {
-            self.parents.insert(field.id, self.curparent);
-        }
-        intravisit::walk_struct_def(self, s)
-    }
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 /// The embargo visitor, used to determine the exports of the ast
 ////////////////////////////////////////////////////////////////////////////////
@@ -475,331 +380,45 @@ struct PrivacyVisitor<'a, 'tcx: 'a> {
     tcx: &'a TyCtxt<'tcx>,
     curitem: ast::NodeId,
     in_foreign: bool,
-    parents: NodeMap<ast::NodeId>,
-}
-
-#[derive(Debug)]
-enum PrivacyResult {
-    Allowable,
-    ExternallyDenied,
-    DisallowedBy(ast::NodeId),
-}
-
-enum FieldName {
-    UnnamedField(usize), // index
-    NamedField(ast::Name),
 }
 
 impl<'a, 'tcx> PrivacyVisitor<'a, 'tcx> {
-    // Determines whether the given definition is public from the point of view
+    fn item_is_visible(&self, did: DefId) -> bool {
-    // of the current item.
+        let visibility = match self.tcx.map.as_local_node_id(did) {
-    fn def_privacy(&self, did: DefId) -> PrivacyResult {
+            Some(node_id) => self.tcx.map.expect_item(node_id).vis,
-        let node_id = if let Some(node_id) = self.tcx.map.as_local_node_id(did) {
+            None => self.tcx.sess.cstore.visibility(did),
-            node_id
-        } else {
-            if self.tcx.sess.cstore.visibility(did) == hir::Public {
-                debug!("privacy - {:?} was externally exported", did);
-                return Allowable;
-            }
-            debug!("privacy - is {:?} a public method", did);
-
-            return match self.tcx.impl_or_trait_items.borrow().get(&did) {
-                Some(&ty::ConstTraitItem(ref ac)) => {
-                    debug!("privacy - it's a const: {:?}", *ac);
-                    match ac.container {
-                        ty::TraitContainer(id) => {
-                            debug!("privacy - recursing on trait {:?}", id);
-                            self.def_privacy(id)
-                        }
-                        ty::ImplContainer(id) => {
-                            match self.tcx.impl_trait_ref(id) {
-                                Some(t) => {
-                                    debug!("privacy - impl of trait {:?}", id);
-                                    self.def_privacy(t.def_id)
-                                }
-                                None => {
-                                    debug!("privacy - found inherent \
-                                            associated constant {:?}",
-                                            ac.vis);
-                                    if ac.vis == hir::Public {
-                                        Allowable
-                                    } else {
-                                        ExternallyDenied
-                                    }
-                                }
-                            }
-                        }
-                    }
-                }
-                Some(&ty::MethodTraitItem(ref meth)) => {
-                    debug!("privacy - well at least it's a method: {:?}",
-                           *meth);
-                    match meth.container {
-                        ty::TraitContainer(id) => {
-                            debug!("privacy - recursing on trait {:?}", id);
-                            self.def_privacy(id)
-                        }
-                        ty::ImplContainer(id) => {
-                            match self.tcx.impl_trait_ref(id) {
-                                Some(t) => {
-                                    debug!("privacy - impl of trait {:?}", id);
-                                    self.def_privacy(t.def_id)
-                                }
-                                None => {
-                                    debug!("privacy - found a method {:?}",
-                                            meth.vis);
-                                    if meth.vis == hir::Public {
-                                        Allowable
-                                    } else {
-                                        ExternallyDenied
-                                    }
-                                }
-                            }
-                        }
-                    }
-                }
-                Some(&ty::TypeTraitItem(ref typedef)) => {
-                    match typedef.container {
-                        ty::TraitContainer(id) => {
-                            debug!("privacy - recursing on trait {:?}", id);
-                            self.def_privacy(id)
-                        }
-                        ty::ImplContainer(id) => {
-                            match self.tcx.impl_trait_ref(id) {
-                                Some(t) => {
-                                    debug!("privacy - impl of trait {:?}", id);
-                                    self.def_privacy(t.def_id)
-                                }
-                                None => {
-                                    debug!("privacy - found a typedef {:?}",
-                                            typedef.vis);
-                                    if typedef.vis == hir::Public {
-                                        Allowable
-                                    } else {
-                                        ExternallyDenied
-                                    }
-                                }
-                            }
-                        }
-                    }
-                }
-                None => {
-                    debug!("privacy - nope, not even a method");
-                    ExternallyDenied
-                }
-            };
         };
-
+        visibility == hir::Public || self.private_accessible(did)
-        debug!("privacy - local {} not public all the way down",
-               self.tcx.map.node_to_string(node_id));
-        // return quickly for things in the same module
-        if self.parents.get(&node_id) == self.parents.get(&self.curitem) {
-            debug!("privacy - same parent, we're done here");
-            return Allowable;
-        }
-
-        let vis = match self.tcx.map.find(node_id) {
-            // If this item is a method, then we know for sure that it's an
-            // actual method and not a static method. The reason for this is
-            // that these cases are only hit in the ExprMethodCall
-            // expression, and ExprCall will have its path checked later
-            // (the path of the trait/impl) if it's a static method.
-            //
-            // With this information, then we can completely ignore all
-            // trait methods. The privacy violation would be if the trait
-            // couldn't get imported, not if the method couldn't be used
-            // (all trait methods are public).
-            //
-            // However, if this is an impl method, then we dictate this
-            // decision solely based on the privacy of the method
-            // invocation.
-            Some(ast_map::NodeImplItem(ii)) => {
-                let imp = self.tcx.map.get_parent_did(node_id);
-                match self.tcx.impl_trait_ref(imp) {
-                    Some(..) => hir::Public,
-                    _ => ii.vis,
-                }
-            }
-            Some(ast_map::NodeTraitItem(_)) => hir::Public,
-
-            // This is not a method call, extract the visibility as one
-            // would normally look at it
-            Some(ast_map::NodeItem(it)) => it.vis,
-            Some(ast_map::NodeForeignItem(_)) => {
-                self.tcx.map.get_foreign_vis(node_id)
-            }
-            _ => hir::Public,
-        };
-        if vis == hir::Public { return Allowable }
-
-        if self.private_accessible(node_id) {
-            Allowable
-        } else {
-            DisallowedBy(node_id)
-        }
     }
 
-    /// True if `id` is both local and private-accessible
+    /// True if `did` is private-accessible
-    fn local_private_accessible(&self, did: DefId) -> bool {
+    fn private_accessible(&self, did: DefId) -> bool {
-        if let Some(node_id) = self.tcx.map.as_local_node_id(did) {
+        match self.tcx.map.as_local_node_id(did) {
-            self.private_accessible(node_id)
+            Some(node_id) => self.tcx.map.private_item_is_visible_from(node_id, self.curitem),
-        } else {
+            None => false,
-            false
         }
     }
 
-    /// For a local private node in the AST, this function will determine
-    /// whether the node is accessible by the current module that iteration is
-    /// inside.
-    fn private_accessible(&self, id: ast::NodeId) -> bool {
-        self.tcx.map.private_item_is_visible_from(id, self.curitem)
-    }
-
-    fn report_error(&self, result: CheckResult) -> bool {
-        match result {
-            None => true,
-            Some((span, msg, note)) => {
-                let mut err = self.tcx.sess.struct_span_err(span, &msg[..]);
-                if let Some((span, msg)) = note {
-                    err.span_note(span, &msg[..]);
-                }
-                err.emit();
-                false
-            },
-        }
-    }
-
-    /// Guarantee that a particular definition is public. Returns a CheckResult
-    /// which contains any errors found. These can be reported using `report_error`.
-    /// If the result is `None`, no errors were found.
-    fn ensure_public(&self,
-                     span: Span,
-                     to_check: DefId,
-                     source_did: Option<DefId>,
-                     msg: &str)
-                     -> CheckResult {
-        debug!("ensure_public(span={:?}, to_check={:?}, source_did={:?}, msg={:?})",
-               span, to_check, source_did, msg);
-        let def_privacy = self.def_privacy(to_check);
-        debug!("ensure_public: def_privacy={:?}", def_privacy);
-        let id = match def_privacy {
-            ExternallyDenied => {
-                return Some((span, format!("{} is private", msg), None))
-            }
-            Allowable => return None,
-            DisallowedBy(id) => id,
-        };
-
-        // If we're disallowed by a particular id, then we attempt to
-        // give a nice error message to say why it was disallowed. It
-        // was either because the item itself is private or because
-        // its parent is private and its parent isn't in our
-        // ancestry. (Both the item being checked and its parent must
-        // be local.)
-        let def_id = source_did.unwrap_or(to_check);
-        let node_id = self.tcx.map.as_local_node_id(def_id);
-
-        let (err_span, err_msg) = if Some(id) == node_id {
-            return Some((span, format!("{} is private", msg), None));
-        } else {
-            (span, format!("{} is inaccessible", msg))
-        };
-        let item = match self.tcx.map.find(id) {
-            Some(ast_map::NodeItem(item)) => {
-                match item.node {
-                    // If an impl disallowed this item, then this is resolve's
-                    // way of saying that a struct/enum's static method was
-                    // invoked, and the struct/enum itself is private. Crawl
-                    // back up the chains to find the relevant struct/enum that
-                    // was private.
-                    hir::ItemImpl(_, _, _, _, ref ty, _) => {
-                        match ty.node {
-                            hir::TyPath(..) => {}
-                            _ => return Some((err_span, err_msg, None)),
-                        };
-                        let def = self.tcx.def_map.borrow().get(&ty.id).unwrap().full_def();
-                        let did = def.def_id();
-                        let node_id = self.tcx.map.as_local_node_id(did).unwrap();
-                        match self.tcx.map.get(node_id) {
-                            ast_map::NodeItem(item) => item,
-                            _ => self.tcx.sess.span_bug(item.span,
-                                                        "path is not an item")
-                        }
-                    }
-                    _ => item
-                }
-            }
-            Some(..) | None => return Some((err_span, err_msg, None)),
-        };
-        let desc = match item.node {
-            hir::ItemMod(..) => "module",
-            hir::ItemTrait(..) => "trait",
-            hir::ItemStruct(..) => "struct",
-            hir::ItemEnum(..) => "enum",
-            _ => return Some((err_span, err_msg, None))
-        };
-        let msg = format!("{} `{}` is private", desc, item.name);
-        Some((err_span, err_msg, Some((span, msg))))
-    }
-
     // Checks that a field is in scope.
-    fn check_field(&mut self,
+    fn check_field(&mut self, span: Span, def: ty::AdtDef<'tcx>, field: ty::FieldDef<'tcx>) {
-                   span: Span,
+        if def.adt_kind() == ty::AdtKind::Struct &&
-                   def: ty::AdtDef<'tcx>,
+                field.vis != hir::Public && !self.private_accessible(def.did) {
-                   v: ty::VariantDef<'tcx>,
+            span_err!(self.tcx.sess, span, E0451, "field `{}` of struct `{}` is private",
-                   name: FieldName) {
+                      field.name, self.tcx.item_path_str(def.did));
-        let field = match name {
-            NamedField(f_name) => {
-                debug!("privacy - check named field {} in struct {:?}", f_name, def);
-                v.field_named(f_name)
-            }
-            UnnamedField(idx) => &v.fields[idx]
-        };
-        if field.vis == hir::Public || self.local_private_accessible(def.did) {
-            return;
         }
-
-        let struct_desc = match def.adt_kind() {
-            ty::AdtKind::Struct =>
-                format!("struct `{}`", self.tcx.item_path_str(def.did)),
-            // struct variant fields have inherited visibility
-            ty::AdtKind::Enum => return
-        };
-        let msg = match name {
-            NamedField(name) => format!("field `{}` of {} is private",
-                                        name, struct_desc),
-            UnnamedField(idx) => format!("field #{} of {} is private",
-                                         idx, struct_desc),
-        };
-        span_err!(self.tcx.sess, span, E0451,
-                  "{}", &msg[..]);
-    }
-
-    // Given the ID of a method, checks to ensure it's in scope.
-    fn check_static_method(&mut self,
-                           span: Span,
-                           method_id: DefId,
-                           name: ast::Name) {
-        self.report_error(self.ensure_public(span,
-                                             method_id,
-                                             None,
-                                             &format!("method `{}`",
-                                                     name)));
     }
 
     // Checks that a method is in scope.
-    fn check_method(&mut self, span: Span, method_def_id: DefId,
+    fn check_method(&mut self, span: Span, method_def_id: DefId) {
-                    name: ast::Name) {
         match self.tcx.impl_or_trait_item(method_def_id).container() {
-            ty::ImplContainer(_) => {
-                self.check_static_method(span, method_def_id, name)
-            }
             // Trait methods are always all public. The only controlling factor
             // is whether the trait itself is accessible or not.
-            ty::TraitContainer(trait_def_id) => {
+            ty::TraitContainer(trait_def_id) if !self.item_is_visible(trait_def_id) => {
-                let msg = format!("source trait `{}`", self.tcx.item_path_str(trait_def_id));
+                let msg = format!("source trait `{}` is private",
-                self.report_error(self.ensure_public(span, trait_def_id, None, &msg));
+                                  self.tcx.item_path_str(trait_def_id));
+                self.tcx.sess.span_err(span, &msg);
             }
+            _ => {}
         }
     }
 }
@@ -819,27 +438,11 @@ impl<'a, 'tcx, 'v> Visitor<'v> for PrivacyVisitor<'a, 'tcx> {
 
     fn visit_expr(&mut self, expr: &hir::Expr) {
         match expr.node {
-            hir::ExprField(ref base, name) => {
+            hir::ExprMethodCall(..) => {
-                if let ty::TyStruct(def, _) = self.tcx.expr_ty_adjusted(&base).sty {
-                    self.check_field(expr.span,
-                                     def,
-                                     def.struct_variant(),
-                                     NamedField(name.node));
-                }
-            }
-            hir::ExprTupField(ref base, idx) => {
-                if let ty::TyStruct(def, _) = self.tcx.expr_ty_adjusted(&base).sty {
-                    self.check_field(expr.span,
-                                     def,
-                                     def.struct_variant(),
-                                     UnnamedField(idx.node));
-                }
-            }
-            hir::ExprMethodCall(name, _, _) => {
                 let method_call = ty::MethodCall::expr(expr.id);
                 let method = self.tcx.tables.borrow().method_map[&method_call];
                 debug!("(privacy checking) checking impl method");
-                self.check_method(expr.span, method.def_id, name.node);
+                self.check_method(expr.span, method.def_id);
             }
             hir::ExprStruct(..) => {
                 let adt = self.tcx.expr_ty(expr).ty_adt_def().unwrap();
@@ -848,7 +451,7 @@ impl<'a, 'tcx, 'v> Visitor<'v> for PrivacyVisitor<'a, 'tcx> {
                 // Rather than computing the set of unmentioned fields
                 // (i.e. `all_fields - fields`), just check them all.
                 for field in &variant.fields {
-                    self.check_field(expr.span, adt, variant, NamedField(field.name));
+                    self.check_field(expr.span, adt, field);
                 }
             }
             hir::ExprPath(..) => {
@@ -862,7 +465,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(def.did)
+                        f.vis != hir::Public && !self.private_accessible(def.did)
                     });
                     if any_priv {
                         span_err!(self.tcx.sess, expr.span, E0450,
@@ -890,8 +493,7 @@ impl<'a, 'tcx, 'v> Visitor<'v> for PrivacyVisitor<'a, 'tcx> {
                 let def = self.tcx.def_map.borrow().get(&pattern.id).unwrap().full_def();
                 let variant = adt.variant_of_def(def);
                 for field in fields {
-                    self.check_field(pattern.span, adt, variant,
+                    self.check_field(pattern.span, adt, variant.field_named(field.node.name));
-                                     NamedField(field.node.name));
                 }
             }
 
@@ -904,10 +506,7 @@ impl<'a, 'tcx, 'v> Visitor<'v> for PrivacyVisitor<'a, 'tcx> {
                             if let PatKind::Wild = field.node {
                                 continue
                             }
-                            self.check_field(field.span,
+                            self.check_field(field.span, def, &def.struct_variant().fields[i]);
-                                             def,
-                                             def.struct_variant(),
-                                             UnnamedField(i));
                         }
                     }
                     ty::TyEnum(..) => {
@@ -1575,20 +1174,11 @@ pub fn check_crate(tcx: &TyCtxt, export_map: &def::ExportMap) -> AccessLevels {
     let mut visitor = SanePrivacyVisitor { tcx: tcx };
     krate.visit_all_items(&mut visitor);
 
-    // Figure out who everyone's parent is
-    let mut visitor = ParentVisitor {
-        tcx: tcx,
-        parents: NodeMap(),
-        curparent: ast::DUMMY_NODE_ID,
-    };
-    intravisit::walk_crate(&mut visitor, krate);
-
     // Use the parent map to check the privacy of everything
     let mut visitor = PrivacyVisitor {
         curitem: ast::DUMMY_NODE_ID,
         in_foreign: false,
         tcx: tcx,
-        parents: visitor.parents,
     };
     intravisit::walk_crate(&mut visitor, krate);
 

src/librustc_typeck/check/method/mod.rs

@@ -43,6 +43,9 @@ pub enum MethodError<'tcx> {
 
     // Using a `Fn`/`FnMut`/etc method on a raw closure type before we have inferred its kind.
     ClosureAmbiguity(/* DefId of fn trait */ DefId),
+
+    // Found an applicable method, but it is not visible.
+    PrivateMatch(Def),
 }
 
 // Contains a list of static methods that may apply, a list of unsatisfied trait predicates which
@@ -90,6 +93,7 @@ pub fn exists<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
         Err(NoMatch(..)) => false,
         Err(Ambiguity(..)) => true,
         Err(ClosureAmbiguity(..)) => true,
+        Err(PrivateMatch(..)) => true,
     }
 }
 

src/librustc_typeck/check/method/probe.rs

@@ -16,6 +16,7 @@ use super::suggest;
 use check;
 use check::{FnCtxt, UnresolvedTypeAction};
 use middle::def_id::DefId;
+use middle::def::Def;
 use rustc::ty::subst;
 use rustc::ty::subst::Subst;
 use rustc::traits;
@@ -47,6 +48,9 @@ struct ProbeContext<'a, 'tcx:'a> {
     /// used for error reporting
     static_candidates: Vec<CandidateSource>,
 
+    /// Some(candidate) if there is a private candidate
+    private_candidate: Option<Def>,
+
     /// Collects near misses when trait bounds for type parameters are unsatisfied and is only used
     /// for error reporting
     unsatisfied_predicates: Vec<TraitRef<'tcx>>
@@ -247,6 +251,7 @@ impl<'a,'tcx> ProbeContext<'a,'tcx> {
             steps: Rc::new(steps),
             opt_simplified_steps: opt_simplified_steps,
             static_candidates: Vec::new(),
+            private_candidate: None,
             unsatisfied_predicates: Vec::new(),
         }
     }
@@ -256,6 +261,7 @@ impl<'a,'tcx> ProbeContext<'a,'tcx> {
         self.extension_candidates.clear();
         self.impl_dups.clear();
         self.static_candidates.clear();
+        self.private_candidate = None;
     }
 
     fn tcx(&self) -> &'a TyCtxt<'tcx> {
@@ -407,6 +413,11 @@ impl<'a,'tcx> ProbeContext<'a,'tcx> {
             return self.record_static_candidate(ImplSource(impl_def_id));
         }
 
+        if item.vis() != hir::Public && !self.fcx.private_item_is_visible(item.def_id()) {
+            self.private_candidate = Some(item.def());
+            return
+        }
+
         let (impl_ty, impl_substs) = self.impl_ty_and_substs(impl_def_id);
         let impl_ty = impl_ty.subst(self.tcx(), &impl_substs);
 
@@ -846,6 +857,7 @@ impl<'a,'tcx> ProbeContext<'a,'tcx> {
         }
 
         let static_candidates = mem::replace(&mut self.static_candidates, vec![]);
+        let private_candidate = mem::replace(&mut self.private_candidate, None);
         let unsatisfied_predicates = mem::replace(&mut self.unsatisfied_predicates, vec![]);
 
         // things failed, so lets look at all traits, for diagnostic purposes now:
@@ -879,9 +891,13 @@ impl<'a,'tcx> ProbeContext<'a,'tcx> {
                 // this error only occurs when assembling candidates
                 tcx.sess.span_bug(span, "encountered ClosureAmbiguity from pick_core");
             }
-            None => vec![],
+            _ => vec![],
         };
 
+        if let Some(def) = private_candidate {
+            return Err(MethodError::PrivateMatch(def));
+        }
+
         Err(MethodError::NoMatch(NoMatchData::new(static_candidates, unsatisfied_predicates,
                                                   out_of_scope_traits, self.mode)))
     }

src/librustc_typeck/check/method/suggest.rs

@@ -91,7 +91,7 @@ pub fn report_error<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
         MethodError::NoMatch(NoMatchData { static_candidates: static_sources,
                                            unsatisfied_predicates,
                                            out_of_scope_traits,
-                                           mode }) => {
+                                           mode, .. }) => {
             let cx = fcx.tcx();
 
             let mut err = fcx.type_error_struct(
@@ -208,6 +208,11 @@ pub fn report_error<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
             };
             fcx.sess().span_err(span, &msg);
         }
+
+        MethodError::PrivateMatch(def) => {
+            let msg = format!("{} `{}` is private", def.kind_name(), item_name);
+            fcx.tcx().sess.span_err(span, &msg);
+        }
     }
 
     fn report_candidates(fcx: &FnCtxt,

src/librustc_typeck/check/mod.rs

@@ -2939,9 +2939,8 @@ fn check_expr_with_expectation_and_lvalue_pref<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
                             base: &'tcx hir::Expr,
                             field: &Spanned<ast::Name>) {
         check_expr_with_lvalue_pref(fcx, base, lvalue_pref);
-        let expr_t = structurally_resolved_type(fcx, expr.span,
+        let expr_t = structurally_resolved_type(fcx, expr.span, fcx.expr_ty(base));
-                                                fcx.expr_ty(base));
+        let mut private_candidate = None;
-        // FIXME(eddyb) #12808 Integrate privacy into this auto-deref loop.
         let (_, autoderefs, field_ty) = autoderef(fcx,
                                                   expr.span,
                                                   expr_t,
@@ -2949,15 +2948,17 @@ fn check_expr_with_expectation_and_lvalue_pref<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
                                                   UnresolvedTypeAction::Error,
                                                   lvalue_pref,
                                                   |base_t, _| {
-                match base_t.sty {
+                if let ty::TyStruct(base_def, substs) = base_t.sty {
-                    ty::TyStruct(base_def, substs) => {
+                    debug!("struct named {:?}",  base_t);
-                        debug!("struct named {:?}",  base_t);
+                    if let Some(field) = base_def.struct_variant().find_field_named(field.node) {
-                        base_def.struct_variant()
+                        let field_ty = fcx.field_ty(expr.span, field, substs);
-                                .find_field_named(field.node)
+                        if field.vis == hir::Public || fcx.private_item_is_visible(base_def.did) {
-                                .map(|f| fcx.field_ty(expr.span, f, substs))
+                            return Some(field_ty);
+                        }
+                        private_candidate = Some((base_def.did, field_ty));
                     }
-                    _ => None
                 }
+                None
             });
         match field_ty {
             Some(field_ty) => {
@@ -2968,12 +2969,14 @@ fn check_expr_with_expectation_and_lvalue_pref<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
             None => {}
         }
 
-        if field.node == special_idents::invalid.name {
+        if let Some((did, field_ty)) = private_candidate {
+            let struct_path = fcx.tcx().item_path_str(did);
+            let msg = format!("field `{}` of struct `{}` is private", field.node, struct_path);
+            fcx.tcx().sess.span_err(expr.span, &msg);
+            fcx.write_ty(expr.id, field_ty);
+        } else if field.node == special_idents::invalid.name {
             fcx.write_error(expr.id);
-            return;
+        } else if method::exists(fcx, field.span, field.node, expr_t, expr.id) {
-        }
-
-        if method::exists(fcx, field.span, field.node, expr_t, expr.id) {
             fcx.type_error_struct(field.span,
                                   |actual| {
                                        format!("attempted to take value of method `{}` on type \
@@ -2984,6 +2987,7 @@ fn check_expr_with_expectation_and_lvalue_pref<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
                                "maybe a `()` to call it is missing? \
                                If not, try an anonymous function")
                 .emit();
+            fcx.write_error(expr.id);
         } else {
             let mut err = fcx.type_error_struct(
                 expr.span,
@@ -2999,9 +3003,8 @@ fn check_expr_with_expectation_and_lvalue_pref<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
                 suggest_field_names(&mut err, def.struct_variant(), field, vec![]);
             }
             err.emit();
+            fcx.write_error(expr.id);
         }
-
-        fcx.write_error(expr.id);
     }
 
     // displays hints about the closest matches in field names
@@ -3036,10 +3039,9 @@ fn check_expr_with_expectation_and_lvalue_pref<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
                                 base: &'tcx hir::Expr,
                                 idx: codemap::Spanned<usize>) {
         check_expr_with_lvalue_pref(fcx, base, lvalue_pref);
-        let expr_t = structurally_resolved_type(fcx, expr.span,
+        let expr_t = structurally_resolved_type(fcx, expr.span, fcx.expr_ty(base));
-                                                fcx.expr_ty(base));
+        let mut private_candidate = None;
         let mut tuple_like = false;
-        // FIXME(eddyb) #12808 Integrate privacy into this auto-deref loop.
         let (_, autoderefs, field_ty) = autoderef(fcx,
                                                   expr.span,
                                                   expr_t,
@@ -3047,25 +3049,27 @@ fn check_expr_with_expectation_and_lvalue_pref<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
                                                   UnresolvedTypeAction::Error,
                                                   lvalue_pref,
                                                   |base_t, _| {
-                match base_t.sty {
+                let (base_def, substs) = match base_t.sty {
-                    ty::TyStruct(base_def, substs) => {
+                    ty::TyStruct(base_def, substs) => (base_def, substs),
-                        tuple_like = base_def.struct_variant().is_tuple_struct();
-                        if tuple_like {
-                            debug!("tuple struct named {:?}",  base_t);
-                            base_def.struct_variant()
-                                    .fields
-                                    .get(idx.node)
-                                    .map(|f| fcx.field_ty(expr.span, f, substs))
-                        } else {
-                            None
-                        }
-                    }
                     ty::TyTuple(ref v) => {
                         tuple_like = true;
-                        if idx.node < v.len() { Some(v[idx.node]) } else { None }
+                        return if idx.node < v.len() { Some(v[idx.node]) } else { None }
                     }
-                    _ => None
+                    _ => return None,
+                };
+
+                tuple_like = base_def.struct_variant().is_tuple_struct();
+                if !tuple_like { return None }
+
+                debug!("tuple struct named {:?}",  base_t);
+                if let Some(field) = base_def.struct_variant().fields.get(idx.node) {
+                    let field_ty = fcx.field_ty(expr.span, field, substs);
+                    if field.vis == hir::Public || fcx.private_item_is_visible(base_def.did) {
+                        return Some(field_ty);
+                    }
+                    private_candidate = Some((base_def.did, field_ty));
                 }
+                None
             });
         match field_ty {
             Some(field_ty) => {
@@ -3075,6 +3079,15 @@ fn check_expr_with_expectation_and_lvalue_pref<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
             }
             None => {}
         }
+
+        if let Some((did, field_ty)) = private_candidate {
+            let struct_path = fcx.tcx().item_path_str(did);
+            let msg = format!("field `{}` of struct `{}` is private", idx.node, struct_path);
+            fcx.tcx().sess.span_err(expr.span, &msg);
+            fcx.write_ty(expr.id, field_ty);
+            return;
+        }
+
         fcx.type_error_message(
             expr.span,
             |actual| {
@@ -3745,23 +3758,30 @@ pub fn resolve_ty_and_def_ufcs<'a, 'b, 'tcx>(fcx: &FnCtxt<'b, 'tcx>,
                                                      &ty_segments[base_ty_end..]);
         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) {
+        let def = match method::resolve_ufcs(fcx, span, item_name, ty, node_id) {
-            Ok(def) => {
+            Ok(def) => Some(def),
-                // Write back the new resolution.
-                fcx.ccx.tcx.def_map.borrow_mut()
-                       .insert(node_id, def::PathResolution {
-                   base_def: def,
-                   depth: 0
-                });
-                Some((Some(ty), slice::ref_slice(item_segment), def))
-            }
             Err(error) => {
+                let def = match error {
+                    method::MethodError::PrivateMatch(def) => Some(def),
+                    _ => None,
+                };
                 if item_name != special_idents::invalid.name {
                     method::report_error(fcx, span, ty, item_name, None, error);
                 }
-                fcx.write_error(node_id);
+                def
-                None
             }
+        };
+
+        if let Some(def) = def {
+            // Write back the new resolution.
+            fcx.ccx.tcx.def_map.borrow_mut().insert(node_id, def::PathResolution {
+                base_def: def,
+                depth: 0,
+            });
+            Some((Some(ty), slice::ref_slice(item_segment), def))
+        } else {
+            fcx.write_error(node_id);
+            None
         }
     }
 }

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

@@ -0,0 +1,28 @@
+// 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 struct Foo;
+    impl Foo {
+        fn bar(&self) {}
+    }
+
+    pub trait Baz {
+        fn bar(&self) -> bool {}
+    }
+    impl Baz for Foo {}
+}
+
+fn main() {
+    use foo::Baz;
+
+    // Check that `bar` resolves to the trait method, not the inherent impl method.
+    let _: () = foo::Foo.bar(); //~ ERROR mismatched types
+}

src/test/compile-fail/privacy1.rs

@@ -102,6 +102,7 @@ mod foo {
                                 //~^ ERROR: method `bar` is private
         ::bar::baz::A.foo2();   //~ ERROR: module `baz` is private
         ::bar::baz::A.bar2();   //~ ERROR: module `baz` is private
+                                //~^ ERROR: method `bar2` is private
 
         let _: isize =
         ::bar::B::foo();        //~ ERROR: trait `B` is private

src/test/compile-fail/privacy5.rs

@@ -63,25 +63,25 @@ fn this_crate() {
     let c = a::C(2, 3); //~ ERROR: cannot invoke tuple struct constructor
     let d = a::D(4);
 
-    let a::A(()) = a; //~ ERROR: field #0 of struct `a::A` is private
+    let a::A(()) = a; //~ ERROR: field `0` of struct `a::A` is private
     let a::A(_) = a;
-    match a { a::A(()) => {} } //~ ERROR: field #0 of struct `a::A` is private
+    match a { a::A(()) => {} } //~ ERROR: field `0` of struct `a::A` is private
     match a { a::A(_) => {} }
 
     let a::B(_) = b;
-    let a::B(_b) = b; //~ ERROR: field #0 of struct `a::B` is private
+    let a::B(_b) = b; //~ ERROR: field `0` of struct `a::B` is private
     match b { a::B(_) => {} }
-    match b { a::B(_b) => {} } //~ ERROR: field #0 of struct `a::B` is private
+    match b { a::B(_b) => {} } //~ ERROR: field `0` of struct `a::B` is private
-    match b { a::B(1) => {} a::B(_) => {} } //~ ERROR: field #0 of struct `a::B` is private
+    match b { a::B(1) => {} a::B(_) => {} } //~ ERROR: field `0` of struct `a::B` is private
 
     let a::C(_, _) = c;
     let a::C(_a, _) = c;
-    let a::C(_, _b) = c; //~ ERROR: field #1 of struct `a::C` is private
+    let a::C(_, _b) = c; //~ ERROR: field `1` of struct `a::C` is private
-    let a::C(_a, _b) = c; //~ ERROR: field #1 of struct `a::C` is private
+    let a::C(_a, _b) = c; //~ ERROR: field `1` of struct `a::C` is private
     match c { a::C(_, _) => {} }
     match c { a::C(_a, _) => {} }
-    match c { a::C(_, _b) => {} } //~ ERROR: field #1 of struct `a::C` is private
+    match c { a::C(_, _b) => {} } //~ ERROR: field `1` of struct `a::C` is private
-    match c { a::C(_a, _b) => {} } //~ ERROR: field #1 of struct `a::C` is private
+    match c { a::C(_a, _b) => {} } //~ ERROR: field `1` of struct `a::C` is private
 
     let a::D(_) = d;
     let a::D(_d) = d;
@@ -101,30 +101,30 @@ fn xcrate() {
     let c = other::C(2, 3); //~ ERROR: cannot invoke tuple struct constructor
     let d = other::D(4);
 
-    let other::A(()) = a; //~ ERROR: field #0 of struct `other::A` is private
+    let other::A(()) = a; //~ ERROR: field `0` of struct `other::A` is private
     let other::A(_) = a;
     match a { other::A(()) => {} }
-    //~^ ERROR: field #0 of struct `other::A` is private
+    //~^ ERROR: field `0` of struct `other::A` is private
     match a { other::A(_) => {} }
 
     let other::B(_) = b;
-    let other::B(_b) = b; //~ ERROR: field #0 of struct `other::B` is private
+    let other::B(_b) = b; //~ ERROR: field `0` of struct `other::B` is private
     match b { other::B(_) => {} }
     match b { other::B(_b) => {} }
-    //~^ ERROR: field #0 of struct `other::B` is private
+    //~^ ERROR: field `0` of struct `other::B` is private
     match b { other::B(1) => {} other::B(_) => {} }
-    //~^ ERROR: field #0 of struct `other::B` is private
+    //~^ ERROR: field `0` of struct `other::B` is private
 
     let other::C(_, _) = c;
     let other::C(_a, _) = c;
-    let other::C(_, _b) = c; //~ ERROR: field #1 of struct `other::C` is private
+    let other::C(_, _b) = c; //~ ERROR: field `1` of struct `other::C` is private
-    let other::C(_a, _b) = c; //~ ERROR: field #1 of struct `other::C` is private
+    let other::C(_a, _b) = c; //~ ERROR: field `1` of struct `other::C` is private
     match c { other::C(_, _) => {} }
     match c { other::C(_a, _) => {} }
     match c { other::C(_, _b) => {} }
-    //~^ ERROR: field #1 of struct `other::C` is private
+    //~^ ERROR: field `1` of struct `other::C` is private
     match c { other::C(_a, _b) => {} }
-    //~^ ERROR: field #1 of struct `other::C` is private
+    //~^ ERROR: field `1` of struct `other::C` is private
 
     let other::D(_) = d;
     let other::D(_d) = d;

src/test/compile-fail/regions-glb-free-free.rs

@@ -11,7 +11,7 @@
 mod argparse {
     pub struct Flag<'a> {
         name: &'a str,
-        desc: &'a str,
+        pub desc: &'a str,
         max_count: usize,
         value: usize
     }

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

@@ -25,9 +25,10 @@ mod inner {
         pub a: isize,
         b: isize,
     }
+    pub struct Z(pub isize, isize);
 }
 
-fn test(a: A, b: inner::A, c: inner::B, d: xc::A, e: xc::B) {
+fn test(a: A, b: inner::A, c: inner::B, d: xc::A, e: xc::B, z: inner::Z) {
     a.a;
     b.a; //~ ERROR: field `a` of struct `inner::A` is private
     b.b;
@@ -39,6 +40,9 @@ fn test(a: A, b: inner::A, c: inner::B, d: xc::A, e: xc::B) {
 
     e.a;
     e.b; //~ ERROR: field `b` of struct `xc::B` is private
+
+    z.0;
+    z.1; //~ ERROR: field `1` of struct `inner::Z` is private
 }
 
 fn main() {}

src/test/parse-fail/pub-method-macro.rs

@@ -29,6 +29,4 @@ mod bleh {
     }
 }
 
-fn main() {
+fn main() {}
-    bleh::S.f();
-}

src/test/run-pass/autoderef-privacy.rs

@@ -0,0 +1,60 @@
+// 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.
+
+// Check we do not select a private method or field when computing autoderefs
+
+#![allow(unused)]
+
+#[derive(Default)]
+pub struct Bar2 { i: i32 }
+#[derive(Default)]
+pub struct Baz2(i32);
+
+impl Bar2 {
+    fn f(&self) -> bool { true }
+}
+
+mod foo {
+    #[derive(Default)]
+    pub struct Bar { i: ::Bar2 }
+    #[derive(Default)]
+    pub struct Baz(::Baz2);
+
+    impl Bar {
+        fn f(&self) -> bool { false }
+    }
+
+    impl ::std::ops::Deref for Bar {
+        type Target = ::Bar2;
+        fn deref(&self) -> &::Bar2 { &self.i }
+    }
+
+    impl ::std::ops::Deref for Baz {
+        type Target = ::Baz2;
+        fn deref(&self) -> &::Baz2 { &self.0 }
+    }
+
+    pub fn f(bar: &Bar, baz: &Baz) {
+        // Since the private fields and methods are visible here, there should be no autoderefs.
+        let _: &::Bar2 = &bar.i;
+        let _: &::Baz2 = &baz.0;
+        assert!(!bar.f());
+    }
+}
+
+fn main() {
+    let bar = foo::Bar::default();
+    let baz = foo::Baz::default();
+    foo::f(&bar, &baz);
+
+    let _: i32 = bar.i;
+    let _: i32 = baz.0;
+    assert!(bar.f());
+}