Unify error reporting for intra-doc links #75916

jyn514 · 2020-08-25T20:23:18Z

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 #75836.
r? @euclio
cc @pickfire - this gets rid of 'disambiguator' like you suggested in #75079 (comment).

- 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

src/librustc_hir/def.rs

jyn514 · 2020-08-25T21:03:39Z

@bors r=euclio

bors · 2020-08-25T21:03:40Z

📌 Commit 5ce4ee9 has been approved by euclio

src/librustdoc/passes/collect_intra_doc_links.rs

jyn514 · 2020-08-25T21:08:31Z

@bors r-

#75916 (comment), and I should probably wait for CI before pinging bors 😅

jyn514 · 2020-08-25T21:19:20Z

@bors r+

56d3f39 succeeded and I fixed my own nit 😆

bors · 2020-08-25T21:19:21Z

📌 Commit e233d80 has been approved by jyn514

jyn514 · 2020-08-25T21:19:54Z

err ... that should say r=euclio. Ugh I'm really messing this up.

@bors r-

@bors r=euclio

bors · 2020-08-25T21:19:56Z

📌 Commit e233d80 has been approved by euclio

src/librustc_hir/def.rs

bors · 2020-08-28T05:40:13Z

📌 Commit d3c5817 has been approved by euclio

jyn514 · 2020-08-28T05:40:54Z

Ok yeah, that fixed it. I guess bors doesn't like it when you have both r+ and r- in the same command 😆

bors · 2020-08-28T05:51:25Z

⌛ Testing commit d3c5817 with merge 5b79d88ff45651e078c0d2a75133d0b414a856d8...

pietroalbini · 2020-08-28T08:01:41Z

Ok yeah, that fixed it. I guess bors doesn't like it when you have both r+ and r- in the same command laughing

Both of them should work, even in the same @bors invocation. My guess is that bors removed the approval once new commits were pushed, but forgot to change the labels.

bors · 2020-08-28T10:06:25Z

💥 Test timed out

jyn514 · 2020-08-28T13:43:01Z

Timed out on MacOS after 4 and a half hours: https://dev.azure.com/rust-lang/rust/_build/results?buildId=36267&view=logs&j=1c64c3ff-4c37-56fe-a133-b407354bbfbf&t=d955a4bf-38f0-58ca-c266-c3c17b7d5f88. cc @rust-lang/infra - is that normal?

@bors retry

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

bors · 2020-08-29T03:48:27Z

⌛ Testing commit d3c5817 with merge 2391420ffa0a0c5125f13a9dc13611188fa3cb8c...

bors · 2020-08-29T04:18:15Z

💔 Test failed - checks-actions

jyn514 · 2020-08-29T04:51:22Z

Looks like an error in the cargo test suite. cc @rust-lang/cargo - I think one of your tests has intermittent failures.

 ---- build::close_output stdout ----
thread 'build::close_output' panicked at 'lines differ:
   Compiling foo v0.1.0 (/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/cit/t216/foo)
error: Broken pipe (os error 32)
warning: build failed, waiting for other jobs to finish...
hello stderr!
error: build failed
', src/tools/cargo/tests/testsuite/build.rs:5043:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    build::close_output

@bors retry

bors · 2020-08-29T07:52:58Z

⌛ Testing commit d3c5817 with merge 1dc748f...

bors · 2020-08-29T10:06:58Z

☀️ Test successful - checks-actions, checks-azure
Approved by: euclio
Pushing 1dc748f to master...

@estebank

…tebank Improve suggestions for broken intra-doc links ~~Depends on rust-lang#74489 and should not be merged before that PR.~~ Merged 🎉 ~~Depends on rust-lang#75916 and should not be merged before.~~ Merged Fixes rust-lang#75305. This does a lot of different things 😆. - Add `PerNS::into_iter()` so I didn't have to keep rewriting hacks around it. Also add `PerNS::iter()` for consistency. Let me know if this should be `impl IntoIterator` instead. - Make `ResolutionFailure` an enum instead of a unit variant. This was most of the changes: everywhere that said `ErrorKind::ResolutionFailure` now has to say _why_ the link failed to resolve. - Store the resolution in case of an anchor failure. Previously this was implemented as variants on `AnchorFailure` which was prone to typos and had inconsistent output compared to the rest of the diagnostics. - Turn some `Err`ors into unwrap() or panic()s, because they're rustdoc bugs and not user error. These have comments as to why they're bugs (in particular this would have caught rust-lang#76073 as a bug a while ago). - If an item is not in scope at all, say the first segment in the path that failed to resolve - If an item exists but not in the current namespaces, say that and suggests linking to that namespace. - If there is a partial resolution for an item (part of the segments resolved, but not all of them), say the partial resolution and why the following segment didn't resolve. - Add the `DefId` of associated items to `kind_side_channel` so it can be used for diagnostics (tl;dr of the hack: the rest of rustdoc expects the id of the item, but for diagnostics we need the associated item). - No longer suggests escaping the brackets for every link that failed to resolve; this was pretty obnoxious. Now it only suggests `\[ \]` if no segment resolved and there is no `::` in the link. - Add `Suggestion`, which says _what_ to prefix the link with, not just 'prefix with the item kind'. Places where this is currently buggy: <details><summary>All outdated</summary> ~~1. When the link has the wrong namespace:~~ Now fixed. <details> ```rust /// [type@S::h] impl S { pub fn h() {} } /// [type@T::g] pub trait T { fn g() {} } ``` ``` error: unresolved link to `T::g` --> /home/joshua/rustc/src/test/rustdoc-ui/intra-link-errors.rs:53:6 | 53 | /// [type@T::g] | ^^^^^^^^^ | = note: this link partially resolves to the trait `T`, = note: `T` has no field, variant, or associated item named `g` error: unresolved link to `S::h` --> /home/joshua/rustc/src/test/rustdoc-ui/intra-link-errors.rs:48:6 | 48 | /// [type@S::h] | ^^^^^^^^^ | = note: this link partially resolves to the struct `S`, = note: `S` has no field, variant, or associated item named `h` ``` Instead it should suggest changing the disambiguator, the way it currently does for macros: ``` error: unresolved link to `S` --> /home/joshua/rustc/src/test/rustdoc-ui/intra-link-errors.rs:38:6 | 38 | /// [S!] | ^^ help: to link to the unit struct, use its disambiguator: `value@S` | = note: this link resolves to the unit struct `S`, which is not in the macro namespace ``` </details> 2. ~~Associated items for values. It says that the value isn't in scope; instead it should say that values can't have associated items.~~ Fixed. <details> ``` error: unresolved link to `f::A` --> /home/joshua/rustc/src/test/rustdoc-ui/intra-link-errors.rs:14:6 | 14 | /// [f::A] | ^^^^ | = note: no item named `f` is in scope = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]` ``` This is _mostly_ fixed, it now says ```rust warning: unresolved link to `f::A` --> /home/joshua/test-rustdoc/f.rs:1:6 | 1 | /// [f::A] | ^^^^ | = note: this link partially resolves to the function `f` = note: `f` is a function, not a module ``` 'function, not a module' seems awfully terse when what I actually mean is '`::` isn't allowed here', though. </details> It looks a lot nicer now, it says ``` error: unresolved link to `f::A` --> /home/joshua/rustc/src/test/rustdoc-ui/intra-link-errors.rs:13:6 | 13 | /// [f::A] | ^^^^ | = note: `f` is a function, not a module or type, and cannot have associated items ``` 3. ~~I'm also not very happy with the second note for this error:~~ <details> ``` error: unresolved link to `S::A` --> /home/joshua/rustc/src/test/rustdoc-ui/intra-link-errors.rs:19:6 | 19 | /// [S::A] | ^^^^ | = note: this link partially resolves to the struct `S`, = note: `S` has no field, variant, or associated item named `A` ``` but I'm not sure how better to word it. I ended up going with 'no `A` in `S`' to match `rustc_resolve` but that seems terse as well. </details> This now says ``` error: unresolved link to `S::A` --> /home/joshua/rustc/src/test/rustdoc-ui/intra-link-errors.rs:17:6 | 17 | /// [S::A] | ^^^^ | = note: the struct `S` has no field or associated item named `A` ``` which I think looks pretty good :) 4. This is minor, but it would be nice to say that `path` wasn't found instead of the full thing: ``` error: unresolved link to `path::to::nonexistent::module` --> /home/joshua/rustc/src/test/rustdoc-ui/intra-link-errors.rs:8:6 | 8 | /// [path::to::nonexistent::module] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ``` It will now look at most 3 paths up (so it reports `path::to` as not in scope), but it doesn't work with arbitrarily many paths. </details> ~~I recommend only reviewing the last few commits - the first 7 are all from rust-lang#74489.~~ Rebased so that only the relevant commits are shown. Let me know if I should squash the history some more. r? @estebank

…bank Improve suggestions for broken intra-doc links ~~Depends on rust-lang#74489 and should not be merged before that PR.~~ Merged 🎉 ~~Depends on rust-lang#75916 and should not be merged before.~~ Merged Fixes rust-lang#75305. This does a lot of different things 😆. - Add `PerNS::into_iter()` so I didn't have to keep rewriting hacks around it. Also add `PerNS::iter()` for consistency. Let me know if this should be `impl IntoIterator` instead. - Make `ResolutionFailure` an enum instead of a unit variant. This was most of the changes: everywhere that said `ErrorKind::ResolutionFailure` now has to say _why_ the link failed to resolve. - Store the resolution in case of an anchor failure. Previously this was implemented as variants on `AnchorFailure` which was prone to typos and had inconsistent output compared to the rest of the diagnostics. - Turn some `Err`ors into unwrap() or panic()s, because they're rustdoc bugs and not user error. These have comments as to why they're bugs (in particular this would have caught rust-lang#76073 as a bug a while ago). - If an item is not in scope at all, say the first segment in the path that failed to resolve - If an item exists but not in the current namespaces, say that and suggests linking to that namespace. - If there is a partial resolution for an item (part of the segments resolved, but not all of them), say the partial resolution and why the following segment didn't resolve. - Add the `DefId` of associated items to `kind_side_channel` so it can be used for diagnostics (tl;dr of the hack: the rest of rustdoc expects the id of the item, but for diagnostics we need the associated item). - No longer suggests escaping the brackets for every link that failed to resolve; this was pretty obnoxious. Now it only suggests `\[ \]` if no segment resolved and there is no `::` in the link. - Add `Suggestion`, which says _what_ to prefix the link with, not just 'prefix with the item kind'. Places where this is currently buggy: <details><summary>All outdated</summary> ~~1. When the link has the wrong namespace:~~ Now fixed. <details> ```rust /// [type@S::h] impl S { pub fn h() {} } /// [type@T::g] pub trait T { fn g() {} } ``` ``` error: unresolved link to `T::g` --> /home/joshua/rustc/src/test/rustdoc-ui/intra-link-errors.rs:53:6 | 53 | /// [type@T::g] | ^^^^^^^^^ | = note: this link partially resolves to the trait `T`, = note: `T` has no field, variant, or associated item named `g` error: unresolved link to `S::h` --> /home/joshua/rustc/src/test/rustdoc-ui/intra-link-errors.rs:48:6 | 48 | /// [type@S::h] | ^^^^^^^^^ | = note: this link partially resolves to the struct `S`, = note: `S` has no field, variant, or associated item named `h` ``` Instead it should suggest changing the disambiguator, the way it currently does for macros: ``` error: unresolved link to `S` --> /home/joshua/rustc/src/test/rustdoc-ui/intra-link-errors.rs:38:6 | 38 | /// [S!] | ^^ help: to link to the unit struct, use its disambiguator: `value@S` | = note: this link resolves to the unit struct `S`, which is not in the macro namespace ``` </details> 2. ~~Associated items for values. It says that the value isn't in scope; instead it should say that values can't have associated items.~~ Fixed. <details> ``` error: unresolved link to `f::A` --> /home/joshua/rustc/src/test/rustdoc-ui/intra-link-errors.rs:14:6 | 14 | /// [f::A] | ^^^^ | = note: no item named `f` is in scope = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]` ``` This is _mostly_ fixed, it now says ```rust warning: unresolved link to `f::A` --> /home/joshua/test-rustdoc/f.rs:1:6 | 1 | /// [f::A] | ^^^^ | = note: this link partially resolves to the function `f` = note: `f` is a function, not a module ``` 'function, not a module' seems awfully terse when what I actually mean is '`::` isn't allowed here', though. </details> It looks a lot nicer now, it says ``` error: unresolved link to `f::A` --> /home/joshua/rustc/src/test/rustdoc-ui/intra-link-errors.rs:13:6 | 13 | /// [f::A] | ^^^^ | = note: `f` is a function, not a module or type, and cannot have associated items ``` 3. ~~I'm also not very happy with the second note for this error:~~ <details> ``` error: unresolved link to `S::A` --> /home/joshua/rustc/src/test/rustdoc-ui/intra-link-errors.rs:19:6 | 19 | /// [S::A] | ^^^^ | = note: this link partially resolves to the struct `S`, = note: `S` has no field, variant, or associated item named `A` ``` but I'm not sure how better to word it. I ended up going with 'no `A` in `S`' to match `rustc_resolve` but that seems terse as well. </details> This now says ``` error: unresolved link to `S::A` --> /home/joshua/rustc/src/test/rustdoc-ui/intra-link-errors.rs:17:6 | 17 | /// [S::A] | ^^^^ | = note: the struct `S` has no field or associated item named `A` ``` which I think looks pretty good :) 4. This is minor, but it would be nice to say that `path` wasn't found instead of the full thing: ``` error: unresolved link to `path::to::nonexistent::module` --> /home/joshua/rustc/src/test/rustdoc-ui/intra-link-errors.rs:8:6 | 8 | /// [path::to::nonexistent::module] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ``` It will now look at most 3 paths up (so it reports `path::to` as not in scope), but it doesn't work with arbitrarily many paths. </details> ~~I recommend only reviewing the last few commits - the first 7 are all from rust-lang#74489.~~ Rebased so that only the relevant commits are shown. Let me know if I should squash the history some more. r? `@estebank`

Add Res::ns() instead of matches_ns()

18e7a1b

jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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 25, 2020

Unify error reporting for intra-doc links

56d3f39

- 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

jyn514 force-pushed the unify-error-reporting branch from 62c00f4 to 56d3f39 Compare August 25, 2020 20:37

petrochenkov reviewed Aug 25, 2020

View reviewed changes

src/librustc_hir/def.rs Show resolved Hide resolved

jyn514 added 2 commits August 25, 2020 16:41

Remove unused ns parameter

05a040f

Add back Res::matches_ns, implemented in terms of ns

5ce4ee9

euclio approved these changes Aug 25, 2020

View reviewed changes

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 25, 2020

jyn514 commented Aug 25, 2020

View reviewed changes

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

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 25, 2020

Remove outdated FIXME

e233d80

jyn514 mentioned this pull request Aug 25, 2020

Improve suggestions for broken intra-doc links #75756

Merged

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 25, 2020

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 25, 2020

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 25, 2020

pickfire reviewed Aug 26, 2020

View reviewed changes

src/librustc_hir/def.rs Outdated Show resolved Hide resolved

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

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 28, 2020

jyn514 mentioned this pull request Aug 28, 2020

Run cancel-outdated-builds after fully setting up the env #76018

Merged

Dylan-DPC-zz mentioned this pull request Aug 29, 2020

Rollup of 16 pull requests #76045

Closed

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

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 29, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 29, 2020

bors merged commit 1dc748f into rust-lang:master Aug 29, 2020

jyn514 deleted the unify-error-reporting branch August 31, 2020 16:55

cuviper added this to the 1.48.0 milestone Nov 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify error reporting for intra-doc links #75916

Unify error reporting for intra-doc links #75916

jyn514 commented Aug 25, 2020

jyn514 commented Aug 25, 2020

bors commented Aug 25, 2020

jyn514 commented Aug 25, 2020

jyn514 commented Aug 25, 2020

bors commented Aug 25, 2020

jyn514 commented Aug 25, 2020

bors commented Aug 25, 2020

bors commented Aug 28, 2020

jyn514 commented Aug 28, 2020

bors commented Aug 28, 2020

pietroalbini commented Aug 28, 2020

bors commented Aug 28, 2020

jyn514 commented Aug 28, 2020

bors commented Aug 29, 2020

bors commented Aug 29, 2020

jyn514 commented Aug 29, 2020

bors commented Aug 29, 2020

bors commented Aug 29, 2020

Unify error reporting for intra-doc links #75916

Unify error reporting for intra-doc links #75916

Conversation

jyn514 commented Aug 25, 2020

jyn514 commented Aug 25, 2020

bors commented Aug 25, 2020

jyn514 commented Aug 25, 2020

jyn514 commented Aug 25, 2020

bors commented Aug 25, 2020

jyn514 commented Aug 25, 2020

bors commented Aug 25, 2020

bors commented Aug 28, 2020

jyn514 commented Aug 28, 2020

bors commented Aug 28, 2020

pietroalbini commented Aug 28, 2020

bors commented Aug 28, 2020

jyn514 commented Aug 28, 2020

bors commented Aug 29, 2020

bors commented Aug 29, 2020

jyn514 commented Aug 29, 2020

bors commented Aug 29, 2020

bors commented Aug 29, 2020