regression: "self" in writeln! drops too early

@dtolnay

In https://crater-reports.s3.amazonaws.com/beta-1.63-2/beta-2022-07-16/reg/cabot-0.7.1/log.txt, it looks like the stderr() temporary isn't living long enough with the changes after #96455. However, I've not actually bisected or investigated thoroughly to confirm this -- it seems likely that this is the expected breakage noted in the FCP comment here (#96455 (comment)), though.

cc @dtolnay

Yeah this would be #96455; it's the "needs_late_drop" due to "custom signature for write_fmt" situation described in #96455 (comment).

https://docs.rs/async-std/latest/async_std/io/prelude/trait.WriteExt.html#method.write_fmt

fn write_fmt<'a>(&'a mut self, fmt: Arguments<'_>) -> WriteFmtFuture<'a, Self>
where
    Self: Unpin;

IMO this signature obviously doesn't fit the "contrived" characterization I made about custom write_fmt signatures in that comment, so I think it would be reasonable to revert to late drop for the writer argument of write! and writeln! to support this, while keeping early drop for only the format arguments (to still avoid all the cases from #96434).

@rust-lang/libs-api

Sadly I think that combination might be impossible to implement (from a brief attempt) and we'll end up forced to drop the format args of write! and writeln! late as well.

Some nonworking implementations:

// Causes $dst to be moved, so you can only write to it once.
// The correct behavior requires an autoref on $dst.
macro_rules! write {
    ($dst:expr, $($arg:tt)*) => {
        match $dst {
            mut dst => {
                let result = dst.write_fmt($crate::format_args!($($arg)*));
                result
            }
        }
    };
}

// Does not work if $dst is declared `f: &mut W` where the reference is mutable
// but the binding is not.
macro_rules! write {
    ($dst:expr, $($arg:tt)*) => {
        match &mut $dst {
            dst => {
                let result = dst.write_fmt($crate::format_args!($($arg)*));
                result
            }
        }
    };
}

// Does not work if $arg references $dst such as `write!(w, "{}", w.foo)`, nor (less
// importantly) if $dst contains a return or break or continue or await.
macro_rules! write {
    ($dst:expr, $($arg:tt)*) => {
        match |args: $crate::fmt::Arguments<'_>| $dst.write_fmt(args) {
            mut write_fmt => {
                let result = write_fmt($crate::format_args!($($arg)*));
                result
            }
        }
    };
}

I put up a revert for now in #99689.

Hmm, |args: $crate::fmt::Arguments<'_>| $dst.write_fmt(args) obviously doesn't work because $dst can be an arbitrarily expression, but I believe that the intended meaning is equivalent to $dst.write_fmt, if we allowed using that syntax to automatically create a closure for the method call, after evaluating $dst (tho that's still not great because one could have a field with that name anyway, which could take precedence over this hypothetical feature).

If autoref is all that's needed to make $dst drop later than the format args, it can be done:

#![feature(adt_const_params)]
#![allow(incomplete_features)]

// Everything inside this module is perma-unstable.
pub mod autoref {
    // See below how this is used as a const-generic parameter.
    #[derive(Eq, PartialEq)]
    pub struct UnstableMethodSeal;

    pub trait AutoRef {
        // Not the best name but trying to avoid conflicts as much as possible.
        #[inline(always)]
        fn __rustc_unstable_auto_ref_mut_helper<
            // This is the clever bit: no stable method could have this parameter:
            const _SEAL: UnstableMethodSeal,
        >(&mut self) -> &mut Self { self }
    }
    impl<T: ?Sized> AutoRef for T {}

    // Appropriate allow_internal_unstable attributes go here:
    #[macro_export]
    macro_rules! autoref_mut {
        ($x:expr) => {{
            use $crate::autoref::AutoRef as _;
            $x.__rustc_unstable_auto_ref_mut_helper::<{$crate::autoref::UnstableMethodSeal}>()
        }}
    }
}

// Tests:
fn assert_ref_mut<T: ?Sized>(_: &mut T) {}

pub fn by_value<T>(mut x: T, f: impl FnOnce() -> T) {
    assert_ref_mut(autoref_mut!(x));
    assert_ref_mut(autoref_mut!(f()));
}
pub fn by_ref<T: ?Sized>(r: &mut T, f: impl FnOnce(&()) -> &mut T) {
    assert_ref_mut(autoref_mut!(r));
    assert_ref_mut(autoref_mut!(f(&())));
}

(playground link)

A type parameter could be used instead of a const generic, but then you need a sealed trait to bound it, and frankly they're equivalent and we could pick based on compilation performance (if there's even a noticeable difference between them).

For completeness, I believe the autoref! macro above would allow:

macro_rules! write {
    ($dst:expr, $($arg:tt)*) => {
        match autoref_mut!($dst) {
            dst => {
                let result = dst.write_fmt($crate::format_args!($($arg)*));
                result
            }
        }
    };
}

@eddyb

@eddyb I tried coding up your suggestion (ddaa6e7b498e81ca93a667e7ebdbca677d048a50) but it failed x.py install with code like this:

            write!(
                writer,
                "{}{} ",
                writer.bold().paint(meta.target()),
                writer.dimmed().paint(":")
            )?;

error[E0502]: cannot borrow `writer` as immutable because it is also borrowed as mutable
   --> github.com-1ecc6299db9ec823/tracing-subscriber-0.3.3/src/fmt/format/mod.rs:925:17
    |
925 |                 writer.bold().paint(meta.target()),
    |                 ^^^^^^^^^^^^^ immutable borrow occurs here
    |
   ::: library/core/src/lib.rs:268:13
    |
268 |             $x.__rustc_unstable_auto_ref_mut_helper::<{ $crate::autoref::UnstableMethodSeal }>()
    |             ------------------------------------------------------------------------------------ mutable borrow occurs here
    |
   ::: library/core/src/macros/mod.rs:503:34
    |
503 |                 let result = dst.write_fmt($crate::format_args!($($arg)*));
    |                                  --------- mutable borrow later used by call

In the T-lang zulip I floated the idea of receiver.fn methodname for partial function application of a receiver, as illustrated by this:

let mut myvec = Vec::new();
iterator.for_each(myvec.fn push);  // for_each(|element| myvec.push(element)), but better

Then:

match $dst.fn write_fmt {
    write_fmt => {
        let result = write_fmt(format_args!($($arg)*));
        result
    }
}

NLL would need to know that $dst is not mutably borrowed until write_fmt is called, so we are free to do immutable access in between $dst.fn write_fmt and the call.

If we ever end up doing qualified paths in method syntax (thing.std::io::Write::write(&buffer)) then something like $dst.<_>::write_fmt may mesh better than $dst.fn write_fmt. That unambiguously refers to a method write_fmt.

You mentioned "just make $dst.write_fmt work", but I suspect that isn't going to fly because we already intentionally distinguish naming fields from naming methods in the syntax, e.g. to "call" a field you're required to write (thing.field)() as opposed to just thing.field(). It would be out of character for thing.field to be "whichever one exists".

@dtolnay

@dtolnay If the language had `let autoref = ..., would that help? (We've talked about that in @rust-lang/lang.)

Meanwhile, applying the revert seems fine as a fix for now.

Yes Josh, I believe that could be defined in such a way as to work here. It'd be:

#[macro_export]
macro_rules! write {
    ($dst:expr, $($arg:tt)*) => {
        match $dst {
            autoref dst => {
                let result = dst.write_fmt($crate::format_args!($($arg)*));
                result
            }
        }
    };
}

but it potentially gets weirder with borrowchecking than hypothetical receiver.fn methodname does, in particular to support:

            write!(
                writer,
                "{}{} ",
                writer.bold().paint(meta.target()),
                writer.dimmed().paint(":")
            )?;

$dst can't turn into an exclusive reference "too early".

Yeah, I almost suggested something like k#autoref binding modifiers, but it won't do you much more good than .autoref_mut() (where all of the complexity goes into ensuring that method resolution actually finds the right autoref helper, and not some user-controlled impostor)

But that worst-case example is an example of "two-phase borrows" (aka 2Φ), so really you'd want:

#[macro_export]
macro_rules! write {
    ($dst:expr, $($arg:tt)*) => {
        match $dst {
            k#autoref2Φ mut dst => {
                let result = dst.write_fmt($crate::format_args!($($arg)*));
                result
            }
        }
    };
}

cc @rust-lang/wg-nll (Personally I prefer if a low-level perma-unstable tool like this was explicit about the powers it tapped into, but maybe it's not that huge of a deal?)

Rollup merge of rust-lang#99689 - dtolnay:write, r=Mark-Simulacrum

Note that there was some more discussion about this over Zulip.

My personal short-term solution, here, barring two-phased borrows, would be to have a helper extension method to take care of the necessary autoref, thence yielding the following key macro rule:

// key macro rule
($dst:expr, $($arg:tt)*) => ({
    use $crate::ByRef as _;
    match $dst.__by_ref() {
        dst => {
            let result = dst.write_fmt($crate::format_args!($($arg)*));
            result
        }
    }
});

See the post in question for more info.

Retro-compatiblity with two-phased borrow

Seems to be the remaining thorny point:

how much breakage stems from that?
- could we have a fcw about it? It really looks like something that shouldn't have been there in the first place;
in the aforementioned post, some hacky workarounds could allow not breaking the current usages which rely on it, I guess, although it's hard to assess without actual evidence of what kind of code breaks when we don't have it.

@danielhenrymantilla

@danielhenrymantilla Isn't that just like my earlier suggestion in #99684 (comment), except without the protection from users defining e.g. inherent __by_ref methods that would end up with higher priority than the one the macro intends to call?

@eddyb

@eddyb yeah, totally¹, sorry for the redundant suggestion 🙇, I somehow scrolled past your post.

so, would the suggested workaround tackle the remainging two-phased borrows failures?

EDIT: I've decided to go ahead and test this idea using Fix #99684 through autoref in write! macro with a two-phased borrows retrocompat workaround #100202
- could we have a crater run for it?

I had left the part where we make by_ref more robust to clashes for a later stage, since we know it's doable (and with opaque/ def_site hygiene, it would even be trivial) ↩

Untagging as a regression since we merged a revert (#99689) in #100171. Not closing since it sounds like we're still investigating some alternative fixes for future releases.

Navigation Menu

Repository navigation

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

Retro-compatiblity with two-phased borrow

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Activity

dtolnay commented on Jul 24, 2022

dtolnay commented on Jul 24, 2022

dtolnay commented on Jul 24, 2022

dtolnay commented on Jul 24, 2022

eddyb commented on Jul 27, 2022

eddyb commented on Jul 27, 2022

dtolnay commented on Jul 27, 2022

joshtriplett commented on Jul 27, 2022

joshtriplett commented on Jul 27, 2022

dtolnay commented on Jul 27, 2022

eddyb commented on Jul 28, 2022

danielhenrymantilla commented on Aug 5, 2022

Retro-compatiblity with two-phased borrow

eddyb commented on Aug 6, 2022

danielhenrymantilla commented on Aug 6, 2022

Footnotes

Mark-Simulacrum commented on Aug 7, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions