Panic on errors in format! or <T: Display>::to_string

[SimonSapin]

… instead of silently ignoring a result.

fmt::Write for String never returns Err, so implementations of Display (or other traits of that family) never should either.

Fixes #40103

[SimonSapin]

By the way, this function and this method are very similar, but only one calls String::shrink_to_fit without an apparent reason for the difference. Should both of them do so? Neither?

[SimonSapin]

Same for using estimated_capacity as the initial string capacity.

[est31]

Isn't this a breaking change? It could in fact lead to UB in existing programs when people update their rustc. E.g. they checked the implementation of the functions and detected no panic, and then they assumed they can call it from C code without any code surrounding it. Now if it panics, it will give UB.

[hanna-kruppe]

It is a behavioral change and so there may be an argument that it's a breaking change and can't be done. But I strongly reject the idea that one can inspect std source code, find no source of panics, and then assume there will never be any panics.

Obviously some simple APIs are intended and can be assumed not to panic (e.g., wrapping_$op). And obviously, if a particular change causes big problems in practice, it shouldn't be done. But users shouldn't rely on implementation artifacts that they can glean from the current source code when they are not documented and can't reasonably be seen to be implied. I would consider the scenario you describe completely irresponsible, not only because it relies on implementation details, but also because it does not account for the fact that mistakes happen (both in future changed to std, and in all the non-std code that factors into the observation). Besides, even code that isn't expected to panic can relatively easily be hardened against it, and IMO should be when UB is at stake.

[alexcrichton]

@ollie27 commented that this is a breaking change (e.g. we've documented the opposite)

In light of that, could you elaborate @SimonSapin on why we should also update that documentation to say otherwise?

[SimonSapin]

@alexcrichton, I don’t understand how "this is a breaking change" follows from @ollie27’s comment. On the contrary, @ollie27 points out that the restriction enforced by this PR is already documented. In other words, this PR makes std panic when a Display implementation is buggy (since does something that the doc already says it’s not supposed to do).

I don’t think we should update the documentation, it already says what I want it to say.

[alexcrichton]

I suppose it depends on how you read it. I'm seeing:

string formatting is an infallible operation

which to me implies 'does not panic'

[SimonSapin]

To me that precise quote means “Returning Err from Display::fmt when Fromatter::write_* didn’t is a bug”. Similar to indexing out of bounds, it should never happen unless the program is buggy, and panicking is the appropriate thing to do if it does.

[alexcrichton]

Discussed during libs triage today the conclusion was that this is a good PR to merge, but @SimonSapin can you update the docs that @ollie27 pointed out to clarify the panic semantics?

[SimonSapin]

Done.

[ollie27]

Do we really need to document that consumers of incorrectly implemented traits can panic!? I thought that went without saying. For example println! currently panic!s when passed an incorrect implementation of Display.

As the aim here is to catch bugs in user code it might be better to add this assertion to the write_fmt method of String (and Vec<u8>) directly.

[alexcrichton]

@ollie27 would you prefer we don't document it? Or would you prefer we don't change the behavior?

Emprical evidence I think shows the documentation is necessary (I personally interpreted it differently)

[ollie27]

This seems like a perfectly reasonable change as long as there's no significant performance impact of course.

We could add a comment saying that if you implement these traits incorrectly then consumers may panic! or otherwise behave unexpectedly but that's true of basically any trait as far as I know. It would be especially weird to single out format! and ToString when println! panic!s under the exact same circumstances. I'm sure the docs can be improved though.

[SimonSapin]

@ollie27 I think that what you describe is already what’s in the pull request? Note that I pushed an amended commit today.

[ollie27]

I hope I'm looking at the latest version. You added "The format! macro and the ToString for T where T: Display implementation will panic if a formatting implementation returns an errors.", but that's also true for print! and println!. I'm trying to say that it shouldn't be necessary to document these panic!s.

[SimonSapin]

shrugs

I added this because Alex asked for it.

[alexcrichton]

Ok then let's remove it

[SimonSapin]

Removed the new bit of module-level docs for std::fmt, but kept the ones specific to format! and impl<T: fmt::Display + ?Sized> ToString for T.

[alexcrichton]

@bors: r+

[bors]

📌 Commit f2017f4 has been approved by alexcrichton

[arielb1]

This is a breaking change.