From 1f7bce012d7df14e3e5d8e595fb6f557b677bd92 Mon Sep 17 00:00:00 2001 From: inquisitivecrystal <22333129+inquisitivecrystal@users.noreply.github.com> Date: Thu, 5 Aug 2021 16:58:46 -0700 Subject: [PATCH] Teach tools that macros are now HIR items --- src/librustdoc/clean/mod.rs | 23 +---- src/librustdoc/doctest.rs | 28 +++--- src/librustdoc/doctree.rs | 12 +-- src/librustdoc/visit_ast.rs | 91 +++++++++---------- .../clippy/clippy_lints/src/missing_doc.rs | 2 +- .../clippy/clippy_lints/src/missing_inline.rs | 1 + .../clippy_lints/src/utils/inspector.rs | 7 ++ 7 files changed, 72 insertions(+), 92 deletions(-) diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index b6ff3890c58..640acffb114 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -91,7 +91,6 @@ impl Clean for doctree::Module<'_> { items.extend(self.foreigns.iter().map(|x| x.clean(cx))); items.extend(self.mods.iter().map(|x| x.clean(cx))); items.extend(self.items.iter().map(|x| x.clean(cx)).flatten()); - items.extend(self.macros.iter().map(|x| x.clean(cx))); // determine if we should display the inner contents or // the outer `mod` item for the source code. @@ -1861,6 +1860,10 @@ impl Clean> for (&hir::Item<'_>, Option) { ItemKind::Fn(ref sig, ref generics, body_id) => { clean_fn_or_proc_macro(item, sig, generics, body_id, &mut name, cx) } + ItemKind::Macro(ref macro_def) => MacroItem(Macro { + source: display_macro_source(cx, name, ¯o_def, def_id, &item.vis), + imported_from: None, + }), ItemKind::Trait(is_auto, unsafety, ref generics, ref bounds, ref item_ids) => { let items = item_ids .iter() @@ -2138,24 +2141,6 @@ impl Clean for (&hir::ForeignItem<'_>, Option) { } } -impl Clean for (&hir::MacroDef<'_>, Option) { - fn clean(&self, cx: &mut DocContext<'_>) -> Item { - let (item, renamed) = self; - let name = renamed.unwrap_or(item.ident.name); - let def_id = item.def_id.to_def_id(); - - Item::from_hir_id_and_parts( - item.hir_id(), - Some(name), - MacroItem(Macro { - source: display_macro_source(cx, name, &item.ast, def_id, &item.vis), - imported_from: None, - }), - cx, - ) - } -} - impl Clean for hir::TypeBinding<'_> { fn clean(&self, cx: &mut DocContext<'_>) -> TypeBinding { TypeBinding { name: self.ident.name, kind: self.kind.clean(cx) } diff --git a/src/librustdoc/doctest.rs b/src/librustdoc/doctest.rs index 083d82cb414..bf14a17c076 100644 --- a/src/librustdoc/doctest.rs +++ b/src/librustdoc/doctest.rs @@ -1171,10 +1171,21 @@ impl<'a, 'hir, 'tcx> intravisit::Visitor<'hir> for HirCollector<'a, 'hir, 'tcx> } fn visit_item(&mut self, item: &'hir hir::Item<'_>) { - let name = if let hir::ItemKind::Impl(impl_) = &item.kind { - rustc_hir_pretty::id_to_string(&self.map, impl_.self_ty.hir_id) - } else { - item.ident.to_string() + let name = match &item.kind { + hir::ItemKind::Macro(ref macro_def) => { + // FIXME(#88038): Non exported macros have historically not been tested, + // but we really ought to start testing them. + let def_id = item.def_id.to_def_id(); + if macro_def.macro_rules && !self.tcx.has_attr(def_id, sym::macro_export) { + intravisit::walk_item(self, item); + return; + } + item.ident.to_string() + } + hir::ItemKind::Impl(impl_) => { + rustc_hir_pretty::id_to_string(&self.map, impl_.self_ty.hir_id) + } + _ => item.ident.to_string(), }; self.visit_testable(name, item.hir_id(), item.span, |this| { @@ -1216,15 +1227,6 @@ impl<'a, 'hir, 'tcx> intravisit::Visitor<'hir> for HirCollector<'a, 'hir, 'tcx> intravisit::walk_field_def(this, f); }); } - - fn visit_macro_def(&mut self, macro_def: &'hir hir::MacroDef<'_>) { - self.visit_testable( - macro_def.ident.to_string(), - macro_def.hir_id(), - macro_def.span, - |_| (), - ); - } } #[cfg(test)] diff --git a/src/librustdoc/doctree.rs b/src/librustdoc/doctree.rs index eadac89f79e..8f1e8f277c5 100644 --- a/src/librustdoc/doctree.rs +++ b/src/librustdoc/doctree.rs @@ -5,6 +5,7 @@ use rustc_span::{self, Span, Symbol}; use rustc_hir as hir; +#[derive(Debug)] crate struct Module<'hir> { crate name: Symbol, crate where_inner: Span, @@ -13,20 +14,11 @@ crate struct Module<'hir> { // (item, renamed) crate items: Vec<(&'hir hir::Item<'hir>, Option)>, crate foreigns: Vec<(&'hir hir::ForeignItem<'hir>, Option)>, - crate macros: Vec<(&'hir hir::MacroDef<'hir>, Option)>, } impl Module<'hir> { crate fn new(name: Symbol, id: hir::HirId, where_inner: Span) -> Module<'hir> { - Module { - name, - id, - where_inner, - mods: Vec::new(), - items: Vec::new(), - foreigns: Vec::new(), - macros: Vec::new(), - } + Module { name, id, where_inner, mods: Vec::new(), items: Vec::new(), foreigns: Vec::new() } } crate fn where_outer(&self, tcx: TyCtxt<'_>) -> Span { diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs index e2891035535..897b9140fc8 100644 --- a/src/librustdoc/visit_ast.rs +++ b/src/librustdoc/visit_ast.rs @@ -9,7 +9,7 @@ use rustc_hir::Node; use rustc_middle::middle::privacy::AccessLevel; use rustc_middle::ty::TyCtxt; use rustc_span; -use rustc_span::def_id::LOCAL_CRATE; +use rustc_span::def_id::{CRATE_DEF_ID, LOCAL_CRATE}; use rustc_span::source_map::Spanned; use rustc_span::symbol::{kw, sym, Symbol}; @@ -79,49 +79,23 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { &krate.module(), self.cx.tcx.crate_name(LOCAL_CRATE), ); - // Attach the crate's exported macros to the top-level module. - // In the case of macros 2.0 (`pub macro`), and for built-in `derive`s or attributes as - // well (_e.g._, `Copy`), these are wrongly bundled in there too, so we need to fix that by - // moving them back to their correct locations. - 'exported_macros: for def in krate.exported_macros() { - // The `def` of a macro in `exported_macros` should correspond to either: - // - a `#[macro_export] macro_rules!` macro, - // - a built-in `derive` (or attribute) macro such as the ones in `::core`, - // - a `pub macro`. - // Only the last two need to be fixed, thus: - if def.ast.macro_rules { - top_level_module.macros.push((def, None)); - continue 'exported_macros; - } - let tcx = self.cx.tcx; - // Note: this is not the same as `.parent_module()`. Indeed, the latter looks - // for the closest module _ancestor_, which is not necessarily a direct parent - // (since a direct parent isn't necessarily a module, c.f. #77828). - let macro_parent_def_id = { - use rustc_middle::ty::DefIdTree; - tcx.parent(def.def_id.to_def_id()).unwrap() - }; - let macro_parent_path = tcx.def_path(macro_parent_def_id); - // HACK: rustdoc has no way to lookup `doctree::Module`s by their HirId. Instead, - // lookup the module by its name, by looking at each path segment one at a time. - let mut cur_mod = &mut top_level_module; - for path_segment in macro_parent_path.data { - // Path segments may refer to a module (in which case they belong to the type - // namespace), which is _necessary_ for the macro to be accessible outside it - // (no "associated macros" as of yet). Else we bail with an outer `continue`. - let path_segment_ty_ns = match path_segment.data { - rustc_hir::definitions::DefPathData::TypeNs(symbol) => symbol, - _ => continue 'exported_macros, - }; - // Descend into the child module that matches this path segment (if any). - match cur_mod.mods.iter_mut().find(|child| child.name == path_segment_ty_ns) { - Some(child_mod) => cur_mod = &mut *child_mod, - None => continue 'exported_macros, + + // `#[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 + // top level rexport of the macro, not its original definition, since + // the rexport defines the path that a user will actually see. Accordingly, + // we add the rexport as an item here, and then skip over the original + // definition in `visit_item()` below. + for export in self.cx.tcx.module_exports(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) { + let hir_id = self.cx.tcx.hir().local_def_id_to_hir_id(local_def_id); + let item = self.cx.tcx.hir().expect_item(hir_id); + top_level_module.items.push((item, None)); + } } } - let cur_mod_def_id = tcx.hir().local_def_id(cur_mod.id).to_def_id(); - assert_eq!(cur_mod_def_id, macro_parent_def_id); - cur_mod.macros.push((def, None)); } self.cx.cache.exact_paths = self.exact_paths; top_level_module @@ -238,10 +212,6 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { self.inlining = prev; true } - Node::MacroDef(def) if !glob => { - om.macros.push((def, renamed)); - true - } _ => false, }; self.view_item_stack.remove(&res_hir_id); @@ -257,7 +227,10 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { debug!("visiting item {:?}", item); let name = renamed.unwrap_or(item.ident.name); - if item.vis.node.is_pub() { + let def_id = item.def_id.to_def_id(); + let is_pub = item.vis.node.is_pub() || self.cx.tcx.has_attr(def_id, sym::macro_export); + + if is_pub { self.store_path(item.def_id.to_def_id()); } @@ -269,7 +242,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { } } // If we're inlining, skip private items. - _ if self.inlining && !item.vis.node.is_pub() => {} + _ if self.inlining && !is_pub => {} hir::ItemKind::GlobalAsm(..) => {} hir::ItemKind::Use(_, hir::UseKind::ListStem) => {} hir::ItemKind::Use(ref path, kind) => { @@ -285,7 +258,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { // If there was a private module in the current path then don't bother inlining // anything as it will probably be stripped anyway. - if item.vis.node.is_pub() && self.inside_public_path { + if is_pub && self.inside_public_path { let please_inline = attrs.iter().any(|item| match item.meta_item_list() { Some(ref list) if item.has_name(sym::doc) => { list.iter().any(|i| i.has_name(sym::inline)) @@ -307,6 +280,26 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { om.items.push((item, renamed)) } + hir::ItemKind::Macro(ref macro_def) => { + // `#[macro_export] macro_rules!` items are handled seperately in `visit()`, + // above, since they need to be documented at the module top level. Accordingly, + // we only want to handle macros if one of three conditions holds: + // + // 1. This macro was defined by `macro`, and thus isn't covered by the case + // above. + // 2. This macro isn't marked with `#[macro_export]`, and thus isn't covered + // by the case above. + // 3. We're inlining, since a reexport where inlining has been requested + // should be inlined even if it is also documented at the top level. + + let def_id = item.def_id.to_def_id(); + let is_macro_2_0 = !macro_def.macro_rules; + 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)); + } + } hir::ItemKind::Mod(ref m) => { om.mods.push(self.visit_mod_contents(&item.vis, item.hir_id(), m, name)); } diff --git a/src/tools/clippy/clippy_lints/src/missing_doc.rs b/src/tools/clippy/clippy_lints/src/missing_doc.rs index da86d28ee0b..940eee7a788 100644 --- a/src/tools/clippy/clippy_lints/src/missing_doc.rs +++ b/src/tools/clippy/clippy_lints/src/missing_doc.rs @@ -122,8 +122,8 @@ impl<'tcx> LateLintPass<'tcx> for MissingDoc { }, hir::ItemKind::Const(..) | hir::ItemKind::Enum(..) - | hir::ItemKind::Mod(..) | hir::ItemKind::Macro(..) + | hir::ItemKind::Mod(..) | hir::ItemKind::Static(..) | hir::ItemKind::Struct(..) | hir::ItemKind::Trait(..) diff --git a/src/tools/clippy/clippy_lints/src/missing_inline.rs b/src/tools/clippy/clippy_lints/src/missing_inline.rs index 977e6d966e8..667cdd83025 100644 --- a/src/tools/clippy/clippy_lints/src/missing_inline.rs +++ b/src/tools/clippy/clippy_lints/src/missing_inline.rs @@ -118,6 +118,7 @@ impl<'tcx> LateLintPass<'tcx> for MissingInline { }, hir::ItemKind::Const(..) | hir::ItemKind::Enum(..) + | hir::ItemKind::Macro(..) | hir::ItemKind::Mod(..) | hir::ItemKind::Static(..) | hir::ItemKind::Struct(..) diff --git a/src/tools/clippy/clippy_lints/src/utils/inspector.rs b/src/tools/clippy/clippy_lints/src/utils/inspector.rs index 6bf216cec16..e97983a2e14 100644 --- a/src/tools/clippy/clippy_lints/src/utils/inspector.rs +++ b/src/tools/clippy/clippy_lints/src/utils/inspector.rs @@ -381,6 +381,13 @@ fn print_item(cx: &LateContext<'_>, item: &hir::Item<'_>) { let item_ty = cx.tcx.type_of(did); println!("function of type {:#?}", item_ty); }, + hir::ItemKind::Macro(ref macro_def) => { + if macro_def.macro_rules { + println!("macro introduced by `macro_rules!`"); + } else { + println!("macro introduced by `macro`"); + } + }, hir::ItemKind::Mod(..) => println!("module"), hir::ItemKind::ForeignMod { abi, .. } => println!("foreign module with abi: {}", abi), hir::ItemKind::GlobalAsm(asm) => println!("global asm: {:?}", asm),