fix [manual_map] not catching type adjustment

J-ZhengLi · J-ZhengLi · commit b38642793f85 · 2024-07-01T11:11:58.000+08:00
diff --git a/clippy_lints/src/matches/manual_utils.rs b/clippy_lints/src/matches/manual_utils.rs
@@ -11,8 +11,9 @@ 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_span::{sym, SyntaxContext};
 
 #[expect(clippy::too_many_arguments)]
@@ -65,15 +66,7 @@ where
 
     let some_expr = get_some_expr_fn(cx, some_pat, some_expr, expr_ctxt)?;
 
-    // These two lints will go back and forth with each other.
-    if cx.typeck_results().expr_ty(some_expr.expr) == cx.tcx.types.unit
-        && !is_lint_allowed(cx, OPTION_MAP_UNIT_FN, expr.hir_id)
-    {
-        return None;
-    }
-
-    // `map` won't perform any adjustments.
-    if !cx.typeck_results().expr_adjustments(some_expr.expr).is_empty() {
+    if !should_lint(cx, expr, some_expr.expr) {
         return None;
     }
 
@@ -274,3 +267,79 @@ 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.
+        .filter(|adj| !matches!(adj.kind, Adjust::NeverToAny))
+        .next()
+        .is_some()
+}
+
+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, _) => {
+            args.iter().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))
+        },
+        // 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.map(|e| expr_has_type_coercion(cx, e)).unwrap_or_default()
+        }
+        ExprKind::Match(_, arms, _) => {
+            arms.iter().map(|arm| arm.body).any(|body| expr_has_type_coercion(cx, body))
+        }
+        _ => false
+    }
+}
+
+fn should_lint<'tcx>(cx: &LateContext<'tcx>, match_expr: &Expr<'tcx>, some_expr: &Expr<'tcx>) -> bool {
+    let typeck = cx.typeck_results();
+    let some_expr_ty = typeck.expr_ty(some_expr);
+
+    // These two lints will go back and forth with each other.
+    if some_expr_ty.is_unit() && !is_lint_allowed(cx, OPTION_MAP_UNIT_FN, match_expr.hir_id) {
+        return false;
+    }
+
+    // `map` won't perform any adjustments.
+    // However, if the ty of `Some` arg is unit, type coercions should be fine.
+    if !some_expr_ty.is_unit()
+        // TODO: this needs to be tested
+        && !some_expr_ty.is_scalar()
+        && expr_has_type_coercion(cx, match_expr)
+    {
+        return false;
+    }
+
+    true
+}
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,92 @@ 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<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(()),
+            });
+    }
+
+    #[allow(clippy::redundant_allocation)]
+    fn bar() {
+        fn f(_: Option<Box<&[u8]>>) {}
+        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,
+        });
+
+        // FIXME: This is a known FN case.
+        let _: Option<Box<&[u8]>> = match Some(0) {
+            Some(_) => Some(g(x)),
+            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.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<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,
+        };
+    }
+
+    #[allow(clippy::redundant_allocation)]
+    fn bar() {
+        fn f(_: Option<Box<&[u8]>>) {}
+        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,
+        });
+
+        // FIXME: This is a known FN case.
+        let _: Option<Box<&[u8]>> = match Some(0) {
+            Some(_) => Some(g(x)),
+            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: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: aborting due to 6 previous errors
 
