-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Improve closure dummy capture suggestion in macros. #88543
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
// run-rustfix | ||
// edition:2018 | ||
// check-pass | ||
#![warn(rust_2021_compatibility)] | ||
|
||
macro_rules! m { | ||
(@ $body:expr) => {{ | ||
let f = || $body; | ||
//~^ WARNING: drop order | ||
f(); | ||
}}; | ||
($body:block) => {{ | ||
m!(@ $body); | ||
}}; | ||
} | ||
|
||
fn main() { | ||
let a = (1.to_string(), 2.to_string()); | ||
m!({ | ||
let _ = &a; | ||
//~^ HELP: add a dummy | ||
let x = a.0; | ||
println!("{}", x); | ||
}); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
// run-rustfix | ||
// edition:2018 | ||
// check-pass | ||
#![warn(rust_2021_compatibility)] | ||
|
||
macro_rules! m { | ||
(@ $body:expr) => {{ | ||
let f = || $body; | ||
//~^ WARNING: drop order | ||
f(); | ||
}}; | ||
($body:block) => {{ | ||
m!(@ $body); | ||
}}; | ||
} | ||
|
||
fn main() { | ||
let a = (1.to_string(), 2.to_string()); | ||
m!({ | ||
//~^ HELP: add a dummy | ||
let x = a.0; | ||
println!("{}", x); | ||
}); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
warning: changes to closure capture in Rust 2021 will affect drop order | ||
--> $DIR/closure-body-macro-fragment.rs:8:17 | ||
| | ||
LL | let f = || $body; | ||
| _________________^ | ||
LL | | | ||
LL | | f(); | ||
LL | | }}; | ||
| | - in Rust 2018, `a` is dropped here, but in Rust 2021, only `a.0` will be dropped here as part of the closure | ||
LL | | ($body:block) => {{ | ||
LL | | m!(@ $body); | ||
| |__________________^ | ||
Comment on lines
+2
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's... an unfortunate span. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, some weird things are going on when mixing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh aboslutely, mixed spans involving macros have long standing issues :) |
||
... | ||
LL | / m!({ | ||
LL | | | ||
LL | | let x = a.0; | ||
| | --- in Rust 2018, this closure captures all of `a`, but in Rust 2021, it will only capture `a.0` | ||
LL | | println!("{}", x); | ||
LL | | }); | ||
| |_______- in this macro invocation | ||
| | ||
note: the lint level is defined here | ||
--> $DIR/closure-body-macro-fragment.rs:4:9 | ||
| | ||
LL | #![warn(rust_2021_compatibility)] | ||
| ^^^^^^^^^^^^^^^^^^^^^^^ | ||
= note: `#[warn(rust_2021_incompatible_closure_captures)]` implied by `#[warn(rust_2021_compatibility)]` | ||
= note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2021/disjoint-capture-in-closures.html> | ||
= note: this warning originates in the macro `m` (in Nightly builds, run with -Z macro-backtrace for more info) | ||
help: add a dummy let to cause `a` to be fully captured | ||
| | ||
LL ~ m!({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm intrigued at why this line is shown to have a change. The correct fix might be changing the suggestion printing machinery to verify that before and after are actually different. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It got a |
||
LL + let _ = &a; | ||
| | ||
|
||
warning: 1 warning emitted | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can/should we walk up the backtrace in the expansion instead of this? I guess this works, I'm thinking to think if there is some kind of "catch".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We track things like "
1
is expanded fromfoo!()
" (which was and is already used here), but we don't track "1
was substituted from$a
", which is sort-of the opposite, and the problem here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I see.