Don't show associated const value anymore

[GuillaumeGomez]

Part of #44348.

Before:

After:

cc @nox

r? @QuietMisdreavus

[nox]

Did you check what it looks like if the constant is documented? If I just hide the code block for the definition with current rustdoc, the spacing around the docs seems closer to the next item than the one it documents.

[GuillaumeGomez]

I didn't but I can add a pass for it if you want.

[nox]

I just mentioned it because I noticed it was messing up the spacing with my small CSS extension.

[GuillaumeGomez]

Didn't see anything special despite the fact that the toggle wasn't added to associated const docs. I fixed it in my last commit. Now it looks like this:

[QuietMisdreavus]

Tagging some people who i know use associated consts in their documentation: @sgrif @SergioBenitez

EDIT: Also, @rust-lang/docs, since this is a "what do we show in docs" question.

Overall i'm fine with this, since like @nox said, it can leak internal information. I'd hoped there could be a more elegant/complicated way to deal with it, but i guess this solves an immediate problem.

[nox]

I've just realised that constant definitions should be hidden too, not just for associated constants.

https://docs.rs/khronos_api/2.2.0/khronos_api/constant.EGL_XML.html

[SimonSapin]

Right, for const items and static items it may make sense to not show the value, but that’s not related to whether the item is associated (in an impl) or module-level.

Though I feel that it might depend. Sometimes the value of a constant is useful and relevant info, not absurdly long, and not "leaking" internal implementation details. #44348 (comment) mentions opaque wrappers, so maybe the presence of private fields (or the absence of public fields, for structs) could be a heuristic that affects the default. And may it’s worth having an attribute like #[doc(value = "hidden")] and #[doc(value = "shown")].

[nox]

One can always click on the [src] link to see the definition.

[GuillaumeGomez]

One can always click on the [src] link to see the definition.

That was the whole point of adding [src] while removing the definition.

[QuietMisdreavus]

It's worth linking this PR to #49259 as well.

[SergioBenitez]

I generally like this change. This might imply, however, that the actual value of a constant is not part of the public interface, which would mean that changing the value of a const is a semver-compatible change, all other things remaining the same. I believe that should be true, but it's something worth considering.

[GuillaumeGomez]

@SergioBenitez: From my point of view, you're supposed to use a const, not its value. So changing its value shouldn't impact someone's code. But that should be considered, indeed.

[QuietMisdreavus]

I would agree with the assessment that the value of a const is not necessarily part of its semver guarantee, only its existence and type. Knowing what the value is is sometimes useful information, but unless you're tracking down a bug or looking for some performance issue you probably don't need to know what it looks like on the inside. It feels like all the people who have commented here agree on that? In some form?

@GuillaumeGomez While you're doing this, can you also do this for non-associated consts, like @nox mentioned?

[GuillaumeGomez]

Yes, I'll do it soon.

[GuillaumeGomez]

Remove constant and static initialization from documentation as well.

[sgrif]

I am extremely 👍 on not showing the value, I felt it was noisy when that was added

[Centril]

It seems to me that in the presence of const generics; the value of an associated const is absolutely part of the interface and can cause compilation errors. I'm quite skeptical about not showing associated consts long term therefore.

[nox]

What you say is already true without const generics and you can click on the source link to see the definition of the constant.

[Centril]

@QuietMisdreavus

I'm skeptical that changing the value of a const without changing its type will affect whether something compiles.

As @nox says, this is the case even today:

trait Foo {
    const BAR: usize;
}

struct Baz;

impl Foo for Baz {
    const BAR: usize = 3; // Change this to 4 and we have a compilation error.
}

fn main() {
    let arr: [u8; <Baz as Foo>::BAR] = [1, 2, 3];
}

The more of (const) dependent types Rust gets, the more this becomes a problem.

[nox]

My point was that saying const generics are a reason to not do that is moot. The compile error you speak of only happens if you used the constant sometimes and its literal value some other times. That doesn’t happen if you use the constant exhaustively as one should. Exposing the value currently means that we are leaking internal details and generating too much cruft in the rustdoc output.

…

-- Anthony Ramine

Le 27 août 2018 à 19:08, Mazdak Farrokhzad ***@***.***> a écrit : @QuietMisdreavus I'm skeptical that changing the value of a const without changing its type will affect whether something compiles. As @nox says, this is the case even today: trait Foo { const BAR: usize; } struct Baz; impl Foo for Baz { const BAR: usize = 3; // Change this to 4 and we have a compilation error. } fn main() { let arr: [u8; <Baz as Foo>::BAR] = [1, 2, 3]; } — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[nox]

Furthermore, even if we don’t hide constant definitions, your case still happen if you replace the constant BAR by a nullary const function that returns 3, yet I don’t think anyone would argue that rustdoc should show definitions of const functions.

[Centril]

@nox

That doesn’t happen if you use the constant exhaustively as one should.

Fair enough.

[bluetech]

If the value is informative for the user, it can always be repeated in the docstring. See e.g. http::StatusCode.

[TimNN]

Ping from triage @QuietMisdreavus / @GuillaumeGomez: What is the status of this PR?

[GuillaumeGomez]

It seems to be ok. Not sure if the debate is over or not...

[QuietMisdreavus]

Something to note for the people wanting a more-robust means of printing associated constants: Since @GuillaumeGomez didn't make this close the linked issue, i can only assume he has some plan up his sleeve? It can be extended later, but the current form of associated consts is unwieldy and awkward. If the concerns of @Centril, @SimonSapin, and/or whoever else was in this thread have been resolved, r=me.

Side note: Is the prefix parameter of document_full/document_short/etc used any more? I seem to remember thinking that was its only use...

[Centril]

@GuillaumeGomez my concerns are resolved. :)

[GuillaumeGomez]

@QuietMisdreavus It's still in use apparently. And @Centril seems to be happy with changes. I'll let you r+ though. :p

[TimNN]

Ping from triage @QuietMisdreavus: It looks like this PR requires a final review from you.

[QuietMisdreavus]

Welp, with no further comment, let's get this merged.

@bors r+

[bors]

📌 Commit 8614179 has been approved by QuietMisdreavus

[bors]

⌛ Testing commit 8614179 with merge f2302da...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: QuietMisdreavus
Pushing f2302da to master...