[rustdoc] Fix invalid jump to def macro link generation by GuillaumeGomez · Pull Request #148080 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation41 Commits4 Checks11 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-rustdoc

Relevant to the rustdoc team, which will review and decide on the PR/issue.

T-rustdoc-frontend

Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.

labels

Oct 24, 2025

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -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.`!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

This comment has been minimized.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

| 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. |

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// 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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it so I made the change.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bors r=lolbinarycat rollup

📌 Commit 4d3d3b3 has been approved by lolbinarycat

It is now in the queue for this repository.

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 added a commit to GuillaumeGomez/rust that referenced this pull request

Nov 11, 2025

… 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

bors added a commit that referenced this pull request

Nov 11, 2025

Rollup of 6 pull requests

Successful merges:

#147753 (Suggest add bounding value for RangeTo)
#148080 ([rustdoc] Fix invalid jump to def macro link generation)
#148465 (Adjust spans into the for loops context before creating the new desugaring spans.)
#148500 (Update git index before running diff-index)
#148536 (cmse: add test for async and const functions)
#148819 (Remove specialized warning for removed target)

r? @ghost @rustbot modify labels: rollup

Zalathar added a commit to Zalathar/rust that referenced this pull request

Nov 11, 2025

… 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

Zalathar added a commit to Zalathar/rust that referenced this pull request

Nov 11, 2025

… 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

bors added a commit that referenced this pull request

Nov 12, 2025

Rollup of 12 pull requests

Successful merges:

#146627 (Simplify jemalloc setup)
#147753 (Suggest add bounding value for RangeTo)
#147974 (Improve diagnostics for buffer reuse with borrowed references)
#148080 ([rustdoc] Fix invalid jump to def macro link generation)
#148424 (bootstrap: Add snapshot tests for path-to-step handling)
#148500 (Update git index before running diff-index)
#148536 (cmse: add test for async and const functions)
#148770 (implement feature(c_variadic_naked_functions))
#148819 (Remove specialized warning for removed target)
#148830 (miri subtree update)
#148833 (Update rustbook dependencies)
#148841 (Remove more #[must_use] from portable-simd)

r? @ghost @rustbot modify labels: rollup

Zalathar added a commit to Zalathar/rust that referenced this pull request

Nov 12, 2025

… 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

bors added a commit that referenced this pull request

Nov 12, 2025

Rollup of 16 pull requests

Successful merges:

#146627 (Simplify jemalloc setup)
#147753 (Suggest add bounding value for RangeTo)
#147832 (rustdoc: Don't pass RenderOptions to DocContext)
#147974 (Improve diagnostics for buffer reuse with borrowed references)
#148080 ([rustdoc] Fix invalid jump to def macro link generation)
#148465 (Adjust spans into the for loops context before creating the new desugaring spans.)
#148500 (Update git index before running diff-index)
#148531 (rustc_target: introduce Abi, Env, Os)
#148536 (cmse: add test for async and const functions)
#148770 (implement feature(c_variadic_naked_functions))
#148780 (fix filecheck typos in tests)
#148819 (Remove specialized warning for removed target)
#148830 (miri subtree update)
#148833 (Update rustbook dependencies)
#148834 (fix(rustdoc): Color doctest errors)
#148841 (Remove more #[must_use] from portable-simd)

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit that referenced this pull request

Nov 12, 2025

Rollup merge of #148080 - GuillaumeGomez:fix-jump-def-links, r=lolbinarycat

[rustdoc] Fix invalid jump to def macro link generation

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

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request

Nov 12, 2025

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-rustdoc

Relevant to the rustdoc team, which will review and decide on the PR/issue.

T-rustdoc-frontend

Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.