[rustdoc] Fix invalid jump to def macro link generation #148080

GuillaumeGomez · 2025-10-24T19:50:52Z

Follow-up of #147820.

I realized that when there was no intra-doc link linking to the same item, then the generated link for macros in jump to def would be invalid.

To make the code less redundant, I merged the "registering" of items and the href generation use the same code for macros.

r? @notriddle

GuillaumeGomez · 2025-10-31T14:11:44Z

ping @notriddle

GuillaumeGomez · 2025-11-04T15:26:10Z

r? @lolbinarycat

lolbinarycat · 2025-11-05T21:37:19Z

tests/rustdoc/jump-to-def/derive-macro.rs

@@ -0,0 +1,21 @@
+// This test ensures that the same link is generated in both intra-doc links


Please put this file in the existing tests/rustdoc/jump-to-def dir. (created #148548 about moving existing files that should also be in there)

Also, I'm slightly confused as to how this test works, and what it's testing, as the PR description says "I realized that when there was no intra-doc link linking to the same item, then the generated link for macros in jump to def would be invalid.", but that's not what's happening here, and trying to run this test against current nightly actually results in an ICE, not an invalid link.

Well, there is no intra-doc link linking to Debug and PartialEq derive proc macro in the docs, which is what we're testing here.

Also, that might be a newer change because originally, it didn't ICE but generated invalid links.

Do you mean "there's no intra-doc link linking to Debug and PartialEq in core"? Because line 10 and 11 literally test that a link exists. Or are you not counting those as intra-doc because they aren't written as a doc comment? If that's the case, you should probably remove the comment on line 9 that describes them as doc comments.

Arf, I'm so bad at explaining.

So:

//! [PartialEq] [Debug] #[derive(Debug, PartialEq)] pub struct Foo;

Before this PR, if the //! [PartialEq] [Debug] doc comment was not present, in "jump to def", the links for Debug and PartialEq would be wrongly generated there (in the source code pages). Because it wasn't in the cache and so the path computation was wrong.

Yeah, that makes sense, it's just that the PR description and the code comments in the test have conflicting definitions of "intra-doc link", which was very confusing at first.

GuillaumeGomez · 2025-11-06T12:19:38Z

Moved the test file to where it's supposed to be.

lolbinarycat

Realizing that rustdoc path handling is kinda a mess, should probably get cleaned up at some point.

View changes since this review

lolbinarycat · 2025-11-06T16:46:02Z

tests/rustdoc/jump-to-def/derive-macro.rs

@@ -0,0 +1,21 @@
+// This test ensures that the same link is generated in both intra-doc links


Do you mean "there's no intra-doc link linking to Debug and PartialEq in core"? Because line 10 and 11 literally test that a link exists. Or are you not counting those as intra-doc because they aren't written as a doc comment? If that's the case, you should probably remove the comment on line 9 that describes them as doc comments.

lolbinarycat · 2025-11-06T16:47:20Z

tests/rustdoc/jump-to-def/assoc-items.rs

@@ -26,8 +26,9 @@ impl C {
    pub fn wat() {}
 }

-//@ has - '//a[@href="{{channel}}/core/fmt/macros/macro.Debug.html"]' 'Debug'
-//@ has - '//a[@href="{{channel}}/core/cmp/macro.PartialEq.html"]' 'PartialEq'
+// These two links must not change and in particular must contain `/derive.`!


Why must they not change? Seems like an odd thing to say without explanation considering this PR is changing them.

Because they are valid as is and since it's derive macros, "derive" must be in the URL.

I mean, they're obviously the best thing to do currently, but that's the case for most things in tests, "must not" seems a bit strong for "this is the current best"

Also, does changing this break any existing links to these pages? It doesn't look like there are redirects.

src/librustdoc/clean/inline.rs

lolbinarycat · 2025-11-06T17:18:58Z

src/librustdoc/html/format.rs

@@ -408,7 +404,7 @@ fn generate_macro_def_id_path(
            return Err(HrefError::NotInExternalCache);
        }
    };
-    Ok((url, ItemType::Macro, fqp))
+    Ok((url, item_type, path))


Unless I'm reading something wrong, this changes behavior in a pretty undesirable way. Previously, the third element in the tuple was the rust path, such as ["std", "println"], now it's the url path, but split into components.

I'm surprised this doesn't cause test failures

We probably shouldn't have functions that return tuples not document what those tuples are

Honestly there's a potential argument that maybe this should just be 3 different functions, tho this exact tuple does seem to be getting passed through quite a few layers, so maybe it should just be a struct.

I'd prefer to tend to a unification rather than having three different functions to achieve a same result. However you're right: it should be documented.

I'm also extremely uncomfortable changing the behavior of this function like this, at least not without a lot of reviewing the code, it seems very likely that it would cause some sort of bug.

Additionally, the current behavior after this PR directly contradicts the newly added documentation.

path is the same as fqp, this behaviour didn't change. Simply fixed the path generated for macros. The part generating the previous fqp is now done in get_item_path

Additionally, the current behavior after this PR directly contradicts the newly added documentation.

Which documentation are you referring to?

path is the same as fqp

I have a really hard time believing that though, since before fqp wasn't getting modified, and now it is getting modified.

this is the contradiction i'm referring to, path is modified to turn it from a Rust path to a URL path. it contradicts with the docs that say it returns a Rust path.

Well, the macro "rust path" is `crate::macro_name", no?

yes, and we're returning [crate, macro.macro_name.html] now.

Ok now I see your point. I'll add some name= checks too because I think it's used for that.

You were right, this generated wrong title attributes. Fixed it and added a regression test. Very good catch, thanks a lot!

GuillaumeGomez · 2025-11-06T17:45:03Z

Added missing docs.

lolbinarycat

Looks mostly good now, one doc comment i'd like to see improved, and a few potential improvements that aren't essential.

View changes since this review

lolbinarycat · 2025-11-10T18:23:40Z

src/librustdoc/clean/inline.rs

    tcx.def_path(def_id).data.into_iter().filter_map(|elem| elem.data.get_opt_name()).collect()
 }

+/// Get the path to an item in a URL sense: we use it to generate the URL to the actual item.


Suggested change

/// Get the path to an item in a URL sense: we use it to generate the URL to the actual item.

/// Get the public Rust path to an item. This is used to generate the URL for the item's page.

I think the previous docs were a bit confusing because "URL path" is it's own thing, notably including the kind.name.html form.

lolbinarycat · 2025-11-10T18:26:47Z

src/librustdoc/html/format.rs

    def_id: DefId,
    cx: &Context<'_>,
    root_path: Option<&str>,
 ) -> Result<(String, ItemType, Vec<Symbol>), HrefError> {


Do you think you could at least extract this tuple (which is used in several functions) into a type alias, with docs?

I would like to eventually make it into a struct, but that would require decently involved refactoring that is probably outside of the scope of this PR, but maybe a type alias wouldn't be too much work?

That seems like less effort than copy-pasting a doc comment that explains the meaning of the return values everywhere.

I like it so I made the change.

lolbinarycat · 2025-11-10T18:31:31Z

src/librustdoc/html/format.rs

    if let Some(last) = path.last_mut() {
-        *last = Symbol::intern(&format!("macro.{last}.html"));
+        *last = Symbol::intern(&format!("{}.{last}.html", item_type.as_str()));
    }


maybe a bit excessive, but the clone isn't actually needed since we only need path to be an iterator.

we could do something like let path = fqp[..fqp.len()-1].iter().chain(once(Symbol::intern(&format("{}.{last}.html"))).

no idea if this would be worth it, but it is possible.

This is surprisingly complex to do because of from_fn so for now I used pop and push on path and added a FIXME.

GuillaumeGomez · 2025-11-11T12:16:59Z

Applied suggestions.

bors · 2025-11-11T13:45:10Z

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

rustbot · 2025-11-11T16:56:46Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

GuillaumeGomez · 2025-11-11T16:57:42Z

Fixed merge conflict.

lolbinarycat

Huge improvement, thanks!

one tiny nitpick, you can r=me with or without that addressed

View changes since this review

lolbinarycat · 2025-11-11T17:35:23Z

src/librustdoc/html/format.rs

+    Ok(HrefInfo { url, kind: item_type, rust_path: path })
 }

+/// If the function succeeded, it will return the full URL to the item, its type and a `Vec`


this doc comment isn't really needed now that we're not returning a tuple

Oh indeed. It's rare to just remove docs because code is now explicit. I like it! 😆

Well, it's not really that we've removed docs, just that we've move the docs to the fields of the struct, so it doesn't need to be on the function anymore.

GuillaumeGomez · 2025-11-11T18:32:18Z

@bors r=lolbinarycat rollup

bors · 2025-11-11T18:32:21Z

📌 Commit 4d3d3b3 has been approved by lolbinarycat

It is now in the queue for this repository.

… r=lolbinarycat [rustdoc] Fix invalid jump to def macro link generation Follow-up of rust-lang#147820. I realized that when there was no intra-doc link linking to the same item, then the generated link for macros in jump to def would be invalid. To make the code less redundant, I merged the "registering" of items and the href generation use the same code for macros. r? `@notriddle`

rustbot assigned notriddle Oct 24, 2025

rustbot assigned lolbinarycat and unassigned notriddle Nov 4, 2025

lolbinarycat reviewed Nov 5, 2025

View reviewed changes

GuillaumeGomez force-pushed the fix-jump-def-links branch from 87a7bdf to ae76c7b Compare November 6, 2025 12:19

lolbinarycat reviewed Nov 6, 2025

View reviewed changes

GuillaumeGomez force-pushed the fix-jump-def-links branch from 7017133 to ca3f622 Compare November 10, 2025 16:17

This comment has been minimized.

Sign in to view

lolbinarycat reviewed Nov 10, 2025

View reviewed changes

GuillaumeGomez added 3 commits November 11, 2025 17:41

Fix invalid jump to def macro link generation

04f798b

Update jump to def macro link generation test

dacabcd

Add missing documentation

f4e1ffc

GuillaumeGomez force-pushed the fix-jump-def-links branch from a874e8e to 4e742dc Compare November 11, 2025 16:56

lolbinarycat reviewed Nov 11, 2025

View reviewed changes

Improve code

4d3d3b3

GuillaumeGomez force-pushed the fix-jump-def-links branch from 4e742dc to 4d3d3b3 Compare November 11, 2025 17:45

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 11, 2025

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Nov 11, 2025

GuillaumeGomez mentioned this pull request Nov 11, 2025

Rollup of 6 pull requests #148829

Open

		@@ -0,0 +1,21 @@
		// This test ensures that the same link is generated in both intra-doc links

	/// Get the path to an item in a URL sense: we use it to generate the URL to the actual item.
	/// Get the public Rust path to an item. This is used to generate the URL for the item's page.

[rustdoc] Fix invalid jump to def macro link generation #148080

Are you sure you want to change the base?

[rustdoc] Fix invalid jump to def macro link generation #148080

Conversation

GuillaumeGomez commented Oct 24, 2025

Uh oh!

GuillaumeGomez commented Oct 31, 2025

Uh oh!

GuillaumeGomez commented Nov 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lolbinarycat left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Nov 6, 2025

Uh oh!

This comment has been minimized.

lolbinarycat left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Nov 11, 2025

Uh oh!

bors commented Nov 11, 2025

Uh oh!

rustbot commented Nov 11, 2025

Uh oh!

GuillaumeGomez commented Nov 11, 2025

GuillaumeGomez commented Nov 6, 2025 •

edited

Loading

lolbinarycat left a comment •

edited by rustbot

Loading

lolbinarycat left a comment •

edited by rustbot

Loading

lolbinarycat left a comment •

edited by rustbot

Loading