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

Improve diagnostics when intra-doc-resolution fails #75305

Closed
jyn514 opened this issue Aug 8, 2020 · 0 comments · Fixed by #75756
Closed

Improve diagnostics when intra-doc-resolution fails #75305

jyn514 opened this issue Aug 8, 2020 · 0 comments · Fixed by #75756
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name C-enhancement Category: An issue proposing an enhancement or a PR with one. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@jyn514
Copy link
Member

jyn514 commented Aug 8, 2020

Similar to but not the same as #74207. Right now rustdoc reports the same error for all the following cases:

/// [<invalid syntax>]
/// [path::to::nonexistent::module]
/// [f::A]
/// [S::A]
/// [S::fmt]
pub fn f() {}
#[derive(Debug)]
pub struct S;
warning: unresolved link to `path::to::nonexistent::module`
 --> /dev/stdin:2:6
  |
2 | /// [path::to::nonexistent::module]
  |      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unresolved link
  |
  = note: `#[warn(broken_intra_doc_links)]` on by default
  = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`

However we can give much more helpful errors:

1 | /// [<invalid syntax>]
  |     ^^^^^^^^^^^^^^^^^^ not valid syntax for an intra-doc link
2 | /// [path::to::nonexistent::module]
  |      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ use of undeclared type or module `path` 
3 | /// [f::A]
  |     ^^^^^^ functions cannot have associated items
4 | /// [S::A]
  |     ^^^^^^ `S` does not have an associated item `A` in scope. help: try importing a trait that defines that item
5 | /// [S::fmt]
  |     ^^^^^^ `S` does not have an associated item `fmt` in scope. help: S implements `std::fmt::Debug`, try importing it.

Most of these do not require special help from rustc_resolve (that would just let rustdoc duplicate less code).

@jyn514 jyn514 added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-diagnostics Area: Messages for errors, warnings, and lints A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Aug 8, 2020
@jyn514 jyn514 self-assigned this Aug 20, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 12, 2020
…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
@bors bors closed this as completed in 0f5c769 Sep 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name C-enhancement Category: An issue proposing an enhancement or a PR with one. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant