From 7cd0291f0fb481d9bc49394db23ecc2f96084450 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Wed, 1 Dec 2021 17:21:05 +0000 Subject: [PATCH 1/5] Fix bug where rustdoc would resolve macros that weren't in scope --- .../passes/collect_intra_doc_links.rs | 17 +---------------- src/test/rustdoc-ui/intra-doc/private-macro.rs | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 16 deletions(-) create mode 100644 src/test/rustdoc-ui/intra-doc/private-macro.rs diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 4e5812d7f8429..139bcb12ef683 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -396,29 +396,14 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { path_str: &'a str, module_id: DefId, ) -> Result> { - let path = ast::Path::from_ident(Ident::from_str(path_str)); self.cx.enter_resolver(|resolver| { - // FIXME(jynelson): does this really need 3 separate lookups? - if let Ok((Some(ext), res)) = resolver.resolve_macro_path( - &path, - None, - &ParentScope::module(resolver.graph_root(), resolver), - false, - false, - ) { - if let SyntaxExtensionKind::LegacyBang { .. } = ext.kind { - return Ok(res.try_into().unwrap()); - } - } - if let Some(&res) = resolver.all_macros().get(&Symbol::intern(path_str)) { - return Ok(res.try_into().unwrap()); - } debug!("resolving {} as a macro in the module {:?}", path_str, module_id); if let Ok((_, res)) = resolver.resolve_str_path_error(DUMMY_SP, path_str, MacroNS, module_id) { // don't resolve builtins like `#[derive]` if let Ok(res) = res.try_into() { + debug!("{} resolved to {:?} in macro namespace (3)", path_str, res); return Ok(res); } } diff --git a/src/test/rustdoc-ui/intra-doc/private-macro.rs b/src/test/rustdoc-ui/intra-doc/private-macro.rs new file mode 100644 index 0000000000000..1860563f0ffd7 --- /dev/null +++ b/src/test/rustdoc-ui/intra-doc/private-macro.rs @@ -0,0 +1,14 @@ +// check-pass +#![deny(rustdoc::broken_intra_doc_links)] + +mod bar { + macro_rules! str {() => {}} +} + +pub mod foo { + pub struct Baz {} + impl Baz { + /// [str] + pub fn foo(&self) {} + } +} From 9630a819d8a328dfe65236566c3459403bafe615 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Wed, 1 Dec 2021 17:33:08 +0000 Subject: [PATCH 2/5] remove unnecessary special-casing for `resolve_macro` --- .../passes/collect_intra_doc_links.rs | 98 +++---------------- 1 file changed, 16 insertions(+), 82 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 139bcb12ef683..b6aff3a667f71 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -2,10 +2,8 @@ //! //! [RFC 1946]: https://github.com/rust-lang/rfcs/blob/master/text/1946-intra-rustdoc-links.md -use rustc_ast as ast; use rustc_data_structures::{fx::FxHashMap, stable_set::FxHashSet}; use rustc_errors::{Applicability, DiagnosticBuilder}; -use rustc_expand::base::SyntaxExtensionKind; use rustc_hir as hir; use rustc_hir::def::{ DefKind, @@ -388,33 +386,6 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { }) } - /// Resolves a string as a macro. - /// - /// FIXME(jynelson): Can this be unified with `resolve()`? - fn resolve_macro( - &self, - path_str: &'a str, - module_id: DefId, - ) -> Result> { - self.cx.enter_resolver(|resolver| { - debug!("resolving {} as a macro in the module {:?}", path_str, module_id); - if let Ok((_, res)) = - resolver.resolve_str_path_error(DUMMY_SP, path_str, MacroNS, module_id) - { - // don't resolve builtins like `#[derive]` - if let Ok(res) = res.try_into() { - debug!("{} resolved to {:?} in macro namespace (3)", path_str, res); - return Ok(res); - } - } - Err(ResolutionFailure::NotResolved { - module_id, - partial_res: None, - unresolved: path_str.into(), - }) - }) - } - /// Convenience wrapper around `resolve_str_path_error`. /// /// This also handles resolving `true` and `false` as booleans. @@ -688,15 +659,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { module_id: DefId, extra_fragment: &Option, ) -> Option { - // resolve() can't be used for macro namespace - let result = match ns { - Namespace::MacroNS => self.resolve_macro(path_str, module_id).map_err(ErrorKind::from), - Namespace::TypeNS | Namespace::ValueNS => { - self.resolve(path_str, ns, module_id, extra_fragment).map(|(res, _)| res) - } - }; - - let res = match result { + let res = match self.resolve(path_str, ns, module_id, extra_fragment).map(|(res, _)| res) { Ok(res) => Some(res), Err(ErrorKind::Resolve(box kind)) => kind.full_res(), Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(res))) => Some(res), @@ -1341,16 +1304,16 @@ impl LinkCollector<'_, '_> { let extra_fragment = &key.extra_fragment; match disambiguator.map(Disambiguator::ns) { - Some(expected_ns @ (ValueNS | TypeNS)) => { + Some(expected_ns) => { match self.resolve(path_str, expected_ns, base_node, extra_fragment) { Ok(res) => Some(res), Err(ErrorKind::Resolve(box mut kind)) => { // We only looked in one namespace. Try to give a better error if possible. if kind.full_res().is_none() { - let other_ns = if expected_ns == ValueNS { TypeNS } else { ValueNS }; + let all_ns = [TypeNS, ValueNS, MacroNS]; // FIXME: really it should be `resolution_failure` that does this, not `resolve_with_disambiguator` // See https://github.com/rust-lang/rust/pull/76955#discussion_r493953382 for a good approach - for new_ns in [other_ns, MacroNS] { + for new_ns in all_ns.into_iter().filter(|x| *x != expected_ns) { if let Some(res) = self.check_full_res(new_ns, path_str, base_node, extra_fragment) { @@ -1373,35 +1336,25 @@ impl LinkCollector<'_, '_> { } None => { // Try everything! - let mut candidates = PerNS { - macro_ns: self - .resolve_macro(path_str, base_node) - .map(|res| (res, extra_fragment.clone())), - type_ns: match self.resolve(path_str, TypeNS, base_node, extra_fragment) { - Ok(res) => { - debug!("got res in TypeNS: {:?}", res); - Ok(res) - } + let mut do_resolve = + |ns| match self.resolve(path_str, ns, base_node, extra_fragment) { + Ok(res) => Some(Ok(res)), Err(ErrorKind::AnchorFailure(msg)) => { - anchor_failure(self.cx, diag, msg); - return None; - } - Err(ErrorKind::Resolve(box kind)) => Err(kind), - }, - value_ns: match self.resolve(path_str, ValueNS, base_node, extra_fragment) { - Ok(res) => Ok(res), - Err(ErrorKind::AnchorFailure(msg)) => { - anchor_failure(self.cx, diag, msg); - return None; + anchor_failure(self.cx, diag.clone(), msg); + None } - Err(ErrorKind::Resolve(box kind)) => Err(kind), - } - .and_then(|(res, fragment)| { + Err(ErrorKind::Resolve(box kind)) => Some(Err(kind)), + }; + let mut candidates = PerNS { + macro_ns: do_resolve(MacroNS)?, + type_ns: do_resolve(TypeNS)?, + value_ns: do_resolve(ValueNS)?.and_then(|(res, fragment)| { // Constructors are picked up in the type namespace. match res { Res::Def(DefKind::Ctor(..), _) => { Err(ResolutionFailure::WrongNamespace { res, expected_ns: TypeNS }) } + // TODO(jyn514): this looks very wrong _ => { match (fragment, extra_fragment.clone()) { (Some(fragment), Some(_)) => { @@ -1443,25 +1396,6 @@ impl LinkCollector<'_, '_> { None } } - Some(MacroNS) => { - match self.resolve_macro(path_str, base_node) { - Ok(res) => Some((res, extra_fragment.clone())), - Err(mut kind) => { - // `resolve_macro` only looks in the macro namespace. Try to give a better error if possible. - for ns in [TypeNS, ValueNS] { - if let Some(res) = - self.check_full_res(ns, path_str, base_node, extra_fragment) - { - kind = - ResolutionFailure::WrongNamespace { res, expected_ns: MacroNS }; - break; - } - } - resolution_failure(self, diag, path_str, disambiguator, smallvec![kind]); - None - } - } - } } } } From 956f7ee0f7715c409995b58e6927177cac9ee26e Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Wed, 1 Dec 2021 17:37:06 +0000 Subject: [PATCH 3/5] remove skechy-looking special casing for fragments in the type namespace rustdoc should already have error-ed out for conflicting anchors, and it doesn't make sense to do this for only one namespace anyway. --- src/librustdoc/passes/collect_intra_doc_links.rs | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index b6aff3a667f71..36daa3fc8a714 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -1292,7 +1292,6 @@ impl LinkCollector<'_, '_> { } /// After parsing the disambiguator, resolve the main part of the link. - // FIXME(jynelson): wow this is just so much fn resolve_with_disambiguator( &mut self, key: &ResolutionInfo, @@ -1354,16 +1353,7 @@ impl LinkCollector<'_, '_> { Res::Def(DefKind::Ctor(..), _) => { Err(ResolutionFailure::WrongNamespace { res, expected_ns: TypeNS }) } - // TODO(jyn514): this looks very wrong - _ => { - match (fragment, extra_fragment.clone()) { - (Some(fragment), Some(_)) => { - // Shouldn't happen but who knows? - Ok((res, Some(fragment))) - } - (fragment, None) | (None, fragment) => Ok((res, fragment)), - } - } + _ => Ok((res, fragment)), } }), }; From 496be6bbec465f4f2c0887b1b11f0ff151ffa8c1 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Wed, 1 Dec 2021 18:05:09 +0000 Subject: [PATCH 4/5] Remove unnecessary `all_macros` field from the resolver --- compiler/rustc_resolve/src/build_reduced_graph.rs | 1 - compiler/rustc_resolve/src/lib.rs | 7 ------- 2 files changed, 8 deletions(-) diff --git a/compiler/rustc_resolve/src/build_reduced_graph.rs b/compiler/rustc_resolve/src/build_reduced_graph.rs index 3cf9d324a38da..0b781a8f1a2f9 100644 --- a/compiler/rustc_resolve/src/build_reduced_graph.rs +++ b/compiler/rustc_resolve/src/build_reduced_graph.rs @@ -1234,7 +1234,6 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { }; let binding = (res, vis, span, expansion).to_name_binding(self.r.arenas); self.r.set_binding_parent_module(binding, parent_scope.module); - self.r.all_macros.insert(ident.name, res); if is_macro_export { let module = self.r.graph_root; self.r.define(module, ident, MacroNS, (res, vis, span, expansion, IsMacroExport)); diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs index d17e8875a1ec0..bd0be05eb0520 100644 --- a/compiler/rustc_resolve/src/lib.rs +++ b/compiler/rustc_resolve/src/lib.rs @@ -981,7 +981,6 @@ pub struct Resolver<'a> { registered_attrs: FxHashSet, registered_tools: FxHashSet, macro_use_prelude: FxHashMap>, - all_macros: FxHashMap, macro_map: FxHashMap>, dummy_ext_bang: Lrc, dummy_ext_derive: Lrc, @@ -1370,7 +1369,6 @@ impl<'a> Resolver<'a> { registered_attrs, registered_tools, macro_use_prelude: FxHashMap::default(), - all_macros: FxHashMap::default(), macro_map: FxHashMap::default(), dummy_ext_bang: Lrc::new(SyntaxExtension::dummy_bang(session.edition())), dummy_ext_derive: Lrc::new(SyntaxExtension::dummy_derive(session.edition())), @@ -3383,11 +3381,6 @@ impl<'a> Resolver<'a> { self.graph_root } - // For rustdoc. - pub fn all_macros(&self) -> &FxHashMap { - &self.all_macros - } - /// Retrieves the span of the given `DefId` if `DefId` is in the local crate. #[inline] pub fn opt_span(&self, def_id: DefId) -> Option { From e70e74fc945b495babcfeb50925e20e8bbc20f7e Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Wed, 1 Dec 2021 18:07:54 +0000 Subject: [PATCH 5/5] Make `resolve_macro_path` private --- compiler/rustc_resolve/src/macros.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_resolve/src/macros.rs b/compiler/rustc_resolve/src/macros.rs index 28dbce0471eaf..7b11adf28eb5c 100644 --- a/compiler/rustc_resolve/src/macros.rs +++ b/compiler/rustc_resolve/src/macros.rs @@ -550,7 +550,7 @@ impl<'a> Resolver<'a> { Ok((ext, res)) } - pub fn resolve_macro_path( + crate fn resolve_macro_path( &mut self, path: &ast::Path, kind: Option,