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

rustdoc: Replace FakeDefId with new ItemId type #86644

Merged
merged 7 commits into from
Jul 6, 2021
2 changes: 1 addition & 1 deletion src/librustdoc/clean/auto_trait.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> {
name: None,
attrs: Default::default(),
visibility: Inherited,
def_id: FakeDefId::new_fake(item_def_id.krate),
def_id: ItemId::Auto { trait_: trait_def_id, for_: item_def_id },
kind: box ImplItem(Impl {
span: Span::dummy(),
unsafety: hir::Unsafety::Normal,
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/clean/blanket_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ impl<'a, 'tcx> BlanketImplFinder<'a, 'tcx> {
name: None,
attrs: Default::default(),
visibility: Inherited,
def_id: FakeDefId::new_fake(item_def_id.krate),
def_id: ItemId::Blanket { impl_id: impl_def_id, for_: item_def_id },
Copy link
Member

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().

Suggested change
def_id: ItemId::Blanket { impl_id: impl_def_id, for_: item_def_id },
def_id: ItemId::Blanket { impl_def_id, },

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Contributor

@CraftSpider CraftSpider Jul 4, 2021

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

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member Author

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?

kind: box ImplItem(Impl {
span: self.cx.tcx.def_span(impl_def_id).clean(self.cx),
unsafety: hir::Unsafety::Normal,
Expand Down
7 changes: 4 additions & 3 deletions src/librustdoc/clean/inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use rustc_span::hygiene::MacroKind;
use rustc_span::symbol::{kw, sym, Symbol};

use crate::clean::{
self, utils, Attributes, AttributesExt, FakeDefId, GetDefId, NestedAttributesExt, Type,
self, utils, Attributes, AttributesExt, GetDefId, ItemId, NestedAttributesExt, Type,
};
use crate::core::DocContext;
use crate::formats::item_type::ItemType;
Expand Down Expand Up @@ -483,10 +483,11 @@ fn build_module(
}
if let Res::PrimTy(p) = item.res {
// Primitive types can't be inlined so generate an import instead.
let prim_ty = clean::PrimitiveType::from(p);
items.push(clean::Item {
name: None,
attrs: box clean::Attributes::default(),
def_id: FakeDefId::new_fake(did.krate),
def_id: ItemId::Primitive(prim_ty, did.krate),
visibility: clean::Public,
kind: box clean::ImportItem(clean::Import::new_simple(
item.ident.name,
Expand All @@ -495,7 +496,7 @@ fn build_module(
global: false,
res: item.res,
segments: vec![clean::PathSegment {
name: clean::PrimitiveType::from(p).as_sym(),
name: prim_ty.as_sym(),
args: clean::GenericArgs::AngleBracketed {
args: Vec::new(),
bindings: Vec::new(),
Expand Down
89 changes: 39 additions & 50 deletions src/librustdoc/clean/types.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::cell::{Cell, RefCell};
use std::cell::RefCell;
use std::default::Default;
use std::hash::{Hash, Hasher};
use std::iter::FromIterator;
Expand Down Expand Up @@ -48,73 +48,68 @@ use self::ItemKind::*;
use self::SelfTy::*;
use self::Type::*;

crate type FakeDefIdSet = FxHashSet<FakeDefId>;
crate type ItemIdSet = FxHashSet<ItemId>;

#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Copy)]
crate enum FakeDefId {
Real(DefId),
Fake(DefIndex, CrateNum),
#[derive(Debug, Clone, PartialEq, Eq, Hash, Copy)]
crate enum ItemId {
/// A "normal" item that uses a [`DefId`] for identification.
DefId(DefId),
/// Identifier that is used for auto traits.
Auto { trait_: DefId, for_: DefId },
/// Identifier that is used for blanket implementations.
Blanket { impl_id: DefId, for_: DefId },
/// Identifier for primitive types.
Primitive(PrimitiveType, CrateNum),
}

impl FakeDefId {
#[cfg(parallel_compiler)]
crate fn new_fake(crate: CrateNum) -> Self {
unimplemented!("")
}

#[cfg(not(parallel_compiler))]
crate fn new_fake(krate: CrateNum) -> Self {
thread_local!(static FAKE_DEF_ID_COUNTER: Cell<usize> = Cell::new(0));
let id = FAKE_DEF_ID_COUNTER.with(|id| {
let tmp = id.get();
id.set(tmp + 1);
tmp
});
Self::Fake(DefIndex::from(id), krate)
}

impl ItemId {
#[inline]
crate fn is_local(self) -> bool {
match self {
FakeDefId::Real(id) => id.is_local(),
FakeDefId::Fake(_, krate) => krate == LOCAL_CRATE,
ItemId::Auto { for_: id, .. }
| ItemId::Blanket { for_: id, .. }
| ItemId::DefId(id) => id.is_local(),
ItemId::Primitive(_, krate) => krate == LOCAL_CRATE,
}
}

#[inline]
#[track_caller]
crate fn expect_real(self) -> rustc_hir::def_id::DefId {
self.as_real().unwrap_or_else(|| panic!("FakeDefId::expect_real: `{:?}` isn't real", self))
crate fn expect_def_id(self) -> DefId {
self.as_def_id()
.unwrap_or_else(|| panic!("ItemId::expect_def_id: `{:?}` isn't a DefId", self))
}

#[inline]
crate fn as_real(self) -> Option<DefId> {
crate fn as_def_id(self) -> Option<DefId> {
match self {
FakeDefId::Real(id) => Some(id),
FakeDefId::Fake(_, _) => None,
ItemId::DefId(id) => Some(id),
_ => None,
}
}

#[inline]
crate fn krate(self) -> CrateNum {
match self {
FakeDefId::Real(id) => id.krate,
FakeDefId::Fake(_, krate) => krate,
ItemId::Auto { for_: id, .. }
| ItemId::Blanket { for_: id, .. }
| ItemId::DefId(id) => id.krate,
ItemId::Primitive(_, krate) => krate,
}
}

#[inline]
crate fn index(self) -> Option<DefIndex> {
Stupremee marked this conversation as resolved.
Show resolved Hide resolved
match self {
FakeDefId::Real(id) => Some(id.index),
FakeDefId::Fake(_, _) => None,
ItemId::DefId(id) => Some(id.index),
_ => None,
}
}
}

impl From<DefId> for FakeDefId {
impl From<DefId> for ItemId {
fn from(id: DefId) -> Self {
Self::Real(id)
Self::DefId(id)
}
}

Expand Down Expand Up @@ -338,14 +333,14 @@ crate struct Item {
/// Information about this item that is specific to what kind of item it is.
/// E.g., struct vs enum vs function.
crate kind: Box<ItemKind>,
crate def_id: FakeDefId,
crate def_id: ItemId,

crate cfg: Option<Arc<Cfg>>,
}

// `Item` is used a lot. Make sure it doesn't unintentionally get bigger.
#[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
rustc_data_structures::static_assert_size!(Item, 48);
rustc_data_structures::static_assert_size!(Item, 56);
Stupremee marked this conversation as resolved.
Show resolved Hide resolved

crate fn rustc_span(def_id: DefId, tcx: TyCtxt<'_>) -> Span {
Span::from_rustc_span(def_id.as_local().map_or_else(
Expand All @@ -359,19 +354,19 @@ crate fn rustc_span(def_id: DefId, tcx: TyCtxt<'_>) -> Span {

impl Item {
crate fn stability<'tcx>(&self, tcx: TyCtxt<'tcx>) -> Option<&'tcx Stability> {
if self.is_fake() { None } else { tcx.lookup_stability(self.def_id.expect_real()) }
self.def_id.as_def_id().and_then(|did| tcx.lookup_stability(did))
}

crate fn const_stability<'tcx>(&self, tcx: TyCtxt<'tcx>) -> Option<&'tcx ConstStability> {
if self.is_fake() { None } else { tcx.lookup_const_stability(self.def_id.expect_real()) }
self.def_id.as_def_id().and_then(|did| tcx.lookup_const_stability(did))
}

crate fn deprecation(&self, tcx: TyCtxt<'_>) -> Option<Deprecation> {
if self.is_fake() { None } else { tcx.lookup_deprecation(self.def_id.expect_real()) }
self.def_id.as_def_id().and_then(|did| tcx.lookup_deprecation(did))
}

crate fn inner_docs(&self, tcx: TyCtxt<'_>) -> bool {
if self.is_fake() { false } else { tcx.get_attrs(self.def_id.expect_real()).inner_docs() }
self.def_id.as_def_id().map(|did| tcx.get_attrs(did).inner_docs()).unwrap_or(false)
}

crate fn span(&self, tcx: TyCtxt<'_>) -> Span {
Expand All @@ -383,10 +378,8 @@ impl Item {
kind
{
*span
} else if self.is_fake() {
Span::dummy()
} else {
rustc_span(self.def_id.expect_real(), tcx)
self.def_id.as_def_id().map(|did| rustc_span(did, tcx)).unwrap_or_else(|| Span::dummy())
Stupremee marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -551,7 +544,7 @@ impl Item {
}

crate fn is_crate(&self) -> bool {
self.is_mod() && self.def_id.as_real().map_or(false, |did| did.index == CRATE_DEF_INDEX)
self.is_mod() && self.def_id.as_def_id().map_or(false, |did| did.index == CRATE_DEF_INDEX)
}
crate fn is_mod(&self) -> bool {
self.type_() == ItemType::Module
Expand Down Expand Up @@ -662,10 +655,6 @@ impl Item {
_ => false,
}
}

crate fn is_fake(&self) -> bool {
matches!(self.def_id, FakeDefId::Fake(_, _))
}
}

#[derive(Clone, Debug)]
Expand Down
11 changes: 6 additions & 5 deletions src/librustdoc/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use std::mem;
use std::rc::Rc;

use crate::clean::inline::build_external_trait;
use crate::clean::{self, FakeDefId, TraitWithExtraInfo};
use crate::clean::{self, ItemId, TraitWithExtraInfo};
use crate::config::{Options as RustdocOptions, OutputFormat, RenderOptions};
use crate::formats::cache::Cache;
use crate::passes::{self, Condition::*, ConditionalPass};
Expand Down Expand Up @@ -78,7 +78,7 @@ crate struct DocContext<'tcx> {
/// This same cache is used throughout rustdoc, including in [`crate::html::render`].
crate cache: Cache,
/// Used by [`clean::inline`] to tell if an item has already been inlined.
crate inlined: FxHashSet<FakeDefId>,
crate inlined: FxHashSet<ItemId>,
/// Used by `calculate_doc_coverage`.
crate output_format: OutputFormat,
}
Expand Down Expand Up @@ -128,12 +128,13 @@ impl<'tcx> DocContext<'tcx> {

/// Like `hir().local_def_id_to_hir_id()`, but skips calling it on fake DefIds.
/// (This avoids a slice-index-out-of-bounds panic.)
crate fn as_local_hir_id(tcx: TyCtxt<'_>, def_id: FakeDefId) -> Option<HirId> {
crate fn as_local_hir_id(tcx: TyCtxt<'_>, def_id: ItemId) -> Option<HirId> {
match def_id {
FakeDefId::Real(real_id) => {
ItemId::DefId(real_id) => {
real_id.as_local().map(|def_id| tcx.hir().local_def_id_to_hir_id(def_id))
}
FakeDefId::Fake(_, _) => None,
// FIXME: Can this be `Some` for `Auto` or `Blanket`?
_ => None,
}
}
}
Expand Down
16 changes: 8 additions & 8 deletions src/librustdoc/formats/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use rustc_middle::middle::privacy::AccessLevels;
use rustc_middle::ty::TyCtxt;
use rustc_span::symbol::sym;

use crate::clean::{self, FakeDefId, GetDefId};
use crate::clean::{self, GetDefId, ItemId};
use crate::fold::DocFolder;
use crate::formats::item_type::ItemType;
use crate::formats::Impl;
Expand Down Expand Up @@ -122,7 +122,7 @@ crate struct Cache {
/// All intra-doc links resolved so far.
///
/// Links are indexed by the DefId of the item they document.
crate intra_doc_links: BTreeMap<FakeDefId, Vec<clean::ItemLink>>,
crate intra_doc_links: FxHashMap<ItemId, Vec<clean::ItemLink>>,
}

/// This struct is used to wrap the `cache` and `tcx` in order to run `DocFolder`.
Expand Down Expand Up @@ -215,7 +215,7 @@ impl<'a, 'tcx> DocFolder for CacheBuilder<'a, 'tcx> {
// Propagate a trait method's documentation to all implementors of the
// trait.
if let clean::TraitItem(ref t) = *item.kind {
self.cache.traits.entry(item.def_id.expect_real()).or_insert_with(|| {
self.cache.traits.entry(item.def_id.expect_def_id()).or_insert_with(|| {
jyn514 marked this conversation as resolved.
Show resolved Hide resolved
clean::TraitWithExtraInfo {
trait_: t.clone(),
is_notable: item.attrs.has_doc_flag(sym::notable_trait),
Expand Down Expand Up @@ -348,11 +348,11 @@ impl<'a, 'tcx> DocFolder for CacheBuilder<'a, 'tcx> {
// `public_items` map, so we can skip inserting into the
// paths map if there was already an entry present and we're
// not a public item.
if !self.cache.paths.contains_key(&item.def_id.expect_real())
|| self.cache.access_levels.is_public(item.def_id.expect_real())
if !self.cache.paths.contains_key(&item.def_id.expect_def_id())
|| self.cache.access_levels.is_public(item.def_id.expect_def_id())
{
self.cache.paths.insert(
item.def_id.expect_real(),
item.def_id.expect_def_id(),
(self.cache.stack.clone(), item.type_()),
);
}
Expand All @@ -361,7 +361,7 @@ impl<'a, 'tcx> DocFolder for CacheBuilder<'a, 'tcx> {
clean::PrimitiveItem(..) => {
self.cache
.paths
.insert(item.def_id.expect_real(), (self.cache.stack.clone(), item.type_()));
.insert(item.def_id.expect_def_id(), (self.cache.stack.clone(), item.type_()));
}

clean::ExternCrateItem { .. }
Expand Down Expand Up @@ -391,7 +391,7 @@ impl<'a, 'tcx> DocFolder for CacheBuilder<'a, 'tcx> {
| clean::StructItem(..)
| clean::UnionItem(..)
| clean::VariantItem(..) => {
self.cache.parent_stack.push(item.def_id.expect_real());
self.cache.parent_stack.push(item.def_id.expect_def_id());
self.cache.parent_is_trait_impl = false;
true
}
Expand Down
6 changes: 3 additions & 3 deletions src/librustdoc/html/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use rustc_span::def_id::CRATE_DEF_INDEX;
use rustc_target::spec::abi::Abi;

use crate::clean::{
self, utils::find_nearest_parent_module, ExternalCrate, FakeDefId, GetDefId, PrimitiveType,
self, utils::find_nearest_parent_module, ExternalCrate, GetDefId, ItemId, PrimitiveType,
};
use crate::formats::item_type::ItemType;
use crate::html::escape::Escape;
Expand Down Expand Up @@ -1181,7 +1181,7 @@ impl clean::FnDecl {
impl clean::Visibility {
crate fn print_with_space<'a, 'tcx: 'a>(
self,
item_did: FakeDefId,
item_did: ItemId,
cx: &'a Context<'tcx>,
) -> impl fmt::Display + 'a + Captures<'tcx> {
let to_print = match self {
Expand All @@ -1191,7 +1191,7 @@ impl clean::Visibility {
// FIXME(camelid): This may not work correctly if `item_did` is a module.
// However, rustdoc currently never displays a module's
// visibility, so it shouldn't matter.
let parent_module = find_nearest_parent_module(cx.tcx(), item_did.expect_real());
let parent_module = find_nearest_parent_module(cx.tcx(), item_did.expect_def_id());

if vis_did.index == CRATE_DEF_INDEX {
"pub(crate) ".to_owned()
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/html/render/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ impl<'tcx> Context<'tcx> {
&self.shared.style_files,
)
} else {
if let Some(&(ref names, ty)) = self.cache.paths.get(&it.def_id.expect_real()) {
if let Some(&(ref names, ty)) = self.cache.paths.get(&it.def_id.expect_def_id()) {
let mut path = String::new();
for name in &names[..names.len() - 1] {
path.push_str(name);
Expand Down
Loading