Get rid of fake `DefId`s in rustdoc by Stupremee · Pull Request #84707 · rust-lang/rust

Stupremee · 2021-04-29T19:13:56Z

Right now there are many errors left, but I wanted to show the current state since all that is left to do is fixing the errors.

Resolves#83183

r? @jyn514

camelid · 2021-04-29T20:35:35Z

Wow, thank you for working on this! expect_real was a good idea :)

jyn514

Only got about halfway through the changes - the bit about contains_key also applies to lots of other changes.

jyn514 · 2021-05-01T13:45:51Z

Can't these be fake for blanket impls?

pubtraitA{}pubtraitB{}impl<T:B>AforT{}
I don't know, but this documents just fine

jyn514 · 2021-05-01T13:48:36Z

This isn't the same behavior - before contains_key would return false for fakes. I have a feeling this will ICE.

jyn514 · 2021-05-01T13:49:11Z

I have a feeling this will ICE.

Should paths also accept fake ids, or should we just check if it's real and only then insert it?

Given that this didn't ICE documenting libstd, I'm ok with just leaving it as-is.

jyn514 · 2021-05-01T13:51:24Z

Hmm, it seems odd to me this would be real - can't rustdoc generate links to impls? Or is that only intra-doc links?

jyn514 · 2021-05-01T15:10:09Z

@Stupremee I didn't realize when reviewing that the test suite was passing - that's probably a better indicator of what's correct than my guessing. Can you document the standard library and make sure there's no panics? And then rebase this?

Stupremee · 2021-05-01T15:12:25Z

std documents just fine. I will rebase now

Stupremee · 2021-05-01T15:41:30Z

Everything should be ready now 🎉

camelid · 2021-05-01T22:40:11Z

@bors rollup=never

This should make it easier to bisect if we get ICE reports.

bors · 2021-05-03T02:58:42Z

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

Stupremee · 2021-05-04T12:49:16Z

Should be ready to merge now. Would be good to merge soon so there are not too many conflicts coming and not too many PRs that need to be rebased on this one.

r? @jyn514

jyn514 · 2021-05-04T13:30:25Z

I'd like to figure out #84707 (comment) first, I don't like merging code when I don't understand how it works. @Manishearth @GuillaumeGomez @Aaron1011 maybe one of you have ideas?

jyn514 · 2021-05-04T14:01:38Z

r=me with Guillaume's comment addressed, this already showing I don't have a good intuition for which ids are fake and which aren't 😆 so it's super helpful!

Stupremee · 2021-05-04T14:03:24Z

In the future someone could even do a follow up PR and try to minimize the amount of fake DefId's. I might even do that now when renaming everything.

Manishearth · 2021-05-04T17:34:44Z

That's fair. To be clear, "the renaming" is creating a FakeDefId wrapper and using it everywhere, right? As I said I'm against having DefId mean something different in rustdoc

Stupremee · 2021-05-04T17:35:36Z

Yes. Thats what I pushed right now. Use FakeDefId whereever required and in some more places, and use DefId for ids that are guaranteed to be real

GuillaumeGomez

Looks good to me now, thanks!

GuillaumeGomez · 2021-05-04T18:15:58Z

We'll now be waiting for the follow-up PRs. ;)

@bors: r=jyn514,GuillaumeGomez

bors · 2021-05-04T18:15:59Z

📌 Commit b6120bf has been approved by jyn514,GuillaumeGomez

bors · 2021-05-04T20:56:52Z

⌛ Testing commit b6120bf with merge ae8b84b...

bors · 2021-05-04T23:26:01Z

☀️ Test successful - checks-actions
Approved by: jyn514,GuillaumeGomez
Pushing ae8b84b to master...

…efids, r=jyn514,GuillaumeGomez Minimize amount of fake `DefId`s used in rustdoc Follow up from rust-lang#84707, which minimizes the amount of fake defids to the smallest amount possible. Every `FakeDefId` that is now used in the rustdoc library must be preserved and can not be replaced with a normal `DefId`.

…lang#82465. (update: placated tidy) (update: rebased post PR rust-lang#84707 ) merge me

…mid, r=jyn514 rustdoc: Replace `FakeDefId` with new `ItemId` type Follow up from rust-lang#84707 `@Manishearth` [suggested](rust-lang#84707 (comment)) that there should be a new `ItemId` type that can distinguish between auto traits, normal ids, and blanket impls instead of using `FakeDefId`s. This type is introduced by this PR. There are still some `FIXME`s left, because I was unsure what the best solution for them would be. Especially the naming in general now is a bit weird right now and needs to be cleaned up. Now there are no "fake" ids so the `is_fake` method on `Item` does not really make sense and maybe the methods on `ItemId` should be renamed too? Also, we need to represent the new item ids in the JSON backend somehow.

rust-highfive assigned jyn514 Apr 29, 2021

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

Stupremee force-pushed the remove-fake-defids-in-rustdoc branch from f63da48 to b5293a5Compare April 29, 2021 19:38

This comment has been minimized.
Sign in to view

jyn514 reviewed Apr 30, 2021
View reviewed changes

Stupremee marked this pull request as ready for review May 1, 2021 12:05

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

jyn514 suggested changes May 1, 2021
View reviewed changes

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 May 1, 2021

Stupremee force-pushed the remove-fake-defids-in-rustdoc branch from 6621591 to 5a3f9c4Compare May 1, 2021 15:41

Aaron1011 reviewed May 1, 2021
View reviewed changes

Comment threadsrc/librustdoc/clean/types.rs Outdated

Aaron1011 reviewed May 1, 2021
View reviewed changes

Comment threadsrc/librustdoc/formats/cache.rs Outdated

Stupremee force-pushed the remove-fake-defids-in-rustdoc branch from 327262e to d3274e3Compare May 4, 2021 12:46

GuillaumeGomez reviewed May 4, 2021
View reviewed changes

Comment threadsrc/librustdoc/clean/types.rs Outdated

GuillaumeGomez approved these changes May 4, 2021
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 May 4, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label May 4, 2021

bors merged commit ae8b84b into rust-lang:masterMay 4, 2021

rustbot added this to the 1.54.0 milestone May 4, 2021

Stupremee mentioned this pull request May 8, 2021
Minimize amount of fake DefIds used in rustdoc #85067
Merged

pnkfelix added a commit to pnkfelix/rust that referenced this pull request Jun 10, 2021
manually crafted revert of PRrust-lang#80653, to address issuerust-…
0deba55
…lang#82465. (update: placated tidy) (update: rebased post PR rust-lang#84707 ) merge me

Mark-Simulacrum pushed a commit to Mark-Simulacrum/rust that referenced this pull request Jun 19, 2021
manually crafted revert of PRrust-lang#80653, to address issuerust-…
93d9ec7
…lang#82465. (update: placated tidy) (update: rebased post PR rust-lang#84707 ) merge me

Stupremee mentioned this pull request Jun 26, 2021
rustdoc: Replace FakeDefId with new ItemId type #86644
Merged

Stupremee deleted the remove-fake-defids-in-rustdoc branch June 26, 2021 12:03

Uh oh!

Uh oh!

Conversation

Stupremee commented Apr 29, 2021

Uh oh!

This comment has been minimized.

camelid commented Apr 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jyn514 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jyn514May 1, 2021

Choose a reason for hiding this comment

Uh oh!

StupremeeMay 1, 2021

Choose a reason for hiding this comment

Uh oh!

jyn514May 1, 2021

Choose a reason for hiding this comment

Uh oh!

jyn514May 1, 2021

Choose a reason for hiding this comment

Uh oh!

StupremeeMay 1, 2021

Choose a reason for hiding this comment

Uh oh!

jyn514May 3, 2021

Choose a reason for hiding this comment

Uh oh!

jyn514May 1, 2021

Choose a reason for hiding this comment

Uh oh!

jyn514 commented May 1, 2021

Uh oh!

Stupremee commented May 1, 2021

Uh oh!

Stupremee commented May 1, 2021

Uh oh!

Uh oh!

Uh oh!

camelid commented May 1, 2021

Uh oh!

bors commented May 3, 2021

Uh oh!

Stupremee commented May 4, 2021

Uh oh!

jyn514 commented May 4, 2021

Uh oh!

Uh oh!

jyn514 commented May 4, 2021

Uh oh!

Stupremee commented May 4, 2021

Uh oh!

Manishearth commented May 4, 2021

Uh oh!

Stupremee commented May 4, 2021

Uh oh!

GuillaumeGomez left a comment

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented May 4, 2021

Uh oh!

bors commented May 4, 2021

Uh oh!

bors commented May 4, 2021

Uh oh!

bors commented May 4, 2021

Uh oh!

Reviewers

Assignees

Labels