Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jul 2, 2018
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9df9c9d
choose a less arbitrary span when parsing the empty visibility modifier
zackmdavis Jul 1, 2018
4ae8912
in which hir::Visibility recalls whence it came (i.e., becomes Spanned)
zackmdavis Jul 1, 2018
104985b
unreachable_pub lint: grab `pub` span from HIR rather than inferring it
zackmdavis Jun 28, 2018
5330747
private no-mangle lints: issue suggestion for restricted visibility
zackmdavis Jun 28, 2018
8d1cbb0
private no-mangle lints: help hint note if visibility modifier is `pub`
zackmdavis Jun 28, 2018
c2d44b2
in which the private/restricted-in-public error messaging gets specific
zackmdavis Jul 1, 2018
43a0a65
call it `hir::VisibilityKind` instead of `hir::Visibility_:*`
zackmdavis Jul 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/librustc/hir/intravisit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 VisibilityRestricted { ref path, id } = vis.node {
visitor.visit_id(id);
visitor.visit_path(path, id)
}
Expand Down
43 changes: 23 additions & 20 deletions src/librustc/hir/lowering.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1285,7 +1285,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::VisibilityInherited),
span: exist_ty_span,
};

Expand Down Expand Up @@ -2770,18 +2770,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::VisibilityPublic => hir::VisibilityPublic,
hir::VisibilityCrate(sugar) => hir::VisibilityCrate(sugar),
hir::VisibilityInherited => hir::VisibilityInherited,
hir::VisibilityRestricted { ref path, id: _ } => {
hir::VisibilityRestricted {
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,
Expand Down Expand Up @@ -2842,18 +2843,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::VisibilityPublic => hir::VisibilityPublic,
hir::VisibilityCrate(sugar) => hir::VisibilityCrate(sugar),
hir::VisibilityInherited => hir::VisibilityInherited,
hir::VisibilityRestricted { ref path, id: _ } => {
hir::VisibilityRestricted {
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,
Expand All @@ -2874,7 +2876,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::VisibilityInherited);
hir::ItemUse(path, hir::UseKind::ListStem)
}
}
Expand Down Expand Up @@ -4274,19 +4276,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::VisibilityPublic,
VisibilityKind::Crate(sugar) => hir::VisibilityCrate(sugar),
VisibilityKind::Restricted { ref path, id } => hir::VisibilityRestricted {
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::VisibilityInherited,
};
respan(v.span, node)
}

fn lower_defaultness(&mut self, d: Defaultness, has_value: bool) -> hir::Defaultness {
Expand Down
10 changes: 5 additions & 5 deletions src/librustc/hir/map/collector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
VisibilityPublic |
VisibilityCrate(_) |
VisibilityInherited => {}
VisibilityRestricted { id, .. } => {
self.insert(id, NodeVisibility(visibility));
self.with_parent(id, |this| {
intravisit::walk_vis(this, visibility);
Expand Down
4 changes: 3 additions & 1 deletion src/librustc/hir/map/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: VisibilityRestricted { ref path, .. }, ..
})) => path.span,
Some(EntryVisibility(_, _, v)) => bug!("unexpected Visibility {:?}", v),
Some(EntryLocal(_, _, local)) => local.span,
Some(EntryMacroDef(_, macro_def)) => macro_def.span,
Expand Down
34 changes: 21 additions & 13 deletions src/librustc/hir/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ pub use self::Stmt_::*;
pub use self::Ty_::*;
pub use self::UnOp::*;
pub use self::UnsafeSource::*;
pub use self::Visibility::{Public, Inherited};
pub use self::Visibility_::*;

use hir::def::Def;
use hir::def_id::{DefId, DefIndex, LocalDefId, CRATE_DEF_INDEX};
Expand Down Expand Up @@ -1929,22 +1929,30 @@ pub struct PolyTraitRef {
pub span: Span,
}

pub type Visibility = Spanned<Visibility_>;

#[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug)]
pub enum Visibility {
Public,
Crate(CrateSugar),
Restricted { path: P<Path>, id: NodeId },
Inherited,
pub enum Visibility_ {
VisibilityPublic,
VisibilityCrate(CrateSugar),
VisibilityRestricted { path: P<Path>, id: NodeId },
VisibilityInherited,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convention enum Something_ { SomethingVariant } is "deprecated" and exists in HIR only because not all HIR structures were updated to the "modern" form enum SomethingKind { Variant } like it was done in AST.

New structures should not use it, so it'd be better to rename Visibility_ to VisibilityKind.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

understood; filed #51968 for the other HIR structures

}

impl Visibility {
impl Visibility_ {
pub fn is_pub(&self) -> bool {
match *self {
VisibilityPublic => true,
_ => false
}
}

pub fn is_pub_restricted(&self) -> bool {
use self::Visibility::*;
match self {
&Public |
&Inherited => false,
&Crate(_) |
&Restricted { .. } => true,
match *self {
VisibilityPublic |
VisibilityInherited => false,
VisibilityCrate(..) |
VisibilityRestricted { .. } => true,
}
}
}
Expand Down
27 changes: 14 additions & 13 deletions src/librustc/hir/print.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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::VisibilityPublic => self.word_nbsp("pub")?,
hir::VisibilityCrate(ast::CrateSugar::JustCrate) => self.word_nbsp("crate")?,
hir::VisibilityCrate(ast::CrateSugar::PubCrate) => self.word_nbsp("pub(crate)")?,
hir::VisibilityRestricted { ref path, .. } => {
self.s.word("pub(")?;
if path.segments.len() == 1 &&
path.segments[0].ident.name == keywords::Super.name() {
Expand All @@ -856,7 +856,7 @@ impl<'a> State<'a> {
}
self.word_nbsp(")")?;
}
hir::Inherited => ()
hir::VisibilityInherited => ()
}

Ok(())
Expand Down Expand Up @@ -952,17 +952,18 @@ 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::VisibilityInherited };
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::VisibilityInherited };
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::VisibilityInherited };
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
Expand Down Expand Up @@ -2266,7 +2267,7 @@ impl<'a> State<'a> {
},
name,
&generics,
&hir::Inherited,
&Spanned { span: syntax_pos::DUMMY_SP, node: hir::VisibilityInherited },
arg_names,
None)?;
self.end()
Expand Down
12 changes: 7 additions & 5 deletions src/librustc/ich/impls_hir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::Visibility_ {
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::VisibilityPublic |
hir::VisibilityInherited => {
// No fields to hash.
}
hir::Visibility::Crate(sugar) => {
hir::VisibilityCrate(sugar) => {
sugar.hash_stable(hcx, hasher);
}
hir::Visibility::Restricted { ref path, id } => {
hir::VisibilityRestricted { ref path, id } => {
hcx.with_node_id_hashing_mode(NodeIdHashingMode::HashDefPath, |hcx| {
id.hash_stable(hcx, hasher);
});
Expand All @@ -733,6 +733,8 @@ impl<'a> HashStable<StableHashingContext<'a>> for hir::Visibility {
}
}

impl_stable_hash_for_spanned!(hir::Visibility_);

impl<'a> HashStable<StableHashingContext<'a>> for hir::Defaultness {
fn hash_stable<W: StableHasherResult>(&self,
hcx: &mut StableHashingContext<'a>,
Expand Down
4 changes: 2 additions & 2 deletions src/librustc/middle/dead.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,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(..)
Expand Down Expand Up @@ -212,7 +212,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));

Expand Down
10 changes: 5 additions & 5 deletions src/librustc/ty/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::VisibilityPublic => Visibility::Public,
hir::VisibilityCrate(_) => Visibility::Restricted(DefId::local(CRATE_DEF_INDEX)),
hir::VisibilityRestricted { 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::VisibilityInherited => {
Visibility::Restricted(tcx.hir.get_module_parent(id))
}
}
Expand Down
Loading