rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
99.13k stars 12.8k forks source link

format!() ignores width when displaying enums #67162

Open dimo414 opened 4 years ago

dimo414 commented 4 years ago

Similar to the issue reported in #55584 (and #55749), the width parameter is ignored when displaying enums, e.g. this fails:

assert_eq!(&format!("{:10}", Enum::FOO), "foo       ");

Playground Example

Notice this is with Display, not Debug. There's some question in the linked issues what the desired behavior for Debug is, but I think it's a bug that Display formatting would not respect width (and precision).

I'd be willing to take a crack at fixing this if I could get some code pointers.

dimo414 commented 4 years ago

Looking at Formatter I see I was confused how formatting works, and the onus is on the Display impl to support whatever formatting features it wants to.

In hindsight this makes sense, but was totally opaque to me and not obvious from the docs, though I see now:

It is up to each format trait implementation to correctly adhere to the requested formatting parameters. The values of these parameters will be listed in the fields of the Formatter struct.

It'd be great if this caveat could be pulled out into a dedicated section about custom impls. Even better if unsupported operations weren't just silently ignored, but that ship has probably sailed.

ExpHP commented 4 years ago

Right, you probably want your implementation of fmt to forward directly to fmt::Display::fmt("foo", f) rather than using write! (a fairly common mistake which is not at all helped by some of the example impls in the current docs).

dimo414 commented 4 years ago

Ah, thanks! So should https://doc.rust-lang.org/std/fmt/trait.Display.html#examples-1 instead use fmt::Display::fmt(&format!(f, "({}, {})", self.longitude, self.latitude), f)?

ExpHP commented 4 years ago

I would say, no. I will add some qualifications to what I said earlier: Using write! when you could forward is a mistake for wrapper types. I.e. if something is supposed to behave like a value of some type T when formatted, you should forward. (and different but related: for Debug specifically, using write!("{:?}") in a Debug impl is almost universally wrong, as one should typically be using f.debug_struct or similar to correctly support {:#?}).

I don't believe there is any universal, ideal behavior for Display that should be followed by all types. This is clear from how differently precision (i.e. {:.3}) works for strings and floats. Granted, the behavior of width (i.e. {:20}) is pretty consistent between primitive types, but having it work on arbitrary types is difficult because a composite type has no way to determine in advance how wide the unpadded output will be. You can work around this as you have by using format!; but this requires allocation and is unfit for #[no_std] libraries.

Besides, I don't see any reason why the author of a crate should be discouraged from implementing completely different semantics for width, e.g.:

assert_eq!(
    format!("{}", Position { longitude: 3., latitude: 7. }),
    "(3, 7)",
);
assert_eq!(
    format!("{:5.2}", Position { longitude: 3., latitude: 7. }),
    "(  3.00,   7.00)",
);

So if you have an enum that is supposed to represent one of a fixed set of string constants, that's a reasonable time to forward. Likewise if you simply desire having padding work for that type. But in general you shouldn't expect width to work for arbitrary types, nor should you feel obligated to make it work. (if a downstream crate needs padding, it can easily take the String-allocating route by itself!)

michiel-de-muynck commented 3 years ago

If I understand correctly, this is not a bug and the issue can be closed. Any implementation of Display needs to ensure it implements the parameters it wants to support, either manually or by delegating to a different Display implementation. It does not (and cannot) happen automatically.

dimo414 commented 3 years ago

a fairly common mistake which is not at all helped by some of the example impls in the current docs

I assume this is still a point of confusion that could be improved in documentation.