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

Consider checking for formattee errors in ToString::to_string and format!() #40103

Closed
nox opened this issue Feb 26, 2017 · 6 comments
Closed

Comments

@nox
Copy link
Contributor

nox commented Feb 26, 2017

In both ToString for T: Display and format!(), it is assumed that no Display implementation will ever return Err(_) because none of the methods of Write for String do.

Given the Display trait does not document that it should not fail on its own, I think both ToString for T: Display types and format!() should unwrap the calls to Write::write_fmt instead of completely ignoring their result.

@SimonSapin
Copy link
Contributor

SimonSapin commented Feb 26, 2017

I think it is an error for an implementation of Display (or other trait of that family) to return Err for a reason other than because Formatter::write_* did. If it’s not in the documentation, we should add it.

So +1 for panicking when that happens.

@SimonSapin
Copy link
Contributor

I think it is an error

Error as in programmer mistake, a bug. Not a Result::Err(_). (Overloaded terminology…)

@ollie27
Copy link
Member

ollie27 commented Feb 26, 2017

It's documented here:

Formatting implementations should ensure that they propagate errors from the Formatter (e.g., when calling write!) however, they should never return errors spuriously. That is, a formatting implementation must and may only return an error if the passed-in Formatter returns an error. This is because, contrary to what the function signature might suggest, string formatting is an infallible operation. This function only returns a result because writing to the underlying stream might fail and it must provide a way to propagate the fact that an error has occurred back up the stack.

@nox
Copy link
Contributor Author

nox commented Feb 26, 2017

@ollie27 Oh thanks.

@SimonSapin
Copy link
Contributor

@alexcrichton, you replaced .unwrap() with let _ = in #14115 / 1de4b65#diff-1d71e11e042c3b99e8acdff366825e70L552, but it’s part of a larger commit and the commit message doesn’t mention this. Do you remember why?

@alexcrichton
Copy link
Member

@SimonSapin that was also three years ago as part of a 6kloc PR. Unfortunately no, I don't remember why that particular line is that way. It was likely because at the time I assumed that no one would ever manufacture fmt::Error, I was only considering I/O errors translating to fmt::Error

SimonSapin added a commit to SimonSapin/rust that referenced this issue Mar 1, 2017
… 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 rust-lang#40103
frewsxcv added a commit to frewsxcv/rust that referenced this issue Mar 2, 2017
…ter, r=alexcrichton

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

… 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 rust-lang#40103
frewsxcv added a commit to frewsxcv/rust that referenced this issue Mar 2, 2017
…ter, r=alexcrichton

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

… 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 rust-lang#40103
frewsxcv added a commit to frewsxcv/rust that referenced this issue Mar 2, 2017
…ter, r=alexcrichton

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

… 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 rust-lang#40103
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants