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

dtolnay · 2024-12-14T23:45:50Z

Before:

After:

dtolnay · 2024-12-14T23:53:09Z

@fmease — I saw that you self-assigned #134320. If you think of a different implementation approach that would be a better fix, I would be happy to close this PR and we can go with yours.

fmease

I have some minor comments, otherwise looks ok (rustdoc's const handling is sadly pretty scuffed atm as you prolly know (and I made some parts worse I know ^^')).

fmease · 2024-12-14T23:50:29Z

src/librustdoc/clean/types.rs

@@ -554,7 +554,7 @@ impl Item {
        matches!(self.kind, AssocConstItem(..) | StrippedItem(box AssocConstItem(..)))
    }
    pub(crate) fn is_ty_associated_const(&self) -> bool {


We should rename is_ty_associated_const to is_required_assoc_const and update the names the assoc ty and fn equivents, too. However, it's fine to do that in a follow-up PR, too (not necessarily authored by you).

Yes, good call. Done.

fmease · 2024-12-15T00:04:23Z

src/librustdoc/clean/types.rs

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.

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,

);

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

fmease · 2024-12-15T00:10:23Z

src/librustdoc/html/render/mod.rs

+        let repr = konst.value(tcx).unwrap_or_else(|| konst.expr(tcx));
+        if match value {
+            AssocConstValue::TraitDefault(_) => true, // always show
+            AssocConstValue::Impl(_) => repr != "_",  // show if there is a meaningful value to show


unfortunate string comparison but it's alright for the time being

fmease · 2024-12-15T00:14:59Z

src/librustdoc/clean/types.rs

-    /// 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.
+    DefaultAssocConstItem(Box<Constant>),


DefaultAssocConstItem sounds like default const ITEM: Ty = EXPR; (specialization) to me. Can you relate? Maybe ProvidedAssocConstItem or DefaultedAssocConstItem instead?

None of the terminology is fantastic, but I'm okay with either of your suggestions; they are both improvements on what is in the PR. "Provided" is probably the least confusable. Initially it sounds like it could possibly refer to "the constant value that a trait impl provided as the value for a trait's associated constant" (provided by impl vs provided by trait) but it doesn't seem possible to remain confused about this for long given the existence of ImplAssocConstItem which clearly refers to something in an impl.

I was also thinking along the lines of OptionalAssocConstItem to contrast with RequiredAssocConstItem or even synonyms like "discretionary" but none of that is established terminology in Rust in this context.

fmease

Sorry for the delay! Thanks r=me after squashing some of the micro renaming commits and if it's not too annoying also git-fixing up the "split assocconstitem" commit with commit "rename default* to provided*".

fmease · 2024-12-19T17:58:42Z

src/librustdoc/clean/types.rs

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

dtolnay · 2024-12-19T18:53:09Z

I squashed all commits renaming functions (is_ty_associated_const, is_ty_associated_type) into the same commit that renamed the corresponding type, and I eliminated the DefaultAssocConstItem naming from commit history.

@bors r=fmease

bors · 2024-12-19T18:53:12Z

📌 Commit 7ee31eb has been approved by fmease

It is now in the queue for this repository.

Hide `= _` as associated constant value inside impl blocks Closes rust-lang#134320. ### Before: <img src="https://github.com/user-attachments/assets/19d28811-45d2-4563-9726-f40c6af411c6" width="300"> <img src="https://github.com/user-attachments/assets/1ecf8764-97ce-47f0-87fa-3b174d2fc578" width="300"> ### After: <img src="https://github.com/user-attachments/assets/6408c4ca-b1c4-42e4-884b-248833a4865f" width="300"> <img src="https://github.com/user-attachments/assets/df2f6981-16f6-409f-8abb-73c0a4a71d6b" width="300"> r? `@fmease`

Rollup of 6 pull requests Successful merges: - rust-lang#126118 (docs: Mention `spare_capacity_mut()` in `Vec::set_len`) - rust-lang#132830 (Rename `elem_offset` to `element_offset`) - rust-lang#133103 (Pass FnAbi to find_mir_or_eval_fn) - rust-lang#134321 (Hide `= _` as associated constant value inside impl blocks) - rust-lang#134518 (fix typos in the example code in the doc comments of `Ipv4Addr::from_bits()`, `Ipv6Addr::from_bits()` & `Ipv6Addr::to_bits()`) - rust-lang#134521 (Arbitrary self types v2: roll loop.) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned fmease Dec 14, 2024

rustbot added A-rustdoc-json Area: Rustdoc JSON backend S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Dec 14, 2024

fmease approved these changes Dec 15, 2024

View reviewed changes

dtolnay removed the A-rustdoc-json Area: Rustdoc JSON backend label Dec 15, 2024

fmease approved these changes Dec 19, 2024

View reviewed changes

dtolnay added 6 commits December 19, 2024 10:42

Rename TyAssocConstItem -> RequiredAssocConstItem

57e1a47

Split AssocConstItem into ProvidedAssocConstItem and ImplAssocConstItem

044885c

Add test for rustdoc showing underscore as assoc const value

da89d10

Suppress = _ on associated constants in impls

6bdfd12

Rename TyAssocTypeItem -> RequiredAssocTypeItem

ff65d62

Rename TyMethodItem -> RequiredMethodItem

7ee31eb

dtolnay force-pushed the docassocconst branch from aadc477 to 7ee31eb Compare December 19, 2024 18:49

rustbot added the A-rustdoc-json Area: Rustdoc JSON backend label Dec 19, 2024

dtolnay removed the A-rustdoc-json Area: Rustdoc JSON backend label Dec 19, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 19, 2024

jhpratt mentioned this pull request Dec 20, 2024

Rollup of 6 pull requests #134550

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dtolnay commented Dec 14, 2024

dtolnay commented Dec 14, 2024

fmease left a comment •

edited

Loading

fmease Dec 14, 2024 •

edited

Loading

dtolnay Dec 15, 2024

fmease Dec 15, 2024

dtolnay Dec 15, 2024

fmease Dec 19, 2024

fmease Dec 15, 2024

fmease Dec 15, 2024 •

edited

Loading

dtolnay Dec 15, 2024

fmease left a comment

fmease Dec 19, 2024

dtolnay commented Dec 19, 2024

bors commented Dec 19, 2024

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

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

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

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

Are you sure you want to change the base?

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

Conversation

dtolnay commented Dec 14, 2024

Before:

After:

dtolnay commented Dec 14, 2024

fmease left a comment • edited Loading

Choose a reason for hiding this comment

fmease Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

dtolnay Dec 15, 2024

Choose a reason for hiding this comment

fmease Dec 15, 2024

Choose a reason for hiding this comment

dtolnay Dec 15, 2024

Choose a reason for hiding this comment

fmease Dec 19, 2024

Choose a reason for hiding this comment

fmease Dec 15, 2024

Choose a reason for hiding this comment

fmease Dec 15, 2024 • edited Loading

Choose a reason for hiding this comment

dtolnay Dec 15, 2024

Choose a reason for hiding this comment

fmease left a comment

Choose a reason for hiding this comment

fmease Dec 19, 2024

Choose a reason for hiding this comment

dtolnay commented Dec 19, 2024

bors commented Dec 19, 2024

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

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

fmease left a comment •

edited

Loading

fmease Dec 14, 2024 •

edited

Loading

fmease Dec 15, 2024 •

edited

Loading