-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
ExprUseVisitor: Treat ByValue use of Copy types as ImmBorrow #87069
Changes from 3 commits
10b536f
36f51c9
6c3774e
c4ac836
75291ee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2,8 +2,6 @@ | |||||||||||||||||||||
//! normal visitor, which just walks the entire body in one shot, the | ||||||||||||||||||||||
//! `ExprUseVisitor` determines how expressions are being used. | ||||||||||||||||||||||
|
||||||||||||||||||||||
pub use self::ConsumeMode::*; | ||||||||||||||||||||||
|
||||||||||||||||||||||
// Export these here so that Clippy can use them. | ||||||||||||||||||||||
pub use rustc_middle::hir::place::{Place, PlaceBase, PlaceWithHirId, Projection}; | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
@@ -28,19 +26,14 @@ use crate::mem_categorization as mc; | |||||||||||||||||||||
/// This trait defines the callbacks you can expect to receive when | ||||||||||||||||||||||
/// employing the ExprUseVisitor. | ||||||||||||||||||||||
pub trait Delegate<'tcx> { | ||||||||||||||||||||||
// The value found at `place` is either copied or moved, depending | ||||||||||||||||||||||
// The value found at `place` is moved, depending | ||||||||||||||||||||||
// on `mode`. Where `diag_expr_id` is the id used for diagnostics for `place`. | ||||||||||||||||||||||
// | ||||||||||||||||||||||
// The parameter `diag_expr_id` indicates the HIR id that ought to be used for | ||||||||||||||||||||||
// diagnostics. Around pattern matching such as `let pat = expr`, the diagnostic | ||||||||||||||||||||||
// id will be the id of the expression `expr` but the place itself will have | ||||||||||||||||||||||
// the id of the binding in the pattern `pat`. | ||||||||||||||||||||||
fn consume( | ||||||||||||||||||||||
&mut self, | ||||||||||||||||||||||
place_with_id: &PlaceWithHirId<'tcx>, | ||||||||||||||||||||||
diag_expr_id: hir::HirId, | ||||||||||||||||||||||
mode: ConsumeMode, | ||||||||||||||||||||||
); | ||||||||||||||||||||||
fn consume(&mut self, place_with_id: &PlaceWithHirId<'tcx>, diag_expr_id: hir::HirId); | ||||||||||||||||||||||
|
||||||||||||||||||||||
// The value found at `place` is being borrowed with kind `bk`. | ||||||||||||||||||||||
// `diag_expr_id` is the id used for diagnostics (see `consume` for more details). | ||||||||||||||||||||||
|
@@ -60,7 +53,7 @@ pub trait Delegate<'tcx> { | |||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
#[derive(Copy, Clone, PartialEq, Debug)] | ||||||||||||||||||||||
pub enum ConsumeMode { | ||||||||||||||||||||||
enum ConsumeMode { | ||||||||||||||||||||||
Copy, // reference to x where x has a type that copies | ||||||||||||||||||||||
Move, // reference to x where x has a type that moves | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
@@ -144,7 +137,13 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> { | |||||||||||||||||||||
debug!("delegate_consume(place_with_id={:?})", place_with_id); | ||||||||||||||||||||||
|
||||||||||||||||||||||
let mode = copy_or_move(&self.mc, place_with_id); | ||||||||||||||||||||||
self.delegate.consume(place_with_id, diag_expr_id, mode); | ||||||||||||||||||||||
|
||||||||||||||||||||||
match mode { | ||||||||||||||||||||||
ConsumeMode::Move => self.delegate.consume(place_with_id, diag_expr_id), | ||||||||||||||||||||||
ConsumeMode::Copy => { | ||||||||||||||||||||||
self.delegate.borrow(place_with_id, diag_expr_id, ty::BorrowKind::ImmBorrow) | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I had expected to make this change within the upvar delegate, and not the ExprUseVisitor. It seems potentially useful to some clients to know that this was a "by-value copy" -- I think there is other code in clippy that uses this, for example. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Otherwise we should just remove the |
||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
fn consume_exprs(&mut self, exprs: &[hir::Expr<'_>]) { | ||||||||||||||||||||||
|
@@ -653,9 +652,16 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> { | |||||||||||||||||||||
delegate.borrow(place, discr_place.hir_id, bk); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
ty::BindByValue(..) => { | ||||||||||||||||||||||
let mode = copy_or_move(mc, &place); | ||||||||||||||||||||||
debug!("walk_pat binding consuming pat"); | ||||||||||||||||||||||
delegate.consume(place, discr_place.hir_id, mode); | ||||||||||||||||||||||
let mode = copy_or_move(mc, &place); | ||||||||||||||||||||||
match mode { | ||||||||||||||||||||||
ConsumeMode::Move => delegate.consume(place, discr_place.hir_id), | ||||||||||||||||||||||
ConsumeMode::Copy => delegate.borrow( | ||||||||||||||||||||||
place, | ||||||||||||||||||||||
discr_place.hir_id, | ||||||||||||||||||||||
ty::BorrowKind::ImmBorrow, | ||||||||||||||||||||||
), | ||||||||||||||||||||||
} | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Does this not work? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we need 2229 enabled to make this work 🙃. The closure is passed to |
||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
@@ -773,8 +779,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> { | |||||||||||||||||||||
|
||||||||||||||||||||||
match capture_info.capture_kind { | ||||||||||||||||||||||
ty::UpvarCapture::ByValue(_) => { | ||||||||||||||||||||||
let mode = copy_or_move(&self.mc, &place_with_id); | ||||||||||||||||||||||
self.delegate.consume(&place_with_id, place_with_id.hir_id, mode); | ||||||||||||||||||||||
self.delegate_consume(&place_with_id, place_with_id.hir_id); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
ty::UpvarCapture::ByRef(upvar_borrow) => { | ||||||||||||||||||||||
self.delegate.borrow( | ||||||||||||||||||||||
|
@@ -798,8 +803,8 @@ fn copy_or_move<'a, 'tcx>( | |||||||||||||||||||||
place_with_id.place.ty(), | ||||||||||||||||||||||
mc.tcx().hir().span(place_with_id.hir_id), | ||||||||||||||||||||||
) { | ||||||||||||||||||||||
Move | ||||||||||||||||||||||
ConsumeMode::Move | ||||||||||||||||||||||
} else { | ||||||||||||||||||||||
Copy | ||||||||||||||||||||||
ConsumeMode::Copy | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't add the detail about closures because ExprUseVisitor and therefore this delegate is used in other contexts too