Skip to content

Lint needless_borrow and explicit_auto_deref on most union field accesses #11508

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

Merged
merged 2 commits into from
Nov 12, 2023
Merged

Lint needless_borrow and explicit_auto_deref on most union field accesses #11508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a68cd88
Lint `needless_borrow` on most union field accesses
Jarcho Sep 14, 2023
1a01132
Lint `explicit_auto_deref` on most union field accesses.
Jarcho Sep 15, 2023
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
247 changes: 151 additions & 96 deletions clippy_lints/src/dereference.rs
View file
Open in desktop

Large diffs are not rendered by default.

5 changes: 5 additions & 0 deletions clippy_utils/src/ty.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1266,3 +1266,8 @@ pub fn normalize_with_regions<'tcx>(tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>
Err(_) => ty,
}
}

/// Checks if the type is `core::mem::ManuallyDrop<_>`
pub fn is_manually_drop(ty: Ty<'_>) -> bool {
ty.ty_adt_def().map_or(false, AdtDef::is_manually_drop)
}
45 changes: 34 additions & 11 deletions tests/ui/explicit_auto_deref.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,24 +301,47 @@ fn main() {
};

// Issue #11474
pub struct Variant {
pub anonymous: Variant0,
#[derive(Clone, Copy)]
struct Wrap<T>(T);
impl<T> core::ops::Deref for Wrap<T> {
type Target = T;
fn deref(&self) -> &T {
&self.0
}
}

pub union Variant0 {
pub anonymous: std::mem::ManuallyDrop<Variant00>,
impl<T> core::ops::DerefMut for Wrap<T> {
fn deref_mut(&mut self) -> &mut T {
&mut self.0
}
}

pub struct Variant00 {
pub anonymous: Variant000,
union U<T: Copy> {
u: T,
}

pub union Variant000 {
pub val: i32,
#[derive(Clone, Copy)]
struct S8 {
x: &'static str,
}

unsafe {
let mut p = core::mem::zeroed::<Variant>();
(*p.anonymous.anonymous).anonymous.val = 1;
let mut x = U {
u: core::mem::ManuallyDrop::new(S8 { x: "" }),
};
let _ = &mut (*x.u).x;
let _ = &mut { x.u }.x;
let _ = &mut ({ *x.u }).x;

let mut x = U {
u: Wrap(core::mem::ManuallyDrop::new(S8 { x: "" })),
};
let _ = &mut (*x.u).x;
let _ = &mut { x.u }.x;
let _ = &mut ({ **x.u }).x;

let mut x = U { u: Wrap(S8 { x: "" }) };
let _ = &mut x.u.x;
let _ = &mut { x.u }.x;
let _ = &mut ({ *x.u }).x;
}
}
45 changes: 34 additions & 11 deletions tests/ui/explicit_auto_deref.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,24 +301,47 @@ fn main() {
};

// Issue #11474
pub struct Variant {
pub anonymous: Variant0,
#[derive(Clone, Copy)]
struct Wrap<T>(T);
impl<T> core::ops::Deref for Wrap<T> {
type Target = T;
fn deref(&self) -> &T {
&self.0
}
}

pub union Variant0 {
pub anonymous: std::mem::ManuallyDrop<Variant00>,
impl<T> core::ops::DerefMut for Wrap<T> {
fn deref_mut(&mut self) -> &mut T {
&mut self.0
}
}

pub struct Variant00 {
pub anonymous: Variant000,
union U<T: Copy> {
u: T,
}

pub union Variant000 {
pub val: i32,
#[derive(Clone, Copy)]
struct S8 {
x: &'static str,
}

unsafe {
let mut p = core::mem::zeroed::<Variant>();
(*p.anonymous.anonymous).anonymous.val = 1;
let mut x = U {
u: core::mem::ManuallyDrop::new(S8 { x: "" }),
};
let _ = &mut (*x.u).x;
let _ = &mut (*{ x.u }).x;
let _ = &mut ({ *x.u }).x;

let mut x = U {
u: Wrap(core::mem::ManuallyDrop::new(S8 { x: "" })),
};
let _ = &mut (**x.u).x;
let _ = &mut (**{ x.u }).x;
let _ = &mut ({ **x.u }).x;

let mut x = U { u: Wrap(S8 { x: "" }) };
let _ = &mut (*x.u).x;
let _ = &mut (*{ x.u }).x;
let _ = &mut ({ *x.u }).x;
}
}
32 changes: 31 additions & 1 deletion tests/ui/explicit_auto_deref.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,5 +241,35 @@ error: deref which would be done by auto-deref
LL | Some(x) => &mut *x,
| ^^^^^^^ help: try: `x`

error: aborting due to 40 previous errors
error: deref which would be done by auto-deref
--> $DIR/explicit_auto_deref.rs:332:22
|
LL | let _ = &mut (*{ x.u }).x;
| ^^^^^^^^^^ help: try: `{ x.u }`

error: deref which would be done by auto-deref
--> $DIR/explicit_auto_deref.rs:338:22
|
LL | let _ = &mut (**x.u).x;
| ^^^^^^^ help: try: `(*x.u)`

error: deref which would be done by auto-deref
--> $DIR/explicit_auto_deref.rs:339:22
|
LL | let _ = &mut (**{ x.u }).x;
| ^^^^^^^^^^^ help: try: `{ x.u }`

error: deref which would be done by auto-deref
--> $DIR/explicit_auto_deref.rs:343:22
|
LL | let _ = &mut (*x.u).x;
| ^^^^^^ help: try: `x.u`

error: deref which would be done by auto-deref
--> $DIR/explicit_auto_deref.rs:344:22
|
LL | let _ = &mut (*{ x.u }).x;
| ^^^^^^^^^^ help: try: `{ x.u }`

error: aborting due to 45 previous errors

51 changes: 36 additions & 15 deletions tests/ui/needless_borrow.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Vec<i32>>,
#[derive(Clone, Copy)]
struct Wrap<T>(T);
impl<T> core::ops::Deref for Wrap<T> {
type Target = T;
fn deref(&self) -> &T {
&self.0
}
}
impl<T> core::ops::DerefMut for Wrap<T> {
fn deref_mut(&mut self) -> &mut T {
&mut self.0
}
}

union Ocean {
coral: ManuallyDrop<Coral>,
union U<T: Copy> {
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);

(*ocean.coral).crab = ManuallyDrop::new(vec![4, 5, 6]);
ManuallyDrop::drop(&mut (*ocean.coral).crab);

ManuallyDrop::drop(&mut ocean.coral);
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;

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;

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;
}
}

Expand Down
51 changes: 36 additions & 15 deletions tests/ui/needless_borrow.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Vec<i32>>,
#[derive(Clone, Copy)]
struct Wrap<T>(T);
impl<T> core::ops::Deref for Wrap<T> {
type Target = T;
fn deref(&self) -> &T {
&self.0
}
}
impl<T> core::ops::DerefMut for Wrap<T> {
fn deref_mut(&mut self) -> &mut T {
&mut self.0
}
}

union Ocean {
coral: ManuallyDrop<Coral>,
union U<T: Copy> {
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);

(*ocean.coral).crab = ManuallyDrop::new(vec![4, 5, 6]);
ManuallyDrop::drop(&mut (*ocean.coral).crab);

ManuallyDrop::drop(&mut ocean.coral);
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;

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;

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;
Comment on lines +220 to +234
Copy link
Member

Choose a reason for hiding this comment

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

Could you add //~ ERROR annotations?

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 thought we weren't going to enforce this until ui_test worked the way we wanted it.

Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, the conclusions from the Zulip thread talking about this were that we soft-mandated //~ERROR annotations (without a body). Even if they aren't obligated by the CI, I just find them to increase legibility 🐱

}
}

Expand Down
26 changes: 25 additions & 1 deletion tests/ui/needless_borrow.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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