Ignore `reference`s in "Type::inner_def_id" #90946

GuillaumeGomez · 2021-11-16T09:23:08Z

Reopening of #90726.

As discussed on zulip, the reference page shouldn't list these implementations (since they are listed on the types and on the traits in any case). And more generally, you don't implement something on a reference but on something behind a reference. I think it's the important point.

So currently it looks like this:

With this PR, only the implementations over generics behind a reference are kept.

You can test it here.

cc @camelid

rust-highfive · 2021-11-16T09:23:12Z

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

GuillaumeGomez · 2021-11-16T09:24:12Z

That is an interesting choice of reviewer. 😆

r? @jyn514

Manishearth · 2021-11-16T16:15:10Z

With this PR, all the implementations are empty.

Wait, all of them? Or just the ones that don't make sense? Because there are a bunch of traits implemented on references generically that should be shown

GuillaumeGomez · 2021-11-16T16:18:50Z

Are you thinking about blanket implementations maybe? We ruled them out because they were displayed on the matching items. But if you feel like we need to discuss this further, we can resume the discussion on zulip.

jyn514 · 2021-11-18T14:54:47Z

@GuillaumeGomez can you please update this to only include blanket impls, like we discussed on Zulip?

GuillaumeGomez · 2021-11-18T14:56:09Z

Yes, I'll try to do it in the next days.

jyn514 · 2022-01-04T22:56:50Z

r? @Manishearth

camelid · 2022-08-02T21:27:43Z

triage: friendly ping @GuillaumeGomez :)

GuillaumeGomez · 2022-08-03T08:56:39Z

I'll try to come back to it at some point. ^^'

GuillaumeGomez · 2022-08-30T18:55:15Z

I finally came back to this and took a different approach this time: instead of removing the reference DefId, I applied the filter suggested by @Manishearth: it now only keeps implementations on fully generics types behind a reference.

I uploaded the std docs generated with this version of rustdoc here.

Manishearth

looks pretty good, would like more explainer comments

Manishearth · 2022-08-30T19:07:00Z

src/librustdoc/html/render/print_item.rs

-    render_assoc_items(w, cx, it, it.item_id.expect_def_id(), AssocItemRender::All)
+    if it.name.map(|n| n.as_str() != "reference").unwrap_or(false) {
+        render_assoc_items(w, cx, it, def_id, AssocItemRender::All);
+        return;


nit: instead of an early return we should just have an if/else

Manishearth · 2022-08-30T19:07:25Z

src/librustdoc/html/render/mod.rs

@@ -2344,9 +2355,49 @@ fn sidebar_trait(cx: &Context<'_>, buf: &mut Buffer, it: &clean::Item, t: &clean
    buf.push_str("</section>")
 }

+pub(crate) fn get_reference_impls<'a>(


nit: get_filtered_impls_for_reference, and add a doc comment saying what this function does

Manishearth · 2022-08-30T19:09:41Z

src/librustdoc/html/render/mod.rs

+) -> (Vec<&'a Impl>, Vec<&'a Impl>, Vec<&'a Impl>) {
+    let def_id = it.item_id.expect_def_id();
+    // We only want to display generic impls for the "reference" primitive type.
+    let Some(v) = shared.cache.impls.get(&def_id) else { return (Vec::new(), Vec::new(), Vec::new()) };


nit: comment "if the reference primitive is somehow not defined, exit early"

Manishearth · 2022-08-30T19:09:56Z

src/librustdoc/html/render/mod.rs

+    let def_id = it.item_id.expect_def_id();
+    // We only want to display generic impls for the "reference" primitive type.
+    let Some(v) = shared.cache.impls.get(&def_id) else { return (Vec::new(), Vec::new(), Vec::new()) };
+    let traits: Vec<_> = v.iter().filter(|i| i.inner_impl().trait_.is_some()).collect();


nit: explain what the filter is here

GuillaumeGomez · 2022-08-30T20:15:42Z

Updated and applied your comments.

Manishearth · 2022-08-30T20:54:23Z

src/librustdoc/html/render/mod.rs

@@ -2346,9 +2355,57 @@ fn sidebar_trait(cx: &Context<'_>, buf: &mut Buffer, it: &clean::Item, t: &clean
    buf.push_str("</section>")
 }

+/// Returns the filtered implementation lists for the primitive reference type. It works as follows:


nit: no need to say "it works as follows"

How about:

"Returns the list of implementations for the primitive reference type, filtering out any implementations that are on concrete or partially generic types, only keeping implementations of the form impl<T> Trait for &T

Manishearth

looks good, one comment

…ive type

GuillaumeGomez · 2022-08-31T11:31:36Z

@bors r=Manishearth

bors · 2022-08-31T11:31:38Z

📌 Commit 477b7ba has been approved by Manishearth

It is now in the queue for this repository.

…case, r=Manishearth Ignore `reference`s in "Type::inner_def_id" Fixes rust-lang#90775. Reopening of rust-lang#90726. As discussed on [zulip](https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/rendering.20for.20reference.20primitive.20doc.20page), the reference page shouldn't list these implementations (since they are listed on the types and on the traits in any case). And more generally, you don't implement something on a reference but on something behind a reference. I think it's the important point. So currently it looks like this: ![Screenshot from 2021-11-16 10-20-41](https://user-images.githubusercontent.com/3050060/141957799-57aeadc5-41f8-45f6-a4a5-33b1eca6a500.png) With this PR, only the implementations over generics behind a reference are kept. You can test it [here](https://rustdoc.crud.net/imperio/def-id-remove-weird-case/std/primitive.reference.html). cc `@camelid`

Rollup of 7 pull requests Successful merges: - rust-lang#90946 (Ignore `reference`s in "Type::inner_def_id") - rust-lang#100730 (Migrate rustc_monomorphize to use SessionDiagnostic) - rust-lang#100753 (translations(rustc_session): migrates `rustc_session` to use `SessionDiagnostic` - Pt. 1) - rust-lang#100831 (Migrate `symbol_mangling` module to new diagnostics structs) - rust-lang#101204 (rustdoc: Resugar async fn return type in `clean`, not `html`) - rust-lang#101216 (Use in-page links for sanitizer docs.) - rust-lang#101237 (fix into_iter on ZST) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

GuillaumeGomez added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Nov 16, 2021

rust-highfive assigned Mark-Simulacrum Nov 16, 2021

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

rust-highfive assigned jyn514 and unassigned Mark-Simulacrum Nov 16, 2021

jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 18, 2021

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 12, 2021

rust-highfive assigned Manishearth and unassigned jyn514 Jan 4, 2022

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 23, 2022

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 12, 2022

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 20, 2022

Clean up render_assoc_items_inner a bit

6c585fc

GuillaumeGomez force-pushed the def-id-remove-weird-case branch from 319c65e to 731749b Compare August 30, 2022 18:52

Manishearth reviewed Aug 30, 2022

View reviewed changes

GuillaumeGomez force-pushed the def-id-remove-weird-case branch 2 times, most recently from a5c773d to 2cad489 Compare August 30, 2022 20:14

Manishearth reviewed Aug 30, 2022

View reviewed changes

Manishearth approved these changes Aug 30, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

GuillaumeGomez added 2 commits August 31, 2022 11:22

Print only blanket implementations on reference primitive type

900cda2

Add regression test for implementations displayed on reference primit…

477b7ba

…ive type

GuillaumeGomez force-pushed the def-id-remove-weird-case branch from 2cad489 to 477b7ba Compare August 31, 2022 09:22

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 Aug 31, 2022

GuillaumeGomez mentioned this pull request Aug 31, 2022

Rollup of 6 pull requests #101233

Closed

RalfJung mentioned this pull request Aug 31, 2022

Rollup of 7 pull requests #101238

Merged

bors merged commit 775e969 into rust-lang:master Aug 31, 2022

rustbot added this to the 1.65.0 milestone Aug 31, 2022

GuillaumeGomez deleted the def-id-remove-weird-case branch August 31, 2022 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore `reference`s in "Type::inner_def_id" #90946

Ignore `reference`s in "Type::inner_def_id" #90946

GuillaumeGomez commented Nov 16, 2021 •

edited

Loading

rust-highfive commented Nov 16, 2021

GuillaumeGomez commented Nov 16, 2021

Manishearth commented Nov 16, 2021

GuillaumeGomez commented Nov 16, 2021

jyn514 commented Nov 18, 2021

GuillaumeGomez commented Nov 18, 2021

jyn514 commented Jan 4, 2022

camelid commented Aug 2, 2022

GuillaumeGomez commented Aug 3, 2022

GuillaumeGomez commented Aug 30, 2022

Manishearth left a comment

Manishearth Aug 30, 2022

Manishearth Aug 30, 2022

Manishearth Aug 30, 2022

Manishearth Aug 30, 2022

GuillaumeGomez commented Aug 30, 2022

Manishearth Aug 30, 2022

GuillaumeGomez Aug 31, 2022

Manishearth left a comment

This comment has been minimized.

GuillaumeGomez commented Aug 31, 2022

bors commented Aug 31, 2022

Ignore references in "Type::inner_def_id" #90946

Ignore references in "Type::inner_def_id" #90946

Conversation

GuillaumeGomez commented Nov 16, 2021 • edited Loading

rust-highfive commented Nov 16, 2021

GuillaumeGomez commented Nov 16, 2021

Manishearth commented Nov 16, 2021

GuillaumeGomez commented Nov 16, 2021

jyn514 commented Nov 18, 2021

GuillaumeGomez commented Nov 18, 2021

jyn514 commented Jan 4, 2022

camelid commented Aug 2, 2022

GuillaumeGomez commented Aug 3, 2022

GuillaumeGomez commented Aug 30, 2022

Manishearth left a comment

Choose a reason for hiding this comment

Manishearth Aug 30, 2022

Choose a reason for hiding this comment

Manishearth Aug 30, 2022

Choose a reason for hiding this comment

Manishearth Aug 30, 2022

Choose a reason for hiding this comment

Manishearth Aug 30, 2022

Choose a reason for hiding this comment

GuillaumeGomez commented Aug 30, 2022

Manishearth Aug 30, 2022

Choose a reason for hiding this comment

GuillaumeGomez Aug 31, 2022

Choose a reason for hiding this comment

Manishearth left a comment

Choose a reason for hiding this comment

This comment has been minimized.

GuillaumeGomez commented Aug 31, 2022

bors commented Aug 31, 2022

Ignore `reference`s in "Type::inner_def_id" #90946

Ignore `reference`s in "Type::inner_def_id" #90946

GuillaumeGomez commented Nov 16, 2021 •

edited

Loading