Skip to content

Commit 02d2594

Browse files
zeegomozeegomo
committed
POC: Move drop elaboration before borrowck
POC to show what's needed to move drop elaboration before borrow checking. Ideally we could have it right aftr MIR build, but this tries to be as minimal as possible. There are a few hacks, mostly around drop-elaboration code that does not pass borrowck / move analysis. I plan to work on the remaining failing tests in the near future.
1 parent 6eb9f2d commit 02d2594

File tree

22 files changed

+222
-91
lines changed

22 files changed

+222
-91
lines changed

‎compiler/rustc_borrowck/src/def_use.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ pub fn categorize(context: PlaceContext) -> Option<DefUse> {
5454

5555
PlaceContext::MutatingUse(MutatingUseContext::AddressOf) |
5656
PlaceContext::NonMutatingUse(NonMutatingUseContext::AddressOf) |
57-
PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect) |
57+
PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect(_)) |
5858
PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy) |
5959
PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) |
6060
PlaceContext::NonUse(NonUseContext::AscribeUserTy) |

‎compiler/rustc_borrowck/src/diagnostics/mod.rs

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -939,7 +939,22 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
939939
return OtherUse(use_span);
940940
}
941941

942-
for stmt in &self.body[location.block].statements[location.statement_index + 1..] {
942+
// drop and replace might have moved the assignment to the next block
943+
let maybe_additional_statement = if let Some(Terminator {
944+
kind: TerminatorKind::Drop { target: drop_target, .. },
945+
..
946+
}) = self.body[location.block].terminator
947+
{
948+
self.body[drop_target].statements.iter().take(1)
949+
} else {
950+
[].iter().take(0)
951+
};
952+
953+
let statements = self.body[location.block].statements[location.statement_index + 1..]
954+
.iter()
955+
.chain(maybe_additional_statement);
956+
957+
for stmt in statements {
943958
if let StatementKind::Assign(box (_, Rvalue::Aggregate(kind, places))) = &stmt.kind {
944959
let (&def_id, is_generator) = match kind {
945960
box AggregateKind::Closure(def_id, _) => (def_id, false),

‎compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -828,7 +828,13 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
828828
let Some(hir::Node::Item(item)) = node else { return; };
829829
let hir::ItemKind::Fn(.., body_id) = item.kind else { return; };
830830
let body = self.infcx.tcx.hir().body(body_id);
831-
let mut v = V { assign_span: span, err, ty, suggested: false };
831+
let mut assign_span = span;
832+
// Drop desugaring is done at MIR build so it's not in the HIR
833+
if let Some(DesugaringKind::Replace) = span.desugaring_kind() {
834+
assign_span.remove_mark();
835+
}
836+
837+
let mut v = V { assign_span, err, ty, suggested: false };
832838
v.visit_body(body);
833839
if !v.suggested {
834840
err.help(&format!(

‎compiler/rustc_borrowck/src/invalidation.rs

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,9 @@ impl<'cx, 'tcx> Visitor<'tcx> for InvalidationGenerator<'cx, 'tcx> {
5858

5959
match &statement.kind {
6060
StatementKind::Assign(box (lhs, rhs)) => {
61-
self.consume_rvalue(location, rhs);
61+
if !matches!(rhs, Rvalue::Discriminant(_)) {
62+
self.consume_rvalue(location, rhs);
63+
}
6264

6365
self.mutate_place(location, *lhs, Shallow(None));
6466
}
@@ -119,13 +121,12 @@ impl<'cx, 'tcx> Visitor<'tcx> for InvalidationGenerator<'cx, 'tcx> {
119121
);
120122
}
121123
TerminatorKind::DropAndReplace {
122-
place: drop_place,
123-
value: new_value,
124+
place: _drop_place,
125+
value: _new_value,
124126
target: _,
125127
unwind: _,
126128
} => {
127-
self.mutate_place(location, *drop_place, Deep);
128-
self.consume_operand(location, new_value);
129+
bug!("undesugared drop and replace in borrowck")
129130
}
130131
TerminatorKind::Call {
131132
func,

‎compiler/rustc_borrowck/src/lib.rs

Lines changed: 37 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -576,7 +576,11 @@ impl<'cx, 'tcx> rustc_mir_dataflow::ResultsVisitor<'cx, 'tcx> for MirBorrowckCtx
576576

577577
match &stmt.kind {
578578
StatementKind::Assign(box (lhs, rhs)) => {
579-
self.consume_rvalue(location, (rhs, span), flow_state);
579+
// FIXME: drop-elaboration checks the discriminant of an enum after it has been
580+
// moved out. This is a known isses and should be fixed in the future.
581+
if !matches!(rhs, Rvalue::Discriminant(_)) {
582+
self.consume_rvalue(location, (rhs, span), flow_state);
583+
}
580584

581585
self.mutate_place(location, (*lhs, span), Shallow(None), flow_state);
582586
}
@@ -661,13 +665,12 @@ impl<'cx, 'tcx> rustc_mir_dataflow::ResultsVisitor<'cx, 'tcx> for MirBorrowckCtx
661665
);
662666
}
663667
TerminatorKind::DropAndReplace {
664-
place: drop_place,
665-
value: new_value,
668+
place: _drop_place,
669+
value: _new_value,
666670
target: _,
667671
unwind: _,
668672
} => {
669-
self.mutate_place(loc, (*drop_place, span), Deep, flow_state);
670-
self.consume_operand(loc, (new_value, span), flow_state);
673+
bug!("undesugared drop and replace in borrowck")
671674
}
672675
TerminatorKind::Call {
673676
func,
@@ -678,11 +681,22 @@ impl<'cx, 'tcx> rustc_mir_dataflow::ResultsVisitor<'cx, 'tcx> for MirBorrowckCtx
678681
from_hir_call: _,
679682
fn_span: _,
680683
} => {
684+
self.mutate_place(loc, (*destination, span), Deep, flow_state);
681685
self.consume_operand(loc, (func, span), flow_state);
686+
687+
// drop glue for box does not pass borrowck
688+
let func_ty = func.ty(self.body, self.infcx.tcx);
689+
use rustc_hir::lang_items::LangItem;
690+
if let ty::FnDef(func_id, _) = func_ty.kind() {
691+
if Some(func_id) == self.infcx.tcx.lang_items().get(LangItem::BoxFree).as_ref()
692+
{
693+
return;
694+
}
695+
}
696+
682697
for arg in args {
683698
self.consume_operand(loc, (arg, span), flow_state);
684699
}
685-
self.mutate_place(loc, (*destination, span), Deep, flow_state);
686700
}
687701
TerminatorKind::Assert { cond, expected: _, msg, target: _, cleanup: _ } => {
688702
self.consume_operand(loc, (cond, span), flow_state);
@@ -1100,13 +1114,23 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
11001114
this.report_conflicting_borrow(location, place_span, bk, borrow);
11011115
this.buffer_error(err);
11021116
}
1103-
WriteKind::StorageDeadOrDrop => this
1104-
.report_borrowed_value_does_not_live_long_enough(
1105-
location,
1106-
borrow,
1107-
place_span,
1108-
Some(kind),
1109-
),
1117+
WriteKind::StorageDeadOrDrop => {
1118+
use rustc_span::DesugaringKind;
1119+
if let Some(DesugaringKind::Replace) = place_span.1.desugaring_kind() {
1120+
// If this is a drop triggered by a reassignment, it's more user friendly
1121+
// to report a problem with the explicit assignment than the implicit drop.
1122+
this.report_illegal_mutation_of_borrowed(
1123+
location, place_span, borrow,
1124+
)
1125+
} else {
1126+
this.report_borrowed_value_does_not_live_long_enough(
1127+
location,
1128+
borrow,
1129+
place_span,
1130+
Some(kind),
1131+
)
1132+
}
1133+
}
11101134
WriteKind::Mutate => {
11111135
this.report_illegal_mutation_of_borrowed(location, place_span, borrow)
11121136
}

‎compiler/rustc_borrowck/src/type_check/liveness/local_use_map.rs

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use rustc_data_structures::vec_linked_list as vll;
22
use rustc_index::vec::IndexVec;
3-
use rustc_middle::mir::visit::{PlaceContext, Visitor};
3+
use rustc_middle::mir::visit::{NonMutatingUseContext, PlaceContext, Visitor};
44
use rustc_middle::mir::{Body, Local, Location};
55

66
use crate::def_use::{self, DefUse};
@@ -158,6 +158,20 @@ impl LocalUseMapBuild<'_> {
158158

159159
impl Visitor<'_> for LocalUseMapBuild<'_> {
160160
fn visit_local(&mut self, local: Local, context: PlaceContext, location: Location) {
161+
if matches!(
162+
context,
163+
PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect(
164+
rustc_middle::mir::visit::InspectKind::Discriminant
165+
))
166+
) {
167+
// drop elaboration inserts discriminant reads for enums, but such reads should
168+
// really be counted as a drop access for liveness computation.
169+
// However, doing so messes with the way we calculate drop points, so for now just ignore
170+
// those, as discriminant reads are ignored anyway by the borrowck.
171+
// FIXME: found a better solution for drop-elab discriminant reads
172+
return;
173+
}
174+
161175
if self.locals_with_use_data[local] {
162176
match def_use::categorize(context) {
163177
Some(DefUse::Def) => self.insert_def(local, location),

‎compiler/rustc_codegen_ssa/src/mir/analyze.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,7 @@ impl<'mir, 'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> Visitor<'tcx>
230230
| MutatingUseContext::Projection,
231231
)
232232
| PlaceContext::NonMutatingUse(
233-
NonMutatingUseContext::Inspect
233+
NonMutatingUseContext::Inspect(_)
234234
| NonMutatingUseContext::SharedBorrow
235235
| NonMutatingUseContext::UniqueBorrow
236236
| NonMutatingUseContext::ShallowBorrow

‎compiler/rustc_middle/src/mir/visit.rs

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -370,7 +370,7 @@ macro_rules! make_mir_visitor {
370370
StatementKind::FakeRead(box (_, place)) => {
371371
self.visit_place(
372372
place,
373-
PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect),
373+
PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect(InspectKind::Other)),
374374
location
375375
);
376376
}
@@ -652,7 +652,7 @@ macro_rules! make_mir_visitor {
652652
Rvalue::CopyForDeref(place) => {
653653
self.visit_place(
654654
place,
655-
PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect),
655+
PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect(InspectKind::Other)),
656656
location
657657
);
658658
}
@@ -672,7 +672,7 @@ macro_rules! make_mir_visitor {
672672
Rvalue::Len(path) => {
673673
self.visit_place(
674674
path,
675-
PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect),
675+
PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect(InspectKind::Other)),
676676
location
677677
);
678678
}
@@ -695,7 +695,7 @@ macro_rules! make_mir_visitor {
695695
Rvalue::Discriminant(place) => {
696696
self.visit_place(
697697
place,
698-
PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect),
698+
PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect(InspectKind::Discriminant)),
699699
location
700700
);
701701
}
@@ -1132,7 +1132,7 @@ macro_rules! visit_place_fns {
11321132
let mut context = context;
11331133

11341134
if !place.projection.is_empty() {
1135-
if context.is_use() {
1135+
if context.is_use() && !context.is_drop() {
11361136
// ^ Only change the context if it is a real use, not a "use" in debuginfo.
11371137
context = if context.is_mutating_use() {
11381138
PlaceContext::MutatingUse(MutatingUseContext::Projection)
@@ -1236,10 +1236,16 @@ pub enum TyContext {
12361236
Location(Location),
12371237
}
12381238

1239+
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
1240+
pub enum InspectKind {
1241+
Other,
1242+
Discriminant,
1243+
}
1244+
12391245
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
12401246
pub enum NonMutatingUseContext {
12411247
/// Being inspected in some way, like loading a len.
1242-
Inspect,
1248+
Inspect(InspectKind),
12431249
/// Consumed as part of an operand.
12441250
Copy,
12451251
/// Consumed as part of an operand.

‎compiler/rustc_mir_build/src/build/expr/stmt.rs

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,19 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
3939

4040
// Generate better code for things that don't need to be
4141
// dropped.
42+
use rustc_span::DesugaringKind;
4243
if lhs.ty.needs_drop(this.tcx, this.param_env) {
4344
let rhs = unpack!(block = this.as_local_operand(block, rhs));
4445
let lhs = unpack!(block = this.as_place(block, lhs));
45-
unpack!(block = this.build_drop_and_replace(block, lhs_span, lhs, rhs));
46+
let span = this.tcx.with_stable_hashing_context(|hcx| {
47+
lhs_span.mark_with_reason(
48+
None,
49+
DesugaringKind::Replace,
50+
this.tcx.sess.edition(),
51+
hcx,
52+
)
53+
});
54+
unpack!(block = this.build_drop_and_replace(block, span, lhs, rhs));
4655
} else {
4756
let rhs = unpack!(block = this.as_local_rvalue(block, rhs));
4857
let lhs = unpack!(block = this.as_place(block, lhs));

‎compiler/rustc_mir_dataflow/src/drop_flag_effects.rs

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,35 @@ where
2929
None
3030
}
3131

32+
// Drop elaboration of a Box makes the inner fields (pointer and allocator) explicit in MIR,
33+
// which causes those path to be tracked by dataflow and checked by the borrowck.
34+
// However, those are not considered to be children of *_box_place (and only of _box_place),
35+
// which causes issues in initialization analysis when accessing _box_place.
36+
// This code basically check if the parent of the path is a box and if so, apply the callback
37+
// to its children to fix such cases.
38+
fn maybe_recurse_box_parent<'tcx, F>(
39+
tcx: TyCtxt<'tcx>,
40+
body: &Body<'tcx>,
41+
move_data: &MoveData<'tcx>,
42+
move_path_index: MovePathIndex,
43+
mut each_child: F,
44+
) where
45+
F: FnMut(MovePathIndex),
46+
{
47+
if let Some(path) = move_data.move_paths[move_path_index].parent {
48+
let place = move_data.move_paths[path].place;
49+
let ty = place.ty(body, tcx).ty;
50+
if let ty::Adt(def, _) = ty.kind() {
51+
if def.is_box() {
52+
each_child(move_path_index);
53+
on_all_children_bits(tcx, body, move_data, path, each_child);
54+
return;
55+
}
56+
}
57+
}
58+
on_all_children_bits(tcx, body, move_data, move_path_index, each_child);
59+
}
60+
3261
pub fn on_lookup_result_bits<'tcx, F>(
3362
tcx: TyCtxt<'tcx>,
3463
body: &Body<'tcx>,
@@ -42,7 +71,7 @@ pub fn on_lookup_result_bits<'tcx, F>(
4271
LookupResult::Parent(..) => {
4372
// access to untracked value - do not touch children
4473
}
45-
LookupResult::Exact(e) => on_all_children_bits(tcx, body, move_data, e, each_child),
74+
LookupResult::Exact(e) => maybe_recurse_box_parent(tcx, body, move_data, e, each_child),
4675
}
4776
}
4877

@@ -194,6 +223,19 @@ pub fn drop_flag_effects_for_location<'tcx, F>(
194223
on_all_children_bits(tcx, body, move_data, path, |mpi| callback(mpi, DropFlagState::Absent))
195224
}
196225

226+
use rustc_middle::mir::{Terminator, TerminatorKind};
227+
228+
// Drop does not count as a move but we should still consider the variable uninitialized.
229+
if let Some(Terminator { kind: TerminatorKind::Drop { place, .. }, .. }) =
230+
body.stmt_at(loc).right()
231+
{
232+
if let LookupResult::Exact(mpi) = move_data.rev_lookup.find(place.as_ref()) {
233+
on_all_children_bits(tcx, body, move_data, mpi, |mpi| {
234+
callback(mpi, DropFlagState::Absent)
235+
})
236+
}
237+
}
238+
197239
debug!("drop_flag_effects: assignment for location({:?})", loc);
198240

199241
for_location_inits(tcx, body, move_data, loc, |mpi| callback(mpi, DropFlagState::Present));
@@ -212,9 +254,7 @@ pub fn for_location_inits<'tcx, F>(
212254
let init = move_data.inits[*ii];
213255
match init.kind {
214256
InitKind::Deep => {
215-
let path = init.path;
216-
217-
on_all_children_bits(tcx, body, move_data, path, &mut callback)
257+
maybe_recurse_box_parent(tcx, body, move_data, init.path, &mut callback)
218258
}
219259
InitKind::Shallow => {
220260
let mpi = init.path;

‎compiler/rustc_mir_dataflow/src/elaborate_drops.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ use rustc_hir::lang_items::LangItem;
33
use rustc_index::vec::Idx;
44
use rustc_middle::mir::patch::MirPatch;
55
use rustc_middle::mir::*;
6-
use rustc_middle::traits::Reveal;
76
use rustc_middle::ty::subst::SubstsRef;
87
use rustc_middle::ty::util::IntTypeExt;
98
use rustc_middle::ty::{self, Ty, TyCtxt};
@@ -273,7 +272,6 @@ where
273272
let subpath = self.elaborator.field_subpath(variant_path, field);
274273
let tcx = self.tcx();
275274

276-
assert_eq!(self.elaborator.param_env().reveal(), Reveal::All);
277275
let field_ty =
278276
tcx.normalize_erasing_regions(self.elaborator.param_env(), f.ty(tcx, substs));
279277
(tcx.mk_place_field(base_place, field, field_ty), subpath)

‎compiler/rustc_mir_dataflow/src/impls/liveness.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,7 @@ impl DefUse {
195195
| PlaceContext::NonMutatingUse(
196196
NonMutatingUseContext::AddressOf
197197
| NonMutatingUseContext::Copy
198-
| NonMutatingUseContext::Inspect
198+
| NonMutatingUseContext::Inspect(_)
199199
| NonMutatingUseContext::Move
200200
| NonMutatingUseContext::ShallowBorrow
201201
| NonMutatingUseContext::SharedBorrow

0 commit comments

Comments
 (0)