Get rid of fake `DefId`s in rustdoc #84707

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 :)

src/librustdoc/clean/types.rs

src/librustdoc/clean/utils.rs

src/librustdoc/passes/collect_trait_impls.rs

jyn514

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

src/librustdoc/clean/inline.rs

src/librustdoc/core.rs

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

src/librustdoc/formats/cache.rs

@@ -35,18  35,18 @@ crate struct Cache {
    ///
    /// The values of the map are a list of implementations and documentation
    /// found on that implementation.
-    crate impls: FxHashMap<DefId, Vec<Impl>>,
+    crate impls: FxHashMap<RealDefId, Vec<Impl>>,


Can't these be fake for blanket impls?

pub trait A {} pub trait B {} impl<T: B> A for T {}

I don't know, but this documents just fine

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

src/librustdoc/formats/cache.rs

-                if !self.cache.paths.contains_key(&item.def_id)
-                    || self.cache.access_levels.is_public(item.def_id)
+                if !self.cache.paths.contains_key(&item.def_id.expect_real())
+                    || self.cache.access_levels.is_public(item.def_id.expect_real())


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

src/librustdoc/formats/cache.rs

                {
-                    self.cache.paths.insert(item.def_id, (self.cache.stack.clone(), item.type_()));
+                    self.cache.paths.insert(
+                        item.def_id.expect_real(),


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

src/librustdoc/html/format.rs

@@ -442,7  442,7 @@ impl clean::GenericArgs {
    }
 }

-crate fn href(did: DefId, cx: &Context<'_>) -> Option<(String, ItemType, Vec<String>)> {
+crate fn href(did: RealDefId, cx: &Context<'_>) -> Option<(String, ItemType, Vec<String>)> {


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 🎉

src/librustdoc/clean/types.rs

src/librustdoc/formats/cache.rs

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?

src/librustdoc/clean/types.rs

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 b5293a5 Compare 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 5a3f9c4 Compare May 1, 2021 15:41

Aaron1011 reviewed May 1, 2021

View reviewed changes

src/librustdoc/clean/types.rs Outdated Show resolved Hide resolved

Aaron1011 reviewed May 1, 2021

View reviewed changes

src/librustdoc/formats/cache.rs Outdated Show resolved Hide resolved

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

GuillaumeGomez reviewed May 4, 2021

View reviewed changes

src/librustdoc/clean/types.rs Outdated Show resolved Hide resolved

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:master May 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 PR rust-lang#80653, to address issue rust-…

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 PR rust-lang#80653, to address issue rust-…

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get rid of fake `DefId`s in rustdoc #84707

Get rid of fake `DefId`s in rustdoc #84707

Stupremee commented Apr 29, 2021

This comment has been minimized.

camelid commented Apr 29, 2021

jyn514 left a comment

jyn514 May 1, 2021

Stupremee May 1, 2021

jyn514 May 1, 2021

jyn514 May 1, 2021

Stupremee May 1, 2021

jyn514 May 3, 2021

jyn514 May 1, 2021

jyn514 commented May 1, 2021

Stupremee commented May 1, 2021

Stupremee commented May 1, 2021

camelid commented May 1, 2021

bors commented May 3, 2021

Stupremee commented May 4, 2021

jyn514 commented May 4, 2021

jyn514 commented May 4, 2021

Stupremee commented May 4, 2021

Manishearth commented May 4, 2021

Stupremee commented May 4, 2021

GuillaumeGomez left a comment

GuillaumeGomez commented May 4, 2021

bors commented May 4, 2021

bors commented May 4, 2021

bors commented May 4, 2021

Get rid of fake DefIds in rustdoc #84707

Get rid of fake DefIds in rustdoc #84707

Conversation

Stupremee commented Apr 29, 2021

This comment has been minimized.

camelid commented Apr 29, 2021

jyn514 left a comment

Choose a reason for hiding this comment

jyn514 May 1, 2021

Choose a reason for hiding this comment

Stupremee May 1, 2021

Choose a reason for hiding this comment

jyn514 May 1, 2021

Choose a reason for hiding this comment

jyn514 May 1, 2021

Choose a reason for hiding this comment

Stupremee May 1, 2021

Choose a reason for hiding this comment

jyn514 May 3, 2021

Choose a reason for hiding this comment

jyn514 May 1, 2021

Choose a reason for hiding this comment

jyn514 commented May 1, 2021

Stupremee commented May 1, 2021

Stupremee commented May 1, 2021

camelid commented May 1, 2021

bors commented May 3, 2021

Stupremee commented May 4, 2021

jyn514 commented May 4, 2021

jyn514 commented May 4, 2021

Stupremee commented May 4, 2021

Manishearth commented May 4, 2021

Stupremee commented May 4, 2021

GuillaumeGomez left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented May 4, 2021

bors commented May 4, 2021

bors commented May 4, 2021

bors commented May 4, 2021

Get rid of fake `DefId`s in rustdoc #84707

Get rid of fake `DefId`s in rustdoc #84707