From 125326e4ed6c4752cd7fdea75128379cb24847f6 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Tue, 10 May 2022 13:43:14 -0700 Subject: [PATCH 1/5] Add test case for the need for fake_read callbacks --- .../generator/drop-track-addassign-yield.rs | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 src/test/ui/generator/drop-track-addassign-yield.rs diff --git a/src/test/ui/generator/drop-track-addassign-yield.rs b/src/test/ui/generator/drop-track-addassign-yield.rs new file mode 100644 index 0000000000000..71cfb170bf6aa --- /dev/null +++ b/src/test/ui/generator/drop-track-addassign-yield.rs @@ -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() +} From 5ba2e09bde735b4a5783fc1e6439f7d3bb00ff7d Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Thu, 5 May 2022 11:59:45 -0700 Subject: [PATCH 2/5] Fix addassign-yield.rs by implementing fake_read --- .../drop_ranges/record_consumed_borrow.rs | 79 +++++++++++-------- compiler/rustc_typeck/src/check/upvar.rs | 11 ++- compiler/rustc_typeck/src/expr_use_visitor.rs | 19 +++-- .../ui/generator/yielding-in-match-guards.rs | 42 +++++----- 4 files changed, 88 insertions(+), 63 deletions(-) diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs index 928daba0a7b39..20269a67c72b6 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs @@ -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)); @@ -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, @@ -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. + self.borrow_place(place_with_id); } } diff --git a/compiler/rustc_typeck/src/check/upvar.rs b/compiler/rustc_typeck/src/check/upvar.rs index fee2efd5804f0..f57d576105185 100644 --- a/compiler/rustc_typeck/src/check/upvar.rs +++ b/compiler/rustc_typeck/src/check/upvar.rs @@ -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, diff --git a/compiler/rustc_typeck/src/expr_use_visitor.rs b/compiler/rustc_typeck/src/expr_use_visitor.rs index e69ba99dcef42..aa902ad726220 100644 --- a/compiler/rustc_typeck/src/expr_use_visitor.rs +++ b/compiler/rustc_typeck/src/expr_use_visitor.rs @@ -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)] @@ -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, ); @@ -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, ); @@ -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, ); @@ -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, + ); } } diff --git a/src/test/ui/generator/yielding-in-match-guards.rs b/src/test/ui/generator/yielding-in-match-guards.rs index 4e89fc975d04c..86481edf76b0d 100644 --- a/src/test/ui/generator/yielding-in-match-guards.rs +++ b/src/test/ui/generator/yielding-in-match-guards.rs @@ -15,21 +15,21 @@ async fn f() -> u8 { 1 } async fn foo() -> [bool; 10] { [false; 10] } -pub async fn g(x: u8) { - match x { - y if f().await == y => (), - _ => (), - } -} +// pub async fn g(x: u8) { +// match x { +// y if f().await == y => (), +// _ => (), +// } +// } // #78366: check the reference to the binding is recorded even if the binding is not autorefed -async fn h(x: usize) { - match x { - y if foo().await[y] => (), - _ => (), - } -} +// async fn h(x: usize) { +// match x { +// y if foo().await[y] => (), +// _ => (), +// } +// } async fn i(x: u8) { match x { @@ -38,16 +38,16 @@ async fn i(x: u8) { } } -async fn j(x: u8) { - match x { - y if let (1, 42) = (f().await, y) => (), - _ => (), - } -} +// async fn j(x: u8) { +// match x { +// y if let (1, 42) = (f().await, y) => (), +// _ => (), +// } +// } fn main() { - let _ = g(10); - let _ = h(9); + // let _ = g(10); + // let _ = h(9); let _ = i(8); - let _ = j(7); + // let _ = j(7); } From 36c4c1e366813e7c25fc2faf3a0566f267b62688 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Tue, 10 May 2022 14:20:34 -0700 Subject: [PATCH 3/5] Update clippy to new rake_read signature --- src/tools/clippy/clippy_lints/src/escape.rs | 2 +- src/tools/clippy/clippy_lints/src/loops/mut_range_bound.rs | 2 +- src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs | 2 +- src/tools/clippy/clippy_utils/src/sugg.rs | 2 +- src/tools/clippy/clippy_utils/src/usage.rs | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/tools/clippy/clippy_lints/src/escape.rs b/src/tools/clippy/clippy_lints/src/escape.rs index af591dd71aa1d..807ecd2ddd16e 100644 --- a/src/tools/clippy/clippy_lints/src/escape.rs +++ b/src/tools/clippy/clippy_lints/src/escape.rs @@ -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> { diff --git a/src/tools/clippy/clippy_lints/src/loops/mut_range_bound.rs b/src/tools/clippy/clippy_lints/src/loops/mut_range_bound.rs index 9d8679d77c6d0..d20df83045589 100644 --- a/src/tools/clippy/clippy_lints/src/loops/mut_range_bound.rs +++ b/src/tools/clippy/clippy_lints/src/loops/mut_range_bound.rs @@ -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<'_, '_> { diff --git a/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs b/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs index 4034079a90c0d..303ce7a5075aa 100644 --- a/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs +++ b/src/tools/clippy/clippy_lints/src/needless_pass_by_value.rs @@ -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) {} } diff --git a/src/tools/clippy/clippy_utils/src/sugg.rs b/src/tools/clippy/clippy_utils/src/sugg.rs index 18915553e61c0..db5299c2c05f4 100644 --- a/src/tools/clippy/clippy_utils/src/sugg.rs +++ b/src/tools/clippy/clippy_utils/src/sugg.rs @@ -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)] diff --git a/src/tools/clippy/clippy_utils/src/usage.rs b/src/tools/clippy/clippy_utils/src/usage.rs index 4236e3aae2fbd..b7b9d54d0b2c1 100644 --- a/src/tools/clippy/clippy_utils/src/usage.rs +++ b/src/tools/clippy/clippy_utils/src/usage.rs @@ -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 { From 6d6be5fd8b6615dbf8b6244d777a462efda3d410 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Tue, 10 May 2022 14:22:03 -0700 Subject: [PATCH 4/5] Revert spurious change --- .../ui/generator/yielding-in-match-guards.rs | 42 +++++++++---------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/src/test/ui/generator/yielding-in-match-guards.rs b/src/test/ui/generator/yielding-in-match-guards.rs index 86481edf76b0d..4e89fc975d04c 100644 --- a/src/test/ui/generator/yielding-in-match-guards.rs +++ b/src/test/ui/generator/yielding-in-match-guards.rs @@ -15,21 +15,21 @@ async fn f() -> u8 { 1 } async fn foo() -> [bool; 10] { [false; 10] } -// pub async fn g(x: u8) { -// match x { -// y if f().await == y => (), -// _ => (), -// } -// } +pub async fn g(x: u8) { + match x { + y if f().await == y => (), + _ => (), + } +} // #78366: check the reference to the binding is recorded even if the binding is not autorefed -// async fn h(x: usize) { -// match x { -// y if foo().await[y] => (), -// _ => (), -// } -// } +async fn h(x: usize) { + match x { + y if foo().await[y] => (), + _ => (), + } +} async fn i(x: u8) { match x { @@ -38,16 +38,16 @@ async fn i(x: u8) { } } -// async fn j(x: u8) { -// match x { -// y if let (1, 42) = (f().await, y) => (), -// _ => (), -// } -// } +async fn j(x: u8) { + match x { + y if let (1, 42) = (f().await, y) => (), + _ => (), + } +} fn main() { - // let _ = g(10); - // let _ = h(9); + let _ = g(10); + let _ = h(9); let _ = i(8); - // let _ = j(7); + let _ = j(7); } From bf21a81b1541a420566fc81f8a1dde5486008387 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 20 May 2022 15:54:22 -0400 Subject: [PATCH 5/5] Update compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs --- .../drop_ranges/record_consumed_borrow.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs index 20269a67c72b6..8dcffd56b5843 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs @@ -211,8 +211,10 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> { "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. + // fake reads happen in places like the scrutinee of a match expression. + // we treat those as a borrow, much like a copy: the idea is that we are + // transiently creating a `&T` ref that we can read from to observe the current + // value (this `&T` is immediately dropped afterwards). self.borrow_place(place_with_id); } }