Auto merge of #51866 - zackmdavis:hir_making_each_day_of_the_year, r=petrochenkov

add modifier keyword spans to hir::Visibility; improve unreachable-pub, private-no-mangle lint suggestions

#50455 pointed out that the unreachable-pub suggestion for brace-grouped `use`s was bogus; #50476 partially ameliorated this by marking the suggestion as `Applicability::MaybeIncorrect`, but this is the actual fix.

Meanwhile, another application of having spans available in `hir::Visibility` is found in the private-no-mangle lints, where we can now issue a suggestion to use `pub` if the item has a more restricted visibility marker (this seems much less likely to come up in practice than not having any visibility keyword at all, but thoroughness is a virtue). While we're there, we can also add a helpful note if the item does have a `pub` (but triggered the lint presumably because enclosing modules were private).

![hir_vis](https://user-images.githubusercontent.com/1076988/42018064-ca830290-7a65-11e8-9c4c-48bc846f861f.png)

r? @nrc
cc @Manishearth

Author: bors

src/librustc/hir/intravisit.rs

@@ -1104,7 +1104,7 @@ pub fn walk_arm<'v, V: Visitor<'v>>(visitor: &mut V, arm: &'v Arm) {
 }
 
 pub fn walk_vis<'v, V: Visitor<'v>>(visitor: &mut V, vis: &'v Visibility) {
-    if let Visibility::Restricted { ref path, id } = *vis {
+    if let VisibilityKind::Restricted { ref path, id } = vis.node {
         visitor.visit_id(id);
         visitor.visit_path(path, id)
     }

src/librustc/hir/lowering.rs

@@ -1296,7 +1296,7 @@ impl<'a> LoweringContext<'a> {
                 name: keywords::Invalid.name(),
                 attrs: Default::default(),
                 node: exist_ty_item_kind,
-                vis: hir::Visibility::Inherited,
+                vis: respan(span.shrink_to_lo(), hir::VisibilityKind::Inherited),
                 span: exist_ty_span,
             };
 
@@ -2797,18 +2797,19 @@ impl<'a> LoweringContext<'a> {
                         let new_id = this.lower_node_id(new_node_id);
                         let path = this.lower_path_extra(def, &path, None, ParamMode::Explicit);
                         let item = hir::ItemUse(P(path), hir::UseKind::Single);
-                        let vis = match vis {
-                            hir::Visibility::Public => hir::Visibility::Public,
-                            hir::Visibility::Crate(sugar) => hir::Visibility::Crate(sugar),
-                            hir::Visibility::Inherited => hir::Visibility::Inherited,
-                            hir::Visibility::Restricted { ref path, id: _ } => {
-                                hir::Visibility::Restricted {
+                        let vis_kind = match vis.node {
+                            hir::VisibilityKind::Public => hir::VisibilityKind::Public,
+                            hir::VisibilityKind::Crate(sugar) => hir::VisibilityKind::Crate(sugar),
+                            hir::VisibilityKind::Inherited => hir::VisibilityKind::Inherited,
+                            hir::VisibilityKind::Restricted { ref path, id: _ } => {
+                                hir::VisibilityKind::Restricted {
                                     path: path.clone(),
                                     // We are allocating a new NodeId here
                                     id: this.next_id().node_id,
                                 }
                             }
                         };
+                        let vis = respan(vis.span, vis_kind);
 
                         this.items.insert(
                             new_id.node_id,
@@ -2869,18 +2870,19 @@ impl<'a> LoweringContext<'a> {
                         self.lower_use_tree(use_tree, &prefix, new_id, &mut vis, &mut name, &attrs);
 
                     self.with_hir_id_owner(new_id, |this| {
-                        let vis = match vis {
-                            hir::Visibility::Public => hir::Visibility::Public,
-                            hir::Visibility::Crate(sugar) => hir::Visibility::Crate(sugar),
-                            hir::Visibility::Inherited => hir::Visibility::Inherited,
-                            hir::Visibility::Restricted { ref path, id: _ } => {
-                                hir::Visibility::Restricted {
+                        let vis_kind = match vis.node {
+                            hir::VisibilityKind::Public => hir::VisibilityKind::Public,
+                            hir::VisibilityKind::Crate(sugar) => hir::VisibilityKind::Crate(sugar),
+                            hir::VisibilityKind::Inherited => hir::VisibilityKind::Inherited,
+                            hir::VisibilityKind::Restricted { ref path, id: _ } => {
+                                hir::VisibilityKind::Restricted {
                                     path: path.clone(),
                                     // We are allocating a new NodeId here
                                     id: this.next_id().node_id,
                                 }
                             }
                         };
+                        let vis = respan(vis.span, vis_kind);
 
                         this.items.insert(
                             new_id,
@@ -2901,7 +2903,7 @@ impl<'a> LoweringContext<'a> {
                 // the stability of `use a::{};`, to avoid it showing up as
                 // a re-export by accident when `pub`, e.g. in documentation.
                 let path = P(self.lower_path(id, &prefix, ParamMode::Explicit));
-                *vis = hir::Inherited;
+                *vis = respan(prefix.span.shrink_to_lo(), hir::VisibilityKind::Inherited);
                 hir::ItemUse(path, hir::UseKind::ListStem)
             }
         }
@@ -4284,19 +4286,20 @@ impl<'a> LoweringContext<'a> {
         v: &Visibility,
         explicit_owner: Option<NodeId>,
     ) -> hir::Visibility {
-        match v.node {
-            VisibilityKind::Public => hir::Public,
-            VisibilityKind::Crate(sugar) => hir::Visibility::Crate(sugar),
-            VisibilityKind::Restricted { ref path, id, .. } => hir::Visibility::Restricted {
+        let node = match v.node {
+            VisibilityKind::Public => hir::VisibilityKind::Public,
+            VisibilityKind::Crate(sugar) => hir::VisibilityKind::Crate(sugar),
+            VisibilityKind::Restricted { ref path, id } => hir::VisibilityKind::Restricted {
                 path: P(self.lower_path(id, path, ParamMode::Explicit)),
                 id: if let Some(owner) = explicit_owner {
                     self.lower_node_id_with_owner(id, owner).node_id
                 } else {
                     self.lower_node_id(id).node_id
                 },
             },
-            VisibilityKind::Inherited => hir::Inherited,
-        }
+            VisibilityKind::Inherited => hir::VisibilityKind::Inherited,
+        };
+        respan(v.span, node)
     }
 
     fn lower_defaultness(&mut self, d: Defaultness, has_value: bool) -> hir::Defaultness {

src/librustc/hir/map/collector.rs

@@ -458,11 +458,11 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {
     }
 
     fn visit_vis(&mut self, visibility: &'hir Visibility) {
-        match *visibility {
-            Visibility::Public |
-            Visibility::Crate(_) |
-            Visibility::Inherited => {}
-            Visibility::Restricted { id, .. } => {
+        match visibility.node {
+            VisibilityKind::Public |
+            VisibilityKind::Crate(_) |
+            VisibilityKind::Inherited => {}
+            VisibilityKind::Restricted { id, .. } => {
                 self.insert(id, NodeVisibility(visibility));
                 self.with_parent(id, |this| {
                     intravisit::walk_vis(this, visibility);

src/librustc/hir/map/mod.rs

@@ -1049,7 +1049,9 @@ impl<'hir> Map<'hir> {
             Some(EntryStructCtor(_, _, _)) => self.expect_item(self.get_parent(id)).span,
             Some(EntryLifetime(_, _, lifetime)) => lifetime.span,
             Some(EntryGenericParam(_, _, param)) => param.span,
-            Some(EntryVisibility(_, _, &Visibility::Restricted { ref path, .. })) => path.span,
+            Some(EntryVisibility(_, _, &Spanned {
+                node: VisibilityKind::Restricted { ref path, .. }, ..
+            })) => path.span,
             Some(EntryVisibility(_, _, v)) => bug!("unexpected Visibility {:?}", v),
             Some(EntryLocal(_, _, local)) => local.span,
             Some(EntryMacroDef(_, macro_def)) => macro_def.span,

src/librustc/hir/mod.rs

@@ -24,7 +24,6 @@ pub use self::Stmt_::*;
 pub use self::Ty_::*;
 pub use self::UnOp::*;
 pub use self::UnsafeSource::*;
-pub use self::Visibility::{Public, Inherited};
 
 use hir::def::Def;
 use hir::def_id::{DefId, DefIndex, LocalDefId, CRATE_DEF_INDEX};
@@ -1929,22 +1928,30 @@ pub struct PolyTraitRef {
     pub span: Span,
 }
 
+pub type Visibility = Spanned<VisibilityKind>;
+
 #[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug)]
-pub enum Visibility {
+pub enum VisibilityKind {
     Public,
     Crate(CrateSugar),
     Restricted { path: P<Path>, id: NodeId },
     Inherited,
 }
 
-impl Visibility {
+impl VisibilityKind {
+    pub fn is_pub(&self) -> bool {
+        match *self {
+            VisibilityKind::Public => true,
+            _ => false
+        }
+    }
+
     pub fn is_pub_restricted(&self) -> bool {
-        use self::Visibility::*;
-        match self {
-            &Public |
-            &Inherited => false,
-            &Crate(_) |
-            &Restricted { .. } => true,
+        match *self {
+            VisibilityKind::Public |
+            VisibilityKind::Inherited => false,
+            VisibilityKind::Crate(..) |
+            VisibilityKind::Restricted { .. } => true,
         }
     }
 }

src/librustc/hir/print.rs

@@ -12,7 +12,7 @@ pub use self::AnnNode::*;
 
 use rustc_target::spec::abi::Abi;
 use syntax::ast;
-use syntax::codemap::CodeMap;
+use syntax::codemap::{CodeMap, Spanned};
 use syntax::parse::ParseSess;
 use syntax::parse::lexer::comments;
 use syntax::print::pp::{self, Breaks};
@@ -839,11 +839,11 @@ impl<'a> State<'a> {
     }
 
     pub fn print_visibility(&mut self, vis: &hir::Visibility) -> io::Result<()> {
-        match *vis {
-            hir::Public => self.word_nbsp("pub")?,
-            hir::Visibility::Crate(ast::CrateSugar::JustCrate) => self.word_nbsp("crate")?,
-            hir::Visibility::Crate(ast::CrateSugar::PubCrate) => self.word_nbsp("pub(crate)")?,
-            hir::Visibility::Restricted { ref path, .. } => {
+        match vis.node {
+            hir::VisibilityKind::Public => self.word_nbsp("pub")?,
+            hir::VisibilityKind::Crate(ast::CrateSugar::JustCrate) => self.word_nbsp("crate")?,
+            hir::VisibilityKind::Crate(ast::CrateSugar::PubCrate) => self.word_nbsp("pub(crate)")?,
+            hir::VisibilityKind::Restricted { ref path, .. } => {
                 self.s.word("pub(")?;
                 if path.segments.len() == 1 &&
                    path.segments[0].ident.name == keywords::Super.name() {
@@ -856,7 +856,7 @@ impl<'a> State<'a> {
                 }
                 self.word_nbsp(")")?;
             }
-            hir::Inherited => ()
+            hir::VisibilityKind::Inherited => ()
         }
 
         Ok(())
@@ -952,17 +952,21 @@ impl<'a> State<'a> {
         self.print_outer_attributes(&ti.attrs)?;
         match ti.node {
             hir::TraitItemKind::Const(ref ty, default) => {
-                self.print_associated_const(ti.ident, &ty, default, &hir::Inherited)?;
+                let vis = Spanned { span: syntax_pos::DUMMY_SP,
+                                    node: hir::VisibilityKind::Inherited };
+                self.print_associated_const(ti.ident, &ty, default, &vis)?;
             }
             hir::TraitItemKind::Method(ref sig, hir::TraitMethod::Required(ref arg_names)) => {
-                self.print_method_sig(ti.ident, sig, &ti.generics, &hir::Inherited, arg_names,
-                    None)?;
+                let vis = Spanned { span: syntax_pos::DUMMY_SP,
+                                    node: hir::VisibilityKind::Inherited };
+                self.print_method_sig(ti.ident, sig, &ti.generics, &vis, arg_names, None)?;
                 self.s.word(";")?;
             }
             hir::TraitItemKind::Method(ref sig, hir::TraitMethod::Provided(body)) => {
+                let vis = Spanned { span: syntax_pos::DUMMY_SP,
+                                    node: hir::VisibilityKind::Inherited };
                 self.head("")?;
-                self.print_method_sig(ti.ident, sig, &ti.generics, &hir::Inherited, &[],
-                    Some(body))?;
+                self.print_method_sig(ti.ident, sig, &ti.generics, &vis, &[], Some(body))?;
                 self.nbsp()?;
                 self.end()?; // need to close a box
                 self.end()?; // need to close a box
@@ -2266,7 +2270,8 @@ impl<'a> State<'a> {
                       },
                       name,
                       &generics,
-                      &hir::Inherited,
+                      &Spanned { span: syntax_pos::DUMMY_SP,
+                                 node: hir::VisibilityKind::Inherited },
                       arg_names,
                       None)?;
         self.end()

src/librustc/ich/impls_hir.rs

@@ -710,20 +710,20 @@ impl_stable_hash_for!(enum ::syntax::ast::CrateSugar {
     PubCrate,
 });
 
-impl<'a> HashStable<StableHashingContext<'a>> for hir::Visibility {
+impl<'a> HashStable<StableHashingContext<'a>> for hir::VisibilityKind {
     fn hash_stable<W: StableHasherResult>(&self,
                                           hcx: &mut StableHashingContext<'a>,
                                           hasher: &mut StableHasher<W>) {
         mem::discriminant(self).hash_stable(hcx, hasher);
         match *self {
-            hir::Visibility::Public |
-            hir::Visibility::Inherited => {
+            hir::VisibilityKind::Public |
+            hir::VisibilityKind::Inherited => {
                 // No fields to hash.
             }
-            hir::Visibility::Crate(sugar) => {
+            hir::VisibilityKind::Crate(sugar) => {
                 sugar.hash_stable(hcx, hasher);
             }
-            hir::Visibility::Restricted { ref path, id } => {
+            hir::VisibilityKind::Restricted { ref path, id } => {
                 hcx.with_node_id_hashing_mode(NodeIdHashingMode::HashDefPath, |hcx| {
                     id.hash_stable(hcx, hasher);
                 });
@@ -733,6 +733,8 @@ impl<'a> HashStable<StableHashingContext<'a>> for hir::Visibility {
     }
 }
 
+impl_stable_hash_for_spanned!(hir::VisibilityKind);
+
 impl<'a> HashStable<StableHashingContext<'a>> for hir::Defaultness {
     fn hash_stable<W: StableHasherResult>(&self,
                                           hcx: &mut StableHashingContext<'a>,

src/librustc/middle/dead.rs

@@ -161,7 +161,7 @@ impl<'a, 'tcx> MarkSymbolVisitor<'a, 'tcx> {
                         intravisit::walk_item(self, &item);
                     }
                     hir::ItemEnum(..) => {
-                        self.inherited_pub_visibility = item.vis == hir::Public;
+                        self.inherited_pub_visibility = item.vis.node.is_pub();
                         intravisit::walk_item(self, &item);
                     }
                     hir::ItemFn(..)
@@ -216,7 +216,7 @@ impl<'a, 'tcx> Visitor<'tcx> for MarkSymbolVisitor<'a, 'tcx> {
         let has_repr_c = self.repr_has_repr_c;
         let inherited_pub_visibility = self.inherited_pub_visibility;
         let live_fields = def.fields().iter().filter(|f| {
-            has_repr_c || inherited_pub_visibility || f.vis == hir::Public
+            has_repr_c || inherited_pub_visibility || f.vis.node.is_pub()
         });
         self.live_symbols.extend(live_fields.map(|f| f.id));
 

src/librustc/ty/mod.rs

@@ -268,16 +268,16 @@ impl<'a, 'gcx, 'tcx> DefIdTree for TyCtxt<'a, 'gcx, 'tcx> {
 
 impl Visibility {
     pub fn from_hir(visibility: &hir::Visibility, id: NodeId, tcx: TyCtxt) -> Self {
-        match *visibility {
-            hir::Public => Visibility::Public,
-            hir::Visibility::Crate(_) => Visibility::Restricted(DefId::local(CRATE_DEF_INDEX)),
-            hir::Visibility::Restricted { ref path, .. } => match path.def {
+        match visibility.node {
+            hir::VisibilityKind::Public => Visibility::Public,
+            hir::VisibilityKind::Crate(_) => Visibility::Restricted(DefId::local(CRATE_DEF_INDEX)),
+            hir::VisibilityKind::Restricted { ref path, .. } => match path.def {
                 // If there is no resolution, `resolve` will have already reported an error, so
                 // assume that the visibility is public to avoid reporting more privacy errors.
                 Def::Err => Visibility::Public,
                 def => Visibility::Restricted(def.def_id()),
             },
-            hir::Inherited => {
+            hir::VisibilityKind::Inherited => {
                 Visibility::Restricted(tcx.hir.get_module_parent(id))
             }
         }