manual_{map,filter}: lint exprs with adjustments, but with reduced Applicability

Author: ada4a

clippy_lints/src/matches/manual_filter.rs

@@ -1,5 +1,5 @@
 use clippy_utils::as_some_expr;
-use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::res::{MaybeDef, MaybeQPath, MaybeResPath};
 use clippy_utils::visitors::contains_unsafe_block;
 
@@ -9,7 +9,7 @@ use rustc_lint::LateContext;
 use rustc_span::{SyntaxContext, sym};
 
 use super::MANUAL_FILTER;
-use super::manual_utils::{SomeExpr, check_with};
+use super::manual_utils::{SomeExpr, SuggInfo, check_with};
 
 // Function called on the <expr> of `[&+]Some((ref | ref mut) x) => <expr>`
 // Need to check if it's of the form `<expr>=if <cond> {<then_expr>} else {<else_expr>}`
@@ -133,7 +133,15 @@ fn check<'tcx>(
     else_pat: Option<&'tcx Pat<'_>>,
     else_body: &'tcx Expr<'_>,
 ) {
-    if let Some(sugg_info) = check_with(
+    if let Some(SuggInfo {
+        needs_brackets,
+        scrutinee_impl_copy,
+        scrutinee_str,
+        as_ref_str,
+        body_str,
+        app,
+        requires_coercion,
+    }) = check_with(
         cx,
         expr,
         scrutinee,
@@ -143,22 +151,27 @@ fn check<'tcx>(
         else_body,
         get_cond_expr,
     ) {
-        let body_str = add_ampersand_if_copy(sugg_info.body_str, sugg_info.scrutinee_impl_copy);
-        span_lint_and_sugg(
+        let body_str = add_ampersand_if_copy(body_str, scrutinee_impl_copy);
+        span_lint_and_then(
             cx,
             MANUAL_FILTER,
             expr.span,
             "manual implementation of `Option::filter`",
-            "try",
-            if sugg_info.needs_brackets {
-                format!(
-                    "{{ {}{}.filter({body_str}) }}",
-                    sugg_info.scrutinee_str, sugg_info.as_ref_str
-                )
-            } else {
-                format!("{}{}.filter({body_str})", sugg_info.scrutinee_str, sugg_info.as_ref_str)
+            |diag| {
+                diag.span_suggestion(
+                    expr.span,
+                    "try",
+                    if needs_brackets {
+                        format!("{{ {}{}.filter({body_str}) }}", scrutinee_str, as_ref_str)
+                    } else {
+                        format!("{}{}.filter({body_str})", scrutinee_str, as_ref_str)
+                    },
+                    app,
+                );
+                if requires_coercion {
+                    diag.note("you may need to add explicit `as` casts/dereferences if this `match` is coerced to another type");
+                }
             },
-            sugg_info.app,
         );
     }
 }

clippy_lints/src/matches/manual_map.rs

@@ -1,6 +1,6 @@
 use super::MANUAL_MAP;
-use super::manual_utils::{SomeExpr, check_with};
-use clippy_utils::diagnostics::span_lint_and_sugg;
+use super::manual_utils::{SomeExpr, SuggInfo, check_with};
+use clippy_utils::diagnostics::span_lint_and_then;
 
 use clippy_utils::res::{MaybeDef, MaybeQPath};
 use rustc_hir::LangItem::OptionSome;
@@ -42,7 +42,15 @@ fn check<'tcx>(
     else_pat: Option<&'tcx Pat<'_>>,
     else_body: &'tcx Expr<'_>,
 ) {
-    if let Some(sugg_info) = check_with(
+    if let Some(SuggInfo {
+        needs_brackets,
+        scrutinee_impl_copy: _,
+        scrutinee_str,
+        as_ref_str,
+        body_str,
+        app,
+        requires_coercion,
+    }) = check_with(
         cx,
         expr,
         scrutinee,
@@ -52,24 +60,26 @@ fn check<'tcx>(
         else_body,
         get_some_expr,
     ) {
-        span_lint_and_sugg(
+        span_lint_and_then(
             cx,
             MANUAL_MAP,
             expr.span,
             "manual implementation of `Option::map`",
-            "try",
-            if sugg_info.needs_brackets {
-                format!(
-                    "{{ {}{}.map({}) }}",
-                    sugg_info.scrutinee_str, sugg_info.as_ref_str, sugg_info.body_str
-                )
-            } else {
-                format!(
-                    "{}{}.map({})",
-                    sugg_info.scrutinee_str, sugg_info.as_ref_str, sugg_info.body_str
-                )
+            |diag| {
+                diag.span_suggestion(
+                    expr.span,
+                    "try",
+                    if needs_brackets {
+                        format!("{{ {}{}.map({}) }}", scrutinee_str, as_ref_str, body_str)
+                    } else {
+                        format!("{}{}.map({})", scrutinee_str, as_ref_str, body_str)
+                    },
+                    app,
+                );
+                if requires_coercion {
+                    diag.note("you may need to add explicit `as` casts/dereferences if this `match` is coerced to another type");
+                }
             },
-            sugg_info.app,
         );
     }
 }

clippy_lints/src/matches/manual_utils.rs

@@ -72,9 +72,12 @@ where
         return None;
     }
 
+    let mut app = Applicability::MachineApplicable;
+
     // `map` won't perform any adjustments.
-    if expr_requires_coercion(cx, expr) {
-        return None;
+    let requires_coercion = expr_requires_coercion(cx, expr);
+    if requires_coercion {
+        app = Applicability::MaybeIncorrect;
     }
 
     // Determine which binding mode to use.
@@ -111,8 +114,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;
@@ -172,16 +173,19 @@ where
         as_ref_str,
         body_str,
         app,
+        requires_coercion,
     })
 }
 
+#[expect(clippy::struct_excessive_bools)]
 pub struct SuggInfo<'a> {
     pub needs_brackets: bool,
     pub scrutinee_impl_copy: bool,
     pub scrutinee_str: String,
     pub as_ref_str: &'a str,
     pub body_str: String,
     pub app: Applicability,
+    pub requires_coercion: bool,
 }
 
 // Checks whether the expression could be passed as a function, or whether a closure is needed.

tests/ui/manual_filter_unfixable.rs

@@ -0,0 +1,17 @@
+//@no-rustfix
+#![warn(clippy::manual_filter)]
+
+// the suggestion is missing the necessary adjustments
+fn issue16031() -> Option<&'static i32> {
+    match Some(&&0) {
+        //~^ manual_filter
+        None => None,
+        Some(x) => {
+            if **x > 0 {
+                None
+            } else {
+                Some(x)
+            }
+        },
+    }
+}

tests/ui/manual_filter_unfixable.stderr

@@ -0,0 +1,18 @@
+error: manual implementation of `Option::filter`
+  --> tests/ui/manual_filter_unfixable.rs:6:5
+   |
+LL | /     match Some(&&0) {
+LL | |
+LL | |         None => None,
+LL | |         Some(x) => {
+...  |
+LL | |         },
+LL | |     }
+   | |_____^ help: try: `Some(&&0).filter(|&x| **x <= 0)`
+   |
+   = note: the expression in `filter` might need additional coercion
+   = note: `-D clippy::manual-filter` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::manual_filter)]`
+
+error: aborting due to 1 previous error
+

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<Vec<&[u8]>> = 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,
-    };
 }

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<Vec<&[u8]>> = 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,
-    };
 }

tests/ui/manual_map_option.stderr

@@ -173,7 +173,27 @@ 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])`
+   |
+note: the expression might need additional coercion
+  --> tests/ui/manual_map_option.rs:185:5
+   |
+LL | /     match Some(0) {
+LL | |
+LL | |         Some(x) => Some(vec![x]),
+LL | |         None => None,
+LL | |     };
+   | |_____^
+
+error: manual implementation of `Option::map`
+  --> tests/ui/manual_map_option.rs:191:5
    |
 LL | /     match option_env!("") {
 LL | |
@@ -183,7 +203,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 +215,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 +226,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