Skip to content

Split traits by crate-local and crate-non-local #130418

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MeetThePatel
Copy link

This PR adds the feature requested by #124826.

Typically, rustdoc generates a list of trait-impls sorted alphabetically. However, in situations where a large number of traits are implemented for a struct/enum, it may be helpful to see the crate-local traits separate from the crate-nonlocal traits.

As a toy example, consider the following code:

pub trait TestTrait {
    fn plus_one(&mut self);
}

pub trait TestTrait2 {
    fn minus_one(&mut self);
}


#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
pub struct TestStruct {
    val: i32
}

impl std::fmt::Display for TestStruct {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "{}", self.val)
    }
}

impl TestTrait for TestStruct {
    fn plus_one (&mut self) {
        self.val += 1;
    }
}
impl TestTrait2 for TestStruct {
    fn minus_one (&mut self) {
        self.val -= 1;
    }
}

The current behaviour of rustdoc is to generate the following:
before

This PR changes this behaviour to the following:
after

@rustbot
Copy link
Collaborator

rustbot commented Sep 15, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @notriddle (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot 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. labels Sep 15, 2024
@rust-log-analyzer

This comment has been minimized.

@workingjubilee workingjubilee changed the title Split traits by crate-local and crate-non-local (Closes #124826) Split traits by crate-local and crate-non-local Sep 15, 2024
@workingjubilee
Copy link
Member

@MeetThePatel something like "closes #124826" is something you should add to the PR message not to the title, thank you!

@notriddle
Copy link
Contributor

I have a bunch of questions about how this is supposed to work...

  • How does this behave when a trait is inlined from another crate? Does it count as the crate that originally defined it, or the crate that holds its documentation?
  • How does this relate to the auto / blanket impl sections? Should there be separate "External Blanket Traits / Crate Blanket Traits" sections? I don't see them in your screenshot, but why not?
  • There's an existing feature that sort of mirrors this, where "Foreign Types" are listed on the trait pages. We had to special-case the standard library to simplify things for new users. Should we do the same thing here?
  • Maybe it should be called "Foreign Trait Implementations"?

But the most important thing to do, I think, is to figure out what problem this is trying to solve and how well it solves it. The original issue said this:

When we generate the documentation for a crate, all the crate implementations are sorted alphabetically. The crate's own traits and the std traits are all mixed together.

And it screenshotted a page that had only trait implementations from winsafe itself and from the standard library. I understand why splitting them up would be an improvement here.

But it doesn't work everywhere. For example, gtk is divided into a core crate (glib) and a facade crate (gtk itself). Its widgets implement a bunch of traits, and all of them come from glib, which means all of the impls are external. It also has a bunch of blanket impls from glib that would benefit from being split up, just like the manual impls are.

Which alternatives might work for gtk, and how do they compare to the crate-local vs crate-non-local design? For example, dividing the impls up by the crate itself (so std and glib both get their own sections), or specifically dividing standard library traits from all others (so Standard Library traits get a separate section from other traits). This is why I'd like to be more specific about the problem being solved: so that we can compare several different solutions and come up with the best one.

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 17, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 17, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust.git master
$ git push --force-with-lease

The following commits are merge commits:

@rust-log-analyzer

This comment has been minimized.

@MeetThePatel MeetThePatel force-pushed the feature-rustdoc-sort-traits branch from 8d3100e to 303ae8d Compare September 18, 2024 20:04
@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 18, 2024
@notriddle
Copy link
Contributor

Does anybody else on @rust-lang/rustdoc-frontend think this should be merged? I’m not so sure, because it seems like an incomplete solution. It makes winsafe better, but gtk worse.

Assuming I understand correctly what problem this is trying to solve.

@GuillaumeGomez
Copy link
Member

I'm also not sure it's a better than what we currently have. I tend to think it's a tiny bit better than currently, but crates like gtk (or bevy I assume) would likely be impacted negatively.

@rust-log-analyzer
Copy link
Collaborator

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

Click to see the possible cause of the failure (guessed by this bot)
------
 > importing cache manifest from ghcr.io/rust-lang/rust-ci-cache:fb97a1d6377f6cf2227825318ca4bbde3889e0c420746f5a03ba46a07e9a862725c26a09b9fc49a0d129ebd75935d3f6cd19acf41cc4267a6846fd4aa574b12c:
------
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-17]
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-17', '--enable-llvm-link-shared', '--set', 'rust.randomize-layout=true', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-17/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.randomize-layout := True
configure: rust.thin-lto-import-instr-limit := 10
---
  Downloaded boml v0.3.1
   Compiling boml v0.3.1
   Compiling y v0.1.0 (/checkout/compiler/rustc_codegen_gcc/build_system)
    Finished `release` profile [optimized] target(s) in 3.83s
     Running `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-codegen/x86_64-unknown-linux-gnu/release/y test --use-system-gcc --use-backend gcc --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/cg_gcc --release --mini-tests --std-tests`
Using system GCC
[BUILD] example
[AOT] mini_core_hello_world
/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/cg_gcc/mini_core_hello_world
abc
---
---- [rustdoc] tests/rustdoc/assoc-type-source-link.rs stdout ----

error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/assoc-type-source-link" "/checkout/tests/rustdoc/assoc-type-source-link.rs"
--- stderr -------------------------------
22: has check failed
22: has check failed
 `XPATH PATTERN` did not match
     //@ has - '//*[@id="trait-implementations-list"]//*[@id="associatedtype.Z"]/a' 'source'
23: has check failed
 `XPATH PATTERN` did not match
     //@ has - '//*[@id="trait-implementations-list"]//*[@id="associatedtype.Z"]/a/@href' '../src/foo/assoc-type-source-link.rs.html#25'
Encountered 2 errors
------------------------------------------



---- [rustdoc] tests/rustdoc/auto-trait-negative-impl-55321.rs stdout ----

error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/auto-trait-negative-impl-55321" "/checkout/tests/rustdoc/auto-trait-negative-impl-55321.rs"
--- stderr -------------------------------
7: has check failed
7: has check failed
 `XPATH PATTERN` did not match
 //@ has - '//*[@id="trait-implementations-list"]//*[@class="impl"]//h3[@class="code-header"]' "impl !Send for A"
9: has check failed
 `XPATH PATTERN` did not match
 //@ has - '//*[@id="trait-implementations-list"]//*[@class="impl"]//h3[@class="code-header"]' "impl !Sync for A"
Encountered 2 errors
------------------------------------------



---- [rustdoc] tests/rustdoc/const-generics/add-impl.rs stdout ----

error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/const-generics/add-impl" "/checkout/tests/rustdoc/const-generics/add-impl.rs"
--- stderr -------------------------------
10: has check failed
10: has check failed
 `XPATH PATTERN` did not match
 //@ has foo/struct.Simd.html '//div[@id="trait-implementations-list"]//h3[@class="code-header"]' 'impl Add for Simd<u8, 16>'
Encountered 1 errors
------------------------------------------



---- [rustdoc] tests/rustdoc/deduplicate-glob-import-impl-21474.rs stdout ----

error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/deduplicate-glob-import-impl-21474" "/checkout/tests/rustdoc/deduplicate-glob-import-impl-21474.rs"
--- stderr -------------------------------
12: count check failed
 Expected 1 occurrences but found 0
 Expected 1 occurrences but found 0
 //@ count issue_21474/struct.What.html '//*[@id="trait-implementations-list"]//*[@class="impl"]' 1
Encountered 1 errors
------------------------------------------



---- [rustdoc] tests/rustdoc/duplicate_impls/sidebar-links-duplicate-impls-33054.rs stdout ----

error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/duplicate_impls/sidebar-links-duplicate-impls-33054" "/checkout/tests/rustdoc/duplicate_impls/sidebar-links-duplicate-impls-33054.rs"
--- stderr -------------------------------
8: count check failed
 Expected 1 occurrences but found 0
 Expected 1 occurrences but found 0
 //@ count - '//*[@id="trait-implementations-list"]//*[@class="impl"]' 1
Encountered 1 errors
------------------------------------------



---- [rustdoc] tests/rustdoc/empty-impls.rs stdout ----

error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/empty-impls" "/checkout/tests/rustdoc/empty-impls.rs"
--- stderr -------------------------------
9: has check failed
9: has check failed
 `XPATH PATTERN` did not match
 //@ has - '//div[@id="trait-implementations-list"]/*[@id="impl-EmptyTrait-for-Foo"]' 'impl EmptyTrait for Foo'
16: has check failed
 `XPATH PATTERN` did not match
 //@ has - '//div[@id="trait-implementations-list"]/details/summary/*[@id="impl-NotEmpty-for-Foo"]' 'impl NotEmpty for Foo'
Encountered 2 errors
------------------------------------------



---- [rustdoc] tests/rustdoc/impl-associated-items-order.rs stdout ----

error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/impl-associated-items-order" "/checkout/tests/rustdoc/impl-associated-items-order.rs"
--- stderr -------------------------------
33: has check failed
33: has check failed
 `XPATH PATTERN` did not match
     //@ has - '//*[@id="trait-implementations-list"]//*[@class="impl-items"]/section[2]/h4' 'type Z = u8'
36: has check failed
 `XPATH PATTERN` did not match
     //@ has - '//*[@id="trait-implementations-list"]//*[@class="impl-items"]/section[1]/h4' 'const W: u32 = 12u32'
39: has check failed
 `XPATH PATTERN` did not match
     //@ has - '//*[@id="trait-implementations-list"]//*[@class="impl-items"]/section[3]/h4' 'fn yeay()'
Encountered 3 errors
------------------------------------------



---- [rustdoc] tests/rustdoc/inline_cross/reexport-with-anonymous-lifetime-98697.rs stdout ----

error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/inline_cross/reexport-with-anonymous-lifetime-98697" "/checkout/tests/rustdoc/inline_cross/reexport-with-anonymous-lifetime-98697.rs"
--- stderr -------------------------------
16: has check failed
16: has check failed
 `XPATH PATTERN` did not match
 //@ has foo/struct.Extra.html '//div[@id="trait-implementations-list"]//h3[@class="code-header"]' 'impl MyTrait<&Extra> for Extra'
Encountered 1 errors
------------------------------------------



---- [rustdoc] tests/rustdoc/inline_cross/ret-pos-impl-trait-in-trait.rs stdout ----

error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/inline_cross/ret-pos-impl-trait-in-trait" "/checkout/tests/rustdoc/inline_cross/ret-pos-impl-trait-in-trait.rs"
--- stderr -------------------------------
18: count check failed
 Expected 1 occurrences but found 0
 Expected 1 occurrences but found 0
 //@ count - '//*[@id="trait-implementations-list"]//*[@class="impl-items"]' 1
 Expected 1 occurrences but found 0
 Expected 1 occurrences but found 0
 //@ count - '//*[@id="trait-implementations-list"]//*[@class="impl-items"]' 1
 Expected 1 occurrences but found 0
 Expected 1 occurrences but found 0
 //@ count - '//*[@id="trait-implementations-list"]//*[@class="impl-items"]' 1
Encountered 3 errors
------------------------------------------



---- [rustdoc] tests/rustdoc/negative-impl-no-items.rs stdout ----

error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/negative-impl-no-items" "/checkout/tests/rustdoc/negative-impl-no-items.rs"
--- stderr -------------------------------
10: has check failed
10: has check failed
 `XPATH PATTERN` did not match
 //@ has - '//div[@id="trait-implementations-list"]/section[@id="impl-Iterator-for-Thing"]/h3' 'impl !Iterator for Thing'
18: has check failed
 `XPATH PATTERN` did not match
 //@ has - '//div[@id="trait-implementations-list"]/details//section[@id="impl-Iterator-for-Witness"]/h3' 'impl Iterator for Witness'
Encountered 2 errors
------------------------------------------



---- [rustdoc] tests/rustdoc/negative-impl-sidebar.rs stdout ----

error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/negative-impl-sidebar" "/checkout/tests/rustdoc/negative-impl-sidebar.rs"
--- stderr -------------------------------
7: has check failed
7: has check failed
 `XPATH PATTERN` did not match
 //@ has - '//div[@class="sidebar-elems"]//h3/a[@href="#trait-implementations"]' 'Trait Implementations'
Encountered 1 errors
------------------------------------------



---- [rustdoc] tests/rustdoc/synthetic_auto/manual.rs stdout ----

error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/synthetic_auto/manual" "/checkout/tests/rustdoc/synthetic_auto/manual.rs"
--- stderr -------------------------------
5: has check failed
5: has check failed
 `XPATH PATTERN` did not match
 //@ has - '//*[@id="trait-implementations-list"]//*[@class="impl"]//h3[@class="code-header"]' 'impl<T> Send for Foo<T>'
 Expected 1 occurrences but found 0
 Expected 1 occurrences but found 0
 //@ count - '//*[@id="trait-implementations-list"]//*[@class="impl"]' 1
Encountered 2 errors
------------------------------------------



---- [rustdoc] tests/rustdoc/trait-implementations-duplicate-self-45584.rs stdout ----

error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/trait-implementations-duplicate-self-45584" "/checkout/tests/rustdoc/trait-implementations-duplicate-self-45584.rs"
--- stderr -------------------------------
9: count check failed
 Expected 1 occurrences but found 0
 Expected 1 occurrences but found 0
 //@ count - '//*[@id="trait-implementations-list"]//*[@class="impl"]' 1
 Expected 1 occurrences but found 0
 Expected 1 occurrences but found 0
 //@ count - '//*[@id="trait-implementations-list"]//*[@class="impl"]' 1
Encountered 2 errors
------------------------------------------



---- [rustdoc] tests/rustdoc/typedef.rs stdout ----

error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/typedef" "/checkout/tests/rustdoc/typedef.rs"
--- stderr -------------------------------
17: has check failed
17: has check failed
 `XPATH PATTERN` did not match
 //@ has - '//*[@class="sidebar"]//a[@href="#trait-implementations"]' 'Trait Implementations'
Encountered 1 errors
------------------------------------------


@MeetThePatel
Copy link
Author

MeetThePatel commented Sep 19, 2024

The main problem that I am trying to solve is to make more clear in the documentation the distinction between:

  • functionality of the current struct/enum that is defined outside the crate, typically in std or core (what I'll call "standardized behaviour").
  • functionality that is defined inside the crate (although in cases like GTK, like @notriddle mentioned above, I think it makes more sense to check if the trait is from the current workspace, as opposed to the current crate) (what I'll call "non-standardized behaviour").

Note: Perhaps there is better terminology than standardized/non-standardized to describe this distinction.

Motivating Example: typed-floats

I had been working with the typed-floats crate a few weeks back. Due to the fact that there are a lot of trait implementations, it can be quite a challenge to see at a glance the set of functionality that typed-floats introduces (e.g. Atan2, Copysign, DivEuclid, etc.), vs. the set of functionality that typed-floats provides compatibility for (e.g. From<f32>, Display, Default).

Essentially, "non-standardized behaviour" is encapsulating behaviour that you would need to understand first if you really want to understand how the struct/enum relates to and works within the crate. "standardized behaviour" would then encapsulate everything else.

To address some of the other concerns:

How does this behave when a trait is inlined from another crate? Does it count as the crate that originally defined it, or the crate that holds its documentation?

I tested this out in this repo. In the implementation I have, it is recognizing the re-exported trait as a foreign trait. I'm not sure if this should be the correct behaviour, as if the trait is being re-exported, it would be considered essential enough to the crate to deem as non-standardized behaviour. Would love input on this choice.

How does this relate to the auto / blanket impl sections? Should there be separate "External Blanket Traits / Crate Blanket Traits" sections? I don't see them in your screenshot, but why not?

Yes. Apologies, I missed this.

There's an existing feature that sort of mirrors this, where "Foreign Types" are listed on the trait pages. We had to special-case the standard library to simplify things for new users. Should we do the same thing here?

After going through the case of GTK like @notriddle suggested above, I think that a similar fix would be needed. When I generated GTK's docs with my PR, it didn't wind up changing much (at least in the files that I checked). The structs that I looked at were organized such that traits (internal to GTK, but external to the current trait) would be recognized as external. I don't think this makes sense, as the whole point of the suggested distinction is to separate out current-project behaviour from external behaviour. So, unless there is a way to check if Items are from the current workspace, I think the fix above will have to be done.

Maybe it should be called "Foreign Trait Implementations"?

This makes a lot more sense.

For example, dividing the impls up by the crate itself (so std and glib both get their own sections), or specifically dividing standard library traits from all others (so Standard Library traits get a separate section from other traits)

For the former, I believe that this would create too many subheadings in the sidebar, ultimately adding confusion to the reader. I think that the latter is a much better option. Again, this would have to be done using the method linked (separate std, alloc and core, and then split the remaining).

I hope this answers the concerns raised. I'd appreciate input on the proposed changes, as perhaps there is a way to, as @GuillaumeGomez mentioned, avoid negatively impacting workspace projects like GTK and Bevy.

@bors
Copy link
Collaborator

bors commented Feb 12, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants