Fix #99684 through autoref in `write!` macro with a two-phased borrows retrocompat workaround #100202

danielhenrymantilla · 2022-08-06T17:34:44Z

Fixes #99684, using the idea suggested over #99684 (comment) by @eddyb, and implemented by @dtolnay to test its viability.

On top of that, it does use a two phased borrows retrocompat workaround (imho, the write! family of macros supporting two-phased borrows to begin with is quite surprising —could we have a FCW against it?).

For those currently really wanting two-phased borrows semantics, it's easy to rewrite the code so that it fits the special rule (use a new binding for the target of write!);

EDIT: as it is implemented now, it should be quite robust and tackle the expected semantics 🙂

danielhenrymantilla · 2022-08-06T17:38:21Z

I think it could be interesting to have a crater run for this approach, seems it seems better than reverting dtolnay's drop-temporaries improvements

eddyb · 2022-08-06T18:05:05Z

It does use a two phased borrows retrocompat "hack", which may not be pretty, but the write! family of macros supporting two-phased borrows to begin with is quite surprising (could we have a FCW against it?).

Is there a typo here? I'm pretty sure my approach doesn't handle two-phased borrows, just the autoref semantics of not needing &mut references to be held in mutable places.

Also, for the record, there are possible variations on both the names of any of the helper definitions, and whether the generic parameter "seal" hack uses a const or a type (bound by a sealed trait), so don't take the initial demo I posted as fixed in stone.

Since some macro shenanigans are involved, cc @petrochenkov.

danielhenrymantilla · 2022-08-06T21:44:22Z

I had phrased it ambiguously, I've edited to clarify:

this PR features your suggestion, which fixes the regression
But, in turn, it introduces a regression with 2-phased-borrows, as dtolnay pointed out (think write!(f, "{}", f.count)), since we are no longer inlining the receiver for the method;
I tackle that aspect, partially¹, by using a syntatical rule/heuristic that detects places, which replaces the match autoref behavior with the initial $dst.write... implementation

I haven't been able to handle .0 / tuple indexing yet, which is the part that bothers me. But I may have a follow-up commit to handle that ↩

dtolnay

This looks promising to me.

Some edge cases where this would still cause breakage— unclear how widespread these would be.

write_fmt that takes self by value. The autoref thing indiscriminately turns everything into a reference.

use std::fmt;

struct Out;

impl Out {
    fn write_fmt(self, _args: fmt::Arguments) {
        unimplemented!()
    }
}

fn main() {
    let out = || Out;
    write!(out(), "...");
}

error[E0507]: cannot move out of `*_dst` which is behind a mutable reference
  --> src/main.rs:13:5
   |
13 |     write!(out(), "...");
   |     ^^^^^^^^^^^^^^^^^^^^ move occurs because `*_dst` has type `Out`, which does not implement the `Copy` trait
   |
   = note: this error originates in the macro `write` (in Nightly builds, run with -Z macro-backtrace for more info)

2φ borrows within a tuple struct (as you already called out).

use std::fmt;

struct Out {
    val: usize,
}

impl Out {
    fn write_fmt(&mut self, _args: fmt::Arguments) {
        unimplemented!()
    }
}

struct Thing(Out);

fn main() {
    let mut thing = Thing(Out { val: 0 });

    write!(thing.0, "{}", {thing.0.val});
}

error[E0503]: cannot use `thing.0.val` because it was mutably borrowed
  --> src/main.rs:18:28
   |
18 |     write!(thing.0, "{}", {thing.0.val});
   |     -----------------------^^^^^^^^^^^--
   |     |                      |
   |     |                      use of borrowed `thing.0`
   |     borrow of `thing.0` occurs here
   |     borrow later used by call

2φ borrows which are not of the form $($dst_place:tt).+.

use std::fmt;

struct Out {
    val: usize,
}

impl Out {
    fn write_fmt(&mut self, _args: fmt::Arguments) {
        unimplemented!()
    }
}

fn main() {
    let mut out = [Out { val: 0 }];

    write!(out[0], "{}", {out[0].val});
}

error[E0503]: cannot use `out[_].val` because it was mutably borrowed
  --> src/main.rs:16:27
   |
16 |     write!(out[0], "{}", {out[0].val});
   |     ----------------------^^^^^^^^^^--
   |     |                     |
   |     |                     use of borrowed `out[_]`
   |     borrow of `out[_]` occurs here
   |     borrow later used by call

rustbot · 2022-08-08T21:03:21Z

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 @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. 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

danielhenrymantilla · 2022-08-08T21:05:22Z

Now that tuple indexing is covered as well, I feel more confident about this PR 🙂.

Regarding commit history, I was thinking of squashing all the intermediary commits together, to end up with something along the lines of:

dtolnay & eddyb autoref idea (maybe with the stability annotations fix squashed onto it)
my value expr heuristic extra rule
tweaking the test case accordingly

Thoughts?

library/core/src/lib.rs

danielhenrymantilla · 2022-08-12T18:20:55Z

(cleaned up the commit history; the original one is available at b3bdbef)

library/core/src/ops/autoref.rs

eddyb · 2022-08-31T04:35:39Z

library/core/src/macros/mod.rs

    ($dst:expr, $($arg:tt)*) => {
-        $dst.write_fmt($crate::format_args_nl!($($arg)*))
+        match $crate::ops::autoref::autoref_mut!($dst) {
+            mut _dst => {


_dst is guaranteed to be &mut _, doesn't that mean the mut here is redundant?
(Also, is that why you had to add a _ prefix? Doesn't really seem needed otherwise AFAICT)

triage:
@danielhenrymantilla ☝️ - can you address this? thanks

Ah, yes, sorry, I had completely forgotten about this comment 😅. So the thing is that sometimes you do need a nested &mut, such as for the [u8] :! Write-but-&'_ mut [u8] : Write case:

let mut buf: &mut [u8] = ...; buf.write_fmt(...) // resolves to `<&mut [u8] as io::Write>::write_fmt(&mut buf : &mut &mut [u8], ...)` // vs match buf.autoref() { // here, the identity function: `match identity::<&mut [u8]>(buf) {` _buf: &mut [u8] => _buf.write_fmt(...), // we need the *nested* `&mut` => `_buf` needs to be `mut` // <&mut [u8] as io::Write>::write_fmt(&mut _buf, ...)

Demo

So the mut is necessary here, but since it's very often unnecessary as well, we need to use either #[allow(unused_mut)] or a _-prefixed binding name (ideally this macro-generated stuff would not lint altogether, although I guess it's not possible for core / the crate where the macro is defined).
I went for _-prefixed as a personal choice since it yields slightly more lightweight code, but I can very well change it to the allow

…illa Fix stability annotations `autoref!` cleanup: improve comments, and move it under `core::ops`

`write!`: tweak the 2φ borrows hack to cover tuple indexing too

…sion tests

rust-log-analyzer · 2022-10-10T20:33:00Z

The job mingw-check failed! Check out the build log: (web) (plain)

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

    Checking hir-expand v0.0.0 (/checkout/src/tools/rust-analyzer/crates/hir-expand)
   Compiling proc-macro-test v0.0.0 (/checkout/src/tools/rust-analyzer/crates/proc-macro-test)
    Checking hir-def v0.0.0 (/checkout/src/tools/rust-analyzer/crates/hir-def)
    Checking proc-macro-srv-cli v0.0.0 (/checkout/src/tools/rust-analyzer/crates/proc-macro-srv-cli)
error[E0502]: cannot borrow `*self.body` as immutable because it is also borrowed as mutable
   --> crates/hir-def/src/body/pretty.rs:135:38
    |
135 |                     w!(self, "{}: ", self.body[*lbl].name);
    |                                      ^^^^^^^^^ immutable borrow occurs here
   ::: /checkout/library/core/src/ops/autoref.rs:45:5
    |
    |
45  |     $x.__rustc_unstable_auto_ref_mut_helper::<{ $crate::ops::autoref::UnstableMethodSeal }>()
    |     ----------------------------------------------------------------------------------------- mutable borrow occurs here
   ::: /checkout/library/core/src/macros/mod.rs:547:35
    |
    |
547 |                 let result = _dst.write_fmt($crate::format_args!($($arg)*));
    |                                   --------- mutable borrow later used by call

error[E0502]: cannot borrow `*self.body` as immutable because it is also borrowed as mutable
   --> crates/hir-def/src/body/pretty.rs:142:38
    |
142 |                     w!(self, "{}: ", self.body[*lbl].name);
    |                                      ^^^^^^^^^ immutable borrow occurs here
   ::: /checkout/library/core/src/ops/autoref.rs:45:5
    |
    |
45  |     $x.__rustc_unstable_auto_ref_mut_helper::<{ $crate::ops::autoref::UnstableMethodSeal }>()
    |     ----------------------------------------------------------------------------------------- mutable borrow occurs here
   ::: /checkout/library/core/src/macros/mod.rs:547:35
    |
    |
547 |                 let result = _dst.write_fmt($crate::format_args!($($arg)*));
    |                                   --------- mutable borrow later used by call

error[E0502]: cannot borrow `*self.body` as immutable because it is also borrowed as mutable
   --> crates/hir-def/src/body/pretty.rs:150:38
    |
150 |                     w!(self, "{}: ", self.body[*lbl].name);
    |                                      ^^^^^^^^^ immutable borrow occurs here
   ::: /checkout/library/core/src/ops/autoref.rs:45:5
    |
    |
45  |     $x.__rustc_unstable_auto_ref_mut_helper::<{ $crate::ops::autoref::UnstableMethodSeal }>()
    |     ----------------------------------------------------------------------------------------- mutable borrow occurs here
   ::: /checkout/library/core/src/macros/mod.rs:547:35
    |
    |
547 |                 let result = _dst.write_fmt($crate::format_args!($($arg)*));
    |                                   --------- mutable borrow later used by call

error[E0502]: cannot borrow `*self.body` as immutable because it is also borrowed as mutable
   --> crates/hir-def/src/body/pretty.rs:409:38
    |
409 |                     w!(self, "{}: ", self.body[*lbl].name);
    |                                      ^^^^^^^^^ immutable borrow occurs here
   ::: /checkout/library/core/src/ops/autoref.rs:45:5
    |
    |
45  |     $x.__rustc_unstable_auto_ref_mut_helper::<{ $crate::ops::autoref::UnstableMethodSeal }>()
    |     ----------------------------------------------------------------------------------------- mutable borrow occurs here
   ::: /checkout/library/core/src/macros/mod.rs:547:35
    |
    |
547 |                 let result = _dst.write_fmt($crate::format_args!($($arg)*));
    |                                   --------- mutable borrow later used by call
For more information about this error, try `rustc --explain E0502`.
For more information about this error, try `rustc --explain E0502`.
error: could not compile `hir-def` due to 4 previous errors
warning: build failed, waiting for other jobs to finish...
error: could not compile `hir-def` due to 4 previous errors

bors · 2022-12-14T01:37:34Z

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

pnkfelix · 2022-12-15T15:48:15Z

r? @pnkfelix

pnkfelix · 2023-02-13T18:22:44Z

I've started doing some review of this PR.

My current slight concern is that there seems like there was a broad set of behaviors presented in the tables on the descriptions of PR #96455 and PR #99689, but this specific PR shows only one existing test changing its behavior, namely:
src/test/ui/macros/format-args-temporaries-in-write.rs. Furthermore, the comment thread on this PR implies there may be corner cases that were once unhandled (but now are handled), and others that remain unhandled, but it is not yet clear to me if those corner cases are encoded via unit tests.

Also, this PR rewrites that existing test rather than adding a new case to it. I'm not sure whether the change to the existing test is a pure generalization, or is meant to make the overall behavior clearer, or is a fundamental change to what cases are covered by the test (which would be bad if the new set of cases is not a superset of the old set of cases).

I'm still digging in, since I haven't fully grokked how to map the tables from PR #96455 and PR #99689 into the actual test code that landed in PR #99689; I'm hoping the fact that the single changed test in this PR (PR #100202) is one of the tests added in PR #99689 is actually a good sign in terms of overall coverage here. So, I will keep digging and establish how to map the tables to the tests, and then work my way up from there.

pnkfelix · 2023-02-13T18:32:17Z

src/test/ui/macros/format-args-temporaries-in-write.rs


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

 struct Mutex;

 impl Mutex {
-    fn lock(&self) -> MutexGuard {
-        MutexGuard(self)
+    /// Dependent item with (potential) drop glue to disable NLL.


Instead of rewriting the test in this manner (which I believe was intended to show a generalization of the desired behavior), I recommend keeping the existing test structure (i.e. have fn lock return MutexGuard (where the <'a> is elided in the return type signature), and then add your variant (that instead returns impl '_ + Display) as a second separate structure in this same single test file. That way, you get the coverage you're looking for, but people who are looking through the history of this issue will be able to more immediately map the original discussions to the MutexGuard test that will remain encoded in the test.

pnkfelix · 2023-02-13T19:00:39Z

I think it would be good to encode the concrete demo demonstrating the change in order that this PR causes to happen.

You can see an explanation of it for print! in dtolnay's comment over here: #96455 (comment)

(I'd recommend adapting that code into a test on its own, and then generalizing it to cover write! and print!, etc.)

But, since I'm not involved on T-libs, I'm not certain whether they were making a deliberate choice to not encode this as a test, so I opened up a zulip topic asking if this was a choice to leave it unencoded as a test, or just considered not important to test directly; see here: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/drop.20timing.20for.20write.2Fprint.20macros.20.2396455/near/327621973

pnkfelix · 2023-03-07T14:50:34Z

Also, the build is failing, as described in the CI comment above

Moving this back to S-waiting-on-author

@rustbot label: +S-waiting-on-author -S-waiting-on-review

danielhenrymantilla · 2023-03-12T12:36:53Z

Hmm, I'm a bit out of bandwidth to go back to this, especially given how the initial motivation of the PR (avoiding a revert which afterwards ended up happening anyways) is no longer there: at that point the "hack" in this PR may no longer be worth it. Thanks for the review @pnkfelix, and I apologize for having made you spend time on it 🙇 (in hindsight I should have closed this earlier, but it just totally fell off my radar)

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 6, 2022

rust-highfive assigned eddyb Aug 6, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 6, 2022

danielhenrymantilla mentioned this pull request Aug 6, 2022

regression: "self" in writeln! drops too early #99684

Open

This comment has been minimized.

Sign in to view

danielhenrymantilla force-pushed the fix-99684-self-in-write-dropped-early branch from a05ef4f to daa955d Compare August 6, 2022 17:41

This comment has been minimized.

Sign in to view

danielhenrymantilla force-pushed the fix-99684-self-in-write-dropped-early branch from daa955d to 6104067 Compare August 6, 2022 17:48

This comment has been minimized.

Sign in to view

danielhenrymantilla force-pushed the fix-99684-self-in-write-dropped-early branch from 6104067 to 1226330 Compare August 6, 2022 22:55

This comment has been minimized.

Sign in to view

dtolnay reviewed Aug 8, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

danielhenrymantilla marked this pull request as ready for review August 8, 2022 21:03

eddyb reviewed Aug 8, 2022

View reviewed changes

library/core/src/lib.rs Outdated Show resolved Hide resolved

danielhenrymantilla force-pushed the fix-99684-self-in-write-dropped-early branch 2 times, most recently from a7a9a3f to 64a147c Compare August 9, 2022 15:16

This comment has been minimized.

Sign in to view

danielhenrymantilla force-pushed the fix-99684-self-in-write-dropped-early branch from 64a147c to b3bdbef Compare August 9, 2022 16:09

danielhenrymantilla changed the title ~~Fix #99684 through autoref in write! macro with a two-phased borrows retrocompat hack~~ Fix #99684 through autoref in write! macro with a two-phased borrows retrocompat workaround Aug 12, 2022

danielhenrymantilla force-pushed the fix-99684-self-in-write-dropped-early branch from b3bdbef to 4fbfec6 Compare August 12, 2022 18:20

danielhenrymantilla force-pushed the fix-99684-self-in-write-dropped-early branch 2 times, most recently from d877e70 to 38a6d3b Compare August 12, 2022 18:54

eddyb reviewed Aug 31, 2022

View reviewed changes

library/core/src/ops/autoref.rs Outdated Show resolved Hide resolved

eddyb reviewed Aug 31, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

dtolnay and others added 3 commits October 10, 2022 19:31

eddyb autoref trick written by dtolnay and tweaked by danielhenrymant…

ae7cda5

…illa Fix stability annotations `autoref!` cleanup: improve comments, and move it under `core::ops`

Add a 2φ-borrows retro-compat workaround to write!

2310efa

`write!`: tweak the 2φ borrows hack to cover tuple indexing too

Fix format-args-temporaries test (fail->pass) & add 2φ-borrows regres…

b84d25d

…sion tests

danielhenrymantilla force-pushed the fix-99684-self-in-write-dropped-early branch from 5cbe233 to b84d25d Compare October 10, 2022 17:37

rustbot assigned pnkfelix and unassigned eddyb Dec 15, 2022

pnkfelix reviewed Feb 13, 2023

View reviewed changes

rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 7, 2023

danielhenrymantilla closed this Mar 12, 2023

dtolnay self-assigned this Mar 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #99684 through autoref in `write!` macro with a two-phased borrows retrocompat workaround #100202

Fix #99684 through autoref in `write!` macro with a two-phased borrows retrocompat workaround #100202

danielhenrymantilla commented Aug 6, 2022 •

edited

Loading

danielhenrymantilla commented Aug 6, 2022 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

eddyb commented Aug 6, 2022 •

edited

Loading

This comment has been minimized.

danielhenrymantilla commented Aug 6, 2022 •

edited

Loading

This comment has been minimized.

dtolnay left a comment

This comment has been minimized.

rustbot commented Aug 8, 2022

danielhenrymantilla commented Aug 8, 2022 •

edited

Loading

This comment has been minimized.

danielhenrymantilla commented Aug 12, 2022 •

edited

Loading

eddyb Aug 31, 2022

JohnCSimon Oct 8, 2022

danielhenrymantilla Oct 9, 2022 •

edited

Loading

This comment has been minimized.

rust-log-analyzer commented Oct 10, 2022

bors commented Dec 14, 2022

pnkfelix commented Dec 15, 2022

pnkfelix commented Feb 13, 2023 •

edited

Loading

pnkfelix Feb 13, 2023 •

edited

Loading

pnkfelix commented Feb 13, 2023

pnkfelix commented Mar 7, 2023

danielhenrymantilla commented Mar 12, 2023

Fix #99684 through autoref in write! macro with a two-phased borrows retrocompat workaround #100202

Fix #99684 through autoref in write! macro with a two-phased borrows retrocompat workaround #100202

Conversation

danielhenrymantilla commented Aug 6, 2022 • edited Loading

danielhenrymantilla commented Aug 6, 2022 • edited Loading

This comment has been minimized.

This comment has been minimized.

eddyb commented Aug 6, 2022 • edited Loading

This comment has been minimized.

danielhenrymantilla commented Aug 6, 2022 • edited Loading

Footnotes

This comment has been minimized.

dtolnay left a comment

Choose a reason for hiding this comment

This comment has been minimized.

rustbot commented Aug 8, 2022

danielhenrymantilla commented Aug 8, 2022 • edited Loading

This comment has been minimized.

danielhenrymantilla commented Aug 12, 2022 • edited Loading

eddyb Aug 31, 2022

Choose a reason for hiding this comment

JohnCSimon Oct 8, 2022

Choose a reason for hiding this comment

danielhenrymantilla Oct 9, 2022 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

rust-log-analyzer commented Oct 10, 2022

bors commented Dec 14, 2022

pnkfelix commented Dec 15, 2022

pnkfelix commented Feb 13, 2023 • edited Loading

pnkfelix Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

pnkfelix commented Feb 13, 2023

pnkfelix commented Mar 7, 2023

danielhenrymantilla commented Mar 12, 2023

Fix #99684 through autoref in `write!` macro with a two-phased borrows retrocompat workaround #100202

Fix #99684 through autoref in `write!` macro with a two-phased borrows retrocompat workaround #100202

danielhenrymantilla commented Aug 6, 2022 •

edited

Loading

danielhenrymantilla commented Aug 6, 2022 •

edited

Loading

eddyb commented Aug 6, 2022 •

edited

Loading

danielhenrymantilla commented Aug 6, 2022 •

edited

Loading

danielhenrymantilla commented Aug 8, 2022 •

edited

Loading

danielhenrymantilla commented Aug 12, 2022 •

edited

Loading

danielhenrymantilla Oct 9, 2022 •

edited

Loading

pnkfelix commented Feb 13, 2023 •

edited

Loading

pnkfelix Feb 13, 2023 •

edited

Loading