From 9d58a426230b5b4200791b9bedee1b14cb971197 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sat, 10 Jun 2023 01:38:04 +0200 Subject: [PATCH] [`map_unwrap_or`]: don't lint when referenced variable is moved --- .../src/methods/option_map_unwrap_or.rs | 105 ++++++++++++------ tests/ui/map_unwrap_or.rs | 29 +++++ 2 files changed, 97 insertions(+), 37 deletions(-) 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().unwrap_or()` with `.map_or(, )` 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, + identifiers: FxHashSet, } 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, - 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, + 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, + } + let s = VecInStruct { v: vec![1, 2, 3, 0] }; + + let y = Some(()).map(|_| s.v.clone()).unwrap_or(s.v); + println!("{y:?}"); + } +}