nordic-dev.net/rust
nordic-dev.net/rust
2
0
Fork 0
mirror of https://github.com/rust-lang/rust.git synced 2025-04-28 02:57:37 +00:00
Code Issues Packages Projects Releases Wiki Activity

Rollup merge of #81912 - sexxi-goose:Migrations2_review, r=nikomatsakis

Browse Source 
Implement the precise analysis pass for lint `disjoint_capture_drop_reorder`

The precision pass for the lint prevents the lint from triggering for a variable (that was previously entirely captured by the closure) if all paths that need Drop starting at root variable have been captured by the closure.

r? `@nikomatsakis`
This commit is contained in:
Dylan DPC 2021-02-14 16:54:46 +01:00 committed by GitHub
commit 25315635ef
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 502 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

283
compiler/rustc_typeck/src/check/upvar.rs
View File

@ -40,13 +40,16 @@ use rustc_hir::def_id::DefId;
use rustc_hir::def_id::LocalDefId;
use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
use rustc_infer::infer::UpvarRegion;
use rustc_middle::hir::place::{Place, PlaceBase, PlaceWithHirId, ProjectionKind};
use rustc_middle::hir::place::{Place, PlaceBase, PlaceWithHirId, Projection, ProjectionKind};
use rustc_middle::ty::fold::TypeFoldable;
use rustc_middle::ty::{self, Ty, TyCtxt, TypeckResults, UpvarSubsts};
use rustc_session::lint;
use rustc_span::sym;
use rustc_span::{MultiSpan, Span, Symbol};
use rustc_index::vec::Idx;
use rustc_target::abi::VariantIdx;
/// Describe the relationship between the paths of two places
/// eg:
/// - `foo` is ancestor of `foo.bar.baz`
@ -535,7 +538,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
span: Span,
body: &'tcx hir::Body<'tcx>,
) {
let need_migrations = self.compute_2229_migrations_first_pass(
let need_migrations = self.compute_2229_migrations(
closure_def_id,
span,
capture_clause,
@ -544,9 +547,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
);
if !need_migrations.is_empty() {
let need_migrations_hir_id = need_migrations.iter().map(|m| m.0).collect::<Vec<_>>();
let migrations_text = migration_suggestion_for_2229(self.tcx, &need_migrations_hir_id);
let migrations_text = 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);
@ -573,15 +574,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
/// - It would have been moved into the closure when `capture_disjoint_fields` wasn't
/// enabled, **and**
/// - It wasn't completely captured by the closure, **and**
/// - The type of the root variable needs Drop.
fn compute_2229_migrations_first_pass(
/// - One of the paths starting at this root variable, that is not captured needs Drop.
fn compute_2229_migrations(
&self,
closure_def_id: DefId,
closure_span: Span,
closure_clause: hir::CaptureBy,
body: &'tcx hir::Body<'tcx>,
min_captures: Option<&ty::RootVariableMinCaptureList<'tcx>>,
) -> Vec<(hir::HirId, Ty<'tcx>)> {
) -> Vec<hir::HirId> {
fn resolve_ty<T: TypeFoldable<'tcx>>(
fcx: &FnCtxt<'_, 'tcx>,
span: Span,
@ -617,7 +618,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
match closure_clause {
// Only migrate if closure is a move closure
hir::CaptureBy::Value => need_migrations.push((var_hir_id, ty)),
hir::CaptureBy::Value => need_migrations.push(var_hir_id),
hir::CaptureBy::Ref => {}
}
@ -625,21 +626,277 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
continue;
};
let is_moved = root_var_min_capture_list
let projections_list = root_var_min_capture_list
.iter()
.any(|capture| matches!(capture.info.capture_kind, ty::UpvarCapture::ByValue(_)));
.filter_map(|captured_place| match captured_place.info.capture_kind {
// Only care about captures that are moved into the closure
ty::UpvarCapture::ByValue(..) => {
Some(captured_place.place.projections.as_slice())
}
ty::UpvarCapture::ByRef(..) => None,
})
.collect::<Vec<_>>();
let is_moved = !projections_list.is_empty();
let is_not_completely_captured =
root_var_min_capture_list.iter().any(|capture| capture.place.projections.len() > 0);
if is_moved && is_not_completely_captured {
need_migrations.push((var_hir_id, ty));
if is_moved
&& is_not_completely_captured
&& self.has_significant_drop_outside_of_captures(
closure_def_id,
closure_span,
ty,
projections_list,
)
{
need_migrations.push(var_hir_id);
}
}
need_migrations
}
/// This is a helper function to `compute_2229_migrations_precise_pass`. Provided the type
/// of a root variable and a list of captured paths starting at this root variable (expressed
/// using list of `Projection` slices), it returns true if there is a path that is not
/// captured starting at this root variable that implements Drop.
///
/// FIXME(project-rfc-2229#35): This should return true only for significant drops.
/// A drop is significant if it's implemented by the user or does
/// anything that will have any observable behavior (other than
/// freeing up memory).
///
/// The way this function works is at a given call it looks at type `base_path_ty` of some base
/// path say P and then list of projection slices which represent the different captures moved
/// into the closure starting off of P.
///
/// This will make more sense with an example:
///
/// ```rust
/// #![feature(capture_disjoint_fields)]
///
/// struct FancyInteger(i32); // This implements Drop
///
/// struct Point { x: FancyInteger, y: FancyInteger }
/// struct Color;
///
/// struct Wrapper { p: Point, c: Color }
///
/// fn f(w: Wrapper) {
/// let c = || {
/// // Closure captures w.p.x and w.c by move.
/// };
///
/// c();
/// }
/// ```
///
/// If `capture_disjoint_fields` wasn't enabled the closure would've moved `w` instead of the
/// precise paths. If we look closely `w.p.y` isn't captured which implements Drop and
/// therefore Drop ordering would change and we want this function to return true.
///
/// Call stack to figure out if we need to migrate for `w` would look as follows:
///
/// Our initial base path is just `w`, and the paths captured from it are `w[p, x]` and
/// `w[c]`.
/// Notation:
/// - Ty(place): Type of place
/// - `(a, b)`: Represents the function parameters `base_path_ty` and `captured_projs`
/// respectively.
/// ```
/// (Ty(w), [ &[p, x], &[c] ])
/// |
/// ----------------------------
/// | |
/// v v
/// (Ty(w.p), [ &[x] ]) (Ty(w.c), [ &[] ]) // I(1)
/// | |
/// v v
/// (Ty(w.p), [ &[x] ]) false
/// |
/// |
/// -------------------------------
/// | |
/// v v
/// (Ty((w.p).x), [ &[] ]) (Ty((w.p).y), []) // IMP 2
/// | |
/// v v
/// false NeedsDrop(Ty(w.p.y))
/// |
/// v
/// true
/// ```
///
/// IMP 1 `(Ty(w.c), [ &[] ])`: Notice the single empty slice inside `captured_projs`.
/// This implies that the `w.c` is completely captured by the closure.
/// Since drop for this path will be called when the closure is
/// dropped we don't need to migrate for it.
///
/// IMP 2 `(Ty((w.p).y), [])`: Notice that `captured_projs` is empty. This implies that this
/// path wasn't captured by the closure. Also note that even
/// though we didn't capture this path, the function visits it,
/// which is kind of the point of this function. We then return
/// if the type of `w.p.y` implements Drop, which in this case is
/// true.
///
/// Consider another example:
///
/// ```rust
/// struct X;
/// impl Drop for X {}
///
/// struct Y(X);
/// impl Drop for Y {}
///
/// fn foo() {
/// let y = Y(X);
/// let c = || move(y.0);
/// }
/// ```
///
/// Note that `y.0` is captured by the closure. When this function is called for `y`, it will
/// return true, because even though all paths starting at `y` are captured, `y` itself
/// implements Drop which will be affected since `y` isn't completely captured.
fn has_significant_drop_outside_of_captures(
&self,
closure_def_id: DefId,
closure_span: Span,
base_path_ty: Ty<'tcx>,
captured_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()))
};
let is_drop_defined_for_ty = |ty: Ty<'tcx>| {
let drop_trait = self.tcx.require_lang_item(hir::LangItem::Drop, Some(closure_span));
let ty_params = self.tcx.mk_substs_trait(base_path_ty, &[]);
self.tcx.type_implements_trait((
drop_trait,
ty,
ty_params,
self.tcx.param_env(closure_def_id.expect_local()),
))
};
let is_drop_defined_for_ty = is_drop_defined_for_ty(base_path_ty);
// If there is a case where no projection is applied on top of current place
// then there must be exactly one capture corresponding to such a case. Note that this
// represents the case of the path being completely captured by the variable.
//
// 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());
assert!(!is_completely_captured || (captured_projs.len() == 1));
if is_completely_captured {
// The place is captured entirely, so doesn't matter if needs dtor, it will be drop
// when the closure is dropped.
return false;
}
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;
}
if captured_projs.is_empty() {
return needs_drop(base_path_ty);
}
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.
// We don't capture derefs in case of move captures, which would have be applied to
// access any further paths.
ty::Adt(def, _) if def.is_box() => unreachable!(),
ty::Ref(..) => unreachable!(),
ty::RawPtr(..) => unreachable!(),
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`.
assert_eq!(def.variants.len(), 1);
// Only Field projections can be applied to a non-box Adt.
assert!(
captured_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
.iter()
.filter_map(|projs| {
if let ProjectionKind::Field(field_idx, _) =
projs.first().unwrap().kind
{
if (field_idx as usize) == i { Some(&projs[1..]) } else { None }
} else {
unreachable!();
}
})
.collect();
let after_field_ty = field.ty(self.tcx, substs);
self.has_significant_drop_outside_of_captures(
closure_def_id,
closure_span,
after_field_ty,
paths_using_field,
)
},
)
}
ty::Tuple(..) => {
// Only Field projections can be applied to a tuple.
assert!(
captured_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
.iter()
.filter_map(|projs| {
if let ProjectionKind::Field(field_idx, _) = projs.first().unwrap().kind
{
if (field_idx as usize) == i { Some(&projs[1..]) } else { None }
} else {
unreachable!();
}
})
.collect();
self.has_significant_drop_outside_of_captures(
closure_def_id,
closure_span,
element_ty,
paths_using_field,
)
})
}
// Anything else would be completely captured and therefore handled already.
_ => unreachable!(),
}
}
fn init_capture_kind(
&self,
capture_clause: hir::CaptureBy,

78
src/test/ui/closures/2229_closure_analysis/migrations/precise.rs Normal file
View File

@ -0,0 +1,78 @@
#![deny(disjoint_capture_drop_reorder)]
//~^ NOTE: the lint level is defined here
#[derive(Debug)]
struct Foo(i32);
impl Drop for Foo {
fn drop(&mut self) {
println!("{:?} dropped", self.0);
}
}
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));
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 = || {
//~^ERROR: drop order affected for closure because of `capture_disjoint_fields`
//~| NOTE: drop(&(u));
let _x = u.0.0;
let _x = u.0.1;
let _x = u.1.0;
};
c();
}
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();
}

49
src/test/ui/closures/2229_closure_analysis/migrations/precise.stderr Normal file
View File

@ -0,0 +1,49 @@
error: drop order affected for closure because of `capture_disjoint_fields`
--> $DIR/precise.rs:27:13
|
LL | let c = || {
| _____________^
LL | |
LL | |
LL | | let _t = t.0;
LL | | };
| |_____^
|
note: the lint level is defined here
--> $DIR/precise.rs:1: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
|
LL | let c = || {
| _____________^
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
|
LL | let c = || {
| _____________^
LL | |
LL | |
LL | | let _x = u.0.0;
LL | | let _x = u.0.1;
LL | | let _x = u.1.0;
LL | | };
| |_____^
|
= note: drop(&(u));
error: aborting due to 3 previous errors

105
src/test/ui/closures/2229_closure_analysis/migrations/precise_no_migrations.rs Normal file
View File

@ -0,0 +1,105 @@
// run-pass
#![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 if all paths starting at root variable that implement Drop are captured
// then it doesn't trigger the lint.
fn test_precise_analysis_simple_1() {
let t = (Foo(10), Foo(20), Foo(30));
let c = || {
let _t = t.0;
let _t = t.1;
let _t = t.2;
};
c();
}
// Test that if all paths starting at root variable that implement Drop are captured
// then it doesn't trigger the lint.
fn test_precise_analysis_simple_2() {
let t = ConstainsDropField(Foo(10), Foo(20));
let c = || {
let _t = t.0;
let _t = t.1;
};
c();
}
#[derive(Debug)]
struct ContainsAndImplsDrop(Foo);
impl Drop for ContainsAndImplsDrop {
fn drop(&mut self) {
println!("{:?} dropped", self.0);
}
}
// If a path isn't directly captured but requires Drop, then this tests that migrations aren't
// needed if the a parent to that path is captured.
fn test_precise_analysis_parent_captured_1() {
let t = ConstainsDropField(Foo(10), Foo(20));
let c = || {
let _t = t;
};
c();
}
// If a path isn't directly captured but requires Drop, then this tests that migrations aren't
// needed if the a parent to that path is captured.
fn test_precise_analysis_parent_captured_2() {
let t = ContainsAndImplsDrop(Foo(10));
let c = || {
let _t = t;
};
c();
}
struct S;
impl Drop for S {
fn drop(&mut self) {
}
}
struct T(S, S);
struct U(T, T);
// Test that if the path is longer than just one element, precise analysis works correctly.
fn test_precise_analysis_long_path() {
let u = U(T(S, S), T(S, S));
let c = || {
let _x = u.0.0;
let _x = u.0.1;
let _x = u.1.0;
let _x = u.1.1;
};
c();
}
fn main() {
test_precise_analysis_simple_1();
test_precise_analysis_simple_2();
test_precise_analysis_parent_captured_1();
test_precise_analysis_parent_captured_2();
test_precise_analysis_long_path();
}