Rustdoc displays private internals of associated constants (regression)

[dtolnay]

pub struct Struct {
    private: (),
}

impl Struct {
    pub const CONST: Self = Struct { private: () };
}

Correct:

Incorrect:

I used the following script to bisect:

#!/bin/bash
cargo doc && ! rg private: $CARGO_TARGET_DIR/doc/repro/struct.Struct.html >/dev/null

cargo-bisect-rustc --script ./bisect.sh

It bisects to nightly-2022-04-14 and specifically #95990, of which #95316 seems the most relevant because it involves associated constants in rustdoc. @fmease @notriddle @GuillaumeGomez

[fmease]

#95316 is indeed the cause. More specifically the following line:

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

Line 715 in 3dea003

write!(w, " = {}", default.value(cx.tcx()).unwrap_or_else(|| default.expr(cx.tcx())));

.

If we “fail” to evaluate (“failure” also includes “the constant is a struct”) the const value of a provided associated const with .value(), we fall back to .expr() which prints the constant in raw uncensored form including structs with their private fields.

I assume in the non-associated const version (shown below) the call to .expr() does not happen and if .value() fails in such a case, neither the value nor the = is being printed.

pub struct Struct { private: () }
pub const CONST: Struct = Struct { private: () };

Note that I haven't verified all those pieces of information yet, I am going to check them very soon.

I am not sure how to proceed @GuillaumeGomez. This is another instance of constant-printing for provided assoc consts being problematic. Previous incident: Performance (#95316 (comment)).

[GuillaumeGomez]

Maybe we could update the .expr() method so that it can only show the public items to fulfill our need?

[fmease]

The ideal solution (non-hotfix) would be to generally improve the printing of (unevaluated) constants (as returned by .expr() -> String. I've already written about it on Zulip. Not sure how easy it is to implement though.

[GuillaumeGomez]

Seems like we want something similar. I don't think it's very difficult but might involve a lot of code. Want to give it a try?

[fmease]

I might as well try. If successful, my PR needs backporting.

@rustbot claim

[rustbot]

Error: Parsing assign command in comment failed: ...'bot assign' | error: specify user to assign to at >| ''...

Please let @rust-lang/release know if you're having trouble with this bot.

[eggyal]

I'm not convinced that displaying the value of (provided associated) constants in rustdoc is ever worthwhile and indeed it could be harmful. Users should not need to know what value a constant holds, only what it represents; indeed they should only ever be using the constant itself to represent that value when it's required—that is to say, the value assigned is an implementation detail and nobody should be relying upon it.

[GuillaumeGomez]

I strongly disagree. Associated constants are meant to be used by externals users, otherwise they'd not be created. They are definitely not an implementation detail and they are part of the public API so everyone should rely on them.

There are a lot of examples so I'll just take one: all SystemExt associated constants in the sysinfo crate.

[eggyal]

I'm not sure I follow. Why do I need to know that <System as SystemExt>::IS_SUPPORTED is true? Surely I will write code that switches on SystemExt::IS_SUPPORTED and the compiler handles the rest?

[fmease]

I agree with @GuillaumeGomez. They are not an implementation detail, at least not those that are of a type that exposes structural equality (deriving PartialEq and Eq) since downstream users may depend (as in “compilation may fail if the implementation changes”) on the concrete value of a constant if they use it in a pattern [exhaustiveness checking] or inside of a const argument [type checking] (which may currently require #![feature(adt_const_params)] if the type is not numeric).

( I even vaguely remember seeing a Zulip discussion about how public constants always leak their implementation / concrete value, not just those of a StructuralEq type, but I am not sure anymore )

[eggyal]

if they use it in a pattern [exhaustiveness checking] or inside of a const argument [type checking] (which may currently require #![feature(adt_const_params)] if the type is not numeric).

Isn't that just relying on an implementation detail in order to work-around current limitations in the language? If/when adt_const_params lands, the constant should be used in place of its value?

[fmease]

What I am saying is that changing the value of a public constant is a breaking change since it is part of the public API.

As a simplified example, consider the case where a crate called constants exports two constants of type bool, named X and Y.

Crate user0 depending on crate constants contains the following code:

pub struct Container<const B: bool>;
const _: Container<{constants::Y}> = Container::<{!constants::X}>;

Crate user1 also depending on constants:

const _: () = match true { constants::X | constants::Y => {} };

In version A of constants, constant X is not equal to Y and the dependent crates user0 and user1 compile just fine.
Now, in the succeeding version B of constants, X and Y are set equal to each other. user0 and user1 no longer compile with the new version. The inequality was part of the public contract but it was broken. If we wanted to follow SemVer, version B would need to be incompatible with A.

[eggyal]

Ah, understood! Sorry for being slow on the uptake :). That all makes sense now.

[camelid]

@fmease I understand your argument to some extent, but changing a function's body can also be a SemVer breaking change. Why should constants — which also hide a complex expression and present a simple API, like functions — be treated differently?

[GuillaumeGomez]

@camelid I'm a bit lost. We only show public parts of the API unless using --document-private-items and @fmease and I are supporting the idea to only show the public parts of "complex" associated constants' value to make it treated like the other items. Or did I completely miss your comment?

[camelid]

My point is that we never show function bodies (even with --document-private-items), and IMO constants are a subset of (const) functions, so we also should not show constant bodies.

We do show the bodies of free (non-associated) constants already, but I think I'd rather stop doing that instead of also displaying associated constants' values.

[GuillaumeGomez]

I see your point. I disagree though. If you update a constant value, it's the same as changing a function's header: it's a breaking change. And it's also super useful to be able to see the value of a constant (associated or not) and I think it's definitely a big plus.

[camelid]

Well, I at least think we shouldn't show the raw expression from the constant's body; we should only show the fully-computed value.

[fmease]

@camelid Yes, we should prefer printing the evaluated const. I am working on this. It's not as trivial and I am going to do it incrementally. The biggest issue is the missing support for const substitutions as you might remember.

Yesterday I almost had a working PR done, in some sense a hotfix for this issue, that “censors” complex unevaluated expressions like struct literals and match expressions by replacing them with _ (over time this procedure can be fine-tuned).
However, I ran into some nasty corner cases (certain qualified paths that contain const arguments) and regressions. I will try to simplify the patch, add tests and open it as a PR today or tomorrow (hopefully).

I have further plans for improving const support but the current representation of constant expressions and the lack of const substitutions in rustdoc is giving me headaches. I have already prepared a more sophisticated PR that adds hyperlinks to const exprs and understands the concept of private and doc(hidden) fields when pretty-printing but it's not quite ready yet.

[camelid]

I have further plans for improving const support but the current representation of constant expressions and the lack of const substitutions in rustdoc is giving me headaches.

Yeah, the current state of the code is quite unfortunate :(

I've tried to improve it, but as you've seen it's very hard to understand and change.

[unageek]

I don't like this.

[fmease]

PR coming up within the next hour. I just need to write some more tests.