diff options
| -rw-r--r-- | clippy_lints/src/tuple_array_conversions.rs | 314 | ||||
| -rw-r--r-- | tests/ui/tuple_array_conversions.rs | 30 | ||||
| -rw-r--r-- | tests/ui/tuple_array_conversions.stderr | 36 |
3 files changed, 195 insertions, 185 deletions
diff --git a/clippy_lints/src/tuple_array_conversions.rs b/clippy_lints/src/tuple_array_conversions.rs index 81270101538..7eec6982092 100644 --- a/clippy_lints/src/tuple_array_conversions.rs +++ b/clippy_lints/src/tuple_array_conversions.rs @@ -1,21 +1,29 @@ use clippy_utils::diagnostics::span_lint_and_help; use clippy_utils::msrvs::{self, Msrv}; +use clippy_utils::visitors::for_each_local_use_after_expr; use clippy_utils::{is_from_proc_macro, path_to_local}; +use itertools::Itertools; use rustc_ast::LitKind; -use rustc_hir::{Expr, ExprKind, HirId, Node, Pat}; +use rustc_hir::{Expr, ExprKind, Node, PatKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; -use rustc_middle::ty; +use rustc_middle::ty::{self, Ty}; use rustc_session::{declare_tool_lint, impl_lint_pass}; use std::iter::once; +use std::ops::ControlFlow; declare_clippy_lint! { /// ### What it does /// Checks for tuple<=>array conversions that are not done with `.into()`. /// /// ### Why is this bad? - /// It may be unnecessary complexity. `.into()` works for converting tuples - /// <=> arrays of up to 12 elements and may convey intent more clearly. + /// It may be unnecessary complexity. `.into()` works for converting tuples<=> arrays of up to + /// 12 elements and conveys the intent more clearly, while also leaving less room for hard to + /// spot bugs! + /// + /// ### Known issues + /// The suggested code may hide potential asymmetry in some cases. See + /// [#11085](https://github.com/rust-lang/rust-clippy/issues/11085) for more info. /// /// ### Example /// ```rust,ignore @@ -41,130 +49,152 @@ pub struct TupleArrayConversions { impl LateLintPass<'_> for TupleArrayConversions { fn check_expr<'tcx>(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { - if !in_external_macro(cx.sess(), expr.span) && self.msrv.meets(msrvs::TUPLE_ARRAY_CONVERSIONS) { - match expr.kind { - ExprKind::Array(elements) if (1..=12).contains(&elements.len()) => check_array(cx, expr, elements), - ExprKind::Tup(elements) if (1..=12).contains(&elements.len()) => check_tuple(cx, expr, elements), - _ => {}, - } + if in_external_macro(cx.sess(), expr.span) || !self.msrv.meets(msrvs::TUPLE_ARRAY_CONVERSIONS) { + return; + } + + match expr.kind { + ExprKind::Array(elements) if (1..=12).contains(&elements.len()) => check_array(cx, expr, elements), + ExprKind::Tup(elements) if (1..=12).contains(&elements.len()) => check_tuple(cx, expr, elements), + _ => {}, } } extract_msrv_attr!(LateContext); } -#[expect( - clippy::blocks_in_if_conditions, - reason = "not a FP, but this is much easier to understand" -)] fn check_array<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, elements: &'tcx [Expr<'tcx>]) { - if should_lint( - cx, - elements, - // This is cursed. - Some, - |(first_id, local)| { - if let Node::Pat(pat) = local - && let parent = parent_pat(cx, pat) - && parent.hir_id == first_id - { - return matches!( - cx.typeck_results().pat_ty(parent).peel_refs().kind(), - ty::Tuple(len) if len.len() == elements.len() - ); - } - - false - }, - ) || should_lint( - cx, - elements, - |(i, expr)| { - if let ExprKind::Field(path, field) = expr.kind && field.as_str() == i.to_string() { - return Some((i, path)); - }; - - None - }, - |(first_id, local)| { - if let Node::Pat(pat) = local - && let parent = parent_pat(cx, pat) - && parent.hir_id == first_id - { - return matches!( - cx.typeck_results().pat_ty(parent).peel_refs().kind(), - ty::Tuple(len) if len.len() == elements.len() - ); - } + let (ty::Array(ty, _) | ty::Slice(ty)) = cx.typeck_results().expr_ty(expr).kind() else { + unreachable!("`expr` must be an array or slice due to `ExprKind::Array`"); + }; + + if let [first, ..] = elements + && let Some(locals) = (match first.kind { + ExprKind::Field(_, _) => elements + .iter() + .enumerate() + .map(|(i, f)| -> Option<&'tcx Expr<'tcx>> { + let ExprKind::Field(lhs, ident) = f.kind else { + return None; + }; + (ident.name.as_str() == i.to_string()).then_some(lhs) + }) + .collect::<Option<Vec<_>>>(), + ExprKind::Path(_) => Some(elements.iter().collect()), + _ => None, + }) + && all_bindings_are_for_conv(cx, &[*ty], expr, elements, &locals, ToType::Array) + && !is_from_proc_macro(cx, expr) + { + span_lint_and_help( + cx, + TUPLE_ARRAY_CONVERSIONS, + expr.span, + "it looks like you're trying to convert a tuple to an array", + None, + "use `.into()` instead, or `<[T; N]>::from` if type annotations are needed", + ); + } +} - false - }, - ) { - emit_lint(cx, expr, ToType::Array); +fn check_tuple<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, elements: &'tcx [Expr<'tcx>]) { + if let ty::Tuple(tys) = cx.typeck_results().expr_ty(expr).kind() + && let [first, ..] = elements + // Fix #11100 + && tys.iter().all_equal() + && let Some(locals) = (match first.kind { + ExprKind::Index(_, _) => elements + .iter() + .enumerate() + .map(|(i, i_expr)| -> Option<&'tcx Expr<'tcx>> { + if let ExprKind::Index(lhs, index) = i_expr.kind + && let ExprKind::Lit(lit) = index.kind + && let LitKind::Int(val, _) = lit.node + { + return (val == i as u128).then_some(lhs); + }; + + None + }) + .collect::<Option<Vec<_>>>(), + ExprKind::Path(_) => Some(elements.iter().collect()), + _ => None, + }) + && all_bindings_are_for_conv(cx, tys, expr, elements, &locals, ToType::Tuple) + && !is_from_proc_macro(cx, expr) + { + span_lint_and_help( + cx, + TUPLE_ARRAY_CONVERSIONS, + expr.span, + "it looks like you're trying to convert an array to a tuple", + None, + "use `.into()` instead, or `<(T0, T1, ..., Tn)>::from` if type annotations are needed", + ); } } -#[expect( - clippy::blocks_in_if_conditions, - reason = "not a FP, but this is much easier to understand" -)] +/// Checks that every binding in `elements` comes from the same parent `Pat` with the kind if there +/// is a parent `Pat`. Returns false in any of the following cases: +/// * `kind` does not match `pat.kind` +/// * one or more elements in `elements` is not a binding +/// * one or more bindings does not have the same parent `Pat` +/// * one or more bindings are used after `expr` +/// * the bindings do not all have the same type #[expect(clippy::cast_possible_truncation)] -fn check_tuple<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, elements: &'tcx [Expr<'tcx>]) { - if should_lint(cx, elements, Some, |(first_id, local)| { - if let Node::Pat(pat) = local - && let parent = parent_pat(cx, pat) - && parent.hir_id == first_id - { - return matches!( - cx.typeck_results().pat_ty(parent).peel_refs().kind(), - ty::Array(_, len) if len.eval_target_usize(cx.tcx, cx.param_env) as usize == elements.len() - ); +fn all_bindings_are_for_conv<'tcx>( + cx: &LateContext<'tcx>, + final_tys: &[Ty<'tcx>], + expr: &Expr<'_>, + elements: &[Expr<'_>], + locals: &[&Expr<'_>], + kind: ToType, +) -> bool { + let Some(locals) = locals.iter().map(|e| path_to_local(e)).collect::<Option<Vec<_>>>() else { + return false; + }; + let Some(local_parents) = locals + .iter() + .map(|&l| cx.tcx.hir().find_parent(l)) + .collect::<Option<Vec<_>>>() + else { + return false; + }; + + local_parents + .iter() + .map(|node| match node { + Node::Pat(pat) => kind.eq(&pat.kind).then_some(pat.hir_id), + Node::Local(l) => Some(l.hir_id), + _ => None, + }) + .all_equal() + // Fix #11124, very convenient utils function! ❤️ + && locals + .iter() + .all(|&l| for_each_local_use_after_expr(cx, l, expr.hir_id, |_| ControlFlow::Break::<()>(())).is_continue()) + && local_parents.first().is_some_and(|node| { + let Some(ty) = match node { + Node::Pat(pat) => Some(pat.hir_id), + Node::Local(l) => Some(l.hir_id), + _ => None, } - - false - }) || should_lint( - cx, - elements, - |(i, expr)| { - if let ExprKind::Index(path, index) = expr.kind - && let ExprKind::Lit(lit) = index.kind - && let LitKind::Int(val, _) = lit.node - && val as usize == i - { - return Some((i, path)); + .map(|hir_id| cx.typeck_results().node_type(hir_id)) else { + return false; }; - - None - }, - |(first_id, local)| { - if let Node::Pat(pat) = local - && let parent = parent_pat(cx, pat) - && parent.hir_id == first_id - { - return matches!( - cx.typeck_results().pat_ty(parent).peel_refs().kind(), - ty::Array(_, len) if len.eval_target_usize(cx.tcx, cx.param_env) as usize == elements.len() - ); + match (kind, ty.kind()) { + // Ensure the final type and the original type have the same length, and that there + // is no implicit `&mut`<=>`&` anywhere (#11100). Bit ugly, I know, but it works. + (ToType::Array, ty::Tuple(tys)) => { + tys.len() == elements.len() && tys.iter().chain(final_tys.iter().copied()).all_equal() + }, + (ToType::Tuple, ty::Array(ty, len)) => { + len.eval_target_usize(cx.tcx, cx.param_env) as usize == elements.len() + && final_tys.iter().chain(once(ty)).all_equal() + }, + _ => false, } - - false - }, - ) { - emit_lint(cx, expr, ToType::Tuple); - } -} - -/// Walks up the `Pat` until it's reached the final containing `Pat`. -fn parent_pat<'tcx>(cx: &LateContext<'tcx>, start: &'tcx Pat<'tcx>) -> &'tcx Pat<'tcx> { - let mut end = start; - for (_, node) in cx.tcx.hir().parent_iter(start.hir_id) { - if let Node::Pat(pat) = node { - end = pat; - } else { - break; - } - } - end + }) } #[derive(Clone, Copy)] @@ -173,61 +203,11 @@ enum ToType { Tuple, } -impl ToType { - fn msg(self) -> &'static str { - match self { - ToType::Array => "it looks like you're trying to convert a tuple to an array", - ToType::Tuple => "it looks like you're trying to convert an array to a tuple", - } - } - - fn help(self) -> &'static str { +impl PartialEq<PatKind<'_>> for ToType { + fn eq(&self, other: &PatKind<'_>) -> bool { match self { - ToType::Array => "use `.into()` instead, or `<[T; N]>::from` if type annotations are needed", - ToType::Tuple => "use `.into()` instead, or `<(T0, T1, ..., Tn)>::from` if type annotations are needed", + ToType::Array => matches!(other, PatKind::Tuple(_, _)), + ToType::Tuple => matches!(other, PatKind::Slice(_, _, _)), } } } - -fn emit_lint<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, to_type: ToType) -> bool { - if !is_from_proc_macro(cx, expr) { - span_lint_and_help( - cx, - TUPLE_ARRAY_CONVERSIONS, - expr.span, - to_type.msg(), - None, - to_type.help(), - ); - - return true; - } - - false -} - -fn should_lint<'tcx>( - cx: &LateContext<'tcx>, - elements: &'tcx [Expr<'tcx>], - map: impl FnMut((usize, &'tcx Expr<'tcx>)) -> Option<(usize, &Expr<'_>)>, - predicate: impl FnMut((HirId, &Node<'tcx>)) -> bool, -) -> bool { - if let Some(elements) = elements - .iter() - .enumerate() - .map(map) - .collect::<Option<Vec<_>>>() - && let Some(locals) = elements - .iter() - .map(|(_, element)| path_to_local(element).and_then(|local| cx.tcx.hir().find(local))) - .collect::<Option<Vec<_>>>() - && let [first, rest @ ..] = &*locals - && let Node::Pat(first_pat) = first - && let parent = parent_pat(cx, first_pat).hir_id - && rest.iter().chain(once(first)).map(|i| (parent, i)).all(predicate) - { - return true; - } - - false -} diff --git a/tests/ui/tuple_array_conversions.rs b/tests/ui/tuple_array_conversions.rs index f96a7c97f1a..569415acbce 100644 --- a/tests/ui/tuple_array_conversions.rs +++ b/tests/ui/tuple_array_conversions.rs @@ -52,6 +52,36 @@ fn main() { let v1: Vec<[u32; 2]> = t1.iter().map(|&(a, b)| [a, b]).collect(); let t2: Vec<(u32, u32)> = v1.iter().map(|&[a, b]| (a, b)).collect(); } + // FP #11082; needs discussion + let (a, b) = (1.0f64, 2.0f64); + let _: &[f64] = &[a, b]; + // FP #11085; impossible to fix + let [src, dest]: [_; 2] = [1, 2]; + (src, dest); + // FP #11100 + fn issue_11100_array_to_tuple(this: [&mut i32; 2]) -> (&i32, &mut i32) { + let [input, output] = this; + (input, output) + } + + fn issue_11100_tuple_to_array<'a>(this: (&'a mut i32, &'a mut i32)) -> [&'a i32; 2] { + let (input, output) = this; + [input, output] + } + // FP #11124 + // tuple=>array + let (a, b) = (1, 2); + [a, b]; + let x = a; + // array=>tuple + let [a, b] = [1, 2]; + (a, b); + let x = a; + // FP #11144 + let (a, (b, c)) = (1, (2, 3)); + [a, c]; + let [[a, b], [c, d]] = [[1, 2], [3, 4]]; + (a, c); } #[clippy::msrv = "1.70.0"] diff --git a/tests/ui/tuple_array_conversions.stderr b/tests/ui/tuple_array_conversions.stderr index be653e8efb7..50bdcf29d1f 100644 --- a/tests/ui/tuple_array_conversions.stderr +++ b/tests/ui/tuple_array_conversions.stderr @@ -15,14 +15,6 @@ LL | let x = [x.0, x.1]; | = help: use `.into()` instead, or `<[T; N]>::from` if type annotations are needed -error: it looks like you're trying to convert an array to a tuple - --> $DIR/tuple_array_conversions.rs:13:13 - | -LL | let x = (x[0], x[1]); - | ^^^^^^^^^^^^ - | - = help: use `.into()` instead, or `<(T0, T1, ..., Tn)>::from` if type annotations are needed - error: it looks like you're trying to convert a tuple to an array --> $DIR/tuple_array_conversions.rs:16:53 | @@ -55,8 +47,24 @@ LL | t1.iter().for_each(|&(a, b)| _ = [a, b]); | = help: use `.into()` instead, or `<[T; N]>::from` if type annotations are needed +error: it looks like you're trying to convert a tuple to an array + --> $DIR/tuple_array_conversions.rs:57:22 + | +LL | let _: &[f64] = &[a, b]; + | ^^^^^^ + | + = help: use `.into()` instead, or `<[T; N]>::from` if type annotations are needed + error: it looks like you're trying to convert an array to a tuple - --> $DIR/tuple_array_conversions.rs:69:13 + --> $DIR/tuple_array_conversions.rs:60:5 + | +LL | (src, dest); + | ^^^^^^^^^^^ + | + = help: use `.into()` instead, or `<(T0, T1, ..., Tn)>::from` if type annotations are needed + +error: it looks like you're trying to convert an array to a tuple + --> $DIR/tuple_array_conversions.rs:99:13 | LL | let x = (x[0], x[1]); | ^^^^^^^^^^^^ @@ -64,20 +72,12 @@ LL | let x = (x[0], x[1]); = help: use `.into()` instead, or `<(T0, T1, ..., Tn)>::from` if type annotations are needed error: it looks like you're trying to convert a tuple to an array - --> $DIR/tuple_array_conversions.rs:70:13 + --> $DIR/tuple_array_conversions.rs:100:13 | LL | let x = [x.0, x.1]; | ^^^^^^^^^^ | = help: use `.into()` instead, or `<[T; N]>::from` if type annotations are needed -error: it looks like you're trying to convert an array to a tuple - --> $DIR/tuple_array_conversions.rs:72:13 - | -LL | let x = (x[0], x[1]); - | ^^^^^^^^^^^^ - | - = help: use `.into()` instead, or `<(T0, T1, ..., Tn)>::from` if type annotations are needed - error: aborting due to 10 previous errors |