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

[RalfJung]

We could optimize in the most common case (multiple mentions of local variables in a formatting string) without even needing type information when expanding format_args!, since taking a reference to a variable won't create a temporary; it's just named constants and nullary constructors where that can be an issue.

For this we need to know whether the x in &x is a local variable or not, right? That requires name resolution results. Does format_args have access to that?

[dianne]

For this we need to know whether the x in &x is a local variable or not, right? That requires name resolution results. Does format_args have access to that?

I believe so. My understanding is that format_args! is expanded in rust_ast_lowering, where we have access non-type-dependent resolutions (via ResolverAstLowering). I think that should be sufficient to tell when a path refers to a local variable (or a static, which should also be fine to optimize for).

[Amanieu]

We discussed this in the @rust-lang/libs-api meeting. The consensus was that the semantics should ideally be to evaluate each placeholder separately since this presents the most opportunities for forwards-compatibility with arbitrary expressions. However many people were concerned about the performance regression and we would want to re-visit this decision if the optimization turns out to not be possible.

[BurntSushi]

All righty, I'm happy to resolve my concern given libs-api meeting consensus. But should that be blocked on fixing the perf problem here?

[m-ou-se]

For this we need to know whether the x in &x is a local variable or not, right? That requires name resolution results. Does format_args have access to that?

I believe so. My understanding is that format_args! is expanded in rust_ast_lowering, where we have access non-type-dependent resolutions (via ResolverAstLowering). I think that should be sufficient to tell when a path refers to a local variable (or a static, which should also be fine to optimize for).

Oh god, please no. We shouldn't do such magical things in format_args lowering. That will make it much harder to maintain and refactor. I don't want to get stuck again in a place where we can't update/improve format_args!() because nobody knows how it fully works and there is too much fragile magic involved.

[workingjubilee]

@dianne @traviscross

comment that happened via accidentally misreading something was briefly here

This still causes the problem that @m-ou-se cited: The performance reasoning is undermined, therefore code authors who actually care about performance must change their code. The ones who are guaranteed to benefit from the optimization are the ones who don't care. The equivalence is still gone for the people who care the most about it.

@tmandry

Regarding the wording in the RFC, it's useful as a fallback option, but the only reason I would find it persuasive is if we had discussed this case explicitly and decided to go with the current behavior. We could still change our minds with an FCP, but knowing that would be important context.

...Okay, but what is persuasive then? How should authors of RFCs approach the exercise? Are we agreeing on doing something, just making a vague suggestion, or something else?

[RalfJung]

I think it would be a worrying trend for Rust to add odd non-compositional special cases to the observable runtime semantics (essentially undermining correctness reasoning) in the name of performance. At the very least that should come with overwhelming evidence that the performance benefit is worth such a nontrivial cost.

[workingjubilee]

If people can't rely on the behavior of whatever RFCs specify the moment T-lang has a mood about it, then we should just slam in the repr(C) fix and damn the consequences.

[the8472]

I think it would be a worrying trend for Rust to add odd non-compositional special cases to the observable runtime semantics (essentially undermining correctness reasoning) in the name of performance. At the very least that should come with overwhelming evidence that the performance benefit is worth such a nontrivial cost.

This is why I argued earlier that this is, as far as users are concerned, a macro invocation and macros usually are a library things where the implementation can swizzle your code however it likes. So if it is not specified what the macro does... or the RFC even explicitly does mention deduplication then users should not apply "correctness reasoning" based on unrelated concepts such as function argument evaluation. That's incorrect correctness reasoning.

If this conflicts with desired future extensions to the format_args syntax (supporting full expression substitution was brought up in the libs-meeting) then we could consider syntactically distinguishing them from things where we do ignore the potential for side-effects.

[RalfJung]

I don't see how being a macro exempts anything we offer in the default Rust distribution from the expectation of having consistent, compositional semantics without unexpected traps. The technical vehicle (macro or keyword or whatever) does not matter for why such traps are problematic.

[the8472]

Well, I tried to address the compositional part by suggesting we give such future extensions a different syntax, then nobody should expect those being related things that ought to compose, they're just different kinds of substitutions.
And consistency (presumably with function argument evaluation) imo just does not apply to macros in general because they're not functions. Sometimes they're used in a function-like manner, but doing things in the middle of strings is hardly that.

I do think future extensions that we expect to do have non-negligible effects do need careful design. I'm skeptical about the corner-cases we're looking at right now.

Tangentially, this isn't the first time where things would be a lot easier if we could require or check purity.

[RalfJung]

Well, I tried to address the compositional part by suggesting we give such future extensions a different syntax,

The semantics we have today break compositionality. When I write a const twice I expect it to be instantiated twice.

It seems people derive this as expected behavior from the RFC. Taking a look, I don't think I agree. The RFC has no example where a variable is captured twice. Even the part of the RFC which people now interpret to specify the deduplication fails to make this explicit. Nowhere does it say that this changes program behavior compared to other situations where a constant is mentioned multiple times. Nothing in the RFC points at this being a deliberate decision, it looks more like an oversight.

This is not t-lang "having a mood", this is someone showing up with an interpretation of the RFC that is new at least to me.

[the8472]

When I write a const twice I expect it to be instantiated twice.

But macros don't guarantee constants being evaluated, they might just be referred by name, incorporated into some generated functions that are never called or completely dropped or perhaps deduplicated with some horrendous stringification-gymnastics or something.

A trivial adaption from the linked issue... unsurprisingly it prints nothing.

#![feature(decl_macro)]
use std::fmt::{self, Display, Formatter};
use std::cell::Cell;

struct PrintCounter(Cell<usize>);

impl Display for PrintCounter {
    fn fmt(&self, f: &mut Formatter) -> fmt::Result {
        let counter = 1 + self.0.get();
        self.0.set(counter);
        write!(f, "{counter}")
    }
}

macro foo($a:expr, $b:expr) {
    {}
}

const ZERO: PrintCounter = PrintCounter(Cell::new(0));

fn main() {
    foo!(ZERO, ZERO);
}

[RalfJung]

But macros don't guarantee constants being evaluated,

As I already tried to express before, I really don't get this argument. Why does it matter that a macro could do arbitrary nonsense with its tokens? The same is true for every keyword in the language, for the parser itself! That doesn't mean it's reasonable to do that. We hold the native syntax of the language to a higher standard than "we can do with your tokens whatever we please so we will"; I would expect the same for macros that we ship with the Rust toolchain.

[joshtriplett]

The performance reasoning is undermined, therefore code authors who actually care about performance must change their code.

This change improves the consistency of behavior between parts of Rust. Write something twice, it gets evaluated twice. If you write f(MY_CONST, MY_CONST), we don't necessarily deduplicate anything, and f(LoudDrop, LoudDrop) will create and drop two things. The compiler is allowed to do as-if optimizations that the programmer can't observe, and seems likely to do so for the f(MY_CONST, MY_CONST) case; I wouldn't expect people to write let x = MY_CONST; f(x, x); just to ensure that.

println!("{x} {x}") does seem likely to be common, but precisely that common case is what @dianne is proposing to optimize.

[joshtriplett]

@BurntSushi wrote:

All righty, I'm happy to resolve my concern given libs-api meeting consensus. But should that be blocked on fixing the perf problem here?

I think that we shouldn't block on the performance optimization going in, but we should wait for confirmation that it is as straightforward to implement as suggested. It sounds like @dianne has a proposed strategy that sounds potentially straightforward to implement, so it'd be helpful to see a PR. And if it's that straightforward, there's also not much harm in blocking, either.

[BurntSushi]

I think I've personally been swayed by the arguments in favor of not deduplicating captured arguments. 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'm also swayed by the arguments about future language evolution and general consistency with how the rest of the language works.

@rfcbot resolve equivalence

[joshtriplett]

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 definitely wouldn't want to see that. It's a perfectly reasonable thing to do.

[iago-lito]

Unless it only fires in case there would be an observable difference between the two interpretations.

[BurntSushi]

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.

[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