Avoid writing a temporary closure kind

nikomatsakis · alexcrichton · commit 6298f09d9223 · 2016-08-08T13:53:55.000-07:00
We used to write a temporary closure kind into the inference table, but this could lead to obligations being incorrectled resolved before inference had completed. This result could then be cached, leading to further trouble. This patch avoids writing any closure kind until the computation is complete. Fixes #34349.
diff --git a/src/librustc/infer/mod.rs b/src/librustc/infer/mod.rs
@@ -175,6 +175,12 @@ pub struct InferCtxt<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {
     // any obligations set during the current snapshot. In that case, the
     // snapshot can't be rolled back.
     pub obligations_in_snapshot: Cell<bool>,
+
+    // This is false except during closure kind inference. It is used
+    // by the mem-categorization code to be able to have stricter
+    // assertions (which are always true except during upvar
+    // inference).
+    during_closure_kind_inference: Cell<bool>,
 }
 
 /// A map returned by `skolemize_late_bound_regions()` indicating the skolemized
@@ -491,6 +497,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'gcx> {
             tainted_by_errors_flag: Cell::new(false),
             err_count_on_creation: self.sess.err_count(),
             obligations_in_snapshot: Cell::new(false),
+            during_closure_kind_inference: Cell::new(false),
         }
     }
 }
@@ -532,6 +539,7 @@ impl<'a, 'gcx, 'tcx> InferCtxtBuilder<'a, 'gcx, 'tcx> {
             tainted_by_errors_flag: Cell::new(false),
             err_count_on_creation: tcx.sess.err_count(),
             obligations_in_snapshot: Cell::new(false),
+            during_closure_kind_inference: Cell::new(false),
         }))
     }
 }
@@ -1294,6 +1302,14 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                                         .map(|method| resolve_ty(method.ty)))
     }
 
+    pub fn set_during_closure_kind_inference(&self, value: bool) {
+        self.during_closure_kind_inference.set(value);
+    }
+
+    pub fn during_closure_kind_inference(&self) -> bool {
+        self.during_closure_kind_inference.get()
+    }
+
     /// True if errors have been reported since this infcx was
     /// created.  This is sometimes used as a heuristic to skip
     /// reporting errors that often occur as a result of earlier
diff --git a/src/librustc/middle/mem_categorization.rs b/src/librustc/middle/mem_categorization.rs
@@ -362,7 +362,9 @@ impl MutabilityCategory {
 impl<'a, 'gcx, 'tcx> MemCategorizationContext<'a, 'gcx, 'tcx> {
     pub fn new(infcx: &'a InferCtxt<'a, 'gcx, 'tcx>)
                -> MemCategorizationContext<'a, 'gcx, 'tcx> {
-        MemCategorizationContext { infcx: infcx }
+        MemCategorizationContext {
+            infcx: infcx,
+        }
     }
 
     fn tcx(&self) -> TyCtxt<'a, 'gcx, 'tcx> {
@@ -584,10 +586,20 @@ impl<'a, 'gcx, 'tcx> MemCategorizationContext<'a, 'gcx, 'tcx> {
                               self.cat_upvar(id, span, var_id, fn_node_id, kind)
                           }
                           None => {
-                              span_bug!(
-                                  span,
-                                  "No closure kind for {:?}",
-                                  closure_id);
+                              if !self.infcx.during_closure_kind_inference() {
+                                  span_bug!(
+                                      span,
+                                      "No closure kind for {:?}",
+                                      closure_id);
+                              }
+
+                              // during closure kind inference, we
+                              // don't know the closure kind yet, but
+                              // it's ok because we detect that we are
+                              // accessing an upvar and handle that
+                              // case specially anyhow. Use Fn
+                              // arbitrarily.
+                              self.cat_upvar(id, span, var_id, fn_node_id, ty::ClosureKind::Fn)
                           }
                       }
                   }
diff --git a/src/librustc_typeck/check/upvar.rs b/src/librustc_typeck/check/upvar.rs
@@ -47,11 +47,11 @@ use middle::mem_categorization as mc;
 use middle::mem_categorization::Categorization;
 use rustc::ty::{self, Ty};
 use rustc::infer::UpvarRegion;
-use std::collections::HashSet;
 use syntax::ast;
 use syntax_pos::Span;
 use rustc::hir;
 use rustc::hir::intravisit::{self, Visitor};
+use rustc::util::nodemap::NodeMap;
 
 ///////////////////////////////////////////////////////////////////////////
 // PUBLIC ENTRY POINTS
@@ -60,9 +60,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
     pub fn closure_analyze_fn(&self, body: &hir::Block) {
         let mut seed = SeedBorrowKind::new(self);
         seed.visit_block(body);
-        let closures_with_inferred_kinds = seed.closures_with_inferred_kinds;
 
-        let mut adjust = AdjustBorrowKind::new(self, &closures_with_inferred_kinds);
+        let mut adjust = AdjustBorrowKind::new(self, seed.temp_closure_kinds);
         adjust.visit_block(body);
 
         // it's our job to process these.
@@ -72,9 +71,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
     pub fn closure_analyze_const(&self, body: &hir::Expr) {
         let mut seed = SeedBorrowKind::new(self);
         seed.visit_expr(body);
-        let closures_with_inferred_kinds = seed.closures_with_inferred_kinds;
 
-        let mut adjust = AdjustBorrowKind::new(self, &closures_with_inferred_kinds);
+        let mut adjust = AdjustBorrowKind::new(self, seed.temp_closure_kinds);
         adjust.visit_expr(body);
 
         // it's our job to process these.
@@ -87,7 +85,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
 
 struct SeedBorrowKind<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {
     fcx: &'a FnCtxt<'a, 'gcx, 'tcx>,
-    closures_with_inferred_kinds: HashSet<ast::NodeId>,
+    temp_closure_kinds: NodeMap<ty::ClosureKind>,
 }
 
 impl<'a, 'gcx, 'tcx, 'v> Visitor<'v> for SeedBorrowKind<'a, 'gcx, 'tcx> {
@@ -106,7 +104,7 @@ impl<'a, 'gcx, 'tcx, 'v> Visitor<'v> for SeedBorrowKind<'a, 'gcx, 'tcx> {
 
 impl<'a, 'gcx, 'tcx> SeedBorrowKind<'a, 'gcx, 'tcx> {
     fn new(fcx: &'a FnCtxt<'a, 'gcx, 'tcx>) -> SeedBorrowKind<'a, 'gcx, 'tcx> {
-        SeedBorrowKind { fcx: fcx, closures_with_inferred_kinds: HashSet::new() }
+        SeedBorrowKind { fcx: fcx, temp_closure_kinds: NodeMap() }
     }
 
     fn check_closure(&mut self,
@@ -116,11 +114,8 @@ impl<'a, 'gcx, 'tcx> SeedBorrowKind<'a, 'gcx, 'tcx> {
     {
         let closure_def_id = self.fcx.tcx.map.local_def_id(expr.id);
         if !self.fcx.tables.borrow().closure_kinds.contains_key(&closure_def_id) {
-            self.closures_with_inferred_kinds.insert(expr.id);
-            self.fcx.tables.borrow_mut().closure_kinds
-                                        .insert(closure_def_id, ty::ClosureKind::Fn);
-            debug!("check_closure: adding closure_id={:?} to closures_with_inferred_kinds",
-                   closure_def_id);
+            self.temp_closure_kinds.insert(expr.id, ty::ClosureKind::Fn);
+            debug!("check_closure: adding closure {:?} as Fn", expr.id);
         }
 
         self.fcx.tcx.with_freevars(expr.id, |freevars| {
@@ -154,14 +149,14 @@ impl<'a, 'gcx, 'tcx> SeedBorrowKind<'a, 'gcx, 'tcx> {
 
 struct AdjustBorrowKind<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {
     fcx: &'a FnCtxt<'a, 'gcx, 'tcx>,
-    closures_with_inferred_kinds: &'a HashSet<ast::NodeId>,
+    temp_closure_kinds: NodeMap<ty::ClosureKind>,
 }
 
 impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> {
     fn new(fcx: &'a FnCtxt<'a, 'gcx, 'tcx>,
-           closures_with_inferred_kinds: &'a HashSet<ast::NodeId>)
+           temp_closure_kinds: NodeMap<ty::ClosureKind>)
            -> AdjustBorrowKind<'a, 'gcx, 'tcx> {
-        AdjustBorrowKind { fcx: fcx, closures_with_inferred_kinds: closures_with_inferred_kinds }
+        AdjustBorrowKind { fcx: fcx, temp_closure_kinds: temp_closure_kinds }
     }
 
     fn analyze_closure(&mut self,
@@ -176,8 +171,10 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> {
         debug!("analyze_closure(id={:?}, body.id={:?})", id, body.id);
 
         {
+            self.fcx.set_during_closure_kind_inference(true);
             let mut euv = euv::ExprUseVisitor::new(self, self.fcx);
             euv.walk_fn(decl, body);
+            self.fcx.set_during_closure_kind_inference(false);
         }
 
         // Now that we've analyzed the closure, we know how each
@@ -211,10 +208,14 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> {
             self.fcx.demand_eqtype(span, final_upvar_ty, upvar_ty);
         }
 
-        // Now we must process and remove any deferred resolutions,
-        // since we have a concrete closure kind.
+        // If we are also inferred the closure kind here, update the
+        // main table and process any deferred resolutions.
         let closure_def_id = self.fcx.tcx.map.local_def_id(id);
-        if self.closures_with_inferred_kinds.contains(&id) {
+        if let Some(&kind) = self.temp_closure_kinds.get(&id) {
+            self.fcx.tables.borrow_mut().closure_kinds
+                                        .insert(closure_def_id, kind);
+            debug!("closure_kind({:?}) = {:?}", closure_def_id, kind);
+
             let mut deferred_call_resolutions =
                 self.fcx.remove_deferred_call_resolutions(closure_def_id);
             for deferred_call_resolution in &mut deferred_call_resolutions {
@@ -259,7 +260,7 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> {
             })
     }
 
-    fn adjust_upvar_borrow_kind_for_consume(&self,
+    fn adjust_upvar_borrow_kind_for_consume(&mut self,
                                             cmt: mc::cmt<'tcx>,
                                             mode: euv::ConsumeMode)
     {
@@ -350,7 +351,7 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> {
         }
     }
 
-    fn adjust_upvar_borrow_kind_for_unique(&self, cmt: mc::cmt<'tcx>) {
+    fn adjust_upvar_borrow_kind_for_unique(&mut self, cmt: mc::cmt<'tcx>) {
         debug!("adjust_upvar_borrow_kind_for_unique(cmt={:?})",
                cmt);
 
@@ -381,7 +382,7 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> {
         }
     }
 
-    fn try_adjust_upvar_deref(&self,
+    fn try_adjust_upvar_deref(&mut self,
                               note: &mc::Note,
                               borrow_kind: ty::BorrowKind)
                               -> bool
@@ -430,7 +431,7 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> {
     /// moving from left to right as needed (but never right to left).
     /// Here the argument `mutbl` is the borrow_kind that is required by
     /// some particular use.
-    fn adjust_upvar_borrow_kind(&self,
+    fn adjust_upvar_borrow_kind(&mut self,
                                 upvar_id: ty::UpvarId,
                                 upvar_capture: &mut ty::UpvarCapture,
                                 kind: ty::BorrowKind) {
@@ -460,36 +461,30 @@ impl<'a, 'gcx, 'tcx> AdjustBorrowKind<'a, 'gcx, 'tcx> {
         }
     }
 
-    fn adjust_closure_kind(&self,
+    fn adjust_closure_kind(&mut self,
                            closure_id: ast::NodeId,
                            new_kind: ty::ClosureKind) {
         debug!("adjust_closure_kind(closure_id={}, new_kind={:?})",
                closure_id, new_kind);
 
-        if !self.closures_with_inferred_kinds.contains(&closure_id) {
-            return;
-        }
-
-        let closure_def_id = self.fcx.tcx.map.local_def_id(closure_id);
-        let closure_kinds = &mut self.fcx.tables.borrow_mut().closure_kinds;
-        let existing_kind = *closure_kinds.get(&closure_def_id).unwrap();
+        if let Some(&existing_kind) = self.temp_closure_kinds.get(&closure_id) {
+            debug!("adjust_closure_kind: closure_id={}, existing_kind={:?}, new_kind={:?}",
+                   closure_id, existing_kind, new_kind);
 
-        debug!("adjust_closure_kind: closure_id={}, existing_kind={:?}, new_kind={:?}",
-               closure_id, existing_kind, new_kind);
-
-        match (existing_kind, new_kind) {
-            (ty::ClosureKind::Fn, ty::ClosureKind::Fn) |
-            (ty::ClosureKind::FnMut, ty::ClosureKind::Fn) |
-            (ty::ClosureKind::FnMut, ty::ClosureKind::FnMut) |
-            (ty::ClosureKind::FnOnce, _) => {
-                // no change needed
-            }
+            match (existing_kind, new_kind) {
+                (ty::ClosureKind::Fn, ty::ClosureKind::Fn) |
+                (ty::ClosureKind::FnMut, ty::ClosureKind::Fn) |
+                (ty::ClosureKind::FnMut, ty::ClosureKind::FnMut) |
+                (ty::ClosureKind::FnOnce, _) => {
+                    // no change needed
+                }
 
-            (ty::ClosureKind::Fn, ty::ClosureKind::FnMut) |
-            (ty::ClosureKind::Fn, ty::ClosureKind::FnOnce) |
-            (ty::ClosureKind::FnMut, ty::ClosureKind::FnOnce) => {
-                // new kind is stronger than the old kind
-                closure_kinds.insert(closure_def_id, new_kind);
+                (ty::ClosureKind::Fn, ty::ClosureKind::FnMut) |
+                (ty::ClosureKind::Fn, ty::ClosureKind::FnOnce) |
+                (ty::ClosureKind::FnMut, ty::ClosureKind::FnOnce) => {
+                    // new kind is stronger than the old kind
+                    self.temp_closure_kinds.insert(closure_id, new_kind);
+                }
             }
         }
     }
diff --git a/src/test/compile-fail/issue-34349.rs b/src/test/compile-fail/issue-34349.rs
@@ -0,0 +1,32 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// This is a regression test for a problem encountered around upvar
+// inference and trait caching: in particular, we were entering a
+// temporary closure kind during inference, and then caching results
+// based on that temporary kind, which led to no error being reported
+// in this particular test.
+
+fn main() {
+    let inc = || {};
+    inc();
+
+    fn apply<F>(f: F) where F: Fn() {
+        f()
+    }
+
+    let mut farewell = "goodbye".to_owned();
+    let diary = || { //~ ERROR E0525
+        farewell.push_str("!!!");
+        println!("Then I screamed {}.", farewell);
+    };
+
+    apply(diary);
+}

Original file line number	Diff line number	Diff line change
`@@ -175,6 +175,12 @@ pub struct InferCtxt<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {`
`175`	`175`	`// any obligations set during the current snapshot. In that case, the`
`176`	`176`	`// snapshot can't be rolled back.`
`177`	`177`	`pub obligations_in_snapshot: Cell<bool>,`
	`178`	`+`
	`179`	`+ // This is false except during closure kind inference. It is used`
	`180`	`+ // by the mem-categorization code to be able to have stricter`
	`181`	`+ // assertions (which are always true except during upvar`
	`182`	`+ // inference).`
	`183`	`+ during_closure_kind_inference: Cell<bool>,`
`178`	`184`	`}`
`179`	`185`
`180`	`186`	/// A map returned by `skolemize_late_bound_regions()` indicating the skolemized
`@@ -491,6 +497,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'gcx> {`
`491`	`497`	`tainted_by_errors_flag: Cell::new(false),`
`492`	`498`	`err_count_on_creation: self.sess.err_count(),`
`493`	`499`	`obligations_in_snapshot: Cell::new(false),`
	`500`	`+ during_closure_kind_inference: Cell::new(false),`
`494`	`501`	`}`
`495`	`502`	`}`
`496`	`503`	`}`
`@@ -532,6 +539,7 @@ impl<'a, 'gcx, 'tcx> InferCtxtBuilder<'a, 'gcx, 'tcx> {`
`532`	`539`	`tainted_by_errors_flag: Cell::new(false),`
`533`	`540`	`err_count_on_creation: tcx.sess.err_count(),`
`534`	`541`	`obligations_in_snapshot: Cell::new(false),`
	`542`	`+ during_closure_kind_inference: Cell::new(false),`
`535`	`543`	`}))`
`536`	`544`	`}`
`537`	`545`	`}`
`@@ -1294,6 +1302,14 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {`
`1294`	`1302`	`.map(\|method\| resolve_ty(method.ty)))`
`1295`	`1303`	`}`
`1296`	`1304`
	`1305`	`+ pub fn set_during_closure_kind_inference(&self, value: bool) {`
	`1306`	`+ self.during_closure_kind_inference.set(value);`
	`1307`	`+ }`
	`1308`	`+`
	`1309`	`+ pub fn during_closure_kind_inference(&self) -> bool {`
	`1310`	`+ self.during_closure_kind_inference.get()`
	`1311`	`+ }`
	`1312`	`+`
`1297`	`1313`	`/// True if errors have been reported since this infcx was`
`1298`	`1314`	`/// created. This is sometimes used as a heuristic to skip`
`1299`	`1315`	`/// reporting errors that often occur as a result of earlier`