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.

Sign up for GitHub

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

Jump to bottom

Warn for variables that are no longer captured #90597

Merged
merged 4 commits into from
Nov 6, 2021
Merged

Warn for variables that are no longer captured #90597

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9c84ac8
introduce an enum for tracking the 2229 migration causes
nikomatsakis Nov 4, 2021
76bc027
rework diagnostic reporting to be more structured
nikomatsakis Nov 4, 2021
fc8113d
handle case of a variable not captured
nikomatsakis Nov 5, 2021
4154e8a
apply suggestions from code review
nikomatsakis Nov 5, 2021
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
185 changes: 108 additions & 77 deletions compiler/rustc_typeck/src/check/upvar.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,19 +88,53 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
/// name (i.e: a.b.c)
#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
enum CapturesInfo {
nikomatsakis marked this conversation as resolved.
Show resolved Hide resolved
/// We previously captured all of `x`, but now we capture some sub-path.
CapturingLess { source_expr: Option<hir::HirId>, var_name: String },
nikomatsakis marked this conversation as resolved.
Show resolved Hide resolved
//CapturingNothing {
// // where the variable appears in the closure (but is not captured)
// use_span: Span,
//},
}

/// Intermediate format to store information needed to generate migration lint. The tuple
/// contains the hir_id pointing to the use that resulted in the
/// corresponding place being captured, a String which contains the captured value's
/// name (i.e: a.b.c) and a String which contains the reason why migration is needed for that
/// capture
type MigrationNeededForCapture = (Option<hir::HirId>, String, String);
/// Reasons that we might issue a migration warning.
#[derive(Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
struct MigrationWarningReason {
/// When we used to capture `x` in its entirety, we implemented the auto-trait(s)
/// in this vec, but now we don't.
auto_traits: Vec<&'static str>,

/// When we used to capture `x` in its entirety, we would execute some destructors
/// at a different time.
drop_order: bool,
}

impl MigrationWarningReason {
fn migration_message(&self) -> String {
let base = "changes to closure capture in Rust 2021 will affect";
if !self.auto_traits.is_empty() && self.drop_order {
nikomatsakis marked this conversation as resolved.
Show resolved Hide resolved
format!("{} drop order and which traits the closure implements", base)
} else if self.drop_order {
format!("{} drop order", base)
} else {
format!("{} which traits the closure implements", base)
}
}
}

/// Intermediate format to store information needed to generate a note in the migration lint.
struct MigrationLintNote {
captures_info: CapturesInfo,

/// reasons why migration is needed for this capture
reason: MigrationWarningReason,
}

/// Intermediate format to store the hir id of the root variable and a HashSet containing
/// information on why the root variable should be fully captured
type MigrationDiagnosticInfo = (hir::HirId, Vec<MigrationNeededForCapture>);
struct NeededMigration {
var_hir_id: hir::HirId,
diagnostics_info: Vec<MigrationLintNote>,
}

struct InferBorrowKindVisitor<'a, 'tcx> {
fcx: &'a FnCtxt<'a, 'tcx>,
Expand Down Expand Up @@ -710,47 +744,50 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
closure_head_span,
|lint| {
let mut diagnostics_builder = lint.build(
format!(
"changes to closure capture in Rust 2021 will affect {}",
reasons
)
.as_str(),
&reasons.migration_message(),
);
for (var_hir_id, diagnostics_info) in need_migrations.iter() {
for NeededMigration { var_hir_id, diagnostics_info } in &need_migrations {
// Labels all the usage of the captured variable and why they are responsible
// for migration being needed
for (captured_hir_id, captured_name, reasons) in diagnostics_info.iter() {
if let Some(captured_hir_id) = captured_hir_id {
let cause_span = self.tcx.hir().span(*captured_hir_id);
diagnostics_builder.span_label(cause_span, format!("in Rust 2018, this closure captures all of `{}`, but in Rust 2021, it will only capture `{}`",
self.tcx.hir().name(*var_hir_id),
captured_name,
));
for lint_note in diagnostics_info.iter() {
match &lint_note.captures_info {
CapturesInfo::CapturingLess { source_expr: Some(capture_expr_id), var_name: captured_name } => {
let cause_span = self.tcx.hir().span(*capture_expr_id);
diagnostics_builder.span_label(cause_span, format!("in Rust 2018, this closure captures all of `{}`, but in Rust 2021, it will only capture `{}`",
self.tcx.hir().name(*var_hir_id),
captured_name,
));
}
_ => { }
}

// Add a label pointing to where a captured variable affected by drop order
// is dropped
if reasons.contains("drop order") {
if lint_note.reason.drop_order {
let drop_location_span = drop_location_span(self.tcx, &closure_hir_id);

diagnostics_builder.span_label(drop_location_span, format!("in Rust 2018, `{}` is dropped here, but in Rust 2021, only `{}` will be dropped here as part of the closure",
self.tcx.hir().name(*var_hir_id),
captured_name,
));
match &lint_note.captures_info {
CapturesInfo::CapturingLess { var_name: captured_name, .. } => {
diagnostics_builder.span_label(drop_location_span, format!("in Rust 2018, `{}` is dropped here, but in Rust 2021, only `{}` will be dropped here as part of the closure",
self.tcx.hir().name(*var_hir_id),
captured_name,
));
}
}
}

// Add a label explaining why a closure no longer implements a trait
if reasons.contains("trait implementation") {
let missing_trait = &reasons[..reasons.find("trait implementation").unwrap() - 1];

diagnostics_builder.span_label(closure_head_span, format!("in Rust 2018, this closure implements {} as `{}` implements {}, but in Rust 2021, this closure will no longer implement {} as `{}` does not implement {}",
missing_trait,
self.tcx.hir().name(*var_hir_id),
missing_trait,
missing_trait,
captured_name,
missing_trait,
));
for &missing_trait in &lint_note.reason.auto_traits {
// not capturing something anymore cannot cause a trait to fail to be implemented:
match &lint_note.captures_info {
CapturesInfo::CapturingLess { var_name: captured_name, .. } => {
diagnostics_builder.span_label(closure_head_span, format!("in Rust 2018, this closure implements {missing_trait} as `{x}` implements {missing_trait}, but in Rust 2021, this closure will no longer implement {missing_trait} as `{p}` does not implement {missing_trait}",
nikomatsakis marked this conversation as resolved.
Show resolved Hide resolved
missing_trait = missing_trait,
nikomatsakis marked this conversation as resolved.
Show resolved Hide resolved
x = self.tcx.hir().name(*var_hir_id),
p = captured_name,
));
}
}
}
}
}
Expand Down Expand Up @@ -843,25 +880,16 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
/// Combines all the reasons for 2229 migrations
fn compute_2229_migrations_reasons(
&self,
auto_trait_reasons: FxHashSet<&str>,
drop_reason: bool,
) -> String {
let mut reasons = String::new();

if !auto_trait_reasons.is_empty() {
reasons = format!(
"{} trait implementation for closure",
auto_trait_reasons.clone().into_iter().collect::<Vec<&str>>().join(", ")
);
}
auto_trait_reasons: FxHashSet<&'static str>,
drop_order: bool,
) -> MigrationWarningReason {
let mut reasons = MigrationWarningReason::default();

if !auto_trait_reasons.is_empty() && drop_reason {
reasons = format!("{} and ", reasons);
for auto_trait in auto_trait_reasons {
reasons.auto_traits.push(auto_trait);
}

if drop_reason {
reasons = format!("{}drop order", reasons);
}
reasons.drop_order = drop_order;

reasons
}
Expand All @@ -877,7 +905,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
min_captures: Option<&ty::RootVariableMinCaptureList<'tcx>>,
var_hir_id: hir::HirId,
closure_clause: hir::CaptureBy,
) -> Option<FxHashMap<CapturesInfo, FxHashSet<&str>>> {
) -> Option<FxHashMap<CapturesInfo, FxHashSet<&'static str>>> {
let auto_traits_def_id = vec![
self.tcx.lang_items().clone_trait(),
self.tcx.lang_items().sync_trait(),
Expand Down Expand Up @@ -1022,7 +1050,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

match closure_clause {
// Only migrate if closure is a move closure
hir::CaptureBy::Value => return Some(FxHashSet::default()),
nikomatsakis marked this conversation as resolved.
Show resolved Hide resolved
hir::CaptureBy::Value => {
let diagnostics_info = FxHashSet::default();
//diagnostics_info.insert(CapturesInfo::CapturingNothing);
//let upvars = self.tcx.upvars_mentioned(closure_def_id).expect("must be an upvar");
//let _span = upvars[&var_hir_id];
return Some(diagnostics_info);
}
hir::CaptureBy::Ref => {}
}

Expand Down Expand Up @@ -1095,9 +1129,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
closure_span: Span,
closure_clause: hir::CaptureBy,
min_captures: Option<&ty::RootVariableMinCaptureList<'tcx>>,
) -> (Vec<MigrationDiagnosticInfo>, String) {
) -> (Vec<NeededMigration>, MigrationWarningReason) {
let Some(upvars) = self.tcx.upvars_mentioned(closure_def_id) else {
return (Vec::new(), String::new());
return (Vec::new(), MigrationWarningReason::default());
};

let mut need_migrations = Vec::new();
Expand All @@ -1106,7 +1140,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

// Perform auto-trait analysis
for (&var_hir_id, _) in upvars.iter() {
let mut responsible_captured_hir_ids = Vec::new();
let mut diagnostics_info = Vec::new();

let auto_trait_diagnostic = if let Some(diagnostics_info) =
self.compute_2229_migrations_for_trait(min_captures, var_hir_id, closure_clause)
Expand Down Expand Up @@ -1138,38 +1172,33 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

let mut capture_diagnostic = capture_diagnostic.into_iter().collect::<Vec<_>>();
capture_diagnostic.sort();
for captured_info in capture_diagnostic.iter() {
for captures_info in capture_diagnostic {
// Get the auto trait reasons of why migration is needed because of that capture, if there are any
let capture_trait_reasons =
if let Some(reasons) = auto_trait_diagnostic.get(captured_info) {
if let Some(reasons) = auto_trait_diagnostic.get(&captures_info) {
reasons.clone()
} else {
FxHashSet::default()
};

// Check if migration is needed because of drop reorder as a result of that capture
let capture_drop_reorder_reason = drop_reorder_diagnostic.contains(captured_info);
let capture_drop_reorder_reason = drop_reorder_diagnostic.contains(&captures_info);

// Combine all the reasons of why the root variable should be captured as a result of
// auto trait implementation issues
auto_trait_migration_reasons.extend(capture_trait_reasons.clone());

match captured_info {
CapturesInfo::CapturingLess { source_expr, var_name } => {
responsible_captured_hir_ids.push((
*source_expr,
var_name.clone(),
self.compute_2229_migrations_reasons(
capture_trait_reasons,
capture_drop_reorder_reason,
),
));
}
}
diagnostics_info.push(MigrationLintNote {
captures_info,
reason: self.compute_2229_migrations_reasons(
capture_trait_reasons,
capture_drop_reorder_reason,
),
});
}

if !capture_diagnostic.is_empty() {
need_migrations.push((var_hir_id, responsible_captured_hir_ids));
if !diagnostics_info.is_empty() {
need_migrations.push(NeededMigration { var_hir_id, diagnostics_info });
}
}
(
Expand Down Expand Up @@ -2130,10 +2159,12 @@ fn should_do_rust_2021_incompatible_closure_captures_analysis(
/// - s2: Comma separated names of the variables being migrated.
fn migration_suggestion_for_2229(
tcx: TyCtxt<'_>,
need_migrations: &Vec<MigrationDiagnosticInfo>,
need_migrations: &Vec<NeededMigration>,
) -> (String, String) {
let need_migrations_variables =
need_migrations.iter().map(|(v, _)| var_name(tcx, *v)).collect::<Vec<_>>();
let need_migrations_variables = need_migrations
.iter()
.map(|NeededMigration { var_hir_id: v, .. }| var_name(tcx, *v))
.collect::<Vec<_>>();

let migration_ref_concat =
need_migrations_variables.iter().map(|v| format!("&{}", v)).collect::<Vec<_>>().join(", ");
Expand Down
9 changes: 5 additions & 4 deletions src/test/ui/closures/2229_closure_analysis/migrations/auto_traits.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ fn test_send_trait() {
let mut f = 10;
let fptr = SendPointer(&mut f as *mut i32);
thread::spawn(move || { let _ = &fptr; unsafe {
//~^ ERROR: `Send` trait implementation for closure
//~^ ERROR: changes to closure capture
//~| NOTE: in Rust 2018, this closure implements `Send` as `fptr` implements `Send`, but in Rust 2021, this closure will no longer implement `Send` as `fptr.0` does not implement `Send`
//~| NOTE: for more information, see
//~| HELP: add a dummy let to cause `fptr` to be fully captured
Expand All @@ -40,8 +40,9 @@ fn test_sync_trait() {
let f = CustomInt(&mut f as *mut i32);
let fptr = SyncPointer(f);
thread::spawn(move || { let _ = &fptr; unsafe {
//~^ ERROR: `Sync`, `Send` trait implementation for closure
//~| NOTE: in Rust 2018, this closure implements `Sync`, `Send` as `fptr` implements `Sync`, `Send`, but in Rust 2021, this closure will no longer implement `Sync`, `Send` as `fptr.0.0` does not implement `Sync`, `Send`
//~^ ERROR: changes to closure capture
//~| NOTE: in Rust 2018, this closure implements `Sync` as `fptr` implements `Sync`, but in Rust 2021, this closure will no longer implement `Sync` as `fptr.0.0` does not implement `Sync`
//~| NOTE: in Rust 2018, this closure implements `Send` as `fptr` implements `Send`, but in Rust 2021, this closure will no longer implement `Send` as `fptr.0.0` does not implement `Send`
//~| NOTE: for more information, see
//~| HELP: add a dummy let to cause `fptr` to be fully captured
*fptr.0.0 = 20;
Expand All @@ -65,7 +66,7 @@ fn test_clone_trait() {
let f = U(S(Foo(0)), T(0));
let c = || {
let _ = &f;
//~^ ERROR: `Clone` trait implementation for closure and drop order
//~^ ERROR: changes to closure capture in Rust 2021 will affect drop order and which traits the closure implements
//~| NOTE: in Rust 2018, this closure implements `Clone` as `f` implements `Clone`, but in Rust 2021, this closure will no longer implement `Clone` as `f.1` does not implement `Clone`
//~| NOTE: for more information, see
//~| HELP: add a dummy let to cause `f` to be fully captured
Expand Down
9 changes: 5 additions & 4 deletions src/test/ui/closures/2229_closure_analysis/migrations/auto_traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ fn test_send_trait() {
let mut f = 10;
let fptr = SendPointer(&mut f as *mut i32);
thread::spawn(move || unsafe {
//~^ ERROR: `Send` trait implementation for closure
//~^ ERROR: changes to closure capture
//~| NOTE: in Rust 2018, this closure implements `Send` as `fptr` implements `Send`, but in Rust 2021, this closure will no longer implement `Send` as `fptr.0` does not implement `Send`
//~| NOTE: for more information, see
//~| HELP: add a dummy let to cause `fptr` to be fully captured
Expand All @@ -40,8 +40,9 @@ fn test_sync_trait() {
let f = CustomInt(&mut f as *mut i32);
let fptr = SyncPointer(f);
thread::spawn(move || unsafe {
//~^ ERROR: `Sync`, `Send` trait implementation for closure
//~| NOTE: in Rust 2018, this closure implements `Sync`, `Send` as `fptr` implements `Sync`, `Send`, but in Rust 2021, this closure will no longer implement `Sync`, `Send` as `fptr.0.0` does not implement `Sync`, `Send`
//~^ ERROR: changes to closure capture
//~| NOTE: in Rust 2018, this closure implements `Sync` as `fptr` implements `Sync`, but in Rust 2021, this closure will no longer implement `Sync` as `fptr.0.0` does not implement `Sync`
//~| NOTE: in Rust 2018, this closure implements `Send` as `fptr` implements `Send`, but in Rust 2021, this closure will no longer implement `Send` as `fptr.0.0` does not implement `Send`
//~| NOTE: for more information, see
//~| HELP: add a dummy let to cause `fptr` to be fully captured
*fptr.0.0 = 20;
Expand All @@ -64,7 +65,7 @@ impl Clone for U {
fn test_clone_trait() {
let f = U(S(Foo(0)), T(0));
let c = || {
//~^ ERROR: `Clone` trait implementation for closure and drop order
//~^ ERROR: changes to closure capture in Rust 2021 will affect drop order and which traits the closure implements
//~| NOTE: in Rust 2018, this closure implements `Clone` as `f` implements `Clone`, but in Rust 2021, this closure will no longer implement `Clone` as `f.1` does not implement `Clone`
//~| NOTE: for more information, see
//~| HELP: add a dummy let to cause `f` to be fully captured
Expand Down
Loading