Specialize format! for simple "{}" case

[workingjubilee]

If the first argument to format! is "{}" then the rest of the args must
be something that implements fmt::Display and thus ToString. We can thus
call ToString on it. In order to type-safely eat all those arguments,
a struct wrapper inside a closure prevents misuse.

This fixes #52804.

[rust-highfive]

r? @Mark-Simulacrum

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

[workingjubilee]

Initial experiments with measureme's toolset suggest mixed results due to more effort in expanding this macro, but because the resulting code is often more familiar to Rust's existing patterns, even with this bespoke type guard, it can sometimes optimize better (or at least with less effort), but I don't have things set up to run any comprehensive/conclusive experiments yet.

[notriddle]

Why not put NeedsDisplay outside of the macro body, hide it with #[unstable] and getting at it with #[allow_internal_unstable]? This way, you don't have to expand a fresh trait on every invocation.

For example, the panic! macro does this:

rust/library/std/src/macros.rs

Line 10 in d92155b

#[allow_internal_unstable(libstd_sys_internals)]

rust/library/std/src/panicking.rs

Line 424 in d92155b

#[unstable(feature = "libstd_sys_internals", reason = "used by the panic! macro", issue = "none")]

[workingjubilee]

Ah, I am not as familiar with that sort of thing yet! It seems like a good idea.

[Mark-Simulacrum]

I am personally unsure that we want to do this. I think clippy already lints on format!("{}", foo) to suggest foo.to_string().

I would prefer that we at least move the fast closure to an associated function on NeedsDisplay, not a closure, to avoid generating more code than is necessary.

[Mark-Simulacrum]

We will want a codegen test I think for this, at least -- it looks like this is just not triggering to me, because I would expect NeedsDisplay to not resolve here. Generally we need the type to be public in order for it to be constructible outside the current crate.

[workingjubilee]

codegen – tests that compile and then test the generated LLVM code to make sure that the optimizations we want are taking effect. See LLVM docs for how to write such tests.

Ah, one of these? Well, that just made this PR more Exciting.

[Mark-Simulacrum]

Note that before working on that I would really fix this macro to make sure this arm is getting expanded, because right now that seems pretty unlikely to me. See also the comment below about needing :ident on the arm, most likely, which'll further simplify this branch.

[scottmcm]

nit: does this need to deal with a full $($arg:tt,?)+? Maybe it could just take $e:expr ,? and let all the more complicated cases still be handled by the other arms of the macro.

(I don't think there's a need to optimize format!("{}", 1, 2, 3, 4, 5, 6).)

[notriddle]

What's worse than that: even the expr version will not be correct.

Right now, format! parses named arguments, like a = 1. If that was parsed as an expression, then it would evaluate to (), and this would not pass the assertion.

let a = format!("{}", x = 1);
assert_eq!(&a, "1");

I think it actually needs to be $arg:ident, and the optimization probably wouldn't be triggered very often as a result. That's annoying.

[workingjubilee]

Oh wow, I didn't think about that kind of usage.
@notriddle Can you post or link me to some more... uh... weird examples like that? I mean, it might make me actually just close this PR but I'd like to have an opportunity to fool around with the alternate cases to see how I can handle it in the pattern matching and if it destroys the gains.

[notriddle]

https://doc.rust-lang.org/std/fmt/index.html#usage

[workingjubilee]

Right, I'm aware of the usual usages, but that doesn't include any examples that I can see which match the format!("{}", x = 1); case that I can see, I've never seen a named parameter then being passed to an "anonymous" formatting parameter.

[ollie27]

but that doesn't include any examples that I can see which match the format!("{}", x = 1); case that I can see

That's because it's a bug and shouldn't be accepted: #45256

[Dylan-DPC-zz]

@workingjubilee any updates?

[workingjubilee]

I can't believe that worked.

[crlf0710]

Triage: Seems ready for review.

[Mark-Simulacrum]

r? @m-ou-se who has some relevant recent context, I think

[m-ou-se]

You'll have to do the same { let res = ...; res } pattern that the macro is already expanding to. Without that, this change would break the following:

async fn f(_: String) {}

fn g() -> impl Send {
    async move {
        let x = 123;
        f(format!("{}", x)).await;
    }
}

the trait `Sync` is not implemented for `dyn std::fmt::Display`

future is not `Send` as this value is used across an await

         $crate::fmt::Display::to_string(&$arg);
                                         ----- has type `&dyn std::fmt::Display` which is not `Send`

See #64477 (comment).

---

You can make it work for all expressions instead of only identifiers by making a separate case to handle ident = expr:

macro_rules! format {
    ("{}", $name:ident = $arg:expr $(,)?) => {{
        let res = $crate::fmt::Display::to_string(&$arg);
        res
    }};
    ("{}", $arg:expr $(,)?) => {{
        let res = $crate::fmt::Display::to_string(&$arg);
        res
    }};
    ...
}

(I also added $(,)? here, to allow for a trailing comma.)

To make sure any future lint like suggested in #45256 (comment) would still warn about the first case, you could add a let _ = format_args!("{}", $name = $arg); there. (Although right now that has no effect. Alternatively, you could just add a note to #45256 to remind the future implementer of that lint to take a look at format!("{}", name = expr) too.)

[m-ou-se]

It'd be good to add some tests. Specifically to:

1. Check format!("{}", thing) still formats the thing correctly. (library/alloc/tests/fmt.rs)
2. Check the await/Send example mentioned above still works. (src/test/ui/async-await/issues/issue-64477-2.rs)
3. Check that format!("{}", thing) still does not compile if thing implements ToString but not Display.
4. Check with a codegen test that format!("{}", thing) does no longer call alloc::fmt::format or use any Display implementation like it did before this change.

[m-ou-se]

I don't think $crate::fmt::Display::to_string(&$arg); is calling the specialized ToString::to_string like you want.

Display::to_string seems to refer to the ToString::to_string implementation for dyn Display. I'm not completely sure, but I think format!("{}", String::new()) would now create a &dyn Display to that String and call the generic Display::to_string on that trait object, instead of calling the specialized <String as ToString>::to_string.

[JohnCSimon]

Ping from triage:
@workingjubilee - can you please address the comments from m-ou-se? Thanks

[workingjubilee]

Aye aye, capitán.

[crlf0710]

@workingjubilee Ping from triage, any updates on this? If there's still questions welcome to talk about it over on official Zulip! Thanks.

[Dylan-DPC-zz]

@workingjubilee any updates?

[workingjubilee]

My sincere apologies, shortly after my last post the very computer I sent it from broke. I now have surplus compute again and I would like to try once more to take a look at this, though if another strike of unluck hits me I will try to reply in a more timely fashion and quit the field, at least on this PR, rather than leaving people waiting any further (and leaving myself open to more lightning bolts from Olympus).

[crlf0710]

@workingjubilee Triage: what's the current status?

[jyn514]

Any reason to make this so restrictive? I think $arg:expr $(,)? would catch a lot more and still allow calling to_string.

[notriddle]

#76531 (comment)

What's worse than that: even the expr version will not be correct.

Right now, format! parses named arguments, like a = 1. If that was parsed as an expression, then it would evaluate to (), and this would not pass the assertion.

let a = format!("{}", x = 1);
assert_eq!(&a, "1");

I think it actually needs to be $arg:ident, and the optimization probably wouldn't be triggered very often as a result. That's annoying.

[Dylan-DPC-zz]

Closing this due to inactivity.