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

Fix closure migration suggestion when the body is a macro. #87956

Merged
merged 4 commits into from
Aug 13, 2021
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
22 changes: 16 additions & 6 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::{MultiSpan, Span, Symbol};
use rustc_span::{MultiSpan, Span, Symbol, DUMMY_SP};
use rustc_trait_selection::infer::InferCtxtExt;

use rustc_data_structures::stable_map::FxHashMap;
Expand Down Expand Up @@ -644,8 +644,18 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}
}
diagnostics_builder.note("for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2021/disjoint-capture-in-closures.html>");
let closure_body_span = self.tcx.hir().span(body_id.hir_id);
let (sugg, app) =

let mut closure_body_span = self.tcx.hir().span(body_id.hir_id);

// 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.
while !closure_body_span.is_dummy() && !closure_span.contains(closure_body_span) {
Copy link
Member

Choose a reason for hiding this comment

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

If we end with a dummy span, then the overall diagnostic will end up worse than it currently is. Can you restore closure_body_span to its original value if the loop ends up setting it to DUMMY_SP?

Copy link
Member Author

Choose a reason for hiding this comment

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

I kept the dummy span for the suggestion part. Otherwise .span_to_snippet is going to return the wrong snippet and the suggestion will be wrong and confusing. The fallback span now uses the entire closure, which chas the highest chance of pointing to the place where modifications need to be made.

closure_body_span = closure_body_span.parent().unwrap_or(DUMMY_SP);
}

let (span, sugg, app) =
match self.tcx.sess.source_map().span_to_snippet(closure_body_span) {
Ok(s) => {
let trimmed = s.trim_start();
Expand All @@ -657,9 +667,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
} else {
format!("{{ {}; {} }}", migration_string, s)
};
(sugg, Applicability::MachineApplicable)
(closure_body_span, sugg, Applicability::MachineApplicable)
}
Err(_) => (migration_string.clone(), Applicability::HasPlaceholders),
Err(_) => (closure_span, migration_string.clone(), Applicability::HasPlaceholders),
};

let diagnostic_msg = format!(
Expand All @@ -668,7 +678,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
);

diagnostics_builder.span_suggestion(
closure_body_span,
span,
&diagnostic_msg,
sugg,
app,
Expand Down
16 changes: 16 additions & 0 deletions src/test/ui/closures/2229_closure_analysis/migrations/macro.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// run-rustfix

// See https://github.com/rust-lang/rust/issues/87955

#![deny(rust_2021_incompatible_closure_captures)]
//~^ NOTE: the lint level is defined here

fn main() {
let a = ("hey".to_string(), "123".to_string());
let _ = || { let _ = &a; dbg!(a.0) };
//~^ ERROR: drop order
//~| NOTE: only captures `a.0`
//~| NOTE: for more information, see
//~| HELP: add a dummy let to cause `a` to be fully captured
}
//~^ NOTE: dropped here
16 changes: 16 additions & 0 deletions src/test/ui/closures/2229_closure_analysis/migrations/macro.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// run-rustfix

// See https://github.com/rust-lang/rust/issues/87955

#![deny(rust_2021_incompatible_closure_captures)]
//~^ NOTE: the lint level is defined here

fn main() {
let a = ("hey".to_string(), "123".to_string());
let _ = || dbg!(a.0);
//~^ ERROR: drop order
//~| NOTE: only captures `a.0`
//~| NOTE: for more information, see
//~| HELP: add a dummy let to cause `a` to be fully captured
}
//~^ NOTE: dropped here
24 changes: 24 additions & 0 deletions src/test/ui/closures/2229_closure_analysis/migrations/macro.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
error: changes to closure capture in Rust 2021 will affect drop order
--> $DIR/macro.rs:10:13
|
LL | let _ = || dbg!(a.0);
| ^^^^^^^^---^
| |
| in Rust 2018, closure captures all of `a`, but in Rust 2021, it only captures `a.0`
...
LL | }
| - in Rust 2018, `a` would be dropped here, but in Rust 2021, only `a.0` would be dropped here alongside the closure
Copy link
Member

Choose a reason for hiding this comment

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

Pre-existing: I think saying 'as part as the closure' would make more sense than 'alongside the closure'.

Copy link
Member Author

Choose a reason for hiding this comment

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

|
note: the lint level is defined here
--> $DIR/macro.rs:5:9
|
LL | #![deny(rust_2021_incompatible_closure_captures)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2021/disjoint-capture-in-closures.html>
help: add a dummy let to cause `a` to be fully captured
|
LL | let _ = || { let _ = &a; dbg!(a.0) };
| ~~~~~~~~~~~~~~~~~~~~~~~~~

error: aborting due to previous error