Tweak mem categorization of upvar mutability

- Correctly categorize env pointer deref for `FnMut` as declared
  rather than inherited.  This fixes an assert in borrowck.
  Closes #18238
- Categorize env pointer deref as mutable only if the closure is
  `FnMut` *and* the original variable is declared mutable.  This
  disallows capture-by-value `FnMut` closures from mutating captured
  variables that aren't declared mutable.  This is a difference
  from the equivalent desugared code which would permit it, but
  it is consistent with the behavior of procs.  Closes #18335
- Avoid computing info about the env pointer if there isn't one.

Author: bkoropoff

src/librustc/middle/mem_categorization.rs

@@ -656,51 +656,54 @@ impl<'t,'tcx,TYPER:Typer<'tcx>> MemCategorizationContext<'t,TYPER> {
         // FnOnce         | copied               | upvar -> &'up bk
         // old stack      | N/A                  | upvar -> &'env mut -> &'up bk
         // old proc/once  | copied               | N/A
+        let var_ty = if_ok!(self.node_ty(var_id));
+
         let upvar_id = ty::UpvarId { var_id: var_id,
                                      closure_expr_id: fn_node_id };
 
-        // Do we need to deref through an env reference?
-        let has_env_deref = kind != ty::FnOnceUnboxedClosureKind;
-
         // Mutability of original variable itself
         let var_mutbl = MutabilityCategory::from_local(self.tcx(), var_id);
 
-        // Mutability of environment dereference
-        let env_mutbl = match kind {
-            ty::FnOnceUnboxedClosureKind => var_mutbl,
-            ty::FnMutUnboxedClosureKind => McInherited,
-            ty::FnUnboxedClosureKind => McImmutable
+        // Construct information about env pointer dereference, if any
+        let mutbl = match kind {
+            ty::FnOnceUnboxedClosureKind => None, // None, env is by-value
+            ty::FnMutUnboxedClosureKind => match mode { // Depends on capture type
+                ast::CaptureByValue => Some(var_mutbl), // Mutable if the original var is
+                ast::CaptureByRef => Some(McDeclared) // Mutable regardless
+            },
+            ty::FnUnboxedClosureKind => Some(McImmutable) // Never mutable
         };
+        let env_info = mutbl.map(|env_mutbl| {
+            // Look up the node ID of the closure body so we can construct
+            // a free region within it
+            let fn_body_id = {
+                let fn_expr = match self.tcx().map.find(fn_node_id) {
+                    Some(ast_map::NodeExpr(e)) => e,
+                    _ => unreachable!()
+                };
 
-        // Look up the node ID of the closure body so we can construct
-        // a free region within it
-        let fn_body_id = {
-            let fn_expr = match self.tcx().map.find(fn_node_id) {
-                Some(ast_map::NodeExpr(e)) => e,
-                _ => unreachable!()
+                match fn_expr.node {
+                    ast::ExprFnBlock(_, _, ref body) |
+                    ast::ExprProc(_, ref body) |
+                    ast::ExprUnboxedFn(_, _, _, ref body) => body.id,
+                    _ => unreachable!()
+                }
             };
 
-            match fn_expr.node {
-                ast::ExprFnBlock(_, _, ref body) |
-                ast::ExprProc(_, ref body) |
-                ast::ExprUnboxedFn(_, _, _, ref body) => body.id,
-                _ => unreachable!()
-            }
-        };
+            // Region of environment pointer
+            let env_region = ty::ReFree(ty::FreeRegion {
+                scope_id: fn_body_id,
+                bound_region: ty::BrEnv
+            });
 
-        // Region of environment pointer
-        let env_region = ty::ReFree(ty::FreeRegion {
-            scope_id: fn_body_id,
-            bound_region: ty::BrEnv
-        });
-
-        let env_ptr = BorrowedPtr(if env_mutbl.is_mutable() {
-            ty::MutBorrow
-        } else {
-            ty::ImmBorrow
-        }, env_region);
+            let env_ptr = BorrowedPtr(if env_mutbl.is_mutable() {
+                ty::MutBorrow
+            } else {
+                ty::ImmBorrow
+            }, env_region);
 
-        let var_ty = if_ok!(self.node_ty(var_id));
+            (env_mutbl, env_ptr)
+        });
 
         // First, switch by capture mode
         Ok(match mode {
@@ -718,25 +721,27 @@ impl<'t,'tcx,TYPER:Typer<'tcx>> MemCategorizationContext<'t,TYPER> {
                     note: NoteNone
                 };
 
-                if has_env_deref {
-                    // We need to add the env deref.  This means that
-                    // the above is actually immutable and has a ref
-                    // type.  However, nothing should actually look at
-                    // the type, so we can get away with stuffing a
-                    // `ty_err` in there instead of bothering to
-                    // construct a proper one.
-                    base.mutbl = McImmutable;
-                    base.ty = ty::mk_err();
-                    Rc::new(cmt_ {
-                        id: id,
-                        span: span,
-                        cat: cat_deref(Rc::new(base), 0, env_ptr),
-                        mutbl: env_mutbl,
-                        ty: var_ty,
-                        note: NoteClosureEnv(upvar_id)
-                    })
-                } else {
-                    Rc::new(base)
+                match env_info {
+                    Some((env_mutbl, env_ptr)) => {
+                        // We need to add the env deref.  This means
+                        // that the above is actually immutable and
+                        // has a ref type.  However, nothing should
+                        // actually look at the type, so we can get
+                        // away with stuffing a `ty_err` in there
+                        // instead of bothering to construct a proper
+                        // one.
+                        base.mutbl = McImmutable;
+                        base.ty = ty::mk_err();
+                        Rc::new(cmt_ {
+                            id: id,
+                            span: span,
+                            cat: cat_deref(Rc::new(base), 0, env_ptr),
+                            mutbl: env_mutbl,
+                            ty: var_ty,
+                            note: NoteClosureEnv(upvar_id)
+                        })
+                    }
+                    None => Rc::new(base)
                 }
             },
             ast::CaptureByRef => {
@@ -756,16 +761,18 @@ impl<'t,'tcx,TYPER:Typer<'tcx>> MemCategorizationContext<'t,TYPER> {
                     note: NoteNone
                 };
 
-                // As in the by-value case, add env deref if needed
-                if has_env_deref {
-                    base = cmt_ {
-                        id: id,
-                        span: span,
-                        cat: cat_deref(Rc::new(base), 0, env_ptr),
-                        mutbl: env_mutbl,
-                        ty: ty::mk_err(),
-                        note: NoteClosureEnv(upvar_id)
-                    };
+                match env_info {
+                    Some((env_mutbl, env_ptr)) => {
+                        base = cmt_ {
+                            id: id,
+                            span: span,
+                            cat: cat_deref(Rc::new(base), 0, env_ptr),
+                            mutbl: env_mutbl,
+                            ty: ty::mk_err(),
+                            note: NoteClosureEnv(upvar_id)
+                        };
+                    }
+                    None => {}
                 }
 
                 // Look up upvar borrow so we can get its region