Clarify/fix formatting docs concerning fmt::Result/fmt::Error

[Stebalien]

1. fmt::Result != io::Result<()>
2. Formatters should only propagate errors, not return their own.

Confusion on reddit: https://www.reddit.com/r/rust/comments/4yorxr/is_implt_tostring_for_t_where_t_display_sized_a/

[rust-highfive]

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

[Stebalien]

r? @steveklabnik

[briansmith]

This kind of documentation is an indication that the API is designed wrongly. In particular, the user of the thing that implements Debug or whatever can pass a fmt: &mut Formatter that keeps track of whether it failed, and then query fmt after the operation to see if it had any failure. Thus, the fmt method should just not have a return type.

In some cases, the implementation of fmt has to choose between panicking and returning Err(fmt::Error). In general, I don't want my code to cause panics so I'm included to return Err(fmt::Error). The suggested documentation says this is not how it is intended to do, but it still isn't clear why it would be worse than calling panic!.

[Stebalien]

@briansmith I agree it's not ideal but it's stable...

In particular, the user of the thing that implements Debug or whatever can pass a fmt: &mut Formatter that keeps track of whether it failed, and then query fmt after the operation to see if it had any failure. Thus, the fmt method should just not have a return type.

That's how it works currently. The point of the result is to tell caller that an error has occurred and that it should stop formatting. The actual error is stashed elsewhere. For example:

impl Display for Something {
    fn fmt(&self, f: &mut Formatter) -> fmt::Result {
        Display::fmt(&self.message, f)?; // bail early on error
        Display::fmt(&":", f)?; // bail early on error
        Display::fmt(&self.detail, f)?;
    }
}

In some cases, the implementation of fmt has to choose between panicking and returning Err(fmt::Error). In general, I don't want my code to cause panics so I'm included to return Err(fmt::Error). The suggested documentation says this is not how it is intended to do, but it still isn't clear why it would be worse than calling panic!.

If it's a bug (programmer error or variant violation), it should panic; that's the point of panic. However, if your Display implementation can fail normally, you shouldn't be implementing Display; formatting is not a fallible operation (the documentation should probably explicitly mention this).

Also, in this case, the error will just be dropped on the floor in trait implementations like ToString.

[Stebalien]

@briansmith is the updated documentation clear now?

[briansmith]

Yes, it is clearer. You might clarify it further by saying more of what you said in your comment above: "If any other kind of error could occur, that is a sign that Display and/or Debug shouldn't be implemented for the given type; regardless the function must not return fmt::Error in such cases; it may panic! instead."

[Stebalien]

@briansmith now that I actually pushed the change I was asking about... any better?

[briansmith]

Yes, it's clearer.

[GuillaumeGomez]

👍

cc @steveklabnik

[steveklabnik]

@bors: r+ rollup

[bors]

📌 Commit c7d5f7e has been approved by steveklabnik

[bors]

⌛ Testing commit c7d5f7e with merge d62f786...

[bors]

💔 Test failed - auto-mac-64-opt-rustbuild

[arielb1]

@bors retry