Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
35 changes: 26 additions & 9 deletions compiler/rustc_typeck/src/check/upvar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ use rustc_middle::ty::{
};
use rustc_session::lint;
use rustc_span::sym;
use rustc_span::{BytePos, MultiSpan, Pos, Span, Symbol, DUMMY_SP};
use rustc_span::{BytePos, MultiSpan, Pos, Span, Symbol};
use rustc_trait_selection::infer::InferCtxtExt;

use rustc_data_structures::stable_map::FxHashMap;
Expand Down Expand Up @@ -680,15 +680,32 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
migrated_variables_concat
);

// If the body was entirely expanded from a macro
// invocation, i.e. the body is not contained inside the
// closure span, then we walk up the expansion until we
// find the span before the expansion.
let closure_body_span = self.tcx.hir().span(body_id.hir_id)
.find_ancestor_inside(closure_span)
.unwrap_or(DUMMY_SP);
let mut closure_body_span = {
// If the body was entirely expanded from a macro
// invocation, i.e. the body is not contained inside the
// closure span, then we walk up the expansion until we
// find the span before the expansion.
let s = self.tcx.hir().span(body_id.hir_id);
s.find_ancestor_inside(closure_span).unwrap_or(s)
};

if let Ok(mut s) = self.tcx.sess.source_map().span_to_snippet(closure_body_span) {
if s.starts_with('$') {
// Looks like a macro fragment. Try to find the real block.
Copy link
Contributor

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

Copy link
Member Author

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 from foo!()" (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.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I see.

if let Some(hir::Node::Expr(&hir::Expr {
kind: hir::ExprKind::Block(block, ..), ..
})) = self.tcx.hir().find(body_id.hir_id) {
// If the body is a block (with `{..}`), we use the span of that block.
// E.g. with a `|| $body` expanded from a `m!({ .. })`, we use `{ .. }`, and not `$body`.
// Since we know it's a block, we know we can insert the `let _ = ..` without
// breaking the macro syntax.
if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(block.span) {
closure_body_span = block.span;
s = snippet;
}
}
}

if let Ok(s) = self.tcx.sess.source_map().span_to_snippet(closure_body_span) {
let mut lines = s.lines();
let line1 = lines.next().unwrap_or_default();

Expand Down
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
Copy link
Contributor

Choose a reason for hiding this comment

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

That's... an unfortunate span.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, some weird things are going on when mixing :block and :expr fragments. And closures just use their first and last token to create their full span, which isn't always great. Something to improve another time. ^^

Copy link
Contributor

Choose a reason for hiding this comment

The 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!({
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

It got a \n appended to it. I'm using the span at the end of this line to add a "\n let _ = .." suggestion. I guess the \n is counted as part of this line.

LL + let _ = &a;
|

warning: 1 warning emitted