-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Add format_to! macro #76240
Add format_to! macro #76240
Conversation
r? @sfackler (rust_highfive has picked a reviewer for you, use r? to override) |
|
||
#[unstable(feature = "liballoc_internals", issue = "none", reason = "implementation detail")] | ||
#[doc(hidden)] | ||
pub fn __push_fmt(&mut self, args: fmt::Arguments<'_>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't it just be an inherent write_fmt
method, so write!(...) always works on String instead of introducing a semi-hidden method and a whole new macro?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That won’t be backward compatible: write_fmt already exists on String (via Write trait) and returns a result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I guess it would have to have the same signature. So I guess the unwrapping would unfortunately stay in that case. Though maybe it could use Result<(), !> and somehow be compatible and not require unwrapping... (Not sure if the stars align for that one though🤔)
@matklad Ping from triage, CI is still red here. |
@crlf0710 this is still waiting on review:
|
Another option might be to add a trait Unfortunately, importing both |
That failure is fixed by re-exporting the macro from Lines 374 to 375 in a9cd294
The next failure would be solved by adding rust/library/alloc/src/macros.rs Lines 36 to 37 in a9cd294
|
It's a version of `format!` which allows appending to an existing `String`. Unlike `write!`, it doesn't require a trait import and unwrap. See https://internals.rust-lang.org/t/add-an-easier-way-to-append-t-display-to-string-to-stdlib/10607 for some preliminary discussion. See /~https://github.com/rust-analyzer/rust-analyzer/blob/3ffa915cbcf4d7a3988142cd94da0463acc87c8a/crates/stdx/src/macros.rs#L12-L19 for an equivalent macro used in rust-analyzer quite a bit.
I agree that the current situation with Here's an attempt at that: #78822 |
We discussed this PR in the libs team meeting and agreed the current situation with There was a slight preference for something like @matklad Would love to hear your thoughts about alternative solutions. (E.g. |
This actually surprises me, I personally have a strong preference against such classes of the solutions. Seems like mine and team's valuations of trad-offs are different in this case (and, to be clear, I don't claim that mine are better). To, me, making In contrast, In terms of alternatives, the one I like less than format!(buf, "format_str", args) => drop(write!(buf, "format_str", args)); This can be done purely on the macro-level (declarative macros can distinguish between literal first argument, and and a more general expression). If we look super-long term, I sort-of feel that the language will grow first-class format string support ( buf += f"hello {name}";
// Equivalent to today's
buf += format_args!("hello {name}", name = name); In this light, we can bet on the future and overload |
We're a bit careful with adding macros for specialized use cases, since macros end up in the root of the public interface of
An alternative like
I personally feel like
There are some plans to try to make let fmt = "hello {}".to_string();
let s = format!(fmt, "world"); |
Triage: What's the current status of this? |
I guess this is stuck in a place where the annoyance is relatively mild, and all available solutions are not without drawbacks. I think it's fair to decide that we in general want to fix this somehow, but, given that this is low priority, and that there might be future considirations here, to just punt on this.
I don't find any, but to me it feels very weird that a unit-returning operation doesn't actually return unit. I don't think this will be a large problem in practice (except for cases like I guess there's an additional minor annoyance that it still requires a trait import, and that there are still two traits one can import. |
Triage: What's next steps here? |
☔ The latest upstream changes (presumably #81271) made this pull request unmergeable. Please resolve the merge conflicts. |
I think I am gong to close this, in the interest of saving the bandwidth -- I do think that we need a solution for this in std, but this is not a pressing problem. |
It's a version of
format!
which allows appending to an existingString
.Unlike
write!
, it doesn't require a trait import and unwrap.See
https://internals.rust-lang.org/t/add-an-easier-way-to-append-t-display-to-string-to-stdlib/10607
for some preliminary discussion.
See
/~https://github.com/rust-analyzer/rust-analyzer/blob/3ffa915cbcf4d7a3988142cd94da0463acc87c8a/crates/stdx/src/macros.rs#L12-L19
for an equivalent macro used in rust-analyzer quite a bit.
If this is roughly a good idea, I'll add some tests & a tracking issue.
I am also not sure why doctest is failing. How do
liballoc
macros end up in thestd
prelude?