Skip to content

Commit

Permalink
Auto merge of #104889 - GuillaumeGomez:fix-impl-block-in-const-expr, …
Browse files Browse the repository at this point in the history
…r=notriddle

Fix impl block in const expr

Fixes #83026.

The problem was that we didn't visit block expressions. Considering how big the [walk_expr](https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_hir/intravisit.rs.html#678) function is, I decided to instead implement the `hir` visitor on the struct. It also answers the question which was in a comment for `RustdocVisitor`: we should have used a visitor instead of our ad-hoc implementation.

Adding this visitor also added some extra checks that weren't present before (check changes in `rustdoc-ui` tests).

r? `@notriddle`
  • Loading branch information
bors committed Dec 22, 2022
2 parents 75f4ee8 + a954d63 commit cce9e72
Show file tree
Hide file tree
Showing 7 changed files with 193 additions and 56 deletions.
2 changes: 1 addition & 1 deletion src/librustdoc/html/render/write_shared.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ pub(super) fn write_shared(
Ok((ret, krates))
}

/// Read a file and return all lines that match the <code>"{crate}":{data},\</code> format,
/// Read a file and return all lines that match the <code>"{crate}":{data},\ </code> format,
/// and return a tuple `(Vec<DataString>, Vec<CrateNameString>)`.
///
/// This forms the payload of files that look like this:
Expand Down
163 changes: 115 additions & 48 deletions src/librustdoc/visit_ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_hir as hir;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::def_id::DefId;
use rustc_hir::intravisit::{walk_item, Visitor};
use rustc_hir::Node;
use rustc_hir::CRATE_HIR_ID;
use rustc_middle::hir::nested_filter;
use rustc_middle::ty::TyCtxt;
use rustc_span::def_id::{CRATE_DEF_ID, LOCAL_CRATE};
use rustc_span::symbol::{kw, sym, Symbol};
Expand Down Expand Up @@ -57,29 +59,34 @@ pub(crate) fn inherits_doc_hidden(tcx: TyCtxt<'_>, mut node: hir::HirId) -> bool
false
}

// Also, is there some reason that this doesn't use the 'visit'
// framework from syntax?.

pub(crate) struct RustdocVisitor<'a, 'tcx> {
cx: &'a mut core::DocContext<'tcx>,
view_item_stack: FxHashSet<hir::HirId>,
inlining: bool,
/// Are the current module and all of its parents public?
inside_public_path: bool,
exact_paths: FxHashMap<DefId, Vec<Symbol>>,
modules: Vec<Module<'tcx>>,
}

impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
pub(crate) fn new(cx: &'a mut core::DocContext<'tcx>) -> RustdocVisitor<'a, 'tcx> {
// If the root is re-exported, terminate all recursion.
let mut stack = FxHashSet::default();
stack.insert(hir::CRATE_HIR_ID);
let om = Module::new(
cx.tcx.crate_name(LOCAL_CRATE),
hir::CRATE_HIR_ID,
cx.tcx.hir().root_module().spans.inner_span,
);

RustdocVisitor {
cx,
view_item_stack: stack,
inlining: false,
inside_public_path: true,
exact_paths: FxHashMap::default(),
modules: vec![om],
}
}

Expand All @@ -89,12 +96,10 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
}

pub(crate) fn visit(mut self) -> Module<'tcx> {
let mut top_level_module = self.visit_mod_contents(
hir::CRATE_HIR_ID,
self.cx.tcx.hir().root_module(),
self.cx.tcx.crate_name(LOCAL_CRATE),
None,
);
let root_module = self.cx.tcx.hir().root_module();
self.visit_mod_contents(CRATE_HIR_ID, root_module);

let mut top_level_module = self.modules.pop().unwrap();

// `#[macro_export] macro_rules!` items are reexported at the top level of the
// crate, regardless of where they're defined. We want to document the
Expand All @@ -109,15 +114,13 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
// macro in the same module.
let mut inserted = FxHashSet::default();
for export in self.cx.tcx.module_reexports(CRATE_DEF_ID).unwrap_or(&[]) {
if let Res::Def(DefKind::Macro(_), def_id) = export.res {
if let Some(local_def_id) = def_id.as_local() {
if self.cx.tcx.has_attr(def_id, sym::macro_export) {
if inserted.insert(def_id) {
let item = self.cx.tcx.hir().expect_item(local_def_id);
top_level_module.items.push((item, None, None));
}
}
}
if let Res::Def(DefKind::Macro(_), def_id) = export.res &&
let Some(local_def_id) = def_id.as_local() &&
self.cx.tcx.has_attr(def_id, sym::macro_export) &&
inserted.insert(def_id)
{
let item = self.cx.tcx.hir().expect_item(local_def_id);
top_level_module.items.push((item, None, None));
}
}

Expand Down Expand Up @@ -151,36 +154,34 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
top_level_module
}

fn visit_mod_contents(
&mut self,
id: hir::HirId,
m: &'tcx hir::Mod<'tcx>,
name: Symbol,
parent_id: Option<hir::HirId>,
) -> Module<'tcx> {
let mut om = Module::new(name, id, m.spans.inner_span);
/// This method will go through the given module items in two passes:
/// 1. The items which are not glob imports/reexports.
/// 2. The glob imports/reexports.
fn visit_mod_contents(&mut self, id: hir::HirId, m: &'tcx hir::Mod<'tcx>) {
debug!("Going through module {:?}", m);
let def_id = self.cx.tcx.hir().local_def_id(id).to_def_id();
// 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 &= self.cx.tcx.visibility(def_id).is_public();

// Reimplementation of `walk_mod` because we need to do it in two passes (explanations in
// the second loop):
for &i in m.item_ids {
let item = self.cx.tcx.hir().item(i);
if matches!(item.kind, hir::ItemKind::Use(_, hir::UseKind::Glob)) {
continue;
if !matches!(item.kind, hir::ItemKind::Use(_, hir::UseKind::Glob)) {
self.visit_item(item);
}
self.visit_item(item, None, &mut om, parent_id);
}
for &i in m.item_ids {
let item = self.cx.tcx.hir().item(i);
// To match the way import precedence works, visit glob imports last.
// Later passes in rustdoc will de-duplicate by name and kind, so if glob-
// imported items appear last, then they'll be the ones that get discarded.
if matches!(item.kind, hir::ItemKind::Use(_, hir::UseKind::Glob)) {
self.visit_item(item, None, &mut om, parent_id);
self.visit_item(item);
}
}
self.inside_public_path = orig_inside_public_path;
om
}

/// Tries to resolve the target of a `pub use` statement and inlines the
Expand All @@ -198,7 +199,6 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
res: Res,
renamed: Option<Symbol>,
glob: bool,
om: &mut Module<'tcx>,
please_inline: bool,
) -> bool {
debug!("maybe_inline_local res: {:?}", res);
Expand Down Expand Up @@ -249,20 +249,20 @@ 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, Some(id));
self.visit_item_inner(i, None, Some(id));
}
self.inlining = prev;
true
}
Node::Item(it) if !glob => {
let prev = mem::replace(&mut self.inlining, true);
self.visit_item(it, renamed, om, Some(id));
self.visit_item_inner(it, renamed, Some(id));
self.inlining = prev;
true
}
Node::ForeignItem(it) if !glob => {
let prev = mem::replace(&mut self.inlining, true);
self.visit_foreign_item(it, renamed, om);
self.visit_foreign_item_inner(it, renamed);
self.inlining = prev;
true
}
Expand All @@ -272,13 +272,22 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
ret
}

fn visit_item(
#[inline]
fn add_to_current_mod(
&mut self,
item: &'tcx hir::Item<'_>,
renamed: Option<Symbol>,
om: &mut Module<'tcx>,
parent_id: Option<hir::HirId>,
) {
self.modules.last_mut().unwrap().items.push((item, renamed, parent_id))
}

fn visit_item_inner(
&mut self,
item: &'tcx hir::Item<'_>,
renamed: Option<Symbol>,
parent_id: Option<hir::HirId>,
) -> bool {
debug!("visiting item {:?}", item);
let name = renamed.unwrap_or(item.ident.name);

Expand All @@ -293,7 +302,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
hir::ItemKind::ForeignMod { items, .. } => {
for item in items {
let item = self.cx.tcx.hir().foreign_item(item.id);
self.visit_foreign_item(item, None, om);
self.visit_foreign_item_inner(item, None);
}
}
// If we're inlining, skip private items or item reexported as "_".
Expand Down Expand Up @@ -326,14 +335,13 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
res,
ident,
is_glob,
om,
please_inline,
) {
continue;
}
}

om.items.push((item, renamed, parent_id))
self.add_to_current_mod(item, renamed, parent_id);
}
}
hir::ItemKind::Macro(ref macro_def, _) => {
Expand All @@ -353,11 +361,11 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
let nonexported = !self.cx.tcx.has_attr(def_id, sym::macro_export);

if is_macro_2_0 || nonexported || self.inlining {
om.items.push((item, renamed, None));
self.add_to_current_mod(item, renamed, None);
}
}
hir::ItemKind::Mod(ref m) => {
om.mods.push(self.visit_mod_contents(item.hir_id(), m, name, parent_id));
self.enter_mod(item.hir_id(), m, name);
}
hir::ItemKind::Fn(..)
| hir::ItemKind::ExternCrate(..)
Expand All @@ -368,33 +376,92 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
| hir::ItemKind::OpaqueTy(..)
| hir::ItemKind::Static(..)
| hir::ItemKind::Trait(..)
| hir::ItemKind::TraitAlias(..) => om.items.push((item, renamed, parent_id)),
| hir::ItemKind::TraitAlias(..) => {
self.add_to_current_mod(item, renamed, parent_id);
}
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, parent_id));
self.add_to_current_mod(item, renamed, parent_id);
}
}
hir::ItemKind::Impl(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, None));
self.add_to_current_mod(item, None, None);
}
}
}
true
}

fn visit_foreign_item(
fn visit_foreign_item_inner(
&mut self,
item: &'tcx hir::ForeignItem<'_>,
renamed: Option<Symbol>,
om: &mut Module<'tcx>,
) {
// If inlining we only want to include public functions.
if !self.inlining || self.cx.tcx.visibility(item.owner_id).is_public() {
om.foreigns.push((item, renamed));
self.modules.last_mut().unwrap().foreigns.push((item, renamed));
}
}

/// This method will create a new module and push it onto the "modules stack" then call
/// `visit_mod_contents`. Once done, it'll remove it from the "modules stack" and instead
/// add into into the list of modules of the current module.
fn enter_mod(&mut self, id: hir::HirId, m: &'tcx hir::Mod<'tcx>, name: Symbol) {
self.modules.push(Module::new(name, id, m.spans.inner_span));

self.visit_mod_contents(id, m);

let last = self.modules.pop().unwrap();
self.modules.last_mut().unwrap().mods.push(last);
}
}

// We need to implement this visitor so it'll go everywhere and retrieve items we're interested in
// such as impl blocks in const blocks.
impl<'a, 'tcx> Visitor<'tcx> for RustdocVisitor<'a, 'tcx> {
type NestedFilter = nested_filter::All;

fn nested_visit_map(&mut self) -> Self::Map {
self.cx.tcx.hir()
}

fn visit_item(&mut self, i: &'tcx hir::Item<'tcx>) {
let parent_id = if self.modules.len() > 1 {
Some(self.modules[self.modules.len() - 2].id)
} else {
None
};
if self.visit_item_inner(i, None, parent_id) {
walk_item(self, i);
}
}

fn visit_mod(&mut self, _: &hir::Mod<'tcx>, _: Span, _: hir::HirId) {
// Handled in `visit_item_inner`
}

fn visit_use(&mut self, _: &hir::UsePath<'tcx>, _: hir::HirId) {
// Handled in `visit_item_inner`
}

fn visit_path(&mut self, _: &hir::Path<'tcx>, _: hir::HirId) {
// Handled in `visit_item_inner`
}

fn visit_label(&mut self, _: &rustc_ast::Label) {
// Unneeded.
}

fn visit_infer(&mut self, _: &hir::InferArg) {
// Unneeded.
}

fn visit_lifetime(&mut self, _: &hir::Lifetime) {
// Unneeded.
}
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
// check-pass
// normalize-stderr-test: "`.*`" -> "`DEF_ID`"
// normalize-stdout-test: "`.*`" -> "`DEF_ID`"
// edition:2018

pub async fn f() -> impl std::fmt::Debug {
// rustdoc doesn't care that this is infinitely sized
#[derive(Debug)]
enum E {
enum E { //~ ERROR
This(E),
Unit,
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
error[E0072]: recursive type `DEF_ID` has infinite size
--> $DIR/infinite-recursive-type-impl-trait-return.rs:7:5
|
LL | enum E {
| ^^^^^^
LL | This(E),
| - recursive without indirection
|
help: insert some indirection (e.g., a `DEF_ID`) to break the cycle
|
LL | This(Box<E>),
| ++++ +

error: aborting due to previous error

For more information about this error, try `DEF_ID`.
5 changes: 1 addition & 4 deletions src/test/rustdoc-ui/infinite-recursive-type-impl-trait.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
// check-pass

fn f() -> impl Sized {
// rustdoc doesn't care that this is infinitely sized
enum E {
enum E { //~ ERROR
V(E),
}
unimplemented!()
Expand Down
16 changes: 16 additions & 0 deletions src/test/rustdoc-ui/infinite-recursive-type-impl-trait.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
error[E0072]: recursive type `f::E` has infinite size
--> $DIR/infinite-recursive-type-impl-trait.rs:2:5
|
LL | enum E {
| ^^^^^^
LL | V(E),
| - recursive without indirection
|
help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to break the cycle
|
LL | V(Box<E>),
| ++++ +

error: aborting due to previous error

For more information about this error, try `rustc --explain E0072`.
Loading

0 comments on commit cce9e72

Please sign in to comment.