Skip to content

Fix json reexports of different items with same name #107766

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Feb 19, 2023
Merged

Fix json reexports of different items with same name #107766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0541a0c
Allow reexports of items with same name but different types to both a…
GuillaumeGomez Feb 7, 2023
7d47d7c
Add regression test for #107677
GuillaumeGomez Feb 7, 2023
3adc081
Fix bad handling of primitive types
GuillaumeGomez Feb 8, 2023
1ae875f
Improve code readability
GuillaumeGomez Feb 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 67 additions & 32 deletions src/librustdoc/json/conversions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ impl JsonRenderer<'_> {
Some(UrlFragment::UserWritten(_)) | None => *page_id,
};

(link.clone(), from_item_id(id.into(), self.tcx))
(link.clone(), id_from_item_default(id.into(), self.tcx))
})
.collect();
let docs = item.attrs.collapsed_doc_value();
Expand All @@ -50,7 +50,8 @@ impl JsonRenderer<'_> {
.collect();
let span = item.span(self.tcx);
let visibility = item.visibility(self.tcx);
let clean::Item { name, attrs: _, kind: _, item_id, cfg: _, .. } = item;
let clean::Item { name, item_id, .. } = item;
let id = id_from_item(&item, self.tcx);
let inner = match *item.kind {
clean::KeywordItem => return None,
clean::StrippedItem(ref inner) => {
Expand All @@ -69,7 +70,7 @@ impl JsonRenderer<'_> {
_ => from_clean_item(item, self.tcx),
};
Some(Item {
id: from_item_id_with_name(item_id, self.tcx, name),
id,
crate_id: item_id.krate().as_u32(),
name: name.map(|sym| sym.to_string()),
span: span.and_then(|span| self.convert_span(span)),
Expand Down Expand Up @@ -107,7 +108,7 @@ impl JsonRenderer<'_> {
Some(ty::Visibility::Public) => Visibility::Public,
Some(ty::Visibility::Restricted(did)) if did.is_crate_root() => Visibility::Crate,
Some(ty::Visibility::Restricted(did)) => Visibility::Restricted {
parent: from_item_id(did.into(), self.tcx),
parent: id_from_item_default(did.into(), self.tcx),
path: self.tcx.def_path(did).to_string_no_crate_verbose(),
},
}
Expand Down Expand Up @@ -204,21 +205,42 @@ impl FromWithTcx<clean::TypeBindingKind> for TypeBindingKind {
}
}

/// It generates an ID as follows:
///
/// `CRATE_ID:ITEM_ID[:NAME_ID]` (if there is no name, NAME_ID is not generated).
pub(crate) fn from_item_id(item_id: ItemId, tcx: TyCtxt<'_>) -> Id {
from_item_id_with_name(item_id, tcx, None)
#[inline]
pub(crate) fn id_from_item_default(item_id: ItemId, tcx: TyCtxt<'_>) -> Id {
id_from_item_inner(item_id, tcx, None, None)
}

// FIXME: this function (and appending the name at the end of the ID) should be removed when
// reexports are not inlined anymore for json format. It should be done in #93518.
pub(crate) fn from_item_id_with_name(item_id: ItemId, tcx: TyCtxt<'_>, name: Option<Symbol>) -> Id {
struct DisplayDefId<'a>(DefId, TyCtxt<'a>, Option<Symbol>);
/// It generates an ID as follows:
///
/// `CRATE_ID:ITEM_ID[:NAME_ID][-EXTRA]`:
/// * If there is no `name`, `NAME_ID` is not generated.
/// * If there is no `extra`, `EXTRA` is not generated.
///
/// * `name` is the item's name if available (it's not for impl blocks for example).
/// * `extra` is used for reexports: it contains the ID of the reexported item. It is used to allow
/// to have items with the same name but different types to both appear in the generated JSON.
pub(crate) fn id_from_item_inner(
item_id: ItemId,
tcx: TyCtxt<'_>,
name: Option<Symbol>,
extra: Option<&Id>,
) -> Id {
struct DisplayDefId<'a, 'b>(DefId, TyCtxt<'a>, Option<&'b Id>, Option<Symbol>);

impl<'a> fmt::Display for DisplayDefId<'a> {
impl<'a, 'b> fmt::Display for DisplayDefId<'a, 'b> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let DisplayDefId(def_id, tcx, name) = self;
let DisplayDefId(def_id, tcx, extra, name) = self;
// We need this workaround because primitive types' DefId actually refers to
// their parent module, which isn't present in the output JSON items. So
// instead, we directly get the primitive symbol and convert it to u32 to
// generate the ID.
let s;
let extra = if let Some(e) = extra {
s = format!("-{}", e.0);
&s
} else {
""
};
let name = match name {
Some(name) => format!(":{}", name.as_u32()),
None => {
Expand All @@ -240,18 +262,33 @@ pub(crate) fn from_item_id_with_name(item_id: ItemId, tcx: TyCtxt<'_>, name: Opt
}
}
};
write!(f, "{}:{}{}", self.0.krate.as_u32(), u32::from(self.0.index), name)
write!(f, "{}:{}{name}{extra}", def_id.krate.as_u32(), u32::from(def_id.index))
}
}

match item_id {
ItemId::DefId(did) => Id(format!("{}", DisplayDefId(did, tcx, name))),
ItemId::Blanket { for_, impl_id } => {
Id(format!("b:{}-{}", DisplayDefId(impl_id, tcx, None), DisplayDefId(for_, tcx, name)))
}
ItemId::Auto { for_, trait_ } => {
Id(format!("a:{}-{}", DisplayDefId(trait_, tcx, None), DisplayDefId(for_, tcx, name)))
ItemId::DefId(did) => Id(format!("{}", DisplayDefId(did, tcx, extra, name))),
ItemId::Blanket { for_, impl_id } => Id(format!(
"b:{}-{}",
DisplayDefId(impl_id, tcx, None, None),
DisplayDefId(for_, tcx, extra, name)
)),
ItemId::Auto { for_, trait_ } => Id(format!(
"a:{}-{}",
DisplayDefId(trait_, tcx, None, None),
DisplayDefId(for_, tcx, extra, name)
)),
}
}

pub(crate) fn id_from_item(item: &clean::Item, tcx: TyCtxt<'_>) -> Id {
match *item.kind {
clean::ItemKind::ImportItem(ref import) => {
let extra =
import.source.did.map(ItemId::from).map(|i| id_from_item_inner(i, tcx, None, None));
id_from_item_inner(item.item_id, tcx, item.name, extra.as_ref())
}
_ => id_from_item_inner(item.item_id, tcx, item.name, None),
}
}

Expand Down Expand Up @@ -525,7 +562,7 @@ impl FromWithTcx<clean::Path> for Path {
fn from_tcx(path: clean::Path, tcx: TyCtxt<'_>) -> Path {
Path {
name: path.whole_name(),
id: from_item_id(path.def_id().into(), tcx),
id: id_from_item_default(path.def_id().into(), tcx),
args: path.segments.last().map(|args| Box::new(args.clone().args.into_tcx(tcx))),
}
}
Expand Down Expand Up @@ -702,7 +739,7 @@ impl FromWithTcx<clean::Import> for Import {
Import {
source: import.source.path.whole_name(),
name,
id: import.source.did.map(ItemId::from).map(|i| from_item_id(i, tcx)),
id: import.source.did.map(ItemId::from).map(|i| id_from_item_default(i, tcx)),
glob,
}
}
Expand Down Expand Up @@ -791,7 +828,7 @@ fn ids(items: impl IntoIterator<Item = clean::Item>, tcx: TyCtxt<'_>) -> Vec<Id>
items
.into_iter()
.filter(|x| !x.is_stripped() && !x.is_keyword())
.map(|i| from_item_id_with_name(i.item_id, tcx, i.name))
.map(|i| id_from_item(&i, tcx))
.collect()
}

Expand All @@ -801,12 +838,10 @@ fn ids_keeping_stripped(
) -> Vec<Option<Id>> {
items
.into_iter()
.map(|i| {
if !i.is_stripped() && !i.is_keyword() {
Some(from_item_id_with_name(i.item_id, tcx, i.name))
} else {
None
}
})
.map(
|i| {
if !i.is_stripped() && !i.is_keyword() { Some(id_from_item(&i, tcx)) } else { None }
},
)
.collect()
}
15 changes: 6 additions & 9 deletions src/librustdoc/json/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use crate::docfs::PathError;
use crate::error::Error;
use crate::formats::cache::Cache;
use crate::formats::FormatRenderer;
use crate::json::conversions::{from_item_id, from_item_id_with_name, IntoWithTcx};
use crate::json::conversions::{id_from_item, id_from_item_default, IntoWithTcx};
use crate::{clean, try_err};

#[derive(Clone)]
Expand Down Expand Up @@ -58,7 +58,7 @@ impl<'tcx> JsonRenderer<'tcx> {
.map(|i| {
let item = &i.impl_item;
self.item(item.clone()).unwrap();
from_item_id_with_name(item.item_id, self.tcx, item.name)
id_from_item(&item, self.tcx)
})
.collect()
})
Expand Down Expand Up @@ -89,7 +89,7 @@ impl<'tcx> JsonRenderer<'tcx> {

if item.item_id.is_local() || is_primitive_impl {
self.item(item.clone()).unwrap();
Some(from_item_id_with_name(item.item_id, self.tcx, item.name))
Some(id_from_item(&item, self.tcx))
} else {
None
}
Expand Down Expand Up @@ -150,7 +150,6 @@ impl<'tcx> FormatRenderer<'tcx> for JsonRenderer<'tcx> {
// Flatten items that recursively store other items
item.kind.inner_items().for_each(|i| self.item(i.clone()).unwrap());

let name = item.name;
let item_id = item.item_id;
if let Some(mut new_item) = self.convert_item(item) {
let can_be_ignored = match new_item.inner {
Expand Down Expand Up @@ -193,10 +192,7 @@ impl<'tcx> FormatRenderer<'tcx> for JsonRenderer<'tcx> {
| types::ItemEnum::Macro(_)
| types::ItemEnum::ProcMacro(_) => false,
};
let removed = self
.index
.borrow_mut()
.insert(from_item_id_with_name(item_id, self.tcx, name), new_item.clone());
let removed = self.index.borrow_mut().insert(new_item.id.clone(), new_item.clone());

// FIXME(adotinthevoid): Currently, the index is duplicated. This is a sanity check
// to make sure the items are unique. The main place this happens is when an item, is
Expand All @@ -207,6 +203,7 @@ impl<'tcx> FormatRenderer<'tcx> for JsonRenderer<'tcx> {
if !can_be_ignored {
assert_eq!(old_item, new_item);
}
trace!("replaced {:?}\nwith {:?}", old_item, new_item);
}
}

Expand Down Expand Up @@ -246,7 +243,7 @@ impl<'tcx> FormatRenderer<'tcx> for JsonRenderer<'tcx> {
.chain(&self.cache.external_paths)
.map(|(&k, &(ref path, kind))| {
(
from_item_id(k.into(), self.tcx),
id_from_item_default(k.into(), self.tcx),
types::ItemSummary {
crate_id: k.krate.as_u32(),
path: path.iter().map(|s| s.to_string()).collect(),
Expand Down
25 changes: 25 additions & 0 deletions tests/rustdoc-json/reexport/same_name_different_types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Regression test for <https://github.com/rust-lang/rust/issues/107677>.

#![feature(no_core)]
#![no_core]

pub mod nested {
// @set foo_struct = "$.index[*][?(@.docs == 'Foo the struct')].id"

/// Foo the struct
pub struct Foo {}

// @set foo_fn = "$.index[*][?(@.docs == 'Foo the function')].id"

#[allow(non_snake_case)]
/// Foo the function
pub fn Foo() {}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this test be expanded a bit more? I'd like to see it checking that their are items for both Foos, and that each name imports both of the foo items (instead of just checking that their are two imports with each name).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eg:

#![feature(no_core)]
#![no_core]

pub mod nested {
    // @set foo_struct = "$.index[*][?(@.docs == 'Foo the struct')].id"

    /// Foo the struct
    pub struct Foo {}

    // @set foo_fn = "$.index[*][?(@.docs == 'Foo the function')].id"

    #[allow(non_snake_case)]
    /// Foo the function
    pub fn Foo() {}
}

// @ismany "$.index[*][?(@.inner.name == 'Foo' && @.kind == 'import')].inner.id" $foo_fn $foo_struct
// @ismany "$.index[*][?(@.inner.name == 'Bar' && @.kind == 'import')].inner.id" $foo_fn $foo_struct

// @count "$.index[*][?(@.inner.name == 'Foo' && @.kind == 'import')]" 2
pub use nested::Foo;
// @count "$.index[*][?(@.inner.name == 'Bar' && @.kind == 'import')]" 2
pub use Foo as Bar;

// @ismany "$.index[*][?(@.inner.name == 'Foo' && @.kind == 'import')].inner.id" $foo_fn $foo_struct
// @ismany "$.index[*][?(@.inner.name == 'Bar' && @.kind == 'import')].inner.id" $foo_fn $foo_struct

// @count "$.index[*][?(@.inner.name == 'Foo' && @.kind == 'import')]" 2
pub use nested::Foo;
// @count "$.index[*][?(@.inner.name == 'Bar' && @.kind == 'import')]" 2
pub use Foo as Bar;