fix [manual_map] not catching type adjustment

J-ZhengLi · J-ZhengLi · commit 00bb73f9468c · 2024-05-10T16:20:12.000+08:00
diff --git 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(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
@@ -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<T: DummyTrait, F: Fn() -> Result<T, ()>>(f: F) {
+        // Don't lint
+        let _: Option<Result<Box<dyn DummyTrait>, ()>> = match Some(0) {
+            Some(_) => Some(match f() {
+                Ok(res) => Ok(Box::new(res)),
+                _ => Err(()),
+            }),
+            None => None,
+        };
+
+        let _: Option<Box<&[u8]>> = match Some(()) {
+            Some(_) => Some(Box::new(b"1234")),
+            None => None,
+        };
+
+        let x = String::new();
+        let _: Option<Box<&str>> = 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<String>) -> 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<String>) -> 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<String>) -> 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
@@ -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<T: DummyTrait, F: Fn() -> Result<T, ()>>(f: F) {
+        // Don't lint
+        let _: Option<Result<Box<dyn DummyTrait>, ()>> = match Some(0) {
+            Some(_) => Some(match f() {
+                Ok(res) => Ok(Box::new(res)),
+                _ => Err(()),
+            }),
+            None => None,
+        };
+
+        let _: Option<Box<&[u8]>> = match Some(()) {
+            Some(_) => Some(Box::new(b"1234")),
+            None => None,
+        };
+
+        let x = String::new();
+        let _: Option<Box<&str>> = 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<String>) -> 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<String>) -> 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<String>) -> 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
@@ -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
 
