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: Fix macros 2.0 and built-in derives being shown at the wrong path #77862

Merged
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
7 changes: 6 additions & 1 deletion compiler/rustc_privacy/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -832,10 +832,15 @@ impl Visitor<'tcx> for EmbargoVisitor<'tcx> {
}

fn visit_macro_def(&mut self, md: &'tcx hir::MacroDef<'tcx>) {
// 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));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In theory #[rustc_macro_transparency = "transparent"] pub macro foo { ... } is also possible and needs update called, but that's not a case used by the standard library, and all of this is a hack anyway, so it should be ok for now.

return;
}

Expand Down
8 changes: 4 additions & 4 deletions library/std/src/prelude/v1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
12 changes: 11 additions & 1 deletion src/librustdoc/clean/inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(ast_def)
if !ast_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()
};
Expand Down
54 changes: 47 additions & 7 deletions src/librustdoc/visit_ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,20 +61,60 @@ 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 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(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_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 == Some(path_segment_ty_ns)) {
Some(child_mod) => cur_mod = &mut *child_mod,
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);
Copy link
Member

Choose a reason for hiding this comment

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

Now that you added the comment, I think this isn't true in general and there will be times the assertion fails, because cur_mod_def_id is always a module and macro_parent may not be. I think looking for the nearest module is the correct behavior.

Copy link
Contributor Author

@danielhenrymantilla danielhenrymantilla Jan 7, 2021

Choose a reason for hiding this comment

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

Technically that should be caught by the loop logic: if one of the ancestors is not a module, we will be hitting one of the two continue 'exported macros; bail points, and thus never reach that part of the code. That being said, I am open to suggestions to make these things clearer (e.g., we could .try_fold() and Err rather than for + continue 'outer 🤷)

Copy link
Member

Choose a reason for hiding this comment

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

Ok, but wouldn't it be simpler to only look up the parent module in the first place? Then you wouldn't have to skip the segments you don't care about, because you care about all of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could add a "fail faster" branch where we'd compare the immediate parent and the parent module, and bail if they're not equal; but even then we'd still need to check that that module in and of itself is reachable / nameable, so most of the logic would still be here. I think the micro optimization is not worth the extra branch; but I keep thinking that a try_fold() for the inner loop may be interesting, since it avoids the need to continue the 'outer loop:

Example (assuming postfix match for the ergonomics)
for def in krate.exported_macros {
    let put_macro_in = |module| module.macros.push((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 {
        put_macro_in(&mut top_level_module);
        continue;
    }
    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(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.
    macro_parent_path.data.try_fold(&mut top_level_module, |(cur_mod, path_segment)| {
        // 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.
        let path_segment_ty_ns = match path_segment.data {
            rustc_hir::definitions::DefPathData::TypeNs(symbol) => symbol,
            _ => return Err(()),
        };
        // Descend into the child module that matches this path segment, if any.
        cur_mod.mods.iter_mut().find(|child| child.name == Some(path_segment_ty_ns)).ok_or(())
    }).match {
        Ok(module) => {
            let module_def_id =
                tcx.hir().local_def_id(module.id).to_def_id()
            ;
            assert_eq!(module_def_id, macro_parent_def_id);
            put_macro_in(&mut module);
        },
        Err(()) => continue,
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, ok. I'm fine with either the current code or the try_fold.

cur_mod.macros.push((def, None));
danielhenrymantilla marked this conversation as resolved.
Show resolved Hide resolved
}
self.cx.renderinfo.get_mut().exact_paths = self.exact_paths;

module
top_level_module
}

fn visit_mod_contents(
Expand Down
13 changes: 13 additions & 0 deletions src/test/rustdoc/auxiliary/macro_pub_in_module.rs
Original file line number Diff line number Diff line change
@@ -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() {}
}

pub use private::external_macro;
}
82 changes: 82 additions & 0 deletions src/test/rustdoc/macro_pub_in_module.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
// aux-build:macro_pub_in_module.rs
// edition:2018
// build-aux-docs

//! See issue #74355
#![feature(decl_macro, no_core, rustc_attrs)]
#![crate_name = "krate"]
#![no_core]

// @has external_crate/some_module/macro.external_macro.html
// @!has external_crate/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 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;
danielhenrymantilla marked this conversation as resolved.
Show resolved Hide resolved

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() {
// @!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.in_both_fn_and_mod.html
}

const __: () = {
// @!has krate/macro.in_both_const_and_mod.html
pub macro in_both_const_and_mod() {}
};
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
},
}