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

Disallow linking to items with a mismatched disambiguator #75079

Merged
merged 13 commits into from
Aug 7, 2020
3 changes: 3 additions & 0 deletions src/librustdoc/clean/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -607,6 +607,9 @@ pub fn register_res(cx: &DocContext<'_>, res: Res) -> DefId {
Res::Def(DefKind::TyAlias, i) => (i, TypeKind::Typedef),
Res::Def(DefKind::Enum, i) => (i, TypeKind::Enum),
Res::Def(DefKind::Trait, i) => (i, TypeKind::Trait),
Res::Def(DefKind::AssocTy | DefKind::AssocFn | DefKind::AssocConst, i) => {
(cx.tcx.parent(i).unwrap(), TypeKind::Trait)
jyn514 marked this conversation as resolved.
Show resolved Hide resolved
}
Res::Def(DefKind::Struct, i) => (i, TypeKind::Struct),
Res::Def(DefKind::Union, i) => (i, TypeKind::Union),
Res::Def(DefKind::Mod, i) => (i, TypeKind::Module),
Expand Down
48 changes: 45 additions & 3 deletions src/librustdoc/passes/collect_intra_doc_links.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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()
Expand All @@ -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");
Copy link
Member

Choose a reason for hiding this comment

The 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 kind derivation a method on it, so we just call disambiguator.kind(), and we can do the same thing with the DefKind, and we can also do the thing with the prefix-matching (something like fn strip_prefix(&mut String) -> Option<Self>)

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can reuse DefKind here. I agree that would be nicer than using strings everywhere, it already has article() and descr().

Copy link
Member

@Manishearth Manishearth Aug 3, 2020

Choose a reason for hiding this comment

The 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 () vs fn vs function and we wanna be able to name them.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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?

error: incompatible link kind for `c`
  --> $DIR/intra-links-disambiguator-mismatch.rs:59:14
   |
LL | /// Link to [c()]
   |              ^^^ help: to link to the constant, use its disambiguator: `const@c`
   |
   = note: this link resolved to a constant, which is not a function

Copy link
Member Author

@jyn514 jyn514 Aug 5, 2020

Choose a reason for hiding this comment

The 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

error: incompatible link kind for `c`
  --> $DIR/intra-links-disambiguator-mismatch.rs:59:14
   |
LL | /// Link to [c()]
   |              ^^^ note: this link resolved to a constant, which is not a function
   |
   = help: to link to the constant, use its disambiguator: `const@c`

link.trim_end_matches("!()")
} else if link.ends_with("()") {
kind = Some(ValueNS);
Expand All @@ -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[..]
Expand Down Expand Up @@ -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 link 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()
Expand Down
53 changes: 53 additions & 0 deletions src/test/rustdoc-ui/intra-links-disambiguator-mismatch.rs
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() {}
127 changes: 127 additions & 0 deletions src/test/rustdoc-ui/intra-links-disambiguator-mismatch.stderr
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:14: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 link resolved to an enum, which did not match the disambiguator 'struct'
--> $DIR/intra-links-disambiguator-mismatch.rs:14:14
|
LL | /// Link to [struct@S]
| ^^^^^^^^

error: unresolved link to `S`
--> $DIR/intra-links-disambiguator-mismatch.rs:18:14
|
LL | /// Link to [mod@S]
| ^^^^^
|
note: this link resolved to an enum, which did not match the disambiguator 'mod'
--> $DIR/intra-links-disambiguator-mismatch.rs:18:14
|
LL | /// Link to [mod@S]
| ^^^^^

error: unresolved link to `S`
--> $DIR/intra-links-disambiguator-mismatch.rs:22:14
|
LL | /// Link to [union@S]
| ^^^^^^^
|
note: this link resolved to an enum, which did not match the disambiguator 'union'
--> $DIR/intra-links-disambiguator-mismatch.rs:22:14
|
LL | /// Link to [union@S]
| ^^^^^^^

error: unresolved link to `S`
--> $DIR/intra-links-disambiguator-mismatch.rs:26:14
|
LL | /// Link to [trait@S]
| ^^^^^^^
|
note: this link resolved to an enum, which did not match the disambiguator 'trait'
--> $DIR/intra-links-disambiguator-mismatch.rs:26:14
|
LL | /// Link to [trait@S]
| ^^^^^^^

error: unresolved link to `T`
--> $DIR/intra-links-disambiguator-mismatch.rs:30:14
|
LL | /// Link to [struct@T]
| ^^^^^^^^
|
note: this link resolved to a trait, which did not match the disambiguator 'struct'
--> $DIR/intra-links-disambiguator-mismatch.rs:30:14
|
LL | /// Link to [struct@T]
| ^^^^^^^^

error: unresolved link to `m`
--> $DIR/intra-links-disambiguator-mismatch.rs:34:14
|
LL | /// Link to [derive@m]
| ^^^^^^^^
|
note: this link resolved to a macro, which did not match the disambiguator 'derive'
--> $DIR/intra-links-disambiguator-mismatch.rs:34:14
|
LL | /// Link to [derive@m]
| ^^^^^^^^

error: unresolved link to `s`
--> $DIR/intra-links-disambiguator-mismatch.rs:38:14
|
LL | /// Link to [const@s]
| ^^^^^^^
|
note: this link resolved to a static, which did not match the disambiguator 'const'
--> $DIR/intra-links-disambiguator-mismatch.rs:38:14
|
LL | /// Link to [const@s]
| ^^^^^^^

error: unresolved link to `c`
--> $DIR/intra-links-disambiguator-mismatch.rs:42:14
|
LL | /// Link to [static@c]
| ^^^^^^^^
|
note: this link resolved to a constant, which did not match the disambiguator 'static'
--> $DIR/intra-links-disambiguator-mismatch.rs:42:14
|
LL | /// Link to [static@c]
| ^^^^^^^^

error: unresolved link to `c`
--> $DIR/intra-links-disambiguator-mismatch.rs:46:14
|
LL | /// Link to [fn@c]
| ^^^^
|
note: this link resolved to a constant, which did not match the disambiguator 'fn'
--> $DIR/intra-links-disambiguator-mismatch.rs:46:14
|
LL | /// Link to [fn@c]
| ^^^^

error: unresolved link to `c`
--> $DIR/intra-links-disambiguator-mismatch.rs:50:14
|
LL | /// Link to [c()]
| ^^^
|
note: this link resolved to a constant, which did not match the disambiguator 'fn'
--> $DIR/intra-links-disambiguator-mismatch.rs:50:14
|
LL | /// Link to [c()]
| ^^^

error: aborting due to 10 previous errors

3 changes: 3 additions & 0 deletions src/test/rustdoc/intra-link-trait-item.rs
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() {}