Auto merge of #47799 - topecongiro:fix-span-of-visibility, r=petroche…

…nkov

Fix span of visibility

This PR

1. adds a closing parenthesis to the span of `Visibility::Crate` (e.g. `pub(crate)`). The current span only covers `pub(crate`.
2. adds a `span` field to `Visibility::Restricted`. This span covers the entire visibility expression (e.g. `pub (in self)`). Currently all we can have is a span for `Path`.

This PR is motivated by the bug found in rustfmt (rust-lang/rustfmt#2398).

The first change is a strict improvement IMHO. The second change may not be desirable, as it adds a field which is currently not used by the compiler.

src/librustc/hir/lowering.rs

@@ -3362,10 +3362,10 @@ impl<'a> LoweringContext<'a> {
                         v: &Visibility,
                         explicit_owner: Option<NodeId>)
                         -> hir::Visibility {
-        match *v {
-            Visibility::Public => hir::Public,
-            Visibility::Crate(..) => hir::Visibility::Crate,
-            Visibility::Restricted { ref path, id } => {
+        match v.node {
+            VisibilityKind::Public => hir::Public,
+            VisibilityKind::Crate(..) => hir::Visibility::Crate,
+            VisibilityKind::Restricted { ref path, id, .. } => {
                 hir::Visibility::Restricted {
                     path: P(self.lower_path(id, path, ParamMode::Explicit, true)),
                     id: if let Some(owner) = explicit_owner {
@@ -3375,7 +3375,7 @@ impl<'a> LoweringContext<'a> {
                     }
                 }
             }
-            Visibility::Inherited => hir::Inherited,
+            VisibilityKind::Inherited => hir::Inherited,
         }
     }
 

src/librustc_allocator/expand.rs

@@ -13,9 +13,9 @@ use rustc_errors;
 use syntax::abi::Abi;
 use syntax::ast::{Crate, Attribute, LitKind, StrStyle, ExprKind};
 use syntax::ast::{Unsafety, Constness, Generics, Mutability, Ty, Mac, Arg};
-use syntax::ast::{self, Ident, Item, ItemKind, TyKind, Visibility, Expr};
+use syntax::ast::{self, Ident, Item, ItemKind, TyKind, VisibilityKind, Expr};
 use syntax::attr;
-use syntax::codemap::dummy_spanned;
+use syntax::codemap::{dummy_spanned, respan};
 use syntax::codemap::{ExpnInfo, NameAndSpan, MacroAttribute};
 use syntax::ext::base::ExtCtxt;
 use syntax::ext::base::Resolver;
@@ -97,7 +97,11 @@ impl<'a> Folder for ExpandAllocatorDirectives<'a> {
         ]);
         let mut items = vec![
             f.cx.item_extern_crate(f.span, f.alloc),
-            f.cx.item_use_simple(f.span, Visibility::Inherited, super_path),
+            f.cx.item_use_simple(
+                f.span,
+                respan(f.span.empty(), VisibilityKind::Inherited),
+                super_path,
+            ),
         ];
         for method in ALLOCATOR_METHODS {
             items.push(f.allocator_fn(method));

src/librustc_metadata/cstore_impl.rs

@@ -34,6 +34,7 @@ use std::rc::Rc;
 
 use syntax::ast;
 use syntax::attr;
+use syntax::codemap;
 use syntax::ext::base::SyntaxExtension;
 use syntax::parse::filemap_to_stream;
 use syntax::symbol::Symbol;
@@ -496,7 +497,7 @@ impl CrateStore for cstore::CStore {
                 tokens: body.into(),
                 legacy: def.legacy,
             }),
-            vis: ast::Visibility::Inherited,
+            vis: codemap::respan(local_span.empty(), ast::VisibilityKind::Inherited),
             tokens: None,
         })
     }

src/librustc_passes/ast_validation.rs

@@ -58,14 +58,14 @@ impl<'a> AstValidator<'a> {
         }
     }
 
-    fn invalid_visibility(&self, vis: &Visibility, span: Span, note: Option<&str>) {
-        if vis != &Visibility::Inherited {
+    fn invalid_visibility(&self, vis: &Visibility, note: Option<&str>) {
+        if vis.node != VisibilityKind::Inherited {
             let mut err = struct_span_err!(self.session,
-                                           span,
+                                           vis.span,
                                            E0449,
                                            "unnecessary visibility qualifier");
-            if vis == &Visibility::Public {
-                err.span_label(span, "`pub` not needed here");
+            if vis.node == VisibilityKind::Public {
+                err.span_label(vis.span, "`pub` not needed here");
             }
             if let Some(note) = note {
                 err.note(note);
@@ -216,7 +216,7 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
     fn visit_item(&mut self, item: &'a Item) {
         match item.node {
             ItemKind::Impl(unsafety, polarity, _, _, Some(..), ref ty, ref impl_items) => {
-                self.invalid_visibility(&item.vis, item.span, None);
+                self.invalid_visibility(&item.vis, None);
                 if ty.node == TyKind::Err {
                     self.err_handler()
                         .struct_span_err(item.span, "`impl Trait for .. {}` is an obsolete syntax")
@@ -226,15 +226,14 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
                     span_err!(self.session, item.span, E0198, "negative impls cannot be unsafe");
                 }
                 for impl_item in impl_items {
-                    self.invalid_visibility(&impl_item.vis, impl_item.span, None);
+                    self.invalid_visibility(&impl_item.vis, None);
                     if let ImplItemKind::Method(ref sig, _) = impl_item.node {
                         self.check_trait_fn_not_const(sig.constness);
                     }
                 }
             }
             ItemKind::Impl(unsafety, polarity, defaultness, _, None, _, _) => {
                 self.invalid_visibility(&item.vis,
-                                        item.span,
                                         Some("place qualifiers on individual impl items instead"));
                 if unsafety == Unsafety::Unsafe {
                     span_err!(self.session, item.span, E0197, "inherent impls cannot be unsafe");
@@ -247,16 +246,16 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
                 }
             }
             ItemKind::ForeignMod(..) => {
-                self.invalid_visibility(&item.vis,
-                                        item.span,
-                                        Some("place qualifiers on individual foreign items \
-                                              instead"));
+                self.invalid_visibility(
+                    &item.vis,
+                    Some("place qualifiers on individual foreign items instead"),
+                );
             }
             ItemKind::Enum(ref def, _) => {
                 for variant in &def.variants {
                     self.invalid_non_exhaustive_attribute(variant);
                     for field in variant.node.data.fields() {
-                        self.invalid_visibility(&field.vis, field.span, None);
+                        self.invalid_visibility(&field.vis, None);
                     }
                 }
             }
@@ -359,8 +358,8 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
     }
 
     fn visit_vis(&mut self, vis: &'a Visibility) {
-        match *vis {
-            Visibility::Restricted { ref path, .. } => {
+        match vis.node {
+            VisibilityKind::Restricted { ref path, .. } => {
                 path.segments.iter().find(|segment| segment.parameters.is_some()).map(|segment| {
                     self.err_handler().span_err(segment.parameters.as_ref().unwrap().span(),
                                                 "generic arguments in visibility path");

src/librustc_resolve/check_unused.rs

@@ -86,7 +86,7 @@ impl<'a, 'b> Visitor<'a> for UnusedImportCheckVisitor<'a, 'b> {
         // because this means that they were generated in some fashion by the
         // compiler and we don't need to consider them.
         if let ast::ItemKind::Use(..) = item.node {
-            if item.vis == ast::Visibility::Public || item.span.source_equal(&DUMMY_SP) {
+            if item.vis.node == ast::VisibilityKind::Public || item.span.source_equal(&DUMMY_SP) {
                 return;
             }
         }

src/librustc_resolve/lib.rs

@@ -3796,13 +3796,15 @@ impl<'a> Resolver<'a> {
     }
 
     fn resolve_visibility(&mut self, vis: &ast::Visibility) -> ty::Visibility {
-        match *vis {
-            ast::Visibility::Public => ty::Visibility::Public,
-            ast::Visibility::Crate(..) => ty::Visibility::Restricted(DefId::local(CRATE_DEF_INDEX)),
-            ast::Visibility::Inherited => {
+        match vis.node {
+            ast::VisibilityKind::Public => ty::Visibility::Public,
+            ast::VisibilityKind::Crate(..) => {
+                ty::Visibility::Restricted(DefId::local(CRATE_DEF_INDEX))
+            }
+            ast::VisibilityKind::Inherited => {
                 ty::Visibility::Restricted(self.current_module.normal_ancestor_id)
             }
-            ast::Visibility::Restricted { ref path, id } => {
+            ast::VisibilityKind::Restricted { ref path, id, .. } => {
                 let def = self.smart_resolve_path(id, None, path,
                                                   PathSource::Visibility).base_def();
                 if def == Def::Err {

src/librustc_save_analysis/dump_visitor.rs

@@ -43,7 +43,7 @@ use syntax::print::pprust::{
     ty_to_string
 };
 use syntax::ptr::P;
-use syntax::codemap::{Spanned, DUMMY_SP};
+use syntax::codemap::{Spanned, DUMMY_SP, respan};
 use syntax_pos::*;
 
 use {escape, generated_code, lower_attributes, PathCollector, SaveContext};
@@ -65,12 +65,19 @@ macro_rules! down_cast_data {
 }
 
 macro_rules! access_from {
+    ($save_ctxt:expr, $vis:expr, $id:expr) => {
+        Access {
+            public: $vis.node == ast::VisibilityKind::Public,
+            reachable: $save_ctxt.analysis.access_levels.is_reachable($id),
+        }
+    };
+
     ($save_ctxt:expr, $item:expr) => {
         Access {
-            public: $item.vis == ast::Visibility::Public,
+            public: $item.vis.node == ast::VisibilityKind::Public,
             reachable: $save_ctxt.analysis.access_levels.is_reachable($item.id),
         }
-    }
+    };
 }
 
 pub struct DumpVisitor<'l, 'tcx: 'l, 'll, O: DumpOutput + 'll> {
@@ -405,12 +412,7 @@ impl<'l, 'tcx: 'l, 'll, O: DumpOutput + 'll> DumpVisitor<'l, 'tcx, 'll, O> {
 
             method_data.value = sig_str;
             method_data.sig = sig::method_signature(id, name, generics, sig, &self.save_ctxt);
-            self.dumper.dump_def(
-                &Access {
-                    public: vis == ast::Visibility::Public,
-                    reachable: self.save_ctxt.analysis.access_levels.is_reachable(id),
-                },
-                method_data);
+            self.dumper.dump_def(&access_from!(self.save_ctxt, vis, id), method_data);
         }
 
         // walk arg and return types
@@ -543,10 +545,7 @@ impl<'l, 'tcx: 'l, 'll, O: DumpOutput + 'll> DumpVisitor<'l, 'tcx, 'll, O> {
             let span = self.span_from_span(sub_span.expect("No span found for variable"));
 
             self.dumper.dump_def(
-                &Access {
-                    public: vis == ast::Visibility::Public,
-                    reachable: self.save_ctxt.analysis.access_levels.is_reachable(id),
-                },
+                &access_from!(self.save_ctxt, vis, id),
                 Def {
                     kind: DefKind::Const,
                     id: ::id_from_node_id(id, &self.save_ctxt),
@@ -597,7 +596,7 @@ impl<'l, 'tcx: 'l, 'll, O: DumpOutput + 'll> DumpVisitor<'l, 'tcx, 'll, O> {
                     .iter()
                     .enumerate()
                     .filter_map(|(i, f)| {
-                        if include_priv_fields || f.vis == ast::Visibility::Public {
+                        if include_priv_fields || f.vis.node == ast::VisibilityKind::Public {
                             f.ident
                                 .map(|i| i.to_string())
                                 .or_else(|| Some(i.to_string()))
@@ -1135,6 +1134,7 @@ impl<'l, 'tcx: 'l, 'll, O: DumpOutput + 'll> DumpVisitor<'l, 'tcx, 'll, O> {
 
     fn process_trait_item(&mut self, trait_item: &'l ast::TraitItem, trait_id: DefId) {
         self.process_macro_use(trait_item.span);
+        let vis_span = trait_item.span.empty();
         match trait_item.node {
             ast::TraitItemKind::Const(ref ty, ref expr) => {
                 self.process_assoc_const(
@@ -1144,7 +1144,7 @@ impl<'l, 'tcx: 'l, 'll, O: DumpOutput + 'll> DumpVisitor<'l, 'tcx, 'll, O> {
                     &ty,
                     expr.as_ref().map(|e| &**e),
                     trait_id,
-                    ast::Visibility::Public,
+                    respan(vis_span, ast::VisibilityKind::Public),
                     &trait_item.attrs,
                 );
             }
@@ -1155,7 +1155,7 @@ impl<'l, 'tcx: 'l, 'll, O: DumpOutput + 'll> DumpVisitor<'l, 'tcx, 'll, O> {
                     trait_item.id,
                     trait_item.ident,
                     &trait_item.generics,
-                    ast::Visibility::Public,
+                    respan(vis_span, ast::VisibilityKind::Public),
                     trait_item.span,
                 );
             }
@@ -1259,10 +1259,7 @@ impl<'l, 'tcx: 'l, 'll, O: DumpOutput + 'll> DumpVisitor<'l, 'tcx, 'll, O> {
 
         // The access is calculated using the current tree ID, but with the root tree's visibility
         // (since nested trees don't have their own visibility).
-        let access = Access {
-            public: root_item.vis == ast::Visibility::Public,
-            reachable: self.save_ctxt.analysis.access_levels.is_reachable(id),
-        };
+        let access = access_from!(self.save_ctxt, root_item.vis, id);
 
         // The parent def id of a given use tree is always the enclosing item.
         let parent = self.save_ctxt.tcx.hir.opt_local_def_id(id)

src/libsyntax/ast.rs

@@ -1937,10 +1937,12 @@ pub enum CrateSugar {
     JustCrate,
 }
 
+pub type Visibility = Spanned<VisibilityKind>;
+
 #[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug)]
-pub enum Visibility {
+pub enum VisibilityKind {
     Public,
-    Crate(Span, CrateSugar),
+    Crate(CrateSugar),
     Restricted { path: P<Path>, id: NodeId },
     Inherited,
 }

src/libsyntax/diagnostics/plugin.rs

@@ -14,6 +14,7 @@ use std::env;
 
 use ast;
 use ast::{Ident, Name};
+use codemap;
 use syntax_pos::Span;
 use ext::base::{ExtCtxt, MacEager, MacResult};
 use ext::build::AstBuilder;
@@ -234,7 +235,7 @@ pub fn expand_build_diagnostic_array<'cx>(ecx: &'cx mut ExtCtxt,
                 ty,
                 expr,
             ),
-            vis: ast::Visibility::Public,
+            vis: codemap::respan(span.empty(), ast::VisibilityKind::Public),
             span,
             tokens: None,
         })

src/libsyntax/ext/build.rs

@@ -987,7 +987,7 @@ impl<'a> AstBuilder for ExtCtxt<'a> {
             attrs,
             id: ast::DUMMY_NODE_ID,
             node,
-            vis: ast::Visibility::Inherited,
+            vis: respan(span.empty(), ast::VisibilityKind::Inherited),
             span,
             tokens: None,
         })
@@ -1033,7 +1033,7 @@ impl<'a> AstBuilder for ExtCtxt<'a> {
                 span: ty.span,
                 ty,
                 ident: None,
-                vis: ast::Visibility::Inherited,
+                vis: respan(span.empty(), ast::VisibilityKind::Inherited),
                 attrs: Vec::new(),
                 id: ast::DUMMY_NODE_ID,
             }

src/libsyntax/ext/expand.rs

@@ -11,7 +11,7 @@
 use ast::{self, Block, Ident, NodeId, PatKind, Path};
 use ast::{MacStmtStyle, StmtKind, ItemKind};
 use attr::{self, HasAttrs};
-use codemap::{ExpnInfo, NameAndSpan, MacroBang, MacroAttribute, dummy_spanned};
+use codemap::{ExpnInfo, NameAndSpan, MacroBang, MacroAttribute, dummy_spanned, respan};
 use config::{is_test_or_bench, StripUnconfigured};
 use errors::FatalError;
 use ext::base::*;
@@ -238,7 +238,7 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
             node: ast::ItemKind::Mod(krate.module),
             ident: keywords::Invalid.ident(),
             id: ast::DUMMY_NODE_ID,
-            vis: ast::Visibility::Public,
+            vis: respan(krate.span.empty(), ast::VisibilityKind::Public),
             tokens: None,
         })));
 
@@ -1022,7 +1022,10 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {
             // Ensure that test functions are accessible from the test harness.
             ast::ItemKind::Fn(..) if self.cx.ecfg.should_test => {
                 if item.attrs.iter().any(|attr| is_test_or_bench(attr)) {
-                    item = item.map(|mut item| { item.vis = ast::Visibility::Public; item });
+                    item = item.map(|mut item| {
+                        item.vis = respan(item.vis.span, ast::VisibilityKind::Public);
+                        item
+                    });
                 }
                 noop_fold_item(item, self)
             }

src/libsyntax/ext/placeholders.rs

@@ -33,7 +33,7 @@ pub fn placeholder(kind: ExpansionKind, id: ast::NodeId) -> Expansion {
     let ident = keywords::Invalid.ident();
     let attrs = Vec::new();
     let generics = ast::Generics::default();
-    let vis = ast::Visibility::Inherited;
+    let vis = dummy_spanned(ast::VisibilityKind::Inherited);
     let span = DUMMY_SP;
     let expr_placeholder = || P(ast::Expr {
         id, span,

src/libsyntax/ext/quote.rs

@@ -9,6 +9,7 @@
 // except according to those terms.
 
 use ast::{self, Arg, Arm, Block, Expr, Item, Pat, Stmt, Ty};
+use codemap::respan;
 use syntax_pos::Span;
 use ext::base::ExtCtxt;
 use ext::base;
@@ -855,7 +856,12 @@ fn expand_wrapper(cx: &ExtCtxt,
     let mut stmts = imports.iter().map(|path| {
         // make item: `use ...;`
         let path = path.iter().map(|s| s.to_string()).collect();
-        cx.stmt_item(sp, cx.item_use_glob(sp, ast::Visibility::Inherited, ids_ext(path)))
+        let use_item = cx.item_use_glob(
+            sp,
+            respan(sp.empty(), ast::VisibilityKind::Inherited),
+            ids_ext(path),
+        );
+        cx.stmt_item(sp, use_item)
     }).chain(Some(stmt_let_ext_cx)).collect::<Vec<_>>();
     stmts.push(cx.stmt_expr(expr));