-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
rustdoc: Replace FakeDefId
with new ItemId
type
#86644
rustdoc: Replace FakeDefId
with new ItemId
type
#86644
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
r? @jyn514 |
This comment has been minimized.
This comment has been minimized.
506b701
to
7a46ba3
Compare
This comment has been minimized.
This comment has been minimized.
f97138e
to
ea66dd9
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this a lot, thanks for doing this! I don't really have any feedback that hasn't been brought up already aside from the boxing thing
ea66dd9
to
6ed4d11
Compare
This comment has been minimized.
This comment has been minimized.
r? @CraftSpider - this looks great other than the JSON IDs. I'm fine with reverting the "Box ImplId" commit. |
This should be ready now! r? @CraftSpider |
This comment has been minimized.
This comment has been minimized.
772408e
to
64e84c3
Compare
@@ -96,7 +96,7 @@ impl<'a, 'tcx> BlanketImplFinder<'a, 'tcx> { | |||
name: None, | |||
attrs: Default::default(), | |||
visibility: Inherited, | |||
def_id: ItemId::Blanket { trait_: trait_def_id, for_: item_def_id }, | |||
def_id: ItemId::Blanket { impl_id: impl_def_id, for_: item_def_id }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was unclear: you don't need for_
at all now, you can reconstruct it from the impl_id alone with tcx.impl_trait_ref(impl_def_id).unwrap().self_ty()
.
def_id: ItemId::Blanket { impl_id: impl_def_id, for_: item_def_id }, | |
def_id: ItemId::Blanket { impl_def_id, }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I do need it, because apparently, only the implementation id is not enough to be unique (?????????).
When I only had the impl_id
, I got a panic when documenting std that assert_eq
on these two failed:
left: `Item { id: Id("b:0:2454"), crate_id: 0, name: None, span: Some(Span { filename: "library/core/src/borrow.rs", begin: (208, 0), end: (213, 1) }), visibility: Default, docs: None, links: {}, attrs: [], deprecation: None, inner: Impl(Impl { is_unsafe: false, generics: Generics { params: [GenericParamDef { name: "T", kind: Type { bounds: [], default: None } }], where_predicates: [BoundPredicate { ty: Generic("T"), bounds: [TraitBound { trait_: ResolvedPath { name: "Sized", id: Id("0:2854"), args: Some(AngleBracketed { args: [], bindings: [] }), param_names: [] }, generic_params: [], modifier: Maybe }] }] }, provided_trait_methods: [], trait_: Some(ResolvedPath { name: "Borrow", id: Id("0:2448"), args: Some(AngleBracketed { args: [Type(Generic("T"))], bindings: [] }), param_names: [] }), for_: ResolvedPath { name: "Wrapping", id: Id("0:24138"), args: Some(AngleBracketed { args: [Type(Generic("T"))], bindings: [] }), param_names: [] }, items: [Id("0:2456")], negative: false, synthetic: false, blanket_impl: Some(Generic("T")) }) }`,
right: `Item { id: Id("b:0:2454"), crate_id: 0, name: None, span: Some(Span { filename: "library/core/src/borrow.rs", begin: (208, 0), end: (213, 1) }), visibility: Default, docs: None, links: {}, attrs: [], deprecation: None, inner: Impl(Impl { is_unsafe: false, generics: Generics { params: [GenericParamDef { name: "T", kind: Type { bounds: [], default: None } }], where_predicates: [BoundPredicate { ty: Generic("T"), bounds: [TraitBound { trait_: ResolvedPath { name: "Sized", id: Id("0:2854"), args: Some(AngleBracketed { args: [], bindings: [] }), param_names: [] }, generic_params: [], modifier: Maybe }] }] }, provided_trait_methods: [], trait_: Some(ResolvedPath { name: "Borrow", id: Id("0:2448"), args: Some(AngleBracketed { args: [Type(Generic("T"))], bindings: [] }), param_names: [] }), for_: ResolvedPath { name: "ParseFloatError", id: Id("0:23084"), args: Some(AngleBracketed { args: [], bindings: [] }), param_names: [] }, items: [Id("0:2456")], negative: false, synthetic: false, blanket_impl: Some(Generic("T")) }) }`', src/librustdoc/json/mod.rs:179:17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... @CraftSpider why are those separate? I would only expect there to be one item per blanket impl, generating a separate item for every type it could apply to seems both inefficient and not particularly useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no clue, I'd expect the same. I'll look into the code tomorrow.
First guess: the equivalent of HTML's 'implements' showing blankets that apply to that type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we wait with this PR until you found a solution? We can also merge this now and make a follow up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yeah this seems fine in the meantime. r=me once you fix the conflict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with Josh, this isn't a blocking concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright. @CraftSpider will you open the follow-up PR with the fix for not emitting duplicate blanket implementations and the fixed ItemId
?
☔ The latest upstream changes (presumably #86282) made this pull request unmergeable. Please resolve the merge conflicts. |
…s size" This reverts commit 41a345d4c46dad1a98c9993bc78513415994e8ba.
c2f403b
to
a89912c
Compare
r? @jyn514 |
@bors r+ |
📌 Commit a89912c has been approved by |
☀️ Test successful - checks-actions |
Follow up from #84707
@Manishearth suggested that there should be a new
ItemId
type that can distinguish between auto traits, normal ids, and blanket impls instead of usingFakeDefId
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 onItem
does not really make sense and maybe the methods onItemId
should be renamed too?Also, we need to represent the new item ids in the JSON backend somehow.