Skip to content

Commit 8a392a5

Browse files
borsbors
committed
Auto merge of #98754 - jyn514:non-trivial-drop, r=compiler-errors
Fix drop-tracking ICE when a struct containing a field with a significant drop is used across an await Previously, drop-tracking would incorrectly assume the struct would be dropped immediately, which was not true. Fixes #98476. Also fixes #98477, I think because the parent HIR node for type variables is the whole function instead of the expression where the variable is used. r? `@eholk`
2 parents cbb07c2 + b30315d commit 8a392a5

11 files changed

+369
-35
lines changed

‎compiler/rustc_typeck/src/check/generator_interior.rs

+25-3
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ use rustc_hir::hir_id::HirIdSet;
1414
use rustc_hir::intravisit::{self, Visitor};
1515
use rustc_hir::{Arm, Expr, ExprKind, Guard, HirId, Pat, PatKind};
1616
use rustc_middle::middle::region::{self, Scope, ScopeData, YieldData};
17-
use rustc_middle::ty::{self, RvalueScopes, Ty, TyCtxt};
17+
use rustc_middle::ty::{self, RvalueScopes, Ty, TyCtxt, TypeVisitable};
1818
use rustc_span::symbol::sym;
1919
use rustc_span::Span;
2020
use tracing::debug;
@@ -376,6 +376,17 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> {
376376

377377
debug!("is_borrowed_temporary: {:?}", self.drop_ranges.is_borrowed_temporary(expr));
378378

379+
let ty = self.fcx.typeck_results.borrow().expr_ty_adjusted_opt(expr);
380+
let may_need_drop = |ty: Ty<'tcx>| {
381+
// Avoid ICEs in needs_drop.
382+
let ty = self.fcx.resolve_vars_if_possible(ty);
383+
let ty = self.fcx.tcx.erase_regions(ty);
384+
if ty.needs_infer() {
385+
return true;
386+
}
387+
ty.needs_drop(self.fcx.tcx, self.fcx.param_env)
388+
};
389+
379390
// Typically, the value produced by an expression is consumed by its parent in some way,
380391
// so we only have to check if the parent contains a yield (note that the parent may, for
381392
// example, store the value into a local variable, but then we already consider local
@@ -384,7 +395,18 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> {
384395
// However, in the case of temporary values, we are going to store the value into a
385396
// temporary on the stack that is live for the current temporary scope and then return a
386397
// reference to it. That value may be live across the entire temporary scope.
387-
let scope = if self.drop_ranges.is_borrowed_temporary(expr) {
398+
//
399+
// There's another subtlety: if the type has an observable drop, it must be dropped after
400+
// the yield, even if it's not borrowed or referenced after the yield. Ideally this would
401+
// *only* happen for types with observable drop, not all types which wrap them, but that
402+
// doesn't match the behavior of MIR borrowck and causes ICEs. See the FIXME comment in
403+
// src/test/ui/generator/drop-tracking-parent-expression.rs.
404+
let scope = if self.drop_ranges.is_borrowed_temporary(expr)
405+
|| ty.map_or(true, |ty| {
406+
let needs_drop = may_need_drop(ty);
407+
debug!(?needs_drop, ?ty);
408+
needs_drop
409+
}) {
388410
self.rvalue_scopes.temporary_scope(self.region_scope_tree, expr.hir_id.local_id)
389411
} else {
390412
debug!("parent_node: {:?}", self.fcx.tcx.hir().find_parent_node(expr.hir_id));
@@ -398,7 +420,7 @@ impl<'a, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'tcx> {
398420

399421
// If there are adjustments, then record the final type --
400422
// this is the actual value that is being produced.
401-
if let Some(adjusted_ty) = self.fcx.typeck_results.borrow().expr_ty_adjusted_opt(expr) {
423+
if let Some(adjusted_ty) = ty {
402424
self.record(adjusted_ty, expr.hir_id, scope, Some(expr), expr.span);
403425
}
404426

‎src/test/ui/async-await/default-struct-update.rs

+22
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
// build-pass
2+
// edition:2018
3+
// compile-flags: -Zdrop-tracking=y
4+
5+
fn main() {
6+
let _ = foo();
7+
}
8+
9+
async fn from_config(_: Config) {}
10+
11+
async fn foo() {
12+
from_config(Config {
13+
nickname: None,
14+
..Default::default()
15+
})
16+
.await;
17+
}
18+
19+
#[derive(Default)]
20+
struct Config {
21+
nickname: Option<Box<u8>>,
22+
}

‎src/test/ui/async-await/issue-70935-complex-spans.drop_tracking.stderr

+3-5
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
error[E0277]: `Sender<i32>` cannot be shared between threads safely
2-
--> $DIR/issue-70935-complex-spans.rs:13:45
2+
--> $DIR/issue-70935-complex-spans.rs:12:45
33
|
44
LL | fn foo(tx: std::sync::mpsc::Sender<i32>) -> impl Future + Send {
55
| ^^^^^^^^^^^^^^^^^^ `Sender<i32>` cannot be shared between threads safely
66
|
77
= help: the trait `Sync` is not implemented for `Sender<i32>`
88
= note: required because of the requirements on the impl of `Send` for `&Sender<i32>`
99
note: required because it's used within this closure
10-
--> $DIR/issue-70935-complex-spans.rs:25:13
10+
--> $DIR/issue-70935-complex-spans.rs:16:13
1111
|
1212
LL | baz(|| async{
1313
| ^^
@@ -16,16 +16,14 @@ note: required because it's used within this `async fn` body
1616
|
1717
LL | async fn baz<T>(_c: impl FnMut() -> T) where T: Future<Output=()> {
1818
| ___________________________________________________________________^
19-
LL | |
2019
LL | | }
2120
| |_^
2221
= note: required because it captures the following types: `ResumeTy`, `impl for<'r, 's, 't0> Future<Output = ()>`, `()`
2322
note: required because it's used within this `async` block
24-
--> $DIR/issue-70935-complex-spans.rs:23:16
23+
--> $DIR/issue-70935-complex-spans.rs:15:16
2524
|
2625
LL | async move {
2726
| ________________^
28-
LL | |
2927
LL | | baz(|| async{
3028
LL | | foo(tx.clone());
3129
LL | | }).await;

‎src/test/ui/async-await/issue-70935-complex-spans.normal.stderr

+4-4
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,22 @@
11
error: future cannot be sent between threads safely
2-
--> $DIR/issue-70935-complex-spans.rs:13:45
2+
--> $DIR/issue-70935-complex-spans.rs:12:45
33
|
44
LL | fn foo(tx: std::sync::mpsc::Sender<i32>) -> impl Future + Send {
55
| ^^^^^^^^^^^^^^^^^^ future created by async block is not `Send`
66
|
77
= help: the trait `Sync` is not implemented for `Sender<i32>`
88
note: future is not `Send` as this value is used across an await
9-
--> $DIR/issue-70935-complex-spans.rs:27:11
9+
--> $DIR/issue-70935-complex-spans.rs:18:11
1010
|
1111
LL | baz(|| async{
1212
| _____________-
1313
LL | | foo(tx.clone());
1414
LL | | }).await;
1515
| | - ^^^^^^ await occurs here, with the value maybe used later
1616
| |_________|
17-
| has type `[closure@$DIR/issue-70935-complex-spans.rs:25:13: 25:15]` which is not `Send`
17+
| has type `[closure@$DIR/issue-70935-complex-spans.rs:16:13: 16:15]` which is not `Send`
1818
note: the value is later dropped here
19-
--> $DIR/issue-70935-complex-spans.rs:27:17
19+
--> $DIR/issue-70935-complex-spans.rs:18:17
2020
|
2121
LL | }).await;
2222
| ^

‎src/test/ui/async-await/issue-70935-complex-spans.rs

+3-12
Original file line numberDiff line numberDiff line change
@@ -7,22 +7,13 @@
77
use std::future::Future;
88

99
async fn baz<T>(_c: impl FnMut() -> T) where T: Future<Output=()> {
10-
//[drop_tracking]~^ within this `async fn` body
1110
}
1211

1312
fn foo(tx: std::sync::mpsc::Sender<i32>) -> impl Future + Send {
14-
//[normal]~^ ERROR: future cannot be sent between threads safely
15-
//[drop_tracking]~^^ ERROR: `Sender<i32>` cannot be shared
16-
//[drop_tracking]~| NOTE: cannot be shared
17-
//[drop_tracking]~| NOTE: requirements on the impl of `Send`
18-
//[drop_tracking]~| NOTE: captures the following types
19-
//[drop_tracking]~| NOTE: in this expansion
20-
//[drop_tracking]~| NOTE: in this expansion
21-
//[drop_tracking]~| NOTE: in this expansion
22-
//[drop_tracking]~| NOTE: in this expansion
13+
//[normal]~^ ERROR future cannot be sent between threads safely
14+
//[drop_tracking]~^^ ERROR `Sender<i32>` cannot be shared between threads
2315
async move {
24-
//[drop_tracking]~^ within this `async` block
25-
baz(|| async{ //[drop_tracking]~ NOTE: used within this closure
16+
baz(|| async{
2617
foo(tx.clone());
2718
}).await;
2819
}

‎src/test/ui/async-await/non-trivial-drop.rs

+36
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
// build-pass
2+
// edition:2018
3+
// compile-flags: -Zdrop-tracking=y
4+
5+
#![feature(generators)]
6+
7+
fn main() {
8+
let _ = foo();
9+
}
10+
11+
fn foo() {
12+
|| {
13+
yield drop(Config {
14+
nickname: NonCopy,
15+
b: NonCopy2,
16+
}.nickname);
17+
};
18+
}
19+
20+
#[derive(Default)]
21+
struct NonCopy;
22+
impl Drop for NonCopy {
23+
fn drop(&mut self) {}
24+
}
25+
26+
#[derive(Default)]
27+
struct NonCopy2;
28+
impl Drop for NonCopy2 {
29+
fn drop(&mut self) {}
30+
}
31+
32+
#[derive(Default)]
33+
struct Config {
34+
nickname: NonCopy,
35+
b: NonCopy2,
36+
}

‎src/test/ui/async-await/type-parameter-send.rs

+18
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
// check-pass
2+
// compile-flags: --crate-type lib
3+
// edition:2018
4+
5+
fn assert_send<F: Send>(_: F) {}
6+
7+
async fn __post<T>() -> T {
8+
if false {
9+
todo!()
10+
} else {
11+
async {}.await;
12+
todo!()
13+
}
14+
}
15+
16+
fn foo<T>() {
17+
assert_send(__post::<T>());
18+
}

‎src/test/ui/generator/derived-drop-parent-expr.rs

+17
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
// build-pass
2+
// compile-flags:-Zdrop-tracking
3+
4+
//! Like drop-tracking-parent-expression, but also tests that this doesn't ICE when building MIR
5+
#![feature(generators)]
6+
7+
fn assert_send<T: Send>(_thing: T) {}
8+
9+
#[derive(Default)]
10+
pub struct Client { pub nickname: String }
11+
12+
fn main() {
13+
let g = move || match drop(Client { ..Client::default() }) {
14+
_status => yield,
15+
};
16+
assert_send(g);
17+
}

‎src/test/ui/generator/drop-tracking-parent-expression.rs

+69
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
// compile-flags: -Zdrop-tracking
2+
#![feature(generators, negative_impls, rustc_attrs)]
3+
4+
macro_rules! type_combinations {
5+
(
6+
$( $name:ident => { $( $tt:tt )* } );* $(;)?
7+
) => { $(
8+
mod $name {
9+
$( $tt )*
10+
11+
impl !Sync for Client {}
12+
impl !Send for Client {}
13+
}
14+
15+
// Struct update syntax. This fails because the Client used in the update is considered
16+
// dropped *after* the yield.
17+
{
18+
let g = move || match drop($name::Client { ..$name::Client::default() }) {
19+
//~^ `significant_drop::Client` which is not `Send`
20+
//~| `insignificant_dtor::Client` which is not `Send`
21+
//~| `derived_drop::Client` which is not `Send`
22+
_ => yield,
23+
};
24+
assert_send(g);
25+
//~^ ERROR cannot be sent between threads
26+
//~| ERROR cannot be sent between threads
27+
//~| ERROR cannot be sent between threads
28+
}
29+
30+
// Simple owned value. This works because the Client is considered moved into `drop`,
31+
// even though the temporary expression doesn't end until after the yield.
32+
{
33+
let g = move || match drop($name::Client::default()) {
34+
_ => yield,
35+
};
36+
assert_send(g);
37+
}
38+
)* }
39+
}
40+
41+
fn assert_send<T: Send>(_thing: T) {}
42+
43+
fn main() {
44+
type_combinations!(
45+
// OK
46+
copy => { #[derive(Copy, Clone, Default)] pub struct Client; };
47+
// NOT OK: MIR borrowck thinks that this is used after the yield, even though
48+
// this has no `Drop` impl and only the drops of the fields are observable.
49+
// FIXME: this should compile.
50+
derived_drop => { #[derive(Default)] pub struct Client { pub nickname: String } };
51+
// NOT OK
52+
significant_drop => {
53+
#[derive(Default)]
54+
pub struct Client;
55+
impl Drop for Client {
56+
fn drop(&mut self) {}
57+
}
58+
};
59+
// NOT OK (we need to agree with MIR borrowck)
60+
insignificant_dtor => {
61+
#[derive(Default)]
62+
#[rustc_insignificant_dtor]
63+
pub struct Client;
64+
impl Drop for Client {
65+
fn drop(&mut self) {}
66+
}
67+
};
68+
);
69+
}

0 commit comments

Comments
 (0)