Skip to content

Commit 4ae8912

Browse files
zackmdaviszackmdavis
committed
in which hir::Visibility recalls whence it came (i.e., becomes Spanned)
There are at least a couple (and plausibly even three) diagnostics that could use the spans of visibility modifiers in order to be reliably correct (rather than hacking and munging surrounding spans to try to infer where the visibility keyword must have been). We follow the naming convention established by the other `Spanned` HIR nodes: the "outer" type alias gets the "prime" node-type name, the "inner" enum gets the name suffixed with an underscore, and the variant names are prefixed with the prime name and `pub use` exported from here (from HIR). Thanks to veteran reviewer Vadim Petrochenkov for suggesting this uniform approach. (A previous draft, based on the reasoning that `Visibility::Inherited` should not have a span, tried to hack in a named `span` field on `Visibility::Restricted` and a positional field on `Public` and `Crate`. This was ... not so uniform.)
1 parent 9df9c9d commit 4ae8912

File tree

17 files changed

+121
-99
lines changed

17 files changed

+121
-99
lines changed

‎src/librustc/hir/intravisit.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1104,7 +1104,7 @@ pub fn walk_arm<'v, V: Visitor<'v>>(visitor: &mut V, arm: &'v Arm) {
11041104
}
11051105

11061106
pub fn walk_vis<'v, V: Visitor<'v>>(visitor: &mut V, vis: &'v Visibility) {
1107-
if let Visibility::Restricted { ref path, id } = *vis {
1107+
if let VisibilityRestricted { ref path, id } = vis.node {
11081108
visitor.visit_id(id);
11091109
visitor.visit_path(path, id)
11101110
}

‎src/librustc/hir/lowering.rs‎

Lines changed: 23 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1285,7 +1285,7 @@ impl<'a> LoweringContext<'a> {
12851285
name: keywords::Invalid.name(),
12861286
attrs: Default::default(),
12871287
node: exist_ty_item_kind,
1288-
vis: hir::Visibility::Inherited,
1288+
vis: respan(span.shrink_to_lo(), hir::VisibilityInherited),
12891289
span: exist_ty_span,
12901290
};
12911291

@@ -2770,18 +2770,19 @@ impl<'a> LoweringContext<'a> {
27702770
let new_id = this.lower_node_id(new_node_id);
27712771
let path = this.lower_path_extra(def, &path, None, ParamMode::Explicit);
27722772
let item = hir::ItemUse(P(path), hir::UseKind::Single);
2773-
let vis = match vis {
2774-
hir::Visibility::Public => hir::Visibility::Public,
2775-
hir::Visibility::Crate(sugar) => hir::Visibility::Crate(sugar),
2776-
hir::Visibility::Inherited => hir::Visibility::Inherited,
2777-
hir::Visibility::Restricted { ref path, id: _ } => {
2778-
hir::Visibility::Restricted {
2773+
let vis_kind = match vis.node {
2774+
hir::VisibilityPublic => hir::VisibilityPublic,
2775+
hir::VisibilityCrate(sugar) => hir::VisibilityCrate(sugar),
2776+
hir::VisibilityInherited => hir::VisibilityInherited,
2777+
hir::VisibilityRestricted { ref path, id: _ } => {
2778+
hir::VisibilityRestricted {
27792779
path: path.clone(),
27802780
// We are allocating a new NodeId here
27812781
id: this.next_id().node_id,
27822782
}
27832783
}
27842784
};
2785+
let vis = respan(vis.span, vis_kind);
27852786

27862787
this.items.insert(
27872788
new_id.node_id,
@@ -2842,18 +2843,19 @@ impl<'a> LoweringContext<'a> {
28422843
self.lower_use_tree(use_tree, &prefix, new_id, &mut vis, &mut name, &attrs);
28432844

28442845
self.with_hir_id_owner(new_id, |this| {
2845-
let vis = match vis {
2846-
hir::Visibility::Public => hir::Visibility::Public,
2847-
hir::Visibility::Crate(sugar) => hir::Visibility::Crate(sugar),
2848-
hir::Visibility::Inherited => hir::Visibility::Inherited,
2849-
hir::Visibility::Restricted { ref path, id: _ } => {
2850-
hir::Visibility::Restricted {
2846+
let vis_kind = match vis.node {
2847+
hir::VisibilityPublic => hir::VisibilityPublic,
2848+
hir::VisibilityCrate(sugar) => hir::VisibilityCrate(sugar),
2849+
hir::VisibilityInherited => hir::VisibilityInherited,
2850+
hir::VisibilityRestricted { ref path, id: _ } => {
2851+
hir::VisibilityRestricted {
28512852
path: path.clone(),
28522853
// We are allocating a new NodeId here
28532854
id: this.next_id().node_id,
28542855
}
28552856
}
28562857
};
2858+
let vis = respan(vis.span, vis_kind);
28572859

28582860
this.items.insert(
28592861
new_id,
@@ -2874,7 +2876,7 @@ impl<'a> LoweringContext<'a> {
28742876
// the stability of `use a::{};`, to avoid it showing up as
28752877
// a re-export by accident when `pub`, e.g. in documentation.
28762878
let path = P(self.lower_path(id, &prefix, ParamMode::Explicit));
2877-
*vis = hir::Inherited;
2879+
*vis = respan(prefix.span.shrink_to_lo(), hir::VisibilityInherited);
28782880
hir::ItemUse(path, hir::UseKind::ListStem)
28792881
}
28802882
}
@@ -4274,19 +4276,20 @@ impl<'a> LoweringContext<'a> {
42744276
v: &Visibility,
42754277
explicit_owner: Option<NodeId>,
42764278
) -> hir::Visibility {
4277-
match v.node {
4278-
VisibilityKind::Public => hir::Public,
4279-
VisibilityKind::Crate(sugar) => hir::Visibility::Crate(sugar),
4280-
VisibilityKind::Restricted { ref path, id, .. } => hir::Visibility::Restricted {
4279+
let node = match v.node {
4280+
VisibilityKind::Public => hir::VisibilityPublic,
4281+
VisibilityKind::Crate(sugar) => hir::VisibilityCrate(sugar),
4282+
VisibilityKind::Restricted { ref path, id } => hir::VisibilityRestricted {
42814283
path: P(self.lower_path(id, path, ParamMode::Explicit)),
42824284
id: if let Some(owner) = explicit_owner {
42834285
self.lower_node_id_with_owner(id, owner).node_id
42844286
} else {
42854287
self.lower_node_id(id).node_id
42864288
},
42874289
},
4288-
VisibilityKind::Inherited => hir::Inherited,
4289-
}
4290+
VisibilityKind::Inherited => hir::VisibilityInherited,
4291+
};
4292+
respan(v.span, node)
42904293
}
42914294

42924295
fn lower_defaultness(&mut self, d: Defaultness, has_value: bool) -> hir::Defaultness {

‎src/librustc/hir/map/collector.rs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -458,11 +458,11 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {
458458
}
459459

460460
fn visit_vis(&mut self, visibility: &'hir Visibility) {
461-
match *visibility {
462-
Visibility::Public |
463-
Visibility::Crate(_) |
464-
Visibility::Inherited => {}
465-
Visibility::Restricted { id, .. } => {
461+
match visibility.node {
462+
VisibilityPublic |
463+
VisibilityCrate(_) |
464+
VisibilityInherited => {}
465+
VisibilityRestricted { id, .. } => {
466466
self.insert(id, NodeVisibility(visibility));
467467
self.with_parent(id, |this| {
468468
intravisit::walk_vis(this, visibility);

‎src/librustc/hir/map/mod.rs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1049,7 +1049,9 @@ impl<'hir> Map<'hir> {
10491049
Some(EntryStructCtor(_, _, _)) => self.expect_item(self.get_parent(id)).span,
10501050
Some(EntryLifetime(_, _, lifetime)) => lifetime.span,
10511051
Some(EntryGenericParam(_, _, param)) => param.span,
1052-
Some(EntryVisibility(_, _, &Visibility::Restricted { ref path, .. })) => path.span,
1052+
Some(EntryVisibility(_, _, &Spanned {
1053+
node: VisibilityRestricted { ref path, .. }, ..
1054+
})) => path.span,
10531055
Some(EntryVisibility(_, _, v)) => bug!("unexpected Visibility {:?}", v),
10541056
Some(EntryLocal(_, _, local)) => local.span,
10551057
Some(EntryMacroDef(_, macro_def)) => macro_def.span,

‎src/librustc/hir/mod.rs‎

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ pub use self::Stmt_::*;
2424
pub use self::Ty_::*;
2525
pub use self::UnOp::*;
2626
pub use self::UnsafeSource::*;
27-
pub use self::Visibility::{Public, Inherited};
27+
pub use self::Visibility_::*;
2828

2929
use hir::def::Def;
3030
use hir::def_id::{DefId, DefIndex, LocalDefId, CRATE_DEF_INDEX};
@@ -1929,22 +1929,30 @@ pub struct PolyTraitRef {
19291929
pub span: Span,
19301930
}
19311931

1932+
pub type Visibility = Spanned<Visibility_>;
1933+
19321934
#[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug)]
1933-
pub enum Visibility {
1934-
Public,
1935-
Crate(CrateSugar),
1936-
Restricted { path: P<Path>, id: NodeId },
1937-
Inherited,
1935+
pub enum Visibility_ {
1936+
VisibilityPublic,
1937+
VisibilityCrate(CrateSugar),
1938+
VisibilityRestricted { path: P<Path>, id: NodeId },
1939+
VisibilityInherited,
19381940
}
19391941

1940-
impl Visibility {
1942+
impl Visibility_ {
1943+
pub fn is_pub(&self) -> bool {
1944+
match *self {
1945+
VisibilityPublic => true,
1946+
_ => false
1947+
}
1948+
}
1949+
19411950
pub fn is_pub_restricted(&self) -> bool {
1942-
use self::Visibility::*;
1943-
match self {
1944-
&Public |
1945-
&Inherited => false,
1946-
&Crate(_) |
1947-
&Restricted { .. } => true,
1951+
match *self {
1952+
VisibilityPublic |
1953+
VisibilityInherited => false,
1954+
VisibilityCrate(..) |
1955+
VisibilityRestricted { .. } => true,
19481956
}
19491957
}
19501958
}

‎src/librustc/hir/print.rs‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ pub use self::AnnNode::*;
1212

1313
use rustc_target::spec::abi::Abi;
1414
use syntax::ast;
15-
use syntax::codemap::CodeMap;
15+
use syntax::codemap::{CodeMap, Spanned};
1616
use syntax::parse::ParseSess;
1717
use syntax::parse::lexer::comments;
1818
use syntax::print::pp::{self, Breaks};
@@ -839,11 +839,11 @@ impl<'a> State<'a> {
839839
}
840840

841841
pub fn print_visibility(&mut self, vis: &hir::Visibility) -> io::Result<()> {
842-
match *vis {
843-
hir::Public => self.word_nbsp("pub")?,
844-
hir::Visibility::Crate(ast::CrateSugar::JustCrate) => self.word_nbsp("crate")?,
845-
hir::Visibility::Crate(ast::CrateSugar::PubCrate) => self.word_nbsp("pub(crate)")?,
846-
hir::Visibility::Restricted { ref path, .. } => {
842+
match vis.node {
843+
hir::VisibilityPublic => self.word_nbsp("pub")?,
844+
hir::VisibilityCrate(ast::CrateSugar::JustCrate) => self.word_nbsp("crate")?,
845+
hir::VisibilityCrate(ast::CrateSugar::PubCrate) => self.word_nbsp("pub(crate)")?,
846+
hir::VisibilityRestricted { ref path, .. } => {
847847
self.s.word("pub(")?;
848848
if path.segments.len() == 1 &&
849849
path.segments[0].ident.name == keywords::Super.name() {
@@ -856,7 +856,7 @@ impl<'a> State<'a> {
856856
}
857857
self.word_nbsp(")")?;
858858
}
859-
hir::Inherited => ()
859+
hir::VisibilityInherited => ()
860860
}
861861

862862
Ok(())
@@ -952,17 +952,18 @@ impl<'a> State<'a> {
952952
self.print_outer_attributes(&ti.attrs)?;
953953
match ti.node {
954954
hir::TraitItemKind::Const(ref ty, default) => {
955-
self.print_associated_const(ti.ident, &ty, default, &hir::Inherited)?;
955+
let vis = Spanned { span: syntax_pos::DUMMY_SP, node: hir::VisibilityInherited };
956+
self.print_associated_const(ti.ident, &ty, default, &vis)?;
956957
}
957958
hir::TraitItemKind::Method(ref sig, hir::TraitMethod::Required(ref arg_names)) => {
958-
self.print_method_sig(ti.ident, sig, &ti.generics, &hir::Inherited, arg_names,
959-
None)?;
959+
let vis = Spanned { span: syntax_pos::DUMMY_SP, node: hir::VisibilityInherited };
960+
self.print_method_sig(ti.ident, sig, &ti.generics, &vis, arg_names, None)?;
960961
self.s.word(";")?;
961962
}
962963
hir::TraitItemKind::Method(ref sig, hir::TraitMethod::Provided(body)) => {
964+
let vis = Spanned { span: syntax_pos::DUMMY_SP, node: hir::VisibilityInherited };
963965
self.head("")?;
964-
self.print_method_sig(ti.ident, sig, &ti.generics, &hir::Inherited, &[],
965-
Some(body))?;
966+
self.print_method_sig(ti.ident, sig, &ti.generics, &vis, &[], Some(body))?;
966967
self.nbsp()?;
967968
self.end()?; // need to close a box
968969
self.end()?; // need to close a box
@@ -2266,7 +2267,7 @@ impl<'a> State<'a> {
22662267
},
22672268
name,
22682269
&generics,
2269-
&hir::Inherited,
2270+
&Spanned { span: syntax_pos::DUMMY_SP, node: hir::VisibilityInherited },
22702271
arg_names,
22712272
None)?;
22722273
self.end()

‎src/librustc/ich/impls_hir.rs‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -710,20 +710,20 @@ impl_stable_hash_for!(enum ::syntax::ast::CrateSugar {
710710
PubCrate,
711711
});
712712

713-
impl<'a> HashStable<StableHashingContext<'a>> for hir::Visibility {
713+
impl<'a> HashStable<StableHashingContext<'a>> for hir::Visibility_ {
714714
fn hash_stable<W: StableHasherResult>(&self,
715715
hcx: &mut StableHashingContext<'a>,
716716
hasher: &mut StableHasher<W>) {
717717
mem::discriminant(self).hash_stable(hcx, hasher);
718718
match *self {
719-
hir::Visibility::Public |
720-
hir::Visibility::Inherited => {
719+
hir::VisibilityPublic |
720+
hir::VisibilityInherited => {
721721
// No fields to hash.
722722
}
723-
hir::Visibility::Crate(sugar) => {
723+
hir::VisibilityCrate(sugar) => {
724724
sugar.hash_stable(hcx, hasher);
725725
}
726-
hir::Visibility::Restricted { ref path, id } => {
726+
hir::VisibilityRestricted { ref path, id } => {
727727
hcx.with_node_id_hashing_mode(NodeIdHashingMode::HashDefPath, |hcx| {
728728
id.hash_stable(hcx, hasher);
729729
});
@@ -733,6 +733,8 @@ impl<'a> HashStable<StableHashingContext<'a>> for hir::Visibility {
733733
}
734734
}
735735

736+
impl_stable_hash_for_spanned!(hir::Visibility_);
737+
736738
impl<'a> HashStable<StableHashingContext<'a>> for hir::Defaultness {
737739
fn hash_stable<W: StableHasherResult>(&self,
738740
hcx: &mut StableHashingContext<'a>,

‎src/librustc/middle/dead.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ impl<'a, 'tcx> MarkSymbolVisitor<'a, 'tcx> {
157157
intravisit::walk_item(self, &item);
158158
}
159159
hir::ItemEnum(..) => {
160-
self.inherited_pub_visibility = item.vis == hir::Public;
160+
self.inherited_pub_visibility = item.vis.node.is_pub();
161161
intravisit::walk_item(self, &item);
162162
}
163163
hir::ItemFn(..)
@@ -212,7 +212,7 @@ impl<'a, 'tcx> Visitor<'tcx> for MarkSymbolVisitor<'a, 'tcx> {
212212
let has_repr_c = self.repr_has_repr_c;
213213
let inherited_pub_visibility = self.inherited_pub_visibility;
214214
let live_fields = def.fields().iter().filter(|f| {
215-
has_repr_c || inherited_pub_visibility || f.vis == hir::Public
215+
has_repr_c || inherited_pub_visibility || f.vis.node.is_pub()
216216
});
217217
self.live_symbols.extend(live_fields.map(|f| f.id));
218218

‎src/librustc/ty/mod.rs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -268,16 +268,16 @@ impl<'a, 'gcx, 'tcx> DefIdTree for TyCtxt<'a, 'gcx, 'tcx> {
268268

269269
impl Visibility {
270270
pub fn from_hir(visibility: &hir::Visibility, id: NodeId, tcx: TyCtxt) -> Self {
271-
match *visibility {
272-
hir::Public => Visibility::Public,
273-
hir::Visibility::Crate(_) => Visibility::Restricted(DefId::local(CRATE_DEF_INDEX)),
274-
hir::Visibility::Restricted { ref path, .. } => match path.def {
271+
match visibility.node {
272+
hir::VisibilityPublic => Visibility::Public,
273+
hir::VisibilityCrate(_) => Visibility::Restricted(DefId::local(CRATE_DEF_INDEX)),
274+
hir::VisibilityRestricted { ref path, .. } => match path.def {
275275
// If there is no resolution, `resolve` will have already reported an error, so
276276
// assume that the visibility is public to avoid reporting more privacy errors.
277277
Def::Err => Visibility::Public,
278278
def => Visibility::Restricted(def.def_id()),
279279
},
280-
hir::Inherited => {
280+
hir::VisibilityInherited => {
281281
Visibility::Restricted(tcx.hir.get_module_parent(id))
282282
}
283283
}

‎src/librustc_lint/builtin.rs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -397,7 +397,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MissingDoc {
397397
hir::ItemUnion(..) => "a union",
398398
hir::ItemTrait(.., ref trait_item_refs) => {
399399
// Issue #11592, traits are always considered exported, even when private.
400-
if it.vis == hir::Visibility::Inherited {
400+
if it.vis.node == hir::VisibilityInherited {
401401
self.private_traits.insert(it.id);
402402
for trait_item_ref in trait_item_refs {
403403
self.private_traits.insert(trait_item_ref.id.node_id);
@@ -414,7 +414,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MissingDoc {
414414
if let Some(node_id) = cx.tcx.hir.as_local_node_id(real_trait) {
415415
match cx.tcx.hir.find(node_id) {
416416
Some(hir_map::NodeItem(item)) => {
417-
if item.vis == hir::Visibility::Inherited {
417+
if item.vis.node == hir::VisibilityInherited {
418418
for impl_item_ref in impl_item_refs {
419419
self.private_traits.insert(impl_item_ref.id.node_id);
420420
}
@@ -1187,7 +1187,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for InvalidNoMangleItems {
11871187
let msg = "function is marked #[no_mangle], but not exported";
11881188
let mut err = cx.struct_span_lint(PRIVATE_NO_MANGLE_FNS, it.span, msg);
11891189
let insertion_span = it.span.shrink_to_lo();
1190-
if it.vis == hir::Visibility::Inherited {
1190+
if it.vis.node == hir::VisibilityInherited {
11911191
err.span_suggestion(insertion_span,
11921192
"try making it public",
11931193
"pub ".to_owned());
@@ -1218,7 +1218,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for InvalidNoMangleItems {
12181218
let msg = "static is marked #[no_mangle], but not exported";
12191219
let mut err = cx.struct_span_lint(PRIVATE_NO_MANGLE_STATICS, it.span, msg);
12201220
let insertion_span = it.span.shrink_to_lo();
1221-
if it.vis == hir::Visibility::Inherited {
1221+
if it.vis.node == hir::VisibilityInherited {
12221222
err.span_suggestion(insertion_span,
12231223
"try making it public",
12241224
"pub ".to_owned());
@@ -1388,7 +1388,7 @@ impl UnreachablePub {
13881388
fn perform_lint(&self, cx: &LateContext, what: &str, id: ast::NodeId,
13891389
vis: &hir::Visibility, span: Span, exportable: bool,
13901390
mut applicability: Applicability) {
1391-
if !cx.access_levels.is_reachable(id) && *vis == hir::Visibility::Public {
1391+
if !cx.access_levels.is_reachable(id) && vis.node.is_pub() {
13921392
if span.ctxt().outer().expn_info().is_some() {
13931393
applicability = Applicability::MaybeIncorrect;
13941394
}

0 commit comments

Comments
 (0)