Skip to content
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.

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

rustdoc: introduce glob shadowing rules to rustdoc #83872

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
4 changes: 2 additions & 2 deletions src/librustdoc/clean/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1934,11 +1934,11 @@ impl Clean<BareFunctionDecl> for hir::BareFnTy<'_> {
}
}

impl Clean<Vec<Item>> for (&hir::Item<'_>, Option<Symbol>) {
impl Clean<Vec<Item>> for doctree::Item<'_> {
fn clean(&self, cx: &mut DocContext<'_>) -> Vec<Item> {
use hir::ItemKind;

let (item, renamed) = self;
let (item, renamed) = (self.hir_item, self.renamed_name);
let def_id = item.def_id.to_def_id();
let mut name = renamed.unwrap_or_else(|| cx.tcx.hir().name(item.hir_id()));
longfangsong marked this conversation as resolved.
Show resolved Hide resolved
cx.with_param_env(def_id, |cx| {
Expand Down
64 changes: 63 additions & 1 deletion src/librustdoc/doctree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,48 @@ use rustc_span::{self, Span, Symbol};

use rustc_hir as hir;

/// A warp around an hir::Item
longfangsong marked this conversation as resolved.
Show resolved Hide resolved
#[derive(Debug)]
pub(crate) struct Item<'hir> {
/// the wrapped item
pub(crate) hir_item: &'hir hir::Item<'hir>,
/// the explicit renamed name
pub(crate) renamed_name: Option<Symbol>,
/// whether the item is from a glob import
/// if `from_glob` is true and we see another item with same name,
/// then this item can be replaced with that one
pub(crate) from_glob: bool,
}
longfangsong marked this conversation as resolved.
Show resolved Hide resolved

impl<'hir> Item<'hir> {
pub(crate) fn new(
hir_item: &'hir hir::Item<'hir>,
renamed_name: Option<Symbol>,
from_glob: bool,
) -> Self {
Self { hir_item, renamed_name, from_glob }
}

fn name(&'hir self) -> &'hir Symbol {
self.renamed_name.as_ref().unwrap_or(&self.hir_item.ident.name)
}
}

crate struct Module<'hir> {
crate name: Symbol,
crate where_outer: Span,
crate where_inner: Span,
crate mods: Vec<Module<'hir>>,
crate id: hir::HirId,
crate items: Vec<Item<'hir>>,
// (item, renamed)
longfangsong marked this conversation as resolved.
Show resolved Hide resolved
crate items: Vec<(&'hir hir::Item<'hir>, Option<Symbol>)>,
crate foreigns: Vec<(&'hir hir::ForeignItem<'hir>, Option<Symbol>)>,
crate macros: Vec<(&'hir hir::MacroDef<'hir>, Option<Symbol>)>,
crate is_crate: bool,
/// whether the module is from a glob import
/// if `from_glob` is true and we see another module with same name,
/// then this item can be replaced with that one
pub(crate) from_glob: bool,
}

impl Module<'hir> {
Expand All @@ -29,6 +60,37 @@ impl Module<'hir> {
foreigns: Vec::new(),
macros: Vec::new(),
is_crate: false,
from_glob: false,
}
}

pub(crate) fn push_item(&mut self, new_item: Item<'hir>) {
for item_iter in self.items.iter_mut() {
if item_iter.name() == new_item.name() {
if item_iter.from_glob {
debug!("push_item: {:?} shadowed by {:?}", *item_iter, new_item);
*item_iter = new_item;
return;
} else if new_item.from_glob {
return;
}
}
}
self.items.push(new_item);
}

pub(crate) fn push_mod(&mut self, new_item: Module<'hir>) {
for item_iter in self.mods.iter_mut() {
if item_iter.name == new_item.name {
longfangsong marked this conversation as resolved.
Show resolved Hide resolved
if item_iter.from_glob {
debug!("push_mod: {:?} shadowed by {:?}", item_iter.name, new_item.name);
*item_iter = new_item;
return;
} else if new_item.from_glob {
return;
}
}
}
self.mods.push(new_item);
}
}
27 changes: 19 additions & 8 deletions src/librustdoc/visit_ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
hir::CRATE_HIR_ID,
&krate.item,
self.cx.tcx.crate_name,
false,
);
top_level_module.is_crate = true;
// Attach the crate's exported macros to the top-level module.
Expand Down Expand Up @@ -134,17 +135,19 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
id: hir::HirId,
m: &'tcx hir::Mod<'tcx>,
name: Symbol,
from_glob: bool,
) -> Module<'tcx> {
let mut om = Module::new(name);
om.where_outer = span;
om.where_inner = m.inner;
om.id = id;
om.from_glob = from_glob;
// Keep track of if there were any private modules in the path.
let orig_inside_public_path = self.inside_public_path;
self.inside_public_path &= vis.node.is_pub();
for &i in m.item_ids {
let item = self.cx.tcx.hir().item(i);
self.visit_item(item, None, &mut om);
self.visit_item(item, None, &mut om, from_glob);
}
self.inside_public_path = orig_inside_public_path;
om
Expand Down Expand Up @@ -225,14 +228,14 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
let prev = mem::replace(&mut self.inlining, true);
for &i in m.item_ids {
let i = self.cx.tcx.hir().item(i);
self.visit_item(i, None, om);
self.visit_item(i, None, om, glob);
}
self.inlining = prev;
true
}
Node::Item(it) if !glob => {
let prev = mem::replace(&mut self.inlining, true);
self.visit_item(it, renamed, om);
self.visit_item(it, renamed, om, glob);
self.inlining = prev;
true
}
Expand All @@ -257,6 +260,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
item: &'tcx hir::Item<'_>,
renamed: Option<Symbol>,
om: &mut Module<'tcx>,
from_glob: bool,
) {
debug!("visiting item {:?}", item);
let name = renamed.unwrap_or(item.ident.name);
Expand Down Expand Up @@ -309,10 +313,17 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
}
}

om.items.push((item, renamed))
om.push_item(Item::new(item, renamed, is_glob))
longfangsong marked this conversation as resolved.
Show resolved Hide resolved
}
hir::ItemKind::Mod(ref m) => {
om.mods.push(self.visit_mod_contents(item.span, &item.vis, item.hir_id(), m, name));
om.push_mod(self.visit_mod_contents(
item.span,
&item.vis,
item.hir_id(),
m,
name,
from_glob,
Copy link
Member

@jyn514 jyn514 Apr 9, 2021

Choose a reason for hiding this comment

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

Hmm, are you sure from_glob should apply recursively like this? What happens if you have

pub mod outer {
  pub const X: usize = 0;
  pub mod inner {
    pub use super::*;
    pub const X: usize = 1;
    pub const Y: usize = 2;
  }
}
pub use outer::inner::*;

Shouldn't inner::X take precedence over outer::X?

Copy link
Contributor

Choose a reason for hiding this comment

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

This could be set as a test case to prevent regression.

));
}
hir::ItemKind::Fn(..)
| hir::ItemKind::ExternCrate(..)
Expand All @@ -323,19 +334,19 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
| hir::ItemKind::OpaqueTy(..)
| hir::ItemKind::Static(..)
| hir::ItemKind::Trait(..)
| hir::ItemKind::TraitAlias(..) => om.items.push((item, renamed)),
| hir::ItemKind::TraitAlias(..) => om.push_item(Item::new(item, renamed, from_glob)),
hir::ItemKind::Const(..) => {
// Underscore constants do not correspond to a nameable item and
// so are never useful in documentation.
if name != kw::Underscore {
om.items.push((item, renamed));
om.push_item(Item::new(item, renamed, from_glob));
}
}
hir::ItemKind::Impl(ref impl_) => {
// Don't duplicate impls when inlining or if it's implementing a trait, we'll pick
// them up regardless of where they're located.
if !self.inlining && impl_.of_trait.is_none() {
om.items.push((item, None));
om.push_item(Item::new(item, None, from_glob));
}
}
}
Expand Down
38 changes: 38 additions & 0 deletions src/test/rustdoc/glob-shadowing.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// @has 'glob_shadowing/index.html'
// @count - '//tr[@class="module-item"]' 2
longfangsong marked this conversation as resolved.
Show resolved Hide resolved
// @has - '//tr[@class="module-item"]' 'mod::prelude'
// @has - '//tr[@class="module-item"]' 'sub2::describe'

mod sub1 {
/// sub1::describe
pub fn describe() -> &'static str {
"sub1::describe"
}

/// sub1::prelude
pub mod prelude {
pub use super::describe;
}
}

mod sub2 {
/// sub2::describe
pub fn describe() -> &'static str {
"sub2::describe"
}
}

/// mod::prelude
pub mod prelude {
/// mod::prelude::describe
pub fn describe() -> &'static str {
"mod::describe"
}
longfangsong marked this conversation as resolved.
Show resolved Hide resolved
}

#[doc(inline)]
pub use sub2::describe;
longfangsong marked this conversation as resolved.
Show resolved Hide resolved

#[doc(inline)]
pub use sub1::*;

longfangsong marked this conversation as resolved.
Show resolved Hide resolved