diff --git a/clippy_lints/src/matches/manual_utils.rs b/clippy_lints/src/matches/manual_utils.rs index be80aebed6df..cac8c58e4294 100644 --- a/clippy_lints/src/matches/manual_utils.rs +++ b/clippy_lints/src/matches/manual_utils.rs @@ -11,8 +11,9 @@ use rustc_ast::util::parser::PREC_UNAMBIGUOUS; use rustc_errors::Applicability; use rustc_hir::def::Res; use rustc_hir::LangItem::{OptionNone, OptionSome}; -use rustc_hir::{BindingMode, Expr, ExprKind, HirId, Mutability, Pat, PatKind, Path, QPath}; +use rustc_hir::{self as hir, BindingMode, Expr, ExprKind, HirId, Mutability, Pat, PatKind, Path, QPath}; use rustc_lint::LateContext; +use rustc_middle::ty::adjustment::Adjust; use rustc_span::{sym, SyntaxContext}; #[expect(clippy::too_many_arguments)] @@ -65,15 +66,7 @@ where let some_expr = get_some_expr_fn(cx, some_pat, some_expr, expr_ctxt)?; - // These two lints will go back and forth with each other. - if cx.typeck_results().expr_ty(some_expr.expr) == cx.tcx.types.unit - && !is_lint_allowed(cx, OPTION_MAP_UNIT_FN, expr.hir_id) - { - return None; - } - - // `map` won't perform any adjustments. - if !cx.typeck_results().expr_adjustments(some_expr.expr).is_empty() { + if !should_lint(cx, expr, some_expr.expr) { return None; } @@ -274,3 +267,79 @@ 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. + .filter(|adj| !matches!(adj.kind, Adjust::NeverToAny)) + .next() + .is_some() +} + +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, _) => { + args.iter().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)) + }, + // 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.map(|e| expr_has_type_coercion(cx, e)).unwrap_or_default() + } + ExprKind::Match(_, arms, _) => { + arms.iter().map(|arm| arm.body).any(|body| expr_has_type_coercion(cx, body)) + } + _ => false + } +} + +fn should_lint<'tcx>(cx: &LateContext<'tcx>, match_expr: &Expr<'tcx>, some_expr: &Expr<'tcx>) -> bool { + let typeck = cx.typeck_results(); + let some_expr_ty = typeck.expr_ty(some_expr); + + // These two lints will go back and forth with each other. + if some_expr_ty.is_unit() && !is_lint_allowed(cx, OPTION_MAP_UNIT_FN, match_expr.hir_id) { + return false; + } + + // `map` won't perform any adjustments. + // However, if the ty of `Some` arg is unit, type coercions should be fine. + if !some_expr_ty.is_unit() + // TODO: this needs to be tested + && !some_expr_ty.is_scalar() + && expr_has_type_coercion(cx, match_expr) + { + return false; + } + + true +} diff --git a/tests/ui/manual_map_option_2.fixed b/tests/ui/manual_map_option_2.fixed index f5bb4e0af1ba..4f449ffa67da 100644 --- a/tests/ui/manual_map_option_2.fixed +++ b/tests/ui/manual_map_option_2.fixed @@ -40,9 +40,14 @@ fn main() { None => None, }; - // Lint. `s` is captured by reference, so no lifetime issues. let s = Some(String::new()); + // Lint. `s` is captured by reference, so no lifetime issues. let _ = s.as_ref().map(|x| { if let Some(ref s) = s { (x.clone(), s) } else { panic!() } }); + // Don't lint this, type of `s` is coercioned from `&String` to `&str` + let x: Option<(String, &str)> = match &s { + Some(x) => Some({ if let Some(ref s) = s { (x.clone(), s) } else { panic!() } }), + None => None, + }; // Issue #7820 unsafe fn f(x: u32) -> u32 { @@ -54,3 +59,92 @@ fn main() { let _ = Some(0).map(|x| unsafe { f(x) }); let _ = Some(0).map(|x| unsafe { f(x) }); } + +// issue #12659 +mod with_type_coercion { + trait DummyTrait {} + + fn foo Result>(f: F) { + // Don't lint + let _: Option, ()>> = match Some(0) { + Some(_) => Some(match f() { + Ok(res) => Ok(Box::new(res)), + _ => Err(()), + }), + None => None, + }; + + let _: Option> = match Some(()) { + Some(_) => Some(Box::new(b"1234")), + None => None, + }; + + let x = String::new(); + let _: Option> = match Some(()) { + Some(_) => Some(Box::new(&x)), + None => None, + }; + + let _: Option<&str> = match Some(()) { + Some(_) => Some(&x), + None => None, + }; + + //~v ERROR: manual implementation of `Option::map` + let _ = Some(0).map(|_| match f() { + Ok(res) => Ok(Box::new(res)), + _ => Err(()), + }); + } + + #[allow(clippy::redundant_allocation)] + fn bar() { + fn f(_: Option>) {} + fn g(b: &[u8]) -> Box<&[u8]> { + Box::new(b) + } + + let x: &[u8; 4] = b"1234"; + f(match Some(()) { + Some(_) => Some(Box::new(x)), + None => None, + }); + + // FIXME: This is a known FN case. + let _: Option> = match Some(0) { + Some(_) => Some(g(x)), + None => None, + }; + } + + fn with_fn_ret(s: &Option) -> Option<(String, &str)> { + // Don't lint, `map` doesn't work as the return type is adjusted. + match s { + Some(x) => Some({ if let Some(ref s) = s { (x.clone(), s) } else { panic!() } }), + None => None, + } + } + + fn with_fn_ret_2(s: &Option) -> Option<(String, &str)> { + if true { + // Don't lint, `map` doesn't work as the return type is adjusted. + return match s { + Some(x) => Some({ if let Some(ref s) = s { (x.clone(), s) } else { panic!() } }), + None => None, + }; + } + None + } + + #[allow(clippy::needless_late_init)] + fn with_fn_ret_3<'a>(s: &'a Option) -> Option<(String, &'a str)> { + let x: Option<(String, &'a str)>; + x = { + match s { + Some(x) => Some({ if let Some(ref s) = s { (x.clone(), s) } else { panic!() } }), + None => None, + } + }; + x + } +} diff --git a/tests/ui/manual_map_option_2.rs b/tests/ui/manual_map_option_2.rs index cbc2356e0a2d..891e69b4c491 100644 --- a/tests/ui/manual_map_option_2.rs +++ b/tests/ui/manual_map_option_2.rs @@ -43,12 +43,17 @@ fn main() { None => None, }; - // Lint. `s` is captured by reference, so no lifetime issues. let s = Some(String::new()); + // Lint. `s` is captured by reference, so no lifetime issues. let _ = match &s { Some(x) => Some({ if let Some(ref s) = s { (x.clone(), s) } else { panic!() } }), None => None, }; + // Don't lint this, type of `s` is coercioned from `&String` to `&str` + let x: Option<(String, &str)> = match &s { + Some(x) => Some({ if let Some(ref s) = s { (x.clone(), s) } else { panic!() } }), + None => None, + }; // Issue #7820 unsafe fn f(x: u32) -> u32 { @@ -69,3 +74,95 @@ fn main() { None => None, }; } + +// issue #12659 +mod with_type_coercion { + trait DummyTrait {} + + fn foo Result>(f: F) { + // Don't lint + let _: Option, ()>> = match Some(0) { + Some(_) => Some(match f() { + Ok(res) => Ok(Box::new(res)), + _ => Err(()), + }), + None => None, + }; + + let _: Option> = match Some(()) { + Some(_) => Some(Box::new(b"1234")), + None => None, + }; + + let x = String::new(); + let _: Option> = match Some(()) { + Some(_) => Some(Box::new(&x)), + None => None, + }; + + let _: Option<&str> = match Some(()) { + Some(_) => Some(&x), + None => None, + }; + + //~v ERROR: manual implementation of `Option::map` + let _ = match Some(0) { + Some(_) => Some(match f() { + Ok(res) => Ok(Box::new(res)), + _ => Err(()), + }), + None => None, + }; + } + + #[allow(clippy::redundant_allocation)] + fn bar() { + fn f(_: Option>) {} + fn g(b: &[u8]) -> Box<&[u8]> { + Box::new(b) + } + + let x: &[u8; 4] = b"1234"; + f(match Some(()) { + Some(_) => Some(Box::new(x)), + None => None, + }); + + // FIXME: This is a known FN case. + let _: Option> = match Some(0) { + Some(_) => Some(g(x)), + None => None, + }; + } + + fn with_fn_ret(s: &Option) -> Option<(String, &str)> { + // Don't lint, `map` doesn't work as the return type is adjusted. + match s { + Some(x) => Some({ if let Some(ref s) = s { (x.clone(), s) } else { panic!() } }), + None => None, + } + } + + fn with_fn_ret_2(s: &Option) -> Option<(String, &str)> { + if true { + // Don't lint, `map` doesn't work as the return type is adjusted. + return match s { + Some(x) => Some({ if let Some(ref s) = s { (x.clone(), s) } else { panic!() } }), + None => None, + }; + } + None + } + + #[allow(clippy::needless_late_init)] + fn with_fn_ret_3<'a>(s: &'a Option) -> Option<(String, &'a str)> { + let x: Option<(String, &'a str)>; + x = { + match s { + Some(x) => Some({ if let Some(ref s) = s { (x.clone(), s) } else { panic!() } }), + None => None, + } + }; + x + } +} diff --git a/tests/ui/manual_map_option_2.stderr b/tests/ui/manual_map_option_2.stderr index 78e4677544bc..a65750c2b99e 100644 --- a/tests/ui/manual_map_option_2.stderr +++ b/tests/ui/manual_map_option_2.stderr @@ -32,7 +32,7 @@ LL | | }; | |_____^ help: try: `s.as_ref().map(|x| { if let Some(ref s) = s { (x.clone(), s) } else { panic!() } })` error: manual implementation of `Option::map` - --> tests/ui/manual_map_option_2.rs:58:17 + --> tests/ui/manual_map_option_2.rs:63:17 | LL | let _ = match Some(0) { | _________________^ @@ -42,7 +42,7 @@ LL | | }; | |_________^ help: try: `Some(0).map(|x| f(x))` error: manual implementation of `Option::map` - --> tests/ui/manual_map_option_2.rs:63:13 + --> tests/ui/manual_map_option_2.rs:68:13 | LL | let _ = match Some(0) { | _____________^ @@ -52,7 +52,7 @@ LL | | }; | |_____^ help: try: `Some(0).map(|x| unsafe { f(x) })` error: manual implementation of `Option::map` - --> tests/ui/manual_map_option_2.rs:67:13 + --> tests/ui/manual_map_option_2.rs:72:13 | LL | let _ = match Some(0) { | _____________^ @@ -61,5 +61,26 @@ LL | | None => None, LL | | }; | |_____^ help: try: `Some(0).map(|x| unsafe { f(x) })` -error: aborting due to 5 previous errors +error: manual implementation of `Option::map` + --> tests/ui/manual_map_option_2.rs:109:17 + | +LL | let _ = match Some(0) { + | _________________^ +LL | | Some(_) => Some(match f() { +LL | | Ok(res) => Ok(Box::new(res)), +LL | | _ => Err(()), +LL | | }), +LL | | None => None, +LL | | }; + | |_________^ + | +help: try + | +LL ~ let _ = Some(0).map(|_| match f() { +LL + Ok(res) => Ok(Box::new(res)), +LL + _ => Err(()), +LL ~ }); + | + +error: aborting due to 6 previous errors