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

Desugaring of drop and replace at MIR build #107844

Merged
merged 4 commits into from
Mar 5, 2023
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
26 changes: 26 additions & 0 deletions compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1467,6 +1467,32 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {

/// Reports StorageDeadOrDrop of `place` conflicts with `borrow`.
///
/// Depending on the origin of the StorageDeadOrDrop, this may be
/// reported as either a drop or an illegal mutation of a borrowed value.
/// The latter is preferred when the this is a drop triggered by a
/// reassignment, as it's more user friendly to report a problem with the
/// explicit assignment than the implicit drop.
#[instrument(level = "debug", skip(self))]
pub(crate) fn report_storage_dead_or_drop_of_borrowed(
&mut self,
location: Location,
place_span: (Place<'tcx>, Span),
borrow: &BorrowData<'tcx>,
) {
// It's sufficient to check the last desugaring as Replace is the last
// one to be applied.
if let Some(DesugaringKind::Replace) = place_span.1.desugaring_kind() {
self.report_illegal_mutation_of_borrowed(location, place_span, borrow)
} else {
self.report_borrowed_value_does_not_live_long_enough(
location,
borrow,
place_span,
Some(WriteKind::StorageDeadOrDrop),
)
}
}

/// This means that some data referenced by `borrow` needs to live
/// past the point where the StorageDeadOrDrop of `place` occurs.
/// This is usually interpreted as meaning that `place` has too
Expand Down
15 changes: 14 additions & 1 deletion compiler/rustc_borrowck/src/diagnostics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -925,7 +925,20 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
return OtherUse(use_span);
}

for stmt in &self.body[location.block].statements[location.statement_index + 1..] {
// drop and replace might have moved the assignment to the next block
let maybe_additional_statement =
if let TerminatorKind::Drop { target: drop_target, .. } =
self.body[location.block].terminator().kind
{
self.body[drop_target].statements.first()
} else {
None
};

let statements =
self.body[location.block].statements[location.statement_index + 1..].iter();

for stmt in statements.chain(maybe_additional_statement) {
if let StatementKind::Assign(box (_, Rvalue::Aggregate(kind, places))) = &stmt.kind {
let (&def_id, is_generator) = match kind {
box AggregateKind::Closure(def_id, _) => (def_id, false),
Expand Down
8 changes: 7 additions & 1 deletion compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -828,7 +828,13 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
let Some(hir::Node::Item(item)) = node else { return; };
let hir::ItemKind::Fn(.., body_id) = item.kind else { return; };
let body = self.infcx.tcx.hir().body(body_id);
let mut v = V { assign_span: span, err, ty, suggested: false };
let mut assign_span = span;
// Drop desugaring is done at MIR build so it's not in the HIR
if let Some(DesugaringKind::Replace) = span.desugaring_kind() {
assign_span.remove_mark();
}

let mut v = V { assign_span, err, ty, suggested: false };
v.visit_body(body);
if !v.suggested {
err.help(&format!(
Expand Down
7 changes: 1 addition & 6 deletions compiler/rustc_borrowck/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1185,12 +1185,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
this.buffer_error(err);
}
WriteKind::StorageDeadOrDrop => this
.report_borrowed_value_does_not_live_long_enough(
location,
borrow,
place_span,
Some(kind),
),
.report_storage_dead_or_drop_of_borrowed(location, place_span, borrow),
WriteKind::Mutate => {
this.report_illegal_mutation_of_borrowed(location, place_span, borrow)
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_build/src/build/expr/stmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// Generate better code for things that don't need to be
// dropped.
if lhs.ty.needs_drop(this.tcx, this.param_env) {
let rhs = unpack!(block = this.as_local_operand(block, rhs));
let rhs = unpack!(block = this.as_local_rvalue(block, rhs));
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a little surprising to me that this wasn't .as_local_rvalue already. Were we just less precise than we could be before?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh no, I guess this change is because build_drop_and_replace was changed to take an rvalue.

let lhs = unpack!(block = this.as_place(block, lhs));
unpack!(block = this.build_drop_and_replace(block, lhs_span, lhs, rhs));
} else {
Expand Down
32 changes: 25 additions & 7 deletions compiler/rustc_mir_build/src/build/scope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ use rustc_middle::middle::region;
use rustc_middle::mir::*;
use rustc_middle::thir::{Expr, LintLevel};

use rustc_span::{Span, DUMMY_SP};
use rustc_span::{DesugaringKind, Span, DUMMY_SP};

#[derive(Debug)]
pub struct Scopes<'tcx> {
Expand Down Expand Up @@ -1118,24 +1118,35 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}

/// Utility function for *non*-scope code to build their own drops
/// Force a drop at this point in the MIR by creating a new block.
pub(crate) fn build_drop_and_replace(
&mut self,
block: BasicBlock,
span: Span,
place: Place<'tcx>,
value: Operand<'tcx>,
value: Rvalue<'tcx>,
) -> BlockAnd<()> {
let span = self.tcx.with_stable_hashing_context(|hcx| {
span.mark_with_reason(None, DesugaringKind::Replace, self.tcx.sess.edition(), hcx)
});
let source_info = self.source_info(span);
let next_target = self.cfg.start_new_block();

// create the new block for the assignment
let assign = self.cfg.start_new_block();
self.cfg.push_assign(assign, source_info, place, value.clone());

// create the new block for the assignment in the case of unwinding
let assign_unwind = self.cfg.start_new_cleanup_block();
self.cfg.push_assign(assign_unwind, source_info, place, value.clone());
Copy link
Contributor

Choose a reason for hiding this comment

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

Having the assignment even in the unwind case is a bit surprising. This is the current behaviour, so ok. Should eventually be removed in the future?

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as drop can unwind, it is necessary to avoid leaving partially destroyed places behind. See #30380 for an example.


self.cfg.terminate(
block,
source_info,
TerminatorKind::DropAndReplace { place, value, target: next_target, unwind: None },
TerminatorKind::Drop { place, target: assign, unwind: Some(assign_unwind) },
);
self.diverge_from(block);

next_target.unit()
assign.unit()
}

/// Creates an `Assert` terminator and return the success block.
Expand Down Expand Up @@ -1413,8 +1424,15 @@ impl<'tcx> DropTreeBuilder<'tcx> for Unwind {
fn add_entry(cfg: &mut CFG<'tcx>, from: BasicBlock, to: BasicBlock) {
let term = &mut cfg.block_data_mut(from).terminator_mut();
match &mut term.kind {
TerminatorKind::Drop { unwind, .. }
| TerminatorKind::DropAndReplace { unwind, .. }
TerminatorKind::Drop { unwind, .. } => {
if let Some(unwind) = *unwind {
let source_info = term.source_info;
cfg.terminate(unwind, source_info, TerminatorKind::Goto { target: to });
} else {
*unwind = Some(to);
}
}
TerminatorKind::DropAndReplace { unwind, .. }
| TerminatorKind::FalseUnwind { unwind, .. }
| TerminatorKind::Call { cleanup: unwind, .. }
| TerminatorKind::Assert { cleanup: unwind, .. }
Expand Down
19 changes: 14 additions & 5 deletions compiler/rustc_mir_transform/src/elaborate_drops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use rustc_mir_dataflow::un_derefer::UnDerefer;
use rustc_mir_dataflow::MoveDataParamEnv;
use rustc_mir_dataflow::{on_all_children_bits, on_all_drop_children_bits};
use rustc_mir_dataflow::{Analysis, ResultsCursor};
use rustc_span::Span;
use rustc_span::{DesugaringKind, Span};
use rustc_target::abi::VariantIdx;
use std::fmt;

Expand Down Expand Up @@ -425,10 +425,19 @@ impl<'b, 'tcx> ElaborateDropsCtxt<'b, 'tcx> {
bb,
),
LookupResult::Parent(..) => {
self.tcx.sess.delay_span_bug(
terminator.source_info.span,
&format!("drop of untracked value {:?}", bb),
);
if !matches!(
terminator.source_info.span.desugaring_kind(),
Some(DesugaringKind::Replace),
) {
self.tcx.sess.delay_span_bug(
terminator.source_info.span,
&format!("drop of untracked value {:?}", bb),
);
}
// A drop and replace behind a pointer/array/whatever.
// The borrow checker requires that these locations are initialized before the assignment,
// so we just leave an unconditional drop.
assert!(!data.is_cleanup);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I have a clear idea of MIR looks like in that case. Do you have an example?

Copy link
Contributor Author

@zeegomo zeegomo Feb 23, 2023

Choose a reason for hiding this comment

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

fn replace(c: &mut String) {
    *c = "new".to_string();
}

fn main() {
    let mut c = "old".to_string();
    replace(&mut c);
}

the body of replace gets translated to something like:

bb1: {
        _2 = <something that builds the new string>;
        drop((*_1)) -> [return: bb3, unwind: bb2]; 
    }

bb2 (cleanup): {
        (*_1) = move _2;
        resume;    
}

bb3: {
        (*_1) = move _2;
        return;
    }

Under normal conditions, dropping something behind a pointer would be bad (drop((*_1))), but in a drop and replace it's fine because it's always initialized before any other use.
assert!(!data.is_cleanup) is there because a drop and replace is never emitted in the unwind path so it shouldn't be possible to hit this branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, should we assert that we will assign to *_1 starting both target and unwind bbs? Just to future-proof against changes to MIR building.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what the check for DesugaringKind::Replace is for.
Alternatively we could check for assignments, but it could be that some unrelated code is inserted before those that does not access that variable and is therefore valid but breaks the check.
For this reason I was kind of considering DesugaringKind::Replace as a guarantee that there will be an assignment later to the same place that is compliant with the semantics of a replace, without having to know exactly how the desugaring is done.
However, I don't see any reason why we should insert stuff between the drop and the assignment for now, so if you think it's safer that way it's quite easy to do. I 'm just worrying it might be a bit sensitive to reorderings in the MIR

}
}
}
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_span/src/hygiene.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1151,6 +1151,7 @@ pub enum DesugaringKind {
Await,
ForLoop,
WhileLoop,
Replace,
}

impl DesugaringKind {
Expand All @@ -1166,6 +1167,7 @@ impl DesugaringKind {
DesugaringKind::OpaqueTy => "`impl Trait`",
DesugaringKind::ForLoop => "`for` loop",
DesugaringKind::WhileLoop => "`while` loop",
DesugaringKind::Replace => "drop and replace",
}
}
}
Expand Down
85 changes: 85 additions & 0 deletions tests/mir-opt/basic_assignment.main.ElaborateDrops.diff
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
- // MIR for `main` before ElaborateDrops
+ // MIR for `main` after ElaborateDrops

fn main() -> () {
let mut _0: (); // return place in scope 0 at $DIR/basic_assignment.rs:+0:11: +0:11
let _1: bool; // in scope 0 at $DIR/basic_assignment.rs:+1:9: +1:17
let mut _3: bool; // in scope 0 at $DIR/basic_assignment.rs:+6:16: +6:24
let mut _6: std::option::Option<std::boxed::Box<u32>>; // in scope 0 at $DIR/basic_assignment.rs:+13:14: +13:20
scope 1 {
debug nodrop_x => _1; // in scope 1 at $DIR/basic_assignment.rs:+1:9: +1:17
let _2: bool; // in scope 1 at $DIR/basic_assignment.rs:+2:9: +2:17
scope 2 {
debug nodrop_y => _2; // in scope 2 at $DIR/basic_assignment.rs:+2:9: +2:17
let _4: std::option::Option<std::boxed::Box<u32>>; // in scope 2 at $DIR/basic_assignment.rs:+8:9: +8:15
scope 3 {
debug drop_x => _4; // in scope 3 at $DIR/basic_assignment.rs:+8:9: +8:15
let _5: std::option::Option<std::boxed::Box<u32>>; // in scope 3 at $DIR/basic_assignment.rs:+9:9: +9:15
scope 4 {
debug drop_y => _5; // in scope 4 at $DIR/basic_assignment.rs:+9:9: +9:15
}
}
}
}

bb0: {
StorageLive(_1); // scope 0 at $DIR/basic_assignment.rs:+1:9: +1:17
_1 = const false; // scope 0 at $DIR/basic_assignment.rs:+1:20: +1:25
StorageLive(_2); // scope 1 at $DIR/basic_assignment.rs:+2:9: +2:17
StorageLive(_3); // scope 2 at $DIR/basic_assignment.rs:+6:16: +6:24
_3 = _1; // scope 2 at $DIR/basic_assignment.rs:+6:16: +6:24
_2 = move _3; // scope 2 at $DIR/basic_assignment.rs:+6:5: +6:24
StorageDead(_3); // scope 2 at $DIR/basic_assignment.rs:+6:23: +6:24
StorageLive(_4); // scope 2 at $DIR/basic_assignment.rs:+8:9: +8:15
_4 = Option::<Box<u32>>::None; // scope 2 at $DIR/basic_assignment.rs:+8:36: +8:40
StorageLive(_5); // scope 3 at $DIR/basic_assignment.rs:+9:9: +9:15
StorageLive(_6); // scope 4 at $DIR/basic_assignment.rs:+13:14: +13:20
_6 = move _4; // scope 4 at $DIR/basic_assignment.rs:+13:14: +13:20
- drop(_5) -> [return: bb1, unwind: bb2]; // scope 4 at $DIR/basic_assignment.rs:+13:5: +13:11
+ goto -> bb1; // scope 4 at $DIR/basic_assignment.rs:+13:5: +13:11
}

bb1: {
_5 = move _6; // scope 4 at $DIR/basic_assignment.rs:+13:5: +13:11
- drop(_6) -> [return: bb3, unwind: bb6]; // scope 4 at $DIR/basic_assignment.rs:+13:19: +13:20
+ goto -> bb3; // scope 4 at $DIR/basic_assignment.rs:+13:19: +13:20
}

bb2 (cleanup): {
_5 = move _6; // scope 4 at $DIR/basic_assignment.rs:+13:5: +13:11
drop(_6) -> bb6; // scope 4 at $DIR/basic_assignment.rs:+13:19: +13:20
}

bb3: {
StorageDead(_6); // scope 4 at $DIR/basic_assignment.rs:+13:19: +13:20
_0 = const (); // scope 0 at $DIR/basic_assignment.rs:+0:11: +14:2
drop(_5) -> [return: bb4, unwind: bb7]; // scope 3 at $DIR/basic_assignment.rs:+14:1: +14:2
}

bb4: {
StorageDead(_5); // scope 3 at $DIR/basic_assignment.rs:+14:1: +14:2
- drop(_4) -> bb5; // scope 2 at $DIR/basic_assignment.rs:+14:1: +14:2
+ goto -> bb5; // scope 2 at $DIR/basic_assignment.rs:+14:1: +14:2
}

bb5: {
StorageDead(_4); // scope 2 at $DIR/basic_assignment.rs:+14:1: +14:2
StorageDead(_2); // scope 1 at $DIR/basic_assignment.rs:+14:1: +14:2
StorageDead(_1); // scope 0 at $DIR/basic_assignment.rs:+14:1: +14:2
return; // scope 0 at $DIR/basic_assignment.rs:+14:2: +14:2
}

bb6 (cleanup): {
drop(_5) -> bb7; // scope 3 at $DIR/basic_assignment.rs:+14:1: +14:2
}

bb7 (cleanup): {
- drop(_4) -> bb8; // scope 2 at $DIR/basic_assignment.rs:+14:1: +14:2
+ goto -> bb8; // scope 2 at $DIR/basic_assignment.rs:+14:1: +14:2
}

bb8 (cleanup): {
resume; // scope 0 at $DIR/basic_assignment.rs:+0:1: +14:2
}
}

28 changes: 15 additions & 13 deletions tests/mir-opt/basic_assignment.main.SimplifyCfg-initial.after.mir
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// MIR for `main` after SimplifyCfg-initial

| User Type Annotations
| 0: user_ty: Canonical { max_universe: U0, variables: [], value: Ty(std::option::Option<std::boxed::Box<u32>>) }, span: $DIR/basic_assignment.rs:18:17: 18:33, inferred_ty: std::option::Option<std::boxed::Box<u32>>
| 1: user_ty: Canonical { max_universe: U0, variables: [], value: Ty(std::option::Option<std::boxed::Box<u32>>) }, span: $DIR/basic_assignment.rs:18:17: 18:33, inferred_ty: std::option::Option<std::boxed::Box<u32>>
| 0: user_ty: Canonical { max_universe: U0, variables: [], value: Ty(std::option::Option<std::boxed::Box<u32>>) }, span: $DIR/basic_assignment.rs:20:17: 20:33, inferred_ty: std::option::Option<std::boxed::Box<u32>>
| 1: user_ty: Canonical { max_universe: U0, variables: [], value: Ty(std::option::Option<std::boxed::Box<u32>>) }, span: $DIR/basic_assignment.rs:20:17: 20:33, inferred_ty: std::option::Option<std::boxed::Box<u32>>
|
fn main() -> () {
let mut _0: (); // return place in scope 0 at $DIR/basic_assignment.rs:+0:11: +0:11
Expand Down Expand Up @@ -41,35 +41,37 @@ fn main() -> () {
StorageLive(_5); // scope 3 at $DIR/basic_assignment.rs:+9:9: +9:15
StorageLive(_6); // scope 4 at $DIR/basic_assignment.rs:+13:14: +13:20
_6 = move _4; // scope 4 at $DIR/basic_assignment.rs:+13:14: +13:20
replace(_5 <- move _6) -> [return: bb1, unwind: bb5]; // scope 4 at $DIR/basic_assignment.rs:+13:5: +13:11
drop(_5) -> [return: bb1, unwind: bb2]; // scope 4 at $DIR/basic_assignment.rs:+13:5: +13:11
zeegomo marked this conversation as resolved.
Show resolved Hide resolved
}

bb1: {
drop(_6) -> [return: bb2, unwind: bb6]; // scope 4 at $DIR/basic_assignment.rs:+13:19: +13:20
_5 = move _6; // scope 4 at $DIR/basic_assignment.rs:+13:5: +13:11
drop(_6) -> [return: bb3, unwind: bb6]; // scope 4 at $DIR/basic_assignment.rs:+13:19: +13:20
}

bb2: {
bb2 (cleanup): {
_5 = move _6; // scope 4 at $DIR/basic_assignment.rs:+13:5: +13:11
drop(_6) -> bb6; // scope 4 at $DIR/basic_assignment.rs:+13:19: +13:20
}

bb3: {
StorageDead(_6); // scope 4 at $DIR/basic_assignment.rs:+13:19: +13:20
_0 = const (); // scope 0 at $DIR/basic_assignment.rs:+0:11: +14:2
drop(_5) -> [return: bb3, unwind: bb7]; // scope 3 at $DIR/basic_assignment.rs:+14:1: +14:2
drop(_5) -> [return: bb4, unwind: bb7]; // scope 3 at $DIR/basic_assignment.rs:+14:1: +14:2
}

bb3: {
bb4: {
StorageDead(_5); // scope 3 at $DIR/basic_assignment.rs:+14:1: +14:2
drop(_4) -> [return: bb4, unwind: bb8]; // scope 2 at $DIR/basic_assignment.rs:+14:1: +14:2
drop(_4) -> [return: bb5, unwind: bb8]; // scope 2 at $DIR/basic_assignment.rs:+14:1: +14:2
}

bb4: {
bb5: {
StorageDead(_4); // scope 2 at $DIR/basic_assignment.rs:+14:1: +14:2
StorageDead(_2); // scope 1 at $DIR/basic_assignment.rs:+14:1: +14:2
StorageDead(_1); // scope 0 at $DIR/basic_assignment.rs:+14:1: +14:2
return; // scope 0 at $DIR/basic_assignment.rs:+14:2: +14:2
}

bb5 (cleanup): {
drop(_6) -> bb6; // scope 4 at $DIR/basic_assignment.rs:+13:19: +13:20
}

bb6 (cleanup): {
drop(_5) -> bb7; // scope 3 at $DIR/basic_assignment.rs:+14:1: +14:2
}
Expand Down
2 changes: 2 additions & 0 deletions tests/mir-opt/basic_assignment.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
// needs-unwind
// this tests move up progration, which is not yet implemented

// EMIT_MIR basic_assignment.main.ElaborateDrops.diff
// EMIT_MIR basic_assignment.main.SimplifyCfg-initial.after.mir

// Check codegen for assignments (`a = b`) where the left-hand-side is
Expand Down
Loading