Made ref pattern bindings correctly pick Deref or DerefMut

Added LvaluePreference::from_mutbl

Closes #15609

Author: Kimundi

src/librustc/middle/pat_util.rs

@@ -135,22 +135,34 @@ pub fn pat_contains_bindings(dm: &DefMap, pat: &ast::Pat) -> bool {
     contains_bindings
 }
 
-/// Checks if the pattern contains any `ref` or `ref mut` bindings.
-pub fn pat_contains_ref_binding(dm: &DefMap, pat: &ast::Pat) -> bool {
-    let mut result = false;
+/// Checks if the pattern contains any `ref` or `ref mut` bindings,
+/// and if yes wether its containing mutable ones or just immutables ones.
+pub fn pat_contains_ref_binding(dm: &DefMap, pat: &ast::Pat) -> Option<ast::Mutability> {
+    let mut result = None;
     pat_bindings(dm, pat, |mode, _, _, _| {
         match mode {
-            ast::BindingMode::BindByRef(_) => { result = true; }
+            ast::BindingMode::BindByRef(m) => {
+                // Pick Mutable as maximum
+                match result {
+                    None | Some(ast::MutImmutable) => result = Some(m),
+                    _ => (),
+                }
+            }
             ast::BindingMode::BindByValue(_) => { }
         }
     });
     result
 }
 
 /// Checks if the patterns for this arm contain any `ref` or `ref mut`
-/// bindings.
-pub fn arm_contains_ref_binding(dm: &DefMap, arm: &ast::Arm) -> bool {
-    arm.pats.iter().any(|pat| pat_contains_ref_binding(dm, pat))
+/// bindings, and if yes wether its containing mutable ones or just immutables ones.
+pub fn arm_contains_ref_binding(dm: &DefMap, arm: &ast::Arm) -> Option<ast::Mutability> {
+    arm.pats.iter()
+            .filter_map(|pat| pat_contains_ref_binding(dm, pat))
+            .max_by(|m| match *m {
+                ast::MutMutable => 1,
+                ast::MutImmutable => 0,
+            })
 }
 
 /// Checks if the pattern contains any patterns that bind something to

src/librustc/middle/ty.rs

@@ -2846,11 +2846,11 @@ impl<'tcx> ctxt<'tcx> {
         self.ty_param_defs.borrow().get(&node_id).unwrap().clone()
     }
 
-    pub fn pat_contains_ref_binding(&self, pat: &ast::Pat) -> bool {
+    pub fn pat_contains_ref_binding(&self, pat: &ast::Pat) -> Option<ast::Mutability> {
         pat_util::pat_contains_ref_binding(&self.def_map, pat)
     }
 
-    pub fn arm_contains_ref_binding(&self, arm: &ast::Arm) -> bool {
+    pub fn arm_contains_ref_binding(&self, arm: &ast::Arm) -> Option<ast::Mutability> {
         pat_util::arm_contains_ref_binding(&self.def_map, arm)
     }
 }

src/librustc_typeck/check/_match.rs

@@ -18,6 +18,7 @@ use middle::subst::Substs;
 use middle::ty::{self, Ty};
 use check::{check_expr, check_expr_has_type, check_expr_with_expectation};
 use check::{check_expr_coercable_to_type, demand, FnCtxt, Expectation};
+use check::{check_expr_with_lvalue_pref, LvaluePreference};
 use check::{instantiate_path, resolve_ty_and_def_ufcs, structurally_resolved_type};
 use require_same_types;
 use util::nodemap::FnvHashMap;
@@ -438,10 +439,15 @@ pub fn check_match<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
     // Not entirely obvious: if matches may create ref bindings, we
     // want to use the *precise* type of the discriminant, *not* some
     // supertype, as the "discriminant type" (issue #23116).
-    let contains_ref_bindings = arms.iter().any(|a| tcx.arm_contains_ref_binding(a));
+    let contains_ref_bindings = arms.iter()
+                                    .filter_map(|a| tcx.arm_contains_ref_binding(a))
+                                    .max_by(|m| match *m {
+                                        ast::MutMutable => 1,
+                                        ast::MutImmutable => 0,
+                                    });
     let discrim_ty;
-    if contains_ref_bindings {
-        check_expr(fcx, discrim);
+    if let Some(m) = contains_ref_bindings {
+        check_expr_with_lvalue_pref(fcx, discrim, LvaluePreference::from_mutbl(m));
         discrim_ty = fcx.expr_ty(discrim);
     } else {
         // ...but otherwise we want to use any supertype of the

src/librustc_typeck/check/coercion.rs

@@ -60,7 +60,7 @@
 //! sort of a minor point so I've opted to leave it for later---after all
 //! we may want to adjust precisely when coercions occur.
 
-use check::{autoderef, FnCtxt, NoPreference, PreferMutLvalue, UnresolvedTypeAction};
+use check::{autoderef, FnCtxt, LvaluePreference, UnresolvedTypeAction};
 
 use middle::infer::{self, Coercion};
 use middle::traits::{self, ObligationCause};
@@ -188,10 +188,7 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> {
         let r_borrow = self.tcx().mk_region(r_borrow);
         let autoref = Some(ty::AutoPtr(r_borrow, mutbl_b));
 
-        let lvalue_pref = match mutbl_b {
-            ast::MutMutable => PreferMutLvalue,
-            ast::MutImmutable => NoPreference
-        };
+        let lvalue_pref = LvaluePreference::from_mutbl(mutbl_b);
         let mut first_error = None;
         let (_, autoderefs, success) = autoderef(self.fcx,
                                                  expr_a.span,

src/librustc_typeck/check/mod.rs

@@ -1908,6 +1908,15 @@ pub enum LvaluePreference {
     NoPreference
 }
 
+impl LvaluePreference {
+    pub fn from_mutbl(m: ast::Mutability) -> Self {
+        match m {
+            ast::MutMutable => PreferMutLvalue,
+            ast::MutImmutable => NoPreference,
+        }
+    }
+}
+
 /// Whether `autoderef` requires types to resolve.
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
 pub enum UnresolvedTypeAction {
@@ -3224,10 +3233,7 @@ fn check_expr_with_unifier<'a, 'tcx, F>(fcx: &FnCtxt<'a, 'tcx>,
                 _ => NoExpectation
             }
         });
-        let lvalue_pref = match mutbl {
-            ast::MutMutable => PreferMutLvalue,
-            ast::MutImmutable => NoPreference
-        };
+        let lvalue_pref = LvaluePreference::from_mutbl(mutbl);
         check_expr_with_expectation_and_lvalue_pref(fcx,
                                                     &**oprnd,
                                                     hint,
@@ -3925,9 +3931,7 @@ pub fn check_decl_initializer<'a,'tcx>(fcx: &FnCtxt<'a,'tcx>,
     let ref_bindings = fcx.tcx().pat_contains_ref_binding(&local.pat);
 
     let local_ty = fcx.local_ty(init.span, local.id);
-    if !ref_bindings {
-        check_expr_coercable_to_type(fcx, init, local_ty)
-    } else {
+    if let Some(m) = ref_bindings {
         // Somewhat subtle: if we have a `ref` binding in the pattern,
         // we want to avoid introducing coercions for the RHS. This is
         // both because it helps preserve sanity and, in the case of
@@ -3936,9 +3940,11 @@ pub fn check_decl_initializer<'a,'tcx>(fcx: &FnCtxt<'a,'tcx>,
         // referent for the reference that results is *equal to* the
         // type of the lvalue it is referencing, and not some
         // supertype thereof.
-        check_expr(fcx, init);
+        check_expr_with_lvalue_pref(fcx, init, LvaluePreference::from_mutbl(m));
         let init_ty = fcx.expr_ty(init);
         demand::eqtype(fcx, init.span, init_ty, local_ty);
+    } else {
+        check_expr_coercable_to_type(fcx, init, local_ty)
     };
 }
 

src/test/run-pass/overloaded_deref_with_ref_pattern.rs

@@ -0,0 +1,102 @@
+// Copyright 2015 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.
+
+// Test that we choose Deref or DerefMut appropriately based on mutability of ref bindings (#15609).
+
+use std::ops::{Deref, DerefMut};
+
+struct DerefOk<T>(T);
+struct DerefMutOk<T>(T);
+
+impl<T> Deref for DerefOk<T> {
+    type Target = T;
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl<T> DerefMut for DerefOk<T> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        panic!()
+    }
+}
+
+impl<T> Deref for DerefMutOk<T> {
+    type Target = T;
+    fn deref(&self) -> &Self::Target {
+        panic!()
+    }
+}
+
+impl<T> DerefMut for DerefMutOk<T> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.0
+    }
+}
+
+fn main() {
+    // Check that mutable ref binding in match picks DerefMut
+    let mut b = DerefMutOk(0);
+    match *b {
+        ref mut n => n,
+    };
+
+    // Check that mutable ref binding in let picks DerefMut
+    let mut y = DerefMutOk(1);
+    let ref mut z = *y;
+
+    // Check that immutable ref binding in match picks Deref
+    let mut b = DerefOk(2);
+    match *b {
+        ref n => n,
+    };
+
+    // Check that immutable ref binding in let picks Deref
+    let mut y = DerefOk(3);
+    let ref z = *y;
+
+    // Check that mixed mutable/immutable ref binding in match picks DerefMut
+    let mut b = DerefMutOk((0, 9));
+    match *b {
+        (ref mut n, ref m) => (n, m),
+    };
+
+    let mut b = DerefMutOk((0, 9));
+    match *b {
+        (ref n, ref mut m) => (n, m),
+    };
+
+    // Check that mixed mutable/immutable ref binding in let picks DerefMut
+    let mut y = DerefMutOk((1, 8));
+    let (ref mut z, ref a) = *y;
+
+    let mut y = DerefMutOk((1, 8));
+    let (ref z, ref mut a) = *y;
+
+    // Check that multiple immutable ref bindings in match picks Deref
+    let mut b = DerefOk((2, 7));
+    match *b {
+        (ref n, ref m) => (n, m),
+    };
+
+    // Check that multiple immutable ref bindings in let picks Deref
+    let mut y = DerefOk((3, 6));
+    let (ref z, ref a) = *y;
+
+    // Check that multiple mutable ref bindings in match picks DerefMut
+    let mut b = DerefMutOk((4, 5));
+    match *b {
+        (ref mut n, ref mut m) => (n, m),
+    };
+
+    // Check that multiple mutable ref bindings in let picks DerefMut
+    let mut y = DerefMutOk((5, 4));
+    let (ref mut z, ref mut a) = *y;
+}

src/test/run-pass/overloaded_deref_with_ref_pattern_issue15609.rs

@@ -0,0 +1,44 @@
+// Copyright 2015 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.
+
+// Test that we choose Deref or DerefMut appropriately based on mutability of ref bindings (#15609).
+
+fn main() {
+    use std::cell::RefCell;
+
+    struct S {
+        node: E,
+    }
+
+    enum E {
+        Foo(u32),
+        Bar,
+    }
+
+    // Check match
+    let x = RefCell::new(S { node: E::Foo(0) });
+
+    let mut b = x.borrow_mut();
+    match b.node {
+        E::Foo(ref mut n) => *n += 1,
+        _ => (),
+    }
+
+    // Check let
+    let x = RefCell::new(0);
+    let mut y = x.borrow_mut();
+    let ref mut z = *y;
+
+    fn foo(a: &mut RefCell<Option<String>>) {
+        if let Some(ref mut s) = *a.borrow_mut() {
+            s.push('a')
+        }
+    }
+}