Prevent private/hidden items to create ambiguities in intra-doc links

GuillaumeGomez · GuillaumeGomez · commit e20b4d47b0c8 · 2024-10-08T23:36:38.000+02:00
diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs
@@ -512,6 +512,8 @@ impl Item {
     pub(crate) fn links(&self, cx: &Context<'_>) -> Vec<RenderedLink> {
         use crate::html::format::{href, link_tooltip};
 
+        eprintln!("yyyyyyyy {self:?}");
+
         let Some(links) = cx.cache().intra_doc_links.get(&self.item_id) else { return vec![] };
         links
             .iter()
diff --git a/src/librustdoc/core.rs b/src/librustdoc/core.rs
@@ -30,6 +30,7 @@ use crate::clean::{self, ItemId};
 use crate::config::{Options as RustdocOptions, OutputFormat, RenderOptions};
 use crate::formats::cache::Cache;
 use crate::passes::Condition::*;
+use crate::passes::collect_intra_doc_links::{LinkCollector, Res as CRes};
 use crate::passes::{self};
 
 pub(crate) struct DocContext<'tcx> {
@@ -440,6 +441,10 @@ pub(crate) fn run_global_ctxt(
         }
     }
 
+    let (mut krate, LinkCollector { visited_links, mut ambiguous_links, .. }) =
+        tcx.sess.time("collect_intra_doc_links", || {
+            passes::collect_intra_doc_links::collect_intra_doc_links(krate, &mut ctxt)
+        });
     tcx.sess.time("check_lint_expectations", || tcx.check_expectations(Some(sym::rustdoc)));
 
     if let Some(guar) = tcx.dcx().has_errors() {
@@ -448,6 +453,30 @@ pub(crate) fn run_global_ctxt(
 
     krate = tcx.sess.time("create_format_cache", || Cache::populate(&mut ctxt, krate));
 
+    let mut collector =
+        LinkCollector { cx: &mut ctxt, visited_links, ambiguous_links: Default::default() };
+
+    for ((item_id, path_str), info) in ambiguous_links.iter_mut() {
+        info.resolved.retain(|(res, _)| match res {
+            CRes::Def(_, def_id) => collector.validate_link(*def_id),
+            CRes::Primitive(_) => true,
+        });
+        if info.resolved.len() == 1 {
+            let (res, fragment) = info.resolved.pop().unwrap();
+            let diag_info = info.diag_info.into_info();
+            if let Some(link) = collector.compute_link(
+                res,
+                fragment,
+                path_str,
+                info.disambiguator,
+                diag_info,
+                &info.link_text,
+            ) {
+                collector.save_link(*item_id, link);
+            }
+        }
+    }
+
     Ok((krate, ctxt.render_options, ctxt.cache))
 }
 
diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs
@@ -14,7 +14,7 @@ use rustc_data_structures::intern::Interned;
 use rustc_errors::{Applicability, Diag, DiagMessage};
 use rustc_hir::def::Namespace::*;
 use rustc_hir::def::{DefKind, Namespace, PerNS};
-use rustc_hir::def_id::{CRATE_DEF_ID, DefId};
+use rustc_hir::def_id::{CRATE_DEF_ID, DefId, LOCAL_CRATE};
 use rustc_hir::{Mutability, Safety};
 use rustc_middle::ty::{Ty, TyCtxt};
 use rustc_middle::{bug, span_bug, ty};
@@ -30,23 +30,30 @@ use smallvec::{SmallVec, smallvec};
 use tracing::{debug, info, instrument, trace};
 
 use crate::clean::utils::find_nearest_parent_module;
-use crate::clean::{self, Crate, Item, ItemLink, PrimitiveType};
+use crate::clean::{self, Crate, Item, ItemId, ItemLink, PrimitiveType};
 use crate::core::DocContext;
 use crate::html::markdown::{MarkdownLink, MarkdownLinkRange, markdown_links};
 use crate::lint::{BROKEN_INTRA_DOC_LINKS, PRIVATE_INTRA_DOC_LINKS};
-use crate::passes::Pass;
 use crate::visit::DocVisitor;
 
-pub(crate) const COLLECT_INTRA_DOC_LINKS: Pass = Pass {
-    name: "collect-intra-doc-links",
-    run: collect_intra_doc_links,
-    description: "resolves intra-doc links",
-};
-
-fn collect_intra_doc_links(krate: Crate, cx: &mut DocContext<'_>) -> Crate {
-    let mut collector = LinkCollector { cx, visited_links: FxHashMap::default() };
+pub(crate) fn collect_intra_doc_links<'a, 'tcx>(
+    krate: Crate,
+    cx: &'a mut DocContext<'tcx>,
+) -> (Crate, LinkCollector<'a, 'tcx>) {
+    let mut collector = LinkCollector {
+        cx,
+        visited_links: FxHashMap::default(),
+        ambiguous_links: FxHashMap::default(),
+    };
     collector.visit_crate(&krate);
-    krate
+    (krate, collector)
+}
+
+pub(crate) struct AmbiguousLinks {
+    pub(crate) disambiguator: Option<Disambiguator>,
+    pub(crate) link_text: Box<str>,
+    pub(crate) diag_info: OwnedDiagnosticInfo,
+    pub(crate) resolved: Vec<(Res, Option<UrlFragment>)>,
 }
 
 fn filter_assoc_items_by_name_and_namespace<'a>(
@@ -61,7 +68,7 @@ fn filter_assoc_items_by_name_and_namespace<'a>(
 }
 
 #[derive(Copy, Clone, Debug, Hash, PartialEq)]
-enum Res {
+pub(crate) enum Res {
     Def(DefKind, DefId),
     Primitive(PrimitiveType),
 }
@@ -233,7 +240,7 @@ impl UrlFragment {
 }
 
 #[derive(Clone, Debug, Hash, PartialEq, Eq)]
-struct ResolutionInfo {
+pub(crate) struct ResolutionInfo {
     item_id: DefId,
     module_id: DefId,
     dis: Option<Disambiguator>,
@@ -242,18 +249,54 @@ struct ResolutionInfo {
 }
 
 #[derive(Clone)]
-struct DiagnosticInfo<'a> {
+pub(crate) struct DiagnosticInfo<'a> {
     item: &'a Item,
     dox: &'a str,
     ori_link: &'a str,
     link_range: MarkdownLinkRange,
 }
 
-struct LinkCollector<'a, 'tcx> {
-    cx: &'a mut DocContext<'tcx>,
+pub(crate) struct OwnedDiagnosticInfo {
+    item: Item,
+    ori_link: String,
+    link_range: MarkdownLinkRange,
+}
+
+impl From<DiagnosticInfo<'_>> for OwnedDiagnosticInfo {
+    fn from(f: DiagnosticInfo<'_>) -> Self {
+        Self {
+            item: f.item.clone(),
+            ori_link: f.ori_link.to_string(),
+            link_range: f.link_range.clone(),
+        }
+    }
+}
+
+impl OwnedDiagnosticInfo {
+    pub(crate) fn into_info(&self) -> DiagnosticInfo<'_> {
+        DiagnosticInfo {
+            item: &self.item,
+            ori_link: &self.ori_link,
+            dox: "",
+            link_range: self.link_range.clone(),
+        }
+    }
+}
+
+pub(crate) struct LinkCollector<'a, 'tcx> {
+    pub(crate) cx: &'a mut DocContext<'tcx>,
     /// Cache the resolved links so we can avoid resolving (and emitting errors for) the same link.
     /// The link will be `None` if it could not be resolved (i.e. the error was cached).
-    visited_links: FxHashMap<ResolutionInfo, Option<(Res, Option<UrlFragment>)>>,
+    pub(crate) visited_links: FxHashMap<ResolutionInfo, Option<(Res, Option<UrlFragment>)>>,
+    /// These links are ambiguous. We need for the cache to have its paths filled. Unfortunately,
+    /// if we run the `LinkCollector` pass after `Cache::populate`, a lot of items that we need
+    /// to go through will be removed, making a lot of intra-doc links to not be inferred.
+    ///
+    /// So instead, we store the ambiguous links and we wait for cache paths to be filled before
+    /// inferring them (if possible).
+    ///
+    /// Key is `(item ID, path str)`.
+    pub(crate) ambiguous_links: FxHashMap<(ItemId, String), AmbiguousLinks>,
 }
 
 impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
@@ -981,14 +1024,17 @@ impl LinkCollector<'_, '_> {
                 _ => find_nearest_parent_module(self.cx.tcx, item_id).unwrap(),
             };
             for md_link in preprocessed_markdown_links(&doc) {
-                let link = self.resolve_link(&doc, item, item_id, module_id, &md_link);
-                if let Some(link) = link {
-                    self.cx.cache.intra_doc_links.entry(item.item_id).or_default().insert(link);
+                if let Some(link) = self.resolve_link(&doc, item, item_id, module_id, &md_link) {
+                    self.save_link(item.item_id, link);
                 }
             }
         }
     }
 
+    pub(crate) fn save_link(&mut self, item_id: ItemId, link: ItemLink) {
+        self.cx.cache.intra_doc_links.entry(item_id).or_default().insert(link);
+    }
+
     /// This is the entry point for resolving an intra-doc link.
     ///
     /// FIXME(jynelson): this is way too many arguments
@@ -1012,7 +1058,7 @@ impl LinkCollector<'_, '_> {
             pp_link.as_ref().map_err(|err| err.report(self.cx, diag_info.clone())).ok()?;
         let disambiguator = *disambiguator;
 
-        let (mut res, fragment) = self.resolve_with_disambiguator_cached(
+        let mut resolved = self.resolve_with_disambiguator_cached(
             ResolutionInfo {
                 item_id,
                 module_id,
@@ -1028,6 +1074,75 @@ impl LinkCollector<'_, '_> {
             false,
         )?;
 
+        if resolved.len() > 1 {
+            let links = AmbiguousLinks {
+                disambiguator,
+                link_text: link_text.clone(),
+                diag_info: diag_info.into(),
+                resolved,
+            };
+            self.ambiguous_links.insert((item.item_id, path_str.to_string()), links);
+            None
+        } else if let Some((res, fragment)) = resolved.pop() {
+            self.compute_link(res, fragment, path_str, disambiguator, diag_info, link_text)
+        } else {
+            None
+        }
+    }
+
+    /// Returns `true` if a link could be generated from the given intra-doc information.
+    ///
+    /// This is a very light version of `format::href_with_root_path` since we're only interested
+    /// about whether we can generate a link to an item or not.
+    ///
+    /// * If `original_did` is local, then we check if the item is reexported or public.
+    /// * If `original_did` is not local, then we check if the crate it comes from is a direct
+    ///   public dependency.
+    pub(crate) fn validate_link(&self, original_did: DefId) -> bool {
+        let tcx = self.cx.tcx;
+        let def_kind = tcx.def_kind(original_did);
+        let did = match def_kind {
+            DefKind::AssocTy | DefKind::AssocFn | DefKind::AssocConst | DefKind::Variant => {
+                // documented on their parent's page
+                tcx.parent(original_did)
+            }
+            // If this a constructor, we get the parent (either a struct or a variant) and then
+            // generate the link for this item.
+            DefKind::Ctor(..) => return self.validate_link(tcx.parent(original_did)),
+            DefKind::ExternCrate => {
+                // Link to the crate itself, not the `extern crate` item.
+                if let Some(local_did) = original_did.as_local() {
+                    tcx.extern_mod_stmt_cnum(local_did).unwrap_or(LOCAL_CRATE).as_def_id()
+                } else {
+                    original_did
+                }
+            }
+            _ => original_did,
+        };
+
+        let cache = &self.cx.cache;
+        if !original_did.is_local()
+            && !cache.effective_visibilities.is_directly_public(tcx, did)
+            && !cache.document_private
+            && !cache.primitive_locations.values().any(|&id| id == did)
+        {
+            return false;
+        }
+
+        cache.paths.get(&did).is_some()
+            || cache.external_paths.get(&did).is_some()
+            || !did.is_local()
+    }
+
+    pub(crate) fn compute_link(
+        &mut self,
+        mut res: Res,
+        fragment: Option<UrlFragment>,
+        path_str: &str,
+        disambiguator: Option<Disambiguator>,
+        diag_info: DiagnosticInfo<'_>,
+        link_text: &Box<str>,
+    ) -> Option<ItemLink> {
         // Check for a primitive which might conflict with a module
         // Report the ambiguity and require that the user specify which one they meant.
         // FIXME: could there ever be a primitive not in the type namespace?
@@ -1073,7 +1188,7 @@ impl LinkCollector<'_, '_> {
                 }
 
                 res.def_id(self.cx.tcx).map(|page_id| ItemLink {
-                    link: Box::<str>::from(&*ori_link.link),
+                    link: Box::<str>::from(&*diag_info.ori_link),
                     link_text: link_text.clone(),
                     page_id,
                     fragment,
@@ -1095,7 +1210,7 @@ impl LinkCollector<'_, '_> {
 
                 let page_id = clean::register_res(self.cx, rustc_hir::def::Res::Def(kind, id));
                 Some(ItemLink {
-                    link: Box::<str>::from(&*ori_link.link),
+                    link: Box::<str>::from(&*diag_info.ori_link),
                     link_text: link_text.clone(),
                     page_id,
                     fragment,
@@ -1208,10 +1323,10 @@ impl LinkCollector<'_, '_> {
         // If this call is intended to be recoverable, then pass true to silence.
         // This is only recoverable when path is failed to resolved.
         recoverable: bool,
-    ) -> Option<(Res, Option<UrlFragment>)> {
+    ) -> Option<Vec<(Res, Option<UrlFragment>)>> {
         if let Some(res) = self.visited_links.get(&key) {
             if res.is_some() || cache_errors {
-                return res.clone();
+                return res.clone().map(|r| vec![r]);
             }
         }
 
@@ -1242,25 +1357,28 @@ impl LinkCollector<'_, '_> {
             candidates = vec![candidate];
         }
 
-        if let &[(res, def_id)] = candidates.as_slice() {
+        let mut resolved = Vec::with_capacity(candidates.len());
+        for (res, def_id) in candidates.as_slice() {
             let fragment = match (&key.extra_fragment, def_id) {
                 (Some(_), Some(def_id)) => {
-                    report_anchor_conflict(self.cx, diag, def_id);
+                    report_anchor_conflict(self.cx, diag, *def_id);
                     return None;
                 }
                 (Some(u_frag), None) => Some(UrlFragment::UserWritten(u_frag.clone())),
-                (None, Some(def_id)) => Some(UrlFragment::Item(def_id)),
+                (None, Some(def_id)) => Some(UrlFragment::Item(*def_id)),
                 (None, None) => None,
             };
-            let r = Some((res, fragment));
-            self.visited_links.insert(key, r.clone());
-            return r;
+            let r = (res.clone(), fragment.clone());
+            self.visited_links.insert(key.clone(), Some(r.clone()));
+            resolved.push(r);
         }
 
-        if cache_errors {
+        if resolved.is_empty() && cache_errors {
             self.visited_links.insert(key, None);
+            None
+        } else {
+            Some(resolved)
         }
-        None
     }
 
     /// After parsing the disambiguator, resolve the main part of the link.
@@ -1416,7 +1534,7 @@ fn should_ignore_link(path_str: &str) -> bool {
 
 #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
 /// Disambiguators for a link.
-enum Disambiguator {
+pub(crate) enum Disambiguator {
     /// `prim@`
     ///
     /// This is buggy, see <https://github.com/rust-lang/rust/pull/77875#discussion_r503583103>
diff --git a/src/librustdoc/passes/mod.rs b/src/librustdoc/passes/mod.rs
@@ -24,7 +24,6 @@ mod propagate_doc_cfg;
 pub(crate) use self::propagate_doc_cfg::PROPAGATE_DOC_CFG;
 
 pub(crate) mod collect_intra_doc_links;
-pub(crate) use self::collect_intra_doc_links::COLLECT_INTRA_DOC_LINKS;
 
 mod check_doc_test_visibility;
 pub(crate) use self::check_doc_test_visibility::CHECK_DOC_TEST_VISIBILITY;
@@ -75,7 +74,6 @@ pub(crate) const PASSES: &[Pass] = &[
     STRIP_PRIVATE,
     STRIP_PRIV_IMPORTS,
     PROPAGATE_DOC_CFG,
-    COLLECT_INTRA_DOC_LINKS,
     COLLECT_TRAIT_IMPLS,
     CALCULATE_DOC_COVERAGE,
     RUN_LINTS,
@@ -89,7 +87,6 @@ pub(crate) const DEFAULT_PASSES: &[ConditionalPass] = &[
     ConditionalPass::new(STRIP_HIDDEN, WhenNotDocumentHidden),
     ConditionalPass::new(STRIP_PRIVATE, WhenNotDocumentPrivate),
     ConditionalPass::new(STRIP_PRIV_IMPORTS, WhenDocumentPrivate),
-    ConditionalPass::always(COLLECT_INTRA_DOC_LINKS),
     ConditionalPass::always(PROPAGATE_DOC_CFG),
     ConditionalPass::always(RUN_LINTS),
 ];
