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

Fix primitive search in parameters and returned values #81557

Merged
merged 3 commits into from
Feb 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 11 additions & 0 deletions src/librustdoc/clean/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1302,6 +1302,7 @@ crate enum TypeKind {
Attr,
Derive,
TraitAlias,
Primitive,
}

crate trait GetDefId {
Expand Down Expand Up @@ -1404,6 +1405,16 @@ impl Type {
matches!(self, Type::Generic(_))
}

crate fn is_primitive(&self) -> bool {
match self {
Self::Primitive(_) => true,
Self::BorrowedRef { ref type_, .. } | Self::RawPointer(_, ref type_) => {
type_.is_primitive()
}
_ => false,
}
}

crate fn projection(&self) -> Option<(&Type, DefId, Symbol)> {
let (self_, trait_, name) = match self {
QPath { self_type, trait_, name } => (self_type, trait_, name),
Expand Down
27 changes: 14 additions & 13 deletions src/librustdoc/clean/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,11 +171,20 @@ crate fn get_real_types(
cx: &DocContext<'_>,
recurse: i32,
) -> FxHashSet<(Type, TypeKind)> {
fn insert(res: &mut FxHashSet<(Type, TypeKind)>, cx: &DocContext<'_>, ty: Type) {
if let Some(kind) = ty.def_id().map(|did| cx.tcx.def_kind(did).clean(cx)) {
res.insert((ty, kind));
} else if ty.is_primitive() {
// This is a primitive, let's store it as such.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't always a primitive. ty.def_id() will return None for generic parameters as well which shouldn't end up the in the search index.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great catch! I'll fix that and add a test to ensure they're not listed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually no because we check before if it's a full generic. So it should never be the case. I'll add a test to ensure it though.

Copy link
Member

Choose a reason for hiding this comment

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

is_full_generic will only return true for generics passed by value. Generics passed by reference like pub fn foo<T>(x: &T) {} or &self show up in the search index with this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. I'll add a is_primitive method then to ensure that.

res.insert((ty, TypeKind::Primitive));
}
}
let mut res = FxHashSet::default();
if recurse >= 10 {
// FIXME: remove this whole recurse thing when the recursion bug is fixed
return res;
}

if arg.is_full_generic() {
let arg_s = Symbol::intern(&arg.print(&cx.cache).to_string());
if let Some(where_pred) = generics.where_predicates.iter().find(|g| match g {
Expand All @@ -194,11 +203,7 @@ crate fn get_real_types(
if !adds.is_empty() {
res.extend(adds);
} else if !ty.is_full_generic() {
if let Some(kind) =
ty.def_id().map(|did| cx.tcx.def_kind(did).clean(cx))
{
res.insert((ty, kind));
}
insert(&mut res, cx, ty);
}
}
}
Expand All @@ -212,26 +217,22 @@ crate fn get_real_types(
if !adds.is_empty() {
res.extend(adds);
} else if !ty.is_full_generic() {
if let Some(kind) = ty.def_id().map(|did| cx.tcx.def_kind(did).clean(cx)) {
res.insert((ty.clone(), kind));
}
insert(&mut res, cx, ty);
}
}
}
}
} else {
if let Some(kind) = arg.def_id().map(|did| cx.tcx.def_kind(did).clean(cx)) {
res.insert((arg.clone(), kind));
}
insert(&mut res, cx, arg.clone());
if let Some(gens) = arg.generics() {
for gen in gens.iter() {
if gen.is_full_generic() {
let adds = get_real_types(generics, gen, cx, recurse + 1);
if !adds.is_empty() {
res.extend(adds);
}
} else if let Some(kind) = gen.def_id().map(|did| cx.tcx.def_kind(did).clean(cx)) {
res.insert((gen.clone(), kind));
} else {
insert(&mut res, cx, gen.clone());
}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/librustdoc/formats/item_type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ impl From<clean::TypeKind> for ItemType {
clean::TypeKind::Attr => ItemType::ProcAttribute,
clean::TypeKind::Derive => ItemType::ProcDerive,
clean::TypeKind::TraitAlias => ItemType::TraitAlias,
clean::TypeKind::Primitive => ItemType::Primitive,
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/html/render/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ crate fn build_index(krate: &clean::Crate, cache: &mut Cache) -> String {
desc: item.doc_value().map_or_else(String::new, |s| short_markdown_summary(&s)),
parent: Some(did),
parent_idx: None,
search_type: get_index_search_type(&item, None),
search_type: get_index_search_type(&item, Some(cache)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure I understand what is happening here: This adds primitives to the cache, because now in an earlier stage they are being properly added to the cache via the new insert method? And previously this wasn't used because... Cache wasn't working right on them, I'm guessing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly. To be more precise: the cache here is needed to get the DefId of primitive types. If we don't have primitives, we don't need cache (and before that we didn't get the primitives in the cache). So you got it exactly rght. :)

});
for alias in item.attrs.get_doc_aliases() {
cache
Expand Down
25 changes: 25 additions & 0 deletions src/test/rustdoc-js/primitive.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// exact-check

const QUERY = [
"i32",
"str",
"TotoIsSomewhere",
];

const EXPECTED = [
{
'in_args': [
{ 'path': 'primitive', 'name': 'foo' },
],
},
{
'returned': [
{ 'path': 'primitive', 'name': 'foo' },
],
},
{
'others': [],
'in_args': [],
'returned': [],
},
];
5 changes: 5 additions & 0 deletions src/test/rustdoc-js/primitive.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pub fn foo(i: i32) -> &'static str {
"hello"
}

pub fn foo2<TotoIsSomewhere>(i: &TotoIsSomewhere, j: TotoIsSomewhere) {}
4 changes: 4 additions & 0 deletions src/tools/rustdoc-js/tester.js
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,10 @@ function betterLookingDiff(entry, data) {
if (!entry.hasOwnProperty(key)) {
continue;
}
if (!data || !data.hasOwnProperty(key)) {
output += '-' + spaces + contentToDiffLine(key, entry[key]) + '\n';
continue;
}
let value = data[key];
if (value !== entry[key]) {
output += '-' + spaces + contentToDiffLine(key, entry[key]) + '\n';
Expand Down