move expr_requires_coercion to clippy_utils & some other adjustments

Author: J-ZhengLi

clippy_lints/src/matches/manual_utils.rs

@@ -4,8 +4,8 @@ use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
 use clippy_utils::sugg::Sugg;
 use clippy_utils::ty::{is_copy, is_type_diagnostic_item, peel_mid_ty_refs_is_mutable, type_is_unsafe_function};
 use clippy_utils::{
-    CaptureKind, can_move_expr_to_closure, is_else_clause, is_lint_allowed, is_res_lang_ctor, path_res,
-    path_to_local_id, peel_blocks, peel_hir_expr_refs, peel_hir_expr_while,
+    CaptureKind, can_move_expr_to_closure, expr_requires_coercion, is_else_clause, is_lint_allowed, is_res_lang_ctor,
+    path_res, path_to_local_id, peel_blocks, peel_hir_expr_refs, peel_hir_expr_while,
 };
 use rustc_ast::util::parser::ExprPrecedence;
 use rustc_errors::Applicability;
@@ -73,7 +73,7 @@ where
     }
 
     // `map` won't perform any adjustments.
-    if expr_has_type_coercion(cx, expr) {
+    if expr_requires_coercion(cx, expr) {
         return None;
     }
 
@@ -272,71 +272,3 @@ pub(super) fn try_parse_pattern<'tcx>(
 fn is_none_expr(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
     is_res_lang_ctor(cx, path_res(cx, peel_blocks(expr)), OptionNone)
 }
-
-fn expr_ty_adjusted(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
-    cx.typeck_results()
-        .expr_adjustments(expr)
-        .iter()
-        // We do not care about exprs with `NeverToAny` adjustments, such as `panic!` call.
-        .any(|adj| !matches!(adj.kind, Adjust::NeverToAny))
-}
-
-fn expr_has_type_coercion<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> bool {
-    if expr.span.from_expansion() {
-        return false;
-    }
-    if expr_ty_adjusted(cx, expr) {
-        return true;
-    }
-
-    // Identify coercion sites and recursively check it those sites
-    // actually has type adjustments.
-    match expr.kind {
-        // Function/method calls, including enum initialization.
-        ExprKind::Call(_, args) | ExprKind::MethodCall(_, _, args, _) if let Some(def_id) = fn_def_id(cx, expr) => {
-            let fn_sig = cx.tcx.fn_sig(def_id).instantiate_identity();
-            if !fn_sig.output().skip_binder().has_type_flags(TypeFlags::HAS_TY_PARAM) {
-                return false;
-            }
-            let mut args_with_ty_param = fn_sig
-                .inputs()
-                .skip_binder()
-                .iter()
-                .zip(args)
-                .filter_map(|(arg_ty, arg)| if arg_ty.has_type_flags(TypeFlags::HAS_TY_PARAM) {
-                    Some(arg)
-                } else {
-                    None
-                });
-            args_with_ty_param.any(|arg| expr_has_type_coercion(cx, arg))
-        },
-        // Struct/union initialization.
-        ExprKind::Struct(_, fields, _) => {
-            fields.iter().map(|expr_field| expr_field.expr).any(|ex| expr_has_type_coercion(cx, ex))
-        },
-        // those two `ref` keywords cannot be removed
-        #[allow(clippy::needless_borrow)]
-        // Function results, including the final line of a block or a `return` expression.
-        ExprKind::Block(hir::Block { expr: Some(ref ret_expr), .. }, _) |
-        ExprKind::Ret(Some(ref ret_expr)) => expr_has_type_coercion(cx, ret_expr),
-
-        // ===== Coercion-propagation expressions =====
-
-        // Array, where the type is `[U; n]`.
-        ExprKind::Array(elems) |
-        // Tuple, `(U_0, U_1, ..., U_n)`.
-        ExprKind::Tup(elems) => {
-            elems.iter().any(|elem| expr_has_type_coercion(cx, elem))
-        },
-        // Array but with repeating syntax.
-        ExprKind::Repeat(rep_elem, _) => expr_has_type_coercion(cx, rep_elem),
-        // Others that may contain coercion sites.
-        ExprKind::If(_, then, maybe_else) => {
-            expr_has_type_coercion(cx, then) || maybe_else.is_some_and(|e| expr_has_type_coercion(cx, e))
-        }
-        ExprKind::Match(_, arms, _) => {
-            arms.iter().map(|arm| arm.body).any(|body| expr_has_type_coercion(cx, body))
-        }
-        _ => false
-    }
-}

clippy_utils/src/lib.rs

@@ -118,7 +118,7 @@ use rustc_middle::ty::fast_reject::SimplifiedType;
 use rustc_middle::ty::layout::IntegerExt;
 use rustc_middle::ty::{
     self as rustc_ty, Binder, BorrowKind, ClosureKind, EarlyBinder, FloatTy, GenericArgsRef, IntTy, Ty, TyCtxt,
-    TypeVisitableExt, UintTy, UpvarCapture,
+    TypeFlags, TypeVisitableExt, UintTy, UpvarCapture,
 };
 use rustc_span::hygiene::{ExpnKind, MacroKind};
 use rustc_span::source_map::SourceMap;
@@ -3459,3 +3459,90 @@ pub fn is_receiver_of_method_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool
     }
     false
 }
+
+/// Returns true if the specified `expr` requires coercion,
+/// meaning that it either has a coercion or propagates a coercion from one of its sub expressions.
+///
+/// Similar to [`is_adjusted`], this not only checks if an expression's type was adjusted,
+/// but also going through extra steps to see if it fits the description of [coercion sites].
+///
+/// You should used this when you want to avoid suggesting replacing an expression that is currently
+/// a coercion site or coercion propagating expression with one that is not.
+///
+/// [coercion sites]: https://doc.rust-lang.org/stable/reference/type-coercions.html#coercion-sites
+pub fn expr_requires_coercion<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> bool {
+    let expr_ty_is_adjusted = cx
+        .typeck_results()
+        .expr_adjustments(expr)
+        .iter()
+        // ignore `NeverToAny` adjustments, such as `panic!` call.
+        .any(|adj| !matches!(adj.kind, Adjust::NeverToAny));
+    if expr_ty_is_adjusted {
+        return true;
+    }
+
+    // Identify coercion sites and recursively check if those sites
+    // actually have type adjustments.
+    match expr.kind {
+        ExprKind::Call(_, args) | ExprKind::MethodCall(_, _, args, _) if let Some(def_id) = fn_def_id(cx, expr) => {
+            let fn_sig = cx.tcx.fn_sig(def_id).instantiate_identity();
+
+            if !fn_sig.output().skip_binder().has_type_flags(TypeFlags::HAS_TY_PARAM) {
+                return false;
+            }
+
+            let self_arg_count = usize::from(matches!(expr.kind, ExprKind::MethodCall(..)));
+            let mut args_with_ty_param = {
+                fn_sig
+                    .inputs()
+                    .skip_binder()
+                    .iter()
+                    .skip(self_arg_count)
+                    .zip(args)
+                    .filter_map(|(arg_ty, arg)| {
+                        if arg_ty.has_type_flags(TypeFlags::HAS_TY_PARAM) {
+                            Some(arg)
+                        } else {
+                            None
+                        }
+                    })
+            };
+            args_with_ty_param.any(|arg| expr_requires_coercion(cx, arg))
+        },
+        // Struct/union initialization.
+        ExprKind::Struct(qpath, _, _) => {
+            let res = cx.typeck_results().qpath_res(qpath, expr.hir_id);
+            if let Some((_, v_def)) = adt_and_variant_of_res(cx, res) {
+                let generic_args = cx.typeck_results().node_args(expr.hir_id);
+                v_def
+                    .fields
+                    .iter()
+                    .any(|field| field.ty(cx.tcx, generic_args).has_type_flags(TypeFlags::HAS_TY_PARAM))
+            } else {
+                false
+            }
+        },
+        // Function results, including the final line of a block or a `return` expression.
+        ExprKind::Block(
+            &Block {
+                expr: Some(ret_expr), ..
+            },
+            _,
+        )
+        | ExprKind::Ret(Some(ret_expr)) => expr_requires_coercion(cx, ret_expr),
+
+        // ===== Coercion-propagation expressions =====
+        ExprKind::Array(elems) | ExprKind::Tup(elems) => elems.iter().any(|elem| expr_requires_coercion(cx, elem)),
+        // Array but with repeating syntax.
+        ExprKind::Repeat(rep_elem, _) => expr_requires_coercion(cx, rep_elem),
+        // Others that may contain coercion sites.
+        ExprKind::If(_, then, maybe_else) => {
+            expr_requires_coercion(cx, then) || maybe_else.is_some_and(|e| expr_requires_coercion(cx, e))
+        },
+        ExprKind::Match(_, arms, _) => arms
+            .iter()
+            .map(|arm| arm.body)
+            .any(|body| expr_requires_coercion(cx, body)),
+        _ => false,
+    }
+}

tests/ui/manual_map_option.fixed

@@ -113,7 +113,16 @@ fn main() {
     }
 
     // #6811
-    Some(0).map(|x| vec![x]);
+    match Some(0) {
+        Some(x) => Some(vec![x]),
+        None => None,
+    };
+
+    // Don't lint, coercion
+    let x: Option<Vec<&[u8]>> = match Some(()) {
+        Some(_) => Some(vec![b"1234"]),
+        None => None,
+    };
 
     option_env!("").map(String::from);
 

tests/ui/manual_map_option.rs

@@ -170,6 +170,12 @@ fn main() {
         None => None,
     };
 
+    // Don't lint, coercion
+    let x: Option<Vec<&[u8]>> = match Some(()) {
+        Some(_) => Some(vec![b"1234"]),
+        None => None,
+    };
+
     match option_env!("") {
         Some(x) => Some(String::from(x)),
         None => None,

tests/ui/manual_map_option.stderr

@@ -156,16 +156,7 @@ LL | |     };
    | |_____^ help: try: `Some((String::new(), "test")).as_ref().map(|(x, y)| (y, x))`
 
 error: manual implementation of `Option::map`
-  --> tests/ui/manual_map_option.rs:168:5
-   |
-LL | /     match Some(0) {
-LL | |         Some(x) => Some(vec![x]),
-LL | |         None => None,
-LL | |     };
-   | |_____^ help: try: `Some(0).map(|x| vec![x])`
-
-error: manual implementation of `Option::map`
-  --> tests/ui/manual_map_option.rs:173:5
+  --> tests/ui/manual_map_option.rs:179:5
    |
 LL | /     match option_env!("") {
 LL | |         Some(x) => Some(String::from(x)),
@@ -174,7 +165,7 @@ LL | |     };
    | |_____^ help: try: `option_env!("").map(String::from)`
 
 error: manual implementation of `Option::map`
-  --> tests/ui/manual_map_option.rs:193:12
+  --> tests/ui/manual_map_option.rs:199:12
    |
 LL |       } else if let Some(x) = Some(0) {
    |  ____________^
@@ -185,7 +176,7 @@ LL | |     };
    | |_____^ help: try: `{ Some(0).map(|x| x + 1) }`
 
 error: manual implementation of `Option::map`
-  --> tests/ui/manual_map_option.rs:201:12
+  --> tests/ui/manual_map_option.rs:207:12
    |
 LL |       } else if let Some(x) = Some(0) {
    |  ____________^
@@ -195,5 +186,5 @@ LL | |         None
 LL | |     };
    | |_____^ help: try: `{ Some(0).map(|x| x + 1) }`
 
-error: aborting due to 21 previous errors
+error: aborting due to 20 previous errors