From a5fcaea2e0b3595613cda90c71e8e21cc717d2c8 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Tue, 2 Jul 2024 16:09:13 +0800 Subject: [PATCH] fix [`manual_map`] not catching type adjustment --- clippy_lints/src/matches/manual_utils.rs | 94 ++++++++++++++++++---- tests/ui/manual_map_option_2.fixed | 93 +++++++++++++++++++++- tests/ui/manual_map_option_2.rs | 99 +++++++++++++++++++++++- tests/ui/manual_map_option_2.stderr | 39 +++++++++- 4 files changed, 306 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/matches/manual_utils.rs b/clippy_lints/src/matches/manual_utils.rs index be80aebed6df..870f5d06f393 100644 --- a/clippy_lints/src/matches/manual_utils.rs +++ b/clippy_lints/src/matches/manual_utils.rs @@ -4,15 +4,17 @@ 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::{ - can_move_expr_to_closure, is_else_clause, is_lint_allowed, is_res_lang_ctor, path_res, path_to_local_id, + can_move_expr_to_closure, fn_def_id, 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_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_middle::ty::{TypeFlags, TypeVisitableExt}; use rustc_span::{sym, SyntaxContext}; #[expect(clippy::too_many_arguments)] @@ -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; } @@ -124,6 +126,12 @@ where }; let closure_expr_snip = some_expr.to_snippet_with_context(cx, expr_ctxt, &mut app); + let closure_body = if some_expr.needs_unsafe_block { + format!("unsafe {}", closure_expr_snip.blockify()) + } else { + closure_expr_snip.to_string() + }; + let body_str = if let PatKind::Binding(annotation, id, some_binding, None) = some_pat.kind { if !some_expr.needs_unsafe_block && let Some(func) = can_pass_as_func(cx, id, some_expr.expr) @@ -145,20 +153,12 @@ where "" }; - if some_expr.needs_unsafe_block { - format!("|{annotation}{some_binding}| unsafe {{ {closure_expr_snip} }}") - } else { - format!("|{annotation}{some_binding}| {closure_expr_snip}") - } + format!("|{annotation}{some_binding}| {closure_body}") } } else if !is_wild_none && explicit_ref.is_none() { // TODO: handle explicit reference annotations. let pat_snip = snippet_with_context(cx, some_pat.span, expr_ctxt, "..", &mut app).0; - if some_expr.needs_unsafe_block { - format!("|{pat_snip}| unsafe {{ {closure_expr_snip} }}") - } else { - format!("|{pat_snip}| {closure_expr_snip}") - } + format!("|{pat_snip}| {closure_body}") } else { // Refutable bindings and mixed reference annotations can't be handled by `map`. return None; @@ -274,3 +274,71 @@ 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/tests/ui/manual_map_option_2.fixed b/tests/ui/manual_map_option_2.fixed index f5bb4e0af1ba..49b9e77b441f 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,89 @@ 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, + }); + + //~v ERROR: manual implementation of `Option::map` + let _: Option> = Some(0).map(|_| g(x)); + } + + 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..962455daf7ba 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, + }); + + //~v ERROR: manual implementation of `Option::map` + 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..db048221db6a 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,36 @@ 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: manual implementation of `Option::map` + --> tests/ui/manual_map_option_2.rs:132:37 + | +LL | let _: Option> = match Some(0) { + | _____________________________________^ +LL | | Some(_) => Some(g(x)), +LL | | None => None, +LL | | }; + | |_________^ help: try: `Some(0).map(|_| g(x))` + +error: aborting due to 7 previous errors