-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Improve suggestions for broken intra-doc links #75756
Conversation
This comment has been minimized.
This comment has been minimized.
8c40d00
to
1f28ca0
Compare
e040d3b
to
a945a3a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2a19ba4
to
1c257d5
Compare
@estebank This is ready for review. |
This comment has been minimized.
This comment has been minimized.
1c257d5
to
949ea53
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left myself some comments 😆
This is both more specific and easier to read.
Previously, this called `check_full_res` for values and types, but not macros. This would result in not showing when there was a partial resolution for a parent of the item. This now calls `check_full_res`. Additionally, it checks if there was a disambiguator, and if so, says that specific kind wasn't found instead of saying generically 'associated item'.
Moves this detection into `resolution_failure` to avoid doing unnecessary work and make the control flow a little easier to work with.
01fb699
to
cd72d90
Compare
ping @estebank, it's been a week or so. I know this is a bit of a giant PR - would it be easier to only review the diagnostics themselves and I can get a rustdoc member to review the logic changes? |
Sorry, reviewing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When it comes to the diagnostic output, the main change I'd make is turn the new note
s into the main span_label. Ideally, we would create a new span for cases like [foo::bar]
to point only at the part of the path that is problematic, be it foo
or bar
, but that can be addressed later.
This decreases the size of the `Result`s being returned, improving performance in the common case.
This puts the error message closer to the link, making it easier to see what went wrong.
'not in scope' -> 'not in `module`'
bc25b52
to
5ea3eaf
Compare
@bors r+ |
📌 Commit 5ea3eaf has been approved by |
…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
☀️ Test successful - checks-actions, checks-azure |
Thanks so much for the review! |
Depends on #74489 and should not be merged before that PR.Merged 🎉Depends on #75916 and should not be merged before.MergedFixes #75305.
This does a lot of different things 😆.
PerNS::into_iter()
so I didn't have to keep rewriting hacks around it. Also addPerNS::iter()
for consistency. Let me know if this should beimpl IntoIterator
instead.ResolutionFailure
an enum instead of a unit variant. This was most of the changes: everywhere that saidErrorKind::ResolutionFailure
now has to say why the link failed to resolve.AnchorFailure
which was prone to typos and had inconsistent output compared to the rest of the diagnostics.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 Intra-doc links are unresolved onpub use _ as _
items #76073 as a bug a while ago).DefId
of associated items tokind_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).\[ \]
if no segment resolved and there is no::
in the link.Suggestion
, which says what to prefix the link with, not just 'prefix with the item kind'.Places where this is currently buggy:
All outdated
1. When the link has the wrong namespace:Now fixed.Instead it should suggest changing the disambiguator, the way it currently does for macros:
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.This is mostly fixed, it now says
'function, not a module' seems awfully terse when what I actually mean is '
::
isn't allowed here', though.It looks a lot nicer now, it says
I'm also not very happy with the second note for this error:but I'm not sure how better to word it.
I ended up going with 'no
A
inS
' to matchrustc_resolve
but that seems terse as well.This now says
which I think looks pretty good :)
path
wasn't found instead of the full thing: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.I recommend only reviewing the last few commits - the first 7 are all from #74489.Rebased so that only the relevant commits are shown. Let me know if I should squash the history some more.r? @estebank