Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions compiler/rustc_middle/src/ty/print/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ use super::*;

macro_rules! p {
(@$lit:literal) => {
write!(scoped_cx!(), $lit)?
scoped_cx!().write_fmt(format_args!($lit))?
};
(@write($($data:expr),+)) => {
write!(scoped_cx!(), $($data),+)?
scoped_cx!().write_fmt(format_args!($($data),+))?
};
(@print($x:expr)) => {
scoped_cx!() = $x.print(scoped_cx!())?
Expand Down
53 changes: 50 additions & 3 deletions library/core/src/macros/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -515,9 +515,39 @@ macro_rules! r#try {
#[macro_export]
#[stable(feature = "rust1", since = "1.0.0")]
#[cfg_attr(not(test), rustc_diagnostic_item = "write_macro")]
#[allow_internal_unstable(autoref)]
macro_rules! write {
// Retrocompat workaround to support two phased borrows:
// when `$dst` is made of chaining `.field` (or `.idx`) accesses to a local,
// it is guaranteed to be a *place* and thus there is no question of
// late-dropping anything inside it.
//
// Whilst this won't cover *all* the possible syntaxes for places (it
// won't handle places with `[…]`-indexing inside them (see
// https://github.com/rust-lang/rust/pull/100202#pullrequestreview-1064499226)
// qualified paths to `static mut`s, nor macro invocations), it ought to
// cover the vast majority of uses in practice, especially regarding
// retro-compatibility.
(
$dst_place:ident $(. $field_or_idx:tt)* ,
$($arg:tt)*
) => ({
let result =
$dst_place $(. $field_or_idx )*
.write_fmt($crate::format_args!($($arg)*))
;
result
});

// default case: early-dropped `format_args!($($args)*)` while keeping
// `$dst` late-dropped.
($dst:expr, $($arg:tt)*) => {
$dst.write_fmt($crate::format_args!($($arg)*))
match $crate::ops::autoref::autoref_mut!($dst) {
mut _dst => {
let result = _dst.write_fmt($crate::format_args!($($arg)*));
result
}
}
};
}

Expand Down Expand Up @@ -549,13 +579,30 @@ macro_rules! write {
#[macro_export]
#[stable(feature = "rust1", since = "1.0.0")]
#[cfg_attr(not(test), rustc_diagnostic_item = "writeln_macro")]
#[allow_internal_unstable(format_args_nl)]
#[allow_internal_unstable(autoref, format_args_nl)]
macro_rules! writeln {
($dst:expr $(,)?) => {
$crate::write!($dst, "\n")
};

(
$dst_place:ident $(. $field_or_idx:tt)* ,
$($arg:tt)*
) => ({
let result =
$dst_place $(. $field_or_idx)*
.write_fmt($crate::format_args_nl!($($arg)*))
;
result
});

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@danielhenrymantilla danielhenrymantilla Oct 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, ...)

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

let result = _dst.write_fmt($crate::format_args_nl!($($arg)*));
result
}
}
};
}

Expand Down
46 changes: 46 additions & 0 deletions library/core/src/ops/autoref.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
#![allow(missing_docs, missing_debug_implementations)]
// Poor man's `k#autoref` operator.
//
// See https://github.com/rust-lang/rust/issues/99684
// and https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Desired.20behavior.20of.20.60write!.60.20is.20unimplementable
// for some more context about this idea.
//
// Right now we polyfill this idea, by reducing support to `&mut`-autoref-only
// (effectively "just" preventing an unnecessary `&mut` level of indirection
// from being applied for a thing already behind a `&mut …`), which happens to
// work for `&mut`-based `.write_fmt()` methods, and some cases of `&`-based
// `.write_fmt()` —the whole duck-typed design / API of `write!` is asking for
// trouble—, but won't work for a `self`-based `.write_fmt()`, as pointed out
// here: https://github.com/rust-lang/rust/pull/100202#pullrequestreview-1064499226
//
// Finally, in order to reduce the chances of name conflicts as much as
// possible, the method name is a bit mangled, and to prevent usage of this
// method in stable-rust, an unstable const generic parameter that needs to be
// turbofished is added to it as well.

/// The unstable const generic parameter achieving the "unstable seal" effect.
#[unstable(feature = "autoref", issue = "none")]
#[derive(Eq, PartialEq)]
pub struct UnstableMethodSeal;

#[unstable(feature = "autoref", issue = "none")]
pub trait AutoRef {
#[unstable(feature = "autoref", issue = "none")]
#[inline(always)]
fn __rustc_unstable_auto_ref_mut_helper<const _SEAL: UnstableMethodSeal>(
&mut self,
) -> &mut Self {
self
}
}

#[unstable(feature = "autoref", issue = "none")]
impl<T: ?Sized> AutoRef for T {}

#[unstable(feature = "autoref", issue = "none")]
#[allow_internal_unstable(autoref)]
#[rustc_macro_transparency = "semitransparent"]
pub macro autoref_mut($x:expr) {{
use $crate::ops::autoref::AutoRef as _;
$x.__rustc_unstable_auto_ref_mut_helper::<{ $crate::ops::autoref::UnstableMethodSeal }>()
}}
3 changes: 3 additions & 0 deletions library/core/src/ops/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,9 @@
#![stable(feature = "rust1", since = "1.0.0")]

mod arith;
#[doc(hidden)]
#[unstable(feature = "autoref", issue = "none")]
pub mod autoref;
mod bit;
mod control_flow;
mod deref;
Expand Down
61 changes: 32 additions & 29 deletions src/test/ui/macros/format-args-temporaries-in-write.rs
Original file line number Diff line number Diff line change
@@ -1,50 +1,53 @@
// check-fail
// check-pass
#![crate_type = "lib"]

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

struct Mutex;

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

@pnkfelix pnkfelix Feb 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fn lock(&self) -> impl '_ + Display {
42
}
}

struct MutexGuard<'a>(&'a Mutex);
struct Stderr();

impl<'a> Drop for MutexGuard<'a> {
fn drop(&mut self) {
// Empty but this is a necessary part of the repro. Otherwise borrow
// checker is fine with 'a dangling at the time that MutexGuard goes out
// of scope.
}
}

struct Out;

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

impl<'a> Display for MutexGuard<'a> {
fn fmt(&self, _formatter: &mut fmt::Formatter) -> fmt::Result {
Ok(())
}
impl Stderr {
/// A "lending" `write_fmt` method. See:
/// https://docs.rs/async-std/1.12.0/async_std/io/prelude/trait.WriteExt.html#method.write_fmt
fn write_fmt(&mut self, _args: fmt::Arguments) -> &() { &() }
}

fn main() {
// FIXME(dtolnay): We actually want both of these to work. I think it's
// sadly unimplementable today though.
fn early_drop_for_format_args_temporaries() {
let mut out = Stderr();

let _write = {
let mutex = Mutex;
write!(Out, "{}", mutex.lock()) /* no semicolon */
//~^ ERROR `mutex` does not live long enough
write!(out, "{}", mutex.lock()) /* no semicolon */
};

let _writeln = {
let mutex = Mutex;
writeln!(Out, "{}", mutex.lock()) /* no semicolon */
//~^ ERROR `mutex` does not live long enough
writeln!(out, "{}", mutex.lock()) /* no semicolon */
};
}

fn late_drop_for_receiver() {
let mutex = Mutex;
drop(write!(&mut Stderr(), "{}", mutex.lock()));
drop(writeln!(&mut Stderr(), "{}", mutex.lock()));
}

fn two_phased_borrows_retrocompat(w: (&mut Stderr, i32)) {
write!(w.0, "{}", w.1);
writeln!(w.0, "{}", w.1);
struct Struct<W> {
w: W,
len: i32
}
let s = (Struct { w: (w.0, ), len: w.1 }, );
write!(s.0.w.0, "{}", s.0.len);
writeln!(s.0.w.0, "{}", s.0.len);
}
43 changes: 0 additions & 43 deletions src/test/ui/macros/format-args-temporaries-in-write.stderr

This file was deleted.