From b66aa09b951dace249fad3376c2d97c95d11b6a9 Mon Sep 17 00:00:00 2001 From: Michael Schubart Date: Sun, 19 Mar 2023 09:29:32 +0000 Subject: [PATCH 1/3] Add [`manual_slice_size_calculation`] --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + .../src/manual_slice_size_calculation.rs | 90 +++++++++++++++++++ tests/ui/manual_slice_size_calculation.rs | 30 +++++++ tests/ui/manual_slice_size_calculation.stderr | 27 ++++++ 6 files changed, 151 insertions(+) create mode 100644 clippy_lints/src/manual_slice_size_calculation.rs create mode 100644 tests/ui/manual_slice_size_calculation.rs create mode 100644 tests/ui/manual_slice_size_calculation.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index ba10cb53ec9..f615b27bf68 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4674,6 +4674,7 @@ Released 2018-09-13 [`manual_rem_euclid`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_rem_euclid [`manual_retain`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_retain [`manual_saturating_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_saturating_arithmetic +[`manual_slice_size_calculation`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_slice_size_calculation [`manual_split_once`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_split_once [`manual_str_repeat`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_str_repeat [`manual_string_new`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_string_new diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index b4510557033..09ae6b8ee57 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -269,6 +269,7 @@ crate::manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE_INFO, crate::manual_rem_euclid::MANUAL_REM_EUCLID_INFO, crate::manual_retain::MANUAL_RETAIN_INFO, + crate::manual_slice_size_calculation::MANUAL_SLICE_SIZE_CALCULATION_INFO, crate::manual_string_new::MANUAL_STRING_NEW_INFO, crate::manual_strip::MANUAL_STRIP_INFO, crate::map_unit_fn::OPTION_MAP_UNIT_FN_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 82d63ddca6d..f842e629c53 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -186,6 +186,7 @@ mod manual_non_exhaustive; mod manual_rem_euclid; mod manual_retain; +mod manual_slice_size_calculation; mod manual_string_new; mod manual_strip; mod map_unit_fn; @@ -957,6 +958,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: }); store.register_late_pass(|_| Box::new(lines_filter_map_ok::LinesFilterMapOk)); store.register_late_pass(|_| Box::new(tests_outside_test_module::TestsOutsideTestModule)); + store.register_late_pass(|_| Box::new(manual_slice_size_calculation::ManualSliceSizeCalculation)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/manual_slice_size_calculation.rs b/clippy_lints/src/manual_slice_size_calculation.rs new file mode 100644 index 00000000000..2659f347778 --- /dev/null +++ b/clippy_lints/src/manual_slice_size_calculation.rs @@ -0,0 +1,90 @@ +use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::in_constant; +use rustc_hir::{BinOpKind, Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::symbol::sym; + +declare_clippy_lint! { + /// ### What it does + /// When `a` is `&[T]`, detect `a.len() * size_of::()` and suggest `size_of_val(a)` + /// instead. + /// + /// ### Why is this better? + /// * Shorter to write + /// * Removes the need for the human and the compiler to worry about overflow in the + /// multiplication + /// * Potentially faster at runtime as rust emits special no-wrapping flags when it + /// calculates the byte length + /// * Less turbofishing + /// + /// ### Example + /// ```rust + /// # let data : &[i32] = &[1, 2, 3]; + /// let newlen = data.len() * std::mem::size_of::(); + /// ``` + /// Use instead: + /// ```rust + /// # let data : &[i32] = &[1, 2, 3]; + /// let newlen = std::mem::size_of_val(data); + /// ``` + #[clippy::version = "1.70.0"] + pub MANUAL_SLICE_SIZE_CALCULATION, + complexity, + "manual slice size calculation" +} +declare_lint_pass!(ManualSliceSizeCalculation => [MANUAL_SLICE_SIZE_CALCULATION]); + +impl<'tcx> LateLintPass<'tcx> for ManualSliceSizeCalculation { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + // Does not apply inside const because size_of_value is not cost in stable. + if !in_constant(cx, expr.hir_id) + && let ExprKind::Binary(ref op, left, right) = expr.kind + && BinOpKind::Mul == op.node + && let Some(_receiver) = simplify(cx, left, right) + { + span_lint_and_help( + cx, + MANUAL_SLICE_SIZE_CALCULATION, + expr.span, + "manual slice size calculation", + None, + "consider using std::mem::size_of_value instead"); + } + } +} + +fn simplify<'tcx>( + cx: &LateContext<'tcx>, + expr1: &'tcx Expr<'tcx>, + expr2: &'tcx Expr<'tcx>, +) -> Option<&'tcx Expr<'tcx>> { + simplify_half(cx, expr1, expr2).or_else(|| simplify_half(cx, expr2, expr1)) +} + +fn simplify_half<'tcx>( + cx: &LateContext<'tcx>, + expr1: &'tcx Expr<'tcx>, + expr2: &'tcx Expr<'tcx>, +) -> Option<&'tcx Expr<'tcx>> { + if + // 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() + // expr2 is `size_of::()`? + && let ExprKind::Call(func, _) = expr2.kind + && let ExprKind::Path(ref func_qpath) = func.kind + && let Some(def_id) = cx.qpath_res(func_qpath, func.hir_id).opt_def_id() + && cx.tcx.is_diagnostic_item(sym::mem_size_of, def_id) + && let Some(ty2) = cx.typeck_results().node_substs(func.hir_id).types().next() + // T1 == T2? + && *ty1 == ty2 + { + Some(receiver) + } else { + None + } +} diff --git a/tests/ui/manual_slice_size_calculation.rs b/tests/ui/manual_slice_size_calculation.rs new file mode 100644 index 00000000000..8ec4ae0aed7 --- /dev/null +++ b/tests/ui/manual_slice_size_calculation.rs @@ -0,0 +1,30 @@ +#![allow(unused)] +#![warn(clippy::manual_slice_size_calculation)] + +use core::mem::{align_of, size_of}; + +fn main() { + let v_i32 = Vec::::new(); + let s_i32 = v_i32.as_slice(); + + // True positives: + let _ = s_i32.len() * size_of::(); // WARNING + let _ = size_of::() * s_i32.len(); // WARNING + let _ = size_of::() * s_i32.len() * 5; // WARNING + + // True negatives: + let _ = size_of::() + s_i32.len(); // Ok, not a multiplication + let _ = size_of::() * s_i32.partition_point(|_| true); // Ok, not len() + let _ = size_of::() * v_i32.len(); // Ok, not a slice + let _ = align_of::() * s_i32.len(); // Ok, not size_of() + let _ = size_of::() * s_i32.len(); // Ok, different types + + // False negatives: + let _ = 5 * size_of::() * s_i32.len(); // Ok (MISSED OPPORTUNITY) + let _ = size_of::() * 5 * s_i32.len(); // Ok (MISSED OPPORTUNITY) +} + +const fn _const(s_i32: &[i32]) { + // True negative: + let _ = s_i32.len() * size_of::(); // Ok, can't use size_of_val in const +} diff --git a/tests/ui/manual_slice_size_calculation.stderr b/tests/ui/manual_slice_size_calculation.stderr new file mode 100644 index 00000000000..33de9fad4d3 --- /dev/null +++ b/tests/ui/manual_slice_size_calculation.stderr @@ -0,0 +1,27 @@ +error: manual slice size calculation + --> $DIR/manual_slice_size_calculation.rs:11:13 + | +LL | let _ = s_i32.len() * size_of::(); // WARNING + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using std::mem::size_of_value instead + = note: `-D clippy::manual-slice-size-calculation` implied by `-D warnings` + +error: manual slice size calculation + --> $DIR/manual_slice_size_calculation.rs:12:13 + | +LL | let _ = size_of::() * s_i32.len(); // WARNING + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using std::mem::size_of_value instead + +error: manual slice size calculation + --> $DIR/manual_slice_size_calculation.rs:13:13 + | +LL | let _ = size_of::() * s_i32.len() * 5; // WARNING + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using std::mem::size_of_value instead + +error: aborting due to 3 previous errors + From b47a322ef11e541231b18c67dfe133dac8764b11 Mon Sep 17 00:00:00 2001 From: Michael Schubart Date: Thu, 6 Apr 2023 13:45:50 +0100 Subject: [PATCH 2/3] Add tests suggested by @llogiq --- tests/ui/manual_slice_size_calculation.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/ui/manual_slice_size_calculation.rs b/tests/ui/manual_slice_size_calculation.rs index 8ec4ae0aed7..2cb8c4c2ad3 100644 --- a/tests/ui/manual_slice_size_calculation.rs +++ b/tests/ui/manual_slice_size_calculation.rs @@ -22,6 +22,12 @@ fn main() { // False negatives: let _ = 5 * size_of::() * s_i32.len(); // Ok (MISSED OPPORTUNITY) let _ = size_of::() * 5 * s_i32.len(); // Ok (MISSED OPPORTUNITY) + + let len = s_i32.len(); + let size = size_of::(); + let _ = len * size_of::(); // Ok (MISSED OPPORTUNITY) + let _ = s_i32.len() * size; // Ok (MISSED OPPORTUNITY) + let _ = len * size; // Ok (MISSED OPPORTUNITY) } const fn _const(s_i32: &[i32]) { From b1c784d31f3361b3093466d61b8e62c997b1d086 Mon Sep 17 00:00:00 2001 From: Michael Schubart Date: Fri, 7 Apr 2023 08:00:53 +0900 Subject: [PATCH 3/3] Fix false negatives by using `expr_or_init` --- .../src/manual_slice_size_calculation.rs | 5 +++- tests/ui/manual_slice_size_calculation.rs | 12 ++++----- tests/ui/manual_slice_size_calculation.stderr | 26 ++++++++++++++++++- 3 files changed, 35 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/manual_slice_size_calculation.rs b/clippy_lints/src/manual_slice_size_calculation.rs index 2659f347778..92ee79453a3 100644 --- a/clippy_lints/src/manual_slice_size_calculation.rs +++ b/clippy_lints/src/manual_slice_size_calculation.rs @@ -1,5 +1,5 @@ use clippy_utils::diagnostics::span_lint_and_help; -use clippy_utils::in_constant; +use clippy_utils::{expr_or_init, in_constant}; use rustc_hir::{BinOpKind, Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty; @@ -60,6 +60,9 @@ fn simplify<'tcx>( expr1: &'tcx Expr<'tcx>, expr2: &'tcx Expr<'tcx>, ) -> Option<&'tcx Expr<'tcx>> { + let expr1 = expr_or_init(cx, expr1); + let expr2 = expr_or_init(cx, expr2); + simplify_half(cx, expr1, expr2).or_else(|| simplify_half(cx, expr2, expr1)) } diff --git a/tests/ui/manual_slice_size_calculation.rs b/tests/ui/manual_slice_size_calculation.rs index 2cb8c4c2ad3..5082f931f3c 100644 --- a/tests/ui/manual_slice_size_calculation.rs +++ b/tests/ui/manual_slice_size_calculation.rs @@ -12,6 +12,12 @@ fn main() { let _ = size_of::() * s_i32.len(); // WARNING let _ = size_of::() * s_i32.len() * 5; // WARNING + let len = s_i32.len(); + let size = size_of::(); + let _ = len * size_of::(); // WARNING + let _ = s_i32.len() * size; // WARNING + let _ = len * size; // WARNING + // True negatives: let _ = size_of::() + s_i32.len(); // Ok, not a multiplication let _ = size_of::() * s_i32.partition_point(|_| true); // Ok, not len() @@ -22,12 +28,6 @@ fn main() { // False negatives: let _ = 5 * size_of::() * s_i32.len(); // Ok (MISSED OPPORTUNITY) let _ = size_of::() * 5 * s_i32.len(); // Ok (MISSED OPPORTUNITY) - - let len = s_i32.len(); - let size = size_of::(); - let _ = len * size_of::(); // Ok (MISSED OPPORTUNITY) - let _ = s_i32.len() * size; // Ok (MISSED OPPORTUNITY) - let _ = len * size; // Ok (MISSED OPPORTUNITY) } const fn _const(s_i32: &[i32]) { diff --git a/tests/ui/manual_slice_size_calculation.stderr b/tests/ui/manual_slice_size_calculation.stderr index 33de9fad4d3..4a24fc60a0f 100644 --- a/tests/ui/manual_slice_size_calculation.stderr +++ b/tests/ui/manual_slice_size_calculation.stderr @@ -23,5 +23,29 @@ LL | let _ = size_of::() * s_i32.len() * 5; // WARNING | = help: consider using std::mem::size_of_value instead -error: aborting due to 3 previous errors +error: manual slice size calculation + --> $DIR/manual_slice_size_calculation.rs:17:13 + | +LL | let _ = len * size_of::(); // WARNING + | ^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using std::mem::size_of_value instead + +error: manual slice size calculation + --> $DIR/manual_slice_size_calculation.rs:18:13 + | +LL | let _ = s_i32.len() * size; // WARNING + | ^^^^^^^^^^^^^^^^^^ + | + = help: consider using std::mem::size_of_value instead + +error: manual slice size calculation + --> $DIR/manual_slice_size_calculation.rs:19:13 + | +LL | let _ = len * size; // WARNING + | ^^^^^^^^^^ + | + = help: consider using std::mem::size_of_value instead + +error: aborting due to 6 previous errors