Auto merge of #11171 - Centri3:tuple_array_conversions, r=llogiq

bors · bors · commit 747df85f956c · 2023-07-18T05:15:50.000Z
Rewrite [`tuple_array_conversions`] Fixes #11100 Fixes #11144 Fixes #11124 #11082 still needs discussion and #11085 likely can't be fixed. changelog: [`tuple_array_conversions`]: Move to `pedantic` changelog: [`tuple_array_conversions`]: Don't lint if mutability of references changes changelog: [`tuple_array_conversions`]: Don't lint if bindings don't come from the exact same pattern changelog: [`tuple_array_conversions`]: Don't lint if bindings are used for more than just the conversion
diff --git 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
@@ -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
