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

Add format_to! macro #76240

Closed
wants to merge 1 commit into from
Closed

Add format_to! macro #76240

wants to merge 1 commit into from

Conversation

matklad
Copy link
Member

@matklad matklad commented Sep 2, 2020

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.

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 the std prelude?

@matklad matklad added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Sep 2, 2020
@rust-highfive
Copy link
Collaborator

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

library/alloc/src/string.rs Outdated Show resolved Hide resolved

#[unstable(feature = "liballoc_internals", issue = "none", reason = "implementation detail")]
#[doc(hidden)]
pub fn __push_fmt(&mut self, args: fmt::Arguments<'_>) {
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

@CryZe CryZe Sep 2, 2020

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

@crlf0710
Copy link
Member

@matklad Ping from triage, CI is still red here.

@matklad
Copy link
Member Author

matklad commented Sep 21, 2020

@crlf0710 this is still waiting on review:

  • I want to get a 👍 from T-libs that this is a good idea before I invest more time into writing tests
  • I don't know exactly why the build fails (some interraction between prelude & unstable macros perhaps), so I'd want to either have guidance on this one, or a 👍 from T-libs to justify digging into this myself

@crlf0710 crlf0710 added the I-needs-decision Issue: In need of a decision. label Oct 8, 2020
@m-ou-se
Copy link
Member

m-ou-se commented Oct 22, 2020

Another option might be to add a trait StringWrite (string::Write?, fmt::WriteInfallible? ..?) that also exposes a write_fmt. Then users can pick that trait instead of fmt::Write to make write!(string, ..) return ():

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=b21d2725badd95a8f98ff7dc787ad2d8

Unfortunately, importing both fmt::Write and StringWrite will error. But I suppose it's not very common to need both.

@m-ou-se
Copy link
Member

m-ou-se commented Oct 22, 2020

I don't know exactly why the build fails

That failure is fixed by re-exporting the macro from std, just like format here:

rust/library/std/src/lib.rs

Lines 374 to 375 in a9cd294

#[stable(feature = "rust1", since = "1.0.0")]
pub use alloc_crate::format;

The next failure would be solved by adding #[allow_internal_unstable(liballoc_internals)] to the macro, just like vec! here:

#[allow_internal_unstable(box_syntax)]
macro_rules! vec {

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.
@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 6, 2020
@m-ou-se
Copy link
Member

m-ou-se commented Nov 7, 2020

I agree that the current situation with write!(string, ..) is bad, but I don't really like the idea of adding a new macro specifically for String. Especially since write! is almost the same. It might be possible to fix the situation with write! without breaking things. Maybe some way of disabling Result's #[must_use] on String::write_fmt. Then write!(string, "bla {}", 123); could work without warnings, but exisitng code that expects a Result still works.

Here's an attempt at that: #78822

@Dylan-DPC-zz Dylan-DPC-zz added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 8, 2020
@m-ou-se
Copy link
Member

m-ou-se commented Nov 20, 2020

We discussed this PR in the libs team meeting and agreed the current situation with write!(string, ..) is far from ideal, but would prefer a solution that does not require a new macro.

There was a slight preference for something like #[may_ignore]. Whether that should be used on the fmt::Write trait implementation or on a new inherent method, was not discussed.

@matklad Would love to hear your thoughts about alternative solutions. (E.g. #[may_ignore], a new trait, s += format_args!(..), or something else.)

@m-ou-se m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). I-needs-decision Issue: In need of a decision. labels Nov 20, 2020
@matklad
Copy link
Member Author

matklad commented Nov 20, 2020

There was a slight preference for something like #[may_ignore].

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 write!(str_buf, "hello") "just work" would seem to be a net negative. From the user's point of view, it introduces an inconsistency between general writes, and writes to string. There would also be a smaller inconsistency in that an infailable side-effectful op should return a (), and not a Result (which might require one to add extra ;, which is annoying in, eg, match arms). If, additionally, #[may_ignore] becomes language public API, then there's one more not-small thing to learn. From the implementation point of view, the mismatch between the problem (which is almost syntactical) and the solution (extra compiler mechanism, extra trait, etc) seems inelegant.

In contrast, format_to! seems to be ... just another boring API, like format!? I do feel uneasy about its automatic addition to prelude, which does seem like it elevates the costs quite a bit. I wish std's macros were modularized.

In terms of alternatives, the one I like less than format_to!, but which seems also viable is overloading format macro:

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 (f"hello {name}!"), and, with such support, the whole family of format-like macros will go away (this is similar to what ? has done to try!). In that world, the API would probably look like this

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 += for format_args today, but I'd feel uncomfortable -- for today, the solution seems pretty inelegant, and every speculation about the future is a bet. We might want to desugar f"" to a different types than format args, for example.

@m-ou-se
Copy link
Member

m-ou-se commented Nov 20, 2020

In contrast, format_to! seems to be ... just another boring API, like format!?

We're a bit careful with adding macros for specialized use cases, since macros end up in the root of the public interface of std, and don't attach to a specific type. A std::string::write_fmt!() or a postfix string.append_fmt!(..) might have lead to less discussion, but alas.

To, me, making write!(str_buf, "hello") "just work" would seem to be a net negative. From the user's point of view, it introduces an inconsistency between general writes, and writes to string.

write!() is already inconsistent/different between io::Write and fmt::Write. It doesn't guarantee any specific return type by itself. Other than the return type, is there any other reason why this would be inconsistent?

There would also be a smaller inconsistency in that an infailable side-effectful op should return a (), and not a Result

An alternative like trait StringWrite { fn write_fmt(..) -> (); } would not have this problem, although adding another trait is not great either.

If, additionally, #[may_ignore] becomes language public API, then there's one more not-small thing to learn.

I personally feel like #[may_ignore] would not be a big change. #[must_use] only causes a warning, and we'd just use it as a tool to turn that warning off in some cases, just like how many other warnings in rustc tend to get smarter over time.

In terms of alternatives, the one I like less than format_to!, but which seems also viable is overloading format macro:

format!(buf, "format_str", args) => drop(write!(buf, "format_str", args));

There are some plans to try to make format_args!() (and format!(), etc.) work with (constant) non-literals. So that would become a problem then. Even without that feature, it might already cause some confusion when somebody tries to use a String as format string:

let fmt = "hello {}".to_string();
let s = format!(fmt, "world");

@crlf0710
Copy link
Member

Triage: What's the current status of this?

@matklad
Copy link
Member Author

matklad commented Dec 11, 2020

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.

Other than the return type, is there any other reason why this would be inconsistent?

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 match foo { bar => write!(buf, "foo"), baz => {} }), but it seems pretty inelegant.

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.

@crlf0710 crlf0710 removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 15, 2021
@crlf0710 crlf0710 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 15, 2021
@crlf0710
Copy link
Member

Triage: What's next steps here?

@bors
Copy link
Contributor

bors commented Jan 22, 2021

☔ The latest upstream changes (presumably #81271) made this pull request unmergeable. Please resolve the merge conflicts.

@matklad
Copy link
Member Author

matklad commented Jan 22, 2021

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.

@matklad matklad closed this Jan 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants