rustdoc: Unify macro intra-doc link resolution with type and value resolution #91427

jyn514 · 2021-12-01T18:02:38Z

Fixes #81633 and also cleans up quite a lot of code.

rustdoc should already have error-ed out for conflicting anchors, and it doesn't make sense to do this for only one namespace anyway.

rust-highfive · 2021-12-01T18:02:41Z

Some changes occurred in intra-doc-links.

cc @jyn514

jyn514 · 2021-12-01T18:03:44Z

Oh, this can probably remove the all_macros field on the resolver too - cc @petrochenkov

rust-log-analyzer · 2021-12-01T18:42:52Z

The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

 Documenting core v0.0.0 (/checkout/library/core)
error: unresolved link to `unreachable`
  --> library/core/src/hint.rs:25:37
   |
25 | /// Otherwise, consider using the [`unreachable!`] macro, which does not allow
   |                                     ^^^^^^^^^^^^ no item named `unreachable` in scope
   |
   = note: `-D rustdoc::broken-intra-doc-links` implied by `-D warnings`
   = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
error: unresolved link to `write`
   --> library/core/src/fmt/mod.rs:166:33
    |
    |
166 |     /// Glue for usage of the [`write!`] macro with implementors of this trait.
    |                                 ^^^^^^ this link resolves to the function `write`, which is not in the macro namespace
help: to link to the function, add parentheses
    |
    |
166 |     /// Glue for usage of the [`write!()`] macro with implementors of this trait.

error: unresolved link to `write`
   --> library/core/src/fmt/mod.rs:169:15
    |
    |
169 |     /// the [`write!`] macro itself.
    |               ^^^^^^ this link resolves to the function `write`, which is not in the macro namespace
help: to link to the function, add parentheses
    |
    |
169 |     /// the [`write!()`] macro itself.

error: unresolved link to `panic`
   --> library/core/src/macros/mod.rs:171:28
    |
    |
171 | /// This will invoke the [`panic!`] macro if the provided expression cannot be
    |                            ^^^^^^ no item named `panic` in scope
    |
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`

error: unresolved link to `assert_eq`
    |
    |
223 | /// Unlike [`assert_eq!`], `debug_assert_eq!` statements are only enabled in non
    |              ^^^^^^^^^^ no item named `assert_eq` in scope
    |
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`

error: unresolved link to `assert_ne`
    |
    |
248 | /// Unlike [`assert_ne!`], `debug_assert_ne!` statements are only enabled in non
    |              ^^^^^^^^^^ no item named `assert_ne` in scope
    |
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
error: unresolved link to `write`
   --> library/core/src/macros/mod.rs:487:33
    |
    |
487 | /// For more information, see [`write!`]. For information on the format string syntax, see
    |
    |
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
error: unresolved link to `panic`
   --> library/core/src/macros/mod.rs:548:45
    |
    |
548 | /// program immediately terminates with a [`panic!`].
    |                                             ^^^^^^ no item named `panic` in scope
    |
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
error: unresolved link to `panic`
   --> library/core/src/macros/mod.rs:557:24
    |
    |
557 | /// This will always [`panic!`] because `unreachable!` is just a shorthand for `panic!` with a
    |                        ^^^^^^ no item named `panic` in scope
    |
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`

error: unresolved link to `todo`
    |
    |
609 | /// The difference between `unimplemented!` and [`todo!`] is that while `todo!`
    |                                                   ^^^^^ no item named `todo` in scope
    |
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
error: unresolved link to `panic`
   --> library/core/src/macros/mod.rs:616:24
    |
    |
616 | /// This will always [`panic!`] because `unimplemented!` is just a shorthand for `panic!` with a
    |                        ^^^^^^ no item named `panic` in scope
    |
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`

error: unresolved link to `unimplemented`
    |
    |
687 | /// The difference between [`unimplemented!`] and `todo!` is that while `todo!` conveys
    |                              ^^^^^^^^^^^^^^ no item named `unimplemented` in scope
    |
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
error: unresolved link to `panic`
   --> library/core/src/macros/mod.rs:694:24
    |
    |
694 | /// This will always [`panic!`].
    |                        ^^^^^^ no item named `panic` in scope
    |
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
error: unresolved link to `panic`
   --> library/core/src/macros/mod.rs:753:91
    |
    |
753 |     /// better error messages for erroneous conditions. It's the compiler-level form of [`panic!`],
    |                                                                                           ^^^^^^ no item named `panic` in scope
    |
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
error: unresolved link to `write`
   --> library/core/src/macros/mod.rs:801:53
    |
    |
801 |     /// All other formatting macros ([`format!`], [`write!`], [`println!`], etc) are
    |
    |
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
error: unresolved link to `panic`
    --> library/core/src/macros/mod.rs:1282:32
     |
     |
1282 |     /// This will invoke the [`panic!`] macro if the provided expression cannot be
     |                                ^^^^^^ no item named `panic` in scope
     |
     = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
error: unresolved link to `debug_assert`
    --> library/core/src/macros/mod.rs:1288:28
     |
     |
1288 |     /// be disabled. See [`debug_assert!`] for assertions that are not enabled in
     |                            ^^^^^^^^^^^^^ no item named `debug_assert` in scope
     |
     = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
error: could not document `core`

Caused by:
Caused by:
  process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustdoc --edition=2018 --crate-type lib --crate-name core library/core/src/lib.rs --target x86_64-unknown-linux-gnu -o /checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/x86_64-unknown-linux-gnu/doc --error-format=json --json=diagnostic-rendered-ansi --markdown-css rust.css --markdown-no-toc -Z unstable-options --resource-suffix 1.59.0 --index-page /checkout/src/doc/index.md -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/release/deps -Zsymbol-mangling-version=legacy -Dwarnings '-Wrustdoc::invalid_codeblock_attributes' --crate-version '1.59.0-nightly
  (b782ab7fa
  2021-12-01)' '-Zcrate-attr=doc(html_root_url="https://doc.rust-lang.org/nightly/")'` (exit status: 1)


command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "rustdoc" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "-p" "core" "-Zskip-rustdoc-fingerprint" "--" "--markdown-css" "rust.css" "--markdown-no-toc" "-Z" "unstable-options" "--resource-suffix" "1.59.0" "--index-page" "/checkout/src/doc/index.md"


Build completed unsuccessfully in 0:31:53

jyn514 · 2021-12-01T20:31:08Z

Hmm, it looks like resolve_str_path_error doesn't consider #[macro_export] ... @petrochenkov would it be easy to make it do that or should I revert all the rustdoc changes and add back the call to resolve_macro_path?

jyn514 · 2021-12-01T20:40:03Z

MCVE:

#[macro_export]
macro_rules! foo {
    () => {};
}


pub mod bar {
    /// [foo!]
    pub fn baz() {
        foo!();
    }
}

Rustc compiles the foo! invocation fine, rustdoc thinks it's a broken link.

Manishearth

sweet, i always disliked this setup

Manishearth · 2021-12-02T04:55:09Z

src/librustdoc/passes/collect_intra_doc_links.rs

        self.cx.enter_resolver(|resolver| {
-            // FIXME(jynelson): does this really need 3 separate lookups?


turns out, no 😆

Manishearth · 2021-12-02T04:56:33Z

src/librustdoc/passes/collect_intra_doc_links.rs

-                None,
-                &ParentScope::module(resolver.graph_root(), resolver),
-                false,
-                false,


perhaps we should also add tests that ensure that macros can still be linked to on both newer and older editions

Ah, yeah, you noted a problem with #[macro_export] above

Yeah this code was like this to handle macro_export/old-style macro resolution. Which still exists. But perhaps we can handle this a bit cleaner.

petrochenkov · 2021-12-02T05:47:50Z

Hmm, it looks like resolve_str_path_error doesn't consider #[macro_export]

macro_export is irrelevant, resolve_str_path_error doesn't consider non-modularized macro_rules scopes in general.

{
    struct S;

    macro_rules! mac { () => {} }
    // `mac`'s scope starts here

    /// `mac` <- `resolve_str_path_error` won't see this
   struct Z;

    //`mac`'s scope ends here
}

As you can see fn resolve_str_path_error will construct the scope in which the path is resolved using ParentScope::module, which leaves the macro_rules component of ParentScope empty.
Ideally the current macro_rules scope should be specifically tracked and updated in the current ParentScope like it's done in BuildReducedGraphVisitor, for example.

So both using and not using all_macros is wrong, but what is more wrong depends on whether you like false positives or false negatives more.

petrochenkov · 2021-12-02T08:34:44Z

Ideally the current macro_rules scope should be specifically tracked and updated in the current ParentScope

This will be very easy to do for local macro_rules after link resolution is moved to early.rs, BTW.

This will be hard to do for links inlined from other crates, because we don't even keep the relevant information in metadata (although we may start keeping it due to macros 2.0).

bors · 2022-01-11T03:20:24Z

☔ The latest upstream changes (presumably #92601) made this pull request unmergeable. Please resolve the merge conflicts.

JohnCSimon · 2022-02-06T02:39:45Z

merge conflicts + broken build.

jyn514 · 2022-02-06T04:22:19Z

yes, I'm aware, that's why it's marked waiting-on-author.

petrochenkov · 2022-02-06T07:54:14Z

Any chance to get this landed with all_macros kept and being used as a fallback if regular resolution fails?
It would be very helpful for "rustdoc: Stop cloning the resolver".

jyn514 · 2022-02-06T14:54:06Z

@petrochenkov I don't know when I'll get a chance to work on this. Feel free to take over the PR and have someone from the rustdoc team review.

jyn514 · 2022-02-07T00:33:04Z

Ok I take it back, I can do this much:

Any chance to get this landed with all_macros kept and being used as a fallback if regular resolution fails?

Not sure how that's helpful for the resolver hackery, since it still requires access to the resolver in resolve_macro, but it's easy enough. I'm not planning to work on removing all_macros.

petrochenkov · 2022-02-07T00:39:13Z

Not sure how that's helpful for the resolver hackery

It removes the use of resolve_macro_path and leaves resolve_str_path_error as the only used resolution method.

jyn514 · 2022-02-07T00:51:24Z

macro_export is irrelevant, resolve_str_path_error doesn't consider non-modularized macro_rules scopes in general.

I'm not sure in what context you could document a non-modularized macro. Rustdoc doesn't document anything at function-scope, only at the module level. I don't understand why the macro in the MCVE (#91427 (comment)) isn't resolved according to the rules you explained.

Manishearth · 2022-02-07T02:20:46Z

I may also have time later to do some rebasing and patchups here if necessary, but not any time soon.

jyn514 · 2022-02-07T02:33:08Z

@Manishearth this requires more than patchups, someone needs to investigate why the MCVE doesn't work and then figure out how to change the resolver internals to make it work with resolve_str_path_error.

…=petrochenkov rustdoc: Special-case macro lookups less Previously, rustdoc had 3 fallbacks it used: 1. `resolve_macro_path` 2. `all_macros` 3. `resolve_str_path_error` Ideally, it would only use `resolve_str_path_error`, to be consistent with other namespaces. Unfortunately, that doesn't consider macros that aren't defined at module scope; consider for instance ```rust { struct S; macro_rules! mac { () => {} } // `mac`'s scope starts here /// `mac` <- `resolve_str_path_error` won't see this struct Z; //`mac`'s scope ends here } ``` This changes it to only use `all_macros` and `resolve_str_path_error`, and gives `resolve_str_path_error` precedence over `all_macros` in case there are two macros with the same name in the same module. This is a smaller version of rust-lang#91427. r? `@petrochenkov`

jyn514 · 2022-02-11T03:43:53Z

Not planning to look into this. Happy for someone else to pick it up.

jyn514 added 3 commits December 1, 2021 18:01

Fix bug where rustdoc would resolve macros that weren't in scope

7cd0291

remove unnecessary special-casing for resolve_macro

9630a81

remove skechy-looking special casing for fragments in the type namespace

956f7ee

rustdoc should already have error-ed out for conflicting anchors, and it doesn't make sense to do this for only one namespace anyway.

jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name labels Dec 1, 2021

rust-highfive assigned Manishearth Dec 1, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 1, 2021

jyn514 added 2 commits December 1, 2021 18:05

Remove unnecessary all_macros field from the resolver

496be6b

Make resolve_macro_path private

e70e74f

jyn514 assigned petrochenkov Dec 1, 2021

Manishearth approved these changes Dec 2, 2021

View reviewed changes

petrochenkov 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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 2, 2021

jyn514 mentioned this pull request Feb 7, 2022

rustdoc: Special-case macro lookups less #93721

Merged

jyn514 closed this Feb 11, 2022

jyn514 mentioned this pull request Apr 23, 2022

rustdoc: Cleanup doc link resolution #96345

Merged

jyn514 deleted the macro-link-scoping branch April 25, 2022 16:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: Unify macro intra-doc link resolution with type and value resolution #91427

rustdoc: Unify macro intra-doc link resolution with type and value resolution #91427

jyn514 commented Dec 1, 2021

rust-highfive commented Dec 1, 2021

jyn514 commented Dec 1, 2021 •

edited

Loading

rust-log-analyzer commented Dec 1, 2021

jyn514 commented Dec 1, 2021

jyn514 commented Dec 1, 2021

Manishearth left a comment

Manishearth Dec 2, 2021

Manishearth Dec 2, 2021

Manishearth Dec 2, 2021

Manishearth Dec 2, 2021

petrochenkov commented Dec 2, 2021

petrochenkov commented Dec 2, 2021

bors commented Jan 11, 2022

JohnCSimon commented Feb 6, 2022 •

edited

Loading

jyn514 commented Feb 6, 2022

petrochenkov commented Feb 6, 2022

jyn514 commented Feb 6, 2022

jyn514 commented Feb 7, 2022

petrochenkov commented Feb 7, 2022

jyn514 commented Feb 7, 2022 •

edited

Loading

Manishearth commented Feb 7, 2022

jyn514 commented Feb 7, 2022

jyn514 commented Feb 11, 2022

		self.cx.enter_resolver(\|resolver\| {
		// FIXME(jynelson): does this really need 3 separate lookups?

rustdoc: Unify macro intra-doc link resolution with type and value resolution #91427

rustdoc: Unify macro intra-doc link resolution with type and value resolution #91427

Conversation

jyn514 commented Dec 1, 2021

rust-highfive commented Dec 1, 2021

jyn514 commented Dec 1, 2021 • edited Loading

rust-log-analyzer commented Dec 1, 2021

jyn514 commented Dec 1, 2021

jyn514 commented Dec 1, 2021

Manishearth left a comment

Choose a reason for hiding this comment

Manishearth Dec 2, 2021

Choose a reason for hiding this comment

Manishearth Dec 2, 2021

Choose a reason for hiding this comment

Manishearth Dec 2, 2021

Choose a reason for hiding this comment

Manishearth Dec 2, 2021

Choose a reason for hiding this comment

petrochenkov commented Dec 2, 2021

petrochenkov commented Dec 2, 2021

bors commented Jan 11, 2022

JohnCSimon commented Feb 6, 2022 • edited Loading

jyn514 commented Feb 6, 2022

petrochenkov commented Feb 6, 2022

jyn514 commented Feb 6, 2022

jyn514 commented Feb 7, 2022

petrochenkov commented Feb 7, 2022

jyn514 commented Feb 7, 2022 • edited Loading

Manishearth commented Feb 7, 2022

jyn514 commented Feb 7, 2022

jyn514 commented Feb 11, 2022

jyn514 commented Dec 1, 2021 •

edited

Loading

JohnCSimon commented Feb 6, 2022 •

edited

Loading

jyn514 commented Feb 7, 2022 •

edited

Loading