Skip to content

Hide = _ as associated constant value inside impl blocks #134321

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 6 commits into from
Dec 20, 2024
Merged

Hide = _ as associated constant value inside impl blocks #134321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
57e1a47
Rename TyAssocConstItem -> RequiredAssocConstItem
dtolnay Dec 11, 2024
044885c
Split AssocConstItem into ProvidedAssocConstItem and ImplAssocConstItem
dtolnay Dec 11, 2024
da89d10
Add test for rustdoc showing underscore as assoc const value
dtolnay Dec 14, 2024
6bdfd12
Suppress `= _` on associated constants in impls
dtolnay Dec 14, 2024
ff65d62
Rename TyAssocTypeItem -> RequiredAssocTypeItem
dtolnay Dec 15, 2024
7ee31eb
Rename TyMethodItem -> RequiredMethodItem
dtolnay Dec 15, 2024
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
47 changes: 27 additions & 20 deletions src/librustdoc/clean/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1222,22 +1222,24 @@ fn clean_trait_item<'tcx>(trait_item: &hir::TraitItem<'tcx>, cx: &mut DocContext
let local_did = trait_item.owner_id.to_def_id();
cx.with_param_env(local_did, |cx| {
let inner = match trait_item.kind {
hir::TraitItemKind::Const(ty, Some(default)) => AssocConstItem(Box::new(Constant {
generics: enter_impl_trait(cx, |cx| clean_generics(trait_item.generics, cx)),
kind: ConstantKind::Local { def_id: local_did, body: default },
type_: clean_ty(ty, cx),
})),
hir::TraitItemKind::Const(ty, Some(default)) => {
ProvidedAssocConstItem(Box::new(Constant {
generics: enter_impl_trait(cx, |cx| clean_generics(trait_item.generics, cx)),
kind: ConstantKind::Local { def_id: local_did, body: default },
type_: clean_ty(ty, cx),
}))
}
hir::TraitItemKind::Const(ty, None) => {
let generics = enter_impl_trait(cx, |cx| clean_generics(trait_item.generics, cx));
TyAssocConstItem(generics, Box::new(clean_ty(ty, cx)))
RequiredAssocConstItem(generics, Box::new(clean_ty(ty, cx)))
}
hir::TraitItemKind::Fn(ref sig, hir::TraitFn::Provided(body)) => {
let m = clean_function(cx, sig, trait_item.generics, FunctionArgs::Body(body));
MethodItem(m, None)
}
hir::TraitItemKind::Fn(ref sig, hir::TraitFn::Required(names)) => {
let m = clean_function(cx, sig, trait_item.generics, FunctionArgs::Names(names));
TyMethodItem(m)
RequiredMethodItem(m)
}
hir::TraitItemKind::Type(bounds, Some(default)) => {
let generics = enter_impl_trait(cx, |cx| clean_generics(trait_item.generics, cx));
Expand All @@ -1257,7 +1259,7 @@ fn clean_trait_item<'tcx>(trait_item: &hir::TraitItem<'tcx>, cx: &mut DocContext
hir::TraitItemKind::Type(bounds, None) => {
let generics = enter_impl_trait(cx, |cx| clean_generics(trait_item.generics, cx));
let bounds = bounds.iter().filter_map(|x| clean_generic_bound(x, cx)).collect();
TyAssocTypeItem(generics, bounds)
RequiredAssocTypeItem(generics, bounds)
}
};
Item::from_def_id_and_parts(local_did, Some(trait_item.ident.name), inner, cx)
Expand All @@ -1271,7 +1273,7 @@ pub(crate) fn clean_impl_item<'tcx>(
let local_did = impl_.owner_id.to_def_id();
cx.with_param_env(local_did, |cx| {
let inner = match impl_.kind {
hir::ImplItemKind::Const(ty, expr) => AssocConstItem(Box::new(Constant {
hir::ImplItemKind::Const(ty, expr) => ImplAssocConstItem(Box::new(Constant {
generics: clean_generics(impl_.generics, cx),
kind: ConstantKind::Local { def_id: local_did, body: expr },
type_: clean_ty(ty, cx),
Expand Down Expand Up @@ -1320,18 +1322,23 @@ pub(crate) fn clean_middle_assoc_item(assoc_item: &ty::AssocItem, cx: &mut DocCo
);
simplify::move_bounds_to_generic_parameters(&mut generics);

let provided = match assoc_item.container {
ty::AssocItemContainer::Impl => true,
ty::AssocItemContainer::Trait => tcx.defaultness(assoc_item.def_id).has_value(),
};
if provided {
AssocConstItem(Box::new(Constant {
match assoc_item.container {
ty::AssocItemContainer::Impl => ImplAssocConstItem(Box::new(Constant {
generics,
kind: ConstantKind::Extern { def_id: assoc_item.def_id },
type_: ty,
}))
} else {
TyAssocConstItem(generics, Box::new(ty))
})),
ty::AssocItemContainer::Trait => {
if tcx.defaultness(assoc_item.def_id).has_value() {
ProvidedAssocConstItem(Box::new(Constant {
generics,
kind: ConstantKind::Extern { def_id: assoc_item.def_id },
type_: ty,
}))
} else {
RequiredAssocConstItem(generics, Box::new(ty))
}
}
}
}
ty::AssocKind::Fn => {
Expand Down Expand Up @@ -1369,7 +1376,7 @@ pub(crate) fn clean_middle_assoc_item(assoc_item: &ty::AssocItem, cx: &mut DocCo
};
MethodItem(item, defaultness)
} else {
TyMethodItem(item)
RequiredMethodItem(item)
}
}
ty::AssocKind::Type => {
Expand Down Expand Up @@ -1486,7 +1493,7 @@ pub(crate) fn clean_middle_assoc_item(assoc_item: &ty::AssocItem, cx: &mut DocCo
bounds,
)
} else {
TyAssocTypeItem(generics, bounds)
RequiredAssocTypeItem(generics, bounds)
}
} else {
AssocTypeItem(
Expand Down
46 changes: 28 additions & 18 deletions src/librustdoc/clean/types.rs
View file
Open in desktop
Copy link
Member

@fmease fmease Dec 15, 2024

Choose a reason for hiding this comment

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

Hmm, having 3 variants for assoc consts is a bit excessive, I wish it was just TraitAssocConst & ImplAssocConst at the maximum but that probably calls for a larger refactoring.

Copy link
Member Author

@dtolnay dtolnay Dec 15, 2024

Choose a reason for hiding this comment

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

Yeah I gave this some thought before going with the current approach. From what I figured out (I haven't worked in this code before):

Before this PR, AssocConstItem and TyAssocConstItem are separate variants because they must store different contents. The variant's data is either Constant (which is Generics + ConstantKind + Type) or just Generics + Type.

In order to merge ProvidedAssocConstItem and RequiredAssocConstItem into a single TraitAssocConstItem variant, either:

  • Constant needs to contain Option<ConstantKind>, which has a big blast radius because non-associated constants and paths (dyn Foo<K = 0>) use this same Constant type in a context where not having a value doesn't make sense.

  • or we need TraitAssocConstItem to store not Constant but some other ConstantWithOptionalValue type — which ends up being not less duplication than splitting ProvidedAssocConstItem vs RequiredAssocConstItem.

There is a more promising approach, which I gave up on but is still potentially salvageable. Notice this parent argument:

rust/src/librustdoc/html/render/mod.rs

Lines 1068 to 1072 in 0aeaa5e

fn render_assoc_item(
w: &mut Buffer,
item: &clean::Item,
link: AssocItemLink<'_>,
parent: ItemType,

rust/src/librustdoc/html/render/mod.rs

Lines 1094 to 1103 in 0aeaa5e

clean::AssocConstItem(ci) => assoc_const(
w,
item,
&ci.generics,
&ci.type_,
Some(&ci.kind),
link,
if parent == ItemType::Trait { 4 } else { 0 },
cx,
),

When printing, associated constants are supposed to know whether they are inside of a ItemType::Trait as opposed to a ItemType::Impl. This would be perfect for deciding whether = _ needs to be shown (only when parent is trait, never when parent is impl).

But this doesn't work because parent ends up being misused under the assumption that it's only relevant for indentation (?). See this call site, which is inside a function called item_trait and is responsible for printing a trait and its contents. And yet it is passing ItemType::Impl as the value of parent, which does not make sense.

rust/src/librustdoc/html/render/print_item.rs

Lines 820 to 827 in 0aeaa5e

render_assoc_item(
w,
m,
AssocItemLink::Anchor(Some(&id)),
ItemType::Impl,
cx,
RenderMode::Normal,
);

Copy link
Member

@fmease fmease Dec 19, 2024

Choose a reason for hiding this comment

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

Oof, that's super odd. Thanks for the thorough investigation, I have to look into cleaning this up at some point!

Original file line number Diff line number Diff line change
Expand Up @@ -547,14 +547,14 @@ impl Item {
pub(crate) fn is_associated_type(&self) -> bool {
matches!(self.kind, AssocTypeItem(..) | StrippedItem(box AssocTypeItem(..)))
}
pub(crate) fn is_ty_associated_type(&self) -> bool {
matches!(self.kind, TyAssocTypeItem(..) | StrippedItem(box TyAssocTypeItem(..)))
pub(crate) fn is_required_associated_type(&self) -> bool {
matches!(self.kind, RequiredAssocTypeItem(..) | StrippedItem(box RequiredAssocTypeItem(..)))
}
pub(crate) fn is_associated_const(&self) -> bool {
matches!(self.kind, AssocConstItem(..) | StrippedItem(box AssocConstItem(..)))
matches!(self.kind, ProvidedAssocConstItem(..) | ImplAssocConstItem(..) | StrippedItem(box (ProvidedAssocConstItem(..) | ImplAssocConstItem(..))))
}
pub(crate) fn is_ty_associated_const(&self) -> bool {
matches!(self.kind, TyAssocConstItem(..) | StrippedItem(box TyAssocConstItem(..)))
pub(crate) fn is_required_associated_const(&self) -> bool {
matches!(self.kind, RequiredAssocConstItem(..) | StrippedItem(box RequiredAssocConstItem(..)))
}
pub(crate) fn is_method(&self) -> bool {
self.type_() == ItemType::Method
Expand Down Expand Up @@ -671,7 +671,9 @@ impl Item {
asyncness: hir::IsAsync::NotAsync,
}
}
ItemKind::FunctionItem(_) | ItemKind::MethodItem(_, _) | ItemKind::TyMethodItem(_) => {
ItemKind::FunctionItem(_)
| ItemKind::MethodItem(_, _)
| ItemKind::RequiredMethodItem(_) => {
let def_id = self.def_id().unwrap();
build_fn_header(def_id, tcx, tcx.asyncness(def_id))
}
Expand Down Expand Up @@ -701,8 +703,13 @@ impl Item {
// Variants always inherit visibility
VariantItem(..) | ImplItem(..) => return None,
// Trait items inherit the trait's visibility
AssocConstItem(..) | TyAssocConstItem(..) | AssocTypeItem(..) | TyAssocTypeItem(..)
| TyMethodItem(..) | MethodItem(..) => {
RequiredAssocConstItem(..)
| ProvidedAssocConstItem(..)
| ImplAssocConstItem(..)
| AssocTypeItem(..)
| RequiredAssocTypeItem(..)
| RequiredMethodItem(..)
| MethodItem(..) => {
let assoc_item = tcx.associated_item(def_id);
let is_trait_item = match assoc_item.container {
ty::AssocItemContainer::Trait => true,
Expand Down Expand Up @@ -847,10 +854,10 @@ pub(crate) enum ItemKind {
TraitAliasItem(TraitAlias),
ImplItem(Box<Impl>),
/// A required method in a trait declaration meaning it's only a function signature.
TyMethodItem(Box<Function>),
RequiredMethodItem(Box<Function>),
/// A method in a trait impl or a provided method in a trait declaration.
///
/// Compared to [TyMethodItem], it also contains a method body.
/// Compared to [RequiredMethodItem], it also contains a method body.
MethodItem(Box<Function>, Option<hir::Defaultness>),
StructFieldItem(Type),
VariantItem(Variant),
Expand All @@ -864,14 +871,16 @@ pub(crate) enum ItemKind {
ProcMacroItem(ProcMacro),
PrimitiveItem(PrimitiveType),
/// A required associated constant in a trait declaration.
TyAssocConstItem(Generics, Box<Type>),
RequiredAssocConstItem(Generics, Box<Type>),
ConstantItem(Box<Constant>),
/// An associated constant in a trait impl or a provided one in a trait declaration.
AssocConstItem(Box<Constant>),
/// An associated constant in a trait declaration with provided default value.
ProvidedAssocConstItem(Box<Constant>),
/// An associated constant in an inherent impl or trait impl.
ImplAssocConstItem(Box<Constant>),
/// A required associated type in a trait declaration.
///
/// The bounds may be non-empty if there is a `where` clause.
TyAssocTypeItem(Generics, Vec<GenericBound>),
RequiredAssocTypeItem(Generics, Vec<GenericBound>),
/// An associated type in a trait impl or a provided one in a trait declaration.
AssocTypeItem(Box<TypeAlias>, Vec<GenericBound>),
/// An item that has been stripped by a rustdoc pass
Expand Down Expand Up @@ -902,7 +911,7 @@ impl ItemKind {
| StaticItem(_)
| ConstantItem(_)
| TraitAliasItem(_)
| TyMethodItem(_)
| RequiredMethodItem(_)
| MethodItem(_, _)
| StructFieldItem(_)
| ForeignFunctionItem(_, _)
Expand All @@ -911,9 +920,10 @@ impl ItemKind {
| MacroItem(_)
| ProcMacroItem(_)
| PrimitiveItem(_)
| TyAssocConstItem(..)
| AssocConstItem(..)
| TyAssocTypeItem(..)
| RequiredAssocConstItem(..)
| ProvidedAssocConstItem(..)
| ImplAssocConstItem(..)
| RequiredAssocTypeItem(..)
| AssocTypeItem(..)
| StrippedItem(_)
| KeywordItem => [].iter(),
Expand Down
9 changes: 5 additions & 4 deletions src/librustdoc/fold.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ pub(crate) trait DocFolder: Sized {
| StaticItem(_)
| ConstantItem(..)
| TraitAliasItem(_)
| TyMethodItem(_)
| RequiredMethodItem(_)
| MethodItem(_, _)
| StructFieldItem(_)
| ForeignFunctionItem(..)
Expand All @@ -91,9 +91,10 @@ pub(crate) trait DocFolder: Sized {
| MacroItem(_)
| ProcMacroItem(_)
| PrimitiveItem(_)
| TyAssocConstItem(..)
| AssocConstItem(..)
| TyAssocTypeItem(..)
| RequiredAssocConstItem(..)
| ProvidedAssocConstItem(..)
| ImplAssocConstItem(..)
| RequiredAssocTypeItem(..)
| AssocTypeItem(..)
| KeywordItem => kind,
}
Expand Down
24 changes: 15 additions & 9 deletions src/librustdoc/formats/cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,12 +334,13 @@ impl DocFolder for CacheBuilder<'_, '_> {
clean::ExternCrateItem { .. }
| clean::ImportItem(..)
| clean::ImplItem(..)
| clean::TyMethodItem(..)
| clean::RequiredMethodItem(..)
| clean::MethodItem(..)
| clean::StructFieldItem(..)
| clean::TyAssocConstItem(..)
| clean::AssocConstItem(..)
| clean::TyAssocTypeItem(..)
| clean::RequiredAssocConstItem(..)
| clean::ProvidedAssocConstItem(..)
| clean::ImplAssocConstItem(..)
| clean::RequiredAssocTypeItem(..)
| clean::AssocTypeItem(..)
| clean::StrippedItem(..)
| clean::KeywordItem => {
Expand Down Expand Up @@ -443,15 +444,17 @@ fn add_item_to_search_index(tcx: TyCtxt<'_>, cache: &mut Cache, item: &clean::It
let item_def_id = item.item_id.as_def_id().unwrap();
let (parent_did, parent_path) = match item.kind {
clean::StrippedItem(..) => return,
clean::AssocConstItem(..) | clean::AssocTypeItem(..)
clean::ProvidedAssocConstItem(..)
| clean::ImplAssocConstItem(..)
| clean::AssocTypeItem(..)
if cache.parent_stack.last().is_some_and(|parent| parent.is_trait_impl()) =>
{
// skip associated items in trait impls
return;
}
clean::TyMethodItem(..)
| clean::TyAssocConstItem(..)
| clean::TyAssocTypeItem(..)
clean::RequiredMethodItem(..)
| clean::RequiredAssocConstItem(..)
| clean::RequiredAssocTypeItem(..)
| clean::StructFieldItem(..)
| clean::VariantItem(..) => {
// Don't index if containing module is stripped (i.e., private),
Expand All @@ -467,7 +470,10 @@ fn add_item_to_search_index(tcx: TyCtxt<'_>, cache: &mut Cache, item: &clean::It
let parent_path = &cache.stack[..cache.stack.len() - 1];
(Some(parent_did), parent_path)
}
clean::MethodItem(..) | clean::AssocConstItem(..) | clean::AssocTypeItem(..) => {
clean::MethodItem(..)
| clean::ProvidedAssocConstItem(..)
| clean::ImplAssocConstItem(..)
| clean::AssocTypeItem(..) => {
let last = cache.parent_stack.last().expect("parent_stack is empty 2");
let parent_did = match last {
// impl Trait for &T { fn method(self); }
Expand Down
8 changes: 5 additions & 3 deletions src/librustdoc/formats/item_type.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,16 +88,18 @@ impl<'a> From<&'a clean::Item> for ItemType {
clean::ConstantItem(..) => ItemType::Constant,
clean::TraitItem(..) => ItemType::Trait,
clean::ImplItem(..) => ItemType::Impl,
clean::TyMethodItem(..) => ItemType::TyMethod,
clean::RequiredMethodItem(..) => ItemType::TyMethod,
clean::MethodItem(..) => ItemType::Method,
clean::StructFieldItem(..) => ItemType::StructField,
clean::VariantItem(..) => ItemType::Variant,
clean::ForeignFunctionItem(..) => ItemType::Function, // no ForeignFunction
clean::ForeignStaticItem(..) => ItemType::Static, // no ForeignStatic
clean::MacroItem(..) => ItemType::Macro,
clean::PrimitiveItem(..) => ItemType::Primitive,
clean::TyAssocConstItem(..) | clean::AssocConstItem(..) => ItemType::AssocConst,
clean::TyAssocTypeItem(..) | clean::AssocTypeItem(..) => ItemType::AssocType,
clean::RequiredAssocConstItem(..)
| clean::ProvidedAssocConstItem(..)
| clean::ImplAssocConstItem(..) => ItemType::AssocConst,
clean::RequiredAssocTypeItem(..) | clean::AssocTypeItem(..) => ItemType::AssocType,
clean::ForeignTypeItem => ItemType::ForeignType,
clean::KeywordItem => ItemType::Keyword,
clean::TraitAliasItem(..) => ItemType::TraitAlias,
Expand Down
Loading
Loading