avoid tearing dbg! prints, implemented using super let#157576
Conversation
Two things: Since `dbg!` is changing, this removes parts of the file that assumed it expands a certain way. The removed tests will no longer get the "consider borrowing" messages going forward, but I think that's fine; it's unlikely that someone would have hand-written them and the suggestion could easily be wrong in the general case. If we want a more general help message to suggest borrowing in those cases, I think it should be designed as such from the ground up, not targeting `dbg!`. From what I can tell, this test file was intended to be for the "consider borrowing" help messages on diagnostics for `dbg!`. To make it harder to accidentally mess with those messages, I've added checks for them and some comments to clarify intent.
|
The Clippy subtree was changed cc @rust-lang/clippy The Miri subtree was changed cc @rust-lang/miri |
rustbot
added
S-waiting-on-review
|
honestly I'm a bit ambivalent about this at this point, but I figure since the work's done, I may as well open a PR so we can go ahead with it if we want (provided crater's clean). assuming we'd like to stop |
|
Preparing for crater: @bors try |
This comment has been minimized.
This comment has been minimized.
avoid tearing `dbg!` prints, implemented using `super let`
|
Unsure if we need to also run tests in crater, but let's go with @craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
| pub macro dbg_internal { | ||
| (($($piece:literal),+) ($($processed:expr => $bound:ident),+) ()) => {{ | ||
| $(let $bound);+; | ||
| $(super let _ = ($bound = $processed));+; |
There was a problem hiding this comment.
I think that this syntax is very confusing and should probably be explained a bit better. To me, I see this as the unit return value of the compound expression ($bound = $processed) being given higher scope, not $bound, which doesn't make any sense. I assume that some heavier lifting is being done but there should be some comments explaining why this works, so people will understand if we later change the semantics of super let before stabilisation and it breaks.
| #[unstable(feature = "std_internals", issue = "none")] | ||
| pub macro dbg_internal { | ||
| (($($piece:literal),+) ($($processed:expr => $bound:ident),+) ()) => {{ | ||
| $(let $bound);+; |
There was a problem hiding this comment.
Minor nit, but I personally would prefer calling this just var or ident or really anything other than bound, since the term "bound" makes my mind immediately go toward trait bounds and that's not what this is. It's technically clear by itself, but that would make the macro code a bit easier to read.
| $(let $bound);+; | ||
| $(super let _ = ($bound = $processed));+; | ||
| $crate::eprint!( | ||
| $crate::concat!($($piece),+), |
There was a problem hiding this comment.
Similarly, I think piece should be reworded as maybe fmt_piece or something that clarifies that this is for the formatting string being used, since otherwise it immediately begs "piece of what?"
Again, this is clear from the implementation alone, but it would be nice to have a grasp on what is being done before reading the full implementation.
| /// Internal macro that processes a list of expressions, binds their results, calls `eprint!` with | ||
| /// the collected information, and returns all the evaluated expressions in a tuple. | ||
| /// | ||
| /// E.g. `dbg_internal!(() () (1, 2))` expands into |
There was a problem hiding this comment.
I would clarify this further to the sequence:
dbg_internal!(() () (1, 2))dbg_internal!(("...") (1 => tmp_1) (2))dbg_internal!(("...", "...") (1 => tmp_1, 2 => tmp_2) ()
Followed by the code generated.
I would also note that the _1 and _2 suffixes are not actually used, and that we rely on macro hygiene to make multiple tmp idents distinct. Again, technically something you should be able to figure out from the implementation, but it speeds up reading it a bit more.
| /// let tmp_2; | ||
| /// super let _ = (tmp_1 = 1); | ||
| /// super let _ = (tmp_2 = 2); | ||
| /// eprint!("...", &tmp_1, &tmp_2, /* some other arguments */); |
There was a problem hiding this comment.
I would replace "..." with concat!("...", "...") to clarify that we do genuinely just concatenate all the strings together, so it's clear that the \n on each string is load-bearing.
5 participants
Fixes #136703.
This is a revival of #149869 using the approach from #155915.
super letstatements aren't the cleanest for this in their current state (it required a bit of trickery), but I'll be working on making temporary scoping in macros nicer soon.r? libs