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

Add item suggestions for broken intra doc links #74207

Open
pickfire opened this issue Jul 10, 2020 · 9 comments
Open

Add item suggestions for broken intra doc links #74207

pickfire opened this issue Jul 10, 2020 · 9 comments
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 A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-enhancement Category: An issue proposing an enhancement or a PR with one. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@pickfire
Copy link
Contributor

pickfire commented Jul 10, 2020

The diagnostics for intra-doc links could be improve to match the diagnostics available in rust.

Current error message

warning: `[extend]` cannot be resolved, ignoring it.
   --> src/vec.rs:704:45
    |
704 |     /// Note that this function is same as [`extend`] except that it is specialized to work with
    |                                             ^^^^^^^^ cannot be resolved, ignoring
    |
    = note: `#[warn(intra_doc_link_resolution_failure)]` on by default
    = help: to escape `[` and `]` characters, just add '\' before them like `\[` or `\]`

The diagnostics should suggest Self::extend(). Note that it should also handle path ambiguity unlike the current diagnostics with struct@, enum@ and others in the original RFC.

Original RFC: https://github.com/rust-lang/rfcs/blob/master/text/1946-intra-rustdoc-links.md
Tracking issue for intra-doc links: #43466

CC @QuietMisdreavus @Manishearth @jyn514

@jyn514
Copy link
Member

jyn514 commented Jul 10, 2020

FYI QuietMisdreavus is no longer an active member of the rustdoc team, so let's avoid pinging her unless we need something specific only she knows.

@jyn514
Copy link
Member

jyn514 commented Jul 10, 2020

@rustbot modify labels: T-rustdoc A-intra-doc-links C-enhancement

@rustbot rustbot added 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jul 10, 2020
@jyn514
Copy link
Member

jyn514 commented Jul 10, 2020

This is non-trivial because we currently don't have any of the information that rustc_resolve has - we only know whether the type resolved or not. See resolve_str_path_error. We'd need to get the ResolutionError from resolve_ast_path, which has a suggestion field. The comment on resolve_str_path_error seems to indicate this might run into trouble with lifetimes.

@jyn514
Copy link
Member

jyn514 commented Jul 10, 2020

Additionally, most of the time we don't know the namespace of the item. So we don't know if we should use type suggestions, macro suggestions, or value suggestions. See

I guess we could try suggesting everything? Better too much information than too little.

@jyn514
Copy link
Member

jyn514 commented Jul 10, 2020

The comment on resolve_str_path_error seems to indicate this might run into trouble with lifetimes.

Oh! This is only an issue because ResolutionError can contain a variant that's a borrow (&'a). But the only thing rustdoc cares about is the FailedToResolve variant, which is not a borrow. So we can return that suggestion directly without having to worry about lifetimes.

@Manishearth Manishearth changed the title Diagnostic for intra-doc links Add item suggestions for broken intra doc links Jul 10, 2020
@Manishearth
Copy link
Member

An easy thing we could do is apply this suggestion for the "link to a method on the current Self type" case only, as a start. That's reasonably easier.

@pickfire
Copy link
Contributor Author

@jyn514 Ah, sorry about pinging her, I didn't know she wasn't an active member. As well, can you please help to add E-hard label for this?

@jyn514
Copy link
Member

jyn514 commented Jul 10, 2020

I don't think this is actually an E-hard, I'm working on it now and I think it's doable in a day or so.

@jyn514
Copy link
Member

jyn514 commented Jul 10, 2020

Ugh this is not as simple as just returning the Suggestion, all the useful suggestions happen in https://doc.rust-lang.org/nightly/nightly-rustc/rustc_resolve/late/struct.LateResolutionVisitor.html#method.smart_resolve_report_errors which is a lot more complicated.

@jyn514 jyn514 added A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` labels Aug 8, 2020
jyn514 added a commit to jyn514/rust that referenced this issue Aug 20, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 30, 2020
…trochenkov

Fix intra-doc links for cross-crate re-exports of default trait methods

The original fix for this was very simple: rust-lang#58972 ignored `extern_traits` because before rust-lang#65983 was fixed, they would always fail to resolve, giving spurious warnings. So the first commit just undoes that change, so extern traits are now seen by the `collect_intra_doc_links` pass. There are also some minor changes in `librustdoc/fold.rs` to avoid borrowing the `extern_traits` RefCell more than once at a time.

However, that brought up a much more thorny problem. `rustc_resolve` started giving 'error: cannot find a built-in macro with name `cfg`' when documenting `libproc_macro` (I still haven't been able to reproduce on anything smaller than the full standard library). The chain of events looked like this (thanks @eddyb for the help debugging!):

0. `x.py build --stage 1` builds the standard library and creates a sysroot
1. `cargo doc` does something like `cargo check` to create `rmeta`s for all the crates (unrelated to what was built above)
2. the `cargo check`-like `libcore-*.rmeta` is loaded as a transitive dependency *and claims ownership* of builtin macros
3. `rustdoc` later tries to resolve some path in a doc link
4. suggestion logic fires and loads "extern prelude" crates by name
5. the sysroot `libcore-*.rlib` is loaded and *fails to claim ownership* of builtin macros

`rustc_resolve` gives the error after step 5. However, `rustdoc` doesn't need suggestions at all - `resolve_str_path_error` completely discards the `ResolutionError`! The fix implemented in this PR is to skip the suggestion logic for `resolve_ast_path`: pass `record_used: false` and skip `lookup_import_candidates` when `record_used` isn't set.

It's possible that if/when rust-lang#74207 is implemented this will need a more in-depth fix which returns a `ResolutionError` from `compile_macro`, to allow rustdoc to reuse the suggestions from rustc_resolve. However, that's a much larger change and there's no need for it yet, so I haven't implemented it here.

Fixes rust-lang#73829.

r? @GuillaumeGomez
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 A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-enhancement Category: An issue proposing an enhancement or a PR with one. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants