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

2229: Drop any deref in move closure #88467

Merged
merged 1 commit into from
Aug 31, 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
25 changes: 7 additions & 18 deletions compiler/rustc_typeck/src/check/upvar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1896,31 +1896,20 @@ fn restrict_capture_precision<'tcx>(
return (place, curr_mode);
}

/// Take ownership if data being accessed is owned by the variable used to access it
/// (or if closure attempts to move data that it doesn’t own).
/// Note: When taking ownership, only capture data found on the stack.
/// Truncate deref of any reference.
fn adjust_for_move_closure<'tcx>(
mut place: Place<'tcx>,
mut kind: ty::UpvarCapture<'tcx>,
) -> (Place<'tcx>, ty::UpvarCapture<'tcx>) {
let contains_deref_of_ref = place.deref_tys().any(|ty| ty.is_ref());
let first_deref = place.projections.iter().position(|proj| proj.kind == ProjectionKind::Deref);

match kind {
ty::UpvarCapture::ByRef(..) if contains_deref_of_ref => (place, kind),

// If there's any Deref and the data needs to be moved into the closure body,
// or it's a Deref of a Box, truncate the path to the first deref
_ => {
if let Some(idx) = first_deref {
truncate_place_to_len_and_update_capture_kind(&mut place, &mut kind, idx);
}

// AMAN: I think we don't need the span inside the ByValue anymore
// we have more detailed span in CaptureInfo
(place, ty::UpvarCapture::ByValue(None))
}
if let Some(idx) = first_deref {
truncate_place_to_len_and_update_capture_kind(&mut place, &mut kind, idx);
}

// AMAN: I think we don't need the span inside the ByValue anymore
// we have more detailed span in CaptureInfo
(place, ty::UpvarCapture::ByValue(None))
}

/// Adjust closure capture just that if taking ownership of data, only move data
Expand Down
10 changes: 5 additions & 5 deletions src/test/ui/closures/2229_closure_analysis/move_closure.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ fn simple_ref() {
//~| ERROR: Min Capture analysis includes:
*ref_s += 10;
//~^ NOTE: Capturing ref_s[Deref] -> MutBorrow
//~| NOTE: Min Capture ref_s[Deref] -> MutBorrow
//~| NOTE: Min Capture ref_s[] -> ByValue
};
c();
}
Expand All @@ -56,7 +56,7 @@ fn struct_contains_ref_to_another_struct_1() {
//~| ERROR: Min Capture analysis includes:
t.0.0 = "new s".into();
//~^ NOTE: Capturing t[(0, 0),Deref,(0, 0)] -> MutBorrow
//~| NOTE: Min Capture t[(0, 0),Deref,(0, 0)] -> MutBorrow
//~| NOTE: Min Capture t[(0, 0)] -> ByValue
};

c();
Expand All @@ -79,7 +79,7 @@ fn struct_contains_ref_to_another_struct_2() {
//~| ERROR: Min Capture analysis includes:
let _t = t.0.0;
//~^ NOTE: Capturing t[(0, 0),Deref,(0, 0)] -> ImmBorrow
//~| NOTE: Min Capture t[(0, 0),Deref] -> ImmBorrow
//~| NOTE: Min Capture t[(0, 0)] -> ByValue
};

c();
Expand Down Expand Up @@ -175,7 +175,7 @@ fn box_mut_1() {
//~| First Pass analysis includes:
//~| NOTE: Capturing box_p_foo[Deref,Deref,(0, 0)] -> MutBorrow
//~| Min Capture analysis includes:
//~| NOTE: Min Capture box_p_foo[Deref,Deref,(0, 0)] -> MutBorrow
//~| NOTE: Min Capture box_p_foo[] -> ByValue
}

// Ensure that even in move closures, if the data is not owned by the root variable
Expand All @@ -192,7 +192,7 @@ fn box_mut_2() {
//~| First Pass analysis includes:
//~| NOTE: Capturing p_foo[Deref,Deref,(0, 0)] -> MutBorrow
//~| Min Capture analysis includes:
//~| NOTE: Min Capture p_foo[Deref,Deref,(0, 0)] -> MutBorrow
//~| NOTE: Min Capture p_foo[] -> ByValue
}

// Test that move closures can take ownership of Copy type
Expand Down
10 changes: 5 additions & 5 deletions src/test/ui/closures/2229_closure_analysis/move_closure.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ LL | |
LL | | };
| |_____^
|
note: Min Capture ref_s[Deref] -> MutBorrow
note: Min Capture ref_s[] -> ByValue
--> $DIR/move_closure.rs:36:9
|
LL | *ref_s += 10;
Expand Down Expand Up @@ -223,7 +223,7 @@ LL | |
LL | | };
| |_____^
|
note: Min Capture t[(0, 0),Deref,(0, 0)] -> MutBorrow
note: Min Capture t[(0, 0)] -> ByValue
--> $DIR/move_closure.rs:57:9
|
LL | t.0.0 = "new s".into();
Expand Down Expand Up @@ -259,7 +259,7 @@ LL | |
LL | | };
| |_____^
|
note: Min Capture t[(0, 0),Deref] -> ImmBorrow
note: Min Capture t[(0, 0)] -> ByValue
--> $DIR/move_closure.rs:80:18
|
LL | let _t = t.0.0;
Expand Down Expand Up @@ -427,7 +427,7 @@ error: Min Capture analysis includes:
LL | let c = #[rustc_capture_analysis] move || box_p_foo.x += 10;
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: Min Capture box_p_foo[Deref,Deref,(0, 0)] -> MutBorrow
note: Min Capture box_p_foo[] -> ByValue
--> $DIR/move_closure.rs:172:47
|
LL | let c = #[rustc_capture_analysis] move || box_p_foo.x += 10;
Expand All @@ -451,7 +451,7 @@ error: Min Capture analysis includes:
LL | let c = #[rustc_capture_analysis] move || p_foo.x += 10;
| ^^^^^^^^^^^^^^^^^^^^^
|
note: Min Capture p_foo[Deref,Deref,(0, 0)] -> MutBorrow
note: Min Capture p_foo[] -> ByValue
--> $DIR/move_closure.rs:189:47
|
LL | let c = #[rustc_capture_analysis] move || p_foo.x += 10;
Expand Down
59 changes: 59 additions & 0 deletions src/test/ui/closures/2229_closure_analysis/run_pass/issue-88431.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// edition:2021
// check-pass

use std::collections::HashMap;
use std::future::Future;
use std::pin::Pin;

pub struct GameMode {}

struct GameStateManager<'a> {
gamestate_stack: Vec<Box<dyn GameState<'a> + 'a>>,
}

pub trait GameState<'a> {}

async fn construct_gamestate_replay<'a>(
_gamemode: &GameMode,
_factory: &mut GameStateManager<'a>,
) -> Box<dyn GameState<'a> + 'a> {
unimplemented!()
}

type FutureGameState<'a, 'b> = Pin<Box<dyn Future<Output = Box<dyn GameState<'a> + 'a>> + 'b>>;

struct MenuOption<'a> {
command: Box<dyn for<'b> Fn(&'b mut GameStateManager<'a>) -> FutureGameState<'a, 'b> + 'a>,
}

impl<'a> MenuOption<'a> {
fn new(
_command: impl for<'b> Fn(&'b mut GameStateManager<'a>) -> FutureGameState<'a, 'b> + 'a,
) -> Self {
unimplemented!()
}
}

struct MenuState<'a> {
options: Vec<MenuOption<'a>>,
}

impl<'a> GameState<'a> for MenuState<'a> {}

pub async fn get_replay_menu<'a>(
gamemodes: &'a HashMap<&str, GameMode>,
) -> Box<dyn GameState<'a> + 'a> {
let recordings: Vec<String> = vec![];
let _ = recordings
.into_iter()
.map(|entry| {
MenuOption::new(move |f| {
Box::pin(construct_gamestate_replay(&gamemodes[entry.as_str()], f))
})
})
.collect::<Vec<_>>();

todo!()
}

fn main() {}
Original file line number Diff line number Diff line change
Expand Up @@ -56,28 +56,6 @@ fn no_ref_nested() {
c();
}

struct A<'a>(&'a mut String, &'a mut String);
// Test that reborrowing works as expected for move closures
// by attempting a disjoint capture through a reference.
fn disjoint_via_ref() {
let mut x = String::new();
let mut y = String::new();

let mut a = A(&mut x, &mut y);
let a = &mut a;

let mut c1 = move || {
a.0.truncate(0);
};

let mut c2 = move || {
a.1.truncate(0);
};

c1();
c2();
}

// Test that even if a path is moved into the closure, the closure is not FnOnce
// if the path is not moved by the closure call.
fn data_moved_but_not_fn_once() {
Expand Down Expand Up @@ -109,7 +87,6 @@ fn main() {
no_ref();
no_ref_nested();

disjoint_via_ref();
data_moved_but_not_fn_once();

returned_closure_owns_copy_type_data();
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/nll/closure-use-spans.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ LL | let y = &x;
LL | x = 0;
| ^^^^^ assignment to borrowed `x` occurs here
LL | move || *y;
| - borrow later captured here by closure
| -- borrow later captured here by closure

error: aborting due to 3 previous errors

Expand Down