diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c798a3c2e5a..13a019cdb3a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7052,6 +7052,7 @@ Released 2018-09-13 [`suspicious_op_assign_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_op_assign_impl [`suspicious_open_options`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_open_options [`suspicious_operation_groupings`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_operation_groupings +[`suspicious_slice_copies`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_slice_copies [`suspicious_splitn`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_splitn [`suspicious_to_owned`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_to_owned [`suspicious_unary_op_formatting`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_unary_op_formatting diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index e16b194c0cad..32b82c99e44e 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -476,6 +476,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::methods::SUSPICIOUS_COMMAND_ARG_SPACE_INFO, crate::methods::SUSPICIOUS_MAP_INFO, crate::methods::SUSPICIOUS_OPEN_OPTIONS_INFO, + crate::methods::SUSPICIOUS_SLICE_COPIES_INFO, crate::methods::SUSPICIOUS_SPLITN_INFO, crate::methods::SUSPICIOUS_TO_OWNED_INFO, crate::methods::SWAP_WITH_TEMPORARY_INFO, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 0ed166f8dd5b..f607f1108fb2 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -117,6 +117,7 @@ mod string_extend_chars; mod string_lit_chars_any; mod suspicious_command_arg_space; mod suspicious_map; +mod suspicious_slice_copies; mod suspicious_splitn; mod suspicious_to_owned; mod swap_with_temporary; @@ -3754,6 +3755,42 @@ declare_clippy_lint! { "suspicious combination of options for opening a file" } +declare_clippy_lint! { + /// ### What it does + /// Checks for taking a mutable reference to a temporary array created by `arr[N..M].try_into()` + /// instead of borrowing the original slice data. + /// + /// ### Why is this bad? + /// Slices have two relevant implementations of the `TryInto` trait: + /// 1. `TryInto<[T; N]>`, which creates a new array by copying elements from the slice + /// 2. `TryInto<&'a mut [T; N]>`, which creates a mutable array reference from slice + /// + /// When writing `&mut arr[N..M].try_into().unwrap()`, the compiler selects the first implementation + /// and creates a new array. As a result, any mutations are applied to this temporary array, while + /// the original data remains unchanged. This is usually unintended. + /// + /// ### Example + /// ```no_run + /// let mut arr: [i32; 5] = [0; 5]; + /// let arr_mut_ref: &mut [i32; 3] = &mut arr[..3].try_into().unwrap(); //copied + /// arr_mut_ref[0] = 1; + /// + /// assert_eq!(arr[0], 0); + /// ``` + /// Use instead: + /// ```no_run + /// let mut arr: [i32; 5] = [0; 5]; + /// let arr_mut_ref: &mut [i32; 3] = (&mut arr[..3]).try_into().unwrap(); //borrowed + /// arr_mut_ref[0] = 1; + /// + /// assert_eq!(arr[0], 1); + /// ``` + #[clippy::version = "1.96.0"] + pub SUSPICIOUS_SLICE_COPIES, + suspicious, + "detects taking a `&mut` to a temporary array created from a slice" +} + declare_clippy_lint! { /// ### What it does /// Checks for calls to [`splitn`] @@ -4873,6 +4910,7 @@ impl_lint_pass!(Methods => [ SUSPICIOUS_COMMAND_ARG_SPACE, SUSPICIOUS_MAP, SUSPICIOUS_OPEN_OPTIONS, + SUSPICIOUS_SLICE_COPIES, SUSPICIOUS_SPLITN, SUSPICIOUS_TO_OWNED, SWAP_WITH_TEMPORARY, @@ -5618,6 +5656,7 @@ impl Methods { }, (sym::try_into, []) if cx.ty_based_def(expr).opt_parent(cx).is_diag_item(cx, sym::TryInto) => { unnecessary_fallible_conversions::check_method(cx, expr); + suspicious_slice_copies::check(cx, expr, recv, call_span, self.msrv); }, (sym::to_owned, []) => { diff --git a/clippy_lints/src/methods/suspicious_slice_copies.rs b/clippy_lints/src/methods/suspicious_slice_copies.rs new file mode 100644 index 000000000000..5020318135b0 --- /dev/null +++ b/clippy_lints/src/methods/suspicious_slice_copies.rs @@ -0,0 +1,110 @@ +use super::SUSPICIOUS_SLICE_COPIES; +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::msrvs::Msrv; +use clippy_utils::source::{IntoSpan, SpanRangeExt, snippet_with_context}; +use clippy_utils::{msrvs, sym}; +use rustc_ast::{BorrowKind, Mutability}; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind, Node}; +use rustc_lint::LateContext; +use rustc_middle::ty; +use rustc_middle::ty::{GenericArgKind, Ty}; +use rustc_span::Span; + +enum TryIntoReceiver { + Slice, + SliceMutRef, +} + +impl TryIntoReceiver { + fn from_ty(ty: Ty<'_>) -> Option { + match ty.kind() { + ty::Slice(_) => Some(TryIntoReceiver::Slice), + ty::Ref(_, ref_ty, Mutability::Mut) if let ty::Slice(_) = ref_ty.kind() => { + Some(TryIntoReceiver::SliceMutRef) + }, + _ => None, + } + } +} + +pub(super) fn check( + cx: &LateContext<'_>, + try_into_expr: &Expr<'_>, + recv_expr: &Expr<'_>, + try_into_span: Span, + msrv: Msrv, +) { + if let Some(receiver) = TryIntoReceiver::from_ty(cx.typeck_results().expr_ty(recv_expr)) + + // try_into was instantiated with [T; N] + && let [_, dst_ty] = &**cx.typeck_results().node_args(try_into_expr.hir_id) + && let GenericArgKind::Type(arg_ty) = dst_ty.kind() + && let ty::Array(..) = arg_ty.kind() + + && let mut parent_iter = cx.tcx.hir_parent_iter(try_into_expr.hir_id) + + // calls unwrap() or expect() + && let Some((_, Node::Expr(unwrap_expr))) = parent_iter.next() + && let ExprKind::MethodCall(path, .., unwrap_span) = unwrap_expr.kind + && let sym::unwrap | sym::expect = path.ident.name + + // use mut ref + && let Some((_, Node::Expr(mut_ref_expr))) = parent_iter.next() + && let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, inner_expr) = mut_ref_expr.kind + { + let span = mut_ref_expr.span.with_hi(inner_expr.span.hi()); + + span_lint_and_then( + cx, + SUSPICIOUS_SLICE_COPIES, + span, + "using a mutable reference to temporary array", + |diag| { + let mut applicability = Applicability::MaybeIncorrect; + + let (recv_snippet, _) = + snippet_with_context(cx, recv_expr.span, try_into_span.ctxt(), "..", &mut applicability); + let (unwrap_snippet, _) = + snippet_with_context(cx, unwrap_span, try_into_span.ctxt(), "..", &mut applicability); + let (mut_ref_snippet, _) = snippet_with_context( + cx, + mut_ref_expr + .span + .until(inner_expr.span.with_leading_whitespace(cx).into_span()), + try_into_span.ctxt(), + "..", + &mut applicability, + ); + + let borrow_sugg = if msrv.meets(cx, msrvs::SLICE_AS_MUT_ARRAY) { + format!("{recv_snippet}.as_mut_array().{unwrap_snippet}") + } else { + match receiver { + TryIntoReceiver::Slice => { + format!("({mut_ref_snippet} {recv_snippet}).try_into().{unwrap_snippet}") + }, + TryIntoReceiver::SliceMutRef => format!("{recv_snippet}.try_into().{unwrap_snippet}"), + } + }; + + let copy_sugg = match receiver { + TryIntoReceiver::Slice => { + format!("{mut_ref_snippet} <[_; _]>::try_from(&{recv_snippet}).{unwrap_snippet}") + }, + TryIntoReceiver::SliceMutRef => { + format!("{mut_ref_snippet} <[_; _]>::try_from({recv_snippet}).{unwrap_snippet}") + }, + }; + + diag.span_suggestion(span, "borrow the slice as an array", borrow_sugg, applicability); + diag.span_suggestion( + span, + "explicitly create a new array using `TryFrom`", + copy_sugg, + applicability, + ); + }, + ); + } +} diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index 5aa457b9d19c..ce3076ea5f11 100644 --- a/clippy_utils/src/msrvs.rs +++ b/clippy_utils/src/msrvs.rs @@ -84,6 +84,7 @@ msrv_aliases! { 1,15,0 { MAYBE_BOUND_IN_WHERE } 1,13,0 { QUESTION_MARK_OPERATOR } 1,3,0 { DURATION_FROM_MILLIS_SECS } + 1,93,0 { SLICE_AS_MUT_ARRAY } } /// `#[clippy::msrv]` attributes are rarely used outside of Clippy's test suite, as a basic diff --git a/tests/ui/suspicious_slice_copies.1.fixed b/tests/ui/suspicious_slice_copies.1.fixed new file mode 100644 index 000000000000..cd6e2f54c92c --- /dev/null +++ b/tests/ui/suspicious_slice_copies.1.fixed @@ -0,0 +1,58 @@ +#![warn(clippy::suspicious_slice_copies)] + +const K: usize = 3; +const N: usize = 5; + +fn arr_incr(arr: &mut [i32; K]) { + for x in arr.iter_mut() { + *x += 1; + } +} + +#[clippy::msrv = "1.92.0"] +fn test_msrv_1_92() { + let mut arr: [i32; N] = [0; 5]; + let slice: &mut [i32] = &mut arr[..K]; + + let mut_arr_ref: &mut [i32; K] = slice.try_into().unwrap(); + //~^ suspicious_slice_copies + + let mut_arr_ref: &mut [i32; K] = (&mut arr[..K]).try_into().unwrap(); + //~^ suspicious_slice_copies + + let mut_arr_ref: &mut [i32; K] = (&mut arr[..K]).try_into().unwrap(); + //~^ suspicious_slice_copies + + arr_incr((&mut arr[..K]).try_into().expect("never happen")); + //~^ suspicious_slice_copies +} + +#[clippy::msrv = "1.93.0"] +fn test_msrv_1_93() { + let mut arr: [i32; N] = [0; 5]; + let slice: &mut [i32] = &mut arr[..K]; + + let mut_arr_ref: &mut [i32; K] = slice.as_mut_array().unwrap(); + //~^ suspicious_slice_copies + + let mut_arr_ref: &mut [i32; K] = arr[..K].as_mut_array().unwrap(); + //~^ suspicious_slice_copies + + let mut_arr_ref: &mut [i32; K] = arr[..K].as_mut_array().unwrap(); + //~^ suspicious_slice_copies + + arr_incr(arr[..K].as_mut_array().expect("never happen")); + //~^ suspicious_slice_copies +} + +fn main() { + let mut arr: [i32; N] = [0; 5]; + + let arr_ref: &[i32; K] = &arr[..K].try_into().unwrap(); + let arr_mut_ref: &mut [i32; K] = (&mut arr[..K]).try_into().unwrap(); + + let arr_copy: [i32; K] = arr[..K].try_into().unwrap(); + + let slice: &[i32] = &arr[..K]; + let arr_mut_ref: &mut [i32; K] = &mut slice.try_into().unwrap(); +} diff --git a/tests/ui/suspicious_slice_copies.2.fixed b/tests/ui/suspicious_slice_copies.2.fixed new file mode 100644 index 000000000000..d9cc296b705d --- /dev/null +++ b/tests/ui/suspicious_slice_copies.2.fixed @@ -0,0 +1,58 @@ +#![warn(clippy::suspicious_slice_copies)] + +const K: usize = 3; +const N: usize = 5; + +fn arr_incr(arr: &mut [i32; K]) { + for x in arr.iter_mut() { + *x += 1; + } +} + +#[clippy::msrv = "1.92.0"] +fn test_msrv_1_92() { + let mut arr: [i32; N] = [0; 5]; + let slice: &mut [i32] = &mut arr[..K]; + + let mut_arr_ref: &mut [i32; K] = &mut <[_; _]>::try_from(slice).unwrap(); + //~^ suspicious_slice_copies + + let mut_arr_ref: &mut [i32; K] = &mut <[_; _]>::try_from(&arr[..K]).unwrap(); + //~^ suspicious_slice_copies + + let mut_arr_ref: &mut [i32; K] = &mut <[_; _]>::try_from(&arr[..K]).unwrap(); + //~^ suspicious_slice_copies + + arr_incr(&mut <[_; _]>::try_from(&arr[..K]).expect("never happen")); + //~^ suspicious_slice_copies +} + +#[clippy::msrv = "1.93.0"] +fn test_msrv_1_93() { + let mut arr: [i32; N] = [0; 5]; + let slice: &mut [i32] = &mut arr[..K]; + + let mut_arr_ref: &mut [i32; K] = &mut <[_; _]>::try_from(slice).unwrap(); + //~^ suspicious_slice_copies + + let mut_arr_ref: &mut [i32; K] = &mut <[_; _]>::try_from(&arr[..K]).unwrap(); + //~^ suspicious_slice_copies + + let mut_arr_ref: &mut [i32; K] = &mut <[_; _]>::try_from(&arr[..K]).unwrap(); + //~^ suspicious_slice_copies + + arr_incr(&mut <[_; _]>::try_from(&arr[..K]).expect("never happen")); + //~^ suspicious_slice_copies +} + +fn main() { + let mut arr: [i32; N] = [0; 5]; + + let arr_ref: &[i32; K] = &arr[..K].try_into().unwrap(); + let arr_mut_ref: &mut [i32; K] = (&mut arr[..K]).try_into().unwrap(); + + let arr_copy: [i32; K] = arr[..K].try_into().unwrap(); + + let slice: &[i32] = &arr[..K]; + let arr_mut_ref: &mut [i32; K] = &mut slice.try_into().unwrap(); +} diff --git a/tests/ui/suspicious_slice_copies.rs b/tests/ui/suspicious_slice_copies.rs new file mode 100644 index 000000000000..fe9f74ba81a8 --- /dev/null +++ b/tests/ui/suspicious_slice_copies.rs @@ -0,0 +1,58 @@ +#![warn(clippy::suspicious_slice_copies)] + +const K: usize = 3; +const N: usize = 5; + +fn arr_incr(arr: &mut [i32; K]) { + for x in arr.iter_mut() { + *x += 1; + } +} + +#[clippy::msrv = "1.92.0"] +fn test_msrv_1_92() { + let mut arr: [i32; N] = [0; 5]; + let slice: &mut [i32] = &mut arr[..K]; + + let mut_arr_ref: &mut [i32; K] = &mut slice.try_into().unwrap(); + //~^ suspicious_slice_copies + + let mut_arr_ref: &mut [i32; K] = &mut arr[..K].try_into().unwrap(); + //~^ suspicious_slice_copies + + let mut_arr_ref: &mut [i32; K] = &mut (arr[..K].try_into().unwrap()); + //~^ suspicious_slice_copies + + arr_incr(&mut arr[..K].try_into().expect("never happen")); + //~^ suspicious_slice_copies +} + +#[clippy::msrv = "1.93.0"] +fn test_msrv_1_93() { + let mut arr: [i32; N] = [0; 5]; + let slice: &mut [i32] = &mut arr[..K]; + + let mut_arr_ref: &mut [i32; K] = &mut slice.try_into().unwrap(); + //~^ suspicious_slice_copies + + let mut_arr_ref: &mut [i32; K] = &mut arr[..K].try_into().unwrap(); + //~^ suspicious_slice_copies + + let mut_arr_ref: &mut [i32; K] = &mut (arr[..K].try_into().unwrap()); + //~^ suspicious_slice_copies + + arr_incr(&mut arr[..K].try_into().expect("never happen")); + //~^ suspicious_slice_copies +} + +fn main() { + let mut arr: [i32; N] = [0; 5]; + + let arr_ref: &[i32; K] = &arr[..K].try_into().unwrap(); + let arr_mut_ref: &mut [i32; K] = (&mut arr[..K]).try_into().unwrap(); + + let arr_copy: [i32; K] = arr[..K].try_into().unwrap(); + + let slice: &[i32] = &arr[..K]; + let arr_mut_ref: &mut [i32; K] = &mut slice.try_into().unwrap(); +} diff --git a/tests/ui/suspicious_slice_copies.stderr b/tests/ui/suspicious_slice_copies.stderr new file mode 100644 index 000000000000..5ca90166c37e --- /dev/null +++ b/tests/ui/suspicious_slice_copies.stderr @@ -0,0 +1,140 @@ +error: using a mutable reference to temporary array + --> tests/ui/suspicious_slice_copies.rs:17:38 + | +LL | let mut_arr_ref: &mut [i32; K] = &mut slice.try_into().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::suspicious-slice-copies` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::suspicious_slice_copies)]` +help: borrow the slice as an array + | +LL - let mut_arr_ref: &mut [i32; K] = &mut slice.try_into().unwrap(); +LL + let mut_arr_ref: &mut [i32; K] = slice.try_into().unwrap(); + | +help: explicitly create a new array using `TryFrom` + | +LL - let mut_arr_ref: &mut [i32; K] = &mut slice.try_into().unwrap(); +LL + let mut_arr_ref: &mut [i32; K] = &mut <[_; _]>::try_from(slice).unwrap(); + | + +error: using a mutable reference to temporary array + --> tests/ui/suspicious_slice_copies.rs:20:38 + | +LL | let mut_arr_ref: &mut [i32; K] = &mut arr[..K].try_into().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: borrow the slice as an array + | +LL - let mut_arr_ref: &mut [i32; K] = &mut arr[..K].try_into().unwrap(); +LL + let mut_arr_ref: &mut [i32; K] = (&mut arr[..K]).try_into().unwrap(); + | +help: explicitly create a new array using `TryFrom` + | +LL - let mut_arr_ref: &mut [i32; K] = &mut arr[..K].try_into().unwrap(); +LL + let mut_arr_ref: &mut [i32; K] = &mut <[_; _]>::try_from(&arr[..K]).unwrap(); + | + +error: using a mutable reference to temporary array + --> tests/ui/suspicious_slice_copies.rs:23:38 + | +LL | let mut_arr_ref: &mut [i32; K] = &mut (arr[..K].try_into().unwrap()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: borrow the slice as an array + | +LL - let mut_arr_ref: &mut [i32; K] = &mut (arr[..K].try_into().unwrap()); +LL + let mut_arr_ref: &mut [i32; K] = (&mut arr[..K]).try_into().unwrap(); + | +help: explicitly create a new array using `TryFrom` + | +LL - let mut_arr_ref: &mut [i32; K] = &mut (arr[..K].try_into().unwrap()); +LL + let mut_arr_ref: &mut [i32; K] = &mut <[_; _]>::try_from(&arr[..K]).unwrap(); + | + +error: using a mutable reference to temporary array + --> tests/ui/suspicious_slice_copies.rs:26:14 + | +LL | arr_incr(&mut arr[..K].try_into().expect("never happen")); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: borrow the slice as an array + | +LL - arr_incr(&mut arr[..K].try_into().expect("never happen")); +LL + arr_incr((&mut arr[..K]).try_into().expect("never happen")); + | +help: explicitly create a new array using `TryFrom` + | +LL - arr_incr(&mut arr[..K].try_into().expect("never happen")); +LL + arr_incr(&mut <[_; _]>::try_from(&arr[..K]).expect("never happen")); + | + +error: using a mutable reference to temporary array + --> tests/ui/suspicious_slice_copies.rs:35:38 + | +LL | let mut_arr_ref: &mut [i32; K] = &mut slice.try_into().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: borrow the slice as an array + | +LL - let mut_arr_ref: &mut [i32; K] = &mut slice.try_into().unwrap(); +LL + let mut_arr_ref: &mut [i32; K] = slice.as_mut_array().unwrap(); + | +help: explicitly create a new array using `TryFrom` + | +LL - let mut_arr_ref: &mut [i32; K] = &mut slice.try_into().unwrap(); +LL + let mut_arr_ref: &mut [i32; K] = &mut <[_; _]>::try_from(slice).unwrap(); + | + +error: using a mutable reference to temporary array + --> tests/ui/suspicious_slice_copies.rs:38:38 + | +LL | let mut_arr_ref: &mut [i32; K] = &mut arr[..K].try_into().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: borrow the slice as an array + | +LL - let mut_arr_ref: &mut [i32; K] = &mut arr[..K].try_into().unwrap(); +LL + let mut_arr_ref: &mut [i32; K] = arr[..K].as_mut_array().unwrap(); + | +help: explicitly create a new array using `TryFrom` + | +LL - let mut_arr_ref: &mut [i32; K] = &mut arr[..K].try_into().unwrap(); +LL + let mut_arr_ref: &mut [i32; K] = &mut <[_; _]>::try_from(&arr[..K]).unwrap(); + | + +error: using a mutable reference to temporary array + --> tests/ui/suspicious_slice_copies.rs:41:38 + | +LL | let mut_arr_ref: &mut [i32; K] = &mut (arr[..K].try_into().unwrap()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: borrow the slice as an array + | +LL - let mut_arr_ref: &mut [i32; K] = &mut (arr[..K].try_into().unwrap()); +LL + let mut_arr_ref: &mut [i32; K] = arr[..K].as_mut_array().unwrap(); + | +help: explicitly create a new array using `TryFrom` + | +LL - let mut_arr_ref: &mut [i32; K] = &mut (arr[..K].try_into().unwrap()); +LL + let mut_arr_ref: &mut [i32; K] = &mut <[_; _]>::try_from(&arr[..K]).unwrap(); + | + +error: using a mutable reference to temporary array + --> tests/ui/suspicious_slice_copies.rs:44:14 + | +LL | arr_incr(&mut arr[..K].try_into().expect("never happen")); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: borrow the slice as an array + | +LL - arr_incr(&mut arr[..K].try_into().expect("never happen")); +LL + arr_incr(arr[..K].as_mut_array().expect("never happen")); + | +help: explicitly create a new array using `TryFrom` + | +LL - arr_incr(&mut arr[..K].try_into().expect("never happen")); +LL + arr_incr(&mut <[_; _]>::try_from(&arr[..K]).expect("never happen")); + | + +error: aborting due to 8 previous errors +