Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

format!() ignores width when displaying enums #67162

Open
dimo414 opened this issue Dec 8, 2019 · 6 comments
Open

format!() ignores width when displaying enums #67162

dimo414 opened this issue Dec 8, 2019 · 6 comments
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one.

Comments

@dimo414
Copy link
Contributor

dimo414 commented Dec 8, 2019

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
Copy link
Contributor Author

dimo414 commented Dec 8, 2019

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
Copy link
Contributor

ExpHP commented Dec 9, 2019

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
Copy link
Contributor Author

dimo414 commented Dec 9, 2019

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
Copy link
Contributor

ExpHP commented Dec 9, 2019

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!)

@Elinvynia Elinvynia added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools labels May 20, 2020
@michiel-de-muynck
Copy link
Contributor

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
Copy link
Contributor Author

dimo414 commented Sep 14, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one.
Projects
None yet
Development

No branches or pull requests

4 participants