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.

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

rustdoc: Also index impl Traits and raw pointers #92339

Closed
wants to merge 5 commits into from
Closed

rustdoc: Also index impl Traits and raw pointers #92339

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8f5a308
rustdoc: Also index `impl Trait`s and raw pointers
camelid Dec 28, 2021
283c71e
Refactor search-index types to avoid `Option`s
camelid Dec 28, 2021
f27ed72
search-index: Yield all trait bounds, not just the first
camelid Jan 4, 2022
1186c15
Make some `clone`s explicit
camelid Jan 4, 2022
e7222b9
Clean up some code to make it simpler
camelid Jan 4, 2022
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
34 changes: 11 additions & 23 deletions src/librustdoc/html/render/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,8 @@ crate struct IndexItem {
/// A type used for the search index.
#[derive(Debug)]
crate struct RenderType {
name: Option<String>,
generics: Option<Vec<TypeWithKind>>,
name: String,
generics: Vec<TypeWithKind>,
Comment on lines -111 to +112
Copy link
Member Author

Choose a reason for hiding this comment

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

Can this and similar refactorings have any effect on behavior? I found it quite bizarre that the search-index representation of a type can exist without a name.

Copy link
Member

Choose a reason for hiding this comment

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

It's possible in case you have something like T: Display + Whatever. In this case, since T isn't a type, we don't keep its name. Please revert this change.

Copy link
Member Author

@camelid camelid Jan 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This change didn't cause any tests to fail though. Could you please write a test case that passes on master and fails with this refactoring? Otherwise it'll be hard to not break this code accidentally.

Copy link
Member

Choose a reason for hiding this comment

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

Please add one then. Something like (in rustdoc-js):

pub fn foo<Something: Debug>(s: Something) {}

Then look for Something. You shouldn't have any result.

Copy link
Contributor

Choose a reason for hiding this comment

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

#93339

}

/// Full type of functions/methods in the search index.
Expand All @@ -125,19 +125,15 @@ impl Serialize for IndexItemFunctionType {
S: Serializer,
{
// If we couldn't figure out a type, just write `null`.
let has_missing = self.inputs.iter().chain(self.output.iter()).any(|i| i.ty.name.is_none());
if has_missing {
serializer.serialize_none()
} else {
let mut seq = serializer.serialize_seq(None)?;
seq.serialize_element(&self.inputs)?;
match self.output.as_slice() {
[] => {}
[one] => seq.serialize_element(one)?,
all => seq.serialize_element(all)?,
}
seq.end()

let mut seq = serializer.serialize_seq(None)?;
seq.serialize_element(&self.inputs)?;
match self.output.as_slice() {
[] => {}
[one] => seq.serialize_element(one)?,
all => seq.serialize_element(all)?,
}
seq.end()
}
}

Expand All @@ -147,12 +143,6 @@ crate struct TypeWithKind {
kind: ItemType,
}

impl From<(RenderType, ItemType)> for TypeWithKind {
fn from(x: (RenderType, ItemType)) -> TypeWithKind {
TypeWithKind { ty: x.0, kind: x.1 }
}
}

impl Serialize for TypeWithKind {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
Expand All @@ -161,9 +151,7 @@ impl Serialize for TypeWithKind {
let mut seq = serializer.serialize_seq(None)?;
seq.serialize_element(&self.ty.name)?;
seq.serialize_element(&self.kind)?;
if let Some(generics) = &self.ty.generics {
seq.serialize_element(generics)?;
}
seq.serialize_element(&self.ty.generics)?;
seq.end()
}
}
Expand Down
35 changes: 16 additions & 19 deletions src/librustdoc/html/render/search_index.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,24 +185,19 @@ crate fn get_function_type_for_search<'tcx>(
item: &clean::Item,
tcx: TyCtxt<'tcx>,
) -> Option<IndexItemFunctionType> {
let (mut inputs, mut output) = match *item.kind {
let (inputs, output) = match *item.kind {
clean::FunctionItem(ref f) => get_fn_inputs_and_outputs(f, tcx),
clean::MethodItem(ref m, _) => get_fn_inputs_and_outputs(m, tcx),
clean::TyMethodItem(ref m) => get_fn_inputs_and_outputs(m, tcx),
_ => return None,
};

inputs.retain(|a| a.ty.name.is_some());
output.retain(|a| a.ty.name.is_some());

Some(IndexItemFunctionType { inputs, output })
}

fn get_index_type(clean_type: &clean::Type, generics: Vec<TypeWithKind>) -> RenderType {
RenderType {
name: get_index_type_name(clean_type).map(|s| s.as_str().to_ascii_lowercase()),
generics: if generics.is_empty() { None } else { Some(generics) },
}
fn get_index_type(clean_type: &clean::Type, generics: Vec<TypeWithKind>) -> Option<RenderType> {
get_index_type_name(clean_type)
.map(|s| RenderType { name: s.as_str().to_ascii_lowercase(), generics })
}

fn get_index_type_name(clean_type: &clean::Type) -> Option<Symbol> {
Expand Down Expand Up @@ -306,19 +301,17 @@ fn add_generics_and_bounds_as_types<'tcx>(
return;
}
}
let mut index_ty = get_index_type(&ty, generics);
if index_ty.name.as_ref().map(|s| s.is_empty()).unwrap_or(true) {
return;
}
let Some(mut index_ty) = get_index_type(&ty, generics)
else { return };
if is_full_generic {
// We remove the name of the full generic because we have no use for it.
index_ty.name = Some(String::new());
Copy link
Contributor

Choose a reason for hiding this comment

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

@GuillaumeGomez

It seems like, in the multi-generic case you were worried about, the old code didn't actually set the value to None. It actually slided the empty string into there instead.

On the one hand, this means we can get rid of the Option wrapper without breaking anything. On the other hand, this is really strange code.

Copy link
Contributor

@notriddle notriddle Jan 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@camelid

For the record, I approve of this refactoring. If the generated JSON uses the empty string as its sentinel here (and it should, because it's smaller), then RenderType should just use the empty string here to match.

My point here is that the old code, which uses Some(""), is very strange. It even seemed to confuse its original author!

Copy link
Member

Choose a reason for hiding this comment

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

There is another open PR which fixes this part but can't find it... T_T

res.push(TypeWithKind::from((index_ty, ItemType::Generic)));
index_ty.name = String::new();
res.push(TypeWithKind { ty: index_ty, kind: ItemType::Generic });
} else if let Some(kind) = ty.def_id_no_primitives().map(|did| tcx.def_kind(did).into()) {
res.push(TypeWithKind::from((index_ty, kind)));
res.push(TypeWithKind { ty: index_ty, kind });
} else if ty.is_primitive() {
// This is a primitive, let's store it as such.
res.push(TypeWithKind::from((index_ty, ItemType::Primitive)));
res.push(TypeWithKind { ty: index_ty, kind: ItemType::Primitive });
}
}

Expand Down Expand Up @@ -416,7 +409,9 @@ fn get_fn_inputs_and_outputs<'tcx>(
} else {
if let Some(kind) = arg.type_.def_id_no_primitives().map(|did| tcx.def_kind(did).into())
{
all_types.push(TypeWithKind::from((get_index_type(&arg.type_, vec![]), kind)));
if let Some(ty) = get_index_type(&arg.type_, vec![]) {
all_types.push(TypeWithKind { ty, kind });
}
}
}
}
Expand All @@ -429,7 +424,9 @@ fn get_fn_inputs_and_outputs<'tcx>(
if let Some(kind) =
return_type.def_id_no_primitives().map(|did| tcx.def_kind(did).into())
{
ret_types.push(TypeWithKind::from((get_index_type(return_type, vec![]), kind)));
if let Some(ty) = get_index_type(return_type, vec![]) {
ret_types.push(TypeWithKind { ty, kind });
}
}
}
}
Expand Down