Skip to content
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

Get rid of fake DefIds in rustdoc #83183

Closed
jyn514 opened this issue Mar 16, 2021 · 3 comments · Fixed by #84707
Closed

Get rid of fake DefIds in rustdoc #83183

jyn514 opened this issue Mar 16, 2021 · 3 comments · Fixed by #84707
Assignees
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@jyn514
Copy link
Member

jyn514 commented Mar 16, 2021

Right now, rustdoc has a whole mess of awful hacks to be able to add DefIds for things that aren't definitions:

rust/src/librustdoc/core.rs

Lines 131 to 140 in 4c10c84

/// Create a new "fake" [`DefId`].
///
/// This is an ugly hack, but it's the simplest way to handle synthetic impls without greatly
/// refactoring either rustdoc or [`rustc_middle`]. In particular, allowing new [`DefId`]s
/// to be registered after the AST is constructed would require storing the [`DefId`] mapping
/// in a [`RefCell`], decreasing the performance for normal compilation for very little gain.
///
/// Instead, we construct "fake" [`DefId`]s, which start immediately after the last `DefId`.
/// In the [`Debug`] impl for [`clean::Item`], we explicitly check for fake `DefId`s,
/// as we'll end up with a panic if we use the `DefId` `Debug` impl for fake `DefId`s.

It's unfortunate that these are needed at all, but it's really unfortunate that these have the same type as normal DefIds - they are not valid and cannot be passed to rustc APIs; trying to do so will usually panic. It also leads to lots of other hacks, like giving primitives DefIds: #83083 (comment)

A better alternative is to add a new wrapper type, something like

// clean/type.rs
enum DefId {
  Real(rustc_span::def_id::DefId),
  Fake(usize),
}

That makes it clear that the two are very different, and doesn't require hacks in rustdoc or rustc.

Things that can be removed once this is implemented:

@jyn514 jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Mar 16, 2021
@camelid
Copy link
Member

camelid commented Mar 21, 2021

Maybe a better solution would be to just get rid of fake IDs altogether? It seems like they're only used for creating monomorphized blanket impls and auto trait impls? Maybe we could remove fake IDs and instead use a new wrapper around ItemKind that doesn't require a DefId or something like that.

@jyn514
Copy link
Member Author

jyn514 commented Mar 21, 2021

@camelid I don't have any clear idea of what that could look like - I don't mind you experimenting with it, but I think it would be much harder than just making 'fake DefId' part of the type system.

@camelid
Copy link
Member

camelid commented Mar 21, 2021

I don't have any clear idea of what that could look like

Yeah, I don't have a clear idea either ;)

I think it would be much harder than just making 'fake DefId' part of the type system.

Maybe, though I think both would be very hard. At some point I think there's not much of a difference between "very hard" and "harder than very hard".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants