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: Remove doc link resolution fallback to all macro_rules in the crate #96676

Merged
merged 1 commit into from
May 16, 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
52 changes: 22 additions & 30 deletions src/librustdoc/passes/collect_intra_doc_links.rs
Original file line number Diff line number Diff line change
Expand Up @@ -443,21 +443,6 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
})
}

/// HACK: Try to search the macro name in the list of all `macro_rules` items in the crate.
/// Used when nothing else works, may often give an incorrect result.
fn resolve_macro_rules(&self, path_str: &str, ns: Namespace) -> Option<Res> {
if ns != MacroNS {
return None;
}

self.cx
.resolver_caches
.all_macro_rules
.get(&Symbol::intern(path_str))
.copied()
.and_then(|res| res.try_into().ok())
}

/// Convenience wrapper around `resolve_rustdoc_path`.
///
/// This also handles resolving `true` and `false` as booleans.
Expand Down Expand Up @@ -489,8 +474,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
})
})
.and_then(|res| res.try_into().ok())
.or_else(|| resolve_primitive(path_str, ns))
.or_else(|| self.resolve_macro_rules(path_str, ns));
.or_else(|| resolve_primitive(path_str, ns));
debug!("{} resolved to {:?} in namespace {:?}", path_str, result, ns);
result
}
Expand Down Expand Up @@ -1391,11 +1375,7 @@ impl LinkCollector<'_, '_> {
}
}
}
resolution_failure(self, diag, path_str, disambiguator, smallvec![err]);
// This could just be a normal link or a broken link
// we could potentially check if something is
// "intra-doc-link-like" and warn in that case.
None
resolution_failure(self, diag, path_str, disambiguator, smallvec![err])
}
}
}
Expand Down Expand Up @@ -1423,15 +1403,13 @@ impl LinkCollector<'_, '_> {
let len = candidates.iter().filter(|res| res.is_ok()).count();

if len == 0 {
resolution_failure(
return resolution_failure(
self,
diag,
path_str,
disambiguator,
candidates.into_iter().filter_map(|res| res.err()).collect(),
);
// this could just be a normal link
return None;
}

if len == 1 {
Expand Down Expand Up @@ -1737,8 +1715,9 @@ fn resolution_failure(
path_str: &str,
disambiguator: Option<Disambiguator>,
kinds: SmallVec<[ResolutionFailure<'_>; 3]>,
) {
) -> Option<(Res, Option<DefId>)> {
let tcx = collector.cx.tcx;
let mut recovered_res = None;
report_diagnostic(
tcx,
BROKEN_INTRA_DOC_LINKS,
Expand Down Expand Up @@ -1826,11 +1805,22 @@ fn resolution_failure(
diag.note(&note);
}

// If the link has `::` in it, assume it was meant to be an intra-doc link.
// Otherwise, the `[]` might be unrelated.
// FIXME: don't show this for autolinks (`<>`), `()` style links, or reference links
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The FIXME is no longer relevant after #96187 as I understand, so I removed it.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed. Thanks for the cleanup!

if !path_str.contains("::") {
diag.help(r#"to escape `[` and `]` characters, add '\' before them like `\[` or `\]`"#);
if disambiguator.map_or(true, |d| d.ns() == MacroNS)
&& let Some(&res) = collector.cx.resolver_caches.all_macro_rules
.get(&Symbol::intern(path_str))
{
diag.note(format!(
"`macro_rules` named `{path_str}` exists in this crate, \
but it is not in scope at this link's location"
));
recovered_res = res.try_into().ok().map(|res| (res, None));
} else {
// If the link has `::` in it, assume it was meant to be an
// intra-doc link. Otherwise, the `[]` might be unrelated.
diag.help("to escape `[` and `]` characters, \
add '\\' before them like `\\[` or `\\]`");
}
}

continue;
Expand Down Expand Up @@ -1915,6 +1905,8 @@ fn resolution_failure(
}
},
);

recovered_res
}

fn report_multiple_anchors(cx: &DocContext<'_>, diag_info: DiagnosticInfo<'_>) {
Expand Down
11 changes: 5 additions & 6 deletions src/test/rustdoc-ui/intra-doc/macro-rules-error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,11 @@ mod no_escape {
}
}

/// [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
/// [before_but_limited_to_module]
//~^ ERROR unresolved link to `before_but_limited_to_module`
/// [after]
//~^ ERROR unresolved link to `after`
/// [str]
fn check() {}

macro_rules! after {
Expand Down
23 changes: 12 additions & 11 deletions src/test/rustdoc-ui/intra-doc/macro-rules-error.stderr
Original file line number Diff line number Diff line change
@@ -1,22 +1,23 @@
error: `str` is both a builtin type and a macro
--> $DIR/macro-rules-error.rs:17:6
error: unresolved link to `before_but_limited_to_module`
--> $DIR/macro-rules-error.rs:13:6
|
LL | /// [str] FIXME: This error shouldn not be reported
| ^^^ ambiguous link
LL | /// [before_but_limited_to_module]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no item named `before_but_limited_to_module` in scope
|
note: the lint level is defined here
--> $DIR/macro-rules-error.rs:5:9
|
LL | #![deny(rustdoc::broken_intra_doc_links)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: to link to the builtin type, prefix with `prim@`
= note: `macro_rules` named `before_but_limited_to_module` exists in this crate, but it is not in scope at this link's location

error: unresolved link to `after`
--> $DIR/macro-rules-error.rs:15:6
|
LL | /// [prim@str] FIXME: This error shouldn not be reported
| +++++
help: to link to the macro, add an exclamation mark
LL | /// [after]
| ^^^^^ no item named `after` in scope
|
LL | /// [str!] FIXME: This error shouldn not be reported
| +
= note: `macro_rules` named `after` exists in this crate, but it is not in scope at this link's location

error: aborting due to previous error
error: aborting due to 2 previous errors