Final comment period for Debug Builders stabilization

[sfackler]

TL;DR

We’re looking to stabilize the debug builder APIs in the near future, so it’s time to make sure we’re happy with the functionality. Speak now or forever hold your peace.

Background

The debug builder APIs are part of the implementation of RFC 640: Debug Improvements. It proposed that the # “alternate” flag would be treated by fmt::Debug implementations as a request to “pretty print” output - turning

{ "foo": ComplexType { thing: Some(BufferedReader { reader: FileStream { path: "/home/sfackler/rust/README.md", mode: R }, buffer: 1013/65536 }), other_thing: 100 }, "bar": ComplexType { thing: Some(BufferedReader { reader: FileStream { path: "/tmp/foobar", mode: R }, buffer: 0/65536 }), other_thing: 0 } }

into

{
    "foo": ComplexType {
        thing: Some(
            BufferedReader {
                reader: FileStream {
                    path: "/home/sfackler/rust/README.md",
                    mode: R
                },
                buffer: 1013/65536
            }
        ),
        other_thing: 100
    },
    "bar": ComplexType {
        thing: Some(
            BufferedReader {
                reader: FileStream {
                    path: "/tmp/foobar",
                    mode: R
                },
                buffer: 0/65536
            }
        ),
        other_thing: 0
    }
}

The information content is the same - the only changes present are newlines and whitespace.

It would be a huge pain to have to manually handle both output variants via manual write! calls in fmt::Debug implementations, so some builder-style types were added to handle the complications automatically. The builders are currently used by #[derive(Debug)] but are unstable so manual Debug implementations can’t use them in stable code.

The API

The entry points consist of a series of methods on the fmt::Formatter struct:

impl<'a> Formatter {
    pub fn debug_struct<'b>(&'b mut self, name: &str) -> DebugStruct<'b, 'a> { ... }
    pub fn debug_tuple<'b>(&'b mut self, name: &str) -> DebugTuple<'b, 'a> { ... }
    pub fn debug_list<'b>(&'b mut self) -> DebugList<'b, 'a> { ... }
    pub fn debug_set<'b>(&'b mut self) -> DebugSet<'b, 'a> { ... }
    pub fn debug_map<'b>(&'b mut self) -> DebugMap<'b, 'a> { ... }
}

Each of these types defines a builder-style API:

impl<'a, 'b> DebugStruct<'a, 'b> {
    pub fn field(self, name: &str, value: &fmt::Debug) -> DebugStruct<'a, 'b> { ... }
    pub fn finish(self) -> fmt::Result { ... }
}

impl<'a, 'b> DebugTuple<'a, 'b> {
    pub fn field(self, value: &fmt::Debug) -> DebugTuple<'a, 'b> { ... }
    pub fn finish(self) -> fmt::Result { ... }
}

impl<'a, 'b> DebugList<'a, 'b> {
    pub fn entry(self, value: &fmt::Debug) -> DebugList<'a, 'b> { ... }
    pub fn finish(self) -> fmt::Result { ... }
}

impl<'a, 'b> DebugSet<'a, 'b> {
    pub fn entry(self, value: &fmt::Debug) -> DebugSet<'a, 'b> { ... }
    pub fn finish(self) -> fmt::Result { ... }
}

impl<'a, 'b> DebugMap<'a, 'b> {
    pub fn entry(self, key: &fmt::Debug, value: &fmt::Debug) -> DebugMap<'a, 'b> { ... }
    pub fn finish(self) -> fmt::Result { ... }
}

Each variant corresponds to a common output format:

• DebugStruct: MyType { field1: value1, field2: value2 }
• DeubgTuple: MyType(value1, value2)
• DebugList: [value1, value2]
• DebugSet: {value1, value2}
• DebugMap: {key1: value1, key2: value2}

Common usage in a Debug implementation would typically look something like

impl fmt::Debug for MyType {
    fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
        fmt.debug_struct("MyType")
            .field("field1", &self.field1)
            .field("field2", &self.field2)
            .finish()
    }
}

Stabilization

An open PR proposes to stabilize these APIs as-is. Is there anything that we should change before that happens? Potential points of discussion that have been mentioned before:

• Should we use debug_tuple_struct and DebugTupleStruct instead of debug_tuple and DebugTuple? It’s technically more correct but adds verbosity.
• DebugStruct and DebugTuple have a field method while DebugList, DebugSet and DebugMap have an entry method. Should they be unified? It’s inconsistent but does match to what we normally refer to the contents of those kinds of structures by.
• Should the builder methods take &mut self instead of self? Taking self by value makes the API a bit more foolproof (you can’t add more fields/entries after calling finish for example) but does mean you have to reassign the builder when using it in more complex cases:

let mut builder = fmt.debug_struct("MyType");
if some_condition {
    builder = builder.field("conditional_field", &self.field);
    // vs simply
    // builder.field("conditional_field", &self.field);
}
builder.finish()

On the other hand, it allows for a nice one liner when implementing Debug for common data structures:

impl<K, V, S> Debug for HashMap<K, V, S>
    where K: Eq + Hash + Debug, V: Debug, S: HashState
{
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        self.iter().fold(f.debug_map(), |b, (k, v)| b.entry(k, v)).finish()
    }
}

Thoughts?

cc @alexcrichton @aturon @bluss

[alexcrichton]

There are some proposed builder guidelines which indicate that &mut self should be preferred unless there’s a reason to do so otherwise, so I would personally prefer to see these as &mut self and perhaps add future convenience methods to handle the fold case better. For example we could add f.debug_map().entries(self) where the entries method took I: IntoIterator, ....

I’m kinda ambivalent about debug_tuple and debug_tuple_struct as one would also debug tuples themselves with debug_tuple (and an empty name), and I also don’t have a super strong opinion about field vs entry. I agree that the current terms match the colloquial usage, but it would perhaps be nice to select a name which is both consistent an colloquial (not sure if this exists though).

[seanmonstar]

Seeing this post, I tried out the API in hyper (by hiding it behind a #[cfg(feature = "nightly")] flag), and immediately noticed what Alex said. Using the fold method felt very odd. I like his suggestion to take a IntoIterator<Item=(Debug, Debug)> instead.

[sfackler]

@alexcrichton Making it harder to screw up is a decent reason to prefer self to &mut self, IMO. If we went with a &mut self model, what would we want the behavior of an entry/field call after finish to be? Options I see are panicking, returning a fmt::Error or going on and printing anyway.

@seanmonstar Yep, something like that seems reasonable, though I’d guess we’d add it unstable for a bit to make sure we were happy with it. In particular, I’m trying to make sure to craft these APIs to minimize binary size (this is why the methods take &Debug trait objects instead of monomorphizing for example). Probably something like

impl<'a, 'b> DebugMap<'a, 'b> {
    fn entries<K, V, I>(self, it: I) -> DebugMap<'a, 'b>
        where K: Debug, V: Debug, I: IntoIterator<Item=(K, V)> { .. }
}

[alexcrichton]

The primary use case for this API is not so much for correctness but rather just getting a debug representation of each type, so I would personally prioritize the API ergonomics/usage/conventions ahead of statically preventing errors. For example I would consider it OK to have a boolean flag to indicate when finish has been called and just have all future function calls immediately return Ok(()) (or the equivalent)

[sfackler]

Here’s a commit switching the methods to take &mut self and adding an entries method to DebugSet, DebugMap, and DebugList: https://github.com/rust-lang/rust/pull/25548.

It seems like a pretty reasonable change on the whole.

[sfackler]

The builder API (with the &mut self and entries changes) has been marked as stable for the 1.2 release.

[alexcrichton]

Thanks so much @sfackler for pushing this through!

[canndrew]

Might be a bit late for this but it would be nice if there was a way to add comments to the Debug Builders output. I’m thinking this sort of thing:

HashMap {
    "foo": ComplexType { /* A type that is very complex */
        thing: Some( /* As opposed to None */
            BufferedReader {
                /* The inner Reader wrapped by BufferedReader */
                reader: FileStream {
                    path: "/home/sfackler/rust/README.md",
                    mode: R
                },
                buffer: 1013/65536
            }
        ),
        other_thing: 100 /* blah blah blah */
    },
}

This could be done by adding additional Option<&str> args to some of the debug builder methods. eg.

fn debug_struct<'b>(&'b mut self, name: &str, comment: Option<&str>) -> ...

f.debug_struct("ComplexType", Some("A type that is very complex"));