-
Notifications
You must be signed in to change notification settings - Fork 13k
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.
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
Improve closure dummy capture suggestion in macros. #88543
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.