diff --git a/clippy_lints/src/matches/manual_utils.rs b/clippy_lints/src/matches/manual_utils.rs index 183caab56c5..7506650a4e4 100644 --- a/clippy_lints/src/matches/manual_utils.rs +++ b/clippy_lints/src/matches/manual_utils.rs @@ -3,17 +3,19 @@ use crate::matches::MATCH_AS_REF; 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::visitors::for_each_expr; use clippy_utils::{ - 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, get_parent_expr, 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, }; use rustc_ast::util::parser::PREC_POSTFIX; 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, Node, Pat, PatKind, Path, QPath}; use rustc_lint::LateContext; use rustc_span::{sym, SyntaxContext}; +use std::ops::ControlFlow; #[expect(clippy::too_many_arguments)] #[expect(clippy::too_many_lines)] @@ -73,7 +75,7 @@ where } // `map` won't perform any adjustments. - if !cx.typeck_results().expr_adjustments(some_expr.expr).is_empty() { + if expr_has_type_coercion(cx, expr) { return None; } @@ -274,3 +276,61 @@ 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).is_empty() +} + +fn expr_has_type_coercion<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { + if is_in_coercion_site(cx, expr.hir_id) { + if expr_ty_adjusted(cx, expr) { + return true; + } + + // Check for coercion in sub-expressions + for_each_expr(cx, expr, |ex| { + // This has FP + // if is_coercion_propagating_sub_expr(cx, expr) && expr_ty_adjusted(cx, ex) { + // This has FN + if expr_ty_adjusted(cx, ex) { + ControlFlow::Break(true) + } else { + ControlFlow::Continue(()) + } + }) + .unwrap_or_default() + } else { + false + } +} + +fn is_in_coercion_site(cx: &LateContext<'_>, hir_id: HirId) -> bool { + match cx.tcx.parent_hir_node(hir_id) { + // `let` statements where an explicit type is given. + Node::LetStmt(..) | + // `static` or `const` item declarations. + Node::Item(hir::Item { kind: hir::ItemKind::Const(..) | hir::ItemKind::Static(..), .. }) | + Node::ImplItem(hir::ImplItem { kind: hir::ImplItemKind::Const(..), .. }) | + Node::TraitItem(hir::TraitItem { kind: hir::TraitItemKind::Const(..), .. }) | + // arguments for function calls. + Node::Expr(Expr { kind: ExprKind::Call(..) | ExprKind::MethodCall(..), .. }) | + // instantiations of struct, union, or enum variant fields. + // FIXME: not complete. + Node::Expr(Expr { kind: ExprKind::Struct(..), .. }) | + // Function results - either the final line of a block or any expr in a `return` statement + Node::Expr(Expr { kind: ExprKind::Ret(_), .. }) | + Node::Block(_) => true, + Node::Expr(expr) if is_coercion_propagating_sub_expr(cx, expr) => true, + _ => false + } +} + +fn is_coercion_propagating_sub_expr(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + let Some(parent_expr) = get_parent_expr(cx, expr) else { + return false; + }; + match parent_expr.kind { + ExprKind::Array(..) | ExprKind::Repeat(..) | ExprKind::Tup(..) | ExprKind::Block(..) => true, + _ => false, + } +} diff --git a/tests/ui/manual_map_option_2.fixed b/tests/ui/manual_map_option_2.fixed index f5bb4e0af1b..967e8e6b26e 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,59 @@ fn main() { let _ = Some(0).map(|x| unsafe { f(x) }); let _ = Some(0).map(|x| unsafe { f(x) }); } + +mod issue_12659 { + trait DummyTrait {} + + fn foo Result>(f: F) { + // Don't lint + let _: Option, ()>> = Some(0).map(|_| match f() { + Ok(res) => Ok(Box::new(res)), + _ => Err(()), + }); + + let _: Option> = Some(()).map(|_| Box::new(b"1234")); + + let x = String::new(); + let _: Option> = Some(()).map(|_| Box::new(&x)); + + let _: Option<&str> = Some(()).map(|_| &x); + + //~v ERROR: manual implementation of `Option::map` + let _ = Some(0).map(|_| match f() { + Ok(res) => Ok(Box::new(res)), + _ => Err(()), + }); + } + + 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 cbc2356e0a2..9b08b883fc7 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,74 @@ fn main() { None => None, }; } + +mod issue_12659 { + 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, + }; + } + + 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 78e4677544b..fadbb418d95 100644 --- a/tests/ui/manual_map_option_2.stderr +++ b/tests/ui/manual_map_option_2.stderr @@ -22,17 +22,7 @@ LL ~ }); | error: manual implementation of `Option::map` - --> tests/ui/manual_map_option_2.rs:48:13 - | -LL | let _ = match &s { - | _____________^ -LL | | Some(x) => Some({ if let Some(ref s) = s { (x.clone(), s) } else { panic!() } }), -LL | | None => None, -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 +32,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 +42,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 +51,5 @@ LL | | None => None, LL | | }; | |_____^ help: try: `Some(0).map(|x| unsafe { f(x) })` -error: aborting due to 5 previous errors +error: aborting due to 4 previous errors