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

Fix impl block in const expr #104889

Merged
merged 6 commits into from
Dec 22, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
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
Copy link
Member

Choose a reason for hiding this comment

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

Tracing the history this essentially appears to be a regression test for #75100 (added in #75127). Making this fail may break some docs that rely on function bodys not being fully type-checked.

Copy link
Member

Choose a reason for hiding this comment

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

(@jyn514 might have more insight whether this specific test came from some real-world code, or is ok to regress).

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Dec 2, 2022

Choose a reason for hiding this comment

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

Erf, that'd be pretty bad if some docs rely on that (meaning if this PR introduces regression for them). At the very least, we should run a crater run to see what the impact is. If there is none, maybe we can consider changing this behaviour? I'd really prefer to avoid rewriting the hir::Visitor if possible.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I should have looked at the original test impl. It was added as a failing test then got changed to check-pass in #102890. So seems likely that it's not relied on in the wild as it isn't yet in a stable release.

Copy link
Member Author

Choose a reason for hiding this comment

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

But your concerns remain completely valid. I think a crater run should definitely be run if we agree on this PR before r+ing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's not in a stable release, then it shouldn't be a problem.

However, this PR changes other long-standing behaviors, such as causing lints to run on doc comments of nested functions. Probably a lot more crates rely on that.

Copy link
Member Author

Choose a reason for hiding this comment

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

So what should we do here? Is it ok and then we run a crater or this approach isn't the right one and we close this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I should have looked at the original test impl. It was added as a failing test then got changed to check-pass in #102890. So seems likely that it's not relied on in the wild as it isn't yet in a stable release.

That sounds right - the original test was just to make sure rustdoc didn't ICE.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be okay with cratering and calling it good. Rust's stability promise reserves the right to change lints, which is what broke rust-lang/rust, so as long as that's the only thing that happened, it's fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll start a crater run soon then.

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