Skip to content

Commit 45fae88

Browse files
committed
When matching against a pattern (either via match or let) that
contains ref-bindings, do not permit any upcasting from the type of the value being matched. Similarly, do not permit coercion in a `let`. This is a [breaking-change] in that it closes a type hole that previously existed, and in that coercion is not performed. You should be able to work around the latter by converting: ```rust let ref mut x: T = expr; ``` into ```rust let x: T = expr; let ref mut x = x; ``` Restricting coercion not to apply in the case of `let ref` or `let ref mut` is sort of unexciting to me, but seems the best solution: 1. Mixing coercion and `let ref` or `let ref mut` is a bit odd, because you are taking the address of a (coerced) temporary, but only sometimes. It's not syntactically evident, in other words, what's going on. When you're doing a coercion, you're kind of 2. Put another way, I would like to preserve the relationship that `equality <= subtyping <= coercion <= as-coercion`, where this is an indication of the number of `(T1,T2)` pairs that are accepted by the various relations. Trying to mix `let ref mut` and coercion would create another kind of relation that is like coercion, but acts differently in the case where a precise match is needed. 3. In any case, this is strictly more conservative than what we had before and we can undo it in the future if we find a way to make coercion mix with type equality. The change to match I feel ok about but similarly unthrilled. There is some subtle text already concerning whether to use eqtype or subtype for identifier bindings. The best fix I think would be to always have match use strict equality but use subtyping on identifier bindings, but the comment `(*)` explains why that's not working at the moment. As above, I think we can change this as we clean up the code there.
1 parent b0aad7d commit 45fae88

File tree

6 files changed

+116
-10
lines changed

6 files changed

+116
-10
lines changed

src/librustc/middle/pat_util.rs

+18
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,24 @@ pub fn pat_contains_bindings(dm: &DefMap, pat: &ast::Pat) -> bool {
119119
contains_bindings
120120
}
121121

122+
/// Checks if the pattern contains any `ref` or `ref mut` bindings.
123+
pub fn pat_contains_ref_binding(dm: &DefMap, pat: &ast::Pat) -> bool {
124+
let mut result = false;
125+
pat_bindings(dm, pat, |mode, _, _, _| {
126+
match mode {
127+
ast::BindingMode::BindByRef(_) => { result = true; }
128+
ast::BindingMode::BindByValue(_) => { }
129+
}
130+
});
131+
result
132+
}
133+
134+
/// Checks if the patterns for this arm contain any `ref` or `ref mut`
135+
/// bindings.
136+
pub fn arm_contains_ref_binding(dm: &DefMap, arm: &ast::Arm) -> bool {
137+
arm.pats.iter().any(|pat| pat_contains_ref_binding(dm, pat))
138+
}
139+
122140
/// Checks if the pattern contains any patterns that bind something to
123141
/// an ident or wildcard, e.g. `foo`, or `Foo(_)`, `foo @ Bar(..)`,
124142
pub fn pat_contains_bindings_or_wild(dm: &DefMap, pat: &ast::Pat) -> bool {

src/librustc/middle/ty.rs

+9
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ use middle::mem_categorization as mc;
5252
use middle::region;
5353
use middle::resolve_lifetime;
5454
use middle::infer;
55+
use middle::pat_util;
5556
use middle::stability;
5657
use middle::subst::{self, ParamSpace, Subst, Substs, VecPerParamSpace};
5758
use middle::traits;
@@ -2684,6 +2685,14 @@ impl<'tcx> ctxt<'tcx> {
26842685
{
26852686
self.ty_param_defs.borrow()[node_id].clone()
26862687
}
2688+
2689+
pub fn pat_contains_ref_binding(&self, pat: &ast::Pat) -> bool {
2690+
pat_util::pat_contains_ref_binding(&self.def_map, pat)
2691+
}
2692+
2693+
pub fn arm_contains_ref_binding(&self, arm: &ast::Arm) -> bool {
2694+
pat_util::arm_contains_ref_binding(&self.def_map, arm)
2695+
}
26872696
}
26882697

26892698
// Interns a type/name combination, stores the resulting box in cx.interner,

src/librustc_typeck/check/_match.rs

+20-6
Original file line numberDiff line numberDiff line change
@@ -287,10 +287,11 @@ pub fn check_pat<'a, 'tcx>(pcx: &pat_ctxt<'a, 'tcx>,
287287
// (nmatsakis) an hour or two debugging to remember, so I thought
288288
// I'd write them down this time.
289289
//
290-
// 1. Most importantly, there is no loss of expressiveness
291-
// here. What we are saying is that the type of `x`
292-
// becomes *exactly* what is expected. This might seem
293-
// like it will cause errors in a case like this:
290+
// 1. There is no loss of expressiveness here, though it does
291+
// cause some inconvenience. What we are saying is that the type
292+
// of `x` becomes *exactly* what is expected. This can cause unnecessary
293+
// errors in some cases, such as this one:
294+
// it will cause errors in a case like this:
294295
//
295296
// ```
296297
// fn foo<'x>(x: &'x int) {
@@ -361,8 +362,21 @@ pub fn check_match<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
361362
match_src: ast::MatchSource) {
362363
let tcx = fcx.ccx.tcx;
363364

364-
let discrim_ty = fcx.infcx().next_ty_var();
365-
check_expr_has_type(fcx, discrim, discrim_ty);
365+
// Not entirely obvious: if matches may create ref bindings, we
366+
// want to use the *precise* type of the discriminant, *not* some
367+
// supertype, as the "discriminant type" (issue #23116).
368+
let contains_ref_bindings = arms.iter().any(|a| tcx.arm_contains_ref_binding(a));
369+
let discrim_ty;
370+
if contains_ref_bindings {
371+
check_expr(fcx, discrim);
372+
discrim_ty = fcx.expr_ty(discrim);
373+
} else {
374+
// ...but otherwise we want to use any supertype of the
375+
// discriminant. This is sort of a workaround, see note (*) in
376+
// `check_pat` for some details.
377+
discrim_ty = fcx.infcx().next_ty_var();
378+
check_expr_has_type(fcx, discrim, discrim_ty);
379+
};
366380

367381
// Typecheck the patterns first, so that we get types for all the
368382
// bindings.

src/librustc_typeck/check/mod.rs

+20-4
Original file line numberDiff line numberDiff line change
@@ -4242,11 +4242,27 @@ impl<'tcx> Repr<'tcx> for Expectation<'tcx> {
42424242
}
42434243

42444244
pub fn check_decl_initializer<'a,'tcx>(fcx: &FnCtxt<'a,'tcx>,
4245-
nid: ast::NodeId,
4245+
local: &'tcx ast::Local,
42464246
init: &'tcx ast::Expr)
42474247
{
4248-
let local_ty = fcx.local_ty(init.span, nid);
4249-
check_expr_coercable_to_type(fcx, init, local_ty)
4248+
let ref_bindings = fcx.tcx().pat_contains_ref_binding(&local.pat);
4249+
4250+
let local_ty = fcx.local_ty(init.span, local.id);
4251+
if !ref_bindings {
4252+
check_expr_coercable_to_type(fcx, init, local_ty)
4253+
} else {
4254+
// Somewhat subtle: if we have a `ref` binding in the pattern,
4255+
// we want to avoid introducing coercions for the RHS. This is
4256+
// both because it helps preserve sanity and, in the case of
4257+
// ref mut, for soundness (issue #23116). In particular, in
4258+
// the latter case, we need to be clear that the type of the
4259+
// referent for the reference that results is *equal to* the
4260+
// type of the lvalue it is referencing, and not some
4261+
// supertype thereof.
4262+
check_expr(fcx, init);
4263+
let init_ty = fcx.expr_ty(init);
4264+
demand::eqtype(fcx, init.span, init_ty, local_ty);
4265+
};
42504266
}
42514267

42524268
pub fn check_decl_local<'a,'tcx>(fcx: &FnCtxt<'a,'tcx>, local: &'tcx ast::Local) {
@@ -4256,7 +4272,7 @@ pub fn check_decl_local<'a,'tcx>(fcx: &FnCtxt<'a,'tcx>, local: &'tcx ast::Local)
42564272
fcx.write_ty(local.id, t);
42574273

42584274
if let Some(ref init) = local.init {
4259-
check_decl_initializer(fcx, local.id, &**init);
4275+
check_decl_initializer(fcx, local, &**init);
42604276
let init_ty = fcx.expr_ty(&**init);
42614277
if ty::type_is_error(init_ty) {
42624278
fcx.write_ty(local.id, init_ty);
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution and at
3+
// http://rust-lang.org/COPYRIGHT.
4+
//
5+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
6+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
7+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
8+
// option. This file may not be copied, modified, or distributed
9+
// except according to those terms.
10+
11+
// Check that when making a ref mut binding with type `&mut T`, the
12+
// type `T` must match precisely the type `U` of the value being
13+
// matched, and in particular cannot be some supertype of `U`. Issue
14+
// #23116. This test focuses on a `match`.
15+
16+
#![allow(dead_code)]
17+
struct S<'b>(&'b i32);
18+
impl<'b> S<'b> {
19+
fn bar<'a>(&'a mut self) -> &'a mut &'a i32 {
20+
match self.0 { ref mut x => x } //~ ERROR mismatched types
21+
}
22+
}
23+
24+
fn main() {}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution and at
3+
// http://rust-lang.org/COPYRIGHT.
4+
//
5+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
6+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
7+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
8+
// option. This file may not be copied, modified, or distributed
9+
// except according to those terms.
10+
11+
// Check that when making a ref mut binding with type `&mut T`, the
12+
// type `T` must match precisely the type `U` of the value being
13+
// matched, and in particular cannot be some supertype of `U`. Issue
14+
// #23116. This test focuses on a `let`.
15+
16+
#![allow(dead_code)]
17+
struct S<'b>(&'b i32);
18+
impl<'b> S<'b> {
19+
fn bar<'a>(&'a mut self) -> &'a mut &'a i32 {
20+
let ref mut x = self.0;
21+
x //~ ERROR mismatched types
22+
}
23+
}
24+
25+
fn main() {}

0 commit comments

Comments
 (0)