Skip to content

don't drop arguments' temporaries in dbg! #154074

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.

Sign up for GitHub

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

Open
dianne wants to merge 1 commit into
base: main
Choose a base branch
from
+100 −111
Open

don't drop arguments' temporaries in dbg! #154074

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
79 changes: 40 additions & 39 deletions library/std/src/macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,19 +359,16 @@ macro_rules! dbg {
};
}

/// Internal macro that processes a list of expressions and produces a chain of
/// nested `match`es, one for each expression, before finally calling `eprint!`
/// with the collected information and returning all the evaluated expressions
/// in a tuple.
/// Internal macro that processes a list of expressions, binds their results
/// with `match`, calls `eprint!` with the collected information, and returns
/// all the evaluated expressions in a tuple.
///
/// E.g. `dbg_internal!(() () (1, 2))` expands into
/// ```rust, ignore
/// match 1 {
/// tmp_1 => match 2 {
/// tmp_2 => {
/// eprint!("...", &tmp_1, &tmp_2, /* some other arguments */);
/// (tmp_1, tmp_2)
/// }
/// match (1, 2) {
/// (tmp_1, tmp_2) => {
/// eprint!("...", &tmp_1, &tmp_2, /* some other arguments */);
/// (tmp_1, tmp_2)
/// }
/// }
/// ```
Expand All @@ -380,37 +377,41 @@ macro_rules! dbg {
#[doc(hidden)]
#[rustc_macro_transparency = "semiopaque"]
pub macro dbg_internal {
(($($piece:literal),+) ($($processed:expr => $bound:expr),+) ()) => {{
$crate::eprint!(
$crate::concat!($($piece),+),
$(
$crate::stringify!($processed),
// The `&T: Debug` check happens here (not in the format literal desugaring)
// to avoid format literal related messages and suggestions.
&&$bound as &dyn $crate::fmt::Debug
),+,
// The location returned here is that of the macro invocation, so
// it will be the same for all expressions. Thus, label these
// arguments so that they can be reused in every piece of the
// formatting template.
file=$crate::file!(),
line=$crate::line!(),
column=$crate::column!()
);
// Comma separate the variables only when necessary so that this will
// not yield a tuple for a single expression, but rather just parenthesize
// the expression.
($($bound),+)
}},
(($($piece:literal),*) ($($processed:expr => $bound:expr),*) ($val:expr $(,$rest:expr)*)) => {
(($($piece:literal),+) ($($processed:expr => $bound:ident),+) ()) => {
// Use of `match` here is intentional because it affects the lifetimes
// of temporaries - https://stackoverflow.com/a/48732525/1063961
match $val {
tmp => $crate::macros::dbg_internal!(
($($piece,)* "[{file}:{line}:{column}] {} = {:#?}\n")
($($processed => $bound,)* $val => tmp)
($($rest),*)
),
// Always put the arguments in a tuple to avoid an unused parens lint on the pattern.
match ($($processed,)+) {
($($bound,)+) => {
$crate::eprint!(
$crate::concat!($($piece),+),
$(
$crate::stringify!($processed),
// The `&T: Debug` check happens here (not in the format literal desugaring)
// to avoid format literal related messages and suggestions.
&&$bound as &dyn $crate::fmt::Debug
Copy link
Contributor

@theemathas theemathas Mar 19, 2026

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@dianne dianne Mar 19, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@dianne dianne Mar 19, 2026

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@theemathas theemathas Mar 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@dianne dianne Mar 19, 2026

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@mejrs mejrs Mar 20, 2026

Choose a reason for hiding this comment

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

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.

),+,
// The location returned here is that of the macro invocation, so
// it will be the same for all expressions. Thus, label these
// arguments so that they can be reused in every piece of the
// formatting template.
file=$crate::file!(),
line=$crate::line!(),
column=$crate::column!()
);
// Comma separate the variables only when necessary so that this will
// not yield a tuple for a single expression, but rather just parenthesize
// the expression.
($($bound),+)

}
}
},
(($($piece:literal),*) ($($processed:expr => $bound:ident),*) ($val:expr $(,$rest:expr)*)) => {
$crate::macros::dbg_internal!(
($($piece,)* "[{file}:{line}:{column}] {} = {:#?}\n")
($($processed => $bound,)* $val => tmp)
($($rest),*)
)
},
}
13 changes: 13 additions & 0 deletions library/std/tests/dbg-macro.rs
View file
Open in desktop
Copy link
Contributor Author

@dianne dianne Mar 19, 2026

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// ignore-tidy-dbg

/// Test for <https://github.com/rust-lang/rust/issues/153850>:
/// `dbg!` shouldn't drop arguments' temporaries.
#[test]
fn no_dropping_temps() {
fn temp() {}

*dbg!(&temp());
*dbg!(&temp(), 1).0;
*dbg!(0, &temp()).1;
*dbg!(0, &temp(), 2).1;
Comment on lines +9 to +12
Copy link
Contributor Author

@dianne dianne Mar 19, 2026

Choose a reason for hiding this comment

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

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.

}
47 changes: 5 additions & 42 deletions src/tools/clippy/clippy_lints/src/dbg_macro.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use clippy_utils::source::snippet_with_applicability;
use clippy_utils::{is_in_test, sym};
use rustc_data_structures::fx::FxHashSet;
use rustc_errors::Applicability;
use rustc_hir::{Arm, Closure, ClosureKind, CoroutineKind, Expr, ExprKind, LetStmt, LocalSource, Node, Stmt, StmtKind};
use rustc_hir::{Closure, ClosureKind, CoroutineKind, Expr, ExprKind, LetStmt, LocalSource, Node, Stmt, StmtKind};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_session::impl_lint_pass;
use rustc_span::{Span, SyntaxContext};
Expand Down Expand Up @@ -92,16 +92,15 @@ impl LateLintPass<'_> for DbgMacro {
(macro_call.span, String::from("()"))
}
},
ExprKind::Match(first, arms, _) => {
let vals = collect_vals(first, arms);
let suggestion = match *vals.as_slice() {
ExprKind::Match(args, _, _) => {
let suggestion = match args.kind {
// dbg!(1) => 1
[val] => {
ExprKind::Tup([val]) => {
snippet_with_applicability(cx, val.span.source_callsite(), "..", &mut applicability)
.to_string()
},
// dbg!(2, 3) => (2, 3)
[first, .., last] => {
ExprKind::Tup([first, .., last]) => {
let snippet = snippet_with_applicability(
cx,
first.span.source_callsite().to(last.span.source_callsite()),
Expand Down Expand Up @@ -165,39 +164,3 @@ fn is_async_move_desugar<'tcx>(expr: &'tcx Expr<'tcx>) -> Option<&'tcx Expr<'tcx
fn first_dbg_macro_in_expansion(cx: &LateContext<'_>, span: Span) -> Option<MacroCall> {
macro_backtrace(span).find(|mc| cx.tcx.is_diagnostic_item(sym::dbg_macro, mc.def_id))
}

/// Extracts all value expressions from the `match`-tree generated by `dbg!`.
///
/// E.g. from
/// ```rust, ignore
/// match 1 {
/// tmp_1 => match 2 {
/// tmp_2 => {
/// /* printing */
/// (tmp_1, tmp_2)
/// }
/// }
/// }
/// ```
/// this extracts `1` and `2`.
fn collect_vals<'hir>(first: &'hir Expr<'hir>, mut arms: &'hir [Arm<'hir>]) -> Vec<&'hir Expr<'hir>> {
let mut vals = vec![first];
loop {
let [arm] = arms else {
unreachable!("dbg! macro expansion only has single-arm matches")
};

match is_async_move_desugar(arm.body)
.unwrap_or(arm.body)
.peel_drop_temps()
.kind
{
ExprKind::Block(..) => return vals,
ExprKind::Match(val, a, _) => {
vals.push(val);
arms = a;
},
_ => unreachable!("dbg! macro expansion only results in block or match expressions"),
}
}
}
2 changes: 1 addition & 1 deletion src/tools/miri/tests/fail/dangling_pointers/dangling_primitive.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ error: Undefined Behavior: memory access failed: ALLOC has been freed, so this p
--> tests/fail/dangling_pointers/dangling_primitive.rs:LL:CC
|
LL | dbg!(*ptr);
| ^^^^^^^^^^ Undefined Behavior occurred here
| ^^^^ Undefined Behavior occurred here
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
Expand Down
2 changes: 1 addition & 1 deletion src/tools/miri/tests/fail/function_calls/return_pointer_on_unwind.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ error: Undefined Behavior: reading memory at ALLOC[0x0..0x4], but memory is unin
--> tests/fail/function_calls/return_pointer_on_unwind.rs:LL:CC
|
LL | dbg!(x.0);
| ^^^^^^^^^ Undefined Behavior occurred here
| ^^^ Undefined Behavior occurred here
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
Expand Down
40 changes: 20 additions & 20 deletions tests/ui/borrowck/dbg-issue-120327.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,44 +4,44 @@ error[E0382]: use of moved value: `a`
LL | let a = String::new();
| - move occurs because `a` has type `String`, which does not implement the `Copy` trait
LL | dbg!(a);
| ------- value moved here
| - value moved here
LL | return a;
| ^ value used here after move
|
help: consider borrowing instead of transferring ownership
help: consider cloning the value if the performance cost is acceptable
|
LL | dbg!(&a);
| +
LL | dbg!(a.clone());
| ++++++++

error[E0382]: use of moved value: `a`
--> $DIR/dbg-issue-120327.rs:10:12
|
LL | let a = String::new();
| - move occurs because `a` has type `String`, which does not implement the `Copy` trait
LL | dbg!(1, 2, a, 1, 2);
| ------------------- value moved here
| - value moved here
LL | return a;
| ^ value used here after move
|
help: consider borrowing instead of transferring ownership
help: consider cloning the value if the performance cost is acceptable
|
LL | dbg!(1, 2, &a, 1, 2);
| +
LL | dbg!(1, 2, a.clone(), 1, 2);
| ++++++++

error[E0382]: use of moved value: `b`
--> $DIR/dbg-issue-120327.rs:16:12
|
LL | let b: String = "".to_string();
| - move occurs because `b` has type `String`, which does not implement the `Copy` trait
LL | dbg!(a, b);
| ---------- value moved here
| - value moved here
LL | return b;
| ^ value used here after move
|
help: consider borrowing instead of transferring ownership
help: consider cloning the value if the performance cost is acceptable
|
LL | dbg!(a, &b);
| +
LL | dbg!(a, b.clone());
| ++++++++

error[E0382]: use of moved value: `a`
--> $DIR/dbg-issue-120327.rs:22:12
Expand All @@ -50,14 +50,14 @@ LL | fn x(a: String) -> String {
| - move occurs because `a` has type `String`, which does not implement the `Copy` trait
LL | let b: String = "".to_string();
LL | dbg!(a, b);
| ---------- value moved here
| - value moved here
LL | return a;
| ^ value used here after move
|
help: consider borrowing instead of transferring ownership
help: consider cloning the value if the performance cost is acceptable
|
LL | dbg!(&a, b);
| +
LL | dbg!(a.clone(), b);
| ++++++++

error[E0382]: use of moved value: `b`
--> $DIR/dbg-issue-120327.rs:46:12
Expand Down Expand Up @@ -103,14 +103,14 @@ error[E0382]: borrow of moved value: `a`
LL | let a: String = "".to_string();
| - move occurs because `a` has type `String`, which does not implement the `Copy` trait
LL | let _res = get_expr(dbg!(a));
| ------- value moved here
| - value moved here
LL | let _l = a.len();
| ^ value borrowed here after move
|
help: consider borrowing instead of transferring ownership
help: consider cloning the value if the performance cost is acceptable
|
LL | let _res = get_expr(dbg!(&a));
| +
LL | let _res = get_expr(dbg!(a.clone()));
| ++++++++

error: aborting due to 7 previous errors

Expand Down
2 changes: 1 addition & 1 deletion tests/ui/liveness/liveness-upvars.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ pub fn g<T: Default>(mut v: T) {
}

pub fn h<T: Copy + Default + std::fmt::Debug>() {
let mut z = T::default();
let mut z = T::default(); //~ WARN unused variable: `z`
let _ = move |b| {
loop {
if b {
Expand Down
10 changes: 9 additions & 1 deletion tests/ui/liveness/liveness-upvars.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,14 @@ LL | z = T::default();
|
= help: maybe it is overwritten before being read?

warning: unused variable: `z`
--> $DIR/liveness-upvars.rs:101:9
|
LL | let mut z = T::default();
| ^^^^^ help: if this is intentional, prefix it with an underscore: `_z`
|
= note: `#[warn(unused_variables)]` implied by `#[warn(unused)]`

warning: value captured by `state` is never read
--> $DIR/liveness-upvars.rs:131:9
|
Expand Down Expand Up @@ -196,5 +204,5 @@ LL | s = yield ();
|
= help: maybe it is overwritten before being read?

warning: 24 warnings emitted
warning: 25 warnings emitted

16 changes: 10 additions & 6 deletions tests/ui/rfcs/rfc-2361-dbg-macro/dbg-macro-move-semantics.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,21 @@
error[E0382]: use of moved value: `a`
--> $DIR/dbg-macro-move-semantics.rs:9:13
--> $DIR/dbg-macro-move-semantics.rs:9:18
|
LL | let a = NoCopy(0);
| - move occurs because `a` has type `NoCopy`, which does not implement the `Copy` trait
LL | let _ = dbg!(a);
| ------- value moved here
| - value moved here
LL | let _ = dbg!(a);
| ^^^^^^^ value used here after move
| ^ value used here after move
|
help: consider borrowing instead of transferring ownership
note: if `NoCopy` implemented `Clone`, you could clone the value
--> $DIR/dbg-macro-move-semantics.rs:4:1
|
LL | let _ = dbg!(&a);
| +
LL | struct NoCopy(usize);
| ^^^^^^^^^^^^^ consider implementing `Clone` for this type
...
LL | let _ = dbg!(a);
| - you could clone this value

error: aborting due to 1 previous error

Expand Down
Loading