diff --git a/clippy_lints/src/matches/manual_utils.rs b/clippy_lints/src/matches/manual_utils.rs index 183caab56c59..cfc10ff5a500 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, is_else_clause, is_lint_allowed, is_res_lang_ctor, path_res, path_to_local, + 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::{BindingMode, Block, 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 some_expr.expr_contains_ty_adjustments(cx) && expr_has_explicit_ty(cx, expr) { return None; } @@ -238,6 +240,17 @@ impl<'tcx> SomeExpr<'tcx> { let sugg = Sugg::hir_with_context(cx, self.expr, ctxt, "..", app); if self.needs_negated { !sugg } else { sugg } } + + fn expr_contains_ty_adjustments(&self, cx: &LateContext<'tcx>) -> bool { + for_each_expr(cx, self.expr, |ex| { + if cx.typeck_results().expr_adjustments(ex).is_empty() { + ControlFlow::Continue(()) + } else { + ControlFlow::Break(true) + } + }) + .unwrap_or_default() + } } // Try to parse into a recognized `Option` pattern. @@ -274,3 +287,60 @@ 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) } + +/// Return `true` if the type of `match` or `if-let` is labeled explicitly. +/// +/// Such as the init binding of: +/// +/// ```ignore +/// let x: &u8 = match Some(0) { .. }; +/// // ^^^^^^^^^^^^^^^^^^^^ +/// ``` +/// +/// Or the return of a function with return ty: +/// +/// ```ignore +/// fn foo() -> u8 { +/// match Some(0) { .. } +/// // ^^^^^^^^^^^^^^^^^^^^ +/// } +/// ``` +fn expr_has_explicit_ty(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + fn check_inner_(cx: &LateContext<'_>, hir_id: HirId) -> bool { + match cx.tcx.parent_hir_node(hir_id) { + Node::LetStmt(let_stmt) => let_stmt.ty.is_some(), + Node::Expr(Expr { + kind: ExprKind::Assign(assignee, ..), + .. + }) => { + if let Some(local_id) = path_to_local(assignee) + && let Node::LetStmt(let_stmt) = cx.tcx.parent_hir_node(local_id) + { + let_stmt.ty.is_some() + } else { + false + } + }, + // Assuming `true` because this branch should only be reached from + // tracing the parent of a `Node::Block` that has return value, + // meaning this is either `Fn` or `Const`, that always (?) has a labeled type. + Node::Item(_) | Node::TraitItem(_) | Node::ImplItem(_) | + // If this expr is being returned, then it's type must've been `labeled` on its owner function. + Node::Expr(Expr { + kind: ExprKind::Ret(_), .. + }) => true, + // The implicit return of a block, check if its a fn body or some init block. + Node::Block(Block { + expr: Some(_), hir_id, .. + }) => check_inner_(cx, cx.tcx.parent_hir_id(*hir_id)), + _ => false, + } + } + + // Sanity check + assert!( + matches!(expr.kind, ExprKind::Match(..) | ExprKind::If(..)), + "must used on `match` or `if-let` expressions" + ); + check_inner_(cx, expr.hir_id) +} diff --git a/tests/ui/manual_map_option_2.fixed b/tests/ui/manual_map_option_2.fixed index f5bb4e0af1ba..e2da4050b6fd 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,71 @@ 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, ()>> = 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(()), + }); + } + + 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..9b08b883fc7b 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 78e4677544bc..c80502beae2d 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:108: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