rustdoc: fix intra-link for generic trait impls #92792

mdibaiee · 2022-01-11T22:25:53Z

mdibaiee · 2022-01-11T22:28:25Z

src/librustdoc/passes/collect_intra_doc_links.rs

+                || match (impl_type.kind(), ty.kind()) {
+                    (ty::Adt(def, _), ty::Adt(tydef, _)) => {
+                        debug!("adt def_id: {:?}", def.did);
+                        def.did == tydef.did
+                    }
+                    (ty::Foreign(def_id), ty::Foreign(tydef_id)) => def_id == tydef_id,
+                    _ => false,


@camelid your intuition was right: this piece was necessary, but I was not sure at the time how to compare them, but I think this is the way: if they both having matching types, we compare the def_id alone, this allows users to write Foo without the generic type parameters, and still have it match with Foo<T>. That's my understanding.

Regarding the Foreign, I'm not sure what that means to be honest, any ideas how can I test that case too to avoid regressions again?

I think that explanation sounds right.

The Foreign is an extern type I believe (feature gate #![feature(extern_types)]; issue #43467).

@camelid I actually realised there are already tests for Foreign, and because the ty::Foreign only has one field, the def_id, doing a == covers what we are doing. The tests are passing without this case in the match. I removed them.

https://github.com/rust-lang/rust/blob/master/src/test/rustdoc/intra-doc/extern-type.rs

That test doesn't test trait impls on extern types, though. Can you please add a test case for that?

It's true that TyKind has derived Eq, but Ty manually implements it and uses pointer equality (because Tys are interned).

What I was asking is why it's okay to skip manually equating Foreign but not Adt.

@camelid ah I see. I'm not sure then... I suppose we have the same question about all the other types, not just Foreign

Oh 🤦

The reason it's necessary for Adts is because we want to ignore generics since the ty we're matching against may just have placeholders (or similar). Can you add a comment about that?

yes... that's what I meant with my initial comment on this piece #92792 (comment), I didn't get my message through well 😁

Yeah, I think I read your message and understood partially but then got confused later 😆

src/test/rustdoc/intra-doc/generic-trait-impl.rs

src/librustdoc/passes/collect_intra_doc_links.rs

src/test/rustdoc/intra-doc/generic-trait-impl.rs

src/librustdoc/passes/collect_intra_doc_links.rs

camelid

Thanks!

camelid · 2022-01-13T20:24:58Z

@bors r+

bors · 2022-01-13T20:24:59Z

📌 Commit ae20500 has been approved by camelid

mdibaiee · 2022-01-13T20:25:27Z

@camelid thanks for your review!

camelid · 2022-01-13T20:26:51Z

Beta-nominating because I believe the regression is on beta now.

…cs, r=camelid rustdoc: fix intra-link for generic trait impls fixes rust-lang#92662 r? `@camelid`

…cs, r=camelid rustdoc: fix intra-link for generic trait impls fixes rust-lang#92662 r? ``@camelid``

…cs, r=camelid rustdoc: fix intra-link for generic trait impls fixes rust-lang#92662 r? ```@camelid```

…cs, r=camelid rustdoc: fix intra-link for generic trait impls fixes rust-lang#92662 r? ````@camelid````

…askrgr Rollup of 10 pull requests Successful merges: - rust-lang#92487 (Fix unclosed boxes in pretty printing of TraitAlias) - rust-lang#92581 (ARMv6K Horizon - Enable default libraries) - rust-lang#92619 (Add diagnostic items for macros) - rust-lang#92635 (rustdoc: Yet more intra-doc links cleanup) - rust-lang#92646 (feat: rustc_pass_by_value lint attribute) - rust-lang#92706 (Clarify explicitly that BTree{Map,Set} are ordered.) - rust-lang#92710 (Include Projections when elaborating TypeOutlives) - rust-lang#92746 (Parse `Ty?` as `Option<Ty>` and provide structured suggestion) - rust-lang#92792 (rustdoc: fix intra-link for generic trait impls) - rust-lang#92814 (remove unused FIXME) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

pnkfelix · 2022-01-20T15:27:17Z

discussed in compiler triage meeting. backport approved.

…r=Mark-Simulacrum [beta] Fix CVE-2022-21658 See https://blog.rust-lang.org/2022/01/20/cve-2022-21658.html. Patches reviewed by `@m-ou-se.` Also backports: * resolve rustfmt issue with generated files rust-lang#92912 * rustdoc: fix intra-link for generic trait impls rust-lang#92792 * Fix rust logo style rust-lang#92764 r? `@ghost`

rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 11, 2022

rust-highfive assigned camelid Jan 11, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 11, 2022

mdibaiee commented Jan 11, 2022

View reviewed changes

camelid reviewed Jan 11, 2022

View reviewed changes

src/test/rustdoc/intra-doc/generic-trait-impl.rs Outdated Show resolved Hide resolved

camelid reviewed Jan 11, 2022

View reviewed changes

src/librustdoc/passes/collect_intra_doc_links.rs Show resolved Hide resolved

camelid reviewed Jan 11, 2022

View reviewed changes

src/librustdoc/passes/collect_intra_doc_links.rs Outdated Show resolved Hide resolved

camelid 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 Jan 11, 2022

mdibaiee force-pushed the 92662/fix-intra-doc-generics branch from 38af78e to fb89bf0 Compare January 12, 2022 08:30

GuillaumeGomez approved these changes Jan 12, 2022

View reviewed changes

mdibaiee force-pushed the 92662/fix-intra-doc-generics branch 2 times, most recently from a647025 to 4a94f5f Compare January 13, 2022 11:23

camelid reviewed Jan 13, 2022

View reviewed changes

src/test/rustdoc/intra-doc/generic-trait-impl.rs Show resolved Hide resolved

mdibaiee force-pushed the 92662/fix-intra-doc-generics branch from 4a94f5f to efdf119 Compare January 13, 2022 19:47

camelid reviewed Jan 13, 2022

View reviewed changes

src/librustdoc/passes/collect_intra_doc_links.rs Show resolved Hide resolved

mdibaiee force-pushed the 92662/fix-intra-doc-generics branch from efdf119 to 5138606 Compare January 13, 2022 20:15

mdibaiee added 2 commits January 13, 2022 20:23

rustdoc: fix intra-link for generic trait impls

9ff8ae0

rustdoc: add intra-doc trait impl test for extern types

ae20500

mdibaiee force-pushed the 92662/fix-intra-doc-generics branch from 5138606 to ae20500 Compare January 13, 2022 20:23

camelid approved these changes Jan 13, 2022

View reviewed changes

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 Jan 13, 2022

camelid added A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name beta-nominated Nominated for backporting to the compiler in the beta channel. labels Jan 13, 2022

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 15, 2022

Rollup merge of rust-lang#92792 - mdibaiee:92662/fix-intra-doc-generi…

e0c04b6

…cs, r=camelid rustdoc: fix intra-link for generic trait impls fixes rust-lang#92662 r? `@camelid`

matthiaskrgr mentioned this pull request Jan 15, 2022

Rollup of 8 pull requests #92925

Closed

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 15, 2022

Rollup merge of rust-lang#92792 - mdibaiee:92662/fix-intra-doc-generi…

dfb31e2

…cs, r=camelid rustdoc: fix intra-link for generic trait impls fixes rust-lang#92662 r? ``@camelid``

matthiaskrgr mentioned this pull request Jan 15, 2022

Rollup of 8 pull requests #92926

Closed

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 15, 2022

Rollup merge of rust-lang#92792 - mdibaiee:92662/fix-intra-doc-generi…

f0b7d8f

…cs, r=camelid rustdoc: fix intra-link for generic trait impls fixes rust-lang#92662 r? ```@camelid```

matthiaskrgr mentioned this pull request Jan 15, 2022

Rollup of 7 pull requests #92931

Closed

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 15, 2022

Rollup merge of rust-lang#92792 - mdibaiee:92662/fix-intra-doc-generi…

91aaee4

…cs, r=camelid rustdoc: fix intra-link for generic trait impls fixes rust-lang#92662 r? ````@camelid````

matthiaskrgr mentioned this pull request Jan 15, 2022

Rollup of 7 pull requests #92934

Closed

matthiaskrgr mentioned this pull request Jan 16, 2022

Rollup of 10 pull requests #92970

Merged

bors merged commit 682ad02 into rust-lang:master Jan 16, 2022

rustbot added this to the 1.60.0 milestone Jan 16, 2022

mdibaiee deleted the 92662/fix-intra-doc-generics branch January 16, 2022 20:35

pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jan 20, 2022

Mark-Simulacrum mentioned this pull request Jan 20, 2022

[beta] Fix CVE-2022-21658 #93111

Merged

Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 20, 2022

Mark-Simulacrum modified the milestones: 1.60.0, 1.59.0 Jan 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: fix intra-link for generic trait impls #92792

rustdoc: fix intra-link for generic trait impls #92792

mdibaiee commented Jan 11, 2022

mdibaiee Jan 11, 2022

camelid Jan 11, 2022

mdibaiee Jan 12, 2022

mdibaiee Jan 12, 2022

camelid Jan 12, 2022

camelid Jan 13, 2022

mdibaiee Jan 13, 2022

camelid Jan 13, 2022

mdibaiee Jan 13, 2022 •

edited

Loading

camelid Jan 13, 2022

camelid left a comment

camelid commented Jan 13, 2022

bors commented Jan 13, 2022

mdibaiee commented Jan 13, 2022

camelid commented Jan 13, 2022 •

edited

Loading

pnkfelix commented Jan 20, 2022

rustdoc: fix intra-link for generic trait impls #92792

rustdoc: fix intra-link for generic trait impls #92792

Conversation

mdibaiee commented Jan 11, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdibaiee Jan 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camelid left a comment

Choose a reason for hiding this comment

camelid commented Jan 13, 2022

bors commented Jan 13, 2022

mdibaiee commented Jan 13, 2022

camelid commented Jan 13, 2022 • edited Loading

pnkfelix commented Jan 20, 2022

mdibaiee Jan 13, 2022 •

edited

Loading

camelid commented Jan 13, 2022 •

edited

Loading