-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Disallow linking to items with a mismatched disambiguator #75079
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
Changes from 3 commits
8e0e925
519c854
743f932
99354f5
444f5a0
fc273a0
2dad90d
f05e9da
0c99d80
ef54cde
d240490
17263bc
9914f73
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -415,7 +415,8 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { | |
AnchorFailure::Method | ||
})) | ||
} else { | ||
Ok((ty_res, Some(format!("{}.{}", kind, item_name)))) | ||
let res = Res::Def(item.kind.as_def_kind(), item.def_id); | ||
Ok((res, Some(format!("{}.{}", kind, item_name)))) | ||
} | ||
} else { | ||
self.variant_field(path_str, current_item, module_id) | ||
|
@@ -574,9 +575,9 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { | |
}; | ||
let resolved_self; | ||
let mut path_str; | ||
let mut disambiguator = None; | ||
let (res, fragment) = { | ||
let mut kind = None; | ||
let mut disambiguator = None; | ||
path_str = if let Some(prefix) = | ||
["struct@", "enum@", "type@", "trait@", "union@", "module@", "mod@"] | ||
.iter() | ||
|
@@ -595,6 +596,7 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { | |
link.trim_start_matches(prefix) | ||
} else if link.ends_with("!()") { | ||
kind = Some(MacroNS); | ||
disambiguator = Some("bang"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we set an enum instead of a string here (yes, it will have multiple variants for each spelling)? Bonus: we can make the We would have to add a method that converts it back to a useful string though. But we'd be able to nicely move a ton of code out of here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can reuse There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jyn514 I'd rather not use DefKind because we can have multiple disambiguators, like A cool thing we could do, however, is somehow generate the span for the disambiguator only, and then we don't need to explicitly name it and instead can call it "the function disambiguator here^^". For perf it might be desirable to generate this only when we need to raise an error, i.e. perform a search for it after the fact and adjust the span. Idk. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I ended up describing the disambiguator with an article but still naming it . What do you think?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh and if you know how to switch the order of the note and the suggestion please let me know, I'd rather it was
|
||
link.trim_end_matches("!()") | ||
} else if link.ends_with("()") { | ||
kind = Some(ValueNS); | ||
|
@@ -610,7 +612,7 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { | |
link.trim_start_matches("derive@") | ||
} else if link.ends_with('!') { | ||
kind = Some(MacroNS); | ||
disambiguator = Some("macro"); | ||
disambiguator = Some("bang"); | ||
link.trim_end_matches('!') | ||
} else { | ||
&link[..] | ||
|
@@ -789,6 +791,46 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { | |
} else { | ||
debug!("intra-doc link to {} resolved to {:?}", path_str, res); | ||
|
||
// Disallow e.g. linking to enums with `struct@` | ||
if let Res::Def(kind, id) = res { | ||
debug!("saw kind {:?} with disambiguator {:?}", kind, disambiguator); | ||
// NOTE: this relies on the fact that `''` is never parsed as a disambiguator | ||
// NOTE: this needs to be kept in sync with the disambiguator parsing | ||
match (kind, disambiguator.unwrap_or_default().trim_end_matches("@")) { | ||
| (DefKind::Struct, "struct") | ||
| (DefKind::Enum, "enum") | ||
| (DefKind::Trait, "trait") | ||
| (DefKind::Union, "union") | ||
| (DefKind::Mod, "mod" | "module") | ||
| (DefKind::Const | DefKind::ConstParam | DefKind::AssocConst | DefKind::AnonConst, "const") | ||
| (DefKind::Static, "static") | ||
// NOTE: this allows 'method' to mean both normal functions and associated functions | ||
// This can't cause ambiguity because both are in the same namespace. | ||
| (DefKind::Fn | DefKind::AssocFn, "fn" | "function" | "method") | ||
| (DefKind::Macro(MacroKind::Bang), "bang") | ||
| (DefKind::Macro(MacroKind::Derive), "derive") | ||
// These are namespaces; allow anything in the namespace to match | ||
| (_, "type" | "macro" | "value") | ||
// If no disambiguator given, allow anything | ||
| (_, "") | ||
// All of these are valid, so do nothing | ||
=> {} | ||
(_, disambiguator) => { | ||
// The resolved item did not match the disambiguator; give a better error than 'not found' | ||
let msg = format!("unresolved link to `{}`", path_str); | ||
report_diagnostic(cx, &msg, &item, &dox, link_range, |diag, sp| { | ||
let msg = format!("this item resolved to {} {}, which did not match the disambiguator '{}'", kind.article(), kind.descr(id), disambiguator); | ||
jyn514 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if let Some(sp) = sp { | ||
diag.span_note(sp, &msg); | ||
} else { | ||
diag.note(&msg); | ||
} | ||
}); | ||
continue; | ||
} | ||
} | ||
} | ||
|
||
// item can be non-local e.g. when using #[doc(primitive = "pointer")] | ||
if let Some((src_id, dst_id)) = res | ||
.opt_def_id() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
#![deny(broken_intra_doc_links)] | ||
//~^ NOTE lint level is defined | ||
pub enum S {} | ||
|
||
macro_rules! m { | ||
() => {}; | ||
} | ||
|
||
static s: usize = 0; | ||
const c: usize = 0; | ||
|
||
trait T {} | ||
|
||
/// Link to [struct@S] | ||
//~^ ERROR unresolved link to `S` | ||
//~| NOTE did not match | ||
|
||
/// Link to [mod@S] | ||
//~^ ERROR unresolved link to `S` | ||
//~| NOTE did not match | ||
|
||
/// Link to [union@S] | ||
//~^ ERROR unresolved link to `S` | ||
//~| NOTE did not match | ||
|
||
/// Link to [trait@S] | ||
//~^ ERROR unresolved link to `S` | ||
//~| NOTE did not match | ||
|
||
/// Link to [struct@T] | ||
//~^ ERROR unresolved link to `T` | ||
//~| NOTE did not match | ||
|
||
/// Link to [derive@m] | ||
//~^ ERROR unresolved link to `m` | ||
//~| NOTE did not match | ||
|
||
/// Link to [const@s] | ||
//~^ ERROR unresolved link to `s` | ||
//~| NOTE did not match | ||
|
||
/// Link to [static@c] | ||
//~^ ERROR unresolved link to `c` | ||
//~| NOTE did not match | ||
|
||
/// Link to [fn@c] | ||
//~^ ERROR unresolved link to `c` | ||
//~| NOTE did not match | ||
|
||
/// Link to [c()] | ||
//~^ ERROR unresolved link to `c` | ||
//~| NOTE did not match | ||
pub fn f() {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,127 @@ | ||
error: unresolved link to `S` | ||
--> $DIR/intra-links-disambiguator-mismatch.rs:16:14 | ||
| | ||
LL | /// Link to [struct@S] | ||
| ^^^^^^^^ | ||
| | ||
note: the lint level is defined here | ||
--> $DIR/intra-links-disambiguator-mismatch.rs:1:9 | ||
| | ||
LL | #![deny(broken_intra_doc_links)] | ||
| ^^^^^^^^^^^^^^^^^^^^^^ | ||
note: this item resolved to an enum, which did not match the disambiguator 'struct' | ||
--> $DIR/intra-links-disambiguator-mismatch.rs:16:14 | ||
| | ||
LL | /// Link to [struct@S] | ||
| ^^^^^^^^ | ||
|
||
error: unresolved link to `S` | ||
--> $DIR/intra-links-disambiguator-mismatch.rs:20:14 | ||
| | ||
LL | /// Link to [mod@S] | ||
| ^^^^^ | ||
| | ||
note: this item resolved to an enum, which did not match the disambiguator 'mod' | ||
--> $DIR/intra-links-disambiguator-mismatch.rs:20:14 | ||
| | ||
LL | /// Link to [mod@S] | ||
| ^^^^^ | ||
|
||
error: unresolved link to `S` | ||
--> $DIR/intra-links-disambiguator-mismatch.rs:24:14 | ||
| | ||
LL | /// Link to [union@S] | ||
| ^^^^^^^ | ||
| | ||
note: this item resolved to an enum, which did not match the disambiguator 'union' | ||
--> $DIR/intra-links-disambiguator-mismatch.rs:24:14 | ||
| | ||
LL | /// Link to [union@S] | ||
| ^^^^^^^ | ||
|
||
error: unresolved link to `S` | ||
--> $DIR/intra-links-disambiguator-mismatch.rs:28:14 | ||
| | ||
LL | /// Link to [trait@S] | ||
| ^^^^^^^ | ||
| | ||
note: this item resolved to an enum, which did not match the disambiguator 'trait' | ||
--> $DIR/intra-links-disambiguator-mismatch.rs:28:14 | ||
| | ||
LL | /// Link to [trait@S] | ||
| ^^^^^^^ | ||
|
||
error: unresolved link to `T` | ||
--> $DIR/intra-links-disambiguator-mismatch.rs:32:14 | ||
| | ||
LL | /// Link to [struct@T] | ||
| ^^^^^^^^ | ||
| | ||
note: this item resolved to a trait, which did not match the disambiguator 'struct' | ||
--> $DIR/intra-links-disambiguator-mismatch.rs:32:14 | ||
| | ||
LL | /// Link to [struct@T] | ||
| ^^^^^^^^ | ||
|
||
error: unresolved link to `m` | ||
--> $DIR/intra-links-disambiguator-mismatch.rs:36:14 | ||
| | ||
LL | /// Link to [derive@m] | ||
| ^^^^^^^^ | ||
| | ||
note: this item resolved to a macro, which did not match the disambiguator 'derive' | ||
--> $DIR/intra-links-disambiguator-mismatch.rs:36:14 | ||
| | ||
LL | /// Link to [derive@m] | ||
| ^^^^^^^^ | ||
|
||
error: unresolved link to `s` | ||
--> $DIR/intra-links-disambiguator-mismatch.rs:40:14 | ||
| | ||
LL | /// Link to [const@s] | ||
| ^^^^^^^ | ||
| | ||
note: this item resolved to a static, which did not match the disambiguator 'const' | ||
--> $DIR/intra-links-disambiguator-mismatch.rs:40:14 | ||
| | ||
LL | /// Link to [const@s] | ||
| ^^^^^^^ | ||
|
||
error: unresolved link to `c` | ||
--> $DIR/intra-links-disambiguator-mismatch.rs:44:14 | ||
| | ||
LL | /// Link to [static@c] | ||
| ^^^^^^^^ | ||
| | ||
note: this item resolved to a constant, which did not match the disambiguator 'static' | ||
--> $DIR/intra-links-disambiguator-mismatch.rs:44:14 | ||
| | ||
LL | /// Link to [static@c] | ||
| ^^^^^^^^ | ||
|
||
error: unresolved link to `c` | ||
--> $DIR/intra-links-disambiguator-mismatch.rs:48:14 | ||
| | ||
LL | /// Link to [fn@c] | ||
| ^^^^ | ||
| | ||
note: this item resolved to a constant, which did not match the disambiguator 'fn' | ||
--> $DIR/intra-links-disambiguator-mismatch.rs:48:14 | ||
| | ||
LL | /// Link to [fn@c] | ||
| ^^^^ | ||
|
||
error: unresolved link to `c` | ||
--> $DIR/intra-links-disambiguator-mismatch.rs:52:14 | ||
| | ||
LL | /// Link to [c()] | ||
| ^^^ | ||
| | ||
note: this item resolved to a constant, which did not match the disambiguator 'fn' | ||
--> $DIR/intra-links-disambiguator-mismatch.rs:52:14 | ||
| | ||
LL | /// Link to [c()] | ||
| ^^^ | ||
|
||
error: aborting due to 10 previous errors | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
#![deny(broken_intra_doc_links_)] | ||
/// Link to [Default::default()] | ||
pub fn f() {} |
Uh oh!
There was an error while loading. Please reload this page.