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 15 commits
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
7 changes: 4 additions & 3 deletions src/librustdoc/clean/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1934,13 +1934,14 @@ 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 = self.hir_item;
let mut name = self.name();
let def_id = item.def_id.to_def_id();
let mut name = renamed.unwrap_or_else(|| cx.tcx.hir().name(item.hir_id()));

cx.with_param_env(def_id, |cx| {
let kind = match item.kind {
ItemKind::Static(ty, mutability, body_id) => {
Expand Down
93 changes: 90 additions & 3 deletions src/librustdoc/doctree.rs
Original file line number Diff line number Diff line change
@@ -1,20 +1,50 @@
//! This module is used to store stuff from Rust's AST in a more convenient
//! manner (and with prettier names) before cleaning.
use crate::core::DocContext;
use rustc_hir as hir;
use rustc_span::{self, Span, Symbol};

use rustc_hir as hir;
/// A wrapper around a [`hir::Item`].
#[derive(Debug)]
crate struct Item<'hir> {
/// the wrapped item
crate hir_item: &'hir hir::Item<'hir>,
/// the explicit renamed name
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 and namespace,
/// then this item can be replaced with that one
crate from_glob: bool,
}

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 }
}

pub(crate) fn name(&self) -> Symbol {
self.renamed_name.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,
// (item, renamed)
crate items: Vec<(&'hir hir::Item<'hir>, Option<Symbol>)>,
crate items: Vec<Item<'hir>>,
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 module can be replaced with that one
crate from_glob: bool,
}

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

pub(crate) fn push_item(&mut self, ctx: &DocContext<'_>, new_item: Item<'hir>) {
let new_item_ns = ctx.tcx.def_kind(new_item.hir_item.def_id).ns();
if !new_item.name().is_empty() && new_item_ns.is_some() {
Copy link
Contributor

@pickfire pickfire Apr 7, 2021

Choose a reason for hiding this comment

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

Is it possible for new_item.name() to be empty? And is it possible that new_item_ns is empty?

If both is not possible maybe we can remove this branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible for new_item.name() to be empty? And is it possible that new_item_ns is empty?

The only situation I found which new_item.name().is_empty() is std::prelude (but IMO we don't need to care about it), remove this check seems won't affect the test result. But this comment mentioned some possible situation. @ollie27 can you give a more detailed example?

On the other hand, new_item_ns can be empty in many situations like macro derives (a minimal example is Debug in src/test/rustdoc/wrapping.rs). If we remove this check, then adding those items may cause panic on L88.

Copy link
Member

Choose a reason for hiding this comment

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

new_item.name can be empty for impl blocks - or if it's not and it's using {{impl}} or something, that's another bug, because impl blocks shouldn't use globbing logic, they don't have have a namespace.

Copy link
Contributor

@pickfire pickfire Apr 8, 2021

Choose a reason for hiding this comment

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

So means in this case both checks are still necessary? Maybe we should leave a comment or it's not necessary?

And should the test includes these cases?

if let Some(existing_item) = self.items.iter_mut().find(|item| {
item.name() == new_item.name()
&& ctx.tcx.def_kind(item.hir_item.def_id).ns() == new_item_ns
}) {
match (existing_item.from_glob, new_item.from_glob) {
(true, _) => {
// `existing_item` is from glob, no matter whether `new_item` is from glob,
// `new_item` should always shadow `existing_item`
debug!("push_item: {:?} shadowed by {:?}", existing_item, new_item);
*existing_item = new_item;
return;
}
(false, true) => {
// `existing_item` is not from glob but `new_item` is,
// just keep `existing_item` and return at once
return;
}
(false, false) => {
// should report "defined multiple time" error before reach this
unreachable!()
}
}
}
}
// no item with same name and namespace exists, just collect `new_item`
self.items.push(new_item);
}

pub(crate) fn push_mod(&mut self, new_mod: Module<'hir>) {
if let Some(existing_mod) = self.mods.iter_mut().find(|mod_| mod_.name == new_mod.name) {
match (existing_mod.from_glob, new_mod.from_glob) {
(true, _) => {
// `existing_mod` is from glob, no matter whether `new_mod` is from glob,
// `new_mod` should always shadow `existing_mod`
debug!("push_mod: {:?} shadowed by {:?}", existing_mod.name, new_mod.name);
*existing_mod = new_mod;
longfangsong marked this conversation as resolved.
Show resolved Hide resolved
return;
}
(false, true) => {
// `existing_mod` is not from glob but `new_mod` is,
// just keep `existing_mod` and return at once
return;
}
(false, false) => {
// should report "defined multiple time" error before reach this
unreachable!()
}
}
}
// no mod with same name exists, just collect `new_mod`
self.mods.push(new_mod);
}
}
29 changes: 21 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(self.cx, Item::new(item, renamed, from_glob))
}
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,21 @@ 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(self.cx, 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(self.cx, 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(self.cx, Item::new(item, None, from_glob));
}
}
}
Expand Down
47 changes: 47 additions & 0 deletions src/test/rustdoc/glob-shadowing.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// @has 'glob_shadowing/index.html'
// @count - '//tr[@class="module-item"]' 4
// @!has - '//tr[@class="module-item"]' 'sub1::describe'
// @has - '//tr[@class="module-item"]' 'mod::prelude'
// @has - '//tr[@class="module-item"]' 'sub2::describe'
// @has - '//tr[@class="module-item"]' 'sub1::Foo (struct)'
// @has - '//tr[@class="module-item"]' 'mod::Foo (function)'

// @has 'glob_shadowing/fn.describe.html'
// @has - '//div[@class="docblock"]' 'sub2::describe'

mod sub1 {
// this should be shadowed by sub2::describe
/// sub1::describe
pub fn describe() -> &'static str {
"sub1::describe"
}

// this should be shadowed by mod::prelude
/// sub1::prelude
pub mod prelude {
pub use super::describe;
}

// this should not be shadowed, because sub1::Foo and mod::Foo are in different namespace
/// sub1::Foo (struct)
pub struct Foo;
}

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

/// mod::prelude
pub mod prelude {}

/// mod::Foo (function)
pub fn Foo() {}

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

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