fix: map_unwrap_or suggests wrongly for empty slice

Author: profetia

clippy_lints/src/derivable_impls.rs

@@ -105,7 +105,7 @@ fn check_struct<'tcx>(
     if let TyKind::Path(QPath::Resolved(_, p)) = self_ty.kind
         && let Some(PathSegment { args, .. }) = p.segments.last()
     {
-        let args = args.map(|a| a.args).unwrap_or(&[]);
+        let args = args.map(|a| a.args).unwrap_or_default();
 
         // ty_args contains the generic parameters of the type declaration, while args contains the
         // arguments used at instantiation time. If both len are not equal, it means that some

clippy_lints/src/methods/map_unwrap_or.rs

@@ -1,12 +1,13 @@
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::msrvs::{self, Msrv};
 use clippy_utils::source::snippet_with_applicability;
-use clippy_utils::ty::{is_copy, is_type_diagnostic_item};
+use clippy_utils::ty::{get_type_diagnostic_name, is_copy};
+use clippy_utils::{is_res_lang_ctor, path_res};
 use rustc_data_structures::fx::FxHashSet;
 use rustc_errors::Applicability;
 use rustc_hir::def::Res;
 use rustc_hir::intravisit::{Visitor, walk_path};
-use rustc_hir::{ExprKind, HirId, Node, PatKind, Path, QPath};
+use rustc_hir::{ExprKind, HirId, LangItem, Node, PatKind, Path, QPath};
 use rustc_lint::LateContext;
 use rustc_middle::hir::nested_filter;
 use rustc_span::{Span, sym};
@@ -27,116 +28,120 @@ pub(super) fn check<'tcx>(
     msrv: Msrv,
 ) {
     let recv_ty = cx.typeck_results().expr_ty(recv).peel_refs();
-    let is_option = is_type_diagnostic_item(cx, recv_ty, sym::Option);
-    let is_result = is_type_diagnostic_item(cx, recv_ty, sym::Result);
+    let is_option = match get_type_diagnostic_name(cx, recv_ty) {
+        Some(sym::Option) => true,
+        Some(sym::Result) if msrv.meets(cx, msrvs::RESULT_MAP_OR) => false,
+        _ => return,
+    };
+
+    let unwrap_arg_ty = cx.typeck_results().expr_ty(unwrap_arg);
+    if !is_copy(cx, unwrap_arg_ty) {
+        // Replacing `.map(<f>).unwrap_or(<a>)` with `.map_or(<a>, <f>)` can sometimes lead to
+        // borrowck errors, see #10579 for one such instance.
+        // In particular, if `a` causes a move and `f` references that moved binding, then we cannot lint:
+        // ```
+        // let x = vec![1, 2];
+        // x.get(0..1).map(|s| s.to_vec()).unwrap_or(x);
+        // ```
+        // This compiles, but changing it to `map_or` will produce a compile error:
+        // ```
+        // let x = vec![1, 2];
+        // x.get(0..1).map_or(x, |s| s.to_vec())
+        //                    ^ moving `x` here
+        // ^^^^^^^^^^^ while it is borrowed here (and later used in the closure)
+        // ```
+        // So, we have to check that `a` is not referenced anywhere (even outside of the `.map` closure!)
+        // before the call to `unwrap_or`.
+
+        let mut unwrap_visitor = UnwrapVisitor {
+            cx,
+            identifiers: FxHashSet::default(),
+        };
+        unwrap_visitor.visit_expr(unwrap_arg);
 
-    if is_result && !msrv.meets(cx, msrvs::RESULT_MAP_OR) {
-        return;
-    }
+        let mut reference_visitor = ReferenceVisitor {
+            cx,
+            identifiers: unwrap_visitor.identifiers,
+            unwrap_or_span: unwrap_arg.span,
+        };
 
-    // lint if the caller of `map()` is an `Option`
-    if is_option || is_result {
-        if !is_copy(cx, cx.typeck_results().expr_ty(unwrap_arg)) {
-            // Replacing `.map(<f>).unwrap_or(<a>)` with `.map_or(<a>, <f>)` can sometimes lead to
-            // borrowck errors, see #10579 for one such instance.
-            // In particular, if `a` causes a move and `f` references that moved binding, then we cannot lint:
-            // ```
-            // let x = vec![1, 2];
-            // x.get(0..1).map(|s| s.to_vec()).unwrap_or(x);
-            // ```
-            // This compiles, but changing it to `map_or` will produce a compile error:
-            // ```
-            // let x = vec![1, 2];
-            // x.get(0..1).map_or(x, |s| s.to_vec())
-            //                    ^ moving `x` here
-            // ^^^^^^^^^^^ while it is borrowed here (and later used in the closure)
-            // ```
-            // So, we have to check that `a` is not referenced anywhere (even outside of the `.map` closure!)
-            // before the call to `unwrap_or`.
-
-            let mut unwrap_visitor = UnwrapVisitor {
-                cx,
-                identifiers: FxHashSet::default(),
-            };
-            unwrap_visitor.visit_expr(unwrap_arg);
-
-            let mut reference_visitor = ReferenceVisitor {
-                cx,
-                identifiers: unwrap_visitor.identifiers,
-                unwrap_or_span: unwrap_arg.span,
-            };
-
-            let body = cx.tcx.hir_body_owned_by(cx.tcx.hir_enclosing_body_owner(expr.hir_id));
-
-            // Visit the body, and return if we've found a reference
-            if reference_visitor.visit_body(body).is_break() {
-                return;
-            }
-        }
+        let body = cx.tcx.hir_body_owned_by(cx.tcx.hir_enclosing_body_owner(expr.hir_id));
 
-        if !unwrap_arg.span.eq_ctxt(map_span) {
+        // Visit the body, and return if we've found a reference
+        if reference_visitor.visit_body(body).is_break() {
             return;
         }
+    }
+
+    if !unwrap_arg.span.eq_ctxt(map_span) {
+        return;
+    }
 
-        // is_some_and is stabilised && `unwrap_or` argument is false; suggest `is_some_and` instead
-        let suggest_is_some_and = matches!(&unwrap_arg.kind, ExprKind::Lit(lit)
+    // is_some_and is stabilised && `unwrap_or` argument is false; suggest `is_some_and` instead
+    let suggest_is_some_and = matches!(&unwrap_arg.kind, ExprKind::Lit(lit)
             if matches!(lit.node, rustc_ast::LitKind::Bool(false)))
-            && msrv.meets(cx, msrvs::OPTION_RESULT_IS_VARIANT_AND);
-
-        let mut applicability = Applicability::MachineApplicable;
-        // get snippet for unwrap_or()
-        let unwrap_snippet = snippet_with_applicability(cx, unwrap_arg.span, "..", &mut applicability);
-        // lint message
-        // comparing the snippet from source to raw text ("None") below is safe
-        // because we already have checked the type.
-        let unwrap_snippet_none = is_option && unwrap_snippet == "None";
-        let arg = if unwrap_snippet_none {
-            "None"
-        } else if suggest_is_some_and {
-            "false"
-        } else {
-            "<a>"
-        };
-        let suggest = if unwrap_snippet_none {
-            "and_then(<f>)"
-        } else if suggest_is_some_and {
-            if is_result {
-                "is_ok_and(<f>)"
-            } else {
-                "is_some_and(<f>)"
-            }
+        && msrv.meets(cx, msrvs::OPTION_RESULT_IS_VARIANT_AND);
+
+    let mut applicability = Applicability::MachineApplicable;
+    // get snippet for unwrap_or()
+    let unwrap_snippet = snippet_with_applicability(cx, unwrap_arg.span, "..", &mut applicability);
+    // lint message
+    // comparing the snippet from source to raw text ("None") below is safe
+    // because we already have checked the type.
+    let unwrap_snippet_none = is_option && is_res_lang_ctor(cx, path_res(cx, unwrap_arg), LangItem::OptionNone);
+    let arg = if unwrap_snippet_none {
+        "None"
+    } else if suggest_is_some_and {
+        "false"
+    } else {
+        "<a>"
+    };
+    let suggest = if unwrap_snippet_none {
+        "and_then(<f>)"
+    } else if suggest_is_some_and {
+        if is_option {
+            "is_some_and(<f>)"
         } else {
-            "map_or(<a>, <f>)"
-        };
-        let msg = format!(
-            "called `map(<f>).unwrap_or({arg})` on an `{}` value",
-            if is_option { "Option" } else { "Result" }
-        );
-
-        span_lint_and_then(cx, MAP_UNWRAP_OR, expr.span, msg, |diag| {
-            let map_arg_span = map_arg.span;
-
-            let mut suggestion = vec![
-                (
-                    map_span,
-                    String::from(if unwrap_snippet_none {
-                        "and_then"
-                    } else if suggest_is_some_and {
-                        if is_result { "is_ok_and" } else { "is_some_and" }
-                    } else {
-                        "map_or"
-                    }),
-                ),
-                (expr.span.with_lo(unwrap_recv.span.hi()), String::new()),
-            ];
-
-            if !unwrap_snippet_none && !suggest_is_some_and {
-                suggestion.push((map_arg_span.with_hi(map_arg_span.lo()), format!("{unwrap_snippet}, ")));
-            }
-
-            diag.multipart_suggestion(format!("use `{suggest}` instead"), suggestion, applicability);
-        });
-    }
+            "is_ok_and(<f>)"
+        }
+    } else {
+        "map_or(<a>, <f>)"
+    };
+    let msg = format!(
+        "called `map(<f>).unwrap_or({arg})` on an `{}` value",
+        if is_option { "Option" } else { "Result" }
+    );
+
+    span_lint_and_then(cx, MAP_UNWRAP_OR, expr.span, msg, |diag| {
+        let map_arg_span = map_arg.span;
+
+        let mut suggestion = vec![
+            (
+                map_span,
+                String::from(if unwrap_snippet_none {
+                    "and_then"
+                } else if suggest_is_some_and {
+                    if is_option { "is_some_and" } else { "is_ok_and" }
+                } else {
+                    let unwrap_arg_ty = unwrap_arg_ty.peel_refs();
+                    if unwrap_arg_ty.is_array()
+                        && let unwrap_arg_ty_adj = cx.typeck_results().expr_ty_adjusted(unwrap_arg).peel_refs()
+                        && unwrap_arg_ty_adj.is_slice()
+                    {
+                        return;
+                    }
+                    "map_or"
+                }),
+            ),
+            (expr.span.with_lo(unwrap_recv.span.hi()), String::new()),
+        ];
+
+        if !unwrap_snippet_none && !suggest_is_some_and {
+            suggestion.push((map_arg_span.with_hi(map_arg_span.lo()), format!("{unwrap_snippet}, ")));
+        }
+
+        diag.multipart_suggestion(format!("use `{suggest}` instead"), suggestion, applicability);
+    });
 }
 
 struct UnwrapVisitor<'a, 'tcx> {

clippy_lints/src/methods/map_unwrap_or_else.rs

@@ -1,7 +1,7 @@
 use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg};
 use clippy_utils::msrvs::{self, Msrv};
 use clippy_utils::source::snippet;
-use clippy_utils::ty::is_type_diagnostic_item;
+use clippy_utils::ty::get_type_diagnostic_name;
 use clippy_utils::usage::mutated_variables;
 use rustc_errors::Applicability;
 use rustc_hir as hir;
@@ -22,55 +22,52 @@ pub(super) fn check<'tcx>(
     msrv: Msrv,
 ) -> bool {
     let recv_ty = cx.typeck_results().expr_ty(recv).peel_refs();
-    let is_option = is_type_diagnostic_item(cx, recv_ty, sym::Option);
-    let is_result = is_type_diagnostic_item(cx, recv_ty, sym::Result);
+    let is_option = match get_type_diagnostic_name(cx, recv_ty) {
+        Some(sym::Option) => true,
+        Some(sym::Result) if msrv.meets(cx, msrvs::RESULT_MAP_OR_ELSE) => false,
+        _ => return false,
+    };
 
-    if is_result && !msrv.meets(cx, msrvs::RESULT_MAP_OR_ELSE) {
-        return false;
-    }
-
-    if is_option || is_result {
-        // Don't make a suggestion that may fail to compile due to mutably borrowing
-        // the same variable twice.
-        let map_mutated_vars = mutated_variables(recv, cx);
-        let unwrap_mutated_vars = mutated_variables(unwrap_arg, cx);
-        if let (Some(map_mutated_vars), Some(unwrap_mutated_vars)) = (map_mutated_vars, unwrap_mutated_vars) {
-            if map_mutated_vars.intersection(&unwrap_mutated_vars).next().is_some() {
-                return false;
-            }
-        } else {
+    // Don't make a suggestion that may fail to compile due to mutably borrowing
+    // the same variable twice.
+    let map_mutated_vars = mutated_variables(recv, cx);
+    let unwrap_mutated_vars = mutated_variables(unwrap_arg, cx);
+    if let (Some(map_mutated_vars), Some(unwrap_mutated_vars)) = (map_mutated_vars, unwrap_mutated_vars) {
+        if map_mutated_vars.intersection(&unwrap_mutated_vars).next().is_some() {
             return false;
         }
+    } else {
+        return false;
+    }
 
-        // lint message
-        let msg = if is_option {
-            "called `map(<f>).unwrap_or_else(<g>)` on an `Option` value"
-        } else {
-            "called `map(<f>).unwrap_or_else(<g>)` on a `Result` value"
-        };
-        // get snippets for args to map() and unwrap_or_else()
-        let map_snippet = snippet(cx, map_arg.span, "..");
-        let unwrap_snippet = snippet(cx, unwrap_arg.span, "..");
-        // lint, with note if neither arg is > 1 line and both map() and
-        // unwrap_or_else() have the same span
-        let multiline = map_snippet.lines().count() > 1 || unwrap_snippet.lines().count() > 1;
-        let same_span = map_arg.span.eq_ctxt(unwrap_arg.span);
-        if same_span && !multiline {
-            let var_snippet = snippet(cx, recv.span, "..");
-            span_lint_and_sugg(
-                cx,
-                MAP_UNWRAP_OR,
-                expr.span,
-                msg,
-                "try",
-                format!("{var_snippet}.map_or_else({unwrap_snippet}, {map_snippet})"),
-                Applicability::MachineApplicable,
-            );
-            return true;
-        } else if same_span && multiline {
-            span_lint(cx, MAP_UNWRAP_OR, expr.span, msg);
-            return true;
-        }
+    // lint message
+    let msg = if is_option {
+        "called `map(<f>).unwrap_or_else(<g>)` on an `Option` value"
+    } else {
+        "called `map(<f>).unwrap_or_else(<g>)` on a `Result` value"
+    };
+    // get snippets for args to map() and unwrap_or_else()
+    let map_snippet = snippet(cx, map_arg.span, "..");
+    let unwrap_snippet = snippet(cx, unwrap_arg.span, "..");
+    // lint, with note if neither arg is > 1 line and both map() and
+    // unwrap_or_else() have the same span
+    let multiline = map_snippet.lines().count() > 1 || unwrap_snippet.lines().count() > 1;
+    let same_span = map_arg.span.eq_ctxt(unwrap_arg.span);
+    if same_span && !multiline {
+        let var_snippet = snippet(cx, recv.span, "..");
+        span_lint_and_sugg(
+            cx,
+            MAP_UNWRAP_OR,
+            expr.span,
+            msg,
+            "try",
+            format!("{var_snippet}.map_or_else({unwrap_snippet}, {map_snippet})"),
+            Applicability::MachineApplicable,
+        );
+        return true;
+    } else if same_span && multiline {
+        span_lint(cx, MAP_UNWRAP_OR, expr.span, msg);
+        return true;
     }
 
     false

tests/ui/map_unwrap_or.rs

@@ -156,3 +156,11 @@ mod issue_10579 {
         println!("{y:?}");
     }
 }
+
+fn issue15752() {
+    struct Foo<'a>(&'a [u32]);
+
+    let x = Some(Foo(&[1, 2, 3]));
+    x.map(|y| y.0).unwrap_or(&[]);
+    //~^ map_unwrap_or
+}

tests/ui/map_unwrap_or.stderr

@@ -199,5 +199,11 @@ LL -     let _ = opt.map(|x| x > 5).unwrap_or(false);
 LL +     let _ = opt.is_some_and(|x| x > 5);
    |
 
-error: aborting due to 15 previous errors
+error: called `map(<f>).unwrap_or(<a>)` on an `Option` value
+  --> tests/ui/map_unwrap_or.rs:164:5
+   |
+LL |     x.map(|y| y.0).unwrap_or(&[]);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 16 previous errors