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

Stop considering moved-out locals when computing auto traits for generators (rebased) #128846

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
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
72 changes: 55 additions & 17 deletions compiler/rustc_mir_transform/src/coroutine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,12 @@ use rustc_middle::mir::*;
use rustc_middle::ty::{self, CoroutineArgs, CoroutineArgsExt, InstanceKind, Ty, TyCtxt};
use rustc_middle::{bug, span_bug};
use rustc_mir_dataflow::impls::{
MaybeBorrowedLocals, MaybeLiveLocals, MaybeRequiresStorage, MaybeStorageLive,
MaybeBorrowedLocals, MaybeInitializedPlaces, MaybeLiveLocals, MaybeRequiresStorage,
MaybeStorageLive,
};
use rustc_mir_dataflow::move_paths::MoveData;
use rustc_mir_dataflow::storage::always_storage_live_locals;
use rustc_mir_dataflow::Analysis;
use rustc_mir_dataflow::{self, Analysis, MaybeReachable};
use rustc_span::def_id::{DefId, LocalDefId};
use rustc_span::symbol::sym;
use rustc_span::Span;
Expand Down Expand Up @@ -725,6 +727,10 @@ struct LivenessInfo {
/// Which locals are live across any suspension point.
saved_locals: CoroutineSavedLocals,

/// Which locals are live *and* initialized across any suspension point.
/// A local that is live but is not initialized does not need to accounted in auto trait checking.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your friendly neighborhood spell checker 😁

Suggested change
/// A local that is live but is not initialized does not need to accounted in auto trait checking.
/// A local that is live but is not initialized does not need to be accounted for in auto trait checking.

init_locals: BitSet<Local>,

/// The set of saved locals live at each suspension point.
live_locals_at_suspension_points: Vec<BitSet<CoroutineSavedLocal>>,

Expand Down Expand Up @@ -784,10 +790,20 @@ fn locals_live_across_suspend_points<'tcx>(
.iterate_to_fixpoint()
.into_results_cursor(body);

let param_env = tcx.param_env(body.source.def_id());
let move_data = MoveData::gather_moves(body, tcx, param_env, |_| true);

// Calculate the set of locals which are initialized
let mut inits = MaybeInitializedPlaces::new(tcx, body, &move_data)
.into_engine(tcx, body)
.iterate_to_fixpoint()
.into_results_cursor(body);

let mut storage_liveness_map = IndexVec::from_elem(None, &body.basic_blocks);
let mut live_locals_at_suspension_points = Vec::new();
let mut source_info_at_suspension_points = Vec::new();
let mut live_locals_at_any_suspension_point = BitSet::new_empty(body.local_decls.len());
let mut init_locals_at_any_suspension_point = BitSet::new_empty(body.local_decls.len());

for (block, data) in body.basic_blocks.iter_enumerated() {
if let TerminatorKind::Yield { .. } = data.terminator().kind {
Expand Down Expand Up @@ -826,12 +842,26 @@ fn locals_live_across_suspend_points<'tcx>(
// The coroutine argument is ignored.
live_locals.remove(SELF_ARG);

debug!("loc = {:?}, live_locals = {:?}", loc, live_locals);
inits.seek_to_block_end(block);
let mut init_locals: BitSet<_> = BitSet::new_empty(body.local_decls.len());
if let MaybeReachable::Reachable(bitset) = inits.get() {
for move_path_index in bitset.iter() {
if let Some(local) = move_data.move_paths[move_path_index].place.as_local() {
init_locals.insert(local);
}
}
}
init_locals.intersect(&live_locals);

debug!(
"loc = {:?}, live_locals = {:?}, init_locals = {:?}",
loc, live_locals, init_locals
);

// Add the locals live at this suspension point to the set of locals which live across
// any suspension points
live_locals_at_any_suspension_point.union(&live_locals);

init_locals_at_any_suspension_point.union(&init_locals);
live_locals_at_suspension_points.push(live_locals);
source_info_at_suspension_points.push(data.terminator().source_info);
}
Expand All @@ -856,6 +886,7 @@ fn locals_live_across_suspend_points<'tcx>(

LivenessInfo {
saved_locals,
init_locals: init_locals_at_any_suspension_point,
live_locals_at_suspension_points,
source_info_at_suspension_points,
storage_conflicts,
Expand Down Expand Up @@ -1030,6 +1061,7 @@ fn compute_layout<'tcx>(
) {
let LivenessInfo {
saved_locals,
init_locals,
live_locals_at_suspension_points,
source_info_at_suspension_points,
storage_conflicts,
Expand All @@ -1046,20 +1078,26 @@ fn compute_layout<'tcx>(
let decl = &body.local_decls[local];
debug!(?decl);

// Do not `assert_crate_local` here, as post-borrowck cleanup may have already cleared
// the information. This is alright, since `ignore_for_traits` is only relevant when
// this code runs on pre-cleanup MIR, and `ignore_for_traits = false` is the safer
// default.
let ignore_for_traits = match decl.local_info {
// Do not include raw pointers created from accessing `static` items, as those could
// well be re-created by another access to the same static.
ClearCrossCrate::Set(box LocalInfo::StaticRef { is_thread_local, .. }) => {
!is_thread_local
let ignore_for_traits = if !init_locals.contains(local) {
// If only the storage is required to be live, but local is not initialized, then we can
// ignore such type for auto trait purposes.
true
} else {
// Do not `assert_crate_local` here, as post-borrowck cleanup may have already cleared
// the information. This is alright, since `ignore_for_traits` is only relevant when
// this code runs on pre-cleanup MIR, and `ignore_for_traits = false` is the safer
// default.
match decl.local_info {
// Do not include raw pointers created from accessing `static` items, as those could
// well be re-created by another access to the same static.
ClearCrossCrate::Set(box LocalInfo::StaticRef { is_thread_local, .. }) => {
!is_thread_local
}
// Fake borrows are only read by fake reads, so do not have any reality in
// post-analysis MIR.
ClearCrossCrate::Set(box LocalInfo::FakeBorrow) => true,
_ => false,
}
// Fake borrows are only read by fake reads, so do not have any reality in
// post-analysis MIR.
ClearCrossCrate::Set(box LocalInfo::FakeBorrow) => true,
_ => false,
};
let decl =
CoroutineSavedTy { ty: decl.ty, source_info: decl.source_info, ignore_for_traits };
Expand Down
44 changes: 0 additions & 44 deletions tests/ui/async-await/drop-track-field-assign-nonsend.rs

This file was deleted.

23 changes: 0 additions & 23 deletions tests/ui/async-await/drop-track-field-assign-nonsend.stderr

This file was deleted.

43 changes: 0 additions & 43 deletions tests/ui/async-await/drop-track-field-assign.rs

This file was deleted.

11 changes: 4 additions & 7 deletions tests/ui/async-await/field-assign-nonsend.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,11 @@ LL | assert_send(agent.handle());
| ^^^^^^^^^^^^^^ future returned by `handle` is not `Send`
|
= help: within `impl Future<Output = ()>`, the trait `Send` is not implemented for `Rc<String>`, which is required by `impl Future<Output = ()>: Send`
note: future is not `Send` as this value is used across an await
--> $DIR/field-assign-nonsend.rs:20:39
note: captured value is not `Send` because `&mut` references cannot be sent unless their referent is `Send`
--> $DIR/field-assign-nonsend.rs:16:21
|
LL | let mut info = self.info_result.clone();
| -------- has type `InfoResult` which is not `Send`
...
LL | let _ = send_element(element).await;
| ^^^^^ await occurs here, with `mut info` maybe used later
LL | async fn handle(&mut self) {
| ^^^^^^^^^ has type `&mut Agent` which is not `Send`, because `Agent` is not `Send`
note: required by a bound in `assert_send`
--> $DIR/field-assign-nonsend.rs:37:19
|
Expand Down
23 changes: 23 additions & 0 deletions tests/ui/async-await/temp-borrow-nonsend.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
//@ check-pass
//@ edition:2021

use core::marker::PhantomData;

struct B(PhantomData<*const ()>);

fn do_sth(_: &B) {}

async fn foo() {}

async fn test() {
let b = B(PhantomData);
do_sth(&b);
drop(b);
foo().await;
}

fn assert_send<T: Send>(_: T) {}

fn main() {
assert_send(test());
}
Loading