Skip to content

Commit

Permalink
Auto merge of #13487 - Coekjan:fix-slice-size-calc-on-ref-ref, r=dswij
Browse files Browse the repository at this point in the history
Fix lint `manual_slice_size_calculation` when a slice is ref more than once

When a slice is ref more than once, current suggestion given by `manual_slice_size_calculation` is wrong. For example:

```rs
let s: &[i32] = &[1, 2][..];
let ss: &&[i32] = &s;  // <-----

let _ = size_of::<i32>() * ss.len();
```

clippy now suggests:

```patch
- let _ = size_of::<i32>() * ss.len();
+ let _ = size_of_val(ss);
```

However, this can result in calculating the size of `&[i32]`, instead of `[i32]` (this wrong suggestion also leads to `size_of_ref` warning: https://rust-lang.github.io/rust-clippy/master/index.html#/size_of_ref )

Now I am sending this PR to fix this bug, so that clippy will suggest (some deref added):

```patch
- let _ = size_of::<i32>() * ss.len();
+ let _ = size_of_val(*ss);
```

As I am not familiar with current clippy code-base, please correct me if I am not doing well or I can do it better :)

changelog: [`manual_slice_size_calculation`]: fix a bug when a slice is ref more than once.
  • Loading branch information
bors committed Oct 13, 2024
2 parents f883e28 + 2080634 commit 6a281e9
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 13 deletions.
14 changes: 8 additions & 6 deletions clippy_lints/src/manual_slice_size_calculation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,11 @@ impl<'tcx> LateLintPass<'tcx> for ManualSliceSizeCalculation {
&& !expr.span.from_expansion()
// Does not apply inside const because size_of_val is not cost in stable.
&& !is_in_const_context(cx)
&& let Some(receiver) = simplify(cx, left, right)
&& let Some((receiver, refs_count)) = simplify(cx, left, right)
{
let ctxt = expr.span.ctxt();
let mut app = Applicability::MachineApplicable;
let deref = "*".repeat(refs_count - 1);
let val_name = snippet_with_context(cx, receiver.span, ctxt, "slice", &mut app).0;
let Some(sugg) = std_or_core(cx) else { return };

Expand All @@ -58,7 +59,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualSliceSizeCalculation {
expr.span,
"manual slice size calculation",
"try",
format!("{sugg}::mem::size_of_val({val_name})"),
format!("{sugg}::mem::size_of_val({deref}{val_name})"),
app,
);
}
Expand All @@ -69,7 +70,7 @@ fn simplify<'tcx>(
cx: &LateContext<'tcx>,
expr1: &'tcx Expr<'tcx>,
expr2: &'tcx Expr<'tcx>,
) -> Option<&'tcx Expr<'tcx>> {
) -> Option<(&'tcx Expr<'tcx>, usize)> {
let expr1 = expr_or_init(cx, expr1);
let expr2 = expr_or_init(cx, expr2);

Expand All @@ -80,13 +81,14 @@ fn simplify_half<'tcx>(
cx: &LateContext<'tcx>,
expr1: &'tcx Expr<'tcx>,
expr2: &'tcx Expr<'tcx>,
) -> Option<&'tcx Expr<'tcx>> {
) -> Option<(&'tcx Expr<'tcx>, usize)> {
if !expr1.span.from_expansion()
// expr1 is `[T1].len()`?
&& let ExprKind::MethodCall(method_path, receiver, [], _) = expr1.kind
&& method_path.ident.name == sym::len
&& let receiver_ty = cx.typeck_results().expr_ty(receiver)
&& let ty::Slice(ty1) = receiver_ty.peel_refs().kind()
&& let (receiver_ty, refs_count) = clippy_utils::ty::walk_ptrs_ty_depth(receiver_ty)
&& let ty::Slice(ty1) = receiver_ty.kind()
// expr2 is `size_of::<T2>()`?
&& let ExprKind::Call(func, []) = expr2.kind
&& let ExprKind::Path(ref func_qpath) = func.kind
Expand All @@ -96,7 +98,7 @@ fn simplify_half<'tcx>(
// T1 == T2?
&& *ty1 == ty2
{
Some(receiver)
Some((receiver, refs_count))
} else {
None
}
Expand Down
4 changes: 4 additions & 0 deletions tests/ui/manual_slice_size_calculation.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,15 @@ use proc_macros::external;
fn main() {
let v_i32 = Vec::<i32>::new();
let s_i32 = v_i32.as_slice();
let s_i32_ref = &s_i32;
let s_i32_ref_ref = &s_i32_ref;

// True positives:
let _ = std::mem::size_of_val(s_i32); // WARNING
let _ = std::mem::size_of_val(s_i32); // WARNING
let _ = std::mem::size_of_val(s_i32) * 5; // WARNING
let _ = std::mem::size_of_val(*s_i32_ref); // WARNING
let _ = std::mem::size_of_val(**s_i32_ref_ref); // WARNING

let len = s_i32.len();
let size = size_of::<i32>();
Expand Down
4 changes: 4 additions & 0 deletions tests/ui/manual_slice_size_calculation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,15 @@ use proc_macros::external;
fn main() {
let v_i32 = Vec::<i32>::new();
let s_i32 = v_i32.as_slice();
let s_i32_ref = &s_i32;
let s_i32_ref_ref = &s_i32_ref;

// True positives:
let _ = s_i32.len() * size_of::<i32>(); // WARNING
let _ = size_of::<i32>() * s_i32.len(); // WARNING
let _ = size_of::<i32>() * s_i32.len() * 5; // WARNING
let _ = size_of::<i32>() * s_i32_ref.len(); // WARNING
let _ = size_of::<i32>() * s_i32_ref_ref.len(); // WARNING

let len = s_i32.len();
let size = size_of::<i32>();
Expand Down
26 changes: 19 additions & 7 deletions tests/ui/manual_slice_size_calculation.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: manual slice size calculation
--> tests/ui/manual_slice_size_calculation.rs:15:13
--> tests/ui/manual_slice_size_calculation.rs:17:13
|
LL | let _ = s_i32.len() * size_of::<i32>(); // WARNING
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `std::mem::size_of_val(s_i32)`
Expand All @@ -8,40 +8,52 @@ LL | let _ = s_i32.len() * size_of::<i32>(); // WARNING
= help: to override `-D warnings` add `#[allow(clippy::manual_slice_size_calculation)]`

error: manual slice size calculation
--> tests/ui/manual_slice_size_calculation.rs:16:13
--> tests/ui/manual_slice_size_calculation.rs:18:13
|
LL | let _ = size_of::<i32>() * s_i32.len(); // WARNING
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `std::mem::size_of_val(s_i32)`

error: manual slice size calculation
--> tests/ui/manual_slice_size_calculation.rs:17:13
--> tests/ui/manual_slice_size_calculation.rs:19:13
|
LL | let _ = size_of::<i32>() * s_i32.len() * 5; // WARNING
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `std::mem::size_of_val(s_i32)`

error: manual slice size calculation
--> tests/ui/manual_slice_size_calculation.rs:20:13
|
LL | let _ = size_of::<i32>() * s_i32_ref.len(); // WARNING
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `std::mem::size_of_val(*s_i32_ref)`

error: manual slice size calculation
--> tests/ui/manual_slice_size_calculation.rs:21:13
|
LL | let _ = size_of::<i32>() * s_i32_ref_ref.len(); // WARNING
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `std::mem::size_of_val(**s_i32_ref_ref)`

error: manual slice size calculation
--> tests/ui/manual_slice_size_calculation.rs:25:13
|
LL | let _ = len * size_of::<i32>(); // WARNING
| ^^^^^^^^^^^^^^^^^^^^^^ help: try: `std::mem::size_of_val(s_i32)`

error: manual slice size calculation
--> tests/ui/manual_slice_size_calculation.rs:22:13
--> tests/ui/manual_slice_size_calculation.rs:26:13
|
LL | let _ = s_i32.len() * size; // WARNING
| ^^^^^^^^^^^^^^^^^^ help: try: `std::mem::size_of_val(s_i32)`

error: manual slice size calculation
--> tests/ui/manual_slice_size_calculation.rs:23:13
--> tests/ui/manual_slice_size_calculation.rs:27:13
|
LL | let _ = len * size; // WARNING
| ^^^^^^^^^^ help: try: `std::mem::size_of_val(s_i32)`

error: manual slice size calculation
--> tests/ui/manual_slice_size_calculation.rs:25:13
--> tests/ui/manual_slice_size_calculation.rs:29:13
|
LL | let _ = external!(&[1u64][..]).len() * size_of::<u64>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `std::mem::size_of_val(external!(&[1u64][..]))`

error: aborting due to 7 previous errors
error: aborting due to 9 previous errors

0 comments on commit 6a281e9

Please sign in to comment.