Auto merge of #27087 - nikomatsakis:closure-exploration, r=nrc

Refactors the "desugaring" of closures to expose the types of the upvars. This is necessary to be faithful with how actual structs work. The reasoning of the particular desugaring that I chose is explained in a fairly detailed comment.

As a side-effect, recursive closure types are prohibited unless a trait object intermediary is used. This fixes #25954 and also eliminates concerns about unrepresentable closure types that have infinite size, I believe. I don't believe this can cause regressions because of #25954.

(As for motivation, besides #25954 etc, this work is also intended as refactoring in support of incremental compilation, since closures are one of the thornier cases encountered when attempting to split node-ids into item-ids and within-item-ids. The goal is to eliminate the "internal def-id" distinction in astdecoding. However, I have to do more work on trans to really make progress there.)

r? @nrc

Author: bors

src/librustc/metadata/tydecode.rs

@@ -564,8 +564,13 @@ fn parse_ty_<'a, 'tcx, F>(st: &mut PState<'a, 'tcx>, conv: &mut F) -> Ty<'tcx> w
           assert_eq!(next(st), '[');
           let did = parse_def_(st, ClosureSource, conv);
           let substs = parse_substs_(st, conv);
+          let mut tys = vec![];
+          while peek(st) != '.' {
+              tys.push(parse_ty_(st, conv));
+          }
+          assert_eq!(next(st), '.');
           assert_eq!(next(st), ']');
-          return st.tcx.mk_closure(did, st.tcx.mk_substs(substs));
+          return st.tcx.mk_closure(did, st.tcx.mk_substs(substs), tys);
       }
       'P' => {
           assert_eq!(next(st), '[');

src/librustc/metadata/tyencode.rs

@@ -143,9 +143,13 @@ pub fn enc_ty<'a, 'tcx>(w: &mut Encoder, cx: &ctxt<'a, 'tcx>, t: Ty<'tcx>) {
             enc_substs(w, cx, substs);
             mywrite!(w, "]");
         }
-        ty::TyClosure(def, substs) => {
+        ty::TyClosure(def, ref substs) => {
             mywrite!(w, "k[{}|", (cx.ds)(def));
-            enc_substs(w, cx, substs);
+            enc_substs(w, cx, &substs.func_substs);
+            for ty in &substs.upvar_tys {
+                enc_ty(w, cx, ty);
+            }
+            mywrite!(w, ".");
             mywrite!(w, "]");
         }
         ty::TyProjection(ref data) => {

src/librustc/middle/expr_use_visitor.rs

@@ -256,7 +256,10 @@ macro_rules! return_if_err {
     ($inp: expr) => (
         match $inp {
             Ok(v) => v,
-            Err(()) => return
+            Err(()) => {
+                debug!("mc reported err");
+                return
+            }
         }
     )
 }

src/librustc/middle/free_region.rs

@@ -40,7 +40,6 @@ impl FreeRegionMap {
                     self.relate_free_regions(free_a, free_b);
                 }
                 Implication::RegionSubRegion(..) |
-                Implication::RegionSubClosure(..) |
                 Implication::RegionSubGeneric(..) |
                 Implication::Predicate(..) => {
                 }

src/librustc/middle/implicator.rs

@@ -28,7 +28,6 @@ use util::nodemap::FnvHashSet;
 pub enum Implication<'tcx> {
     RegionSubRegion(Option<Ty<'tcx>>, ty::Region, ty::Region),
     RegionSubGeneric(Option<Ty<'tcx>>, ty::Region, GenericKind<'tcx>),
-    RegionSubClosure(Option<Ty<'tcx>>, ty::Region, ast::DefId, &'tcx Substs<'tcx>),
     Predicate(ast::DefId, ty::Predicate<'tcx>),
 }
 
@@ -96,9 +95,47 @@ impl<'a, 'tcx> Implicator<'a, 'tcx> {
                 // No borrowed content reachable here.
             }
 
-            ty::TyClosure(def_id, substs) => {
-                let &(r_a, opt_ty) = self.stack.last().unwrap();
-                self.out.push(Implication::RegionSubClosure(opt_ty, r_a, def_id, substs));
+            ty::TyClosure(_, ref substs) => {
+                // FIXME(#27086). We do not accumulate from substs, since they
+                // don't represent reachable data. This means that, in
+                // practice, some of the lifetime parameters might not
+                // be in scope when the body runs, so long as there is
+                // no reachable data with that lifetime. For better or
+                // worse, this is consistent with fn types, however,
+                // which can also encapsulate data in this fashion
+                // (though it's somewhat harder, and typically
+                // requires virtual dispatch).
+                //
+                // Note that changing this (in a naive way, at least)
+                // causes regressions for what appears to be perfectly
+                // reasonable code like this:
+                //
+                // ```
+                // fn foo<'a>(p: &Data<'a>) {
+                //    bar(|q: &mut Parser| q.read_addr())
+                // }
+                // fn bar(p: Box<FnMut(&mut Parser)+'static>) {
+                // }
+                // ```
+                //
+                // Note that `p` (and `'a`) are not used in the
+                // closure at all, but to meet the requirement that
+                // the closure type `C: 'static` (so it can be coerced
+                // to the object type), we get the requirement that
+                // `'a: 'static` since `'a` appears in the closure
+                // type `C`.
+                //
+                // A smarter fix might "prune" unused `func_substs` --
+                // this would avoid breaking simple examples like
+                // this, but would still break others (which might
+                // indeed be invalid, depending on your POV). Pruning
+                // would be a subtle process, since we have to see
+                // what func/type parameters are used and unused,
+                // taking into consideration UFCS and so forth.
+
+                for &upvar_ty in &substs.upvar_tys {
+                    self.accumulate_from_ty(upvar_ty);
+                }
             }
 
             ty::TyTrait(ref t) => {
@@ -273,6 +310,21 @@ impl<'a, 'tcx> Implicator<'a, 'tcx> {
         self.out.extend(obligations);
 
         let variances = self.tcx().item_variances(def_id);
+        self.accumulate_from_substs(substs, Some(&variances));
+    }
+
+    fn accumulate_from_substs(&mut self,
+                              substs: &Substs<'tcx>,
+                              variances: Option<&ty::ItemVariances>)
+    {
+        let mut tmp_variances = None;
+        let variances = variances.unwrap_or_else(|| {
+            tmp_variances = Some(ty::ItemVariances {
+                types: substs.types.map(|_| ty::Variance::Invariant),
+                regions: substs.regions().map(|_| ty::Variance::Invariant),
+            });
+            tmp_variances.as_ref().unwrap()
+        });
 
         for (&region, &variance) in substs.regions().iter().zip(&variances.regions) {
             match variance {

src/librustc/middle/infer/mod.rs

@@ -1162,7 +1162,12 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
     /// these unconstrained type variables.
     fn resolve_type_vars_or_error(&self, t: &Ty<'tcx>) -> mc::McResult<Ty<'tcx>> {
         let ty = self.resolve_type_vars_if_possible(t);
-        if ty.has_infer_types() || ty.references_error() { Err(()) } else { Ok(ty) }
+        if ty.references_error() || ty.is_ty_var() {
+            debug!("resolve_type_vars_or_error: error from {:?}", ty);
+            Err(())
+        } else {
+            Ok(ty)
+        }
     }
 
     pub fn fully_resolve<T:TypeFoldable<'tcx>>(&self, value: &T) -> FixupResult<T> {
@@ -1374,38 +1379,23 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
     }
 
     pub fn closure_type(&self,
-                    def_id: ast::DefId,
-                    substs: &subst::Substs<'tcx>)
-                    -> ty::ClosureTy<'tcx>
+                        def_id: ast::DefId,
+                        substs: &ty::ClosureSubsts<'tcx>)
+                        -> ty::ClosureTy<'tcx>
     {
-
         let closure_ty = self.tables
                              .borrow()
                              .closure_tys
                              .get(&def_id)
                              .unwrap()
-                             .subst(self.tcx, substs);
+                             .subst(self.tcx, &substs.func_substs);
 
         if self.normalize {
             normalize_associated_type(&self.tcx, &closure_ty)
         } else {
             closure_ty
         }
     }
-
-    pub fn closure_upvars(&self,
-                          def_id: ast::DefId,
-                          substs: &Substs<'tcx>)
-                          -> Option<Vec<ty::ClosureUpvar<'tcx>>>
-    {
-        let result = ty::ctxt::closure_upvars(self, def_id, substs);
-
-        if self.normalize {
-            normalize_associated_type(&self.tcx, &result)
-        } else {
-            result
-        }
-    }
 }
 
 impl<'tcx> TypeTrace<'tcx> {

src/librustc/middle/liveness.rs

@@ -1493,7 +1493,7 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> {
     fn fn_ret(&self, id: NodeId) -> ty::PolyFnOutput<'tcx> {
         let fn_ty = self.ir.tcx.node_id_to_type(id);
         match fn_ty.sty {
-            ty::TyClosure(closure_def_id, substs) =>
+            ty::TyClosure(closure_def_id, ref substs) =>
                 self.ir.tcx.closure_type(closure_def_id, substs).sig.output(),
             _ => fn_ty.fn_ret()
         }

src/librustc/middle/mem_categorization.rs

@@ -367,7 +367,13 @@ impl<'t, 'a,'tcx> MemCategorizationContext<'t, 'a, 'tcx> {
     }
 
     fn expr_ty(&self, expr: &ast::Expr) -> McResult<Ty<'tcx>> {
-        self.typer.node_ty(expr.id)
+        match self.typer.node_ty(expr.id) {
+            Ok(t) => Ok(t),
+            Err(()) => {
+                debug!("expr_ty({:?}) yielded Err", expr);
+                Err(())
+            }
+        }
     }
 
     fn expr_ty_adjusted(&self, expr: &ast::Expr) -> McResult<Ty<'tcx>> {

src/librustc/middle/traits/mod.rs

@@ -270,7 +270,7 @@ pub struct VtableImplData<'tcx, N> {
 #[derive(Clone, PartialEq, Eq)]
 pub struct VtableClosureData<'tcx, N> {
     pub closure_def_id: ast::DefId,
-    pub substs: subst::Substs<'tcx>,
+    pub substs: ty::ClosureSubsts<'tcx>,
     /// Nested obligations. This can be non-empty if the closure
     /// signature contains associated types.
     pub nested: Vec<N>
@@ -548,7 +548,7 @@ impl<'tcx, N> Vtable<'tcx, N> {
             VtableClosure(c) => VtableClosure(VtableClosureData {
                 closure_def_id: c.closure_def_id,
                 substs: c.substs,
-                nested: c.nested.into_iter().map(f).collect()
+                nested: c.nested.into_iter().map(f).collect(),
             })
         }
     }

src/librustc/middle/traits/project.rs

@@ -154,7 +154,7 @@ fn consider_unification_despite_ambiguity<'cx,'tcx>(selcx: &mut SelectionContext
     debug!("consider_unification_despite_ambiguity: self_ty.sty={:?}",
            self_ty.sty);
     match self_ty.sty {
-        ty::TyClosure(closure_def_id, substs) => {
+        ty::TyClosure(closure_def_id, ref substs) => {
             let closure_typer = selcx.closure_typer();
             let closure_type = closure_typer.closure_type(closure_def_id, substs);
             let ty::Binder((_, ret_type)) =