Make write/print macros eagerly drop temporaries

[dtolnay]

This PR fixes the 2 regressions in #96434 (println and eprintln) and changes all the other similar macros (write, writeln, print, eprint) to match the old pre-#94868 behavior of println and eprintln.

argument position	before #94868	after #94868	after this PR
write!($tmp, "…", …)	😡	😡	😺
write!(…, "…", $tmp)	😡	😡	😺
writeln!($tmp, "…", …)	😡	😡	😺
writeln!(…, "…", $tmp)	😡	😡	😺
print!("…", $tmp)	😡	😡	😺
println!("…", $tmp)	😺	😡	😺
eprint!("…", $tmp)	😡	😡	😺
eprintln!("…", $tmp)	😺	😡	😺
panic!("…", $tmp)	😺	😺	😺

Example of code that is affected by this change:

use std::sync::Mutex;

fn main() {
    let mutex = Mutex::new(0);
    print!("{}", mutex.lock().unwrap()) /* no semicolon */
}

You can see several real-world examples like this in the Crater links at the top of #96434. This code failed to compile prior to this PR as follows, but works after this PR.

error[E0597]: `mutex` does not live long enough
 --> src/main.rs:5:18
  |
5 |     print!("{}", mutex.lock().unwrap()) /* no semicolon */
  |                  ^^^^^^^^^^^^---------
  |                  |
  |                  borrowed value does not live long enough
  |                  a temporary with access to the borrow is created here ...
6 | }
  | -
  | |
  | `mutex` dropped here while still borrowed
  | ... and the borrow might be used here, when that temporary is dropped and runs the `Drop` code for type `MutexGuard`

[rust-highfive]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[Mark-Simulacrum]

I suppose we want T-libs-api FCP here since this affects stable behavior, but r=me on the PR itself.

Also nominating for beta backport.

[dtolnay]

@rust-lang/libs-api:
@rfcbot fcp merge

In the table above, I think everything other than the 1st and 3rd rows are an obvious improvement. These are all equivalent to #64856.

The 1st and 3rd row (how to treat the "writer" argument of write! and writeln!) are not as obvious. In particular I don't see a way to simultaneously support both of the following situations:

use std::fmt;

fn needs_late_drop() {
    struct Mutex;
    struct MutexGuard<'a>(&'a Mutex);

    impl<'a> MutexGuard<'a> {
        fn write_fmt(&self, _args: fmt::Arguments) -> &Self {
            self
        }

        fn lol(&self) {}
    }

    // NEEDS LATE DROP. Needs the MutexGuard temporary to drop at the nearest
    // semicolon _outside_ the write macro.
    let mutex = Mutex;
    write!(MutexGuard(&mutex), "").lol();
}

fn needs_early_drop() -> fmt::Result {
    struct Mutex;
    struct MutexGuard<'a>(&'a Mutex);

    impl<'a> Drop for MutexGuard<'a> {
        fn drop(&mut self) {}
    }

    impl<'a> MutexGuard<'a> {
        fn write_fmt(&self, _args: fmt::Arguments) -> fmt::Result {
            Ok(())
        }
    }

    // NEEDS EARLY DROP. Needs the MutexGuard temporary to drop at a semicolon
    // _inside_ of the write macro.
    let mutex = Mutex;
    write!(MutexGuard(&mutex), "") /* no semicolon */
}

If write! drops the writer inside of the macro, then needs_late_drop fails with:

error[E0716]: temporary value dropped while borrowed
  --> src/main.rs:18:12
   |
18 |     write!(MutexGuard(&mutex), "").lol();
   |     -------^^^^^^^^^^^^^^^^^^-----
   |     |      |
   |     |      creates a temporary which is freed while still in use
   |     temporary value is freed at the end of this statement
   |     borrow later used here
   |
   = note: consider using a `let` binding to create a longer lived value

If write! does not drop the writer inside of the macro, then needs_early_drop fails with:

error[E0597]: `mutex` does not live long enough
  --> src/main.rs:38:23
   |
38 |     write!(MutexGuard(&mutex), "") /* no semicolon */
   |            -----------^^^^^^-
   |            |          |
   |            |          borrowed value does not live long enough
   |            a temporary with access to the borrow is created here ...
39 | }
   | -
   | |
   | `mutex` dropped here while still borrowed
   | ... and the borrow might be used here, when that temporary is dropped and runs the `Drop` code for type `needs_early_drop::MutexGuard`

In either case the fix required by the programmer is the same: just make a let for the writer.

-    write!(MutexGuard(&mutex), "").lol();
+    let writer = MutexGuard(&mutex);
+    write!(writer, "").lol();

-    write!(MutexGuard(&mutex), "") /* no semicolon */
+    let writer = MutexGuard(&mutex);
+    write!(writer, "") /* no semicolon */

My opinion is that in the two functions above, needs_early_drop is the one that should compile successfully and needs_late_drop is the one that should not. My rationale is that needs_early_drop is a situation that can happen with the ordinary io::Write and fmt::Write signatures (fn write_fmt(&mut self, Arguments) -> io::Result<()> and fn write_fmt(&mut self, Arguments) -> fmt::Result) while needs_late_drop is a situation that occurs only in contrived code with a highly unusual custom signature for write_fmt.

[rfcbot]

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

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @yaahc

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!

See this document for info about what commands tagged team members can give me.

[joshtriplett]

@dtolnay Seeking clarification: can this ever change the semantics of existing code by changing when drop happens, or can it only ever make code that didn't compile start compiling.

[dtolnay]

Yes, it's about moving drops from the nearest end of statement outside the macro to inside the macro. So for example:

use std::fmt::{self, Display};
use std::ops::Add;

struct X(usize);

impl Display for X {
    fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
        formatter.write_str("X")
    }
}

impl Add<X> for () {
    type Output = ();
    fn add(self, _: X) -> Self::Output {}
}

impl Drop for X {
    fn drop(&mut self) {
        print!("{}", self.0);
    }
}

fn main() {
    print!("{}", X(0)) + X(1);
}

would go from X10 to X01.

[rfcbot]

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

[dtolnay]

Un-nominating for backport and nominating #96490 instead.

Both PRs can land in nightly without conflicting, but only #96490 needs to be backported I think.

[rust-log-analyzer]

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

  IMAGE: x86_64-gnu-tools
##[endgroup]
From https://github.com/rust-lang/rust
 * branch              master     -> FETCH_HEAD
Searching for toolstate changes between 07e7b4346c48fe6d5c3026207e2d04c9a9f00fb8 and 39f628fd0c5bfedb296f3c086dfcd18d7868b4df
Clippy or rustfmt subtrees were updated
##[group]Run src/ci/scripts/verify-channel.sh
src/ci/scripts/verify-channel.sh
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
---
.......... (60/64)
...       (64/64)


/checkout/src/test/rustdoc-gui/escape-key.goml escape-key... FAILED
[ERROR] (line 6) Error: The following CSS selector "#search h1" was not found: for command `wait-for: "#search h1" // The search element is empty before the first search `
Build completed unsuccessfully in 0:00:41

[dtolnay]

Failure is rustdoc-gui being flaky, unrelated to this PR. #93784

/checkout/src/test/rustdoc-gui/escape-key.goml escape-key... FAILED
[ERROR] (line 6) Error: The following CSS selector "#search h1" was not found: for command `wait-for: "#search h1" // The search element is empty before the first search `

[dtolnay]

@bors r=m-ou-se

[bors]

📌 Commit a610098 has been approved by m-ou-se

[bors]

⌛ Testing commit a610098 with merge c186f7c...

[bors]

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing c186f7c to master...

[rust-timer]

Finished benchmarking commit (c186f7c): comparison url.

Instruction count

• Primary benchmarks: 😿 relevant regressions found
• Secondary benchmarks: no relevant changes found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count1	3	0	0	0	3
mean2	0.4%	N/A	N/A	N/A	0.4%
max	0.6%	N/A	N/A	N/A	0.6%

Max RSS (memory usage)

Results

• Primary benchmarks: 😿 relevant regressions found
• Secondary benchmarks: 😿 relevant regression found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count1	2	1	0	0	2
mean2	3.1%	1.1%	N/A	N/A	3.1%
max	3.4%	1.1%	N/A	N/A	3.4%

Cycles

Results

• Primary benchmarks: 🎉 relevant improvement found
• Secondary benchmarks: 😿 relevant regressions found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count1	0	3	1	0	1
mean2	N/A	4.4%	-2.5%	N/A	-2.5%
max	N/A	4.6%	-2.5%	N/A	-2.5%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Footnotes

1. number of relevant changes ↩ ↩² ↩³

2. the arithmetic mean of the percent change ↩ ↩² ↩³

[Mark-Simulacrum]

This change is relatively minor and limited to a single benchmark; it doesn't look to be obviously noise, but I think is not worth further investigation. Marking as triaged.