fix dead link for method in trait of blanket impl from third party crate #86662

mockersf · 2021-06-27T09:35:32Z

changes href method to raise the actual error it had instead of an Option
set the href link correctly in case of an error

I did not manage to make a small reproducer, I think it happens in a situation where

crate A expose a trait with a blanket impl
crate B use the trait from crate A
crate C use types from crate B
building docs for crate C without dependencies

jyn514

I would like to find a test for the other crate before merging this; I'll try to do that this afternoon. I don't yet understand why your fix works.

src/librustdoc/html/render/mod.rs

jyn514 · 2021-06-27T12:30:12Z

src/librustdoc/html/format.rs

@@ -456,15 +456,26 @@ impl clean::GenericArgs {
    }
 }

-crate fn href(did: DefId, cx: &Context<'_>) -> Option<(String, ItemType, Vec<String>)> {
+// Possible errors when computing href link source for a `DefId`
+crate enum HrefError {


This would help quite a lot with #82014 I think :) good idea.

(So I remember: the perma-link for this is 8f19606)

jyn514 · 2021-07-02T00:20:53Z

Can you add the MCVE from the issue as a test? I'll review this in the meantime and see if I understand it better now.

jyn514

This is not the right fix, see below.

src/librustdoc/html/format.rs

jyn514 · 2021-07-02T00:28:04Z

src/librustdoc/html/format.rs

+    NotInExternalCache,
+}
+
+crate fn href(did: DefId, cx: &Context<'_>) -> Result<(String, ItemType, Vec<String>), HrefError> {
    let cache = &cx.cache();
    let relative_to = &cx.current;
    fn to_module_fqp(shortty: ItemType, fqp: &[String]) -> &[String] {
        if shortty == ItemType::Module { &fqp[..] } else { &fqp[..fqp.len() - 1] }
    }

    if !did.is_local() && !cache.access_levels.is_public(did) && !cache.document_private {


So, !is_local looks correct to me, but for a non-trivial reason - the strip_private pass should get rid of private local items before they ever get here. Can you change this to an assert instead?

Suggested change

if !did.is_local() && !cache.access_levels.is_public(did) && !cache.document_private {

if did.is_local() {

// Private items should have already been stripped.

assert!(cache.access_levels.is_public(did) || cache.document_private);

}

if !cache.access_levels.is_public(did) && !cache.document_private {

(I know this is unrelated to your change - I can make a separate PR for this if you like.)

adding this assert made the following tests fail:

failures: [rustdoc] rustdoc/all.rs [rustdoc] rustdoc/async-fn.rs [rustdoc] rustdoc/blanket-reexport-item.rs [rustdoc] rustdoc/impl-trait-alias.rs [rustdoc] rustdoc/inline_cross/renamed-via-module.rs [rustdoc] rustdoc/inline_local/glob-private-no-defaults.rs [rustdoc] rustdoc/inline_local/glob-private.rs [rustdoc] rustdoc/inline_local/issue-28537.rs [rustdoc] rustdoc/inline_local/issue-32343.rs [rustdoc] rustdoc/inline_local/trait-vis.rs [rustdoc] rustdoc/intra-doc/cross-crate/hidden.rs [rustdoc] rustdoc/issue-34473.rs [rustdoc] rustdoc/issue-35488.rs [rustdoc] rustdoc/issue-46767.rs [rustdoc] rustdoc/namespaces.rs [rustdoc] rustdoc/redirect-map.rs [rustdoc] rustdoc/redirect-rename.rs [rustdoc] rustdoc/redirect.rs [rustdoc] rustdoc/search-index.rs [rustdoc] rustdoc/struct-arg-pattern.rs [rustdoc] rustdoc/synthetic_auto/complex.rs [rustdoc] rustdoc/synthetic_auto/project.rs

ahhhh of course it did :( I'll open an issue

Oh, I was just confused: the proper assert is

assert!(cache.access_levels.is_public(did) || cache.document_private || cache.paths.get(&did).is_none(), "{:?}", did);

well, there was a bug after all: #87048

src/librustdoc/html/format.rs

jyn514 · 2021-07-02T00:35:35Z

src/librustdoc/html/render/mod.rs

+                match href(did.expect_real(), cx) {
+                    Ok(p) => Some(format!("{}#{}.{}", p.0, ty, name)),
+                    Err(HrefError::UnknownLocation) => None,
+                    Err(_) => Some(format!("#{}.{}", ty, name)),
+                }


This does not look like the right fix. I expect if you change cargo doc --no-deps to cargo doc you'll still run into the same issue (you can test with cargo deadlinks --dir target/doc).

The proper fix is probably to use #tymethod for external methods and #method for local ones? Something about the if on line 881 looks wrong, I'm not sure exactly what.

I tried building docs with and without dependencies for serde-encrypt, no deadlinks on vzip.

Yes, #method is for the link on the local method, and is available on the mouse over anchor. #tymethod would be the link to the trait definition, on the function name, but it's not available here and this fix removes it.

mockersf · 2021-07-04T23:50:36Z

Can you add the MCVE from the issue as a test? I'll review this in the meantime and see if I understand it better now.

I think I added the MCVE with the correct annotations. The test is working with this PR, and not without

jyn514 · 2021-07-06T02:35:59Z

src/librustdoc/html/render/mod.rs

-                    .unwrap_or_else(|| format!("#{}.{}", ty, name))
+                match href(did.expect_real(), cx) {
+                    Ok(p) => Some(format!("{}#{}.{}", p.0, ty, name)),
+                    Err(HrefError::DocumentationNotBuilt) => None,


Ok, I think I see why I was confused - rustdoc can never be missing the documentation for the current crate, because it's building it right now.

Can you add an assert that ty == ItemType::TyMethod with a comment that TyMethods only appear for extern crates?

This assert makes this test fail (with this include).

It seems it's possible to have a ItemType::Method when reexporting a struct with a method for which doc has not been built. I changed this PR to still allow this link as it's valid

Do you happen to know why the test didn't fail without the assert? It seems to be testing that the method. link is generated - maybe it also needs to test that a tymethod. link is generated too?

As we are importing and using the struct in the current crate, we are importing its impl with its method, but the source is in the dependency and the doc is not built there...

The existing assertion was not on the link itself, but on the presence of the function in the doc. I added assertions on the links generated

jyn514 · 2021-07-10T22:34:40Z

I don't really understand why this works, but I also won't have time to look into it in the near future and the tests seem about right, so r=me with both comments addressed(#86662 (comment) and #86662 (comment)).

mockersf · 2021-07-14T22:38:45Z

Thank you for your reviews, I think I addressed the two comments

r? @jyn514

jyn514 · 2021-07-14T22:46:49Z

Looks good to me, but I want to wait for CI to pass.

@bors delegate=mockersf (please use r=jyn514)

bors · 2021-07-14T22:46:50Z

✌️ @mockersf can now approve this pull request

mockersf · 2021-07-16T00:57:41Z

@bors r=jyn514

bors · 2021-07-16T00:57:42Z

📌 Commit 450c28a has been approved by jyn514

bors · 2021-07-16T06:44:15Z

⌛ Testing commit 450c28a with merge a6470c7...

bors · 2021-07-16T09:28:16Z

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing a6470c7 to master...

rust-highfive assigned jyn514 Jun 27, 2021

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

jyn514 reviewed Jun 27, 2021

View reviewed changes

jyn514 suggested changes Jul 2, 2021

View reviewed changes

jyn514 mentioned this pull request Jul 2, 2021

rustdoc: Some items in cache.paths are non-local #86798

Open

jyn514 reviewed Jul 6, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

mockersf added 5 commits July 6, 2021 22:13

fix dead link for method in trait of blanket impl from third party crate

53447d8

add mcve as test

fc97fbe

review - better error names/doc

afb5b21

add check on anchor link

441a350

only remove link for TyMethod

a75e629

mockersf force-pushed the fix-86620-link-unknown-location branch from c466de2 to a75e629 Compare July 6, 2021 20:47

mockersf added 2 commits July 11, 2021 02:18

comment about link without a href

c6ae96d

add assertions on existing tests with missing doc source

450c28a

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 16, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 16, 2021

bors merged commit a6470c7 into rust-lang:master Jul 16, 2021

rustbot added this to the 1.55.0 milestone Jul 16, 2021

This was referenced Jul 16, 2021

Generate links to definition in rustdoc source code pages #84176

Merged

Fix rustdoc handling of primitive items #87073

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix dead link for method in trait of blanket impl from third party crate #86662

fix dead link for method in trait of blanket impl from third party crate #86662

mockersf commented Jun 27, 2021

jyn514 left a comment

jyn514 Jun 27, 2021 •

edited

Loading

jyn514 commented Jul 2, 2021

jyn514 left a comment

jyn514 Jul 2, 2021

mockersf Jul 4, 2021 •

edited

Loading

jyn514 Jul 10, 2021

jyn514 Jul 10, 2021

jyn514 Jul 11, 2021

jyn514 Jul 2, 2021

mockersf Jul 4, 2021

mockersf commented Jul 4, 2021

jyn514 Jul 6, 2021

mockersf Jul 6, 2021

jyn514 Jul 10, 2021

mockersf Jul 14, 2021

This comment has been minimized.

jyn514 commented Jul 10, 2021 •

edited

Loading

mockersf commented Jul 14, 2021

jyn514 commented Jul 14, 2021

bors commented Jul 14, 2021

mockersf commented Jul 16, 2021

bors commented Jul 16, 2021

bors commented Jul 16, 2021

bors commented Jul 16, 2021

-    if !did.is_local() && !cache.access_levels.is_public(did) && !cache.document_private {
+    if did.is_local() {
+        // Private items should have already been stripped.
+        assert!(cache.access_levels.is_public(did) || cache.document_private);
+    }
+    if !cache.access_levels.is_public(did) && !cache.document_private {

fix dead link for method in trait of blanket impl from third party crate #86662

fix dead link for method in trait of blanket impl from third party crate #86662

Conversation

mockersf commented Jun 27, 2021

jyn514 left a comment

Choose a reason for hiding this comment

jyn514 Jun 27, 2021 • edited Loading

Choose a reason for hiding this comment

jyn514 commented Jul 2, 2021

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mockersf Jul 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mockersf commented Jul 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

jyn514 commented Jul 10, 2021 • edited Loading

mockersf commented Jul 14, 2021

jyn514 commented Jul 14, 2021

bors commented Jul 14, 2021

mockersf commented Jul 16, 2021

bors commented Jul 16, 2021

bors commented Jul 16, 2021

bors commented Jul 16, 2021

jyn514 Jun 27, 2021 •

edited

Loading

mockersf Jul 4, 2021 •

edited

Loading

jyn514 commented Jul 10, 2021 •

edited

Loading