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

Issues with intra-doc links to core/std derive macros #110111

Closed
mvanbem opened this issue Apr 9, 2023 · 10 comments · Fixed by #112014
Closed

Issues with intra-doc links to core/std derive macros #110111

mvanbem opened this issue Apr 9, 2023 · 10 comments · Fixed by #112014
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-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@mvanbem
Copy link

mvanbem commented Apr 9, 2023

I tried to refer to the core Clone derive macro (rather than the core Clone trait) in a rustdoc intra-doc link, but was not able to find a way that works. Along the way I found broken generated links and a broken error message.

Environment

I am using a rust-toolchain.toml with toolchain.channel set to "nightly-2023-04-07". Rustdoc identifies itself as:

$ rustdoc --version --verbose
rustdoc 1.70.0-nightly (28a29282f 2023-04-06)
binary: rustdoc
commit-hash: unknown
commit-date: unknown
host: x86_64-unknown-linux-gnu
release: 1.70.0-nightly
LLVM version: 16.0.2

I happen to be working on a no_std crate, so all of my generated links will point to the core variants of these items.

Correct behavior

First, what works:

/// [`Clone`]

Generated link: https://doc.rust-lang.org/nightly/core/clone/trait.Clone.html

This links to the Clone trait and does not issue an ambiguity warning, apparently due to an intentional disambiguation rule that serves the common case: #82478 (comment). Great! I have benefited from this tens if not hundreds of times. But I'm on the unusual path and want to link to the derive macro.

Incomplete documentation

I did a search and found the Namespaces and Disambiguators section of the rustdoc book. It says, "Paths in Rust have three namespaces: type, value, and macro." This implies to me that type@, value@, and macro@ would be the only valid disambiguators. But then the only examples use struct@ and fn@.

I also found RFC 1946, which provides a more exhaustive (though potentially outdated) list of disambiguators.

The book says there's a macro namespace and the RFC mentions the macro@ disambiguator, so I tried that one. I also found that a derive@ disambiguator is accepted and has the same effect, though neither doc mentions it. It would be nice if there was an exhaustive list of these disambiguators and their behaviors.

Broken links

I tried using the macro@ and derive@ disambiguators in my doc links:

/// [`macro@Clone`]
/// [`derive@Clone`]

Generated links:

Those are 404s! And rustdoc does not issue any warnings or errors. Navigating the official docs, this is the link I want:

https://doc.rust-lang.org/nightly/core/clone/macro.Clone.html

So the official docs and and rustdoc disagree on how to refer to this item.

Third-party derive macros work

Does this happen with user-defined derive macros? I was surprised to see I was unable to link to the derive_more::Add or clap::Parser derive macros (no idea why and #[doc(hidden)], respectively). But here's serde 1.0.151 with the derive feature enabled:

/// [`serde::Serialize`]
/// [`macro@serde::Serialize`]
/// [`derive@serde::Serialize`]

Generated links:

Those links all work!

So I'm led to believe the std/core docs are doing something unusual with the naming of their derive macro pages.

Sliced error message

For fun, I tried specifying an incorrect disambiguator:

/// For bug report: [`struct@Clone`]
warning: incompatible link kind for `Clone`
   --> my_crate/src/lib.rs:175:23
    |
175 | /// For bug report: [`struct@Clone`]
    |                       ^^^^^^^^^^^^ this link resolved to a trait, which is not a struct
    |
    = note: `#[warn(rustdoc::broken_intra_doc_links)]` on by default
help: to link to the trait, prefix with `trait@`
    |
175 | /// For bug report: [`trait@lone`]
    |                       ~~~~~~

The suggestion replaced struct with trait, which is reasonable, but it removed the C from Clone in the process.

The error message seems to do the right thing if I omit the backticks:

warning: incompatible link kind for `Clone`
   --> my_crate/src/lib.rs:175:19
    |
175 | /// Bug attempt: [struct@Clone]
    |                   ^^^^^^^^^^^^ this link resolved to a trait, which is not a struct
    |
    = note: `#[warn(rustdoc::broken_intra_doc_links)]` on by default
help: to link to the trait, prefix with `trait@`
    |
175 | /// Bug attempt: [trait@Clone]
    |                   ~~~~~~
@mvanbem mvanbem added the C-bug Category: This is a bug. label Apr 9, 2023
@jyn514 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 Apr 10, 2023
@GuillaumeGomez
Copy link
Member

So there are actually two issues from what I can understand: some std proc-macros are not considered as such and there should be an error in case the name matches a proc-macro. I'll start taking a look at why compiler proc-macros are not considered as proc-macros.

@fmease
Copy link
Member

fmease commented Apr 13, 2023

I'll start taking a look at why compiler proc-macros are not considered as proc-macros.

@GuillaumeGomez I've already taken a look at this a few days ago. Basically, the derive macro Clone is located at macro.Clone.html instead of derive.Clone.html since it is declared as a plain 2.0 macro (macro Clone(…) { … }) except that it also bears the internal attribute #[rustc_builtin_macro].
Rustdoc faithfully cleans it to a macro and uses the prefix macro since nothing indicates that it is a derive-macro.

The compiler has an internal hard-coded mapping from macro name to macro kind (bang, attribute, derive).
It's located here: rustc_builtin_macros::register_builtin_macros.

Now, the intra-doc link resolves the macro Clone to a derive-macro (and uses the prefix derive) since rustc has since mapped it to a derive macro.

@GuillaumeGomez
Copy link
Member

Yes but they are marked as such in the ItemKind::Macro(_, MacroKind::Derive). I already wrote the fix for this part. :)

@fmease
Copy link
Member

fmease commented Apr 13, 2023

Okay, so in your fix do you change the prefix found in the URL of the page of the built-in macro from macro to derive or do you change the intra-doc link resolver to use macro instead of derive for built-in macros?

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Apr 13, 2023

I only fixed the part where a proc-macro is not correctly recognized for now. I'll make the intra-doc link fix after. You can see my fix here. I need to add a test for it, I'll do so after lunch.

@fmease
Copy link
Member

fmease commented Apr 13, 2023

Okay, so in your fix do you change the prefix found in the URL of the page of the built-in macro from macro to derive or do you change the intra-doc link resolver to use macro instead of derive for built-in macros?

Since I didn't know which of those two directions I should pursue, I held off from writing a fix. Personally I prefer changing the URL (and maybe also the page layout in the future) of those built-in derive macros to reflect that they are derive-macros even if not marked as such but that would be a breaking change. Special-casing built-in macros in the intra-doc resolution code to use macro no matter what seems hacky to me. Just my stance.

@GuillaumeGomez
Copy link
Member

Agreed. The fix should prevent that. However, the second part still needs to be done (ie, emitting a warning if a macro - bang, derive, etc - has the same name as another matching item). I improved things recently in this area so I think it shouldn't be too complicated to achieve.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 14, 2023
…ve, r=notriddle

rustdoc: Correctly handle built-in compiler proc-macros as proc-macro and not macro

Part of rust-lang#110111.

There were actually one issue split in two parts:
 * Compiler built-in proc-macro were incorrectly considered as macros and not proc-macros.
 * Re-exports of compiler built-in proc-macros were considering them as macros.

Both issues can be fixed by looking at the `MacroKind` variant instead of just relying on information extracted later on.

r? `@fmease`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 14, 2023
…ve, r=notriddle

rustdoc: Correctly handle built-in compiler proc-macros as proc-macro and not macro

Part of rust-lang#110111.

There were actually one issue split in two parts:
 * Compiler built-in proc-macro were incorrectly considered as macros and not proc-macros.
 * Re-exports of compiler built-in proc-macros were considering them as macros.

Both issues can be fixed by looking at the `MacroKind` variant instead of just relying on information extracted later on.

r? ``@fmease``
@jyn514 jyn514 added A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-diagnostics Area: Messages for errors, warnings, and lints D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. labels Apr 14, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 14, 2023
…-proc-trait, r=notriddle

[rustdoc] Add explanations for auto-disambiguation when an intra doc link is resolved to a proc-macro and a trait at the same time

Part of rust-lang#110111.

r? `@notriddle`
@GuillaumeGomez
Copy link
Member

So only the sliced error message is now remaining. Small summary:

/// For bug report: [`struct@Clone`]

Result:

warning: incompatible link kind for `Clone`
   --> my_crate/src/lib.rs:175:23
    |
175 | /// For bug report: [`struct@Clone`]
    |                       ^^^^^^^^^^^^ this link resolved to a trait, which is not a struct
    |
    = note: `#[warn(rustdoc::broken_intra_doc_links)]` on by default
help: to link to the trait, prefix with `trait@`
    |
175 | /// For bug report: [`trait@lone`]
    |                       ~~~~~~

Any idea @estebank maybe?

@fmease
Copy link
Member

fmease commented Apr 18, 2023

I'm taking a look. I've already found the source of the bug. Now I just need to find a good fix.
@rustbot claim

@fmease
Copy link
Member

fmease commented Apr 21, 2023

Just as an update, since I couldn't quite find a most general proper fix yet. Here are some more examples where it goes wrong:

  • source: [`struct@Clone`] , suggestion: [`trait@lone`]
  • source: [```struct@Clone```], suggestion: [`trait@lone```]
  • source: [ ` struct@Clone ` ], suggestion: trait@ct@Clone ` ] (sic!)
  • source: [ `Clone ()` ], suggestion: trait@[ `Clon (sic!)

There are several issues at play here: When creating a Span

  • we are basing off our index calculations on a HTML-normalized string (i.e. where consecutive spaces have been collapsed into single ones) instead of the source string (and we can't easily get the source string from pulldown-cmark unfortunately)
  • we assume that the delimiter like trait@ is right at the start of the destination string but actually spaces, backticks and sometimes even [ can come first (under certain circumstances).
  • sometimes the base Span is trimmed by one backtick, further rendering the offset calculations incorrect

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue May 27, 2023
…d-syntax, r=GuillaumeGomez,fmease

rustdoc: get unnormalized link destination for suggestions

Fixes rust-lang#110111

This bug, and the workaround in this PR, is closely linked to [pulldown-cmark/pulldown-cmark#441], getting offsets of link components. In particular, pulldown-cmark doesn't provide the offsets of the contents of a link.

To work around this, rustdoc parser parts of a link definition itself.

[pulldown-cmark/pulldown-cmark#441]: pulldown-cmark/pulldown-cmark#441
@bors bors closed this as completed in 1a77d9a May 27, 2023
@fmease fmease removed their assignment Jun 25, 2023
xobs pushed a commit to betrusted-io/rust that referenced this issue Aug 5, 2023
Fixes rust-lang#110111

This bug, and the workaround in this commit, is closely linked to
[pulldown-cmark/pulldown-cmark#441], getting offsets of link
components. In particular, pulldown-cmark doesn't provide the
offsets of the contents of a link.

To work around this, rustdoc parser parts of a link definition
itself.

[pulldown-cmark/pulldown-cmark#441]: pulldown-cmark/pulldown-cmark#441
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-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. 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.

4 participants