Auto merge of #10919 - y21:issue10579, r=blyxyas,xFrednet

[`map_unwrap_or`]: don't lint when referenced variable is moved

Fixes #10579.

The previous way of checking if changing `map(f).unwrap_or(a)` to `map_or(a, f)` is safe had a flaw when the argument to `unwrap_or` moves a binding and the `map` closure references that binding in some way.

It used to simply check if any of the identifiers in the `unwrap_or` argument are referenced in the `map` closure, but it didn't consider the case where the moved binding is referred to through references, for example:
```rs
let x = vec![1, 2];
let x_ref = &x;
Some(()).map(|_| x_ref.clone()).unwrap_or(x);
```
This compiles as is, but we cannot change it to `map_or`. This lint however did suggest changing it, because the simple way of checking if `x` is referenced anywhere in the `map` closure fails here. The safest thing to do here imo (what this PR does) is check if the moved value `x` is referenced *anywhere* in the body (before the `unwrap_or` call). One can always create a reference to the value and smuggle them into the closure, without actually referring to `x`. The original, linked issue shows another one such example:
```rs
    let x = vec![1,2,3,0];
    let y = x.strip_suffix(&[0]).map(|s| s.to_vec()).unwrap_or(x);
```
`x.strip_suffix(&[0])` creates a reference to `x` that is available through `s` inside of the `map` closure, so we can't change it to `map_or`.

changelog: [`map_unwrap_or`]: don't lint when referenced variable is moved

Author: bors

clippy_lints/src/methods/option_map_unwrap_or.rs

@@ -4,12 +4,17 @@ use clippy_utils::ty::is_copy;
 use clippy_utils::ty::is_type_diagnostic_item;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_errors::Applicability;
+use rustc_hir::def::Res;
 use rustc_hir::intravisit::{walk_path, Visitor};
+use rustc_hir::ExprKind;
+use rustc_hir::Node;
+use rustc_hir::PatKind;
+use rustc_hir::QPath;
 use rustc_hir::{self, HirId, Path};
 use rustc_lint::LateContext;
 use rustc_middle::hir::nested_filter;
 use rustc_span::source_map::Span;
-use rustc_span::{sym, Symbol};
+use rustc_span::sym;
 
 use super::MAP_UNWRAP_OR;
 
@@ -26,23 +31,41 @@ pub(super) fn check<'tcx>(
     // lint if the caller of `map()` is an `Option`
     if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(recv), sym::Option) {
         if !is_copy(cx, cx.typeck_results().expr_ty(unwrap_arg)) {
-            // Do not lint if the `map` argument uses identifiers in the `map`
-            // argument that are also used in the `unwrap_or` argument
+            // 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 map_expr_visitor = MapExprVisitor {
+            let mut reference_visitor = ReferenceVisitor {
                 cx,
                 identifiers: unwrap_visitor.identifiers,
-                found_identifier: false,
+                found_reference: false,
+                unwrap_or_span: unwrap_arg.span,
             };
-            map_expr_visitor.visit_expr(map_arg);
 
-            if map_expr_visitor.found_identifier {
+            let map = cx.tcx.hir();
+            let body = map.body(map.body_owned_by(map.enclosing_body_owner(expr.hir_id)));
+            reference_visitor.visit_body(body);
+
+            if reference_visitor.found_reference {
                 return;
             }
         }
@@ -91,14 +114,19 @@ pub(super) fn check<'tcx>(
 
 struct UnwrapVisitor<'a, 'tcx> {
     cx: &'a LateContext<'tcx>,
-    identifiers: FxHashSet<Symbol>,
+    identifiers: FxHashSet<HirId>,
 }
 
 impl<'a, 'tcx> Visitor<'tcx> for UnwrapVisitor<'a, 'tcx> {
     type NestedFilter = nested_filter::All;
 
-    fn visit_path(&mut self, path: &Path<'tcx>, _id: HirId) {
-        self.identifiers.insert(ident(path));
+    fn visit_path(&mut self, path: &Path<'tcx>, _: HirId) {
+        if let Res::Local(local_id) = path.res
+            && let Some(Node::Pat(pat)) = self.cx.tcx.hir().find(local_id)
+            && let PatKind::Binding(_, local_id, ..) = pat.kind
+        {
+            self.identifiers.insert(local_id);
+        }
         walk_path(self, path);
     }
 
@@ -107,32 +135,35 @@ impl<'a, 'tcx> Visitor<'tcx> for UnwrapVisitor<'a, 'tcx> {
     }
 }
 
-struct MapExprVisitor<'a, 'tcx> {
+struct ReferenceVisitor<'a, 'tcx> {
     cx: &'a LateContext<'tcx>,
-    identifiers: FxHashSet<Symbol>,
-    found_identifier: bool,
+    identifiers: FxHashSet<HirId>,
+    found_reference: bool,
+    unwrap_or_span: Span,
 }
 
-impl<'a, 'tcx> Visitor<'tcx> for MapExprVisitor<'a, 'tcx> {
+impl<'a, 'tcx> Visitor<'tcx> for ReferenceVisitor<'a, 'tcx> {
     type NestedFilter = nested_filter::All;
-
-    fn visit_path(&mut self, path: &Path<'tcx>, _id: HirId) {
-        if self.identifiers.contains(&ident(path)) {
-            self.found_identifier = true;
-            return;
+    fn visit_expr(&mut self, expr: &'tcx rustc_hir::Expr<'_>) {
+        // If we haven't found a reference yet, check if this references
+        // one of the locals that was moved in the `unwrap_or` argument.
+        // We are only interested in exprs that appear before the `unwrap_or` call.
+        if !self.found_reference {
+            if expr.span < self.unwrap_or_span
+                && let ExprKind::Path(ref path) = expr.kind
+                && let QPath::Resolved(_, path) = path
+                && let Res::Local(local_id) = path.res
+                && let Some(Node::Pat(pat)) = self.cx.tcx.hir().find(local_id)
+                && let PatKind::Binding(_, local_id, ..) = pat.kind
+                && self.identifiers.contains(&local_id)
+            {
+                self.found_reference = true;
+            }
+            rustc_hir::intravisit::walk_expr(self, expr);
         }
-        walk_path(self, path);
     }
 
     fn nested_visit_map(&mut self) -> Self::Map {
         self.cx.tcx.hir()
     }
 }
-
-fn ident(path: &Path<'_>) -> Symbol {
-    path.segments
-        .last()
-        .expect("segments should be composed of at least 1 element")
-        .ident
-        .name
-}

tests/ui/map_unwrap_or.rs

@@ -94,3 +94,32 @@ fn msrv_1_41() {
 
     let _ = res.map(|x| x + 1).unwrap_or_else(|_e| 0);
 }
+
+mod issue_10579 {
+    // Different variations of the same issue.
+    fn v1() {
+        let x = vec![1, 2, 3, 0];
+        let y = x.strip_suffix(&[0]).map(|s| s.to_vec()).unwrap_or(x);
+        println!("{y:?}");
+    }
+    fn v2() {
+        let x = vec![1, 2, 3, 0];
+        let y = Some(()).map(|_| x.to_vec()).unwrap_or(x);
+        println!("{y:?}");
+    }
+    fn v3() {
+        let x = vec![1, 2, 3, 0];
+        let xref = &x;
+        let y = Some(()).map(|_| xref.to_vec()).unwrap_or(x);
+        println!("{y:?}");
+    }
+    fn v4() {
+        struct VecInStruct {
+            v: Vec<u8>,
+        }
+        let s = VecInStruct { v: vec![1, 2, 3, 0] };
+
+        let y = Some(()).map(|_| s.v.clone()).unwrap_or(s.v);
+        println!("{y:?}");
+    }
+}