Document that Debug output of structures is unstable

[rivy]

Pretty printed structures for cfg!(windows) have trailing commas, which is inconsistent with the debug output format documentation and output on non-windows platforms.

use std::fmt;
struct Foo { bar: u32, baz: Option<u32> }
impl fmt::Debug for Foo {
    fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
        fmt.debug_struct("Foo")
           .field("bar", &self.bar)
           .field("baz", &self.baz)
           .finish()
    }
}
fn main() {
    let cfg_windows = if cfg!(windows) { "true" } else { "false" };
    let foo = Foo { bar: 1, baz: Some(1) };
    println!("[cfg!(windows) == {}]\n{:?}\n{:#?}", cfg_windows, foo, foo);
}

C:> rustc --version
rustc 1.36.0 (a53f9df32 2019-07-03)
C:> cargo run
   Compiling t-debug_struct v0.1.0 (C:\Users\Roy\OneDrive\Projects\rust\t-debug_struct)
    Finished dev [unoptimized + debuginfo] target(s) in 0.53s
     Running `target\debug\t-debug_struct.exe`
[cfg!(windows) == true]
Foo { bar: 1, baz: Some(1) }
Foo {
    bar: 1,
    baz: Some(
        1,
    ),
}

$ rustc --version
rustc 1.33.0 (2aa4c46cf 2019-02-28)
$ cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/t-debug_struct`
[cfg!(windows) == false]
Foo { bar: 1, baz: Some(1) }
Foo {
    bar: 1,
    baz: Some(
        1
    )
}

[CryZe]

Isn't the formatting of Debug implementations (at least the derived ones) considered an implementation detail and you shouldn't be testing against those?

Also you seem to be using an outdated compiler on Linux. The formatting changed to include the commas and it does so for me on Linux as well:

[cfg!(windows) == false]
Foo { bar: 1, baz: Some(1) }
Foo {
    bar: 1,
    baz: Some(
        1,
    ),
}

[RalfJung]

@rivy could you do rustc --version on both platforms to check that you are using the same compiler version?

[rivy]

I was using fairly recent version of rustc for both (1.36.0 [2019-07-03] and v1.33.0 [2019-02-28], respectively). I've inserted the version output into the original issue statement.

I ran across this while trying to fix some failing windows tests for "ansi-term".

I've fixed that by converting to a regex comparison with optional trailing commas. So, that issue is essentially solved. And, I'm just fine if it's an "implementation detail" and subject to change, but that's not mentioned anywhere within the usual documentation that I could find.

Pretty-print seems to be a convenient way to compare structures visually, so, I suspect that others are using it for comparisons. A mention in the official documentation about what can be expected to remain stable and what might change would be helpful.

[RalfJung]

1.36.0 [2019-07-03] and v1.33.0 [2019-02-28]

That would explain the difference then. Could you adjust the issue title? This has nothing to do with Windows vs non-Windows. The output simply changed between two versions.

[rivy]

Yes, you're correct. The pretty-print is differing between versions, not platforms. It's the same, per platform, when using the same rustc version.

Since at least the ansi-term package is using the pretty-print to compare structures, I think it'd be a good idea to make a notation in the official documentation about stability and/or instability of the pretty-print output.

Also, any recommendations for structural comparison during testing?

I'd like to give @ogham a PR for a better way to test his structure (see https://github.com/ogham/rust-ansi-term/blob/master/src/debug.rs#L104-L122). I've already supplied a regex comparison which fixes the specific trailing-comma / no-trailing-comma problem (see ogham/rust-ansi-term#52), but that seems quite fragile if pretty-print has no output guarantees.

[rivy]

Thanks for the input @CryZe and @RalfJung!

[RalfJung]

I have adjusted the title to include Debug; I hope that's accurate. IMO this is mostly a docs issue about stating more clearly in the docs that Debug output is not stable. But I will leave that to the libs or docs team. ;)

I'd like to give @ogham a PR for a better way to test his structure (see https://github.com/ogham/rust-ansi-term/blob/master/src/debug.rs#L104-L122).

The expected way for people to compare structures is to use PartialEq. Why does that not work here?

[ogham]

Hi,

In this case, I'm literally testing against the auto-generated Debug output! The type in question has a short debug style if you use {:?} and a longer debug style if you use {:#?} (using debug_struct). There's a test to see if the short debug style works, but there's also a test to see if the longer debug style works that has started feeling. This is why I can't just use PartialEq.

[RalfJung]

What is the point of the test?

If you want to test the debug output itself, you will just have to live with the fact that it can change arbitrarily any time. It's like testing &mut 4 as *mut _ as usize -- some tests just won't produce stable results. I also don't know why you would do that, but whatever. ;)

[rivy]

@ogham & @RalfJung , I think I've made the current testing a bit more robust in the face changing debug formatting between rustc versions (see ogham/rust-ansi-term#52, specifically ogham/rust-ansi-term@bc76cd1). All tests are passing on all platforms and rustc versions that I'm seeing. But it's obviously a fragile test if the debug output is unstable between versions.

I understand the motivation to check for an expected structure instantiation via testing. But using PartialEq seems like a large amount coding for such a simple (and, I would think, common) test. Maybe, I'm missing something... @RalfJung , would you have a more concrete example of how to do this, or suggest a different method to verify a correctly constructed structure?

I'm happy to flesh it out and add it to a new PR for ansi-term.

And maybe we should direct further discussion to that PR?

[RalfJung]

I am still wondering what it is that you are trying to achieve here. Please explain what the test is testing. :)

But using PartialEq seems like a large amount coding

You do not have to write the PartialEq implementation yourself, you can use #[derive(PartialEq)].

[Mark-Simulacrum]

I'm going to close this issue -- auto-generated Debug implementations are explicitly not guaranteed to produce the same output over time, and the libs team has discussed this (I believe for both this specific change, and in general) and confirmed as such. Further discussion can happen on internals.rust-lang.org.

[rivy]

@Mark-Simulacrum , when you say "explicitly not guaranteed to produce the same output", where is that documented? When researching this, I found no such documentation. While I don't have a problem with closing this issue as "works as designed", I don't think the issue should be closed until the behavior is documented in a place that is reasonably easy for users to find/see.

[Mark-Simulacrum]

Reopening as such, I was also unable to find such a place. I think a note on stability in std::fmt noting that the impls for all standard library types of Debug and Display are unstable in terms of output (though not existence) should be added. We'll probably want to fcp the wording and such with libs team but that can happen on the PR.

[KamilaBorowska]

@Mark-Simulacrum

impls for all standard library types of Debug and Display are unstable in terms of output

Huh, Display implementations for non-error/Escape types are unstable? That I find myself to be really surprising, and frankly something that I don't think Rust developers can realistically change at this point as many programs depend on Display implementations to stay stable.

In particular, Display for integers, floating point numbers, characters, strings, booleans, IP addresses, socket addresses, and std::fmt::Arguments should remain stable. Programs do depend on those.

[Mark-Simulacrum]

To be clear, I'm not an authority here -- I don't know that the libs team has made the assertion about Display impls before. I do think that it may make sense to say that Display impls are stable in terms of output, though perhaps such a broad claim might be excessive (an audit might be in order for where we implement Display).

[alilleybrinker]

Doesn't look like anything has happened on this. I'm happy to attempt an explanation in the docs. The question is where to put it. My first thought is at the end of the last paragraph before "Examples" in the API docs for std::fmt::Debug, specifically changing:

Current

/// This trait can be used with `#[derive]` if all fields implement `Debug`. When
/// `derive`d for structs, it will use the name of the `struct`, then `{`, then a
/// comma-separated list of each field's name and `Debug` value, then `}`. For
/// `enum`s, it will use the name of the variant and, if applicable, `(`, then the
/// `Debug` values of the fields, then `)`.

Possible Change

/// This trait can be used with `#[derive]` if all fields implement `Debug`. When
/// `derive`d for structs, it will use the name of the `struct`, then `{`, then a
/// comma-separated list of each field's name and `Debug` value, then `}`. For
/// `enum`s, it will use the name of the variant and, if applicable, `(`, then the
/// `Debug` values of the fields, then `)`. __Derived `Debug` formats are not
/// stable, and so may change with future Rust versions.__

Is this notice enough? Should there be an additional notice in std::fmt? Was a conclusion ever reached on the stability of Display impls for types in std?

[RalfJung]

@alilleybrinker that seems like a good start!

Orthogonally to this, though, Debug for types in std may also change, whether they are derived or not. So maybe it is worth having a subsection, like ## Stability or so, discussing the various aspects of stability around Debug.

Was a conclusion ever reached on the stability of Display impls for types in std?

Not that I know of. And as @Mark-Simulacrum said, an audit of the existing impls should probably be done before we promise anything like that.

[alilleybrinker]

Hm. Alright, I think you're right that a new Stability section is probably best. Thanks!

[alilleybrinker]

With #72551 addressing the Debug docs, perhaps a new issue should be opened for the question of auditing the stability guarantees of stdlib Display impls.

[RalfJung]

@alilleybrinker sure. In either case I am closing this issue now. Thanks for the PR!