Open RalfJung opened 1 year ago
Is there precedence for this on any other types?
Not that I am aware of. But String
is a very fundamental type and at the same time it is far from obvious that it would implement fmt::Write
or io::Write
. People writing to a File
will probably have io::Write
imported anyway, or will at least have std::io
around and be generally looking in the right direction.
One problem I could see is that the implementation of fmt_write
will take precedence over any trait impl. So it would prevent using this macro with io::Write
on String
, if we wanted to add an impl for that.
Even if we added such an impl, of course its write_fmt
would behave exactly the same as fmt::Write::write_fmt
so that would not be a problem. There's no reasonable world in which io::Write
and fmt::Write
are both implemented on String
but their write_fmt
behave differently.
If I understand the problem at hand, this could use inherent traits, if those ever existed.
To add, r-a does this so frequently that we have a similar macro in our code base https://github.com/rust-lang/rust-analyzer/blob/62268e474e9165de0cdb08d3794eec4b6ef1c6cd/crates/stdx/src/macros.rs#L13-L20
(though we discard the result since it doesn't ever fail for us)
Yeah writing to a String is never supposed to fail, but I prefer a panic over silently throwing away an error.
Given a custom Debug/Display impl can fail even when writing to strings this proposal returning a Result
is appropriate imo.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c5e2ef87f493f8e15dafea9a967f2aa8
struct S;
impl std::fmt::Display for S {
fn fmt(&self, _: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
Result::Err(std::fmt::Error)
}
}
fn main() {
use std::fmt::Write as _;
write!(String::new(), "{}", S).unwrap();
}
Given a custom Debug/Display impl can fail even when writing to strings this proposal returning a Result is appropriate imo.
The documentation says they are not allowed to do that, so this is a buggy Display
impl. (I don't know where those docs are, but I am sure I have seen docs saying that Debug-Display are only allowed to forward errors from the formatter, and not raise their own errors. The ToString
impl relies on that.)
TIL
https://doc.rust-lang.org/std/fmt/struct.Error.html states
This type does not support transmission of an error other than that an error occurred. Any extra information must be arranged to be transmitted through some other means.
but I'll have to say that is very much not discoverable, nor does it express that I guess (was the closest I could find). I assume this is just describing the fact that the error has no payload. So ye I can't really find where this is described, aside from the comment on the ToString
impl
I don't think those docs are meant to imply "no new errors during Debug/Display". I thought I had seen a fairly clear statement of that rule but I don't remember where...
I thought I had seen a fairly clear statement of that rule but I don't remember where...
Possibly here https://github.com/rust-lang/rust/blob/cedbe5c715c1fa9359683c5f108bed2054ac258b/library/alloc/src/string.rs#L2441-L2446. I'm not sure it's normative though, since it's so buried away. Notably we panic in these cases deliberately, rather than ignoring the error, because such an error may exist. I would be opposed to changing that, for example.
I think you mean this? https://doc.rust-lang.org/std/fmt/index.html#formatting-traits
Additionally, the return value of this function is
fmt::Result
which is a type alias ofResult
<(),
std::fmt::Error
>
. Formatting implementations should ensure that they propagate errors from theFormatter
(e.g., when callingwrite!
). However, they should never return errors spuriously. That is, a formatting implementation must and may only return an error if the passed-inFormatter
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.
Given the premise that formatting implementations "must and may only return an error if…", it would be really nice if String::write_fmt()
returned Result<(), Infallible>
to communicate that it can't actually fail and not require the caller to use either _ =
or .unwrap()
to silence the unused_must_use
warning. To my eyes, that's the one big inelegance of using write!
on strings.
Unfortunately, that would cause a type error in any existing code that tries to propagate std::fmt::Error
out (though adding impl From<Infallible> for fmt::Error
would solve that for ?
s) when the inherent method shadows fmt::Write
. But perhaps there's a solution I haven't thought of, or a crater run will show that it's OK.
(And ignoring a Result<(), Infallible>
or Result<(), !>
still prompts an unused_must_use
warning, but that could be fixed.)
I think if we do this it should just return fmt::Error
. The slight convenience factor isn't worth the potential breakage. I'd be willing to try a crater run, but I expect this breakage does exist.
BTW, String
does not, and can not, implement io::Write
, because io::Write
allows writing non-UTF-8 data.
However, there is an edge case that could break:
trait CustomWrite {
fn write_fmt(&self, a: std::fmt::Arguments<'_>) -> () {}
}
impl CustomWrite for String {}
write!(&mut s, "");
One thought I had was to add a new trait for infallible write-to-string in the prelude:
mod prelude {
pub trait WriteStringInfallible {
fn write_fmt(&mut self, args: std::fmt::Arguments<'_>);
}
impl WriteStringInfallible for String {
fn write_fmt(&mut self, args: std::fmt::Arguments<'_>) {
let _ = <Self as std::fmt::Write>::write_fmt(self, args);
}
}
}
use prelude::*;
fn main() {
let mut s = String::new();
write!(s, "Hello, world!");
println!("{s}");
}
Currently this results in an ambiguity error. Could we change the resolver to automatically pick an explicitly imported trait method if all of the other options are glob-imported?
Then you could still do
use std::fmt::Write;
To use write!
with that trait instead.
But I think an easier solution would be a way to suppress must_use
. Then we can put that attribute on String::write_fmt
:
impl String {
#[suppress_must_use]
fn write_fmt(&mut self, args: fmt::Arguments<'_>) -> fmt::Result {
<Self as fmt::Write>::write_fmt(self, args)
}
}
On IRLO, some alternatives have been suggested to achieving the goal of "easily and efficiently extending a string with formatting":
s += format_args!("hello {world}");
s.push_display(&format_args!("hello {world}"));
I feel like something like this is probably preferable over my original proposal here.
Is there precedence for this on any other types?
Formatter::write_fmt
is also an inherent method:
https://doc.rust-lang.org/stable/std/fmt/struct.Formatter.html#method.write_fmt
Making s += format_args!("hello {world}");
work seems very harmless. I see no reason not to add this.
The only open question for that is how to handle a misbehaving (error-returning) Display/Debug/... implementation. Panicking seems a bit cleaner than just ignoring, but might result in more generated code, which might not be worth given that the docs say:
contrary to what the function signature might suggest, string formatting is an infallible operation
Making
s += format_args!("hello {world}");
work seems very harmless.
It might lead to inference failures, because right now there's only one RHS type for AddAssign<&str>
. So for example, with two String
values, s += s2.as_ref()
has to infer that using AsRef<str>
, and I'm not sure that will happen if there's another AddAssign
possibility. (Maybe it's saved if the other RHS isn't a reference?)
At the very least, we'll need a crater run to gauge this.
(edit): This point was also raised here: https://internals.rust-lang.org/t/mut-string-format-args/19461/18
IMO we should definitely panic on errors when debug assertions are enabled. There's no good reason to silently ignore what is clearly a bug.
I have less of a strong opinion for builds without debug assertions.
We discussed this in a recent libs-api meeting.
We don't think we should be adding an inherent String::write_fmt
that returns a fmt::Result
, as the Err
case is not useful. We'd love to find an effective way to append a fmt::Arguments
to a String
without having to (needlessly) handle fmt/io errors.
A few of the possible directions are a +=
operator implementation, some edition-dependent infallible write_fmt
method, or a new trait (with write_fmt
method) for infallible formatting.
Unfortunately, implementing the +=
operator currently breaks a lot of things, as pointed out above: https://github.com/rust-lang/libs-team/issues/261#issuecomment-1802313172
The other possible directions might be made impossible or much trickier if we were to add an inherent String::fmt
with the same signature as fmt::Write::write_fmt
. We should not close off these possibilities until we have figured out what path forward to take.
I wouldn't rule out +=
just yet. https://github.com/rust-lang/libs-team/issues/261#issuecomment-1802313172 says that it breaks s += s2.as_ref()
(AsRef<str>
) but I don't think it does. See playground.
Proposal
Problem statement
It would be great if this code compiled:
Using
write!
to append to aString
is one of these neat Rust things that are not obvious to discover, but once you know about it it is amazingly useful.Solution sketch
The code currently fails saying that
io::Write
orfmt::Write
needs to be imported or awrite_fmt
method needs to be added. (The right trait to import isfmt::Write
, as I found out by trial-and-error.)From the error message it sounds like that could be avoided if we simply added a
write_fmt
inherent method toString
. That would just get one minor roadblock out of the way which sounds like a win to me. :)Alternatives
We could decide the roadblock isn't bad enough to warrant doing anything.
What happens now?
This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.
Possible responses
The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):
Second, if there's a concrete solution: