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: Resolve doc links referring to macro_rules items #96521

Merged
merged 4 commits into from
May 1, 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
6 changes: 4 additions & 2 deletions compiler/rustc_resolve/src/build_reduced_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1267,13 +1267,15 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
self.insert_unused_macro(ident, def_id, item.id);
}
self.r.visibilities.insert(def_id, vis);
self.r.arenas.alloc_macro_rules_scope(MacroRulesScope::Binding(
let scope = self.r.arenas.alloc_macro_rules_scope(MacroRulesScope::Binding(
self.r.arenas.alloc_macro_rules_binding(MacroRulesBinding {
parent_macro_rules_scope: parent_scope.macro_rules,
binding,
ident,
}),
))
));
self.r.macro_rules_scopes.insert(def_id, scope);
scope
} else {
let module = parent_scope.module;
let vis = match item.kind {
Expand Down
27 changes: 15 additions & 12 deletions compiler/rustc_resolve/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,9 @@ enum ScopeSet<'a> {
/// but not for late resolution yet.
#[derive(Clone, Copy, Debug)]
pub struct ParentScope<'a> {
module: Module<'a>,
pub module: Module<'a>,
expansion: LocalExpnId,
macro_rules: MacroRulesScopeRef<'a>,
pub macro_rules: MacroRulesScopeRef<'a>,
derives: &'a [ast::Path],
}

Expand Down Expand Up @@ -990,6 +990,8 @@ pub struct Resolver<'a> {
/// `macro_rules` scopes *produced* by expanding the macro invocations,
/// include all the `macro_rules` items and other invocations generated by them.
output_macro_rules_scopes: FxHashMap<LocalExpnId, MacroRulesScopeRef<'a>>,
/// `macro_rules` scopes produced by `macro_rules` item definitions.
macro_rules_scopes: FxHashMap<LocalDefId, MacroRulesScopeRef<'a>>,
/// Helper attributes that are in scope for the given expansion.
helper_attrs: FxHashMap<LocalExpnId, Vec<Ident>>,
/// Ready or in-progress results of resolving paths inside the `#[derive(...)]` attribute
Expand Down Expand Up @@ -1361,6 +1363,7 @@ impl<'a> Resolver<'a> {
non_macro_attr: Lrc::new(SyntaxExtension::non_macro_attr(session.edition())),
invocation_parent_scopes: Default::default(),
output_macro_rules_scopes: Default::default(),
macro_rules_scopes: Default::default(),
helper_attrs: Default::default(),
derive_data: Default::default(),
local_macro_def_scopes: FxHashMap::default(),
Expand Down Expand Up @@ -1874,25 +1877,25 @@ impl<'a> Resolver<'a> {
&mut self,
path_str: &str,
ns: Namespace,
mut module_id: DefId,
mut parent_scope: ParentScope<'a>,
) -> Option<Res> {
let mut segments =
Vec::from_iter(path_str.split("::").map(Ident::from_str).map(Segment::from_ident));
if let Some(segment) = segments.first_mut() {
if segment.ident.name == kw::Crate {
// FIXME: `resolve_path` always resolves `crate` to the current crate root, but
// rustdoc wants it to resolve to the `module_id`'s crate root. This trick of
// rustdoc wants it to resolve to the `parent_scope`'s crate root. This trick of
// replacing `crate` with `self` and changing the current module should achieve
// the same effect.
segment.ident.name = kw::SelfLower;
module_id = module_id.krate.as_def_id();
parent_scope.module =
self.expect_module(parent_scope.module.def_id().krate.as_def_id());
} else if segment.ident.name == kw::Empty {
segment.ident.name = kw::PathRoot;
}
}

let module = self.expect_module(module_id);
match self.maybe_resolve_path(&segments, Some(ns), &ParentScope::module(module, self)) {
match self.maybe_resolve_path(&segments, Some(ns), &parent_scope) {
PathResult::Module(ModuleOrUniformRoot::Module(module)) => Some(module.res().unwrap()),
PathResult::NonModule(path_res) if path_res.unresolved_segments() == 0 => {
Some(path_res.base_res())
Expand All @@ -1904,11 +1907,6 @@ impl<'a> Resolver<'a> {
}
}

// For rustdoc.
pub fn graph_root(&self) -> Module<'a> {
self.graph_root
}

// For rustdoc.
pub fn take_all_macro_rules(&mut self) -> FxHashMap<Symbol, Res> {
mem::take(&mut self.all_macro_rules)
Expand All @@ -1924,6 +1922,11 @@ impl<'a> Resolver<'a> {
}
}

/// For rustdoc.
pub fn macro_rules_scope(&self, def_id: LocalDefId) -> MacroRulesScopeRef<'a> {
*self.macro_rules_scopes.get(&def_id).expect("not a `macro_rules` item")
}

/// Retrieves the span of the given `DefId` if `DefId` is in the local crate.
#[inline]
pub fn opt_span(&self, def_id: DefId) -> Option<Span> {
Expand Down
2 changes: 2 additions & 0 deletions library/core/src/macros/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -640,6 +640,8 @@ macro_rules! unreachable {
///
/// Like `panic!`, this macro has a second form for displaying custom values.
///
/// [`todo!`]: crate::todo
Copy link
Contributor

Choose a reason for hiding this comment

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

Changes like this imply the need for a Crater run and rel-notes tag. Right?

///
/// # Examples
///
/// Say we have a trait `Foo`:
Expand Down
4 changes: 4 additions & 0 deletions library/std/src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ macro_rules! panic {
/// [`eprint!`] instead to print error and progress messages.
///
/// [flush]: crate::io::Write::flush
/// [`println!`]: crate::println
/// [`eprint!`]: crate::eprint
///
/// # Panics
///
Expand Down Expand Up @@ -77,6 +79,7 @@ macro_rules! print {
/// [`eprintln!`] instead to print error and progress messages.
///
/// [`std::fmt`]: crate::fmt
/// [`eprintln!`]: crate::eprintln
///
/// # Panics
///
Expand Down Expand Up @@ -146,6 +149,7 @@ macro_rules! eprint {
///
/// [`io::stderr`]: crate::io::stderr
/// [`io::stdout`]: crate::io::stdout
/// [`println!`]: crate::println
///
/// # Panics
///
Expand Down
2 changes: 2 additions & 0 deletions library/std/src/thread/local.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ use crate::fmt;
/// The [`with`] method yields a reference to the contained value which cannot be
/// sent across threads or escape the given closure.
///
/// [`thread_local!`]: crate::thread_local
///
/// # Initialization and Destruction
///
/// Initialization is dynamically performed on the first call to [`with`]
Expand Down
1 change: 1 addition & 0 deletions library/std/src/thread/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@
//! [`Cell`]: crate::cell::Cell
//! [`RefCell`]: crate::cell::RefCell
//! [`with`]: LocalKey::with
//! [`thread_local!`]: crate::thread_local

#![stable(feature = "rust1", since = "1.0.0")]
#![deny(unsafe_op_in_unsafe_fn)]
Expand Down
5 changes: 4 additions & 1 deletion src/librustdoc/passes/collect_intra_doc_links.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use rustc_hir::def_id::{DefId, CRATE_DEF_ID};
use rustc_hir::Mutability;
use rustc_middle::ty::{DefIdTree, Ty, TyCtxt};
use rustc_middle::{bug, span_bug, ty};
use rustc_resolve::ParentScope;
use rustc_session::lint::Lint;
use rustc_span::hygiene::MacroKind;
use rustc_span::symbol::{sym, Ident, Symbol};
Expand Down Expand Up @@ -564,7 +565,9 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
.copied()
.unwrap_or_else(|| {
self.cx.enter_resolver(|resolver| {
resolver.resolve_rustdoc_path(path_str, ns, module_id)
let parent_scope =
ParentScope::module(resolver.expect_module(module_id), resolver);
resolver.resolve_rustdoc_path(path_str, ns, parent_scope)
})
})
.and_then(|res| res.try_into().ok())
Expand Down
105 changes: 79 additions & 26 deletions src/librustdoc/passes/collect_intra_doc_links/early.rs
Original file line number Diff line number Diff line change
@@ -1,20 +1,21 @@
use crate::clean::Attributes;
use crate::core::ResolverCaches;
use crate::passes::collect_intra_doc_links::preprocessed_markdown_links;
use crate::passes::collect_intra_doc_links::PreprocessedMarkdownLink;
use crate::passes::collect_intra_doc_links::{Disambiguator, PreprocessedMarkdownLink};

use rustc_ast::visit::{self, AssocCtxt, Visitor};
use rustc_ast::{self as ast, ItemKind};
use rustc_ast_lowering::ResolverAstLowering;
use rustc_data_structures::fx::FxHashMap;
use rustc_hir::def::Namespace::*;
use rustc_hir::def::{DefKind, Namespace, Res};
use rustc_hir::def_id::{DefId, DefIdMap, DefIdSet, LocalDefId, CRATE_DEF_ID};
use rustc_hir::def_id::{DefId, DefIdMap, DefIdSet, CRATE_DEF_ID};
use rustc_hir::TraitCandidate;
use rustc_middle::ty::{DefIdTree, Visibility};
use rustc_resolve::{ParentScope, Resolver};
use rustc_session::config::Externs;
use rustc_session::Session;
use rustc_span::symbol::sym;
use rustc_span::{Symbol, SyntaxContext};

use std::collections::hash_map::Entry;
Expand All @@ -27,10 +28,12 @@ crate fn early_resolve_intra_doc_links(
externs: Externs,
document_private_items: bool,
) -> ResolverCaches {
let parent_scope =
ParentScope::module(resolver.expect_module(CRATE_DEF_ID.to_def_id()), resolver);
let mut link_resolver = EarlyDocLinkResolver {
resolver,
sess,
current_mod: CRATE_DEF_ID,
parent_scope,
visited_mods: Default::default(),
markdown_links: Default::default(),
doc_link_resolutions: Default::default(),
Expand All @@ -52,7 +55,7 @@ crate fn early_resolve_intra_doc_links(
// DO NOT REMOVE THIS without first testing on the reproducer in
// https://github.com/jyn514/objr/commit/edcee7b8124abf0e4c63873e8422ff81beb11ebb
for (extern_name, _) in externs.iter().filter(|(_, entry)| entry.add_prelude) {
link_resolver.resolver.resolve_rustdoc_path(extern_name, TypeNS, CRATE_DEF_ID.to_def_id());
link_resolver.resolver.resolve_rustdoc_path(extern_name, TypeNS, parent_scope);
}

ResolverCaches {
Expand All @@ -72,7 +75,7 @@ fn doc_attrs<'a>(attrs: impl Iterator<Item = &'a ast::Attribute>) -> Attributes
struct EarlyDocLinkResolver<'r, 'ra> {
resolver: &'r mut Resolver<'ra>,
sess: &'r Session,
current_mod: LocalDefId,
parent_scope: ParentScope<'ra>,
visited_mods: DefIdSet,
markdown_links: FxHashMap<String, Vec<PreprocessedMarkdownLink>>,
doc_link_resolutions: FxHashMap<(Symbol, Namespace, DefId), Option<Res<ast::NodeId>>>,
Expand All @@ -82,7 +85,7 @@ struct EarlyDocLinkResolver<'r, 'ra> {
document_private_items: bool,
}

impl EarlyDocLinkResolver<'_, '_> {
impl<'ra> EarlyDocLinkResolver<'_, 'ra> {
fn add_traits_in_scope(&mut self, def_id: DefId) {
// Calls to `traits_in_scope` are expensive, so try to avoid them if only possible.
// Keys in the `traits_in_scope` cache are always module IDs.
Expand Down Expand Up @@ -205,34 +208,64 @@ impl EarlyDocLinkResolver<'_, '_> {
if !attrs.iter().any(|attr| attr.may_have_doc_links()) {
return;
}
let module_id = self.current_mod.to_def_id();
self.resolve_doc_links(doc_attrs(attrs.iter()), module_id);
self.resolve_doc_links(doc_attrs(attrs.iter()), self.parent_scope);
}

fn resolve_doc_links(&mut self, attrs: Attributes, module_id: DefId) {
fn resolve_and_cache(
&mut self,
path_str: &str,
ns: Namespace,
parent_scope: &ParentScope<'ra>,
) -> bool {
// FIXME: This caching may be incorrect in case of multiple `macro_rules`
// items with the same name in the same module.
self.doc_link_resolutions
.entry((Symbol::intern(path_str), ns, parent_scope.module.def_id()))
.or_insert_with_key(|(path, ns, _)| {
self.resolver.resolve_rustdoc_path(path.as_str(), *ns, *parent_scope)
})
.is_some()
}

fn resolve_doc_links(&mut self, attrs: Attributes, parent_scope: ParentScope<'ra>) {
let mut need_traits_in_scope = false;
for (doc_module, doc) in attrs.prepare_to_doc_link_resolution() {
assert_eq!(doc_module, None);
let links = self
.markdown_links
.entry(doc)
.or_insert_with_key(|doc| preprocessed_markdown_links(doc));
let mut tmp_links = mem::take(&mut self.markdown_links);
let links =
tmp_links.entry(doc).or_insert_with_key(|doc| preprocessed_markdown_links(doc));
for PreprocessedMarkdownLink(pp_link, _) in links {
if let Ok(pinfo) = pp_link {
// FIXME: Resolve the path in all namespaces and resolve its prefixes too.
let ns = TypeNS;
self.doc_link_resolutions
.entry((Symbol::intern(&pinfo.path_str), ns, module_id))
.or_insert_with_key(|(path, ns, module_id)| {
self.resolver.resolve_rustdoc_path(path.as_str(), *ns, *module_id)
});
need_traits_in_scope = true;
// The logic here is a conservative approximation for path resolution in
// `resolve_with_disambiguator`.
if let Some(ns) = pinfo.disambiguator.map(Disambiguator::ns) {
if self.resolve_and_cache(&pinfo.path_str, ns, &parent_scope) {
continue;
}
}

// Resolve all namespaces due to no disambiguator or for diagnostics.
let mut any_resolved = false;
let mut need_assoc = false;
for ns in [TypeNS, ValueNS, MacroNS] {
if self.resolve_and_cache(&pinfo.path_str, ns, &parent_scope) {
any_resolved = true;
} else if ns != MacroNS {
need_assoc = true;
}
}

// FIXME: Resolve all prefixes for type-relative resolution or for diagnostics.
if (need_assoc || !any_resolved) && pinfo.path_str.contains("::") {
need_traits_in_scope = true;
}
}
}
self.markdown_links = tmp_links;
}

if need_traits_in_scope {
self.add_traits_in_scope(module_id);
self.add_traits_in_scope(parent_scope.module.def_id());
}
}

Expand Down Expand Up @@ -274,19 +307,33 @@ impl Visitor<'_> for EarlyDocLinkResolver<'_, '_> {
fn visit_item(&mut self, item: &ast::Item) {
self.resolve_doc_links_local(&item.attrs); // Outer attribute scope
if let ItemKind::Mod(..) = item.kind {
let old_mod = mem::replace(&mut self.current_mod, self.resolver.local_def_id(item.id));
let module_def_id = self.resolver.local_def_id(item.id).to_def_id();
let module = self.resolver.expect_module(module_def_id);
let old_module = mem::replace(&mut self.parent_scope.module, module);
let old_macro_rules = self.parent_scope.macro_rules;
self.resolve_doc_links_local(&item.attrs); // Inner attribute scope
self.process_module_children_or_reexports(self.current_mod.to_def_id());
self.process_module_children_or_reexports(module_def_id);
visit::walk_item(self, item);
self.current_mod = old_mod;
if item
.attrs
.iter()
.all(|attr| !attr.has_name(sym::macro_use) && !attr.has_name(sym::macro_escape))
{
self.parent_scope.macro_rules = old_macro_rules;
}
self.parent_scope.module = old_module;
} else {
match item.kind {
match &item.kind {
ItemKind::Trait(..) => {
self.all_traits.push(self.resolver.local_def_id(item.id).to_def_id());
}
ItemKind::Impl(box ast::Impl { of_trait: Some(..), .. }) => {
self.all_trait_impls.push(self.resolver.local_def_id(item.id).to_def_id());
}
ItemKind::MacroDef(macro_def) if macro_def.macro_rules => {
self.parent_scope.macro_rules =
self.resolver.macro_rules_scope(self.resolver.local_def_id(item.id));
}
_ => {}
}
visit::walk_item(self, item);
Expand All @@ -313,6 +360,12 @@ impl Visitor<'_> for EarlyDocLinkResolver<'_, '_> {
visit::walk_field_def(self, field)
}

fn visit_block(&mut self, block: &ast::Block) {
let old_macro_rules = self.parent_scope.macro_rules;
visit::walk_block(self, block);
self.parent_scope.macro_rules = old_macro_rules;
}

// NOTE: if doc-comments are ever allowed on other nodes (e.g. function parameters),
// then this will have to implement other visitor methods too.
}
27 changes: 27 additions & 0 deletions src/test/rustdoc-ui/intra-doc/macro-rules-error.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// `macro_rules` scopes are respected during doc link resolution.

// compile-flags: --document-private-items

#![deny(rustdoc::broken_intra_doc_links)]

mod no_escape {
macro_rules! before_but_limited_to_module {
() => {};
}
}

/// [before_but_limited_to_module] FIXME: This error should be reported
// ERROR unresolved link to `before_but_limited_to_module`
/// [after] FIXME: This error should be reported
// ERROR unresolved link to `after`
/// [str] FIXME: This error shouldn not be reported
//~^ ERROR `str` is both a builtin type and a macro
fn check() {}

macro_rules! after {
() => {};
}

macro_rules! str {
() => {};
}
Loading