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: Also index impl Traits and raw pointers #92339

Closed
wants to merge 5 commits into from
Closed
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
1 change: 1 addition & 0 deletions src/librustdoc/clean/auto_trait.rs
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,7 @@ impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> {
has_sized.insert(ty.clone());
} else if !b
.get_trait_path()
.cloned()
.and_then(|trait_| {
ty_to_traits
.get(&ty)
Expand Down
15 changes: 6 additions & 9 deletions src/librustdoc/clean/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -292,19 +292,16 @@ impl ExternalCrate {
let as_primitive = |res: Res<!>| {
if let Res::Def(DefKind::Mod, def_id) = res {
let attrs = tcx.get_attrs(def_id);
let mut prim = None;
for attr in attrs.lists(sym::doc) {
if let Some(v) = attr.value_str() {
if attr.has_name(sym::primitive) {
prim = PrimitiveType::from_symbol(v);
if prim.is_some() {
break;
if let Some(prim) = PrimitiveType::from_symbol(v) {
return Some((def_id, prim));
}
// FIXME: should warn on unknown primitives?
}
}
}
return prim.map(|p| (def_id, p));
}
None
};
Expand Down Expand Up @@ -1178,16 +1175,16 @@ impl GenericBound {
false
}

crate fn get_poly_trait(&self) -> Option<PolyTrait> {
crate fn get_poly_trait(&self) -> Option<&PolyTrait> {
if let GenericBound::TraitBound(ref p, _) = *self {
return Some(p.clone());
return Some(p);
}
None
}

crate fn get_trait_path(&self) -> Option<Path> {
crate fn get_trait_path(&self) -> Option<&Path> {
if let GenericBound::TraitBound(PolyTrait { ref trait_, .. }, _) = *self {
Some(trait_.clone())
Some(trait_)
} else {
None
}
Expand Down
38 changes: 13 additions & 25 deletions src/librustdoc/html/render/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,10 +106,10 @@ crate struct IndexItem {
}

/// A type used for the search index.
#[derive(Debug)]
#[derive(Debug, Clone)]
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

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.

}

/// Full type of functions/methods in the search index.
Expand All @@ -125,34 +125,24 @@ 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()
}
}

#[derive(Debug)]
#[derive(Debug, Clone)]
crate struct TypeWithKind {
ty: RenderType,
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
81 changes: 45 additions & 36 deletions src/librustdoc/html/render/search_index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use rustc_data_structures::fx::FxHashMap;
use rustc_middle::ty::TyCtxt;
use rustc_span::symbol::Symbol;
use serde::ser::{Serialize, SerializeStruct, Serializer};
use smallvec::{smallvec, SmallVec};

use crate::clean;
use crate::clean::types::{FnRetTy, Function, GenericBound, Generics, Type, WherePredicate};
Expand Down Expand Up @@ -185,47 +186,52 @@ 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_types(clean_type: &clean::Type, generics: Vec<TypeWithKind>) -> Vec<RenderType> {
get_index_type_names(clean_type)
.iter()
.map(|s| RenderType { name: s.as_str().to_ascii_lowercase(), generics: generics.clone() })
.collect()
}

fn get_index_type_name(clean_type: &clean::Type) -> Option<Symbol> {
fn get_index_type_names(clean_type: &clean::Type) -> SmallVec<[Symbol; 2]> {
match *clean_type {
clean::Type::Path { ref path, .. } => {
let path_segment = path.segments.last().unwrap();
Some(path_segment.name)
smallvec![path_segment.name]
}
clean::DynTrait(ref bounds, _) => {
let path = &bounds[0].trait_;
Some(path.segments.last().unwrap().name)
clean::DynTrait(ref traits, _) => {
traits.iter().map(|t| t.trait_.segments.last().unwrap().name).collect()
}
clean::ImplTrait(ref bounds) => bounds
.iter()
.filter_map(|b| match b {
clean::GenericBound::TraitBound(poly_trait, _) => {
Some(poly_trait.trait_.segments.last().unwrap().name)
}
clean::GenericBound::Outlives(_) => None,
})
.collect(),
clean::Generic(s) => smallvec![s],
clean::Primitive(ref p) => smallvec![p.as_sym()],
clean::BorrowedRef { ref type_, .. } | clean::RawPointer(_, ref type_) => {
get_index_type_names(type_)
}
clean::Generic(s) => Some(s),
clean::Primitive(ref p) => Some(p.as_sym()),
clean::BorrowedRef { ref type_, .. } => get_index_type_name(type_),
clean::BareFunction(_)
| clean::Tuple(_)
| clean::Slice(_)
| clean::Array(_, _)
| clean::RawPointer(_, _)
| clean::QPath { .. }
| clean::Infer
| clean::ImplTrait(_) => None,
| clean::Infer => smallvec![],
}
}

Expand Down Expand Up @@ -299,19 +305,18 @@ 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;
}
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

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)));
} else if let Some(kind) = ty.def_id_no_primitives().map(|did| tcx.def_kind(did).into()) {
res.push(TypeWithKind::from((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)));
for mut index_ty in get_index_types(&ty, generics) {
if is_full_generic {
// We remove the name of the full generic because we have no use for it.
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 { ty: index_ty, kind });
} else if ty.is_primitive() {
// This is a primitive, let's store it as such.
res.push(TypeWithKind { ty: index_ty, kind: ItemType::Primitive });
}
}
}

Expand Down Expand Up @@ -358,7 +363,7 @@ fn add_generics_and_bounds_as_types<'tcx>(
let mut ty_generics = Vec::new();
for bound in bound.get_bounds().unwrap_or(&[]) {
if let Some(path) = bound.get_trait_path() {
let ty = Type::Path { path };
let ty = Type::Path { path: path.clone() };
add_generics_and_bounds_as_types(
generics,
&ty,
Expand Down Expand Up @@ -409,7 +414,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)));
for ty in get_index_types(&arg.type_, vec![]) {
all_types.push(TypeWithKind { ty, kind });
}
}
}
}
Expand All @@ -422,7 +429,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)));
for ty in get_index_types(return_type, vec![]) {
ret_types.push(TypeWithKind { ty, kind });
}
}
}
}
Expand Down