diff --git a/compiler/rustc_typeck/src/check/upvar.rs b/compiler/rustc_typeck/src/check/upvar.rs index 91021b3f6f589..6f8dd39958c04 100644 --- a/compiler/rustc_typeck/src/check/upvar.rs +++ b/compiler/rustc_typeck/src/check/upvar.rs @@ -34,6 +34,7 @@ use super::FnCtxt; use crate::expr_use_visitor as euv; use rustc_data_structures::fx::FxIndexMap; +use rustc_errors::Applicability; use rustc_hir as hir; use rustc_hir::def_id::DefId; use rustc_hir::def_id::LocalDefId; @@ -91,7 +92,7 @@ impl<'a, 'tcx> Visitor<'tcx> for InferBorrowKindVisitor<'a, 'tcx> { if let hir::ExprKind::Closure(cc, _, body_id, _, _) = expr.kind { let body = self.fcx.tcx.hir().body(body_id); self.visit_body(body); - self.fcx.analyze_closure(expr.hir_id, expr.span, body, cc); + self.fcx.analyze_closure(expr.hir_id, expr.span, body_id, body, cc); } intravisit::walk_expr(self, expr); @@ -104,6 +105,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { &self, closure_hir_id: hir::HirId, span: Span, + body_id: hir::BodyId, body: &'tcx hir::Body<'tcx>, capture_clause: hir::CaptureBy, ) { @@ -167,7 +169,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { let closure_hir_id = self.tcx.hir().local_def_id_to_hir_id(local_def_id); if should_do_migration_analysis(self.tcx, closure_hir_id) { - self.perform_2229_migration_anaysis(closure_def_id, capture_clause, span); + self.perform_2229_migration_anaysis(closure_def_id, body_id, capture_clause, span); } // We now fake capture information for all variables that are mentioned within the closure @@ -465,6 +467,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { fn perform_2229_migration_anaysis( &self, closure_def_id: DefId, + body_id: hir::BodyId, capture_clause: hir::CaptureBy, span: Span, ) { @@ -476,7 +479,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { ); if !need_migrations.is_empty() { - let migrations_text = migration_suggestion_for_2229(self.tcx, &need_migrations); + let (migration_string, migrated_variables_concat) = + migration_suggestion_for_2229(self.tcx, &need_migrations); let local_def_id = closure_def_id.expect_local(); let closure_hir_id = self.tcx.hir().local_def_id_to_hir_id(local_def_id); @@ -488,7 +492,35 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { let mut diagnostics_builder = lint.build( "drop order affected for closure because of `capture_disjoint_fields`", ); - diagnostics_builder.note(&migrations_text); + let closure_body_span = self.tcx.hir().span(body_id.hir_id); + let (sugg, app) = + match self.tcx.sess.source_map().span_to_snippet(closure_body_span) { + Ok(s) => { + let trimmed = s.trim_start(); + + // If the closure contains a block then replace the opening brace + // with "{ let _ = (..); " + let sugg = if let Some('{') = trimmed.chars().next() { + format!("{{ {}; {}", migration_string, &trimmed[1..]) + } else { + format!("{{ {}; {} }}", migration_string, s) + }; + (sugg, Applicability::MachineApplicable) + } + Err(_) => (migration_string.clone(), Applicability::HasPlaceholders), + }; + + let diagnostic_msg = format!( + "add a dummy let to cause {} to be fully captured", + migrated_variables_concat + ); + + diagnostics_builder.span_suggestion( + closure_body_span, + &diagnostic_msg, + sugg, + app, + ); diagnostics_builder.emit(); }, ); @@ -621,7 +653,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { /// `w[c]`. /// Notation: /// - Ty(place): Type of place - /// - `(a, b)`: Represents the function parameters `base_path_ty` and `captured_projs` + /// - `(a, b)`: Represents the function parameters `base_path_ty` and `captured_by_move_projs` /// respectively. /// ``` /// (Ty(w), [ &[p, x], &[c] ]) @@ -682,7 +714,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { closure_def_id: DefId, closure_span: Span, base_path_ty: Ty<'tcx>, - captured_projs: Vec<&[Projection<'tcx>]>, + captured_by_move_projs: Vec<&[Projection<'tcx>]>, ) -> bool { let needs_drop = |ty: Ty<'tcx>| { ty.needs_drop(self.tcx, self.tcx.param_env(closure_def_id.expect_local())) @@ -707,9 +739,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // // eg. If `a.b` is captured and we are processing `a.b`, then we can't have the closure also // capture `a.b.c`, because that voilates min capture. - let is_completely_captured = captured_projs.iter().any(|projs| projs.is_empty()); + let is_completely_captured = captured_by_move_projs.iter().any(|projs| projs.is_empty()); - assert!(!is_completely_captured || (captured_projs.len() == 1)); + assert!(!is_completely_captured || (captured_by_move_projs.len() == 1)); if is_completely_captured { // The place is captured entirely, so doesn't matter if needs dtor, it will be drop @@ -717,23 +749,27 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { return false; } + if captured_by_move_projs.is_empty() { + return needs_drop(base_path_ty); + } + if is_drop_defined_for_ty { // If drop is implemented for this type then we need it to be fully captured, - // which we know it is not because of the previous check. Therefore we need to - // do migrate. - return true; - } + // and we know it is not completely captured because of the previous checks. - if captured_projs.is_empty() { - return needs_drop(base_path_ty); + // Note that this is a bug in the user code that will be reported by the + // borrow checker, since we can't move out of drop types. + + // The bug exists in the user's code pre-migration, and we don't migrate here. + return false; } match base_path_ty.kind() { // Observations: - // - `captured_projs` is not empty. Therefore we can call - // `captured_projs.first().unwrap()` safely. - // - All entries in `captured_projs` have atleast one projection. - // Therefore we can call `captured_projs.first().unwrap().first().unwrap()` safely. + // - `captured_by_move_projs` is not empty. Therefore we can call + // `captured_by_move_projs.first().unwrap()` safely. + // - All entries in `captured_by_move_projs` have atleast one projection. + // Therefore we can call `captured_by_move_projs.first().unwrap().first().unwrap()` safely. // We don't capture derefs in case of move captures, which would have be applied to // access any further paths. @@ -743,19 +779,19 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { ty::Adt(def, substs) => { // Multi-varaint enums are captured in entirety, - // which would've been handled in the case of single empty slice in `captured_projs`. + // which would've been handled in the case of single empty slice in `captured_by_move_projs`. assert_eq!(def.variants.len(), 1); // Only Field projections can be applied to a non-box Adt. assert!( - captured_projs.iter().all(|projs| matches!( + captured_by_move_projs.iter().all(|projs| matches!( projs.first().unwrap().kind, ProjectionKind::Field(..) )) ); def.variants.get(VariantIdx::new(0)).unwrap().fields.iter().enumerate().any( |(i, field)| { - let paths_using_field = captured_projs + let paths_using_field = captured_by_move_projs .iter() .filter_map(|projs| { if let ProjectionKind::Field(field_idx, _) = @@ -782,14 +818,14 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { ty::Tuple(..) => { // Only Field projections can be applied to a tuple. assert!( - captured_projs.iter().all(|projs| matches!( + captured_by_move_projs.iter().all(|projs| matches!( projs.first().unwrap().kind, ProjectionKind::Field(..) )) ); base_path_ty.tuple_fields().enumerate().any(|(i, element_ty)| { - let paths_using_field = captured_projs + let paths_using_field = captured_by_move_projs .iter() .filter_map(|projs| { if let ProjectionKind::Field(field_idx, _) = projs.first().unwrap().kind @@ -1515,12 +1551,29 @@ fn should_do_migration_analysis(tcx: TyCtxt<'_>, closure_id: hir::HirId) -> bool !matches!(level, lint::Level::Allow) } -fn migration_suggestion_for_2229(tcx: TyCtxt<'_>, need_migrations: &Vec) -> String { - let need_migrations_strings = - need_migrations.iter().map(|v| format!("{}", var_name(tcx, *v))).collect::>(); - let migrations_list_concat = need_migrations_strings.join(", "); +/// Return a two string tuple (s1, s2) +/// - s1: Line of code that is needed for the migration: eg: `let _ = (&x, ...)`. +/// - s2: Comma separated names of the variables being migrated. +fn migration_suggestion_for_2229( + tcx: TyCtxt<'_>, + need_migrations: &Vec, +) -> (String, String) { + let need_migrations_variables = + need_migrations.iter().map(|v| var_name(tcx, *v)).collect::>(); + + let migration_ref_concat = + need_migrations_variables.iter().map(|v| format!("&{}", v)).collect::>().join(", "); + + let migration_string = if 1 == need_migrations.len() { + format!("let _ = {}", migration_ref_concat) + } else { + format!("let _ = ({})", migration_ref_concat) + }; + + let migrated_variables_concat = + need_migrations_variables.iter().map(|v| format!("`{}`", v)).collect::>().join(", "); - format!("drop(&({}));", migrations_list_concat) + (migration_string, migrated_variables_concat) } /// Helper function to determine if we need to escalate CaptureKind from diff --git a/src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop.fixed b/src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop.fixed new file mode 100644 index 0000000000000..300f67e8b1e81 --- /dev/null +++ b/src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop.fixed @@ -0,0 +1,133 @@ +// run-rustfix + +#![deny(disjoint_capture_drop_reorder)] +//~^ NOTE: the lint level is defined here + +// Test cases for types that implement a insignificant drop (stlib defined) + +// `t` needs Drop because one of its elements needs drop, +// therefore precise capture might affect drop ordering +fn test1_all_need_migration() { + let t = (String::new(), String::new()); + let t1 = (String::new(), String::new()); + let t2 = (String::new(), String::new()); + + let c = || { let _ = (&t, &t1, &t2); + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t`, `t1`, `t2` to be fully captured + + let _t = t.0; + let _t1 = t1.0; + let _t2 = t2.0; + }; + + c(); +} + +// String implements drop and therefore should be migrated. +// But in this test cases, `t2` is completely captured and when it is dropped won't be affected +fn test2_only_precise_paths_need_migration() { + let t = (String::new(), String::new()); + let t1 = (String::new(), String::new()); + let t2 = (String::new(), String::new()); + + let c = || { let _ = (&t, &t1); + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t`, `t1` to be fully captured + let _t = t.0; + let _t1 = t1.0; + let _t2 = t2; + }; + + c(); +} + +// If a variable would've not been captured by value then it would've not been +// dropped with the closure and therefore doesn't need migration. +fn test3_only_by_value_need_migration() { + let t = (String::new(), String::new()); + let t1 = (String::new(), String::new()); + let c = || { let _ = &t; + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t` to be fully captured + let _t = t.0; + println!("{}", t1.1); + }; + + c(); +} + +// Copy types get copied into the closure instead of move. Therefore we don't need to +// migrate then as their drop order isn't tied to the closure. +fn test4_only_non_copy_types_need_migration() { + let t = (String::new(), String::new()); + + // `t1` is Copy because all of its elements are Copy + let t1 = (0i32, 0i32); + + let c = || { let _ = &t; + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t` to be fully captured + let _t = t.0; + let _t1 = t1.0; + }; + + c(); +} + +fn test5_only_drop_types_need_migration() { + struct S(i32, i32); + + let t = (String::new(), String::new()); + + // `s` doesn't implement Drop or any elements within it, and doesn't need migration + let s = S(0i32, 0i32); + + let c = || { let _ = &t; + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t` to be fully captured + let _t = t.0; + let _s = s.0; + }; + + c(); +} + +// Since we are using a move closure here, both `t` and `t1` get moved +// even though they are being used by ref inside the closure. +fn test6_move_closures_non_copy_types_might_need_migration() { + let t = (String::new(), String::new()); + let t1 = (String::new(), String::new()); + let c = move || { let _ = (&t1, &t); + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t1`, `t` to be fully captured + println!("{} {}", t1.1, t.1); + }; + + c(); +} + +// Test migration analysis in case of Drop + Non Drop aggregates. +// Note we need migration here only because the non-copy (because Drop type) is captured, +// otherwise we won't need to, since we can get away with just by ref capture in that case. +fn test7_drop_non_drop_aggregate_need_migration() { + let t = (String::new(), String::new(), 0i32); + + let c = || { let _ = &t; + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t` to be fully captured + let _t = t.0; + }; + + c(); +} + +fn main() { + test1_all_need_migration(); + test2_only_precise_paths_need_migration(); + test3_only_by_value_need_migration(); + test4_only_non_copy_types_need_migration(); + test5_only_drop_types_need_migration(); + test6_move_closures_non_copy_types_might_need_migration(); + test7_drop_non_drop_aggregate_need_migration(); +} diff --git a/src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop.rs b/src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop.rs index 02b373620966e..a17c70d3e2877 100644 --- a/src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop.rs +++ b/src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop.rs @@ -1,3 +1,5 @@ +// run-rustfix + #![deny(disjoint_capture_drop_reorder)] //~^ NOTE: the lint level is defined here @@ -11,8 +13,9 @@ fn test1_all_need_migration() { let t2 = (String::new(), String::new()); let c = || { - //~^ERROR: drop order affected for closure because of `capture_disjoint_fields` - //~| NOTE: drop(&(t, t1, t2)); + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t`, `t1`, `t2` to be fully captured + let _t = t.0; let _t1 = t1.0; let _t2 = t2.0; @@ -29,8 +32,8 @@ fn test2_only_precise_paths_need_migration() { let t2 = (String::new(), String::new()); let c = || { - //~^ERROR: drop order affected for closure because of `capture_disjoint_fields` - //~| NOTE: drop(&(t, t1)); + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t`, `t1` to be fully captured let _t = t.0; let _t1 = t1.0; let _t2 = t2; @@ -45,8 +48,8 @@ fn test3_only_by_value_need_migration() { let t = (String::new(), String::new()); let t1 = (String::new(), String::new()); let c = || { - //~^ERROR: drop order affected for closure because of `capture_disjoint_fields` - //~| NOTE: drop(&(t)); + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t` to be fully captured let _t = t.0; println!("{}", t1.1); }; @@ -63,8 +66,8 @@ fn test4_only_non_copy_types_need_migration() { let t1 = (0i32, 0i32); let c = || { - //~^ERROR: drop order affected for closure because of `capture_disjoint_fields` - //~| NOTE: drop(&(t)); + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t` to be fully captured let _t = t.0; let _t1 = t1.0; }; @@ -81,8 +84,8 @@ fn test5_only_drop_types_need_migration() { let s = S(0i32, 0i32); let c = || { - //~^ERROR: drop order affected for closure because of `capture_disjoint_fields` - //~| NOTE: drop(&(t)); + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t` to be fully captured let _t = t.0; let _s = s.0; }; @@ -96,8 +99,8 @@ fn test6_move_closures_non_copy_types_might_need_migration() { let t = (String::new(), String::new()); let t1 = (String::new(), String::new()); let c = move || { - //~^ERROR: drop order affected for closure because of `capture_disjoint_fields` - //~| NOTE: drop(&(t1, t)); + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t1`, `t` to be fully captured println!("{} {}", t1.1, t.1); }; @@ -111,8 +114,8 @@ fn test7_drop_non_drop_aggregate_need_migration() { let t = (String::new(), String::new(), 0i32); let c = || { - //~^ERROR: drop order affected for closure because of `capture_disjoint_fields` - //~| NOTE: drop(&(t)); + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t` to be fully captured let _t = t.0; }; diff --git a/src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop.stderr b/src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop.stderr index 656c132c12dee..69c12d2bb56c0 100644 --- a/src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop.stderr +++ b/src/test/ui/closures/2229_closure_analysis/migrations/insignificant_drop.stderr @@ -1,25 +1,33 @@ error: drop order affected for closure because of `capture_disjoint_fields` - --> $DIR/insignificant_drop.rs:13:13 + --> $DIR/insignificant_drop.rs:15:13 | LL | let c = || { | _____________^ LL | | LL | | -LL | | let _t = t.0; -LL | | let _t1 = t1.0; +LL | | +... | LL | | let _t2 = t2.0; LL | | }; | |_____^ | note: the lint level is defined here - --> $DIR/insignificant_drop.rs:1:9 + --> $DIR/insignificant_drop.rs:3:9 | LL | #![deny(disjoint_capture_drop_reorder)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - = note: drop(&(t, t1, t2)); +help: add a dummy let to cause `t`, `t1`, `t2` to be fully captured + | +LL | let c = || { let _ = (&t, &t1, &t2); +LL | +LL | +LL | +LL | let _t = t.0; +LL | let _t1 = t1.0; + ... error: drop order affected for closure because of `capture_disjoint_fields` - --> $DIR/insignificant_drop.rs:31:13 + --> $DIR/insignificant_drop.rs:34:13 | LL | let c = || { | _____________^ @@ -31,10 +39,18 @@ LL | | let _t2 = t2; LL | | }; | |_____^ | - = note: drop(&(t, t1)); +help: add a dummy let to cause `t`, `t1` to be fully captured + | +LL | let c = || { let _ = (&t, &t1); +LL | +LL | +LL | let _t = t.0; +LL | let _t1 = t1.0; +LL | let _t2 = t2; + ... error: drop order affected for closure because of `capture_disjoint_fields` - --> $DIR/insignificant_drop.rs:47:13 + --> $DIR/insignificant_drop.rs:50:13 | LL | let c = || { | _____________^ @@ -45,10 +61,18 @@ LL | | println!("{}", t1.1); LL | | }; | |_____^ | - = note: drop(&(t)); +help: add a dummy let to cause `t` to be fully captured + | +LL | let c = || { let _ = &t; +LL | +LL | +LL | let _t = t.0; +LL | println!("{}", t1.1); +LL | }; + | error: drop order affected for closure because of `capture_disjoint_fields` - --> $DIR/insignificant_drop.rs:65:13 + --> $DIR/insignificant_drop.rs:68:13 | LL | let c = || { | _____________^ @@ -59,10 +83,18 @@ LL | | let _t1 = t1.0; LL | | }; | |_____^ | - = note: drop(&(t)); +help: add a dummy let to cause `t` to be fully captured + | +LL | let c = || { let _ = &t; +LL | +LL | +LL | let _t = t.0; +LL | let _t1 = t1.0; +LL | }; + | error: drop order affected for closure because of `capture_disjoint_fields` - --> $DIR/insignificant_drop.rs:83:13 + --> $DIR/insignificant_drop.rs:86:13 | LL | let c = || { | _____________^ @@ -73,10 +105,18 @@ LL | | let _s = s.0; LL | | }; | |_____^ | - = note: drop(&(t)); +help: add a dummy let to cause `t` to be fully captured + | +LL | let c = || { let _ = &t; +LL | +LL | +LL | let _t = t.0; +LL | let _s = s.0; +LL | }; + | error: drop order affected for closure because of `capture_disjoint_fields` - --> $DIR/insignificant_drop.rs:98:13 + --> $DIR/insignificant_drop.rs:101:13 | LL | let c = move || { | _____________^ @@ -86,10 +126,17 @@ LL | | println!("{} {}", t1.1, t.1); LL | | }; | |_____^ | - = note: drop(&(t1, t)); +help: add a dummy let to cause `t1`, `t` to be fully captured + | +LL | let c = move || { let _ = (&t1, &t); +LL | +LL | +LL | println!("{} {}", t1.1, t.1); +LL | }; + | error: drop order affected for closure because of `capture_disjoint_fields` - --> $DIR/insignificant_drop.rs:113:13 + --> $DIR/insignificant_drop.rs:116:13 | LL | let c = || { | _____________^ @@ -99,7 +146,14 @@ LL | | let _t = t.0; LL | | }; | |_____^ | - = note: drop(&(t)); +help: add a dummy let to cause `t` to be fully captured + | +LL | let c = || { let _ = &t; +LL | +LL | +LL | let _t = t.0; +LL | }; + | error: aborting due to 7 previous errors diff --git a/src/test/ui/closures/2229_closure_analysis/migrations/migrations_rustfix.fixed b/src/test/ui/closures/2229_closure_analysis/migrations/migrations_rustfix.fixed new file mode 100644 index 0000000000000..a3e51a2b8e91a --- /dev/null +++ b/src/test/ui/closures/2229_closure_analysis/migrations/migrations_rustfix.fixed @@ -0,0 +1,40 @@ +// run-rustfix +#![deny(disjoint_capture_drop_reorder)] +//~^ NOTE: the lint level is defined here + +// Test the two possible cases for automated migartion using rustfix +// - Closure contains a block i.e. `|| { .. };` +// - Closure contains just an expr `|| ..;` + +#[derive(Debug)] +struct Foo(i32); +impl Drop for Foo { + fn drop(&mut self) { + println!("{:?} dropped", self.0); + } +} + +fn closure_contains_block() { + let t = (Foo(0), Foo(0)); + let c = || { let _ = &t; + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t` to be fully captured + let _t = t.0; + }; + + c(); +} + +fn closure_doesnt_contain_block() { + let t = (Foo(0), Foo(0)); + let c = || { let _ = &t; t.0 }; + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t` to be fully captured + + c(); +} + +fn main() { + closure_contains_block(); + closure_doesnt_contain_block(); +} diff --git a/src/test/ui/closures/2229_closure_analysis/migrations/migrations_rustfix.rs b/src/test/ui/closures/2229_closure_analysis/migrations/migrations_rustfix.rs new file mode 100644 index 0000000000000..0eb837b688835 --- /dev/null +++ b/src/test/ui/closures/2229_closure_analysis/migrations/migrations_rustfix.rs @@ -0,0 +1,40 @@ +// run-rustfix +#![deny(disjoint_capture_drop_reorder)] +//~^ NOTE: the lint level is defined here + +// Test the two possible cases for automated migartion using rustfix +// - Closure contains a block i.e. `|| { .. };` +// - Closure contains just an expr `|| ..;` + +#[derive(Debug)] +struct Foo(i32); +impl Drop for Foo { + fn drop(&mut self) { + println!("{:?} dropped", self.0); + } +} + +fn closure_contains_block() { + let t = (Foo(0), Foo(0)); + let c = || { + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t` to be fully captured + let _t = t.0; + }; + + c(); +} + +fn closure_doesnt_contain_block() { + let t = (Foo(0), Foo(0)); + let c = || t.0; + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t` to be fully captured + + c(); +} + +fn main() { + closure_contains_block(); + closure_doesnt_contain_block(); +} diff --git a/src/test/ui/closures/2229_closure_analysis/migrations/migrations_rustfix.stderr b/src/test/ui/closures/2229_closure_analysis/migrations/migrations_rustfix.stderr new file mode 100644 index 0000000000000..e6173217edc2f --- /dev/null +++ b/src/test/ui/closures/2229_closure_analysis/migrations/migrations_rustfix.stderr @@ -0,0 +1,38 @@ +error: drop order affected for closure because of `capture_disjoint_fields` + --> $DIR/migrations_rustfix.rs:19:13 + | +LL | let c = || { + | _____________^ +LL | | +LL | | +LL | | let _t = t.0; +LL | | }; + | |_____^ + | +note: the lint level is defined here + --> $DIR/migrations_rustfix.rs:2:9 + | +LL | #![deny(disjoint_capture_drop_reorder)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: add a dummy let to cause `t` to be fully captured + | +LL | let c = || { let _ = &t; +LL | +LL | +LL | let _t = t.0; +LL | }; + | + +error: drop order affected for closure because of `capture_disjoint_fields` + --> $DIR/migrations_rustfix.rs:30:13 + | +LL | let c = || t.0; + | ^^^^^^ + | +help: add a dummy let to cause `t` to be fully captured + | +LL | let c = || { let _ = &t; t.0 }; + | ^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 2 previous errors + diff --git a/src/test/ui/closures/2229_closure_analysis/migrations/precise.fixed b/src/test/ui/closures/2229_closure_analysis/migrations/precise.fixed new file mode 100644 index 0000000000000..b739035c78422 --- /dev/null +++ b/src/test/ui/closures/2229_closure_analysis/migrations/precise.fixed @@ -0,0 +1,56 @@ +// run-rustfix + +#![deny(disjoint_capture_drop_reorder)] + +#[derive(Debug)] +struct Foo(i32); +impl Drop for Foo { + fn drop(&mut self) { + println!("{:?} dropped", self.0); + } +} + +struct ConstainsDropField(Foo, Foo); + +// Test that lint is triggered if a path that implements Drop is not captured by move +fn test_precise_analysis_drop_paths_not_captured_by_move() { + let t = ConstainsDropField(Foo(10), Foo(20)); + + let c = || { let _ = &t; + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t` to be fully captured + let _t = t.0; + let _t = &t.1; + }; + + c(); +} + +struct S; +impl Drop for S { + fn drop(&mut self) { + } +} + +struct T(S, S); +struct U(T, T); + +// Test precise analysis for the lint works with paths longer than one. +fn test_precise_analysis_long_path_missing() { + let u = U(T(S, S), T(S, S)); + + let c = || { let _ = &u; + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `u` to be fully captured + let _x = u.0.0; + let _x = u.0.1; + let _x = u.1.0; + }; + + c(); +} + +fn main() { + test_precise_analysis_drop_paths_not_captured_by_move(); + test_precise_analysis_long_path_missing(); +} diff --git a/src/test/ui/closures/2229_closure_analysis/migrations/precise.rs b/src/test/ui/closures/2229_closure_analysis/migrations/precise.rs index 79702cc6b56f3..e1f29c9d0e9d8 100644 --- a/src/test/ui/closures/2229_closure_analysis/migrations/precise.rs +++ b/src/test/ui/closures/2229_closure_analysis/migrations/precise.rs @@ -1,5 +1,6 @@ +// run-rustfix + #![deny(disjoint_capture_drop_reorder)] -//~^ NOTE: the lint level is defined here #[derive(Debug)] struct Foo(i32); @@ -11,35 +12,13 @@ impl Drop for Foo { struct ConstainsDropField(Foo, Foo); -#[derive(Debug)] -struct ContainsAndImplsDrop(Foo); -impl Drop for ContainsAndImplsDrop { - fn drop(&mut self) { - println!("{:?} dropped", self.0); - } -} - -// Test that even if all paths starting at root variable that implement Drop are captured, -// the lint is triggered if the root variable implements drop and isn't captured. -fn test_precise_analysis_parent_root_impl_drop_not_captured() { - let t = ContainsAndImplsDrop(Foo(10)); - - let c = || { - //~^ERROR: drop order affected for closure because of `capture_disjoint_fields` - //~| NOTE: drop(&(t)); - let _t = t.0; - }; - - c(); -} - // Test that lint is triggered if a path that implements Drop is not captured by move fn test_precise_analysis_drop_paths_not_captured_by_move() { let t = ConstainsDropField(Foo(10), Foo(20)); let c = || { - //~^ERROR: drop order affected for closure because of `capture_disjoint_fields` - //~| NOTE: drop(&(t)); + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t` to be fully captured let _t = t.0; let _t = &t.1; }; @@ -61,8 +40,8 @@ fn test_precise_analysis_long_path_missing() { let u = U(T(S, S), T(S, S)); let c = || { - //~^ERROR: drop order affected for closure because of `capture_disjoint_fields` - //~| NOTE: drop(&(u)); + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `u` to be fully captured let _x = u.0.0; let _x = u.0.1; let _x = u.1.0; @@ -72,7 +51,6 @@ fn test_precise_analysis_long_path_missing() { } fn main() { - test_precise_analysis_parent_root_impl_drop_not_captured(); test_precise_analysis_drop_paths_not_captured_by_move(); test_precise_analysis_long_path_missing(); } diff --git a/src/test/ui/closures/2229_closure_analysis/migrations/precise.stderr b/src/test/ui/closures/2229_closure_analysis/migrations/precise.stderr index 968ca395f946e..7135ded13c256 100644 --- a/src/test/ui/closures/2229_closure_analysis/migrations/precise.stderr +++ b/src/test/ui/closures/2229_closure_analysis/migrations/precise.stderr @@ -1,37 +1,32 @@ error: drop order affected for closure because of `capture_disjoint_fields` - --> $DIR/precise.rs:27:13 + --> $DIR/precise.rs:19:13 | LL | let c = || { | _____________^ LL | | LL | | LL | | let _t = t.0; +LL | | let _t = &t.1; LL | | }; | |_____^ | note: the lint level is defined here - --> $DIR/precise.rs:1:9 + --> $DIR/precise.rs:3:9 | LL | #![deny(disjoint_capture_drop_reorder)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - = note: drop(&(t)); - -error: drop order affected for closure because of `capture_disjoint_fields` - --> $DIR/precise.rs:40:13 +help: add a dummy let to cause `t` to be fully captured | -LL | let c = || { - | _____________^ -LL | | -LL | | -LL | | let _t = t.0; -LL | | let _t = &t.1; -LL | | }; - | |_____^ +LL | let c = || { let _ = &t; +LL | +LL | +LL | let _t = t.0; +LL | let _t = &t.1; +LL | }; | - = note: drop(&(t)); error: drop order affected for closure because of `capture_disjoint_fields` - --> $DIR/precise.rs:63:13 + --> $DIR/precise.rs:42:13 | LL | let c = || { | _____________^ @@ -43,7 +38,15 @@ LL | | let _x = u.1.0; LL | | }; | |_____^ | - = note: drop(&(u)); +help: add a dummy let to cause `u` to be fully captured + | +LL | let c = || { let _ = &u; +LL | +LL | +LL | let _x = u.0.0; +LL | let _x = u.0.1; +LL | let _x = u.1.0; + ... -error: aborting due to 3 previous errors +error: aborting due to 2 previous errors diff --git a/src/test/ui/closures/2229_closure_analysis/migrations/significant_drop.fixed b/src/test/ui/closures/2229_closure_analysis/migrations/significant_drop.fixed new file mode 100644 index 0000000000000..e1b212153f431 --- /dev/null +++ b/src/test/ui/closures/2229_closure_analysis/migrations/significant_drop.fixed @@ -0,0 +1,136 @@ +// run-rustfix +#![deny(disjoint_capture_drop_reorder)] +//~^ NOTE: the lint level is defined here + +// Test cases for types that implement a significant drop (user defined) + +#[derive(Debug)] +struct Foo(i32); +impl Drop for Foo { + fn drop(&mut self) { + println!("{:?} dropped", self.0); + } +} + +#[derive(Debug)] +struct ConstainsDropField(Foo, Foo); + +// `t` needs Drop because one of its elements needs drop, +// therefore precise capture might affect drop ordering +fn test1_all_need_migration() { + let t = (Foo(0), Foo(0)); + let t1 = (Foo(0), Foo(0)); + let t2 = (Foo(0), Foo(0)); + + let c = || { let _ = (&t, &t1, &t2); + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t`, `t1`, `t2` to be fully captured + let _t = t.0; + let _t1 = t1.0; + let _t2 = t2.0; + }; + + c(); +} + +// String implements drop and therefore should be migrated. +// But in this test cases, `t2` is completely captured and when it is dropped won't be affected +fn test2_only_precise_paths_need_migration() { + let t = (Foo(0), Foo(0)); + let t1 = (Foo(0), Foo(0)); + let t2 = (Foo(0), Foo(0)); + + let c = || { let _ = (&t, &t1); + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t`, `t1` to be fully captured + let _t = t.0; + let _t1 = t1.0; + let _t2 = t2; + }; + + c(); +} + +// If a variable would've not been captured by value then it would've not been +// dropped with the closure and therefore doesn't need migration. +fn test3_only_by_value_need_migration() { + let t = (Foo(0), Foo(0)); + let t1 = (Foo(0), Foo(0)); + let c = || { let _ = &t; + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t` to be fully captured + let _t = t.0; + println!("{:?}", t1.1); + }; + + c(); +} + +// The root variable might not implement drop themselves but some path starting +// at the root variable might implement Drop. +// +// If this path isn't captured we need to migrate for the root variable. +fn test4_type_contains_drop_need_migration() { + let t = ConstainsDropField(Foo(0), Foo(0)); + + let c = || { let _ = &t; + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t` to be fully captured + let _t = t.0; + }; + + c(); +} + +// Test migration analysis in case of Drop + Non Drop aggregates. +// Note we need migration here only because the non-copy (because Drop type) is captured, +// otherwise we won't need to, since we can get away with just by ref capture in that case. +fn test5_drop_non_drop_aggregate_need_migration() { + let t = (Foo(0), Foo(0), 0i32); + + let c = || { let _ = &t; + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t` to be fully captured + let _t = t.0; + }; + + c(); +} + +// Test migration analysis in case of Significant and Insignificant Drop aggregates. +fn test6_significant_insignificant_drop_aggregate_need_migration() { + let t = (Foo(0), String::new()); + + let c = || { let _ = &t; + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t` to be fully captured + let _t = t.1; + }; + + c(); +} + +// Since we are using a move closure here, both `t` and `t1` get moved +// even though they are being used by ref inside the closure. +fn test7_move_closures_non_copy_types_might_need_migration() { + let t = (Foo(0), Foo(0)); + let t1 = (Foo(0), Foo(0), Foo(0)); + + let c = move || { let _ = (&t1, &t); + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t1`, `t` to be fully captured + println!("{:?} {:?}", t1.1, t.1); + }; + + c(); +} + +fn main() { + test1_all_need_migration(); + test2_only_precise_paths_need_migration(); + test3_only_by_value_need_migration(); + test4_type_contains_drop_need_migration(); + test5_drop_non_drop_aggregate_need_migration(); + test6_significant_insignificant_drop_aggregate_need_migration(); + test7_move_closures_non_copy_types_might_need_migration(); +} diff --git a/src/test/ui/closures/2229_closure_analysis/migrations/significant_drop.rs b/src/test/ui/closures/2229_closure_analysis/migrations/significant_drop.rs index ed5e4ea8be011..106b293351515 100644 --- a/src/test/ui/closures/2229_closure_analysis/migrations/significant_drop.rs +++ b/src/test/ui/closures/2229_closure_analysis/migrations/significant_drop.rs @@ -1,3 +1,4 @@ +// run-rustfix #![deny(disjoint_capture_drop_reorder)] //~^ NOTE: the lint level is defined here @@ -22,8 +23,8 @@ fn test1_all_need_migration() { let t2 = (Foo(0), Foo(0)); let c = || { - //~^ERROR: drop order affected for closure because of `capture_disjoint_fields` - //~| NOTE: drop(&(t, t1, t2)); + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t`, `t1`, `t2` to be fully captured let _t = t.0; let _t1 = t1.0; let _t2 = t2.0; @@ -40,8 +41,8 @@ fn test2_only_precise_paths_need_migration() { let t2 = (Foo(0), Foo(0)); let c = || { - //~^ERROR: drop order affected for closure because of `capture_disjoint_fields` - //~| NOTE: drop(&(t, t1)); + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t`, `t1` to be fully captured let _t = t.0; let _t1 = t1.0; let _t2 = t2; @@ -56,8 +57,8 @@ fn test3_only_by_value_need_migration() { let t = (Foo(0), Foo(0)); let t1 = (Foo(0), Foo(0)); let c = || { - //~^ERROR: drop order affected for closure because of `capture_disjoint_fields` - //~| NOTE: drop(&(t)); + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t` to be fully captured let _t = t.0; println!("{:?}", t1.1); }; @@ -73,8 +74,8 @@ fn test4_type_contains_drop_need_migration() { let t = ConstainsDropField(Foo(0), Foo(0)); let c = || { - //~^ERROR: drop order affected for closure because of `capture_disjoint_fields` - //~| NOTE: drop(&(t)); + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t` to be fully captured let _t = t.0; }; @@ -88,8 +89,8 @@ fn test5_drop_non_drop_aggregate_need_migration() { let t = (Foo(0), Foo(0), 0i32); let c = || { - //~^ERROR: drop order affected for closure because of `capture_disjoint_fields` - //~| NOTE: drop(&(t)); + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t` to be fully captured let _t = t.0; }; @@ -98,13 +99,11 @@ fn test5_drop_non_drop_aggregate_need_migration() { // Test migration analysis in case of Significant and Insignificant Drop aggregates. fn test6_significant_insignificant_drop_aggregate_need_migration() { - struct S(i32, i32); - let t = (Foo(0), String::new()); let c = || { - //~^ERROR: drop order affected for closure because of `capture_disjoint_fields` - //~| NOTE: drop(&(t)); + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t` to be fully captured let _t = t.1; }; @@ -118,8 +117,8 @@ fn test7_move_closures_non_copy_types_might_need_migration() { let t1 = (Foo(0), Foo(0), Foo(0)); let c = move || { - //~^ERROR: drop order affected for closure because of `capture_disjoint_fields` - //~| NOTE: drop(&(t1, t)); + //~^ ERROR: drop order affected for closure because of `capture_disjoint_fields` + //~| HELP: add a dummy let to cause `t1`, `t` to be fully captured println!("{:?} {:?}", t1.1, t.1); }; diff --git a/src/test/ui/closures/2229_closure_analysis/migrations/significant_drop.stderr b/src/test/ui/closures/2229_closure_analysis/migrations/significant_drop.stderr index 6c21b27b493ba..ee29fe1306059 100644 --- a/src/test/ui/closures/2229_closure_analysis/migrations/significant_drop.stderr +++ b/src/test/ui/closures/2229_closure_analysis/migrations/significant_drop.stderr @@ -1,5 +1,5 @@ error: drop order affected for closure because of `capture_disjoint_fields` - --> $DIR/significant_drop.rs:24:13 + --> $DIR/significant_drop.rs:25:13 | LL | let c = || { | _____________^ @@ -12,14 +12,22 @@ LL | | }; | |_____^ | note: the lint level is defined here - --> $DIR/significant_drop.rs:1:9 + --> $DIR/significant_drop.rs:2:9 | LL | #![deny(disjoint_capture_drop_reorder)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - = note: drop(&(t, t1, t2)); +help: add a dummy let to cause `t`, `t1`, `t2` to be fully captured + | +LL | let c = || { let _ = (&t, &t1, &t2); +LL | +LL | +LL | let _t = t.0; +LL | let _t1 = t1.0; +LL | let _t2 = t2.0; + ... error: drop order affected for closure because of `capture_disjoint_fields` - --> $DIR/significant_drop.rs:42:13 + --> $DIR/significant_drop.rs:43:13 | LL | let c = || { | _____________^ @@ -31,10 +39,18 @@ LL | | let _t2 = t2; LL | | }; | |_____^ | - = note: drop(&(t, t1)); +help: add a dummy let to cause `t`, `t1` to be fully captured + | +LL | let c = || { let _ = (&t, &t1); +LL | +LL | +LL | let _t = t.0; +LL | let _t1 = t1.0; +LL | let _t2 = t2; + ... error: drop order affected for closure because of `capture_disjoint_fields` - --> $DIR/significant_drop.rs:58:13 + --> $DIR/significant_drop.rs:59:13 | LL | let c = || { | _____________^ @@ -45,10 +61,18 @@ LL | | println!("{:?}", t1.1); LL | | }; | |_____^ | - = note: drop(&(t)); +help: add a dummy let to cause `t` to be fully captured + | +LL | let c = || { let _ = &t; +LL | +LL | +LL | let _t = t.0; +LL | println!("{:?}", t1.1); +LL | }; + | error: drop order affected for closure because of `capture_disjoint_fields` - --> $DIR/significant_drop.rs:75:13 + --> $DIR/significant_drop.rs:76:13 | LL | let c = || { | _____________^ @@ -58,10 +82,17 @@ LL | | let _t = t.0; LL | | }; | |_____^ | - = note: drop(&(t)); +help: add a dummy let to cause `t` to be fully captured + | +LL | let c = || { let _ = &t; +LL | +LL | +LL | let _t = t.0; +LL | }; + | error: drop order affected for closure because of `capture_disjoint_fields` - --> $DIR/significant_drop.rs:90:13 + --> $DIR/significant_drop.rs:91:13 | LL | let c = || { | _____________^ @@ -71,10 +102,17 @@ LL | | let _t = t.0; LL | | }; | |_____^ | - = note: drop(&(t)); +help: add a dummy let to cause `t` to be fully captured + | +LL | let c = || { let _ = &t; +LL | +LL | +LL | let _t = t.0; +LL | }; + | error: drop order affected for closure because of `capture_disjoint_fields` - --> $DIR/significant_drop.rs:105:13 + --> $DIR/significant_drop.rs:104:13 | LL | let c = || { | _____________^ @@ -84,10 +122,17 @@ LL | | let _t = t.1; LL | | }; | |_____^ | - = note: drop(&(t)); +help: add a dummy let to cause `t` to be fully captured + | +LL | let c = || { let _ = &t; +LL | +LL | +LL | let _t = t.1; +LL | }; + | error: drop order affected for closure because of `capture_disjoint_fields` - --> $DIR/significant_drop.rs:120:13 + --> $DIR/significant_drop.rs:119:13 | LL | let c = move || { | _____________^ @@ -97,7 +142,14 @@ LL | | println!("{:?} {:?}", t1.1, t.1); LL | | }; | |_____^ | - = note: drop(&(t1, t)); +help: add a dummy let to cause `t1`, `t` to be fully captured + | +LL | let c = move || { let _ = (&t1, &t); +LL | +LL | +LL | println!("{:?} {:?}", t1.1, t.1); +LL | }; + | error: aborting due to 7 previous errors