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.

Sign up for GitHub

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

Drop Tracking: Implement fake_read callback #96923

Merged
merged 5 commits into from
May 21, 2022
Merged

Drop Tracking: Implement fake_read callback #96923

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
125326e
Add test case for the need for fake_read callbacks
eholk May 10, 2022
5ba2e09
Fix addassign-yield.rs by implementing fake_read
eholk May 5, 2022
36c4c1e
Update clippy to new rake_read signature
eholk May 10, 2022
6d6be5f
Revert spurious change
eholk May 10, 2022
bf21a81
Update compiler/rustc_typeck/src/check/generator_interior/drop_ranges…
nikomatsakis May 20, 2022
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
79 changes: 45 additions & 34 deletions compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,38 +77,8 @@ impl<'tcx> ExprUseDelegate<'tcx> {
}
self.places.consumed.get_mut(&consumer).map(|places| places.insert(target));
}
}

impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> {
fn consume(
&mut self,
place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>,
diag_expr_id: HirId,
) {
let parent = match self.tcx.hir().find_parent_node(place_with_id.hir_id) {
Some(parent) => parent,
None => place_with_id.hir_id,
};
debug!(
"consume {:?}; diag_expr_id={:?}, using parent {:?}",
place_with_id, diag_expr_id, parent
);
place_with_id
.try_into()
.map_or((), |tracked_value| self.mark_consumed(parent, tracked_value));
}

fn borrow(
&mut self,
place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>,
diag_expr_id: HirId,
bk: rustc_middle::ty::BorrowKind,
) {
debug!(
"borrow: place_with_id = {place_with_id:?}, diag_expr_id={diag_expr_id:?}, \
borrow_kind={bk:?}"
);

fn borrow_place(&mut self, place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>) {
self.places
.borrowed
.insert(TrackedValue::from_place_with_projections_allowed(place_with_id));
Expand Down Expand Up @@ -158,6 +128,40 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> {
self.places.borrowed_temporaries.insert(place_with_id.hir_id);
}
}
}

impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> {
fn consume(
&mut self,
place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>,
diag_expr_id: HirId,
) {
let parent = match self.tcx.hir().find_parent_node(place_with_id.hir_id) {
Some(parent) => parent,
None => place_with_id.hir_id,
};
debug!(
"consume {:?}; diag_expr_id={:?}, using parent {:?}",
place_with_id, diag_expr_id, parent
);
place_with_id
.try_into()
.map_or((), |tracked_value| self.mark_consumed(parent, tracked_value));
}

fn borrow(
&mut self,
place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>,
diag_expr_id: HirId,
bk: rustc_middle::ty::BorrowKind,
) {
debug!(
"borrow: place_with_id = {place_with_id:?}, diag_expr_id={diag_expr_id:?}, \
borrow_kind={bk:?}"
);

self.borrow_place(place_with_id);
}

fn copy(
&mut self,
Expand Down Expand Up @@ -199,9 +203,16 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> {

fn fake_read(
&mut self,
_place: expr_use_visitor::Place<'tcx>,
_cause: rustc_middle::mir::FakeReadCause,
_diag_expr_id: HirId,
place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>,
cause: rustc_middle::mir::FakeReadCause,
diag_expr_id: HirId,
) {
debug!(
"fake_read place_with_id={place_with_id:?}; cause={cause:?}; diag_expr_id={diag_expr_id:?}"
);

// fake reads happen in places like the scrutinee of a match expression, so we can treat
// these as a borrow.
nikomatsakis marked this conversation as resolved.
Show resolved Hide resolved
self.borrow_place(place_with_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, I'm surprised this would borrow the place and not just "read" it. I guess I have to take a look at where this is invoked.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a borrow is actually ok, just like with copy -- i.e., we are kind of "transiently borrowing" this to read its value -- the only thing is that we don't necessarily want to consider it a borrowed temporary -- but I am don't see it does any harm if we do so (we might over-approximate a bit).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like your updated comment, and thanks for the review!

We don't really have a concept of read here, we just have borrows, so I think treating a read as a borrow makes sense. You borrow it so that you can read from it.

}
}
11 changes: 8 additions & 3 deletions compiler/rustc_typeck/src/check/upvar.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1755,14 +1755,19 @@ struct InferBorrowKind<'a, 'tcx> {
}

impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> {
fn fake_read(&mut self, place: Place<'tcx>, cause: FakeReadCause, diag_expr_id: hir::HirId) {
let PlaceBase::Upvar(_) = place.base else { return };
fn fake_read(
&mut self,
place: &PlaceWithHirId<'tcx>,
cause: FakeReadCause,
diag_expr_id: hir::HirId,
) {
let PlaceBase::Upvar(_) = place.place.base else { return };

// We need to restrict Fake Read precision to avoid fake reading unsafe code,
// such as deref of a raw pointer.
let dummy_capture_kind = ty::UpvarCapture::ByRef(ty::BorrowKind::ImmBorrow);

let (place, _) = restrict_capture_precision(place, dummy_capture_kind);
let (place, _) = restrict_capture_precision(place.place.clone(), dummy_capture_kind);

let (place, _) = restrict_repr_packed_field_ref_capture(
self.fcx.tcx,
Expand Down
19 changes: 14 additions & 5 deletions compiler/rustc_typeck/src/expr_use_visitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,12 @@ pub trait Delegate<'tcx> {
}

/// The `place` should be a fake read because of specified `cause`.
fn fake_read(&mut self, place: Place<'tcx>, cause: FakeReadCause, diag_expr_id: hir::HirId);
fn fake_read(
&mut self,
place_with_id: &PlaceWithHirId<'tcx>,
cause: FakeReadCause,
diag_expr_id: hir::HirId,
);
}

#[derive(Copy, Clone, PartialEq, Debug)]
Expand Down Expand Up @@ -327,7 +332,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
};

self.delegate.fake_read(
discr_place.place.clone(),
&discr_place,
FakeReadCause::ForMatchedPlace(closure_def_id),
discr_place.hir_id,
);
Expand Down Expand Up @@ -617,7 +622,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
};

self.delegate.fake_read(
discr_place.place.clone(),
discr_place,
FakeReadCause::ForMatchedPlace(closure_def_id),
discr_place.hir_id,
);
Expand All @@ -641,7 +646,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
};

self.delegate.fake_read(
discr_place.place.clone(),
discr_place,
FakeReadCause::ForLet(closure_def_id),
discr_place.hir_id,
);
Expand Down Expand Up @@ -764,7 +769,11 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
);
}
};
self.delegate.fake_read(fake_read.clone(), *cause, *hir_id);
self.delegate.fake_read(
&PlaceWithHirId { place: fake_read.clone(), hir_id: *hir_id },
*cause,
*hir_id,
);
}
}

Expand Down
41 changes: 41 additions & 0 deletions src/test/ui/generator/drop-track-addassign-yield.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// run-pass
// compile-flags: -Zdrop-tracking

// Based on addassign-yield.rs, but with drop tracking enabled. Originally we did not implement
// the fake_read callback on ExprUseVisitor which caused this case to break.

#![feature(generators)]

fn foo() {
let _y = static || {
let x = &mut 0;
*{
yield;
x
} += match String::new() {
_ => 0,
};
};

// Please don't ever actually write something like this
let _z = static || {
let x = &mut 0;
*{
let inner = &mut 1;
*{
yield ();
inner
} += match String::new() {
_ => 1,
};
yield;
x
} += match String::new() {
_ => 2,
};
};
}

fn main() {
foo()
}
2 changes: 1 addition & 1 deletion src/tools/clippy/clippy_lints/src/escape.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ impl<'a, 'tcx> Delegate<'tcx> for EscapeDelegate<'a, 'tcx> {
}
}

fn fake_read(&mut self, _: rustc_typeck::expr_use_visitor::Place<'tcx>, _: FakeReadCause, _: HirId) {}
fn fake_read(&mut self, _: &rustc_typeck::expr_use_visitor::PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {}
}

impl<'a, 'tcx> EscapeDelegate<'a, 'tcx> {
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/clippy_lints/src/loops/mut_range_bound.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ impl<'tcx> Delegate<'tcx> for MutatePairDelegate<'_, 'tcx> {
}
}

fn fake_read(&mut self, _: rustc_typeck::expr_use_visitor::Place<'tcx>, _: FakeReadCause, _: HirId) {}
fn fake_read(&mut self, _: &rustc_typeck::expr_use_visitor::PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {}
}

impl MutatePairDelegate<'_, '_> {
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,5 +343,5 @@ impl<'tcx> euv::Delegate<'tcx> for MovedVariablesCtxt {

fn mutate(&mut self, _: &euv::PlaceWithHirId<'tcx>, _: HirId) {}

fn fake_read(&mut self, _: rustc_typeck::expr_use_visitor::Place<'tcx>, _: FakeReadCause, _: HirId) {}
fn fake_read(&mut self, _: &rustc_typeck::expr_use_visitor::PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {}
}
2 changes: 1 addition & 1 deletion src/tools/clippy/clippy_utils/src/sugg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1033,7 +1033,7 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> {

fn mutate(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {}

fn fake_read(&mut self, _: rustc_typeck::expr_use_visitor::Place<'tcx>, _: FakeReadCause, _: HirId) {}
fn fake_read(&mut self, _: &rustc_typeck::expr_use_visitor::PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {}
}

#[cfg(test)]
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/clippy_utils/src/usage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ impl<'tcx> Delegate<'tcx> for MutVarsDelegate {
self.update(cmt);
}

fn fake_read(&mut self, _: rustc_typeck::expr_use_visitor::Place<'tcx>, _: FakeReadCause, _: HirId) {}
fn fake_read(&mut self, _: &rustc_typeck::expr_use_visitor::PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {}
}

pub struct ParamBindingIdCollector {
Expand Down