From a68cd88860fc9d25223416bb10782a70d556dca7 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Thu, 14 Sep 2023 12:41:16 -0400 Subject: [PATCH] Lint `needless_borrow` on most union field accesses --- clippy_lints/src/dereference.rs | 21 +++++++++++++-- tests/ui/needless_borrow.fixed | 47 ++++++++++++++++++++++++--------- tests/ui/needless_borrow.rs | 47 ++++++++++++++++++++++++--------- tests/ui/needless_borrow.stderr | 26 +++++++++++++++++- 4 files changed, 112 insertions(+), 29 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 6c109a51f83..5371704f66e 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -5,6 +5,7 @@ use clippy_utils::{ expr_use_ctxt, get_parent_expr, get_parent_node, is_lint_allowed, path_to_local, DefinedTy, ExprUseNode, }; +use core::mem; use rustc_ast::util::parser::{PREC_POSTFIX, PREC_PREFIX}; use rustc_data_structures::fx::FxIndexMap; use rustc_errors::Applicability; @@ -342,8 +343,24 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { TyCoercionStability::for_defined_ty(cx, ty, use_cx.node.is_return()) }); let can_auto_borrow = match use_cx.node { - ExprUseNode::Callee => true, - ExprUseNode::FieldAccess(_) => adjusted_ty.ty_adt_def().map_or(true, |adt| !adt.is_union()), + ExprUseNode::FieldAccess(_) + if !use_cx.moved_before_use && matches!(sub_expr.kind, ExprKind::Field(..)) => + { + // `ManuallyDrop` fields of a union will not automatically call + // `deref_mut` when accessing a field. + // Note: if the `ManuallyDrop` value is not directly a field of a + // union then `DerefMut` will work as normal. + // + // This means we need to not modify an expression such as `(&mut x.y).z` + // if accessing `z` would require `DerefMut`. + let mut ty = expr_ty; + !use_cx.adjustments.iter().any(|a| { + let ty = mem::replace(&mut ty, a.target); + matches!(a.kind, Adjust::Deref(Some(ref op)) if op.mutbl == Mutability::Mut) + && ty.ty_adt_def().map_or(false, |def| def.is_manually_drop()) + }) + }, + ExprUseNode::Callee | ExprUseNode::FieldAccess(_) => true, ExprUseNode::MethodArg(hir_id, _, 0) if !use_cx.moved_before_use => { // Check for calls to trait methods where the trait is implemented // on a reference. diff --git a/tests/ui/needless_borrow.fixed b/tests/ui/needless_borrow.fixed index c2c5f765abf..ff1e2dc8875 100644 --- a/tests/ui/needless_borrow.fixed +++ b/tests/ui/needless_borrow.fixed @@ -190,27 +190,48 @@ fn issue9383() { // Should not lint because unions need explicit deref when accessing field use std::mem::ManuallyDrop; - union Coral { - crab: ManuallyDrop>, + #[derive(Clone, Copy)] + struct Wrap(T); + impl core::ops::Deref for Wrap { + type Target = T; + fn deref(&self) -> &T { + &self.0 + } + } + impl core::ops::DerefMut for Wrap { + fn deref_mut(&mut self) -> &mut T { + &mut self.0 + } } - union Ocean { - coral: ManuallyDrop, + union U { + u: T, } - let mut ocean = Ocean { - coral: ManuallyDrop::new(Coral { - crab: ManuallyDrop::new(vec![1, 2, 3]), - }), - }; + #[derive(Clone, Copy)] + struct Foo { + x: u32, + } unsafe { - ManuallyDrop::drop(&mut (&mut ocean.coral).crab); + let mut x = U { + u: ManuallyDrop::new(Foo { x: 0 }), + }; + let _ = &mut (&mut x.u).x; + let _ = &mut { x.u }.x; + let _ = &mut ({ &mut x.u }).x; - (*ocean.coral).crab = ManuallyDrop::new(vec![4, 5, 6]); - ManuallyDrop::drop(&mut (*ocean.coral).crab); + let mut x = U { + u: Wrap(ManuallyDrop::new(Foo { x: 0 })), + }; + let _ = &mut (&mut x.u).x; + let _ = &mut { x.u }.x; + let _ = &mut ({ &mut x.u }).x; - ManuallyDrop::drop(&mut ocean.coral); + let mut x = U { u: Wrap(Foo { x: 0 }) }; + let _ = &mut x.u.x; + let _ = &mut { x.u }.x; + let _ = &mut ({ &mut x.u }).x; } } diff --git a/tests/ui/needless_borrow.rs b/tests/ui/needless_borrow.rs index 0cd6e41b8a4..597021539ac 100644 --- a/tests/ui/needless_borrow.rs +++ b/tests/ui/needless_borrow.rs @@ -190,27 +190,48 @@ fn issue9383() { // Should not lint because unions need explicit deref when accessing field use std::mem::ManuallyDrop; - union Coral { - crab: ManuallyDrop>, + #[derive(Clone, Copy)] + struct Wrap(T); + impl core::ops::Deref for Wrap { + type Target = T; + fn deref(&self) -> &T { + &self.0 + } + } + impl core::ops::DerefMut for Wrap { + fn deref_mut(&mut self) -> &mut T { + &mut self.0 + } } - union Ocean { - coral: ManuallyDrop, + union U { + u: T, } - let mut ocean = Ocean { - coral: ManuallyDrop::new(Coral { - crab: ManuallyDrop::new(vec![1, 2, 3]), - }), - }; + #[derive(Clone, Copy)] + struct Foo { + x: u32, + } unsafe { - ManuallyDrop::drop(&mut (&mut ocean.coral).crab); + let mut x = U { + u: ManuallyDrop::new(Foo { x: 0 }), + }; + let _ = &mut (&mut x.u).x; + let _ = &mut (&mut { x.u }).x; + let _ = &mut ({ &mut x.u }).x; - (*ocean.coral).crab = ManuallyDrop::new(vec![4, 5, 6]); - ManuallyDrop::drop(&mut (*ocean.coral).crab); + let mut x = U { + u: Wrap(ManuallyDrop::new(Foo { x: 0 })), + }; + let _ = &mut (&mut x.u).x; + let _ = &mut (&mut { x.u }).x; + let _ = &mut ({ &mut x.u }).x; - ManuallyDrop::drop(&mut ocean.coral); + let mut x = U { u: Wrap(Foo { x: 0 }) }; + let _ = &mut (&mut x.u).x; + let _ = &mut (&mut { x.u }).x; + let _ = &mut ({ &mut x.u }).x; } } diff --git a/tests/ui/needless_borrow.stderr b/tests/ui/needless_borrow.stderr index e91b78b0a15..44552ee6abe 100644 --- a/tests/ui/needless_borrow.stderr +++ b/tests/ui/needless_borrow.stderr @@ -133,5 +133,29 @@ error: this expression borrows a value the compiler would automatically borrow LL | (&mut self.f)() | ^^^^^^^^^^^^^ help: change this to: `(self.f)` -error: aborting due to 22 previous errors +error: this expression borrows a value the compiler would automatically borrow + --> $DIR/needless_borrow.rs:221:22 + | +LL | let _ = &mut (&mut { x.u }).x; + | ^^^^^^^^^^^^^^ help: change this to: `{ x.u }` + +error: this expression borrows a value the compiler would automatically borrow + --> $DIR/needless_borrow.rs:228:22 + | +LL | let _ = &mut (&mut { x.u }).x; + | ^^^^^^^^^^^^^^ help: change this to: `{ x.u }` + +error: this expression borrows a value the compiler would automatically borrow + --> $DIR/needless_borrow.rs:232:22 + | +LL | let _ = &mut (&mut x.u).x; + | ^^^^^^^^^^ help: change this to: `x.u` + +error: this expression borrows a value the compiler would automatically borrow + --> $DIR/needless_borrow.rs:233:22 + | +LL | let _ = &mut (&mut { x.u }).x; + | ^^^^^^^^^^^^^^ help: change this to: `{ x.u }` + +error: aborting due to 26 previous errors