From 1923e2a8bc514b7a08a95858595cb6eaae0eec94 Mon Sep 17 00:00:00 2001 From: Ada Alakbarova Date: Mon, 3 Nov 2025 17:51:45 +0100 Subject: [PATCH] `manual_map`: lint exprs with adjustments, but with reduced `Applicability` --- clippy_lints/src/matches/manual_utils.rs | 6 +- tests/ui/manual_map_option.fixed | 19 +-- tests/ui/manual_map_option.rs | 15 +-- tests/ui/manual_map_option.stderr | 18 ++- tests/ui/manual_map_option_2.fixed | 67 ---------- tests/ui/manual_map_option_2.rs | 67 ---------- tests/ui/manual_map_option_2.stderr | 10 +- tests/ui/manual_map_option_unfixable.rs | 118 +++++++++++++++++ tests/ui/manual_map_option_unfixable.stderr | 140 ++++++++++++++++++++ 9 files changed, 282 insertions(+), 178 deletions(-) create mode 100644 tests/ui/manual_map_option_unfixable.rs create mode 100644 tests/ui/manual_map_option_unfixable.stderr diff --git a/clippy_lints/src/matches/manual_utils.rs b/clippy_lints/src/matches/manual_utils.rs index 09e49c16624c..b3f4ee936256 100644 --- a/clippy_lints/src/matches/manual_utils.rs +++ b/clippy_lints/src/matches/manual_utils.rs @@ -72,9 +72,11 @@ where return None; } + let mut app = Applicability::MachineApplicable; + // `map` won't perform any adjustments. if expr_requires_coercion(cx, expr) { - return None; + app = Applicability::MaybeIncorrect; } // Determine which binding mode to use. @@ -111,8 +113,6 @@ where None => return None, } - let mut app = Applicability::MachineApplicable; - // Remove address-of expressions from the scrutinee. Either `as_ref` will be called, or // it's being passed by value. let scrutinee = peel_hir_expr_refs(scrutinee).0; diff --git a/tests/ui/manual_map_option.fixed b/tests/ui/manual_map_option.fixed index 3586979ab358..285fbf357b73 100644 --- a/tests/ui/manual_map_option.fixed +++ b/tests/ui/manual_map_option.fixed @@ -113,16 +113,7 @@ fn main() { } // #6811 - match Some(0) { - Some(x) => Some(vec![x]), - None => None, - }; - - // Don't lint, coercion - let x: Option> = match Some(()) { - Some(_) => Some(vec![b"1234"]), - None => None, - }; + Some(0).map(|x| vec![x]); option_env!("").map(String::from); @@ -154,12 +145,4 @@ fn main() { None => None, }; } - - // #7077 - let s = &String::new(); - #[allow(clippy::needless_match)] - let _: Option<&str> = match Some(s) { - Some(s) => Some(s), - None => None, - }; } diff --git a/tests/ui/manual_map_option.rs b/tests/ui/manual_map_option.rs index 40133748d459..e99ae3535832 100644 --- a/tests/ui/manual_map_option.rs +++ b/tests/ui/manual_map_option.rs @@ -183,16 +183,11 @@ fn main() { // #6811 match Some(0) { + //~^ manual_map Some(x) => Some(vec![x]), None => None, }; - // Don't lint, coercion - let x: Option> = match Some(()) { - Some(_) => Some(vec![b"1234"]), - None => None, - }; - match option_env!("") { //~^ manual_map Some(x) => Some(String::from(x)), @@ -237,12 +232,4 @@ fn main() { None => None, }; } - - // #7077 - let s = &String::new(); - #[allow(clippy::needless_match)] - let _: Option<&str> = match Some(s) { - Some(s) => Some(s), - None => None, - }; } diff --git a/tests/ui/manual_map_option.stderr b/tests/ui/manual_map_option.stderr index 486379c1e5f3..ce04b63ac127 100644 --- a/tests/ui/manual_map_option.stderr +++ b/tests/ui/manual_map_option.stderr @@ -173,7 +173,17 @@ 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:196:5 + --> tests/ui/manual_map_option.rs:185:5 + | +LL | / match Some(0) { +LL | | +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:191:5 | LL | / match option_env!("") { LL | | @@ -183,7 +193,7 @@ LL | | }; | |_____^ help: try: `option_env!("").map(String::from)` error: manual implementation of `Option::map` - --> tests/ui/manual_map_option.rs:217:12 + --> tests/ui/manual_map_option.rs:212:12 | LL | } else if let Some(x) = Some(0) { | ____________^ @@ -195,7 +205,7 @@ LL | | }; | |_____^ help: try: `{ Some(0).map(|x| x + 1) }` error: manual implementation of `Option::map` - --> tests/ui/manual_map_option.rs:226:12 + --> tests/ui/manual_map_option.rs:221:12 | LL | } else if let Some(x) = Some(0) { | ____________^ @@ -206,5 +216,5 @@ LL | | None LL | | }; | |_____^ help: try: `{ Some(0).map(|x| x + 1) }` -error: aborting due to 20 previous errors +error: aborting due to 21 previous errors diff --git a/tests/ui/manual_map_option_2.fixed b/tests/ui/manual_map_option_2.fixed index 206c6d5d0776..b3f1c95a49c5 100644 --- a/tests/ui/manual_map_option_2.fixed +++ b/tests/ui/manual_map_option_2.fixed @@ -43,11 +43,6 @@ fn main() { 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 { @@ -65,31 +60,6 @@ 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, - }; - let _ = Some(0).map(|_| match f() { Ok(res) => Ok(Box::new(res)), _ => Err(()), @@ -98,48 +68,11 @@ mod with_type_coercion { #[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, - }); - 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(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(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(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 a47dc950760e..7689d69b204c 100644 --- a/tests/ui/manual_map_option_2.rs +++ b/tests/ui/manual_map_option_2.rs @@ -51,11 +51,6 @@ fn main() { 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 { @@ -85,31 +80,6 @@ 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, - }; - let _ = match Some(0) { //~^ manual_map Some(_) => Some(match f() { @@ -122,52 +92,15 @@ mod with_type_coercion { #[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, - }); - let _: Option> = match Some(0) { //~^ manual_map 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(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(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(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 c9e040ce1d0e..fbec640eef31 100644 --- a/tests/ui/manual_map_option_2.stderr +++ b/tests/ui/manual_map_option_2.stderr @@ -33,7 +33,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:65:17 + --> tests/ui/manual_map_option_2.rs:60:17 | LL | let _ = match Some(0) { | _________________^ @@ -44,7 +44,7 @@ LL | | }; | |_________^ help: try: `Some(0).map(|x| f(x))` error: manual implementation of `Option::map` - --> tests/ui/manual_map_option_2.rs:71:13 + --> tests/ui/manual_map_option_2.rs:66:13 | LL | let _ = match Some(0) { | _____________^ @@ -55,7 +55,7 @@ LL | | }; | |_____^ help: try: `Some(0).map(|x| unsafe { f(x) })` error: manual implementation of `Option::map` - --> tests/ui/manual_map_option_2.rs:76:13 + --> tests/ui/manual_map_option_2.rs:71:13 | LL | let _ = match Some(0) { | _____________^ @@ -66,7 +66,7 @@ LL | | }; | |_____^ help: try: `Some(0).map(|x| unsafe { f(x) })` error: manual implementation of `Option::map` - --> tests/ui/manual_map_option_2.rs:113:17 + --> tests/ui/manual_map_option_2.rs:83:17 | LL | let _ = match Some(0) { | _________________^ @@ -87,7 +87,7 @@ LL ~ }); | error: manual implementation of `Option::map` - --> tests/ui/manual_map_option_2.rs:136:37 + --> tests/ui/manual_map_option_2.rs:100:37 | LL | let _: Option> = match Some(0) { | _____________________________________^ diff --git a/tests/ui/manual_map_option_unfixable.rs b/tests/ui/manual_map_option_unfixable.rs new file mode 100644 index 000000000000..978ab0a54fa8 --- /dev/null +++ b/tests/ui/manual_map_option_unfixable.rs @@ -0,0 +1,118 @@ +//! In the following cases, the suggestion is incorrect due to coercion + +//@no-rustfix +#![warn(clippy::manual_map)] + +fn main() {} + +fn issue7077() { + let s = &String::new(); + #[allow(clippy::needless_match)] + let _: Option<&str> = match Some(s) { + //~^ manual_map + Some(s) => Some(s), + None => None, + }; +} + +fn pull_12712() { + let x: Option> = match Some(()) { + //~^ manual_map + Some(_) => Some(vec![b"1234"]), + None => None, + }; + + // type of `s` is coerced from `&String` to `&str` + let s = Some(String::new()); + let x: Option<(String, &str)> = match &s { + //~^ manual_map + Some(x) => Some({ if let Some(ref s) = s { (x.clone(), s) } else { panic!() } }), + None => None, + }; +} + +// issue #12659 +mod with_type_coercion { + trait DummyTrait {} + + fn foo Result>(f: F) { + let _: Option, ()>> = match Some(0) { + //~^ manual_map + Some(_) => Some(match f() { + Ok(res) => Ok(Box::new(res)), + _ => Err(()), + }), + None => None, + }; + + let _: Option> = match Some(()) { + //~^ manual_map + Some(_) => Some(Box::new(b"1234")), + None => None, + }; + + let x = String::new(); + let _: Option> = match Some(()) { + //~^ manual_map + Some(_) => Some(Box::new(&x)), + None => None, + }; + + let _: Option<&str> = match Some(()) { + //~^ manual_map + Some(_) => Some(&x), + None => None, + }; + } + + #[allow(clippy::redundant_allocation)] + fn bar() { + fn f(_: Option>) {} + + let x: &[u8; 4] = b"1234"; + f(match Some(()) { + //~^ manual_map + Some(_) => Some(Box::new(x)), + None => None, + }); + } + + fn with_fn_ret(s: &Option) -> Option<(String, &str)> { + // `map` doesn't work as the return type is adjusted. + match s { + //~^ manual_map + Some(x) => Some({ if let Some(s) = s { (x.clone(), s) } else { panic!() } }), + None => None, + } + } + + fn with_fn_ret_2(s: &Option) -> Option<(String, &str)> { + if true { + // `map` doesn't work as the return type is adjusted. + return match s { + //~^ manual_map + Some(x) => Some({ if let Some(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 { + //~^ manual_map + Some(x) => Some({ if let Some(s) = s { (x.clone(), s) } else { panic!() } }), + None => None, + } + }; + x + } +} + +pub fn issue14389(opt: Option<&'static i32>) -> Option<&'static i32> { + if let Some(o) = opt { Some(o) } else { None } + //~^ manual_map +} diff --git a/tests/ui/manual_map_option_unfixable.stderr b/tests/ui/manual_map_option_unfixable.stderr new file mode 100644 index 000000000000..449e83039aec --- /dev/null +++ b/tests/ui/manual_map_option_unfixable.stderr @@ -0,0 +1,140 @@ +error: manual implementation of `Option::map` + --> tests/ui/manual_map_option_unfixable.rs:11:27 + | +LL | let _: Option<&str> = match Some(s) { + | ___________________________^ +LL | | +LL | | Some(s) => Some(s), +LL | | None => None, +LL | | }; + | |_____^ help: try: `Some(s).map(|s| s)` + | + = note: `-D clippy::manual-map` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::manual_map)]` + +error: manual implementation of `Option::map` + --> tests/ui/manual_map_option_unfixable.rs:19:33 + | +LL | let x: Option> = match Some(()) { + | _________________________________^ +LL | | +LL | | Some(_) => Some(vec![b"1234"]), +LL | | None => None, +LL | | }; + | |_____^ help: try: `Some(()).map(|_| vec![b"1234"])` + +error: manual implementation of `Option::map` + --> tests/ui/manual_map_option_unfixable.rs:27:37 + | +LL | let x: Option<(String, &str)> = match &s { + | _____________________________________^ +LL | | +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_unfixable.rs:39:58 + | +LL | let _: Option, ()>> = match Some(0) { + | __________________________________________________________^ +LL | | +LL | | Some(_) => Some(match f() { +LL | | Ok(res) => Ok(Box::new(res)), +... | +LL | | None => None, +LL | | }; + | |_________^ + | +help: try + | +LL ~ let _: Option, ()>> = 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_unfixable.rs:48:37 + | +LL | let _: Option> = match Some(()) { + | _____________________________________^ +LL | | +LL | | Some(_) => Some(Box::new(b"1234")), +LL | | None => None, +LL | | }; + | |_________^ help: try: `Some(()).map(|_| Box::new(b"1234"))` + +error: manual implementation of `Option::map` + --> tests/ui/manual_map_option_unfixable.rs:55:36 + | +LL | let _: Option> = match Some(()) { + | ____________________________________^ +LL | | +LL | | Some(_) => Some(Box::new(&x)), +LL | | None => None, +LL | | }; + | |_________^ help: try: `Some(()).map(|_| Box::new(&x))` + +error: manual implementation of `Option::map` + --> tests/ui/manual_map_option_unfixable.rs:61:31 + | +LL | let _: Option<&str> = match Some(()) { + | _______________________________^ +LL | | +LL | | Some(_) => Some(&x), +LL | | None => None, +LL | | }; + | |_________^ help: try: `Some(()).map(|_| &x)` + +error: manual implementation of `Option::map` + --> tests/ui/manual_map_option_unfixable.rs:73:11 + | +LL | f(match Some(()) { + | ___________^ +LL | | +LL | | Some(_) => Some(Box::new(x)), +LL | | None => None, +LL | | }); + | |_________^ help: try: `Some(()).map(|_| Box::new(x))` + +error: manual implementation of `Option::map` + --> tests/ui/manual_map_option_unfixable.rs:82:9 + | +LL | / match s { +LL | | +LL | | Some(x) => Some({ if let Some(s) = s { (x.clone(), s) } else { panic!() } }), +LL | | None => None, +LL | | } + | |_________^ help: try: `s.as_ref().map(|x| { if let Some(s) = s { (x.clone(), s) } else { panic!() } })` + +error: manual implementation of `Option::map` + --> tests/ui/manual_map_option_unfixable.rs:92:20 + | +LL | return match s { + | ____________________^ +LL | | +LL | | Some(x) => Some({ if let Some(s) = s { (x.clone(), s) } else { panic!() } }), +LL | | None => None, +LL | | }; + | |_____________^ help: try: `s.as_ref().map(|x| { if let Some(s) = s { (x.clone(), s) } else { panic!() } })` + +error: manual implementation of `Option::map` + --> tests/ui/manual_map_option_unfixable.rs:105:13 + | +LL | / match s { +LL | | +LL | | Some(x) => Some({ if let Some(s) = s { (x.clone(), s) } else { panic!() } }), +LL | | None => None, +LL | | } + | |_____________^ help: try: `s.as_ref().map(|x| { if let Some(s) = s { (x.clone(), s) } else { panic!() } })` + +error: manual implementation of `Option::map` + --> tests/ui/manual_map_option_unfixable.rs:116:5 + | +LL | if let Some(o) = opt { Some(o) } else { None } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `opt.map(|o| o)` + +error: aborting due to 12 previous errors +