diff options
| -rw-r--r-- | clippy_lints/src/matches/manual_utils.rs | 74 | ||||
| -rw-r--r-- | clippy_utils/src/lib.rs | 89 | ||||
| -rw-r--r-- | tests/ui/manual_map_option.fixed | 11 | ||||
| -rw-r--r-- | tests/ui/manual_map_option.rs | 6 | ||||
| -rw-r--r-- | tests/ui/manual_map_option.stderr | 17 |
5 files changed, 111 insertions, 86 deletions
diff --git a/clippy_lints/src/matches/manual_utils.rs b/clippy_lints/src/matches/manual_utils.rs index c15e9a50a8e..c5690e9588c 100644 --- a/clippy_lints/src/matches/manual_utils.rs +++ b/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 - } -} diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 2f5639b686b..3ed9313d82b 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -117,7 +117,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, GenericArgKind, GenericArgsRef, IntTy, Ty, - TyCtxt, TypeVisitableExt, UintTy, UpvarCapture, + TyCtxt, TypeFlags, TypeVisitableExt, UintTy, UpvarCapture, }; use rustc_span::hygiene::{ExpnKind, MacroKind}; use rustc_span::source_map::SourceMap; @@ -3489,3 +3489,90 @@ pub fn leaks_droppable_temporary_with_limited_lifetime<'tcx>(cx: &LateContext<'t }) .is_break() } + +/// 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, + } +} diff --git a/tests/ui/manual_map_option.fixed b/tests/ui/manual_map_option.fixed index 16cee3fd382..3586979ab35 100644 --- a/tests/ui/manual_map_option.fixed +++ b/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); diff --git a/tests/ui/manual_map_option.rs b/tests/ui/manual_map_option.rs index 4655acf1406..2f21628977c 100644 --- a/tests/ui/manual_map_option.rs +++ b/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, diff --git a/tests/ui/manual_map_option.stderr b/tests/ui/manual_map_option.stderr index 47cc18303ba..c496752e2f6 100644 --- a/tests/ui/manual_map_option.stderr +++ b/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 |