Disallow linking to items with a mismatched disambiguator #75079

jyn514 · 2020-08-03T00:46:42Z

rust-highfive · 2020-08-03T00:46:45Z

Some changes occurred in intra-doc-links.

This caused the following false positive: ``` warning: unresolved link to `Default::default` --> /home/joshua/rustc2/default.rs:1:14 | 1 | /// Link to [Default::default()] | ^^^^^^^^^^^^^^^^^^ | = note: `#[warn(broken_intra_doc_links)]` on by default note: this item resolved to a trait, which did not match the disambiguator 'fn' --> /home/joshua/rustc2/default.rs:1:14 | 1 | /// Link to [Default::default()] | ^^^^^^^^^^^^^^^^^^ ```

Now that we're returning the `Res` of the associated item, not the trait itself, it got confused.

src/librustdoc/passes/collect_intra_doc_links.rs

src/librustdoc/clean/utils.rs

Manishearth · 2020-08-03T06:46:45Z

src/librustdoc/passes/collect_intra_doc_links.rs

@@ -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");


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.

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

@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.

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

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`

src/librustdoc/passes/collect_intra_doc_links.rs

Clearly it has been resolved, because we say on the next line what it resolved to.

src/librustdoc/passes/collect_intra_doc_links.rs

Manishearth

I was going to suggest that Disambiguator have different variants for mod vs module etc but it's fine.

Manishearth · 2020-08-05T20:40:55Z

r=me when ready

jyn514 · 2020-08-06T00:59:54Z

Ugh, this is failing to document libstd because the res for an associated function is the struct, not the method itself.

error: incompatible link kind for `Initializer::nop`
   --> library/std/src/io/mod.rs:595:33
    |
595 |     /// memory, it should call [`Initializer::nop()`]. See the documentation for
    |                                 ^^^^^^^^^^^^^^^^^^^^ help: to link to the struct, use its disambiguator: `struct@Initializer::nop`

Not sure how to fix this ... I think later parts of rustdoc do need the actual struct to know which page to link to.

jyn514 · 2020-08-06T01:13:28Z

Oh wait, this is the issue I already fixed in https://github.com/rust-lang/rust/pull/75079/files#diff-2b73a40b29f966df2a77a3bbaf34a9f1 😆 should be pretty easy to rewrite it to an associated item.

See comments in the diff; this is such a hack. The reason this can't be done properly in `register_res` is because there's no way to get back the parent type: calling `tcx.parent(assoc_item)` gets you the _impl_, not the type. You can call `tcx.impl_trait_ref(impl_).self_ty()`, but there's no way to go from that to a DefId without unwrapping.

jyn514 · 2020-08-06T22:17:45Z

I had to hack around associated items in the very last commit, you might want to look again to make sure that's fine. Otherwise this should hopefully be good to go.

jyn514 · 2020-08-06T22:23:10Z

Oh hold on, I have a bad feeling https://github.com/rust-lang/rust/pull/75079/files#diff-2bcad7b16b56ef2ebb92f8e60ae33773R424 is broken. Let me write some more tests.

jyn514 · 2020-08-06T22:34:09Z

Suprisingly it worked! I added a test anyway.

Manishearth · 2020-08-07T02:55:18Z

Hack looks okay to me, but please add a docstr to the definition of the side channel field. r=me otherwise

jyn514 · 2020-08-07T03:21:18Z

@bors r=Manishearth

bors · 2020-08-07T03:21:19Z

📌 Commit 9914f73 has been approved by Manishearth

@ghost

…arth Rollup of 4 pull requests Successful merges: - rust-lang#74774 (adds [*mut|*const] ptr::set_ptr_value) - rust-lang#75079 (Disallow linking to items with a mismatched disambiguator) - rust-lang#75203 (Make `IntoIterator` lifetime bounds of `&BTreeMap` match with `&HashMap` ) - rust-lang#75227 (Fix ICE when using asm! on an unsupported architecture) Failed merges: r? @ghost

@euclio

Unify error reporting for intra-doc links - Give a suggestion even if there is no span available - Give a more accurate description of the change than 'use the disambiguator' - Write much less code Closes rust-lang#75836. r? @euclio cc @pickfire - this gets rid of 'disambiguator' like you suggested in rust-lang#75079 (comment).

@euclio

Unify error reporting for intra-doc links - Give a suggestion even if there is no span available - Give a more accurate description of the change than 'use the disambiguator' - Write much less code Closes rust-lang#75836. r? @euclio cc @pickfire - this gets rid of 'disambiguator' like you suggested in rust-lang#75079 (comment).

jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name labels Aug 3, 2020

rust-highfive assigned Manishearth Aug 3, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 3, 2020

jyn514 mentioned this pull request Aug 3, 2020

Don't ping file reviewers if they are the PR author rust-lang/highfive#281

Merged

jyn514 force-pushed the disambiguator branch from 17a397d to e910fd4 Compare August 3, 2020 02:24

Disallow linking to items with a mismatched disambiguator

8e0e925

jyn514 force-pushed the disambiguator branch from f8129d8 to 41c732f Compare August 3, 2020 02:39

jyn514 force-pushed the disambiguator branch from 41c732f to 519c854 Compare August 3, 2020 02:49

Keep the previous behavior of register_res

743f932

Now that we're returning the `Res` of the associated item, not the trait itself, it got confused.

jyn514 force-pushed the disambiguator branch from eee1d6d to 743f932 Compare August 3, 2020 03:49

pickfire reviewed Aug 3, 2020

View reviewed changes

src/librustdoc/passes/collect_intra_doc_links.rs Outdated Show resolved Hide resolved

Manishearth reviewed Aug 3, 2020

View reviewed changes

src/librustdoc/clean/utils.rs Show resolved Hide resolved

Manishearth reviewed Aug 3, 2020

View reviewed changes

item -> link

99354f5

pickfire reviewed Aug 3, 2020

View reviewed changes

src/librustdoc/passes/collect_intra_doc_links.rs Outdated Show resolved Hide resolved

jyn514 added 2 commits August 5, 2020 00:20

Give a much better error message if the struct failed to resolve

444f5a0

Unresolved link -> incompatible link kind

fc273a0

Clearly it has been resolved, because we say on the next line what it resolved to.

jyn514 force-pushed the disambiguator branch from 2c976ad to fc273a0 Compare August 5, 2020 04:20

Manishearth reviewed Aug 5, 2020

View reviewed changes

src/librustdoc/passes/collect_intra_doc_links.rs Outdated Show resolved Hide resolved

Manishearth approved these changes Aug 5, 2020

View reviewed changes

Suggest f() for functions and add a test case

2dad90d

jyn514 added 2 commits August 6, 2020 17:10

Still print help even if there's no span

f05e9da

Improve tests

ef54cde

Fix outdated code

d240490

jyn514 force-pushed the disambiguator branch from 38cd359 to d240490 Compare August 6, 2020 22:53

Remove dead code

17263bc

Add doc-comment for kind_side_channel

9914f73

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 7, 2020

Manishearth mentioned this pull request Aug 7, 2020

Rollup of 4 pull requests #75244

Merged

bors merged commit 5b1ed09 into rust-lang:master Aug 7, 2020

jyn514 deleted the disambiguator branch August 7, 2020 11:44

This was referenced Aug 8, 2020

Stabilize intra-doc links #74430

Merged

Move to doc links inside the prelude #75368

Merged

cuviper added this to the 1.47.0 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow linking to items with a mismatched disambiguator #75079

Disallow linking to items with a mismatched disambiguator #75079

jyn514 commented Aug 3, 2020

rust-highfive commented Aug 3, 2020

Manishearth Aug 3, 2020

jyn514 Aug 3, 2020

Manishearth Aug 3, 2020 •

edited

Loading

jyn514 Aug 5, 2020

jyn514 Aug 5, 2020 •

edited

Loading

Manishearth left a comment

Manishearth commented Aug 5, 2020

jyn514 commented Aug 6, 2020 •

edited

Loading

jyn514 commented Aug 6, 2020

jyn514 commented Aug 6, 2020

jyn514 commented Aug 6, 2020

jyn514 commented Aug 6, 2020

Manishearth commented Aug 7, 2020

jyn514 commented Aug 7, 2020

bors commented Aug 7, 2020

Disallow linking to items with a mismatched disambiguator #75079

Disallow linking to items with a mismatched disambiguator #75079

Conversation

jyn514 commented Aug 3, 2020

rust-highfive commented Aug 3, 2020

Manishearth Aug 3, 2020

Choose a reason for hiding this comment

jyn514 Aug 3, 2020

Choose a reason for hiding this comment

Manishearth Aug 3, 2020 • edited Loading

Choose a reason for hiding this comment

jyn514 Aug 5, 2020

Choose a reason for hiding this comment

jyn514 Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

Manishearth left a comment

Choose a reason for hiding this comment

Manishearth commented Aug 5, 2020

jyn514 commented Aug 6, 2020 • edited Loading

jyn514 commented Aug 6, 2020

jyn514 commented Aug 6, 2020

jyn514 commented Aug 6, 2020

jyn514 commented Aug 6, 2020

Manishearth commented Aug 7, 2020

jyn514 commented Aug 7, 2020

bors commented Aug 7, 2020

Manishearth Aug 3, 2020 •

edited

Loading

jyn514 Aug 5, 2020 •

edited

Loading

jyn514 commented Aug 6, 2020 •

edited

Loading