2229: Capture box completely in move closures #86445

arora-aman · 2021-06-18T20:44:53Z

Even if the content from box is used in a sharef-ref context,
we capture the box entirerly.

This is motivated by:

We only capture data that is on the stack.
Capturing data from within the box might end up moving more data than
the user anticipated.

nikomatsakis · 2021-06-21T14:48:28Z

compiler/rustc_typeck/src/check/upvar.rs

@@ -1630,7 +1630,14 @@ impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> {
            self.fcx.param_env,
            &place_with_id.place,
        );
+
+        let place = restrict_preicision_for_box(&place, self.capture_clause);


Suggested change

let place = restrict_preicision_for_box(&place, self.capture_clause);

let place = restrict_precision_for_box(&place, self.capture_clause);

I'm a litle bit surprised, though, to see this call here, in borrow. Doesn't this apply no matter what type of access we have, so long as we are in a move closure? I think I would expect some common helper routine invoked by consume and borrow to do this (along with other forms of "restrict").

nikomatsakis · 2021-06-21T14:48:38Z

compiler/rustc_typeck/src/check/upvar.rs

+//   2. One motivatiton for the user to use a box might be to reduce the amount of data that gets
+//      moved (if size of pointer < size of data). We want to make sure that this optimization that
+//      the user made is respected.
+fn restrict_preicision_for_box(place: &Place<'tcx>, capture_by: hir::CaptureBy) -> Place<'tcx> {


Suggested change

fn restrict_preicision_for_box(place: &Place<'tcx>, capture_by: hir::CaptureBy) -> Place<'tcx> {

fn restrict_precision_for_box(place: &Place<'tcx>, capture_by: hir::CaptureBy) -> Place<'tcx> {

nikomatsakis · 2021-06-21T14:48:51Z

compiler/rustc_typeck/src/check/upvar.rs

@@ -1654,6 +1661,34 @@ impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> {
    }
 }

+// In case of move closures we don't want to capture derefs on a box.


Suggested change

// In case of move closures we don't want to capture derefs on a box.

// In the case of move closures, we don't want to capture derefs on a box.

nikomatsakis · 2021-06-21T14:49:24Z

compiler/rustc_typeck/src/check/upvar.rs

+// In case of move closures we don't want to capture derefs on a box.
+// This is motivated by:
+//   1. We only want to capture data that is on the stack
+//   2. One motivatiton for the user to use a box might be to reduce the amount of data that gets


Suggested change

// 2. One motivatiton for the user to use a box might be to reduce the amount of data that gets

// 2. One motivation for the user to use a box might be to reduce the amount of data that gets

nikomatsakis · 2021-06-21T14:50:49Z

compiler/rustc_typeck/src/check/upvar.rs

+//      moved (if size of pointer < size of data). We want to make sure that this optimization that
+//      the user made is respected.
+fn restrict_preicision_for_box(place: &Place<'tcx>, capture_by: hir::CaptureBy) -> Place<'tcx> {
+    let mut rv = place.clone();


Suggested change

let mut rv = place.clone();

nikomatsakis · 2021-06-21T14:50:56Z

compiler/rustc_typeck/src/check/upvar.rs

+fn restrict_preicision_for_box(place: &Place<'tcx>, capture_by: hir::CaptureBy) -> Place<'tcx> {
+    let mut rv = place.clone();
+    match capture_by {
+        hir::CaptureBy::Ref => rv,


Suggested change

hir::CaptureBy::Ref => rv,

hir::CaptureBy::Ref => place.clone(),

nikomatsakis · 2021-06-21T14:51:16Z

compiler/rustc_typeck/src/check/upvar.rs

+        hir::CaptureBy::Ref => rv,
+        hir::CaptureBy::Value => {
+            if ty::TyS::is_box(place.base_ty) {
+                Place { projections: Vec::new(), ..rv }


Suggested change

Place { projections: Vec::new(), ..rv }

Place { projections: Vec::new(), ..place.clone() }

nikomatsakis · 2021-06-21T14:52:02Z

compiler/rustc_typeck/src/check/upvar.rs

+                // In either case we want to stop at the box.
+                let pos = place.projections.iter().position(|proj| ty::TyS::is_box(proj.ty));
+                match pos {
+                    None => rv,


Suggested change

None => rv,

None => place.clone(),

nikomatsakis · 2021-06-21T14:52:27Z

compiler/rustc_typeck/src/check/upvar.rs

+                match pos {
+                    None => rv,
+                    Some(idx) => {
+                        Place { projections: rv.projections.drain(0..=idx).collect(), ..rv }


Suggested change

Place { projections: rv.projections.drain(0..=idx).collect(), ..rv }

let mut rv = place.clone();

Place { projections: rv.projections.drain(0..=idx).collect(), ..rv }

This could be more efficient, but good enough I guess.

nikomatsakis · 2021-06-23T19:21:27Z

@bors r+

bors · 2021-06-23T19:21:28Z

📌 Commit 757add16b06fbd8088d8ff5f85505f780f58e4fb has been approved by nikomatsakis

bors · 2021-06-24T07:56:02Z

☔ The latest upstream changes (presumably #86588) made this pull request unmergeable. Please resolve the merge conflicts.

Even if the content from box is used in a sharef-ref context, we capture the box entirerly. This is motivated by: 1) We only capture data that is on the stack. 2) Capturing data from within the box might end up moving more data than the user anticipated.

fixup! 2229: Capture box completely in move closures

arora-aman · 2021-06-25T01:47:54Z

@nikomatsakis udated

m-ou-se · 2021-06-27T16:06:58Z

@bors r=nikomatsakis

bors · 2021-06-27T16:06:59Z

📌 Commit d37a07f has been approved by nikomatsakis

bors · 2021-06-27T18:10:40Z

⌛ Testing commit d37a07f with merge a4f832b...

bors · 2021-06-27T20:33:20Z

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing a4f832b to master...

rust-highfive assigned nikomatsakis Jun 18, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 18, 2021

nikomatsakis requested changes Jun 21, 2021

View reviewed changes

arora-aman force-pushed the box_fix branch from d663bb3 to 757add1 Compare June 22, 2021 06:17

nikomatsakis approved these changes Jun 23, 2021

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 23, 2021

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 24, 2021

arora-aman added 2 commits June 24, 2021 19:39

fixup! 2229: Capture box completely in move closures

d37a07f

fixup! 2229: Capture box completely in move closures

arora-aman force-pushed the box_fix branch from 757add1 to d37a07f Compare June 25, 2021 01:47

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 27, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 27, 2021

bors merged commit a4f832b into rust-lang:master Jun 27, 2021

rustbot added this to the 1.55.0 milestone Jun 27, 2021

bors mentioned this pull request Jun 27, 2021

Editon 2021 enables precise capture #86661

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2229: Capture box completely in move closures #86445

2229: Capture box completely in move closures #86445

arora-aman commented Jun 18, 2021

nikomatsakis Jun 21, 2021

nikomatsakis Jun 21, 2021

nikomatsakis Jun 21, 2021

nikomatsakis Jun 21, 2021

nikomatsakis Jun 21, 2021

nikomatsakis Jun 21, 2021

nikomatsakis Jun 21, 2021

nikomatsakis Jun 21, 2021

nikomatsakis Jun 21, 2021

nikomatsakis Jun 21, 2021

nikomatsakis commented Jun 23, 2021

bors commented Jun 23, 2021

bors commented Jun 24, 2021

arora-aman commented Jun 25, 2021

m-ou-se commented Jun 27, 2021

bors commented Jun 27, 2021

bors commented Jun 27, 2021

bors commented Jun 27, 2021

	let place = restrict_preicision_for_box(&place, self.capture_clause);
	let place = restrict_precision_for_box(&place, self.capture_clause);

	fn restrict_preicision_for_box(place: &Place<'tcx>, capture_by: hir::CaptureBy) -> Place<'tcx> {
	fn restrict_precision_for_box(place: &Place<'tcx>, capture_by: hir::CaptureBy) -> Place<'tcx> {

	// In case of move closures we don't want to capture derefs on a box.
	// In the case of move closures, we don't want to capture derefs on a box.

	// 2. One motivatiton for the user to use a box might be to reduce the amount of data that gets
	// 2. One motivation for the user to use a box might be to reduce the amount of data that gets

	hir::CaptureBy::Ref => rv,
	hir::CaptureBy::Ref => place.clone(),

	Place { projections: Vec::new(), ..rv }
	Place { projections: Vec::new(), ..place.clone() }

	Place { projections: rv.projections.drain(0..=idx).collect(), ..rv }
	let mut rv = place.clone();
	Place { projections: rv.projections.drain(0..=idx).collect(), ..rv }

2229: Capture box completely in move closures #86445

2229: Capture box completely in move closures #86445

Conversation

arora-aman commented Jun 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Jun 23, 2021

bors commented Jun 23, 2021

bors commented Jun 24, 2021

arora-aman commented Jun 25, 2021

m-ou-se commented Jun 27, 2021

bors commented Jun 27, 2021

bors commented Jun 27, 2021

bors commented Jun 27, 2021