don't drop arguments' temporaries in dbg!

[dianne]

Fixes #153850

Credit to @theemathas for help with macro engineering ^^

r? libs

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[dianne]

I'm not totally sure where this should go. Conceptually, it makes the most sense for it to be a std test, but I can't find where dbg! is actually tested. It also only needs to pass borrowck, not be built and executed, so we could maybe save CI time by making it a //@ check-pass ui test or such.

[Mark-Simulacrum]

I think this is OK. It probably doesn't need to be an integration test though? i.e., you could add it to macros/test.rs or so?

[dianne]

Only the last couple of these actually regressed, but I figure we may as well be resilient to changes in dbg!.

Also, the dereferences of the temporary aren't necessary to reproduce this and weren't present in the reproducer found by crater, but I feel like being explicit makes the intent of the test clearer.

[theemathas]

Why do we need a double reference here? Is it to support some crazy code doing impl Debug for &Thing ?

[dianne]

I'm not sure! The double reference just coerces away as part of the cast, right? But it's been that way since #142594, so I figured I'd leave it be.

[dianne]

.. though on second thought, just removing the & would be a very small PR on its own, so maybe it could be bundled in here, especially if the decision is to backport a revert of #149869 instead of this.

[theemathas]

Huh. This code has compiled ever since the stabilization of dbg!() in 1.32.0.

use std::fmt::{self, Debug, Formatter};
struct Thing;
impl Debug for &Thing {
    fn fmt(&self, _: &mut Formatter) -> fmt::Result {
        Ok(())
    }
}
fn main() {
    dbg!(Thing);
}

Removing the double reference would break this code.

[dianne]

That feels probably accidental ^^ but it definitely shouldn't be changed as part of this, then.

[mejrs]

The dbg! macro has always done the equivalent of println!("{:?}", &x). The double reference is necessary to keep those semantics.

You can do something like

#![feature(impl_trait_in_bindings)]

fn main() {
    let x = 4_u8;
    let y: impl std::fmt::Debug = &x;
}

if you really want to get rid of the double reference/cast, though.

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[Mark-Simulacrum]

Generally this looks fine to me. I'm also not sure where (or whether they exist) there's more tests, but I think as long as you confirmed the one you added fails CI if the changes are reverted it feels OK to me.

r=me, I think it would probably make sense to revert #149869 as the beta backport but I'll leave that up to T-libs backport decision making.

View changes since this review

[Mark-Simulacrum]

I think this is OK. It probably doesn't need to be an integration test though? i.e., you could add it to macros/test.rs or so?

[Mark-Simulacrum]

This feels like an unfortunate regression (fine for this PR but maybe file an issue for it)? It seems highly likely that borrowing is better here.