From 4838c78ba4ef784379ae6ec5617479de2a32d3f6 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sun, 1 Aug 2021 18:39:56 -0400 Subject: [PATCH 1/6] Improve `manual_map` and `map_entry` Locals which can be partially moved created within the to-be-created closure shouldn't block the use of a closure --- clippy_lints/src/entry.rs | 31 ++++++++++++++------ clippy_utils/src/lib.rs | 44 +++++++++++++++++++++++------ tests/ui/entry.fixed | 15 +++++----- tests/ui/entry.rs | 9 +++--- tests/ui/entry.stderr | 16 ++++++----- tests/ui/entry_btree.fixed | 18 ++++++++++++ tests/ui/entry_btree.rs | 18 ++++++++++++ tests/ui/entry_btree.stderr | 20 +++++++++++++ tests/ui/manual_map_option_2.fixed | 10 +++++++ tests/ui/manual_map_option_2.rs | 13 +++++++++ tests/ui/manual_map_option_2.stderr | 24 ++++++++++++++++ 11 files changed, 181 insertions(+), 37 deletions(-) create mode 100644 tests/ui/entry_btree.fixed create mode 100644 tests/ui/entry_btree.rs create mode 100644 tests/ui/entry_btree.stderr create mode 100644 tests/ui/manual_map_option_2.fixed create mode 100644 tests/ui/manual_map_option_2.rs create mode 100644 tests/ui/manual_map_option_2.stderr diff --git a/clippy_lints/src/entry.rs b/clippy_lints/src/entry.rs index e1d0d65edb1b..7fb8e4276600 100644 --- a/clippy_lints/src/entry.rs +++ b/clippy_lints/src/entry.rs @@ -7,8 +7,9 @@ use clippy_utils::{ }; use rustc_errors::Applicability; use rustc_hir::{ + hir_id::HirIdSet, intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor}, - Block, Expr, ExprKind, Guard, HirId, Local, Stmt, StmtKind, UnOp, + Block, Expr, ExprKind, Guard, HirId, Pat, Stmt, StmtKind, UnOp, }; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -336,6 +337,8 @@ struct InsertSearcher<'cx, 'tcx> { edits: Vec>, /// A stack of loops the visitor is currently in. loops: Vec, + /// Local variables created in the expression. These don't need to be captured. + locals: HirIdSet, } impl<'tcx> InsertSearcher<'_, 'tcx> { /// Visit the expression as a branch in control flow. Multiple insert calls can be used, but @@ -383,13 +386,16 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> { } }, StmtKind::Expr(e) => self.visit_expr(e), - StmtKind::Local(Local { init: Some(e), .. }) => { - self.allow_insert_closure &= !self.in_tail_pos; - self.in_tail_pos = false; - self.is_single_insert = false; - self.visit_expr(e); + StmtKind::Local(l) => { + self.visit_pat(l.pat); + if let Some(e) = l.init { + self.allow_insert_closure &= !self.in_tail_pos; + self.in_tail_pos = false; + self.is_single_insert = false; + self.visit_expr(e); + } }, - _ => { + StmtKind::Item(_) => { self.allow_insert_closure &= !self.in_tail_pos; self.is_single_insert = false; }, @@ -471,6 +477,7 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> { // Each branch may contain it's own insert expression. let mut is_map_used = self.is_map_used; for arm in arms { + self.visit_pat(arm.pat); if let Some(Guard::If(guard) | Guard::IfLet(_, guard)) = arm.guard { self.visit_non_tail_expr(guard); } @@ -496,7 +503,8 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> { }, _ => { self.allow_insert_closure &= !self.in_tail_pos; - self.allow_insert_closure &= can_move_expr_to_closure_no_visit(self.cx, expr, &self.loops); + self.allow_insert_closure &= + can_move_expr_to_closure_no_visit(self.cx, expr, &self.loops, &self.locals); // Sub expressions are no longer in the tail position. self.is_single_insert = false; self.in_tail_pos = false; @@ -505,6 +513,12 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> { }, } } + + fn visit_pat(&mut self, p: &'tcx Pat<'tcx>) { + p.each_binding_or_first(&mut |_, id, _, _| { + self.locals.insert(id); + }); + } } struct InsertSearchResults<'tcx> { @@ -630,6 +644,7 @@ fn find_insert_calls( in_tail_pos: true, is_single_insert: true, loops: Vec::new(), + locals: HirIdSet::default(), }; s.visit_expr(expr); let allow_insert_closure = s.allow_insert_closure; diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 1d59d6bfea1b..aee9f791b039 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -67,6 +67,7 @@ use rustc_data_structures::unhash::UnhashMap; use rustc_hir as hir; use rustc_hir::def::{DefKind, Res}; use rustc_hir::def_id::DefId; +use rustc_hir::hir_id::HirIdSet; use rustc_hir::intravisit::{self, walk_expr, ErasedMap, FnKind, NestedVisitorMap, Visitor}; use rustc_hir::LangItem::{ResultErr, ResultOk}; use rustc_hir::{ @@ -626,7 +627,12 @@ pub fn can_mut_borrow_both(cx: &LateContext<'_>, e1: &Expr<'_>, e2: &Expr<'_>) - } /// Checks if the top level expression can be moved into a closure as is. -pub fn can_move_expr_to_closure_no_visit(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, jump_targets: &[HirId]) -> bool { +pub fn can_move_expr_to_closure_no_visit( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'_>, + jump_targets: &[HirId], + ignore_locals: &HirIdSet, +) -> bool { match expr.kind { ExprKind::Break(Destination { target_id: Ok(id), .. }, _) | ExprKind::Continue(Destination { target_id: Ok(id), .. }) @@ -642,15 +648,24 @@ pub fn can_move_expr_to_closure_no_visit(cx: &LateContext<'tcx>, expr: &'tcx Exp | ExprKind::LlvmInlineAsm(_) => false, // Accessing a field of a local value can only be done if the type isn't // partially moved. - ExprKind::Field(base_expr, _) - if matches!( - base_expr.kind, - ExprKind::Path(QPath::Resolved(_, Path { res: Res::Local(_), .. })) - ) && can_partially_move_ty(cx, cx.typeck_results().expr_ty(base_expr)) => - { + ExprKind::Field( + &Expr { + hir_id, + kind: + ExprKind::Path(QPath::Resolved( + _, + Path { + res: Res::Local(local_id), + .. + }, + )), + .. + }, + _, + ) if !ignore_locals.contains(local_id) && can_partially_move_ty(cx, cx.typeck_results().node_type(hir_id)) => { // TODO: check if the local has been partially moved. Assume it has for now. false - } + }, _ => true, } } @@ -659,7 +674,11 @@ pub fn can_move_expr_to_closure_no_visit(cx: &LateContext<'tcx>, expr: &'tcx Exp pub fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool { struct V<'cx, 'tcx> { cx: &'cx LateContext<'tcx>, + // Stack of potential break targets contained in the expression. loops: Vec, + /// Local variables created in the expression. These don't need to be captured. + locals: HirIdSet, + /// Whether this expression can be turned into a closure. allow_closure: bool, } impl Visitor<'tcx> for V<'_, 'tcx> { @@ -677,16 +696,23 @@ pub fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> self.visit_block(b); self.loops.pop(); } else { - self.allow_closure &= can_move_expr_to_closure_no_visit(self.cx, e, &self.loops); + self.allow_closure &= can_move_expr_to_closure_no_visit(self.cx, e, &self.loops, &self.locals); walk_expr(self, e); } } + + fn visit_pat(&mut self, p: &'tcx Pat<'tcx>) { + p.each_binding_or_first(&mut |_, id, _, _| { + self.locals.insert(id); + }); + } } let mut v = V { cx, allow_closure: true, loops: Vec::new(), + locals: HirIdSet::default(), }; v.visit_expr(expr); v.allow_closure diff --git a/tests/ui/entry.fixed b/tests/ui/entry.fixed index cfad3090ba38..8a36ec833d76 100644 --- a/tests/ui/entry.fixed +++ b/tests/ui/entry.fixed @@ -4,7 +4,7 @@ #![warn(clippy::map_entry)] #![feature(asm)] -use std::collections::{BTreeMap, HashMap}; +use std::collections::HashMap; use std::hash::Hash; macro_rules! m { @@ -142,14 +142,13 @@ fn hash_map(m: &mut HashMap, m2: &mut HashMa if !m.contains_key(&k) { insert!(m, k, v); } -} -fn btree_map(m: &mut BTreeMap, k: K, v: V, v2: V) { - // insert then do something, use if let - if let std::collections::btree_map::Entry::Vacant(e) = m.entry(k) { - e.insert(v); - foo(); - } + // or_insert_with. Partial move of a local declared in the closure is ok. + m.entry(k).or_insert_with(|| { + let x = (String::new(), String::new()); + let _ = x.0; + v + }); } fn main() {} diff --git a/tests/ui/entry.rs b/tests/ui/entry.rs index fa9280b58de1..d972a201ad76 100644 --- a/tests/ui/entry.rs +++ b/tests/ui/entry.rs @@ -4,7 +4,7 @@ #![warn(clippy::map_entry)] #![feature(asm)] -use std::collections::{BTreeMap, HashMap}; +use std::collections::HashMap; use std::hash::Hash; macro_rules! m { @@ -146,13 +146,12 @@ fn hash_map(m: &mut HashMap, m2: &mut HashMa if !m.contains_key(&k) { insert!(m, k, v); } -} -fn btree_map(m: &mut BTreeMap, k: K, v: V, v2: V) { - // insert then do something, use if let + // or_insert_with. Partial move of a local declared in the closure is ok. if !m.contains_key(&k) { + let x = (String::new(), String::new()); + let _ = x.0; m.insert(k, v); - foo(); } } diff --git a/tests/ui/entry.stderr b/tests/ui/entry.stderr index 8f2e383d675d..1076500498d3 100644 --- a/tests/ui/entry.stderr +++ b/tests/ui/entry.stderr @@ -165,21 +165,23 @@ LL | | m.insert(m!(k), m!(v)); LL | | } | |_____^ help: try this: `m.entry(m!(k)).or_insert_with(|| m!(v));` -error: usage of `contains_key` followed by `insert` on a `BTreeMap` - --> $DIR/entry.rs:153:5 +error: usage of `contains_key` followed by `insert` on a `HashMap` + --> $DIR/entry.rs:151:5 | LL | / if !m.contains_key(&k) { +LL | | let x = (String::new(), String::new()); +LL | | let _ = x.0; LL | | m.insert(k, v); -LL | | foo(); LL | | } | |_____^ | help: try this | -LL ~ if let std::collections::btree_map::Entry::Vacant(e) = m.entry(k) { -LL + e.insert(v); -LL + foo(); -LL + } +LL ~ m.entry(k).or_insert_with(|| { +LL + let x = (String::new(), String::new()); +LL + let _ = x.0; +LL + v +LL + }); | error: aborting due to 10 previous errors diff --git a/tests/ui/entry_btree.fixed b/tests/ui/entry_btree.fixed new file mode 100644 index 000000000000..94979104556b --- /dev/null +++ b/tests/ui/entry_btree.fixed @@ -0,0 +1,18 @@ +// run-rustfix + +#![warn(clippy::map_entry)] +#![allow(dead_code)] + +use std::collections::BTreeMap; + +fn foo() {} + +fn btree_map(m: &mut BTreeMap, k: K, v: V) { + // insert then do something, use if let + if let std::collections::btree_map::Entry::Vacant(e) = m.entry(k) { + e.insert(v); + foo(); + } +} + +fn main() {} diff --git a/tests/ui/entry_btree.rs b/tests/ui/entry_btree.rs new file mode 100644 index 000000000000..080c1d959e89 --- /dev/null +++ b/tests/ui/entry_btree.rs @@ -0,0 +1,18 @@ +// run-rustfix + +#![warn(clippy::map_entry)] +#![allow(dead_code)] + +use std::collections::BTreeMap; + +fn foo() {} + +fn btree_map(m: &mut BTreeMap, k: K, v: V) { + // insert then do something, use if let + if !m.contains_key(&k) { + m.insert(k, v); + foo(); + } +} + +fn main() {} diff --git a/tests/ui/entry_btree.stderr b/tests/ui/entry_btree.stderr new file mode 100644 index 000000000000..5c6fcdf1a28c --- /dev/null +++ b/tests/ui/entry_btree.stderr @@ -0,0 +1,20 @@ +error: usage of `contains_key` followed by `insert` on a `BTreeMap` + --> $DIR/entry_btree.rs:12:5 + | +LL | / if !m.contains_key(&k) { +LL | | m.insert(k, v); +LL | | foo(); +LL | | } + | |_____^ + | + = note: `-D clippy::map-entry` implied by `-D warnings` +help: try this + | +LL ~ if let std::collections::btree_map::Entry::Vacant(e) = m.entry(k) { +LL + e.insert(v); +LL + foo(); +LL + } + | + +error: aborting due to previous error + diff --git a/tests/ui/manual_map_option_2.fixed b/tests/ui/manual_map_option_2.fixed new file mode 100644 index 000000000000..147a0c49e6a0 --- /dev/null +++ b/tests/ui/manual_map_option_2.fixed @@ -0,0 +1,10 @@ +// run-rustfix + +#![warn(clippy::manual_map)] + +fn main() { + let _ = Some(0).map(|x| { + let y = (String::new(), String::new()); + (x, y.0) + }); +} diff --git a/tests/ui/manual_map_option_2.rs b/tests/ui/manual_map_option_2.rs new file mode 100644 index 000000000000..cc612dcc601d --- /dev/null +++ b/tests/ui/manual_map_option_2.rs @@ -0,0 +1,13 @@ +// run-rustfix + +#![warn(clippy::manual_map)] + +fn main() { + let _ = match Some(0) { + Some(x) => Some({ + let y = (String::new(), String::new()); + (x, y.0) + }), + None => None, + }; +} diff --git a/tests/ui/manual_map_option_2.stderr b/tests/ui/manual_map_option_2.stderr new file mode 100644 index 000000000000..745737079f37 --- /dev/null +++ b/tests/ui/manual_map_option_2.stderr @@ -0,0 +1,24 @@ +error: manual implementation of `Option::map` + --> $DIR/manual_map_option_2.rs:6:13 + | +LL | let _ = match Some(0) { + | _____________^ +LL | | Some(x) => Some({ +LL | | let y = (String::new(), String::new()); +LL | | (x, y.0) +LL | | }), +LL | | None => None, +LL | | }; + | |_____^ + | + = note: `-D clippy::manual-map` implied by `-D warnings` +help: try this + | +LL | let _ = Some(0).map(|x| { +LL | let y = (String::new(), String::new()); +LL | (x, y.0) +LL | }); + | + +error: aborting due to previous error + From 251dd30d77c98cbebd1c68840fce029affe9b6a8 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Wed, 4 Aug 2021 13:48:45 -0400 Subject: [PATCH 2/6] Improve `manual_map` In some cases check if a borrow made in the scrutinee expression would prevent creating the closure used by `map` --- clippy_lints/src/manual_map.rs | 36 +++++++-- clippy_utils/src/lib.rs | 117 +++++++++++++++++++++++++---- tests/ui/manual_map_option_2.fixed | 6 ++ tests/ui/manual_map_option_2.rs | 6 ++ 4 files changed, 145 insertions(+), 20 deletions(-) diff --git a/clippy_lints/src/manual_map.rs b/clippy_lints/src/manual_map.rs index 7dec1595e0d1..3ea88f52a1cc 100644 --- a/clippy_lints/src/manual_map.rs +++ b/clippy_lints/src/manual_map.rs @@ -4,12 +4,14 @@ use clippy_utils::source::{snippet_with_applicability, snippet_with_context}; use clippy_utils::ty::{is_type_diagnostic_item, peel_mid_ty_refs_is_mutable}; use clippy_utils::{ can_move_expr_to_closure, in_constant, is_else_clause, is_lang_ctor, is_lint_allowed, path_to_local_id, - peel_hir_expr_refs, + peel_hir_expr_refs, peel_hir_expr_while, CaptureKind, }; use rustc_ast::util::parser::PREC_POSTFIX; use rustc_errors::Applicability; use rustc_hir::LangItem::{OptionNone, OptionSome}; -use rustc_hir::{Arm, BindingAnnotation, Block, Expr, ExprKind, HirId, MatchSource, Mutability, Pat, PatKind}; +use rustc_hir::{ + def::Res, Arm, BindingAnnotation, Block, Expr, ExprKind, HirId, MatchSource, Mutability, Pat, PatKind, Path, QPath, +}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -111,10 +113,6 @@ impl LateLintPass<'_> for ManualMap { return; } - if !can_move_expr_to_closure(cx, some_expr) { - return; - } - // Determine which binding mode to use. let explicit_ref = some_pat.contains_explicit_ref_binding(); let binding_ref = explicit_ref.or_else(|| (ty_ref_count != pat_ref_count).then(|| ty_mutability)); @@ -125,6 +123,32 @@ impl LateLintPass<'_> for ManualMap { None => "", }; + match can_move_expr_to_closure(cx, some_expr) { + Some(captures) => { + // Check if captures the closure will need conflict with borrows made in the scrutinee. + // TODO: check all the references made in the scrutinee expression. This will require interacting + // with the borrow checker. Currently only `[.]*` is checked for. + if let Some(binding_ref_mutability) = binding_ref { + let e = peel_hir_expr_while(scrutinee, |e| match e.kind { + ExprKind::Field(e, _) | ExprKind::AddrOf(_, _, e) => Some(e), + _ => None, + }); + if let ExprKind::Path(QPath::Resolved(None, Path { res: Res::Local(l), .. })) = e.kind { + match captures.get(l) { + Some(CaptureKind::Value | CaptureKind::Ref(Mutability::Mut)) => return, + Some(CaptureKind::Ref(Mutability::Not)) + if binding_ref_mutability == Mutability::Mut => + { + return; + } + Some(CaptureKind::Ref(Mutability::Not)) | None => (), + } + } + } + }, + None => return, + }; + let mut app = Applicability::MachineApplicable; // Remove address-of expressions from the scrutinee. Either `as_ref` will be called, or diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index aee9f791b039..2655ba6a8e9d 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -67,19 +67,20 @@ use rustc_data_structures::unhash::UnhashMap; use rustc_hir as hir; use rustc_hir::def::{DefKind, Res}; use rustc_hir::def_id::DefId; -use rustc_hir::hir_id::HirIdSet; +use rustc_hir::hir_id::{HirIdMap, HirIdSet}; use rustc_hir::intravisit::{self, walk_expr, ErasedMap, FnKind, NestedVisitorMap, Visitor}; use rustc_hir::LangItem::{ResultErr, ResultOk}; use rustc_hir::{ def, Arm, BindingAnnotation, Block, Body, Constness, Destination, Expr, ExprKind, FnDecl, GenericArgs, HirId, Impl, - ImplItem, ImplItemKind, IsAsync, Item, ItemKind, LangItem, Local, MatchSource, Node, Param, Pat, PatKind, Path, - PathSegment, PrimTy, QPath, Stmt, StmtKind, TraitItem, TraitItemKind, TraitRef, TyKind, UnOp, + ImplItem, ImplItemKind, IsAsync, Item, ItemKind, LangItem, Local, MatchSource, Mutability, Node, Param, Pat, + PatKind, Path, PathSegment, PrimTy, QPath, Stmt, StmtKind, TraitItem, TraitItemKind, TraitRef, TyKind, UnOp, }; use rustc_lint::{LateContext, Level, Lint, LintContext}; use rustc_middle::hir::exports::Export; use rustc_middle::hir::map::Map; use rustc_middle::ty as rustc_ty; -use rustc_middle::ty::{layout::IntegerExt, DefIdTree, Ty, TyCtxt, TypeFoldable}; +use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow}; +use rustc_middle::ty::{layout::IntegerExt, DefIdTree, Ty, TyCtxt, TypeAndMut, TypeFoldable}; use rustc_semver::RustcVersion; use rustc_session::Session; use rustc_span::hygiene::{ExpnKind, MacroKind}; @@ -670,8 +671,82 @@ pub fn can_move_expr_to_closure_no_visit( } } -/// Checks if the expression can be moved into a closure as is. -pub fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool { +/// How a local is captured by a closure +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum CaptureKind { + Value, + Ref(Mutability), +} +impl std::ops::BitOr for CaptureKind { + type Output = Self; + fn bitor(self, rhs: Self) -> Self::Output { + match (self, rhs) { + (CaptureKind::Value, _) | (_, CaptureKind::Value) => CaptureKind::Value, + (CaptureKind::Ref(Mutability::Mut), CaptureKind::Ref(_)) + | (CaptureKind::Ref(_), CaptureKind::Ref(Mutability::Mut)) => CaptureKind::Ref(Mutability::Mut), + (CaptureKind::Ref(Mutability::Not), CaptureKind::Ref(Mutability::Not)) => CaptureKind::Ref(Mutability::Not), + } + } +} +impl std::ops::BitOrAssign for CaptureKind { + fn bitor_assign(&mut self, rhs: Self) { + *self = *self | rhs; + } +} + +/// Given an expression referencing a local, determines how it would be captured in a closure. +/// Note as this will walk up to parent expressions until the capture can be determined it should +/// only be used while making a closure somewhere a value is consumed. e.g. a block, match arm, or +/// function argument (other than a receiver). +pub fn capture_local_usage(cx: &LateContext<'tcx>, e: &Expr<'_>) -> CaptureKind { + debug_assert!(matches!( + e.kind, + ExprKind::Path(QPath::Resolved(None, Path { res: Res::Local(_), .. })) + )); + + let map = cx.tcx.hir(); + let mut child_id = e.hir_id; + let mut capture = CaptureKind::Value; + + for (parent_id, parent) in map.parent_iter(e.hir_id) { + if let [Adjustment { + kind: Adjust::Deref(_) | Adjust::Borrow(AutoBorrow::Ref(..)), + target, + }, ref adjust @ ..] = *cx + .typeck_results() + .adjustments() + .get(child_id) + .map_or(&[][..], |x| &**x) + { + if let rustc_ty::RawPtr(TypeAndMut { mutbl: mutability, .. }) | rustc_ty::Ref(_, _, mutability) = + *adjust.last().map_or(target, |a| a.target).kind() + { + return CaptureKind::Ref(mutability); + } + } + + if let Node::Expr(e) = parent { + match e.kind { + ExprKind::AddrOf(_, mutability, _) => return CaptureKind::Ref(mutability), + ExprKind::Index(..) | ExprKind::Unary(UnOp::Deref, _) => capture = CaptureKind::Ref(Mutability::Not), + ExprKind::Assign(lhs, ..) | ExprKind::Assign(_, lhs, _) if lhs.hir_id == child_id => { + return CaptureKind::Ref(Mutability::Mut); + }, + _ => break, + } + } else { + break; + } + + child_id = parent_id; + } + + capture +} + +/// Checks if the expression can be moved into a closure as is. This will return a list of captures +/// if so, otherwise, `None`. +pub fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option> { struct V<'cx, 'tcx> { cx: &'cx LateContext<'tcx>, // Stack of potential break targets contained in the expression. @@ -680,6 +755,9 @@ pub fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> locals: HirIdSet, /// Whether this expression can be turned into a closure. allow_closure: bool, + /// Locals which need to be captured, and whether they need to be by value, reference, or + /// mutable reference. + captures: HirIdMap, } impl Visitor<'tcx> for V<'_, 'tcx> { type Map = ErasedMap<'tcx>; @@ -691,13 +769,23 @@ pub fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> if !self.allow_closure { return; } - if let ExprKind::Loop(b, ..) = e.kind { - self.loops.push(e.hir_id); - self.visit_block(b); - self.loops.pop(); - } else { - self.allow_closure &= can_move_expr_to_closure_no_visit(self.cx, e, &self.loops, &self.locals); - walk_expr(self, e); + + match e.kind { + ExprKind::Path(QPath::Resolved(None, &Path { res: Res::Local(l), .. })) => { + if !self.locals.contains(&l) { + let cap = capture_local_usage(self.cx, e); + self.captures.entry(l).and_modify(|e| *e |= cap).or_insert(cap); + } + }, + ExprKind::Loop(b, ..) => { + self.loops.push(e.hir_id); + self.visit_block(b); + self.loops.pop(); + }, + _ => { + self.allow_closure &= can_move_expr_to_closure_no_visit(self.cx, e, &self.loops, &self.locals); + walk_expr(self, e); + }, } } @@ -713,9 +801,10 @@ pub fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> allow_closure: true, loops: Vec::new(), locals: HirIdSet::default(), + captures: HirIdMap::default(), }; v.visit_expr(expr); - v.allow_closure + v.allow_closure.then(|| v.captures) } /// Returns the method names and argument list of nested method call expressions that make up diff --git a/tests/ui/manual_map_option_2.fixed b/tests/ui/manual_map_option_2.fixed index 147a0c49e6a0..637f03279543 100644 --- a/tests/ui/manual_map_option_2.fixed +++ b/tests/ui/manual_map_option_2.fixed @@ -7,4 +7,10 @@ fn main() { let y = (String::new(), String::new()); (x, y.0) }); + + let s = Some(String::new()); + let _ = match &s { + Some(x) => Some((x.clone(), s)), + None => None, + }; } diff --git a/tests/ui/manual_map_option_2.rs b/tests/ui/manual_map_option_2.rs index cc612dcc601d..98e00604a1b3 100644 --- a/tests/ui/manual_map_option_2.rs +++ b/tests/ui/manual_map_option_2.rs @@ -10,4 +10,10 @@ fn main() { }), None => None, }; + + let s = Some(String::new()); + let _ = match &s { + Some(x) => Some((x.clone(), s)), + None => None, + }; } From 9500974bdb5f7cd9a8ba056b41bd5af5f373e0d3 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Mon, 9 Aug 2021 14:18:53 -0400 Subject: [PATCH 3/6] Fix tracking of which locals would need to be captured in a closure. * Captures by sub closures are now considered * Copy types are correctly borrowed by reference when their value is used * Fields are no longer automatically borrowed by value * Bindings in `match` and `let` patterns are now checked to determine how a local is captured --- clippy_utils/src/lib.rs | 95 +++++++++++++++++++++++++---- tests/ui/manual_map_option_2.fixed | 34 +++++++++++ tests/ui/manual_map_option_2.rs | 37 +++++++++++ tests/ui/manual_map_option_2.stderr | 23 ++++++- 4 files changed, 176 insertions(+), 13 deletions(-) diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 2655ba6a8e9d..eb9ad04527f4 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -62,7 +62,7 @@ use std::collections::hash_map::Entry; use std::hash::BuildHasherDefault; use if_chain::if_chain; -use rustc_ast::ast::{self, Attribute, BorrowKind, LitKind}; +use rustc_ast::ast::{self, Attribute, LitKind}; use rustc_data_structures::unhash::UnhashMap; use rustc_hir as hir; use rustc_hir::def::{DefKind, Res}; @@ -78,9 +78,11 @@ use rustc_hir::{ use rustc_lint::{LateContext, Level, Lint, LintContext}; use rustc_middle::hir::exports::Export; use rustc_middle::hir::map::Map; +use rustc_middle::hir::place::PlaceBase; use rustc_middle::ty as rustc_ty; use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow}; -use rustc_middle::ty::{layout::IntegerExt, DefIdTree, Ty, TyCtxt, TypeAndMut, TypeFoldable}; +use rustc_middle::ty::binding::BindingMode; +use rustc_middle::ty::{layout::IntegerExt, BorrowKind, DefIdTree, Ty, TyCtxt, TypeAndMut, TypeFoldable, UpvarCapture}; use rustc_semver::RustcVersion; use rustc_session::Session; use rustc_span::hygiene::{ExpnKind, MacroKind}; @@ -91,7 +93,7 @@ use rustc_span::{Span, DUMMY_SP}; use rustc_target::abi::Integer; use crate::consts::{constant, Constant}; -use crate::ty::{can_partially_move_ty, is_recursively_primitive_type}; +use crate::ty::{can_partially_move_ty, is_copy, is_recursively_primitive_type}; pub fn parse_msrv(msrv: &str, sess: Option<&Session>, span: Option) -> Option { if let Ok(version) = RustcVersion::parse(msrv) { @@ -628,6 +630,11 @@ pub fn can_mut_borrow_both(cx: &LateContext<'_>, e1: &Expr<'_>, e2: &Expr<'_>) - } /// Checks if the top level expression can be moved into a closure as is. +/// Currently checks for: +/// * Break/Continue outside the given jump targets +/// * Yield/Return statments. +/// * Inline assembly +/// * Usages of a field of a local where the type of the local can be partially moved. pub fn can_move_expr_to_closure_no_visit( cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, @@ -699,6 +706,22 @@ impl std::ops::BitOrAssign for CaptureKind { /// only be used while making a closure somewhere a value is consumed. e.g. a block, match arm, or /// function argument (other than a receiver). pub fn capture_local_usage(cx: &LateContext<'tcx>, e: &Expr<'_>) -> CaptureKind { + fn pat_capture_kind(cx: &LateContext<'_>, pat: &Pat<'_>) -> CaptureKind { + let mut capture = CaptureKind::Ref(Mutability::Not); + pat.each_binding(|_, id, span, _| { + match cx.typeck_results().extract_binding_mode(cx.sess(), id, span).unwrap() { + BindingMode::BindByValue(_) if !is_copy(cx, cx.typeck_results().node_type(id)) => { + capture = CaptureKind::Value; + }, + BindingMode::BindByReference(Mutability::Mut) if capture != CaptureKind::Value => { + capture = CaptureKind::Ref(Mutability::Mut); + }, + _ => (), + } + }); + capture + } + debug_assert!(matches!( e.kind, ExprKind::Path(QPath::Resolved(None, Path { res: Res::Local(_), .. })) @@ -707,6 +730,7 @@ pub fn capture_local_usage(cx: &LateContext<'tcx>, e: &Expr<'_>) -> CaptureKind let map = cx.tcx.hir(); let mut child_id = e.hir_id; let mut capture = CaptureKind::Value; + let mut capture_expr_ty = e; for (parent_id, parent) in map.parent_iter(e.hir_id) { if let [Adjustment { @@ -725,23 +749,47 @@ pub fn capture_local_usage(cx: &LateContext<'tcx>, e: &Expr<'_>) -> CaptureKind } } - if let Node::Expr(e) = parent { - match e.kind { + match parent { + Node::Expr(e) => match e.kind { ExprKind::AddrOf(_, mutability, _) => return CaptureKind::Ref(mutability), ExprKind::Index(..) | ExprKind::Unary(UnOp::Deref, _) => capture = CaptureKind::Ref(Mutability::Not), ExprKind::Assign(lhs, ..) | ExprKind::Assign(_, lhs, _) if lhs.hir_id == child_id => { return CaptureKind::Ref(Mutability::Mut); }, + ExprKind::Field(..) => { + if capture == CaptureKind::Value { + capture_expr_ty = e; + } + }, + ExprKind::Match(_, arms, _) => { + let mut mutability = Mutability::Not; + for capture in arms.iter().map(|arm| pat_capture_kind(cx, arm.pat)) { + match capture { + CaptureKind::Value => break, + CaptureKind::Ref(Mutability::Mut) => mutability = Mutability::Mut, + CaptureKind::Ref(Mutability::Not) => (), + } + } + return CaptureKind::Ref(mutability); + }, _ => break, - } - } else { - break; + }, + Node::Local(l) => match pat_capture_kind(cx, l.pat) { + CaptureKind::Value => break, + capture @ CaptureKind::Ref(_) => return capture, + }, + _ => break, } child_id = parent_id; } - capture + if capture == CaptureKind::Value && is_copy(cx, cx.typeck_results().expr_ty(capture_expr_ty)) { + // Copy types are never automatically captured by value. + CaptureKind::Ref(Mutability::Not) + } else { + capture + } } /// Checks if the expression can be moved into a closure as is. This will return a list of captures @@ -777,6 +825,31 @@ pub fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> self.captures.entry(l).and_modify(|e| *e |= cap).or_insert(cap); } }, + ExprKind::Closure(..) => { + let closure_id = self.cx.tcx.hir().local_def_id(e.hir_id).to_def_id(); + for capture in self.cx.typeck_results().closure_min_captures_flattened(closure_id) { + let local_id = match capture.place.base { + PlaceBase::Local(id) => id, + PlaceBase::Upvar(var) => var.var_path.hir_id, + _ => continue, + }; + if !self.locals.contains(&local_id) { + let capture = match capture.info.capture_kind { + UpvarCapture::ByValue(_) => CaptureKind::Value, + UpvarCapture::ByRef(borrow) => match borrow.kind { + BorrowKind::ImmBorrow => CaptureKind::Ref(Mutability::Not), + BorrowKind::UniqueImmBorrow | BorrowKind::MutBorrow => { + CaptureKind::Ref(Mutability::Mut) + }, + }, + }; + self.captures + .entry(local_id) + .and_modify(|e| *e |= capture) + .or_insert(capture); + } + } + }, ExprKind::Loop(b, ..) => { self.loops.push(e.hir_id); self.visit_block(b); @@ -1830,7 +1903,7 @@ pub fn peel_hir_expr_while<'tcx>( pub fn peel_n_hir_expr_refs(expr: &'a Expr<'a>, count: usize) -> (&'a Expr<'a>, usize) { let mut remaining = count; let e = peel_hir_expr_while(expr, |e| match e.kind { - ExprKind::AddrOf(BorrowKind::Ref, _, e) if remaining != 0 => { + ExprKind::AddrOf(ast::BorrowKind::Ref, _, e) if remaining != 0 => { remaining -= 1; Some(e) }, @@ -1844,7 +1917,7 @@ pub fn peel_n_hir_expr_refs(expr: &'a Expr<'a>, count: usize) -> (&'a Expr<'a>, pub fn peel_hir_expr_refs(expr: &'a Expr<'a>) -> (&'a Expr<'a>, usize) { let mut count = 0; let e = peel_hir_expr_while(expr, |e| match e.kind { - ExprKind::AddrOf(BorrowKind::Ref, _, e) => { + ExprKind::AddrOf(ast::BorrowKind::Ref, _, e) => { count += 1; Some(e) }, diff --git a/tests/ui/manual_map_option_2.fixed b/tests/ui/manual_map_option_2.fixed index 637f03279543..8cc12149403d 100644 --- a/tests/ui/manual_map_option_2.fixed +++ b/tests/ui/manual_map_option_2.fixed @@ -1,16 +1,50 @@ // run-rustfix #![warn(clippy::manual_map)] +#![allow(clippy::toplevel_ref_arg)] fn main() { + // Lint. `y` is declared within the arm, so it isn't captured by the map closure let _ = Some(0).map(|x| { let y = (String::new(), String::new()); (x, y.0) }); + // Don't lint. `s` is borrowed until partway through the arm, but needs to be captured by the map + // closure let s = Some(String::new()); let _ = match &s { Some(x) => Some((x.clone(), s)), None => None, }; + + // Don't lint. `s` is borrowed until partway through the arm, but needs to be captured by the map + // closure + let s = Some(String::new()); + let _ = match &s { + Some(x) => Some({ + let clone = x.clone(); + let s = || s; + (clone, s()) + }), + None => None, + }; + + // Don't lint. `s` is borrowed until partway through the arm, but needs to be captured as a mutable + // reference by the map closure + let mut s = Some(String::new()); + let _ = match &s { + Some(x) => Some({ + let clone = x.clone(); + let ref mut s = s; + (clone, s) + }), + None => None, + }; + + // Lint. `s` is captured by reference, so no lifetime issues. + let s = Some(String::new()); + let _ = s.as_ref().map(|x| { + if let Some(ref s) = s { (x.clone(), s) } else { panic!() } + }); } diff --git a/tests/ui/manual_map_option_2.rs b/tests/ui/manual_map_option_2.rs index 98e00604a1b3..0862b201ead4 100644 --- a/tests/ui/manual_map_option_2.rs +++ b/tests/ui/manual_map_option_2.rs @@ -1,8 +1,10 @@ // run-rustfix #![warn(clippy::manual_map)] +#![allow(clippy::toplevel_ref_arg)] fn main() { + // Lint. `y` is declared within the arm, so it isn't captured by the map closure let _ = match Some(0) { Some(x) => Some({ let y = (String::new(), String::new()); @@ -11,9 +13,44 @@ fn main() { None => None, }; + // Don't lint. `s` is borrowed until partway through the arm, but needs to be captured by the map + // closure let s = Some(String::new()); let _ = match &s { Some(x) => Some((x.clone(), s)), None => None, }; + + // Don't lint. `s` is borrowed until partway through the arm, but needs to be captured by the map + // closure + let s = Some(String::new()); + let _ = match &s { + Some(x) => Some({ + let clone = x.clone(); + let s = || s; + (clone, s()) + }), + None => None, + }; + + // Don't lint. `s` is borrowed until partway through the arm, but needs to be captured as a mutable + // reference by the map closure + let mut s = Some(String::new()); + let _ = match &s { + Some(x) => Some({ + let clone = x.clone(); + let ref mut s = s; + (clone, s) + }), + None => None, + }; + + // Lint. `s` is captured by reference, so no lifetime issues. + let s = Some(String::new()); + let _ = match &s { + Some(x) => Some({ + if let Some(ref s) = s { (x.clone(), s) } else { panic!() } + }), + None => None, + }; } diff --git a/tests/ui/manual_map_option_2.stderr b/tests/ui/manual_map_option_2.stderr index 745737079f37..9cfd83f445b1 100644 --- a/tests/ui/manual_map_option_2.stderr +++ b/tests/ui/manual_map_option_2.stderr @@ -1,5 +1,5 @@ error: manual implementation of `Option::map` - --> $DIR/manual_map_option_2.rs:6:13 + --> $DIR/manual_map_option_2.rs:8:13 | LL | let _ = match Some(0) { | _____________^ @@ -20,5 +20,24 @@ LL | (x, y.0) LL | }); | -error: aborting due to previous error +error: manual implementation of `Option::map` + --> $DIR/manual_map_option_2.rs:50:13 + | +LL | let _ = match &s { + | _____________^ +LL | | Some(x) => Some({ +LL | | if let Some(ref s) = s { (x.clone(), s) } else { panic!() } +LL | | }), +LL | | None => None, +LL | | }; + | |_____^ + | +help: try this + | +LL | let _ = s.as_ref().map(|x| { +LL | if let Some(ref s) = s { (x.clone(), s) } else { panic!() } +LL | }); + | + +error: aborting due to 2 previous errors From 5e4d8b44f9524f28fd1ebca18f48ecad204cf184 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Mon, 9 Aug 2021 16:26:27 -0400 Subject: [PATCH 4/6] Improve doc for `can_move_expr_to_closure_no_visit` --- clippy_utils/src/lib.rs | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index eb9ad04527f4..b40b42fa6d3b 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -631,20 +631,45 @@ pub fn can_mut_borrow_both(cx: &LateContext<'_>, e1: &Expr<'_>, e2: &Expr<'_>) - /// Checks if the top level expression can be moved into a closure as is. /// Currently checks for: -/// * Break/Continue outside the given jump targets +/// * Break/Continue outside the given loop HIR ids. /// * Yield/Return statments. -/// * Inline assembly +/// * Inline assembly. /// * Usages of a field of a local where the type of the local can be partially moved. +/// +/// For example, given the following function: +/// +/// ``` +/// fn f<'a>(iter: &mut impl Iterator) { +/// for item in iter { +/// let s = item.1; +/// if item.0 > 10 { +/// continue; +/// } else { +/// s.clear(); +/// } +/// } +/// } +/// ``` +/// +/// When called on the expression `item.0` this will return false unless the local `item` is in the +/// `ignore_locals` set. The type `(usize, &mut String)` can have the second element moved, so it +/// isn't always safe to move into a closure when only a single field is needed. +/// +/// When called on the `continue` expression this will return false unless the outer loop expression +/// is in the `loop_ids` set. +/// +/// Note that this check is not recursive, so passing the `if` expression will always return true +/// even though sub-expressions might return false. pub fn can_move_expr_to_closure_no_visit( cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, - jump_targets: &[HirId], + loop_ids: &[HirId], ignore_locals: &HirIdSet, ) -> bool { match expr.kind { ExprKind::Break(Destination { target_id: Ok(id), .. }, _) | ExprKind::Continue(Destination { target_id: Ok(id), .. }) - if jump_targets.contains(&id) => + if loop_ids.contains(&id) => { true }, From 10c0460a471d03a633cd15367bf0a85c007e0219 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sat, 14 Aug 2021 19:52:59 -0400 Subject: [PATCH 5/6] update stderr messages --- tests/ui/manual_map_option_2.stderr | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/ui/manual_map_option_2.stderr b/tests/ui/manual_map_option_2.stderr index 9cfd83f445b1..711ff6c4a4b0 100644 --- a/tests/ui/manual_map_option_2.stderr +++ b/tests/ui/manual_map_option_2.stderr @@ -14,10 +14,10 @@ LL | | }; = note: `-D clippy::manual-map` implied by `-D warnings` help: try this | -LL | let _ = Some(0).map(|x| { -LL | let y = (String::new(), String::new()); -LL | (x, y.0) -LL | }); +LL ~ let _ = Some(0).map(|x| { +LL + let y = (String::new(), String::new()); +LL + (x, y.0) +LL ~ }); | error: manual implementation of `Option::map` @@ -34,9 +34,9 @@ LL | | }; | help: try this | -LL | let _ = s.as_ref().map(|x| { -LL | if let Some(ref s) = s { (x.clone(), s) } else { panic!() } -LL | }); +LL ~ let _ = s.as_ref().map(|x| { +LL + if let Some(ref s) = s { (x.clone(), s) } else { panic!() } +LL ~ }); | error: aborting due to 2 previous errors From f0444d73def161f7f0fda7b3f5a13e9908e9c550 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sun, 15 Aug 2021 20:25:10 -0400 Subject: [PATCH 6/6] Use `each_binding_or_first` in `capture_local_usage` --- clippy_utils/src/lib.rs | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index b40b42fa6d3b..bd229402f418 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -733,16 +733,18 @@ impl std::ops::BitOrAssign for CaptureKind { pub fn capture_local_usage(cx: &LateContext<'tcx>, e: &Expr<'_>) -> CaptureKind { fn pat_capture_kind(cx: &LateContext<'_>, pat: &Pat<'_>) -> CaptureKind { let mut capture = CaptureKind::Ref(Mutability::Not); - pat.each_binding(|_, id, span, _| { - match cx.typeck_results().extract_binding_mode(cx.sess(), id, span).unwrap() { - BindingMode::BindByValue(_) if !is_copy(cx, cx.typeck_results().node_type(id)) => { - capture = CaptureKind::Value; - }, - BindingMode::BindByReference(Mutability::Mut) if capture != CaptureKind::Value => { - capture = CaptureKind::Ref(Mutability::Mut); - }, - _ => (), - } + pat.each_binding_or_first(&mut |_, id, span, _| match cx + .typeck_results() + .extract_binding_mode(cx.sess(), id, span) + .unwrap() + { + BindingMode::BindByValue(_) if !is_copy(cx, cx.typeck_results().node_type(id)) => { + capture = CaptureKind::Value; + }, + BindingMode::BindByReference(Mutability::Mut) if capture != CaptureKind::Value => { + capture = CaptureKind::Ref(Mutability::Mut); + }, + _ => (), }); capture }