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: Fix handling of Self type in search index and refactor its representation #128471

Merged
merged 6 commits into from
Aug 5, 2024
Merged
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
16 changes: 5 additions & 11 deletions src/librustdoc/clean/inline.rs
Original file line number Diff line number Diff line change
@@ -12,7 +12,7 @@ use rustc_middle::ty::fast_reject::SimplifiedType;
use rustc_middle::ty::{self, TyCtxt};
use rustc_span::def_id::LOCAL_CRATE;
use rustc_span::hygiene::MacroKind;
use rustc_span::symbol::{kw, sym, Symbol};
use rustc_span::symbol::{sym, Symbol};
use thin_vec::{thin_vec, ThinVec};
use {rustc_ast as ast, rustc_hir as hir};

@@ -792,11 +792,7 @@ fn build_macro(
fn filter_non_trait_generics(trait_did: DefId, mut g: clean::Generics) -> clean::Generics {
for pred in &mut g.where_predicates {
match *pred {
clean::WherePredicate::BoundPredicate {
ty: clean::Generic(ref s),
ref mut bounds,
..
} if *s == kw::SelfUpper => {
clean::WherePredicate::BoundPredicate { ty: clean::SelfTy, ref mut bounds, .. } => {
bounds.retain(|bound| match bound {
clean::GenericBound::TraitBound(clean::PolyTrait { trait_, .. }, _) => {
trait_.def_id() != trait_did
@@ -812,13 +808,13 @@ fn filter_non_trait_generics(trait_did: DefId, mut g: clean::Generics) -> clean:
clean::WherePredicate::BoundPredicate {
ty:
clean::QPath(box clean::QPathData {
self_type: clean::Generic(ref s),
self_type: clean::Generic(_),
trait_: Some(trait_),
..
}),
bounds,
..
} => !(bounds.is_empty() || *s == kw::SelfUpper && trait_.def_id() == trait_did),
} => !bounds.is_empty() && trait_.def_id() != trait_did,
Copy link
Member Author

Choose a reason for hiding this comment

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

Just an application of De Morgan rules combined with knowing that clean::Generic(kw::SelfUpper) is gone now.

_ => true,
});
g
@@ -832,9 +828,7 @@ fn separate_supertrait_bounds(
) -> (clean::Generics, Vec<clean::GenericBound>) {
let mut ty_bounds = Vec::new();
g.where_predicates.retain(|pred| match *pred {
clean::WherePredicate::BoundPredicate { ty: clean::Generic(ref s), ref bounds, .. }
if *s == kw::SelfUpper =>
{
clean::WherePredicate::BoundPredicate { ty: clean::SelfTy, ref bounds, .. } => {
ty_bounds.extend(bounds.iter().cloned());
false
}
11 changes: 6 additions & 5 deletions src/librustdoc/clean/mod.rs
Original file line number Diff line number Diff line change
@@ -1351,11 +1351,11 @@ pub(crate) fn clean_middle_assoc_item<'tcx>(
let self_arg_ty =
tcx.fn_sig(assoc_item.def_id).instantiate_identity().input(0).skip_binder();
if self_arg_ty == self_ty {
item.decl.inputs.values[0].type_ = Generic(kw::SelfUpper);
item.decl.inputs.values[0].type_ = SelfTy;
} else if let ty::Ref(_, ty, _) = *self_arg_ty.kind() {
if ty == self_ty {
match item.decl.inputs.values[0].type_ {
BorrowedRef { ref mut type_, .. } => **type_ = Generic(kw::SelfUpper),
BorrowedRef { ref mut type_, .. } => **type_ = SelfTy,
_ => unreachable!(),
}
}
@@ -1439,9 +1439,8 @@ pub(crate) fn clean_middle_assoc_item<'tcx>(
if trait_.def_id() != assoc_item.container_id(tcx) {
return true;
}
match *self_type {
Generic(ref s) if *s == kw::SelfUpper => {}
_ => return true,
if *self_type != SelfTy {
return true;
}
match &assoc.args {
GenericArgs::AngleBracketed { args, constraints } => {
@@ -2228,6 +2227,8 @@ pub(crate) fn clean_middle_ty<'tcx>(
ty::Param(ref p) => {
if let Some(bounds) = cx.impl_trait_bounds.remove(&p.index.into()) {
ImplTrait(bounds)
} else if p.name == kw::SelfUpper {
SelfTy
} else {
Generic(p.name)
}
1 change: 0 additions & 1 deletion src/librustdoc/clean/simplify.rs
Original file line number Diff line number Diff line change
@@ -145,7 +145,6 @@ pub(crate) fn sized_bounds(cx: &mut DocContext<'_>, generics: &mut clean::Generi
// should be handled when cleaning associated types.
generics.where_predicates.retain(|pred| {
if let WP::BoundPredicate { ty: clean::Generic(param), bounds, .. } = pred
&& *param != rustc_span::symbol::kw::SelfUpper
&& bounds.iter().any(|b| b.is_sized_bound(cx))
{
sized_params.insert(*param);
37 changes: 11 additions & 26 deletions src/librustdoc/clean/types.rs
Original file line number Diff line number Diff line change
@@ -34,10 +34,9 @@ use thin_vec::ThinVec;
use {rustc_ast as ast, rustc_hir as hir};

pub(crate) use self::ItemKind::*;
pub(crate) use self::SelfTy::*;
pub(crate) use self::Type::{
Array, BareFunction, BorrowedRef, DynTrait, Generic, ImplTrait, Infer, Primitive, QPath,
RawPointer, Slice, Tuple,
RawPointer, SelfTy, Slice, Tuple,
};
use crate::clean::cfg::Cfg;
use crate::clean::clean_middle_path;
@@ -1384,8 +1383,8 @@ pub(crate) struct FnDecl {
}

impl FnDecl {
pub(crate) fn self_type(&self) -> Option<SelfTy> {
self.inputs.values.get(0).and_then(|v| v.to_self())
pub(crate) fn receiver_type(&self) -> Option<&Type> {
self.inputs.values.get(0).and_then(|v| v.to_receiver())
}
}

@@ -1403,27 +1402,9 @@ pub(crate) struct Argument {
pub(crate) is_const: bool,
}

#[derive(Clone, PartialEq, Debug)]
pub(crate) enum SelfTy {
SelfValue,
SelfBorrowed(Option<Lifetime>, Mutability),
SelfExplicit(Type),
}

impl Argument {
pub(crate) fn to_self(&self) -> Option<SelfTy> {
if self.name != kw::SelfLower {
return None;
}
if self.type_.is_self_type() {
return Some(SelfValue);
}
match self.type_ {
BorrowedRef { ref lifetime, mutability, ref type_ } if type_.is_self_type() => {
Some(SelfBorrowed(lifetime.clone(), mutability))
}
_ => Some(SelfExplicit(self.type_.clone())),
}
pub(crate) fn to_receiver(&self) -> Option<&Type> {
if self.name == kw::SelfLower { Some(&self.type_) } else { None }
}
}

@@ -1477,6 +1458,8 @@ pub(crate) enum Type {
DynTrait(Vec<PolyTrait>, Option<Lifetime>),
/// A type parameter.
Generic(Symbol),
/// The `Self` type.
SelfTy,
/// A primitive (aka, builtin) type.
Primitive(PrimitiveType),
/// A function pointer: `extern "ABI" fn(...) -> ...`
@@ -1571,6 +1554,8 @@ impl Type {
// If both sides are generic, this returns true.
(_, Type::Generic(_)) => true,
(Type::Generic(_), _) => false,
// `Self` only matches itself.
(Type::SelfTy, Type::SelfTy) => true,
// Paths account for both the path itself and its generics.
(Type::Path { path: a }, Type::Path { path: b }) => {
a.def_id() == b.def_id()
@@ -1642,7 +1627,7 @@ impl Type {

pub(crate) fn is_self_type(&self) -> bool {
match *self {
Generic(name) => name == kw::SelfUpper,
SelfTy => true,
_ => false,
}
}
@@ -1700,7 +1685,7 @@ impl Type {
Type::Pat(..) => PrimitiveType::Pat,
RawPointer(..) => PrimitiveType::RawPointer,
QPath(box QPathData { ref self_type, .. }) => return self_type.def_id(cache),
Generic(_) | Infer | ImplTrait(_) => return None,
Generic(_) | SelfTy | Infer | ImplTrait(_) => return None,
};
Primitive(t).def_id(cache)
}
2 changes: 1 addition & 1 deletion src/librustdoc/clean/utils.rs
Original file line number Diff line number Diff line change
@@ -468,7 +468,7 @@ pub(crate) fn resolve_type(cx: &mut DocContext<'_>, path: Path) -> Type {
match path.res {
Res::PrimTy(p) => Primitive(PrimitiveType::from(p)),
Res::SelfTyParam { .. } | Res::SelfTyAlias { .. } if path.segments.len() == 1 => {
Copy link
Member

@compiler-errors compiler-errors Aug 1, 2024

Choose a reason for hiding this comment

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

I don't believe that the handling of SelfTyAlias is correct here 🤔 Or at least I do want to point out that SelfTyParam and SelfTyAlias act differently and should probably not continue to be conflated.

SelfTyAlias is used for the self type of an impl; this is not some abstract Self (which is literally just parameter -- unrelated, but I'm not sure why this PR moves away from representing self as a parameter when it is one in trait, but I digress) -- SelfTyAlias instead refers to a specific, sometimes concrete type. For example, in:

impl MyCustomType<i32> { /* Self is a SelfTypeAlias here */ }

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, thanks for raising this. I believe that some other part of rustdoc is "unwrapping" the Self alias; see for example Vec::new()'s return type. It may make sense to centralize that behavior in one place—namely here—though.

Regarding your comment about the change in representation: I understand that Self in trait is logically a parameter (à la Haskell type classes) in the compiler's view. However, from the perspective of users and many of Rustdoc's transformations, we treat it specially and don't think of it as such. For example, we don't normally render it as a parameter on a trait's page (though we do currently have some bugs, like the : PartialEq<Self> here but not here). So I feel like from Rustdoc's perspective, it makes sense to treat it specially unless and until we replace clean::Type with rustc_middle::ty::Ty. Thoughts?

I do intend to add information about the meaning of the type (alias, with value, or parameter, with DefId) to Type::SelfTy, but rustdoc currently doesn't use that information.

Copy link
Member

@fmease fmease Aug 15, 2024

Choose a reason for hiding this comment

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

Yes, thanks for raising this. I believe that some other part of rustdoc is "unwrapping" the Self alias; see for example Vec::new()'s return type.

No, that's not the reason. You've linked to std's Vec::new which is an inlined cross-crate re-export. If you take a look at the local docs in alloc: https://doc.rust-lang.org/nightly/alloc/vec/struct.Vec.html#method.new, you can see that rustdoc retains the literal Self.

This is due to the fact that in the local case rustdoc cleans the HIR which contains the still-unresolved Self path. In the IXCRE case, rustdoc cleans rustc_middle::ty types were the SelfTyAlias has been resolved to a 'concrete' type (here: Vec<T>). In rustc_middle::ty only SelfTyParam is retained (as tcx.types.self_param / "TyKind::Param(0, kw::SelfUpper)").

This bug (or discrepancy at least) is tracked in #44306. See my latest comment from almost a year ago: #44306 (comment) describing how we might go about fixing it and why I haven't taken it on (yet). Note that that comment assumes we want to render Self over the resolved aliased type which might lead to a better UX (unknown).

Generic(kw::SelfUpper)
Type::SelfTy
}
Res::Def(DefKind::TyParam, _) if path.segments.len() == 1 => Generic(path.segments[0].name),
_ => {
30 changes: 12 additions & 18 deletions src/librustdoc/html/format.rs
Original file line number Diff line number Diff line change
@@ -1006,6 +1006,7 @@ fn fmt_type<'cx>(

match *t {
clean::Generic(name) => f.write_str(name.as_str()),
clean::SelfTy => f.write_str("Self"),
clean::Type::Path { ref path } => {
// Paths like `T::Output` and `Self::Output` should be rendered with all segments.
let did = path.def_id();
@@ -1452,29 +1453,22 @@ impl clean::FnDecl {

let last_input_index = self.inputs.values.len().checked_sub(1);
for (i, input) in self.inputs.values.iter().enumerate() {
if let Some(selfty) = input.to_self() {
if let Some(selfty) = input.to_receiver() {
match selfty {
clean::SelfValue => {
clean::SelfTy => {
write!(f, "self")?;
}
clean::SelfBorrowed(Some(ref lt), mutability) => {
write!(
f,
"{amp}{lifetime} {mutability}self",
lifetime = lt.print(),
mutability = mutability.print_with_space(),
)?;
}
clean::SelfBorrowed(None, mutability) => {
write!(
f,
"{amp}{mutability}self",
mutability = mutability.print_with_space(),
)?;
clean::BorrowedRef { lifetime, mutability, type_: box clean::SelfTy } => {
write!(f, "{amp}")?;
match lifetime {
Some(lt) => write!(f, "{lt} ", lt = lt.print())?,
None => {}
}
write!(f, "{mutability}self", mutability = mutability.print_with_space())?;
}
clean::SelfExplicit(ref typ) => {
_ => {
write!(f, "self: ")?;
typ.print(cx).fmt(f)?;
selfty.print(cx).fmt(f)?;
}
}
} else {
15 changes: 7 additions & 8 deletions src/librustdoc/html/render/mod.rs
Original file line number Diff line number Diff line change
@@ -58,7 +58,7 @@ use serde::{Serialize, Serializer};

pub(crate) use self::context::*;
pub(crate) use self::span_map::{collect_spans_and_sources, LinkFromSrc};
use crate::clean::{self, ItemId, RenderedLink, SelfTy};
use crate::clean::{self, ItemId, RenderedLink};
use crate::error::Error;
use crate::formats::cache::Cache;
use crate::formats::item_type::ItemType;
@@ -1372,21 +1372,20 @@ fn render_deref_methods(

fn should_render_item(item: &clean::Item, deref_mut_: bool, tcx: TyCtxt<'_>) -> bool {
let self_type_opt = match *item.kind {
clean::MethodItem(ref method, _) => method.decl.self_type(),
clean::TyMethodItem(ref method) => method.decl.self_type(),
clean::MethodItem(ref method, _) => method.decl.receiver_type(),
clean::TyMethodItem(ref method) => method.decl.receiver_type(),
_ => None,
};

if let Some(self_ty) = self_type_opt {
let (by_mut_ref, by_box, by_value) = match self_ty {
SelfTy::SelfBorrowed(_, mutability)
| SelfTy::SelfExplicit(clean::BorrowedRef { mutability, .. }) => {
let (by_mut_ref, by_box, by_value) = match *self_ty {
clean::Type::BorrowedRef { mutability, .. } => {
(mutability == Mutability::Mut, false, false)
}
SelfTy::SelfExplicit(clean::Type::Path { path }) => {
clean::Type::Path { ref path } => {
(false, Some(path.def_id()) == tcx.lang_items().owned_box(), false)
}
SelfTy::SelfValue => (false, false, true),
clean::Type::SelfTy => (false, false, true),
_ => (false, false, false),
};

483 changes: 248 additions & 235 deletions src/librustdoc/html/render/search_index.rs

Large diffs are not rendered by default.

4 changes: 3 additions & 1 deletion src/librustdoc/json/conversions.rs
Original file line number Diff line number Diff line change
@@ -578,7 +578,7 @@ impl FromWithTcx<clean::Type> for Type {
fn from_tcx(ty: clean::Type, tcx: TyCtxt<'_>) -> Self {
use clean::Type::{
Array, BareFunction, BorrowedRef, Generic, ImplTrait, Infer, Primitive, QPath,
RawPointer, Slice, Tuple,
RawPointer, SelfTy, Slice, Tuple,
};

match ty {
@@ -588,6 +588,8 @@ impl FromWithTcx<clean::Type> for Type {
traits: bounds.into_tcx(tcx),
}),
Generic(s) => Type::Generic(s.to_string()),
// FIXME: add dedicated variant to json Type?
SelfTy => Type::Generic("Self".to_owned()),
Comment on lines +591 to +592
Copy link
Member Author

Choose a reason for hiding this comment

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

cc @aDotInTheVoid -- what would you like to do here?

Copy link
Member

Choose a reason for hiding this comment

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

This is fine for now as it preserves the existing behavior, but yeah this should almost certainly be changed in a future PR

Copy link
Member

Choose a reason for hiding this comment

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

after some discussion with @compiler-errors, i’m not sure the current json behaviour is wrong. but either way, this is fine for 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.

Yes, technically speaking Self is either an alias (in the case of concrete impls) or a type parameter (in the case of traits). So what we're doing here in Rustdoc is more of a convenience for how users perceive Self, rather than how the compiler does.

Primitive(p) => Type::Primitive(p.as_sym().to_string()),
BareFunction(f) => Type::FunctionPointer(Box::new((*f).into_tcx(tcx))),
Tuple(t) => Type::Tuple(t.into_tcx(tcx)),
22 changes: 22 additions & 0 deletions tests/rustdoc-js/self-is-not-generic.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// exact-check

const EXPECTED = [
{
'query': 'A -> A',
'others': [
{ 'path': 'self_is_not_generic::Thing', 'name': 'from' }
],
},
{
'query': 'A -> B',
'others': [
{ 'path': 'self_is_not_generic::Thing', 'name': 'try_from' }
],
},
{
'query': 'Combine -> Combine',
'others': [
{ 'path': 'self_is_not_generic::Combine', 'name': 'combine' }
],
}
];
11 changes: 11 additions & 0 deletions tests/rustdoc-js/self-is-not-generic.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
pub trait Combine {
fn combine(&self, other: &Self) -> Self;
}

pub struct Thing;

impl Combine for Thing {
fn combine(&self, other: &Self) -> Self {
Self
}
}