Initial work for doing minimum capture analysis for RFC-2229

Co-authored-by: Chris Pardy <chrispardy36@gmail.com>
Co-authored-by: Logan Mosier <logmosier@gmail.com>

5 files changed, 374 insertions, 115 deletions

diff --git a/compiler/rustc_middle/src/ty/context.rs b/compiler/rustc_middle/src/ty/context.rs
index e9bc4b9b90d..76ca0c51ce1 100644
--- a/compiler/rustc_middle/src/ty/context.rs
+++ b/compiler/rustc_middle/src/ty/context.rs
@@ -415,9 +415,10 @@ pub struct TypeckResults<'tcx> {
     /// entire variable.
     pub closure_captures: ty::UpvarListMap,
 
-    /// Given the closure ID this map provides the list of
-    /// `Place`s and how/why are they captured by the closure.
-    pub closure_capture_information: ty::CaptureInformationMap<'tcx>,
+    /// Given the closure DefId this map provides a map of
+    /// root variables to minimum set of `Place`s (and how) that need to be tracked
+    /// to support all captures of that closure.
+    pub closure_min_captures: ty::MinCaptureInformationMap<'tcx>,
 
     /// Stores the type, expression, span and optional scope span of all types
     /// that are live across the yield of this generator (if a generator).
@@ -446,7 +447,7 @@ impl<'tcx> TypeckResults<'tcx> {
             tainted_by_errors: None,
             concrete_opaque_types: Default::default(),
             closure_captures: Default::default(),
-            closure_capture_information: Default::default(),
+            closure_min_captures: Default::default(),
             generator_interior_types: Default::default(),
         }
     }
@@ -681,7 +682,7 @@ impl<'a, 'tcx> HashStable<StableHashingContext<'a>> for TypeckResults<'tcx> {
             tainted_by_errors,
             ref concrete_opaque_types,
             ref closure_captures,
-            ref closure_capture_information,
+            ref closure_min_captures,
             ref generator_interior_types,
         } = *self;
 
@@ -715,7 +716,7 @@ impl<'a, 'tcx> HashStable<StableHashingContext<'a>> for TypeckResults<'tcx> {
             tainted_by_errors.hash_stable(hcx, hasher);
             concrete_opaque_types.hash_stable(hcx, hasher);
             closure_captures.hash_stable(hcx, hasher);
-            closure_capture_information.hash_stable(hcx, hasher);
+            closure_min_captures.hash_stable(hcx, hasher);
             generator_interior_types.hash_stable(hcx, hasher);
         })
     }
diff --git a/compiler/rustc_middle/src/ty/mod.rs b/compiler/rustc_middle/src/ty/mod.rs
index 52b49184cf1..c41463d1844 100644
--- a/compiler/rustc_middle/src/ty/mod.rs
+++ b/compiler/rustc_middle/src/ty/mod.rs
@@ -788,30 +788,18 @@ pub struct CaptureInfo<'tcx> {
     pub capture_kind: UpvarCapture<'tcx>,
 }
 
+#[derive(PartialEq, Clone, Debug, TyEncodable, TyDecodable, HashStable)]
+pub struct CapturedPlace<'tcx> {
+    pub place: HirPlace<'tcx>,
+    pub info: CaptureInfo<'tcx>,
+}
+
 pub type UpvarListMap = FxHashMap<DefId, FxIndexMap<hir::HirId, UpvarId>>;
 pub type UpvarCaptureMap<'tcx> = FxHashMap<UpvarId, UpvarCapture<'tcx>>;
 
-/// Consider closure where s.str1 is captured via an ImmutableBorrow and s.str2 via a MutableBorrow
-///
-/// ```rust
-/// // Assume that thte HirId for the variable definition is `V1`
-/// let mut s = SomeStruct { str1: format!("s1"), str2: format!("s2") }
-///
-/// let fix_s = |new_s2| {
-///     // Assume that the HirId for the expression `s.str1` is `E1`
-///     println!("Updating SomeStruct with str1=", s.str1);
-///     // Assume that the HirId for the expression `*s.str2` is `E2`
-///     s.str2 = new_s2;
-/// }
-/// ```
-///
-/// For closure `fix_s`, (at a high level) the IndexMap will contain:
-///
-/// Place { V1, [ProjectionKind::Field(Index=0, Variant=0)] } : CaptureKind { E1, ImmutableBorrow }
-/// Place { V1, [ProjectionKind::Field(Index=1, Variant=0)] } : CaptureKind { E2, MutableBorrow }
-///
-pub type CaptureInformationMap<'tcx> =
-    FxHashMap<DefId, FxIndexMap<HirPlace<'tcx>, CaptureInfo<'tcx>>>;
+pub type MinCaptureList<'tcx> = Vec<CapturedPlace<'tcx>>;
+pub type RootVariableMinCaptureList<'tcx> = FxIndexMap<hir::HirId, MinCaptureList<'tcx>>;
+pub type MinCaptureInformationMap<'tcx> = FxHashMap<DefId, RootVariableMinCaptureList<'tcx>>;
 
 #[derive(Clone, Copy, PartialEq, Eq)]
 pub enum IntVarValue {
diff --git a/compiler/rustc_mir/src/borrow_check/diagnostics/mod.rs b/compiler/rustc_mir/src/borrow_check/diagnostics/mod.rs
index 4256f6e39d5..41f3edaa413 100644
--- a/compiler/rustc_mir/src/borrow_check/diagnostics/mod.rs
+++ b/compiler/rustc_mir/src/borrow_check/diagnostics/mod.rs
@@ -383,16 +383,14 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                     self.describe_field_from_ty(&ty, field, variant_index)
                 }
                 ty::Closure(def_id, _) | ty::Generator(def_id, _, _) => {
-                    // `tcx.upvars_mentioned(def_id)` returns an `Option`, which is `None` in case
-                    // the closure comes from another crate. But in that case we wouldn't
-                    // be borrowck'ing it, so we can just unwrap:
-                    let (&var_id, _) = self
-                        .infcx
-                        .tcx
-                        .upvars_mentioned(def_id)
-                        .unwrap()
-                        .get_index(field.index())
-                        .unwrap();
+                    // We won't be borrowck'ing here if the closure came from another crate,
+                    // so it's safe to call `expect_local`.
+                    //
+                    // We know the field exists so it's safe to call operator[] and `unwrap` here.
+                    let (&var_id, _) =
+                        self.infcx.tcx.typeck(def_id.expect_local()).closure_captures[&def_id]
+                            .get_index(field.index())
+                            .unwrap();
 
                     self.infcx.tcx.hir().name(var_id).to_string()
                 }
@@ -967,9 +965,12 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         let expr = &self.infcx.tcx.hir().expect_expr(hir_id).kind;
         debug!("closure_span: hir_id={:?} expr={:?}", hir_id, expr);
         if let hir::ExprKind::Closure(.., body_id, args_span, _) = expr {
-            for ((upvar_hir_id, upvar), place) in
-                self.infcx.tcx.upvars_mentioned(def_id)?.iter().zip(places)
+            for (upvar_hir_id, place) in
+                self.infcx.tcx.typeck(def_id.expect_local()).closure_captures[&def_id]
+                    .keys()
+                    .zip(places)
             {
+                let span = self.infcx.tcx.upvars_mentioned(local_did)?[upvar_hir_id].span;
                 match place {
                     Operand::Copy(place) | Operand::Move(place)
                         if target_place == place.as_ref() =>
@@ -991,7 +992,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                         let usage_span =
                             match self.infcx.tcx.typeck(local_did).upvar_capture(upvar_id) {
                                 ty::UpvarCapture::ByValue(Some(span)) => span,
-                                _ => upvar.span,
+                                _ => span,
                             };
                         return Some((*args_span, generator_kind, usage_span));
                     }
diff --git a/compiler/rustc_typeck/src/check/upvar.rs b/compiler/rustc_typeck/src/check/upvar.rs
index 63657971485..4b6d10357ef 100644
--- a/compiler/rustc_typeck/src/check/upvar.rs
+++ b/compiler/rustc_typeck/src/check/upvar.rs
@@ -39,11 +39,13 @@ use rustc_hir::def_id::DefId;
 use rustc_hir::def_id::LocalDefId;
 use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
 use rustc_infer::infer::UpvarRegion;
-use rustc_middle::hir::place::{Place, PlaceBase, PlaceWithHirId};
+use rustc_middle::hir::place::{Place, PlaceBase, PlaceWithHirId, ProjectionKind};
 use rustc_middle::ty::{self, Ty, TyCtxt, UpvarSubsts};
 use rustc_span::sym;
 use rustc_span::{Span, Symbol};
 
+use std::env;
+
 macro_rules! log_capture_analysis {
     ($fcx:expr, $closure_def_id:expr, $fmt:literal) => {
         if $fcx.should_log_capture_analysis($closure_def_id) {
@@ -60,6 +62,17 @@ macro_rules! log_capture_analysis {
     };
 }
 
+/// Describe the relationship between the paths of two places
+/// eg:
+/// - foo is ancestor of foo.bar.baz
+/// - foo.bar.baz is an descendant of foo.bar,
+/// - foo.bar and foo.baz are divergent
+enum PlaceAncestryRelation {
+    Ancestor,
+    Descendant,
+    Divergent,
+}
+
 impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     pub fn closure_analyze(&self, body: &'tcx hir::Body<'tcx>) {
         InferBorrowKindVisitor { fcx: self }.visit_body(body);
@@ -130,7 +143,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         let local_def_id = closure_def_id.expect_local();
 
         let mut capture_information = FxIndexMap::<Place<'tcx>, ty::CaptureInfo<'tcx>>::default();
-        if self.tcx.features().capture_disjoint_fields {
+        if self.tcx.features().capture_disjoint_fields || matches!(env::var("SG_NEW"), Ok(_)) {
             log_capture_analysis!(self, closure_def_id, "Using new-style capture analysis");
         } else {
             log_capture_analysis!(self, closure_def_id, "Using old-style capture analysis");
@@ -192,12 +205,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             }
         }
 
-        self.set_closure_captures(closure_def_id, &delegate);
-
-        self.typeck_results
-            .borrow_mut()
-            .closure_capture_information
-            .insert(closure_def_id, delegate.capture_information);
+        self.compute_min_captures(closure_def_id, delegate);
+        self.set_closure_captures(closure_def_id);
 
         // Now that we've analyzed the closure, we know how each
         // variable is borrowed, and we know what traits the closure
@@ -266,43 +275,221 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             .collect()
     }
 
-    fn set_closure_captures(
+    /// Bridge for closure analysis
+    /// ----------------------------
+    ///
+    /// For closure with DefId `c`, the bridge converts structures required for supporting RFC 2229,
+    /// to structures currently used in the compiler for handling closure captures.
+    ///
+    /// For example the following structure will be converted:
+    ///
+    /// closure_min_captures
+    /// foo -> [ {foo.x, ImmBorrow}, {foo.y, MutBorrow} ]
+    /// bar -> [ {bar.z, ByValue}, {bar.q, MutBorrow} ]
+    ///
+    /// to
+    ///
+    /// 1. closure_captures
+    /// foo -> UpvarId(foo, c), bar -> UpvarId(bar, c)
+    ///
+    /// 2. upvar_capture_map
+    /// UpvarId(foo,c) -> MutBorrow, UpvarId(bar, c) -> ByValue
+
+    fn set_closure_captures(&self, closure_def_id: DefId) {
+        let mut closure_captures: FxIndexMap<hir::HirId, ty::UpvarId> = Default::default();
+        let mut upvar_capture_map = ty::UpvarCaptureMap::default();
+
+        if let Some(min_captures) =
+            self.typeck_results.borrow().closure_min_captures.get(&closure_def_id)
+        {
+            for (var_hir_id, min_list) in min_captures.iter() {
+                for captured_place in min_list {
+                    let place = &captured_place.place;
+                    let capture_info = captured_place.info;
+
+                    let upvar_id = match place.base {
+                        PlaceBase::Upvar(upvar_id) => upvar_id,
+                        base => bug!("Expected upvar, found={:?}", base),
+                    };
+
+                    assert_eq!(upvar_id.var_path.hir_id, *var_hir_id);
+                    assert_eq!(upvar_id.closure_expr_id, closure_def_id.expect_local());
+
+                    closure_captures.insert(*var_hir_id, upvar_id);
+
+                    let new_capture_kind = if let Some(capture_kind) =
+                        upvar_capture_map.get(&upvar_id)
+                    {
+                        // upvar_capture_map only stores the UpvarCapture (CaptureKind),
+                        // so we create a fake capture info with no expression.
+                        let fake_capture_info =
+                            ty::CaptureInfo { expr_id: None, capture_kind: capture_kind.clone() };
+                        self.determine_capture_info(fake_capture_info, capture_info.clone())
+                            .capture_kind
+                    } else {
+                        capture_info.capture_kind
+                    };
+                    upvar_capture_map.insert(upvar_id, new_capture_kind);
+                }
+            }
+        }
+        debug!(
+            "For closure_def_id={:?}, set_closure_captures={:#?}",
+            closure_def_id, closure_captures
+        );
+        debug!(
+            "For closure_def_id={:?}, upvar_capture_map={:#?}",
+            closure_def_id, upvar_capture_map
+        );
+
+        if !closure_captures.is_empty() {
+            self.typeck_results
+                .borrow_mut()
+                .closure_captures
+                .insert(closure_def_id, closure_captures);
+
+            self.typeck_results.borrow_mut().upvar_capture_map.extend(upvar_capture_map);
+        }
+    }
+
+    /// Analyses the information collected by InferBorrowKind to compute the min number of
+    /// Places (and corresponding capture kind) that we need to keep track of to support all
+    /// the required captured paths.
+    ///
+    /// Eg:
+    /// ```rust
+    /// struct Point { x: i32, y: i32 }
+    ///
+    /// let s: String;  // hir_id_s
+    /// let mut p: Point; // his_id_p
+    /// let c = || {
+    ///        println!("{}", s);  // L1
+    ///        p.x += 10;  // L2
+    ///        println!("{}" , p.y) // L3
+    ///        println!("{}", p) // L4
+    ///        drop(s);   // L5
+    /// };
+    /// ```
+    /// and let hir_id_L1..5 be the expressions pointing to use of a captured variable on
+    /// the lines L1..5 respectively.
+    ///
+    /// InferBorrowKind results in a structure like this:
+    ///
+    /// ```
+    /// {
+    ///       Place(base: hir_id_s, projections: [], ....) -> (hir_id_L5, ByValue),
+    ///       Place(base: hir_id_p, projections: [Field(0, 0)], ...) -> (hir_id_L2, ByRef(MutBorrow))
+    ///       Place(base: hir_id_p, projections: [Field(1, 0)], ...) -> (hir_id_L3, ByRef(ImmutBorrow))
+    ///       Place(base: hir_id_p, projections: [], ...) -> (hir_id_L4, ByRef(ImmutBorrow))
+    /// ```
+    ///
+    /// After the min capture analysis, we get:
+    /// ```
+    /// {
+    ///       hir_id_s -> [
+    ///            Place(base: hir_id_s, projections: [], ....) -> (hir_id_L4, ByValue)
+    ///       ],
+    ///       hir_id_p -> [
+    ///            Place(base: hir_id_p, projections: [], ...) -> (hir_id_L2, ByRef(MutBorrow)),
+    ///       ],
+    /// ```
+    fn compute_min_captures(
         &self,
         closure_def_id: DefId,
-        inferred_info: &InferBorrowKind<'_, 'tcx>,
+        inferred_info: InferBorrowKind<'_, 'tcx>,
     ) {
-        let mut closure_captures: FxIndexMap<hir::HirId, ty::UpvarId> = Default::default();
+        let mut root_var_min_capture_list: ty::RootVariableMinCaptureList<'_> = Default::default();
 
-        for (place, capture_info) in inferred_info.capture_information.iter() {
-            let upvar_id = match place.base {
-                PlaceBase::Upvar(upvar_id) => upvar_id,
+        for (place, capture_info) in inferred_info.capture_information.into_iter() {
+            let var_hir_id = match place.base {
+                PlaceBase::Upvar(upvar_id) => upvar_id.var_path.hir_id,
                 base => bug!("Expected upvar, found={:?}", base),
             };
 
-            assert_eq!(upvar_id.closure_expr_id, closure_def_id.expect_local());
-
-            let var_hir_id = upvar_id.var_path.hir_id;
-            closure_captures.insert(var_hir_id, upvar_id);
-
-            let new_capture_kind = if let Some(capture_kind) =
-                self.typeck_results.borrow_mut().upvar_capture_map.get(&upvar_id)
-            {
-                // upvar_capture_map only stores the UpvarCapture (CaptureKind),
-                // so we create a fake capture info with no expression.
-                let fake_capture_info =
-                    ty::CaptureInfo { expr_id: None, capture_kind: capture_kind.clone() };
-                self.determine_capture_info(fake_capture_info, capture_info.clone()).capture_kind
-            } else {
-                capture_info.capture_kind
+            // Arrays are captured in entirety, drop Index projections and projections
+            // after Index projections.
+            let first_index_projection =
+                place.projections.split(|proj| ProjectionKind::Index == proj.kind).next();
+            let place = Place {
+                base_ty: place.base_ty,
+                base: place.base,
+                projections: first_index_projection.map_or(Vec::new(), |p| p.to_vec()),
+            };
+
+            let min_cap_list = match root_var_min_capture_list.get_mut(&var_hir_id) {
+                None => {
+                    let min_cap_list = vec![ty::CapturedPlace { place: place, info: capture_info }];
+                    root_var_min_capture_list.insert(var_hir_id, min_cap_list);
+                    continue;
+                }
+                Some(min_cap_list) => min_cap_list,
             };
-            self.typeck_results.borrow_mut().upvar_capture_map.insert(upvar_id, new_capture_kind);
+
+            // Go through each entry in the current list of min_captures
+            // - if ancestor is found, update it's capture kind to account for current place's
+            // capture information.
+            //
+            // - if descendant is found, remove it from the list, and update the current place's
+            // capture information to account for the descendants's capture kind.
+            //
+            // We can never be in a case where the list contains both an ancestor and a descendant
+            // Also there can only be ancestor but in case of descendants there might be
+            // multiple.
+
+            let mut descendant_found = false;
+            let mut updated_capture_info = capture_info;
+            min_cap_list.retain(|possible_descendant| {
+                match determine_place_ancestry_relation(&place, &possible_descendant.place) {
+                    // current place is ancestor of possible_descendant
+                    PlaceAncestryRelation::Ancestor => {
+                        descendant_found = true;
+                        updated_capture_info = self
+                            .determine_capture_info(updated_capture_info, possible_descendant.info);
+                        false
+                    }
+
+                    _ => true,
+                }
+            });
+
+            let mut ancestor_found = false;
+            if !descendant_found {
+                for possible_ancestor in min_cap_list.iter_mut() {
+                    match determine_place_ancestry_relation(&place, &possible_ancestor.place) {
+                        // current place is descendant of possible_ancestor
+                        PlaceAncestryRelation::Descendant => {
+                            ancestor_found = true;
+                            possible_ancestor.info =
+                                self.determine_capture_info(possible_ancestor.info, capture_info);
+
+                            // Only one ancestor of the current place will be in the list.
+                            break;
+                        }
+                        _ => {}
+                    }
+                }
+            }
+
+            // Only need to insert when we don't have an ancestor in the existing min capture list
+            if !ancestor_found {
+                let captured_place =
+                    ty::CapturedPlace { place: place.clone(), info: updated_capture_info };
+                min_cap_list.push(captured_place);
+            }
         }
 
-        if !closure_captures.is_empty() {
+        log_capture_analysis!(
+            self,
+            closure_def_id,
+            "min_captures={:#?}",
+            root_var_min_capture_list
+        );
+
+        if !root_var_min_capture_list.is_empty() {
             self.typeck_results
                 .borrow_mut()
-                .closure_captures
-                .insert(closure_def_id, closure_captures);
+                .closure_min_captures
+                .insert(closure_def_id, root_var_min_capture_list);
         }
     }
 
@@ -418,8 +605,27 @@ struct InferBorrowKind<'a, 'tcx> {
     // variable access that caused us to do so.
     current_origin: Option<(Span, Symbol)>,
 
-    // For each upvar that we access, we track the minimal kind of
-    // access we need (ref, ref mut, move, etc).
+    /// For each Place that we access, we track the minimal kind of
+    /// access we need (ref, ref mut, move, etc) and the expression that resulted in such access.
+    /// Consider closure where s.str1 is captured via an ImmutableBorrow and
+    /// s.str2 via a MutableBorrow
+    ///
+    /// ```rust
+    /// // Assume that the HirId for the variable definition is `V1`
+    /// let mut s = SomeStruct { str1: format!("s1"), str2: format!("s2") }
+    ///
+    /// let fix_s = |new_s2| {
+    ///     // Assume that the HirId for the expression `s.str1` is `E1`
+    ///     println!("Updating SomeStruct with str1=", s.str1);
+    ///     // Assume that the HirId for the expression `*s.str2` is `E2`
+    ///     s.str2 = new_s2;
+    /// }
+    /// ```
+    ///
+    /// For closure `fix_s`, (at a high level) the map contains
+    ///
+    /// Place { V1, [ProjectionKind::Field(Index=0, Variant=0)] } : CaptureKind { E1, ImmutableBorrow }
+    /// Place { V1, [ProjectionKind::Field(Index=1, Variant=0)] } : CaptureKind { E2, MutableBorrow }
     capture_information: FxIndexMap<Place<'tcx>, ty::CaptureInfo<'tcx>>,
 }
 
@@ -714,3 +920,45 @@ impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> {
 fn var_name(tcx: TyCtxt<'_>, var_hir_id: hir::HirId) -> Symbol {
     tcx.hir().name(var_hir_id)
 }
+
+/// Determines the Ancestry relationship of Place A relative to Place B
+///
+/// `PlaceAncestryRelation::Ancestor` implies Place A is ancestor of Place B
+/// `PlaceAncestryRelation::Descendant` implies Place A is descendant of Place B
+/// `PlaceAncestryRelation::Divergent` implies neither of them is the ancestor of the other.
+fn determine_place_ancestry_relation(
+    place_a: &Place<'tcx>,
+    place_b: &Place<'tcx>,
+) -> PlaceAncestryRelation {
+    // If Place A and Place B, don't start off from the same root variable, they are divergent.
+    if place_a.base != place_b.base {
+        return PlaceAncestryRelation::Divergent;
+    }
+
+    // Assume of length of projections_a = n
+    let projections_a = &place_a.projections;
+
+    // Assume of length of projections_b = m
+    let projections_b = &place_b.projections;
+
+    let mut same_initial_projections = true;
+
+    for (proj_a, proj_b) in projections_a.iter().zip(projections_b.iter()) {
+        if proj_a != proj_b {
+            same_initial_projections = false;
+            break;
+        }
+    }
+
+    if same_initial_projections {
+        // First min(n, m) projections are the same
+        // Select Ancestor/Descendant
+        if projections_b.len() >= projections_a.len() {
+            PlaceAncestryRelation::Ancestor
+        } else {
+            PlaceAncestryRelation::Descendant
+        }
+    } else {
+        PlaceAncestryRelation::Divergent
+    }
+}
diff --git a/compiler/rustc_typeck/src/expr_use_visitor.rs b/compiler/rustc_typeck/src/expr_use_visitor.rs
index a8cac3e0fc8..83cc1da6985 100644
--- a/compiler/rustc_typeck/src/expr_use_visitor.rs
+++ b/compiler/rustc_typeck/src/expr_use_visitor.rs
@@ -18,7 +18,6 @@ use rustc_middle::ty::{self, adjustment, TyCtxt};
 use rustc_target::abi::VariantIdx;
 
 use crate::mem_categorization as mc;
-use rustc_span::Span;
 
 ///////////////////////////////////////////////////////////////////////////
 // The Delegate trait
@@ -331,8 +330,8 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
                 self.consume_expr(base);
             }
 
-            hir::ExprKind::Closure(_, _, _, fn_decl_span, _) => {
-                self.walk_captures(expr, fn_decl_span);
+            hir::ExprKind::Closure(..) => {
+                self.walk_captures(expr);
             }
 
             hir::ExprKind::Box(ref base) => {
@@ -571,51 +570,73 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
         }));
     }
 
-    // FIXME(arora-aman):  fix the fn_decl_span
-    fn walk_captures(&mut self, closure_expr: &hir::Expr<'_>, _fn_decl_span: Span) {
+    /// Handle the case where the current body contains a closure.
+    ///
+    /// When the current body being handled is a closure, then we must make sure that
+    /// - The parent closure only captures Places from the nested closure that are not local to it.
+    ///
+    /// In the following example the closures `c` only captures `p.x`` even though `incr`
+    /// is a capture of the nested closure
+    ///
+    /// ```rust
+    /// let p = ..;
+    /// let c = || {
+    ///    let incr = 10;
+    ///    let nested = || p.x += incr;
+    /// }
+    /// ```
+    ///
+    /// - When reporting the Place back to the Delegate, ensure that the UpvarId uses the enclosing
+    /// closure as the DefId.
+    fn walk_captures(&mut self, closure_expr: &hir::Expr<'_>) {
         debug!("walk_captures({:?})", closure_expr);
 
-        // We are currently walking a closure that is within a given body
-        // We need to process all the captures for this closure.
         let closure_def_id = self.tcx().hir().local_def_id(closure_expr.hir_id).to_def_id();
         let upvars = self.tcx().upvars_mentioned(self.body_owner);
-        if let Some(closure_capture_information) =
-            self.mc.typeck_results.closure_capture_information.get(&closure_def_id)
-        {
-            for (place, capture_info) in closure_capture_information.iter() {
-                let var_hir_id = if let PlaceBase::Upvar(upvar_id) = place.base {
-                    upvar_id.var_path.hir_id
-                } else {
-                    continue;
-                    // FIXME(arora-aman): throw err?
-                };
 
-                if !upvars.map_or(false, |upvars| upvars.contains_key(&var_hir_id)) {
-                    // The nested closure might be capturing our local variables
-                    // Since for the current body these aren't captures, we will ignore them.
+        // For purposes of this function, generator and closures are equivalent.
+        let body_owner_is_closure = match self.tcx().type_of(self.body_owner.to_def_id()).kind() {
+            ty::Closure(..) | ty::Generator(..) => true,
+            _ => false,
+        };
+
+        if let Some(min_captures) = self.mc.typeck_results.closure_min_captures.get(&closure_def_id)
+        {
+            for (var_hir_id, min_list) in min_captures.iter() {
+                if upvars.map_or(body_owner_is_closure, |upvars| !upvars.contains_key(var_hir_id)) {
+                    // The nested closure might be capturing the current (enclosing) closure's local variables.
+                    // We check if the root variable is ever mentioned within the enclosing closure, if not
+                    // then for the current body (if it's a closure) these aren't captures, we will ignore them.
                     continue;
                 }
+                for captured_place in min_list {
+                    let place = &captured_place.place;
+                    let capture_info = captured_place.info;
 
-                // The place is being captured by the enclosing closure
-                // FIXME(arora-aman) Make sure this is valid to do when called from clippy.
-                let upvar_id = ty::UpvarId::new(var_hir_id, self.body_owner);
-                let place_with_id = PlaceWithHirId::new(
-                    capture_info.expr_id.unwrap_or(closure_expr.hir_id),
-                    place.base_ty,
-                    PlaceBase::Upvar(upvar_id),
-                    place.projections.clone(),
-                );
-                match capture_info.capture_kind {
-                    ty::UpvarCapture::ByValue(_) => {
-                        let mode = copy_or_move(&self.mc, &place_with_id);
-                        self.delegate.consume(&place_with_id, place_with_id.hir_id, mode);
-                    }
-                    ty::UpvarCapture::ByRef(upvar_borrow) => {
-                        self.delegate.borrow(
-                            &place_with_id,
-                            place_with_id.hir_id,
-                            upvar_borrow.kind,
-                        );
+                    let upvar_id = if body_owner_is_closure {
+                        // Mark the place to be captured by the enclosing closure
+                        ty::UpvarId::new(*var_hir_id, self.body_owner)
+                    } else {
+                        ty::UpvarId::new(*var_hir_id, closure_def_id.expect_local())
+                    };
+                    let place_with_id = PlaceWithHirId::new(
+                        capture_info.expr_id.unwrap_or(closure_expr.hir_id),
+                        place.base_ty,
+                        PlaceBase::Upvar(upvar_id),
+                        place.projections.clone(),
+                    );
+                    match capture_info.capture_kind {
+                        ty::UpvarCapture::ByValue(_) => {
+                            let mode = copy_or_move(&self.mc, &place_with_id);
+                            self.delegate.consume(&place_with_id, place_with_id.hir_id, mode);
+                        }
+                        ty::UpvarCapture::ByRef(upvar_borrow) => {
+                            self.delegate.borrow(
+                                &place_with_id,
+                                place_with_id.hir_id,
+                                upvar_borrow.kind,
+                            );
+                        }
                     }
                 }
             }