From 9500974bdb5f7cd9a8ba056b41bd5af5f373e0d3 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Mon, 9 Aug 2021 14:18:53 -0400 Subject: [PATCH] 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 2655ba6a8e9..eb9ad04527f 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -62,7 +62,7 @@ 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_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_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 @@ fn bitor_assign(&mut self, rhs: Self) { /// 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 @@ fn visit_expr(&mut self, e: &'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 637f0327954..8cc12149403 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 98e00604a1b..0862b201ead 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 745737079f3..9cfd83f445b 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