]> git.lizzy.rs Git - rust.git/blobdiff - src/librustc_privacy/lib.rs
privacy: change def_privacy so that it checks for visiblity instead of nameability
[rust.git] / src / librustc_privacy / lib.rs
index 1424616e792f64ddaa732d7bd09696e938967d10..226539b8e804a864a1a09b5fa1125d1510bf5bef 100644 (file)
 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::middle::ty::{self, TyCtxt};
 use rustc::util::nodemap::{NodeMap, NodeSet};
 use rustc::front::map as ast_map;
 
@@ -66,7 +63,7 @@
 ////////////////////////////////////////////////////////////////////////////////
 
 struct ParentVisitor<'a, 'tcx:'a> {
-    tcx: &'a ty::ctxt<'tcx>,
+    tcx: &'a TyCtxt<'tcx>,
     parents: NodeMap<ast::NodeId>,
     curparent: ast::NodeId,
 }
@@ -147,7 +144,7 @@ fn visit_variant_data(&mut self, s: &hir::VariantData, _: ast::Name,
         // 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.node.id, self.curparent);
+            self.parents.insert(field.id, self.curparent);
         }
         intravisit::walk_struct_def(self, s)
     }
@@ -158,7 +155,7 @@ fn visit_variant_data(&mut self, s: &hir::VariantData, _: ast::Name,
 ////////////////////////////////////////////////////////////////////////////////
 
 struct EmbargoVisitor<'a, 'tcx: 'a> {
-    tcx: &'a ty::ctxt<'tcx>,
+    tcx: &'a TyCtxt<'tcx>,
     export_map: &'a def::ExportMap,
 
     // Accessibility levels for reachable nodes
@@ -169,6 +166,10 @@ struct EmbargoVisitor<'a, 'tcx: 'a> {
     changed: bool,
 }
 
+struct ReachEverythingInTheInterfaceVisitor<'b, 'a: 'b, 'tcx: 'a> {
+    ev: &'b mut EmbargoVisitor<'a, 'tcx>,
+}
+
 impl<'a, 'tcx> EmbargoVisitor<'a, 'tcx> {
     fn ty_level(&self, ty: &hir::Ty) -> Option<AccessLevel> {
         if let hir::TyPath(..) = ty.node {
@@ -214,6 +215,10 @@ fn update(&mut self, id: ast::NodeId, level: Option<AccessLevel>) -> Option<Acce
             old_level
         }
     }
+
+    fn reach<'b>(&'b mut self) -> ReachEverythingInTheInterfaceVisitor<'b, 'a, 'tcx> {
+        ReachEverythingInTheInterfaceVisitor { ev: self }
+    }
 }
 
 impl<'a, 'tcx, 'v> Visitor<'v> for EmbargoVisitor<'a, 'tcx> {
@@ -245,16 +250,16 @@ fn visit_item(&mut self, item: &hir::Item) {
             }
         };
 
-        // Update id of the item itself
+        // Update level of the item itself
         let item_level = self.update(item.id, inherited_item_level);
 
-        // Update ids of nested things
+        // Update levels of nested things
         match item.node {
             hir::ItemEnum(ref def, _) => {
                 for variant in &def.variants {
                     let variant_level = self.update(variant.node.data.id(), item_level);
                     for field in variant.node.data.fields() {
-                        self.update(field.node.id, variant_level);
+                        self.update(field.id, variant_level);
                     }
                 }
             }
@@ -280,8 +285,8 @@ fn visit_item(&mut self, item: &hir::Item) {
                     self.update(def.id(), item_level);
                 }
                 for field in def.fields() {
-                    if field.node.kind.visibility() == hir::Public {
-                        self.update(field.node.id, item_level);
+                    if field.vis == hir::Public {
+                        self.update(field.id, item_level);
                     }
                 }
             }
@@ -292,19 +297,72 @@ fn visit_item(&mut self, item: &hir::Item) {
                     }
                 }
             }
-            hir::ItemTy(ref ty, _) if item_level.is_some() => {
-                if let hir::TyPath(..) = ty.node {
-                    match self.tcx.def_map.borrow().get(&ty.id).unwrap().full_def() {
-                        Def::PrimTy(..) | Def::SelfTy(..) | Def::TyParam(..) => {},
-                        def => {
-                            if let Some(node_id) = self.tcx.map.as_local_node_id(def.def_id()) {
-                                self.update(node_id, Some(AccessLevel::Reachable));
-                            }
+            _ => {}
+        }
+
+        // Mark all items in interfaces of reachable items as reachable
+        match item.node {
+            // The interface is empty
+            hir::ItemExternCrate(..) => {}
+            // All nested items are checked by visit_item
+            hir::ItemMod(..) => {}
+            // Reexports are handled in visit_mod
+            hir::ItemUse(..) => {}
+            // Visit everything
+            hir::ItemConst(..) | hir::ItemStatic(..) | hir::ItemFn(..) |
+            hir::ItemTrait(..) | hir::ItemTy(..) | hir::ItemImpl(_, _, _, Some(..), _, _) => {
+                if item_level.is_some() {
+                    self.reach().visit_item(item);
+                }
+            }
+            // Visit everything, but enum variants have their own levels
+            hir::ItemEnum(ref def, ref generics) => {
+                if item_level.is_some() {
+                    self.reach().visit_generics(generics);
+                }
+                for variant in &def.variants {
+                    if self.get(variant.node.data.id()).is_some() {
+                        for field in variant.node.data.fields() {
+                            self.reach().visit_struct_field(field);
+                        }
+                        // Corner case: if the variant is reachable, but its
+                        // enum is not, make the enum reachable as well.
+                        self.update(item.id, Some(AccessLevel::Reachable));
+                    }
+                }
+            }
+            // Visit everything, but foreign items have their own levels
+            hir::ItemForeignMod(ref foreign_mod) => {
+                for foreign_item in &foreign_mod.items {
+                    if self.get(foreign_item.id).is_some() {
+                        self.reach().visit_foreign_item(foreign_item);
+                    }
+                }
+            }
+            // Visit everything except for private fields
+            hir::ItemStruct(ref struct_def, ref generics) => {
+                if item_level.is_some() {
+                    self.reach().visit_generics(generics);
+                    for field in struct_def.fields() {
+                        if self.get(field.id).is_some() {
+                            self.reach().visit_struct_field(field);
+                        }
+                    }
+                }
+            }
+            // The interface is empty
+            hir::ItemDefaultImpl(..) => {}
+            // Visit everything except for private impl items
+            hir::ItemImpl(_, _, ref generics, None, _, ref impl_items) => {
+                if item_level.is_some() {
+                    self.reach().visit_generics(generics);
+                    for impl_item in impl_items {
+                        if self.get(impl_item.id).is_some() {
+                            self.reach().visit_impl_item(impl_item);
                         }
                     }
                 }
             }
-            _ => {}
         }
 
         let orig_level = self.prev_level;
@@ -347,12 +405,74 @@ fn visit_macro_def(&mut self, md: &'v hir::MacroDef) {
     }
 }
 
+impl<'b, 'a, 'tcx: 'a> ReachEverythingInTheInterfaceVisitor<'b, 'a, 'tcx> {
+    // Make the type hidden under a type alias reachable
+    fn reach_aliased_type(&mut self, item: &hir::Item, path: &hir::Path) {
+        if let hir::ItemTy(ref ty, ref generics) = item.node {
+            // See `fn is_public_type_alias` for details
+            self.visit_ty(ty);
+            let provided_params = path.segments.last().unwrap().parameters.types().len();
+            for ty_param in &generics.ty_params[provided_params..] {
+                if let Some(ref default_ty) = ty_param.default {
+                    self.visit_ty(default_ty);
+                }
+            }
+        }
+    }
+}
+
+impl<'b, 'a, 'tcx: 'a, 'v> Visitor<'v> for ReachEverythingInTheInterfaceVisitor<'b, 'a, 'tcx> {
+    fn visit_ty(&mut self, ty: &hir::Ty) {
+        if let hir::TyPath(_, ref path) = ty.node {
+            let def = self.ev.tcx.def_map.borrow().get(&ty.id).unwrap().full_def();
+            match def {
+                Def::Struct(def_id) | Def::Enum(def_id) | Def::TyAlias(def_id) |
+                Def::Trait(def_id) | Def::AssociatedTy(def_id, _) => {
+                    if let Some(node_id) = self.ev.tcx.map.as_local_node_id(def_id) {
+                        let item = self.ev.tcx.map.expect_item(node_id);
+                        if let Def::TyAlias(..) = def {
+                            // Type aliases are substituted. Associated type aliases are not
+                            // substituted yet, but ideally they should be.
+                            if self.ev.get(item.id).is_none() {
+                                self.reach_aliased_type(item, path);
+                            }
+                        } else {
+                            self.ev.update(item.id, Some(AccessLevel::Reachable));
+                        }
+                    }
+                }
+
+                _ => {}
+            }
+        }
+
+        intravisit::walk_ty(self, ty);
+    }
+
+    fn visit_trait_ref(&mut self, trait_ref: &hir::TraitRef) {
+        let def_id = self.ev.tcx.trait_ref_to_def_id(trait_ref);
+        if let Some(node_id) = self.ev.tcx.map.as_local_node_id(def_id) {
+            let item = self.ev.tcx.map.expect_item(node_id);
+            self.ev.update(item.id, Some(AccessLevel::Reachable));
+        }
+
+        intravisit::walk_trait_ref(self, trait_ref);
+    }
+
+    // Don't recurse into function bodies
+    fn visit_block(&mut self, _: &hir::Block) {}
+    // Don't recurse into expressions in array sizes or const initializers
+    fn visit_expr(&mut self, _: &hir::Expr) {}
+    // Don't recurse into patterns in function arguments
+    fn visit_pat(&mut self, _: &hir::Pat) {}
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 /// The privacy visitor, where privacy checks take place (violations reported)
 ////////////////////////////////////////////////////////////////////////////////
 
 struct PrivacyVisitor<'a, 'tcx: 'a> {
-    tcx: &'a ty::ctxt<'tcx>,
+    tcx: &'a TyCtxt<'tcx>,
     curitem: ast::NodeId,
     in_foreign: bool,
     parents: NodeMap<ast::NodeId>,
@@ -372,11 +492,6 @@ enum FieldName {
 }
 
 impl<'a, 'tcx> PrivacyVisitor<'a, 'tcx> {
-    // used when debugging
-    fn nodestr(&self, id: ast::NodeId) -> String {
-        self.tcx.map.node_to_string(id).to_string()
-    }
-
     // Determines whether the given definition is public from the point of view
     // of the current item.
     fn def_privacy(&self, did: DefId) -> PrivacyResult {
@@ -484,75 +599,50 @@ fn def_privacy(&self, did: DefId) -> PrivacyResult {
             return Allowable;
         }
 
-        // We now know that there is at least one private member between the
-        // destination and the root.
-        let mut closest_private_id = node_id;
-        loop {
-            debug!("privacy - examining {}", self.nodestr(closest_private_id));
-            let vis = match self.tcx.map.find(closest_private_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.
-                // FIXME(#10573) is this the right behavior? Why not consider
-                //               where the method was defined?
-                Some(ast_map::NodeImplItem(ii)) => {
-                    match ii.node {
-                        hir::ImplItemKind::Const(..) |
-                        hir::ImplItemKind::Method(..) => {
-                            let imp = self.tcx.map
-                                          .get_parent_did(closest_private_id);
-                            match self.tcx.impl_trait_ref(imp) {
-                                Some(..) => return Allowable,
-                                _ if ii.vis == hir::Public => {
-                                    return Allowable
-                                }
-                                _ => ii.vis
-                            }
+        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)) => {
+                match ii.node {
+                    hir::ImplItemKind::Const(..) |
+                    hir::ImplItemKind::Method(..) => {
+                        let imp = self.tcx.map.get_parent_did(node_id);
+                        match self.tcx.impl_trait_ref(imp) {
+                            Some(..) => hir::Public,
+                            _ => ii.vis
                         }
-                        hir::ImplItemKind::Type(_) => return Allowable,
                     }
+                    hir::ImplItemKind::Type(_) => hir::Public,
                 }
-                Some(ast_map::NodeTraitItem(_)) => {
-                    return Allowable;
-                }
+            }
+            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(closest_private_id)
-                }
-                Some(ast_map::NodeVariant(..)) => {
-                    hir::Public // need to move up a level (to the enum)
-                }
-                _ => hir::Public,
-            };
-            if vis != hir::Public { break }
-            // if we've reached the root, then everything was allowable and this
-            // access is public.
-            if closest_private_id == ast::CRATE_NODE_ID { return Allowable }
-            closest_private_id = *self.parents.get(&closest_private_id).unwrap();
-
-            // If we reached the top, then we were public all the way down and
-            // we can allow this access.
-            if closest_private_id == ast::DUMMY_NODE_ID { return Allowable }
-        }
-        debug!("privacy - closest priv {}", self.nodestr(closest_private_id));
-        if self.private_accessible(closest_private_id) {
+            // 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(closest_private_id)
+            DisallowedBy(node_id)
         }
     }
 
@@ -569,32 +659,7 @@ fn local_private_accessible(&self, did: DefId) -> bool {
     /// 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();
-        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();
-        }
+        self.tcx.map.private_item_is_visible_from(id, self.curitem)
     }
 
     fn report_error(&self, result: CheckResult) -> bool {
@@ -640,6 +705,7 @@ fn ensure_public(&self,
         // 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 {
@@ -696,7 +762,7 @@ fn check_field(&mut self,
             }
             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;
         }
 
@@ -710,7 +776,7 @@ fn check_field(&mut self,
             NamedField(name) => format!("field `{}` of {} is private",
                                         name, struct_desc),
             UnnamedField(idx) => format!("field #{} of {} is private",
-                                         idx + 1, struct_desc),
+                                         idx, struct_desc),
         };
         span_err!(self.tcx.sess, span, E0451,
                   "{}", &msg[..]);
@@ -728,100 +794,6 @@ fn check_static_method(&mut self,
                                                      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) {
@@ -897,7 +869,7 @@ fn visit_expr(&mut self, expr: &hir::Expr) {
                         _ => 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,
@@ -963,25 +935,6 @@ fn visit_foreign_item(&mut self, fi: &hir::ForeignItem) {
         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);
-    }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -989,44 +942,20 @@ fn visit_path_list_item(&mut self, prefix: &hir::Path, item: &hir::PathListItem)
 ////////////////////////////////////////////////////////////////////////////////
 
 struct SanePrivacyVisitor<'a, 'tcx: 'a> {
-    tcx: &'a ty::ctxt<'tcx>,
-    in_block: bool,
+    tcx: &'a TyCtxt<'tcx>,
 }
 
 impl<'a, 'tcx, 'v> Visitor<'v> for SanePrivacyVisitor<'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.check_sane_privacy(item);
-        if self.in_block {
-            self.check_all_inherited(item);
-        }
-
-        let orig_in_block = self.in_block;
-
-        // Modules turn privacy back on, otherwise we inherit
-        self.in_block = if let hir::ItemMod(..) = item.node { false } else { orig_in_block };
-
         intravisit::walk_item(self, item);
-        self.in_block = orig_in_block;
-    }
-
-    fn visit_block(&mut self, b: &'v hir::Block) {
-        let orig_in_block = replace(&mut self.in_block, true);
-        intravisit::walk_block(self, b);
-        self.in_block = orig_in_block;
     }
 }
 
 impl<'a, 'tcx> SanePrivacyVisitor<'a, 'tcx> {
-    /// Validates all of the visibility qualifiers placed on the item given. This
-    /// ensures that there are no extraneous qualifiers that don't actually do
-    /// anything. In theory these qualifiers wouldn't parse, but that may happen
-    /// later on down the road...
+    /// Validate that items that shouldn't have visibility qualifiers don't have them.
+    /// Such qualifiers can be set by syntax extensions even if the parser doesn't allow them,
+    /// so we check things like variant fields too.
     fn check_sane_privacy(&self, item: &hir::Item) {
         let check_inherited = |sp, vis, note: &str| {
             if vis != hir::Inherited {
@@ -1040,13 +969,12 @@ fn check_sane_privacy(&self, item: &hir::Item) {
         };
 
         match item.node {
-            // implementations of traits don't need visibility qualifiers because
-            // that's controlled by having the trait in scope.
             hir::ItemImpl(_, _, _, Some(..), _, ref impl_items) => {
                 check_inherited(item.span, item.vis,
                                 "visibility qualifiers have no effect on trait impls");
                 for impl_item in impl_items {
-                    check_inherited(impl_item.span, impl_item.vis, "");
+                    check_inherited(impl_item.span, impl_item.vis,
+                                    "visibility qualifiers have no effect on trait impl items");
                 }
             }
             hir::ItemImpl(_, _, _, None, _, _) => {
@@ -1061,41 +989,15 @@ fn check_sane_privacy(&self, item: &hir::Item) {
                 check_inherited(item.span, item.vis,
                                 "place qualifiers on individual functions instead");
             }
-            hir::ItemStruct(..) | hir::ItemEnum(..) | hir::ItemTrait(..) |
-            hir::ItemConst(..) | hir::ItemStatic(..) | hir::ItemFn(..) |
-            hir::ItemMod(..) | hir::ItemExternCrate(..) |
-            hir::ItemUse(..) | hir::ItemTy(..) => {}
-        }
-    }
-
-    /// When inside of something like a function or a method, visibility has no
-    /// control over anything so this forbids any mention of any visibility
-    fn check_all_inherited(&self, item: &hir::Item) {
-        let check_inherited = |sp, vis| {
-            if vis != hir::Inherited {
-                span_err!(self.tcx.sess, sp, E0447,
-                          "visibility has no effect inside functions or block expressions");
-            }
-        };
-
-        check_inherited(item.span, item.vis);
-        match item.node {
-            hir::ItemImpl(_, _, _, _, _, ref impl_items) => {
-                for impl_item in impl_items {
-                    check_inherited(impl_item.span, impl_item.vis);
-                }
-            }
-            hir::ItemForeignMod(ref fm) => {
-                for fi in &fm.items {
-                    check_inherited(fi.span, fi.vis);
-                }
-            }
-            hir::ItemStruct(ref vdata, _) => {
-                for f in vdata.fields() {
-                    check_inherited(f.span, f.node.kind.visibility());
+            hir::ItemEnum(ref def, _) => {
+                for variant in &def.variants {
+                    for field in variant.node.data.fields() {
+                        check_inherited(field.span, field.vis,
+                                        "visibility qualifiers have no effect on variant fields");
+                    }
                 }
             }
-            hir::ItemDefaultImpl(..) | hir::ItemEnum(..) | hir::ItemTrait(..) |
+            hir::ItemStruct(..) | hir::ItemTrait(..) |
             hir::ItemConst(..) | hir::ItemStatic(..) | hir::ItemFn(..) |
             hir::ItemMod(..) | hir::ItemExternCrate(..) |
             hir::ItemUse(..) | hir::ItemTy(..) => {}
@@ -1111,7 +1013,7 @@ fn check_all_inherited(&self, item: &hir::Item) {
 ///////////////////////////////////////////////////////////////////////////////
 
 struct ObsoleteVisiblePrivateTypesVisitor<'a, 'tcx: 'a> {
-    tcx: &'a ty::ctxt<'tcx>,
+    tcx: &'a TyCtxt<'tcx>,
     access_levels: &'a AccessLevels,
     in_variant: bool,
     // set of errors produced by this obsolete visitor
@@ -1426,10 +1328,7 @@ fn visit_variant(&mut self, v: &hir::Variant, g: &hir::Generics, item_id: ast::N
     }
 
     fn visit_struct_field(&mut self, s: &hir::StructField) {
-        let vis = match s.node.kind {
-            hir::NamedField(_, vis) | hir::UnnamedField(vis) => vis
-        };
-        if vis == hir::Public || self.in_variant {
+        if s.vis == hir::Public || self.in_variant {
             intravisit::walk_struct_field(self, s);
         }
     }
@@ -1453,7 +1352,7 @@ fn visit_expr(&mut self, _: &hir::Expr) {}
 ///////////////////////////////////////////////////////////////////////////////
 
 struct SearchInterfaceForPrivateItemsVisitor<'a, 'tcx: 'a> {
-    tcx: &'a ty::ctxt<'tcx>,
+    tcx: &'a TyCtxt<'tcx>,
     // Do not report an error when a private type is found
     is_quiet: bool,
     // Is private component found?
@@ -1584,7 +1483,7 @@ fn visit_pat(&mut self, _: &hir::Pat) {}
 }
 
 struct PrivateItemsInPublicInterfacesVisitor<'a, 'tcx: 'a> {
-    tcx: &'a ty::ctxt<'tcx>,
+    tcx: &'a TyCtxt<'tcx>,
     old_error_set: &'a NodeSet,
 }
 
@@ -1640,7 +1539,7 @@ fn visit_item(&mut self, item: &hir::Item) {
                 if item.vis == hir::Public {
                     check.visit_generics(generics);
                     for field in struct_def.fields() {
-                        if field.node.kind.visibility() == hir::Public {
+                        if field.vis == hir::Public {
                             check.visit_struct_field(field);
                         }
                     }
@@ -1674,7 +1573,7 @@ fn visit_item(&mut self, item: &hir::Item) {
     }
 }
 
-pub fn check_crate(tcx: &ty::ctxt,
+pub fn check_crate(tcx: &TyCtxt,
                    export_map: &def::ExportMap,
                    external_exports: ExternalExports)
                    -> AccessLevels {
@@ -1682,13 +1581,9 @@ pub fn check_crate(tcx: &ty::ctxt,
 
     let krate = tcx.map.krate();
 
-    // Sanity check to make sure that all privacy usage and controls are
-    // reasonable.
-    let mut visitor = SanePrivacyVisitor {
-        tcx: tcx,
-        in_block: false,
-    };
-    intravisit::walk_crate(&mut visitor, krate);
+    // Sanity check to make sure that all privacy usage is reasonable.
+    let mut visitor = SanePrivacyVisitor { tcx: tcx };
+    krate.visit_all_items(&mut visitor);
 
     // Figure out who everyone's parent is
     let mut visitor = ParentVisitor {