Mirrors/rust-clippy
2
0
Fork 0
mirror of https://github.com/rust-lang/rust-clippy synced 2024-11-27 23:20:39 +00:00
Code Issues Projects Releases Packages Wiki Activity

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

Browse source 
[`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
This commit is contained in:
bors 2023-06-14 09:48:22 +00:00
commit a59236ffb4
2 changed files with 97 additions and 37 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

105
clippy_lints/src/methods/option_map_unwrap_or.rs
View file

@ -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()
}
}

29
tests/ui/map_unwrap_or.rs
View file

@ -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:?}");
}
}