Skip to content

Commit

Permalink
Auto merge of #77859 - bugadani:no-duplicate-ref-link-error, r=jyn514
Browse files Browse the repository at this point in the history 
Rustdoc: only report broken ref-style links once

This PR assigns the markdown `LinkType` to each parsed link and passes this information into the link collector.
If a link can't be resolved in `resolve_with_disambiguator`, the failure is cached for the link types where we only want to report the error once (namely `Shortcut` and `Reference`).

Fixes  #77681
  • Loading branch information
@bors
bors committed Jan 3, 2021
2 parents db69136 + 4b612dd commit 8018418
Show file tree
Hide file tree
Showing 6 changed files with 175 additions and 67 deletions.
24 changes: 17 additions & 7 deletions src/librustdoc/html/markdown.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,9 @@ use crate::doctest;
use crate::html::highlight;
use crate::html::toc::TocBuilder;

use pulldown_cmark::{html, BrokenLink, CodeBlockKind, CowStr, Event, Options, Parser, Tag};
use pulldown_cmark::{
html, BrokenLink, CodeBlockKind, CowStr, Event, LinkType, Options, Parser, Tag,
};

#[cfg(test)]
mod tests;
Expand Down Expand Up @@ -327,8 +329,6 @@ impl<'a, I: Iterator<Item = Event<'a>>> Iterator for LinkReplacer<'a, I> {
type Item = Event<'a>;

fn next(&mut self) -> Option<Self::Item> {
use pulldown_cmark::LinkType;

let mut event = self.inner.next();

// Replace intra-doc links and remove disambiguators from shortcut links (`[fn@f]`).
Expand Down Expand Up @@ -1123,7 +1123,13 @@ crate fn plain_text_summary(md: &str) -> String {
s
}

crate fn markdown_links(md: &str) -> Vec<(String, Range<usize>)> {
crate struct MarkdownLink {
pub kind: LinkType,
pub link: String,
pub range: Range<usize>,
}

crate fn markdown_links(md: &str) -> Vec<MarkdownLink> {
if md.is_empty() {
return vec![];
}
Expand Down Expand Up @@ -1163,7 +1169,11 @@ crate fn markdown_links(md: &str) -> Vec<(String, Range<usize>)> {

let mut push = |link: BrokenLink<'_>| {
let span = span_for_link(&CowStr::Borrowed(link.reference), link.span);
links.borrow_mut().push((link.reference.to_owned(), span));
links.borrow_mut().push(MarkdownLink {
kind: LinkType::ShortcutUnknown,
link: link.reference.to_owned(),
range: span,
});
None
};
let p = Parser::new_with_broken_link_callback(md, opts(), Some(&mut push)).into_offset_iter();
Expand All @@ -1174,10 +1184,10 @@ crate fn markdown_links(md: &str) -> Vec<(String, Range<usize>)> {
let iter = Footnotes::new(HeadingLinks::new(p, None, &mut ids));

for ev in iter {
if let Event::Start(Tag::Link(_, dest, _)) = ev.0 {
if let Event::Start(Tag::Link(kind, dest, _)) = ev.0 {
debug!("found link: {}", dest);
let span = span_for_link(&dest, ev.1);
links.borrow_mut().push((dest.into_string(), span));
links.borrow_mut().push(MarkdownLink { kind, link: dest.into_string(), range: span });
}
}

Expand Down
126 changes: 74 additions & 52 deletions src/librustdoc/passes/collect_intra_doc_links.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ use rustc_span::symbol::Symbol;
use rustc_span::DUMMY_SP;
use smallvec::{smallvec, SmallVec};

use pulldown_cmark::LinkType;

use std::borrow::Cow;
use std::cell::Cell;
use std::convert::{TryFrom, TryInto};
Expand All @@ -34,7 +36,7 @@ use std::ops::Range;
use crate::clean::{self, utils::find_nearest_parent_module, Crate, Item, ItemLink, PrimitiveType};
use crate::core::DocContext;
use crate::fold::DocFolder;
use crate::html::markdown::markdown_links;
use crate::html::markdown::{markdown_links, MarkdownLink};
use crate::passes::Pass;

use super::span_of_attrs;
Expand Down Expand Up @@ -265,8 +267,9 @@ struct LinkCollector<'a, 'tcx> {
/// because `clean` and the disambiguator code expect them to be different.
/// See the code for associated items on inherent impls for details.
kind_side_channel: Cell<Option<(DefKind, DefId)>>,
/// Cache the resolved links so we can avoid resolving (and emitting errors for) the same link
visited_links: FxHashMap<ResolutionInfo, CachedLink>,
/// 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<CachedLink>>,
}

impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
Expand Down Expand Up @@ -901,16 +904,8 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
};
// NOTE: if there are links that start in one crate and end in another, this will not resolve them.
// This is a degenerate case and it's not supported by rustdoc.
for (ori_link, link_range) in markdown_links(&doc) {
let link = self.resolve_link(
&item,
&doc,
&self_name,
parent_node,
krate,
ori_link,
link_range,
);
for md_link in markdown_links(&doc) {
let link = self.resolve_link(&item, &doc, &self_name, parent_node, krate, md_link);
if let Some(link) = link {
item.attrs.links.push(link);
}
Expand Down Expand Up @@ -942,27 +937,26 @@ impl LinkCollector<'_, '_> {
self_name: &Option<String>,
parent_node: Option<DefId>,
krate: CrateNum,
ori_link: String,
link_range: Range<usize>,
ori_link: MarkdownLink,
) -> Option<ItemLink> {
trace!("considering link '{}'", ori_link);
trace!("considering link '{}'", ori_link.link);

// Bail early for real links.
if ori_link.contains('/') {
if ori_link.link.contains('/') {
return None;
}

// [] is mostly likely not supposed to be a link
if ori_link.is_empty() {
if ori_link.link.is_empty() {
return None;
}

let cx = self.cx;
let link = ori_link.replace("`", "");
let link = ori_link.link.replace("`", "");
let parts = link.split('#').collect::<Vec<_>>();
let (link, extra_fragment) = if parts.len() > 2 {
// A valid link can't have multiple #'s
anchor_failure(cx, &item, &link, dox, link_range, AnchorFailure::MultipleAnchors);
anchor_failure(cx, &item, &link, dox, ori_link.range, AnchorFailure::MultipleAnchors);
return None;
} else if parts.len() == 2 {
if parts[0].trim().is_empty() {
Expand Down Expand Up @@ -1018,7 +1012,7 @@ impl LinkCollector<'_, '_> {
path_str,
disambiguator,
dox,
link_range,
ori_link.range,
smallvec![ResolutionFailure::NoParentItem],
);
return None;
Expand Down Expand Up @@ -1058,7 +1052,7 @@ impl LinkCollector<'_, '_> {
path_str,
disambiguator,
dox,
link_range,
ori_link.range,
smallvec![err_kind],
);
return None;
Expand All @@ -1074,15 +1068,22 @@ impl LinkCollector<'_, '_> {
return None;
}

let key = ResolutionInfo {
module_id,
dis: disambiguator,
path_str: path_str.to_owned(),
extra_fragment,
let diag_info = DiagnosticInfo {
item,
dox,
ori_link: &ori_link.link,
link_range: ori_link.range.clone(),
};
let diag =
DiagnosticInfo { item, dox, ori_link: &ori_link, link_range: link_range.clone() };
let (mut res, mut fragment) = self.resolve_with_disambiguator_cached(key, diag)?;
let (mut res, mut fragment) = self.resolve_with_disambiguator_cached(
ResolutionInfo {
module_id,
dis: disambiguator,
path_str: path_str.to_owned(),
extra_fragment,
},
diag_info,
matches!(ori_link.kind, LinkType::Reference | LinkType::Shortcut),
)?;

// Check for a primitive which might conflict with a module
// Report the ambiguity and require that the user specify which one they meant.
Expand All @@ -1101,7 +1102,7 @@ impl LinkCollector<'_, '_> {
&item,
path_str,
dox,
link_range,
ori_link.range,
AnchorFailure::RustdocAnchorConflict(prim),
);
return None;
Expand All @@ -1111,7 +1112,7 @@ impl LinkCollector<'_, '_> {
} else {
// `[char]` when a `char` module is in scope
let candidates = vec![res, prim];
ambiguity_error(cx, &item, path_str, dox, link_range, candidates);
ambiguity_error(cx, &item, path_str, dox, ori_link.range, candidates);
return None;
}
}
Expand All @@ -1129,14 +1130,22 @@ impl LinkCollector<'_, '_> {
specified.descr()
);
diag.note(&note);
suggest_disambiguator(resolved, diag, path_str, dox, sp, &link_range);
suggest_disambiguator(resolved, diag, path_str, dox, sp, &ori_link.range);
};
report_diagnostic(cx, BROKEN_INTRA_DOC_LINKS, &msg, &item, dox, &link_range, callback);
report_diagnostic(
cx,
BROKEN_INTRA_DOC_LINKS,
&msg,
&item,
dox,
&ori_link.range,
callback,
);
};
match res {
Res::Primitive(_) => match disambiguator {
Some(Disambiguator::Primitive | Disambiguator::Namespace(_)) | None => {
Some(ItemLink { link: ori_link, link_text, did: None, fragment })
Some(ItemLink { link: ori_link.link, link_text, did: None, fragment })
}
Some(other) => {
report_mismatch(other, Disambiguator::Primitive);
Expand Down Expand Up @@ -1179,11 +1188,11 @@ impl LinkCollector<'_, '_> {
if self.cx.tcx.privacy_access_levels(LOCAL_CRATE).is_exported(hir_src)
&& !self.cx.tcx.privacy_access_levels(LOCAL_CRATE).is_exported(hir_dst)
{
privacy_error(cx, &item, &path_str, dox, link_range);
privacy_error(cx, &item, &path_str, dox, &ori_link);
}
}
let id = clean::register_res(cx, rustc_hir::def::Res::Def(kind, id));
Some(ItemLink { link: ori_link, link_text, did: Some(id), fragment })
Some(ItemLink { link: ori_link.link, link_text, did: Some(id), fragment })
}
}
}
Expand All @@ -1192,28 +1201,47 @@ impl LinkCollector<'_, '_> {
&mut self,
key: ResolutionInfo,
diag: DiagnosticInfo<'_>,
cache_resolution_failure: bool,
) -> Option<(Res, Option<String>)> {
// Try to look up both the result and the corresponding side channel value
if let Some(ref cached) = self.visited_links.get(&key) {
self.kind_side_channel.set(cached.side_channel);
return Some(cached.res.clone());
match cached {
Some(cached) => {
self.kind_side_channel.set(cached.side_channel.clone());
return Some(cached.res.clone());
}
None if cache_resolution_failure => return None,
None => {
// Although we hit the cache and found a resolution error, this link isn't
// supposed to cache those. Run link resolution again to emit the expected
// resolution error.
}
}
}

let res = self.resolve_with_disambiguator(&key, diag);

// Cache only if resolved successfully - don't silence duplicate errors
if let Some(res) = &res {
if let Some(res) = res {
// Store result for the actual namespace
self.visited_links.insert(
key,
CachedLink {
Some(CachedLink {
res: res.clone(),
side_channel: self.kind_side_channel.clone().into_inner(),
},
}),
);
}

res
Some(res)
} else {
if cache_resolution_failure {
// For reference-style links we only want to report one resolution error
// so let's cache them as well.
self.visited_links.insert(key, None);
}

None
}
}

/// After parsing the disambiguator, resolve the main part of the link.
Expand Down Expand Up @@ -1964,13 +1992,7 @@ fn suggest_disambiguator(
}

/// Report a link from a public item to a private one.
fn privacy_error(
cx: &DocContext<'_>,
item: &Item,
path_str: &str,
dox: &str,
link_range: Range<usize>,
) {
fn privacy_error(cx: &DocContext<'_>, item: &Item, path_str: &str, dox: &str, link: &MarkdownLink) {
let sym;
let item_name = match item.name {
Some(name) => {
Expand All @@ -1982,7 +2004,7 @@ fn privacy_error(
let msg =
format!("public documentation for `{}` links to private item `{}`", item_name, path_str);

report_diagnostic(cx, PRIVATE_INTRA_DOC_LINKS, &msg, item, dox, &link_range, |diag, sp| {
report_diagnostic(cx, PRIVATE_INTRA_DOC_LINKS, &msg, item, dox, &link.range, |diag, sp| {
if let Some(sp) = sp {
diag.span_label(sp, "this item is private");
}
Expand Down
20 changes: 20 additions & 0 deletions src/test/rustdoc-ui/reference-link-reports-error-once.rs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#![deny(broken_intra_doc_links)]

/// Links to [a] [link][a]
/// And also a [third link][a]
/// And also a [reference link][b]
///
/// Other links to the same target should still emit error: [ref] //~ERROR unresolved link to `ref`
/// Duplicate [ref] //~ERROR unresolved link to `ref`
///
/// Other links to other targets should still emit error: [ref2] //~ERROR unresolved link to `ref2`
/// Duplicate [ref2] //~ERROR unresolved link to `ref2`
///
/// [a]: ref
//~^ ERROR unresolved link to `ref`
/// [b]: ref2
//~^ ERROR unresolved link to

/// [ref][]
//~^ ERROR unresolved link
pub fn f() {}

0 comments on commit 8018418

Please sign in to comment.