From 0bee80210d1e155b5502e51b693b6c478ed29efd Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Mon, 12 Oct 2020 19:50:12 +0200 Subject: [PATCH 01/12] Rustdoc: Fix macros 2.0 and built-in derives being shown at the wrong path. Fixes #74355 The issue with the built-in derives may be related to: https://github.com/rust-lang/rust/issues/55482#issuecomment-434035721 --- src/librustdoc/visit_ast.rs | 56 ++++++++++++++++++++++--- src/test/rustdoc/macro_pub_in_module.rs | 9 ++++ 2 files changed, 60 insertions(+), 5 deletions(-) create mode 100644 src/test/rustdoc/macro_pub_in_module.rs diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs index 3c0aeaad43e5d..8508af080bebf 100644 --- a/src/librustdoc/visit_ast.rs +++ b/src/librustdoc/visit_ast.rs @@ -61,20 +61,66 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { } crate fn visit(mut self, krate: &'tcx hir::Crate<'_>) -> Module<'tcx> { - let mut module = self.visit_mod_contents( + let mut top_level_module = self.visit_mod_contents( krate.item.span, &Spanned { span: rustc_span::DUMMY_SP, node: hir::VisibilityKind::Public }, hir::CRATE_HIR_ID, &krate.item.module, None, ); - // Attach the crate's exported macros to the top-level module: - module.macros.extend(krate.exported_macros.iter().map(|def| (def, None))); - module.is_crate = true; + top_level_module.is_crate = true; + // 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 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. + krate.exported_macros.iter().for_each(|def| { + macro_rules! try_some {($($body:tt)*) => ({ + fn fn_once R> (f: F) -> F { f } + fn_once(|| Some({ $($body)* }))() + })} + // In the case of dummy items, some of the following operations may fail. We propagate + // that within a `?`-capturing block, so as to fallback to the basic behavior. + let containing_module_of_def = try_some! { + // The `def` of a macro in `exported_macros` should correspond to either: + // - a `#[macro-export] macro_rules!` macro, + // - a built-in `derive` macro such as the ones in `::core`, + // - a `pub macro`. + // Only the last two need to be fixed, thus: + if def.ast.macro_rules { + return None; + } + let macro_parent_module = self.cx.tcx.def_path({ + use rustc_middle::ty::DefIdTree; + self.cx + .tcx + /* Because of #77828 we cannot do the simpler: + .parent_module(def.hir_id).to_def_id() + // and instead have to do: */ + .parent(self.cx.tcx.hir().local_def_id(def.hir_id).to_def_id())? + }); + let mut cur_mod = &mut top_level_module; + for path_segment in macro_parent_module.data { + let path_segment = path_segment.to_string(); + cur_mod = cur_mod.mods.iter_mut().find(|module| { + matches!( + module.name, Some(symbol) + if symbol.with(|mod_name| mod_name == path_segment) + ) + })?; + } + cur_mod + }; + if let Some(module) = containing_module_of_def { + &mut module.macros + } else { + &mut top_level_module.macros + } + .push(self.visit_local_macro(def, None)); + }); self.cx.renderinfo.get_mut().exact_paths = self.exact_paths; - module + top_level_module } fn visit_mod_contents( diff --git a/src/test/rustdoc/macro_pub_in_module.rs b/src/test/rustdoc/macro_pub_in_module.rs new file mode 100644 index 0000000000000..2dfc6b4170668 --- /dev/null +++ b/src/test/rustdoc/macro_pub_in_module.rs @@ -0,0 +1,9 @@ +//! See issue #74355 +#![crate_name = "krate"] +#![feature(decl_macro)] + +// @has krate/some_module/macro.my_macro.html +pub mod some_module { + // + pub macro my_macro() {} +} From 7d03870882aa05fc4c600afa3585251f54d299c4 Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Tue, 13 Oct 2020 20:25:19 +0200 Subject: [PATCH 02/12] Implement suggestions from code review. --- src/librustdoc/visit_ast.rs | 44 +++++++++++++------------ src/test/rustdoc/macro_pub_in_module.rs | 16 ++++++--- 2 files changed, 35 insertions(+), 25 deletions(-) diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs index 8508af080bebf..8b68a2bd65ca8 100644 --- a/src/librustdoc/visit_ast.rs +++ b/src/librustdoc/visit_ast.rs @@ -70,35 +70,36 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { ); top_level_module.is_crate = true; // 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 as well - // (_e.g._, `Copy`), these are wrongly bundled in there too, so we need to fix that by + // 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. krate.exported_macros.iter().for_each(|def| { - macro_rules! try_some {($($body:tt)*) => ({ - fn fn_once R> (f: F) -> F { f } - fn_once(|| Some({ $($body)* }))() - })} - // In the case of dummy items, some of the following operations may fail. We propagate - // that within a `?`-capturing block, so as to fallback to the basic behavior. - let containing_module_of_def = try_some! { + /// A return value of `None` signifies a fallback to the default behavior (locating + /// the macro at the root of the crate). + fn containing_mod_of_macro<'module, 'hir>( + def: &'_ rustc_hir::MacroDef<'_>, + tcx: TyCtxt<'_>, + top_level_module: &'module mut Module<'hir>, + ) -> Option<&'module mut Module<'hir>> { // The `def` of a macro in `exported_macros` should correspond to either: // - a `#[macro-export] macro_rules!` macro, - // - a built-in `derive` macro such as the ones in `::core`, + // - 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 { return None; } - let macro_parent_module = self.cx.tcx.def_path({ + /* Because of #77828 we cannot do the simpler: + let macro_parent_module = tcx.def_path(tcx.parent_module(def.hir_id).to_def_id()); + // and instead have to do: */ + let macro_parent_module = tcx.def_path({ use rustc_middle::ty::DefIdTree; - self.cx - .tcx - /* Because of #77828 we cannot do the simpler: - .parent_module(def.hir_id).to_def_id() - // and instead have to do: */ - .parent(self.cx.tcx.hir().local_def_id(def.hir_id).to_def_id())? + tcx.parent(tcx.hir().local_def_id(def.hir_id).to_def_id())? }); - let mut cur_mod = &mut top_level_module; + // 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. + // WARNING: this will probably break in the presence of re-exports or shadowing. + let mut cur_mod = top_level_module; for path_segment in macro_parent_module.data { let path_segment = path_segment.to_string(); cur_mod = cur_mod.mods.iter_mut().find(|module| { @@ -108,9 +109,10 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { ) })?; } - cur_mod - }; - if let Some(module) = containing_module_of_def { + Some(cur_mod) + } + + if let Some(module) = containing_mod_of_macro(def, self.cx.tcx, &mut top_level_module) { &mut module.macros } else { &mut top_level_module.macros diff --git a/src/test/rustdoc/macro_pub_in_module.rs b/src/test/rustdoc/macro_pub_in_module.rs index 2dfc6b4170668..035d2a8c4418a 100644 --- a/src/test/rustdoc/macro_pub_in_module.rs +++ b/src/test/rustdoc/macro_pub_in_module.rs @@ -1,9 +1,17 @@ //! See issue #74355 +#![feature(decl_macro, no_core, rustc_attrs)] #![crate_name = "krate"] -#![feature(decl_macro)] +#![no_core] -// @has krate/some_module/macro.my_macro.html -pub mod some_module { - // +pub mod inner { + // @has krate/inner/macro.my_macro.html pub macro my_macro() {} + + // @has krate/inner/macro.test.html + #[rustc_builtin_macro] + pub macro test($item:item) {} + + // @has krate/inner/macro.Clone.html + #[rustc_builtin_macro] + pub macro Clone($item:item) {} } From d3a33eb1f9803da21460b675f2452b2784d9f63b Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Wed, 14 Oct 2020 21:12:03 +0200 Subject: [PATCH 03/12] Fix type/value namespace clashes + test for that --- src/librustdoc/visit_ast.rs | 80 +++++++++---------- .../rustdoc/auxiliary/macro_pub_in_module.rs | 13 +++ src/test/rustdoc/macro_pub_in_module.rs | 36 ++++++++- 3 files changed, 85 insertions(+), 44 deletions(-) create mode 100644 src/test/rustdoc/auxiliary/macro_pub_in_module.rs diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs index 8b68a2bd65ca8..ebad35f4e550e 100644 --- a/src/librustdoc/visit_ast.rs +++ b/src/librustdoc/visit_ast.rs @@ -74,50 +74,46 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { // 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. krate.exported_macros.iter().for_each(|def| { - /// A return value of `None` signifies a fallback to the default behavior (locating - /// the macro at the root of the crate). - fn containing_mod_of_macro<'module, 'hir>( - def: &'_ rustc_hir::MacroDef<'_>, - tcx: TyCtxt<'_>, - top_level_module: &'module mut Module<'hir>, - ) -> Option<&'module mut Module<'hir>> { - // 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 { - return None; - } - /* Because of #77828 we cannot do the simpler: - let macro_parent_module = tcx.def_path(tcx.parent_module(def.hir_id).to_def_id()); - // and instead have to do: */ - let macro_parent_module = tcx.def_path({ - use rustc_middle::ty::DefIdTree; - tcx.parent(tcx.hir().local_def_id(def.hir_id).to_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. - // WARNING: this will probably break in the presence of re-exports or shadowing. - let mut cur_mod = top_level_module; - for path_segment in macro_parent_module.data { - let path_segment = path_segment.to_string(); - cur_mod = cur_mod.mods.iter_mut().find(|module| { - matches!( - module.name, Some(symbol) - if symbol.with(|mod_name| mod_name == path_segment) - ) - })?; - } - Some(cur_mod) + let visit_macro = || self.visit_local_macro(def, None); + // 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(visit_macro()); + return; } - - if let Some(module) = containing_mod_of_macro(def, self.cx.tcx, &mut top_level_module) { - &mut module.macros - } else { - &mut top_level_module.macros + let tcx = self.cx.tcx; + /* Because of #77828 we cannot do the simpler: + let macro_parent_module = tcx.def_path(tcx.parent_module(def.hir_id).to_def_id()); + // and instead have to do: */ + let macro_parent_module = tcx.def_path({ + use rustc_middle::ty::DefIdTree; + tcx.parent(tcx.hir().local_def_id(def.hir_id).to_def_id()).unwrap() + }); + // 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_module.data { + let path_segment_ty_ns = match path_segment.data { + rustc_hir::definitions::DefPathData::TypeNs(symbol) => symbol, + _ => { + // If the path segment is not from the type namespace + // (_e.g._, it can be from a value namespace in the case of `f::` in: + // `fn f() { pub macro m() {} }` + // then the item is not accessible, and should thus act as if it didn't + // exist (unless "associated macros" (inside an `impl`) were a thing…). + return; + } + }; + cur_mod = cur_mod + .mods + .iter_mut() + .find(|module| module.name == Some(path_segment_ty_ns)) + .unwrap(); } - .push(self.visit_local_macro(def, None)); + cur_mod.macros.push(visit_macro()); }); self.cx.renderinfo.get_mut().exact_paths = self.exact_paths; diff --git a/src/test/rustdoc/auxiliary/macro_pub_in_module.rs b/src/test/rustdoc/auxiliary/macro_pub_in_module.rs new file mode 100644 index 0000000000000..fa987689ec650 --- /dev/null +++ b/src/test/rustdoc/auxiliary/macro_pub_in_module.rs @@ -0,0 +1,13 @@ +// edition:2018 + +#![feature(decl_macro)] +#![crate_name = "external_crate"] + +pub mod some_module { + /* == Make sure the logic is not affected by a re-export == */ + mod private { + pub macro external_macro() {} + } + // @has external_crate/some_module/macro.external_macro.html + pub use private::external_macro; +} diff --git a/src/test/rustdoc/macro_pub_in_module.rs b/src/test/rustdoc/macro_pub_in_module.rs index 035d2a8c4418a..7d92246279fb6 100644 --- a/src/test/rustdoc/macro_pub_in_module.rs +++ b/src/test/rustdoc/macro_pub_in_module.rs @@ -1,11 +1,18 @@ +// aux-build:macro_pub_in_module.rs +// edition:2018 +// build-aux-docs +// @has external_crate/some_module/macro.external_macro.html + //! See issue #74355 #![feature(decl_macro, no_core, rustc_attrs)] #![crate_name = "krate"] #![no_core] +extern crate external_crate; + pub mod inner { - // @has krate/inner/macro.my_macro.html - pub macro my_macro() {} + // @has krate/inner/macro.raw_const.html + pub macro raw_const() {} // @has krate/inner/macro.test.html #[rustc_builtin_macro] @@ -14,4 +21,29 @@ pub mod inner { // @has krate/inner/macro.Clone.html #[rustc_builtin_macro] pub macro Clone($item:item) {} + + // Make sure the logic is not affected by a re-export. + mod private { + pub macro m() {} + } + // @has krate/inner/macro.renamed.html + pub use private::m as renamed; + + // @has krate/inner/macro.external_macro.html + pub use ::external_crate::some_module::external_macro; +} + +// Namespaces: Make sure the logic does not mix up a function name with a module name… +fn both_fn_and_mod() { + pub macro m() {} +} +pub mod both_fn_and_mod { + // @!has krate/both_fn_and_mod/macro.m.html +} + +const __: () = { + pub macro m() {} +}; +pub mod __ { + // @!has krate/__/macro.m.html } From aa863caebe74710d119662894ecd946fa8bcf725 Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Sun, 18 Oct 2020 02:26:31 +0200 Subject: [PATCH 04/12] Style nit: replace `for_each` & `return` with `for` & `continue` Co-Authored-By: Joshua Nelson --- library/std/src/prelude/v1.rs | 8 ++++---- src/librustdoc/clean/inline.rs | 12 +++++++++++- src/librustdoc/visit_ast.rs | 13 ++++++------- 3 files changed, 21 insertions(+), 12 deletions(-) diff --git a/library/std/src/prelude/v1.rs b/library/std/src/prelude/v1.rs index 0fbd6b62f18ff..26302d0ecf2b1 100644 --- a/library/std/src/prelude/v1.rs +++ b/library/std/src/prelude/v1.rs @@ -41,17 +41,17 @@ pub use crate::result::Result::{self, Err, Ok}; pub use core::prelude::v1::{ asm, assert, cfg, column, compile_error, concat, concat_idents, env, file, format_args, format_args_nl, global_asm, include, include_bytes, include_str, line, llvm_asm, log_syntax, - module_path, option_env, stringify, trace_macros, + module_path, option_env, stringify, trace_macros, Clone, Copy, Debug, Default, Eq, Hash, Ord, + PartialEq, PartialOrd, }; -// FIXME: Attribute and derive macros are not documented because for them rustdoc generates +// FIXME: Attribute and internal derive macros are not documented because for them rustdoc generates // dead links which fail link checker testing. #[stable(feature = "builtin_macro_prelude", since = "1.38.0")] #[allow(deprecated)] #[doc(hidden)] pub use core::prelude::v1::{ - bench, global_allocator, test, test_case, Clone, Copy, Debug, Default, Eq, Hash, Ord, - PartialEq, PartialOrd, RustcDecodable, RustcEncodable, + bench, global_allocator, test, test_case, RustcDecodable, RustcEncodable, }; #[unstable( diff --git a/src/librustdoc/clean/inline.rs b/src/librustdoc/clean/inline.rs index ed972cc16e954..beb2c09feccea 100644 --- a/src/librustdoc/clean/inline.rs +++ b/src/librustdoc/clean/inline.rs @@ -169,7 +169,17 @@ crate fn record_extern_fqn(cx: &DocContext<'_>, did: DefId, kind: clean::TypeKin if !s.is_empty() { Some(s) } else { None } }); let fqn = if let clean::TypeKind::Macro = kind { - vec![crate_name, relative.last().expect("relative was empty")] + // Check to see if it is a macro 2.0 or built-in macro + if matches!( + cx.enter_resolver(|r| r.cstore().load_macro_untracked(did, cx.sess())), + LoadedMacro::MacroDef(def, _) + if matches!(&def.kind, ast::ItemKind::MacroDef(def) + if !def.macro_rules) + ) { + once(crate_name).chain(relative).collect() + } else { + vec![crate_name, relative.last().expect("relative was empty")] + } } else { once(crate_name).chain(relative).collect() }; diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs index ebad35f4e550e..1e78a01404815 100644 --- a/src/librustdoc/visit_ast.rs +++ b/src/librustdoc/visit_ast.rs @@ -73,16 +73,15 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { // 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. - krate.exported_macros.iter().for_each(|def| { - let visit_macro = || self.visit_local_macro(def, None); + '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(visit_macro()); - return; + top_level_module.macros.push((def, None)); + continue 'exported_macros; } let tcx = self.cx.tcx; /* Because of #77828 we cannot do the simpler: @@ -104,7 +103,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { // `fn f() { pub macro m() {} }` // then the item is not accessible, and should thus act as if it didn't // exist (unless "associated macros" (inside an `impl`) were a thing…). - return; + continue 'exported_macros; } }; cur_mod = cur_mod @@ -113,8 +112,8 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { .find(|module| module.name == Some(path_segment_ty_ns)) .unwrap(); } - cur_mod.macros.push(visit_macro()); - }); + cur_mod.macros.push((def, None)); + } self.cx.renderinfo.get_mut().exact_paths = self.exact_paths; From 28e9c6723a9111b649384ca909eded20c18c76a3 Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Sun, 27 Dec 2020 19:05:35 +0100 Subject: [PATCH 05/12] Update test assertions (showcases bug) --- .../rustdoc/auxiliary/macro_pub_in_module.rs | 2 +- src/test/rustdoc/macro_pub_in_module.rs | 36 +++++++++++++++---- 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/src/test/rustdoc/auxiliary/macro_pub_in_module.rs b/src/test/rustdoc/auxiliary/macro_pub_in_module.rs index fa987689ec650..137b12386007c 100644 --- a/src/test/rustdoc/auxiliary/macro_pub_in_module.rs +++ b/src/test/rustdoc/auxiliary/macro_pub_in_module.rs @@ -8,6 +8,6 @@ pub mod some_module { mod private { pub macro external_macro() {} } - // @has external_crate/some_module/macro.external_macro.html + pub use private::external_macro; } diff --git a/src/test/rustdoc/macro_pub_in_module.rs b/src/test/rustdoc/macro_pub_in_module.rs index 7d92246279fb6..d488f51cd494f 100644 --- a/src/test/rustdoc/macro_pub_in_module.rs +++ b/src/test/rustdoc/macro_pub_in_module.rs @@ -1,49 +1,73 @@ // aux-build:macro_pub_in_module.rs // edition:2018 // build-aux-docs -// @has external_crate/some_module/macro.external_macro.html //! See issue #74355 #![feature(decl_macro, no_core, rustc_attrs)] #![crate_name = "krate"] #![no_core] +// @has external_crate/some_module/macro.external_macro.html extern crate external_crate; pub mod inner { // @has krate/inner/macro.raw_const.html + // @!has krate/macro.raw_const.html pub macro raw_const() {} // @has krate/inner/macro.test.html + // @!has krate/macro.test.html #[rustc_builtin_macro] pub macro test($item:item) {} // @has krate/inner/macro.Clone.html + // @!has krate/macro.Clone.html #[rustc_builtin_macro] pub macro Clone($item:item) {} - // Make sure the logic is not affected by a re-export. + // Make sure the logic is not affected by re-exports. + mod unrenamed { + // @!has krate/macro.unrenamed.html + #[rustc_macro_transparency = "semitransparent"] + pub macro unrenamed() {} + } + // @has krate/inner/macro.unrenamed.html + pub use unrenamed::unrenamed; + mod private { + // @!has krate/macro.m.html pub macro m() {} } // @has krate/inner/macro.renamed.html + // @!has krate/macro.renamed.html pub use private::m as renamed; + mod private2 { + // @!has krate/macro.m2.html + pub macro m2() {} + } + use private2 as renamed_mod; + // @has krate/inner/macro.m2.html + pub use renamed_mod::m2; + // @has krate/inner/macro.external_macro.html + // @!has krate/macro.external_macro.html pub use ::external_crate::some_module::external_macro; } // Namespaces: Make sure the logic does not mix up a function name with a module name… fn both_fn_and_mod() { - pub macro m() {} + // @!has krate/macro.in_both_fn_and_mod.html + pub macro in_both_fn_and_mod() {} } pub mod both_fn_and_mod { - // @!has krate/both_fn_and_mod/macro.m.html + // @!has krate/both_fn_and_mod/macro.in_both_fn_and_mod.html } const __: () = { - pub macro m() {} + // @!has krate/macro.in_both_const_and_mod.html + pub macro in_both_const_and_mod() {} }; pub mod __ { - // @!has krate/__/macro.m.html + // @!has krate/__/macro.in_both_const_and_mod.html } From dd15f410df09233389d7ac084b2634044d50e58b Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Sun, 27 Dec 2020 21:15:01 +0100 Subject: [PATCH 06/12] WIP: attempt to fix the undocument re-export issue --- compiler/rustc_privacy/src/lib.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_privacy/src/lib.rs b/compiler/rustc_privacy/src/lib.rs index 1bcfdf0faf66a..6332130f80e8c 100644 --- a/compiler/rustc_privacy/src/lib.rs +++ b/compiler/rustc_privacy/src/lib.rs @@ -832,8 +832,19 @@ impl Visitor<'tcx> for EmbargoVisitor<'tcx> { } fn visit_macro_def(&mut self, md: &'tcx hir::MacroDef<'tcx>) { - if attr::find_transparency(&self.tcx.sess, &md.attrs, md.ast.macro_rules).0 - != Transparency::Opaque + // HACK (or fix?): a + // ```rust,ignore (dummy example) + // mod private { + // #[rustc_macro_transparency(semitransparent)] + // pub macro m { … } + // } + // ``` + // is *not* `Public`ly reachable and yet this shortcut would express + // that. + // FIXME! + if md.ast.macro_rules + && attr::find_transparency(&self.tcx.sess, &md.attrs, md.ast.macro_rules).0 + != Transparency::Opaque { self.update(md.hir_id, Some(AccessLevel::Public)); return; From 8df60663cc5331f1c911766975619932b8e18bb0 Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Sun, 27 Dec 2020 22:14:43 +0100 Subject: [PATCH 07/12] Enhance tests based on code review Co-authored-by: Joshua Nelson --- src/test/rustdoc/macro_pub_in_module.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/rustdoc/macro_pub_in_module.rs b/src/test/rustdoc/macro_pub_in_module.rs index d488f51cd494f..59e69625455de 100644 --- a/src/test/rustdoc/macro_pub_in_module.rs +++ b/src/test/rustdoc/macro_pub_in_module.rs @@ -7,7 +7,8 @@ #![crate_name = "krate"] #![no_core] -// @has external_crate/some_module/macro.external_macro.html + // @has external_crate/some_module/macro.external_macro.html + // @!has external_crate/macro.external_macro.html extern crate external_crate; pub mod inner { From ce3ecd612f2ac52770293201848de733c86dcaaa Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Tue, 5 Jan 2021 14:34:32 +0100 Subject: [PATCH 08/12] Style nit: avoid confusing name shadowing in pattern match Co-authored-by: Joshua Nelson --- src/librustdoc/clean/inline.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustdoc/clean/inline.rs b/src/librustdoc/clean/inline.rs index beb2c09feccea..ee45f74666a54 100644 --- a/src/librustdoc/clean/inline.rs +++ b/src/librustdoc/clean/inline.rs @@ -173,8 +173,8 @@ crate fn record_extern_fqn(cx: &DocContext<'_>, did: DefId, kind: clean::TypeKin if matches!( cx.enter_resolver(|r| r.cstore().load_macro_untracked(did, cx.sess())), LoadedMacro::MacroDef(def, _) - if matches!(&def.kind, ast::ItemKind::MacroDef(def) - if !def.macro_rules) + if matches!(&def.kind, ast::ItemKind::MacroDef(ast_def) + if !ast_def.macro_rules) ) { once(crate_name).chain(relative).collect() } else { From 255f107cacb8927e72798313d69fa83d2e752a20 Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Tue, 5 Jan 2021 15:07:40 +0100 Subject: [PATCH 09/12] Fix ICE on `pub macro`s defined within a non-module type namespace. --- src/librustdoc/visit_ast.rs | 18 +++++++++++++----- src/test/rustdoc/macro_pub_in_module.rs | 8 ++++++++ 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs index 1e78a01404815..2dc9c7bc758cf 100644 --- a/src/librustdoc/visit_ast.rs +++ b/src/librustdoc/visit_ast.rs @@ -93,6 +93,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { }); // 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. + // Once #80415 is merged, this whole `for` loop research can be replaced by that. let mut cur_mod = &mut top_level_module; for path_segment in macro_parent_module.data { let path_segment_ty_ns = match path_segment.data { @@ -106,11 +107,18 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { continue 'exported_macros; } }; - cur_mod = cur_mod - .mods - .iter_mut() - .find(|module| module.name == Some(path_segment_ty_ns)) - .unwrap(); + // The obtained name in the type namespace may belong to something that is not + // a `mod`ule (_e.g._, it could be an `enum` with a `pub macro` defined within + // the block used for a discriminant. + if let Some(child_mod) = + cur_mod.mods.iter_mut().find(|module| module.name == Some(path_segment_ty_ns)) + { + cur_mod = child_mod; + } else { + // If the macro's parent def path is not exclusively made of module + // components, then it is not accessible (c.f. previous `continue`). + continue 'exported_macros; + } } cur_mod.macros.push((def, None)); } diff --git a/src/test/rustdoc/macro_pub_in_module.rs b/src/test/rustdoc/macro_pub_in_module.rs index 59e69625455de..4fd85d6899401 100644 --- a/src/test/rustdoc/macro_pub_in_module.rs +++ b/src/test/rustdoc/macro_pub_in_module.rs @@ -72,3 +72,11 @@ const __: () = { pub mod __ { // @!has krate/__/macro.in_both_const_and_mod.html } + +enum Enum { + Crazy = { + // @!has krate/macro.this_is_getting_weird.html; + pub macro this_is_getting_weird() {} + 42 + }, +} From a4de27aeec7a8c876cce56561598898313a5a6bf Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Tue, 5 Jan 2021 15:16:56 +0100 Subject: [PATCH 10/12] Fixed non-declarative-nor-opaque macros effective privacy. cc @petrochenkov --- compiler/rustc_privacy/src/lib.rs | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_privacy/src/lib.rs b/compiler/rustc_privacy/src/lib.rs index 6332130f80e8c..c2db2c82fa1c4 100644 --- a/compiler/rustc_privacy/src/lib.rs +++ b/compiler/rustc_privacy/src/lib.rs @@ -832,21 +832,15 @@ impl Visitor<'tcx> for EmbargoVisitor<'tcx> { } fn visit_macro_def(&mut self, md: &'tcx hir::MacroDef<'tcx>) { - // HACK (or fix?): a - // ```rust,ignore (dummy example) - // mod private { - // #[rustc_macro_transparency(semitransparent)] - // pub macro m { … } - // } - // ``` - // is *not* `Public`ly reachable and yet this shortcut would express - // that. - // FIXME! - if md.ast.macro_rules - && attr::find_transparency(&self.tcx.sess, &md.attrs, md.ast.macro_rules).0 - != Transparency::Opaque + // Non-opaque macros cannot make other items more accessible than they already are. + if attr::find_transparency(&self.tcx.sess, &md.attrs, md.ast.macro_rules).0 + != Transparency::Opaque { - self.update(md.hir_id, Some(AccessLevel::Public)); + // `#[macro_export]`-ed `macro_rules!` are `Public` since they + // ignore their containing path to always appear at the crate root. + if md.ast.macro_rules { + self.update(md.hir_id, Some(AccessLevel::Public)); + } return; } From 0311fe98e026579cd3581b7655e9ff84d4376682 Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Wed, 6 Jan 2021 16:46:47 +0100 Subject: [PATCH 11/12] Fix rebase and clean up some code. --- src/librustdoc/visit_ast.rs | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-) diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs index 2dc9c7bc758cf..c559949f01e9d 100644 --- a/src/librustdoc/visit_ast.rs +++ b/src/librustdoc/visit_ast.rs @@ -93,38 +93,24 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { }); // 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. - // Once #80415 is merged, this whole `for` loop research can be replaced by that. let mut cur_mod = &mut top_level_module; for path_segment in macro_parent_module.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, - _ => { - // If the path segment is not from the type namespace - // (_e.g._, it can be from a value namespace in the case of `f::` in: - // `fn f() { pub macro m() {} }` - // then the item is not accessible, and should thus act as if it didn't - // exist (unless "associated macros" (inside an `impl`) were a thing…). - continue 'exported_macros; - } + _ => continue 'exported_macros, }; - // The obtained name in the type namespace may belong to something that is not - // a `mod`ule (_e.g._, it could be an `enum` with a `pub macro` defined within - // the block used for a discriminant. - if let Some(child_mod) = - cur_mod.mods.iter_mut().find(|module| module.name == Some(path_segment_ty_ns)) - { - cur_mod = child_mod; - } else { - // If the macro's parent def path is not exclusively made of module - // components, then it is not accessible (c.f. previous `continue`). - 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 == Some(path_segment_ty_ns)) { + Some(child_mod) => cur_mod = &mut *child_mod, + None => continue 'exported_macros, } } cur_mod.macros.push((def, None)); } - self.cx.renderinfo.get_mut().exact_paths = self.exact_paths; - top_level_module } From bceb1737ed2ba6dbf8e294519011f0e4e264b7a3 Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Thu, 7 Jan 2021 19:27:03 +0100 Subject: [PATCH 12/12] Apply suggestions from code review & other minor nits Co-authored-by: Joshua Nelson --- src/librustdoc/visit_ast.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs index c559949f01e9d..1fedd26a1ef2f 100644 --- a/src/librustdoc/visit_ast.rs +++ b/src/librustdoc/visit_ast.rs @@ -75,7 +75,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { // 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 `#[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: @@ -84,17 +84,18 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { continue 'exported_macros; } let tcx = self.cx.tcx; - /* Because of #77828 we cannot do the simpler: - let macro_parent_module = tcx.def_path(tcx.parent_module(def.hir_id).to_def_id()); - // and instead have to do: */ - let macro_parent_module = tcx.def_path({ + // 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(tcx.hir().local_def_id(def.hir_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_module.data { + 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`. @@ -108,6 +109,8 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { None => continue 'exported_macros, } } + 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.renderinfo.get_mut().exact_paths = self.exact_paths;