Do not deduplicate captured args while expanding format_args!

[ShoyuVanilla]

Resolves #145739

I ran crater with #149291.
While there are still a few seemingly flaky, spurious results, no crates appear to be affected by this breaking change.

The only hit from the lint was
https://github.com/multiversx/mx-sdk-rs/blob/813927c03a7b512a3c6ef9a15690eaf87872cc5c/framework/meta-lib/src/tools/rustc_version_warning.rs#L19-L30,
which performs formatting on consts of type ::semver::Version. These constants contain a nested ::semver::Identifier (Version.pre.identifier) that has a custom destructor. However, this case is not impacted by the change, so no breakage is expected.

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_ast_lowering/src/format.rs

cc @m-ou-se

[rustbot]

r? @spastorino

rustbot has assigned @spastorino.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[ShoyuVanilla]

@rustbot author

[ShoyuVanilla]

@rustbot ready

[theemathas]

Nominating as per #145739 (comment)

[traviscross]

It'd be worth adding a test for the drop behavior.

[traviscross]

Given that this makes more sense for the language, along with the clean crater results and the intuition that it'd be surprising if anything actually leaned on this, I propose:

@rfcbot fcp merge lang

[rust-rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[m-ou-se]

I don't think we should do this. It will make the generated code for println!("{x} {x}"); less efficient, as it will get two separate arguments instead of one.

I don't want to end up in a situation where it would make sense for Clippy to suggest something like:

warning: using the same placeholder multiple times is inefficient as of Rust 1.94.0
 --> src/main.rs:3:5
  |
3 |     println!("{x} {x}");
  |     ^^^^^^^^^^^^^^^^^^^
  |
help: change this to
  |
3 -     println!("{x} {x}");
3 +     println!("{x} {x}", x = x);
  |

Adding , x = x shouldn't make a difference. If adding that makes the resulting code more efficient, I strongly feel like we've done something wrong.

@rust-rfcbot concern equivalence

[nikomatsakis]

@rfcbot fcp reviewed

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[joshtriplett]

@BurntSushi wrote:

I mean that if we can't fix the performance issue. I could see there being a lint that gets triggered, pushing one toward doing your own deduplication. I think this was Mara's concern. I'm saying that even if we do end up with such a lint existing (if we can't fix the performance issue), then I think I can live with that.

Got it. I personally would not want to see such a lint, and would rather see us avoid pushing people to do that. But it sounds like we're aligned on @dianne's current approach, assuming the implementation is as feasible as expected.

[scottmcm]

I like the simplification of not trying to dedup things. I was glad to read the conversation about the concern, though -- that was insightful.

@rfcbot reviewed

[nia-e]

@RalfJung wrote:

Why does it matter that a macro could do arbitrary nonsense with its tokens?

I'm not convinced deduplication is "arbitrary"; an equally reasonable fix is to just explicitly document this behaviour. I don't see why format_args! being part of the compiler itself means it should have special restrictions - the purpose of a macro is to rewrite syntax after all, and just saying "inside of format_args! we deduplicate calls to the Display impl the values displayed" doesn't strike me as particularly unexpected anyway.

Also, I do have a real-world example of the perf issue @m-ou-se pointed out for complex Debug/Display implementations; we actually take advantage of the deduplication to speed up some internal logging-related infrastructure at Zed, which would need to be rewritten in the style of the format_args!("{x}, {x}", x = x) idea brought up previously. That's obviously not a catastrophic change but it would have been a surprising and probably noticeable regression. If this gets approved we can just make that change, but only because I happened to stumble on this thread and know this is the issue, and I can't imagine we're the only project doing something like this ^^

[theemathas]

@nia-e I believe that even if the parameters are deduplicated, the Debug/Display impl will still be called multiple times. So, I don't think it matters whether the Debug/Display impls are complicated or not. Or if it matters at all, a simple Debug/Display impl is more likely to have a noticable performance difference due to deduplication.

Deduplication only means that the buffer (created by format_args!()) that stores the pointers to the thing being formatted will only need to store a single pointer, instead of multiple pointers to the same thing.

May I know how much performance gain did the deduplicating get you?

[nia-e]

@theemathas I'd have to somehow locally profile the cloud backend under load which is perhaps a bit difficult 😅 but a significant amount of time is spent inside that bit of code.

If the actual calls to Display aren't deduped, then I would go the other way and explicitly welcome/make a PR to do so

[theemathas]

@nia-e I don't think deduplicating the Display calls is feasible, at least without a lot of reworking of the formatting machinery. Much of the formatting machinery must work in a setting where allocation isn't available. Storing a string to write it out twice requires allocation.

[nia-e]

@theemathas In that case RIP that idea, but the performance argument applies regardless. Deduplicating the pointers inside format_args!'s buffer still saves us a good bit of memory; there's a line in that display impl with a format_args! call which has 6 different locals 5 times each in different places for pretty-printing some tables. This change would make the 6 pointers expand to 30 in that buffer.

[m-ou-se]

I think we're making a mistake if we accept this.

Today, Clippy's uninlined_format_args lint suggests:

warning: variables can be used directly in the `format!` string
 --> src/main.rs:5:5
  |
5 |     println!("Hello, {0}! How are you, {0}?", name);
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
help: change this to
  |
5 -     println!("Hello, {0}! How are you, {0}?", name);
5 +     println!("Hello, {name}! How are you, {name}?");
  |

Because these two lines are considered equivalent.

The change proposed in this PR makes these two lines no longer equivalent. After this change, the user is either expected to be okay with a subtle performance hit, or to apply this seemingly useless change to keep things the same:

  |
5 -     println!("Hello, {name}! How are you, {name}?");
5 +     println!("Hello, {name}! How are you, {name}?", name=name);
  |

The only suggestion to prevent that in the common case is by making the format_args!() builtin macro way more magical, by changing how it expands depending on whether arguments refer to a local variable.

While this is technically implementable, this is simply not a reasonable/acceptable change. As I mentioned above, this would make the macro beyond reasonably magical and above all hard to maintain. To be extra clear: as the implementor and maintainer of the format_args builtin macro: I will not accept such a change.

Got it. I personally would not want to see such a lint, and would rather see us avoid pushing people to do that. But it sounds like we're aligned on @dianne's current approach, assuming the implementation is as feasible as expected.

@joshtriplett Such an implementation is not feasible/reasonable. We will end up in a situation where users who care about performance will need to add things like arg=arg.

In particular, even if we end up with a Clippy lint that fires for println!("{x} {x}"), I think we can live with that.

I don't think we should accept that. Using the same captured argument multiple times is very normal and even suggested by Clippy today.

It'd be a bad developer experience if in Rust 1.58 we suggest that users change print!("{0} {0}", x) to print!("{x} {x}"), but in 1.94 we then go "ha never mind, change that back or make it print!("{x} {x}", x=x)". How is that acceptable?

I would not feel great sending out a bunch of PRs to projects on GitHub adding arg1=arg1, arg2=arg2 to a bunch of formatting macro invocations.

It is true that for many projects a slight performance hit for formatting doesn't matter much, but using that to argue for changes like this is exactly what makes Rust's string formatting something that's currently being avoided on embedded platforms.

I've been working for years to try to improve the situation. But changes like the one in FCP here are pushing in the opposite direction, making the situation worse.

Consistency for funny weird edge cases like #145739 is great, but please let's not use that as an argument for making the real world experience of and performance of Rust worse.

[joshtriplett]

We discussed this in today's @rust-lang/libs-api meeting. This issue has been going back and forth for a while. At this point, rather than arguing over the feasibility or non-feasibility of an optimization, we felt like we'd like to see the code of the optimization and evaluate that. Knowing that it's possible, seeing whether it looks maintainable, and knowing how much or how little code it takes, would affect our decision on this.

@rfcbot concern lets-see-the-optimization

@dianne, would you be able to show us a PR for the optimization?

[iago-lito]

Deduplication feels wrong to me in the context of #145739, but m-ou-se arguments seem right to me here.

Do I understand correctly that this introduces a tension with potential future extensions like:

println!("{a.b} {a.b}");

because deduplicating these would require deduplicating expressions, not just identifiers?

My guess is that stopping deduplication opens the door to such extensions, and sticking to deduplication instead makes them less realistic.

_{(although I am having a hard time reasoning about my sentence here with all these stacked negations)}

I am okay with a future without format!("{field.access.paths}") and only format!("{strict_formatting_identifiers}"), if this makes sense to Rust. The former is much comfortable but it actually does feel a bit "too magic" to me.

[joshtriplett]

because deduplicating these would require deduplicating expressions, not just identifiers?

Or alternatively, defining increasingly complex rules like "we deduplicate the a but not the a.b", factoring out common prefixes...

We would have to define those because they'd affect the semantics of the program. If we ensure that we only deduplicate as an as-if optimization that isn't semantically visible, the user doesn't have to care about deduplication as a complicating factor, they can just treat it as an optimization.

I am okay with a future without format!("{field.access.paths}") and only format!("{strict_formatting_identifiers}"), if this makes sense to Rust. The former is much comfortable but it actually does feel a bit "too magic" to me.

The ability to format {self.field} is a very common request.

[ShoyuVanilla]

As the author of this PR, I approached this work under the assumption that it was a fairly straightforward and uncontroversial change, based on the discussions in #145739
. The main challenge I anticipated was identifying the affected crates.

I didn't expect the topic to be this debatable, and had I known that upfront, I might have reconsidered taking this on, since I'm not deeply familiar with the formatting. For that reason, I've mostly stayed quiet so far. I don't feel I have particularly strong insights to add beyond what's already been discussed.

Apologies if this PR caused extra discussion or confusion; that wasn't my intention 😢

[RalfJung]

@ShoyuVanilla you don't have to be sorry! You just had the bad luck of stumbling into a superficially simple problem that has some deep subtle issues. This happens occasionally when one builds a complicated system such as a programming language, but we can't predict which issues it affects. It's nor your fault that these issues exist. :) I'm sorry this turned out to be heavier than you thought.

[jackh726]

because deduplicating these would require deduplicating expressions, not just identifiers?

Or alternatively, defining increasingly complex rules like "we deduplicate the a but not the a.b", factoring out common prefixes...

There's also the only-slightly more complex rule of format_args!("{a} {a}") is deduplicated, but format_args!("{a.b} {a.b}") is not, under the logic that you can factor out the former into format_args!("{a} {a}", a=a) but not the latter into format_args!("{a.b} {a.b}", a.b=a.b).

Also, to be clear: the above rule would allow us to move forward with implementing (and even stabilizing) format_args!("{a.b} {a.b}") without even needing to worry about whether or not we want to change the deduplication of format_args!("{a} {a}"). I don't think anybody is arguing that doing any set of deduplication on format_args!("{a.b} {a.b}") makes much sense, but even less so for format_args!("{a.b.c} {a.b.c}").

[m-ou-se]

Do we expect format_args!("{p.name}", p=get_person()) to work? If so, format_args!("{a.b}") could simply desugar to format_args!("{a.b}", a=a).

[dianne]

@dianne, would you be able to show us a PR for the optimization?

I've got a half-baked PR that does the optimization for captured formatted arguments: #152480. It's missing the equivalent optimization for captured width and precision arguments for now, since I think that would be more invasive. Plus, when attempting to preserve the behavior of writing the args out separately, {:w$} only seems about as de-duplicable as {a.b}; both reasonably should be pure, but can execute user code via Deref impls (though that's probably much less likely from coercing a width to &usize than from a field access).

I'm not sure this is the right thing to do, especially given that width/precision messiness. Hopefully the draft PR makes things a bit clearer at least?

[joshtriplett]

As the author of this PR, I approached this work under the assumption that it was a fairly straightforward and uncontroversial change, based on the discussions in #145739 . The main challenge I anticipated was identifying the affected crates.

I didn't expect the topic to be this debatable, and had I known that upfront, I might have reconsidered taking this on, since I'm not deeply familiar with the formatting. For that reason, I've mostly stayed quiet so far. I don't feel I have particularly strong insights to add beyond what's already been discussed.

Apologies if this PR caused extra discussion or confusion; that wasn't my intention 😢

You didn't do anything wrong here. We're having a vigorous discussion about whether we should do this, but none of that was caused by your PR; we appreciate you doing the work on this.