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

rustdoc: Semantically disambiguate impls and associated impl items rather than using numeric suffixes #92052

Closed
camelid opened this issue Dec 17, 2021 · 8 comments
Assignees
Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) C-enhancement Category: An issue proposing an enhancement or a PR with one. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@camelid
Copy link
Member

camelid commented Dec 17, 2021

Problem

Links to impls and their associated items are currently unstable in the sense that the addition of other items causes pre-existing items to have different URL fragments. This is because the URL fragments are only disambiguated by suffixing a number. If more items are added, the suffix changes. In addition, the suffix is not very human-understandable; it's hard to know which suffix will be chosen by rustdoc.

Proposed changes

Concerns and unresolved questions

@camelid camelid added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-ui Area: Rustdoc UI (generated HTML) labels Dec 17, 2021
@pierwill
Copy link
Member

pierwill commented Jan 3, 2022

I'm interested in working on this, but I might not be able to get to it for a few weeks. If someone else is interested in taking this on, feel free to ping me here. :) In the meantime, @rustbot claim

@pierwill
Copy link
Member

Is this the relevant code, @camelid?

https://github.com/rust-lang/rust/blob/ec59f5aa3c4adf43089d57b17bf42775b89bb710/src/librustdoc/html/format.rs#L991-L1023

(I've never worked on rustdoc before, so this should be fun!)

@camelid
Copy link
Member Author

camelid commented Jan 10, 2022

Hmm, I don't think that's it. I think the relevant code is at the top of render_impl_summary:

    let id = cx.derive_id(match i.inner_impl().trait_ {
        Some(ref t) => {
            if is_on_foreign_type {
                get_id_for_impl_on_foreign_type(&i.inner_impl().for_, t, cx)
            } else {
                format!("impl-{}", small_url_encode(format!("{:#}", t.print(cx))))
            }
        }
        None => "impl".to_string(),
    });

though there's also get_id_for_impl_on_foreign_type, which uses a form of semantic disambiguation that you may want to re-use.

The cx.derive_id() part, when applied to "impl", will return "impl" the first time, then "impl-1", "impl-2", etc.

@pierwill
Copy link
Member

pierwill commented Jan 10, 2022

Could we do something as naive as this, as a starting point?

     aliases: &[String],
 ) {
     let id = cx.derive_id(match i.inner_impl().trait_ {
-        Some(ref t) => {
-            if is_on_foreign_type {
-                get_id_for_impl_on_foreign_type(&i.inner_impl().for_, t, cx)
-            } else {
-                format!("impl-{}", small_url_encode(format!("{:#}", t.print(cx))))
-            }
-        }
+        Some(ref t) => get_id_for_impl(&i.inner_impl().for_, t, cx),
         None => "impl".to_string(),
     });
     let aliases = if aliases.is_empty() {
@@ -2194,7 +2188,7 @@ fn sidebar_struct(cx: &Context<'_>, buf: &mut Buffer, it: &clean::Item, s: &clea
     }
 }
 
-fn get_id_for_impl_on_foreign_type(
+fn get_id_for_impl(
     for_: &clean::Type,
     trait_: &clean::Path,
     cx: &Context<'_>,
@@ -2210,7 +2204,7 @@ fn extract_for_impl_name(item: &clean::Item, cx: &Context<'_>) -> Option<(String
                 // so this parameter does nothing.
                 (
                     format!("{:#}", i.for_.print(cx)),
-                    get_id_for_impl_on_foreign_type(&i.for_, trait_, cx),
+                    get_id_for_impl(&i.for_, trait_, cx),
                 )
             })
         }

Except we do want to know about foreign types, still...

@camelid
Copy link
Member Author

camelid commented Jan 10, 2022

Could we do something as naive as this, as a starting point?

Perhaps. It's hard to know without seeing the results.

Except we do want to know about foreign types, still...

I'm not sure I understand what you mean.

@pierwill
Copy link
Member

I'm afraid I'm too far removed from this code now to see this across the finish line...

@pierwill
Copy link
Member

@GuillaumeGomez : Can this be closed now that #98939 is merged?

@GuillaumeGomez
Copy link
Member

It can indeed! Thanks for notifying.

nars1 pushed a commit to YottaDB/YDBRust that referenced this issue Sep 29, 2022
**Change**
* Pipeline job `pages` started failing because of the error shown below
  ```
  Found invalid urls in struct.Context.html:
	  Fragment #impl-Display at struct.YDBError.html does not exist!
  ```
* This error is from execution of `cargo deadlinks` command
* This error is due to the fact that implementation of trait `fmt::Display` for `YDBError` was not found at fragment `impl-Display` of `struct.YDBError.html`
* This error started occuring with the `rust version 1.64.0 (a55dd71d5 2022-09-19)` and not before. The previous version as seen in pipeline jobs previous to this failure is `1.63.0 (4b91a6ea7 2022-08-08)`
* When attempted to generate the docs locally it was found that the documentation for `Display` with the new rust version (1.64.0) was seen at fragment `impl-Display-for-YDBError`
* Changing the fragment part in rustdoc solved the issue
* The following pull request seems to have introduced this change to rustdoc rust-lang/rust#98939 (rustdoc: Add more semantic information to impl IDs #98939) and this is the issue on github related to this change -> rust-lang/rust#92052 (rustdoc: Semantically disambiguate impls and associated impl items rather than using numeric suffixes #92052)

**Misc**
* @jsikri94 found another issue where the intra-doc link format was broken. This change also fixed the issue.
  * Previous to this change following line was seen in docs : `ci_t! and cip_t! cip_t!: crate::cip_t! ci_t!: crate::ci_t!`
  * After this change it is replaced by : `ci_t! and cip_t!`
  * The fix was to include an empty line between the intra link and its definition. As it was absent both the lines were being combined into one as seen above in the first sub-point. With the empty line the line seen in docs is fixed and is seen as shown by the second sub-point above.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) C-enhancement Category: An issue proposing an enhancement or a PR with one. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants