diff --git a/clippy_lints/src/methods/option_map_unwrap_or.rs b/clippy_lints/src/methods/option_map_unwrap_or.rs
index 4c6328481e4..e931c6277f4 100644
--- a/clippy_lints/src/methods/option_map_unwrap_or.rs
+++ b/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,8 +31,22 @@ 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,
@@ -35,14 +54,18 @@ pub(super) fn check<'tcx>(
};
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,35 +114,18 @@ 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));
- walk_path(self, path);
- }
-
- fn nested_visit_map(&mut self) -> Self::Map {
- self.cx.tcx.hir()
- }
-}
-
-struct MapExprVisitor<'a, 'tcx> {
- cx: &'a LateContext<'tcx>,
- identifiers: FxHashSet<Symbol>,
- found_identifier: bool,
-}
-
-impl<'a, 'tcx> Visitor<'tcx> for MapExprVisitor<'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_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);
}
@@ -129,10 +135,35 @@ impl<'a, 'tcx> Visitor<'tcx> for MapExprVisitor<'a, 'tcx> {
}
}
-fn ident(path: &Path<'_>) -> Symbol {
- path.segments
- .last()
- .expect("segments should be composed of at least 1 element")
- .ident
- .name
+struct ReferenceVisitor<'a, 'tcx> {
+ cx: &'a LateContext<'tcx>,
+ identifiers: FxHashSet<HirId>,
+ found_reference: bool,
+ unwrap_or_span: Span,
+}
+
+impl<'a, 'tcx> Visitor<'tcx> for ReferenceVisitor<'a, 'tcx> {
+ type NestedFilter = nested_filter::All;
+ 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);
+ }
+ }
+
+ fn nested_visit_map(&mut self) -> Self::Map {
+ self.cx.tcx.hir()
+ }
}
diff --git a/tests/ui/map_unwrap_or.rs b/tests/ui/map_unwrap_or.rs
index cb25d8567aa..0cd49dd7760 100644
--- a/tests/ui/map_unwrap_or.rs
+++ b/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:?}");
+ }
+}