Skip to content

Commit 37c7d0e

Browse files
borsbors
committed
Auto merge of #43399 - tschottdorf:bndmode-pat-adjustments, r=nikomatsakis
default binding modes: add pat_binding_modes This PR kicks off the implementation of the [default binding modes RFC][1] by introducing the `pat_binding_modes` typeck table mentioned in the [mentoring instructions][2]. It is a WIP because I wasn't able to avoid all uses of the binding modes as not all call sites are close enough to the typeck tables. I added marker comments to any line matching `BindByRef|BindByValue` so that reviewers are aware of all of them. I will look into changing the HIR (as suggested in [2]) to not carry a `BindingMode` unless one was explicitly specified, but this PR is good for a first round of comments. The actual changes are quite small and CI will fail due to overlong lines caused by the marker comments. See #42640. cc @nikomatsakis [1]: rust-lang/rfcs#2005 [2]: #42640 (comment)
2 parents 2a6828e + 8f67f1e commit 37c7d0e

File tree

22 files changed

+311
-106
lines changed

22 files changed

+311
-106
lines changed

‎src/librustc/hir/lowering.rs

+13-9
Original file line numberDiff line numberDiff line change
@@ -2191,7 +2191,7 @@ impl<'a> LoweringContext<'a> {
21912191
let next_ident = self.str_to_ident("__next");
21922192
let next_pat = self.pat_ident_binding_mode(e.span,
21932193
next_ident,
2194-
hir::BindByValue(hir::MutMutable));
2194+
hir::BindingAnnotation::Mutable);
21952195

21962196
// `::std::option::Option::Some(val) => next = val`
21972197
let pat_arm = {
@@ -2215,8 +2215,9 @@ impl<'a> LoweringContext<'a> {
22152215
};
22162216

22172217
// `mut iter`
2218-
let iter_pat = self.pat_ident_binding_mode(e.span, iter,
2219-
hir::BindByValue(hir::MutMutable));
2218+
let iter_pat = self.pat_ident_binding_mode(e.span,
2219+
iter,
2220+
hir::BindingAnnotation::Mutable);
22202221

22212222
// `match ::std::iter::Iterator::next(&mut iter) { ... }`
22222223
let match_expr = {
@@ -2503,10 +2504,13 @@ impl<'a> LoweringContext<'a> {
25032504
}
25042505
}
25052506

2506-
fn lower_binding_mode(&mut self, b: &BindingMode) -> hir::BindingMode {
2507+
fn lower_binding_mode(&mut self, b: &BindingMode) -> hir::BindingAnnotation {
25072508
match *b {
2508-
BindingMode::ByRef(m) => hir::BindByRef(self.lower_mutability(m)),
2509-
BindingMode::ByValue(m) => hir::BindByValue(self.lower_mutability(m)),
2509+
BindingMode::ByValue(Mutability::Immutable) =>
2510+
hir::BindingAnnotation::Unannotated,
2511+
BindingMode::ByRef(Mutability::Immutable) => hir::BindingAnnotation::Ref,
2512+
BindingMode::ByValue(Mutability::Mutable) => hir::BindingAnnotation::Mutable,
2513+
BindingMode::ByRef(Mutability::Mutable) => hir::BindingAnnotation::RefMut,
25102514
}
25112515
}
25122516

@@ -2647,7 +2651,7 @@ impl<'a> LoweringContext<'a> {
26472651
fn stmt_let(&mut self, sp: Span, mutbl: bool, ident: Name, ex: P<hir::Expr>)
26482652
-> (hir::Stmt, NodeId) {
26492653
let pat = if mutbl {
2650-
self.pat_ident_binding_mode(sp, ident, hir::BindByValue(hir::MutMutable))
2654+
self.pat_ident_binding_mode(sp, ident, hir::BindingAnnotation::Mutable)
26512655
} else {
26522656
self.pat_ident(sp, ident)
26532657
};
@@ -2703,10 +2707,10 @@ impl<'a> LoweringContext<'a> {
27032707
}
27042708

27052709
fn pat_ident(&mut self, span: Span, name: Name) -> P<hir::Pat> {
2706-
self.pat_ident_binding_mode(span, name, hir::BindByValue(hir::MutImmutable))
2710+
self.pat_ident_binding_mode(span, name, hir::BindingAnnotation::Unannotated)
27072711
}
27082712

2709-
fn pat_ident_binding_mode(&mut self, span: Span, name: Name, bm: hir::BindingMode)
2713+
fn pat_ident_binding_mode(&mut self, span: Span, name: Name, bm: hir::BindingAnnotation)
27102714
-> P<hir::Pat> {
27112715
let id = self.next_id();
27122716
let parent_def = self.parent_def.unwrap();

‎src/librustc/hir/mod.rs

+22-5
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010

1111
// The Rust HIR.
1212

13-
pub use self::BindingMode::*;
1413
pub use self::BinOp_::*;
1514
pub use self::BlockCheckMode::*;
1615
pub use self::CaptureClause::*;
@@ -628,10 +627,28 @@ pub struct FieldPat {
628627
pub is_shorthand: bool,
629628
}
630629

630+
/// Explicit binding annotations given in the HIR for a binding. Note
631+
/// that this is not the final binding *mode* that we infer after type
632+
/// inference.
631633
#[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug, Copy)]
632-
pub enum BindingMode {
633-
BindByRef(Mutability),
634-
BindByValue(Mutability),
634+
pub enum BindingAnnotation {
635+
/// No binding annotation given: this means that the final binding mode
636+
/// will depend on whether we have skipped through a `&` reference
637+
/// when matching. For example, the `x` in `Some(x)` will have binding
638+
/// mode `None`; if you do `let Some(x) = &Some(22)`, it will
639+
/// ultimately be inferred to be by-reference.
640+
///
641+
/// Note that implicit reference skipping is not implemented yet (#42640).
642+
Unannotated,
643+
644+
/// Annotated with `mut x` -- could be either ref or not, similar to `None`.
645+
Mutable,
646+
647+
/// Annotated as `ref`, like `ref x`
648+
Ref,
649+
650+
/// Annotated as `ref mut x`.
651+
RefMut,
635652
}
636653

637654
#[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug)]
@@ -647,7 +664,7 @@ pub enum PatKind {
647664

648665
/// A fresh binding `ref mut binding @ OPT_SUBPATTERN`.
649666
/// The `DefId` is for the definition of the variable being bound.
650-
Binding(BindingMode, DefId, Spanned<Name>, Option<P<Pat>>),
667+
Binding(BindingAnnotation, DefId, Spanned<Name>, Option<P<Pat>>),
651668

652669
/// A struct or struct variant pattern, e.g. `Variant {x, y, ..}`.
653670
/// The `bool` is `true` in the presence of a `..`.

‎src/librustc/hir/pat_util.rs

+23-17
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ impl hir::Pat {
8787
/// Call `f` on every "binding" in a pattern, e.g., on `a` in
8888
/// `match foo() { Some(a) => (), None => () }`
8989
pub fn each_binding<F>(&self, mut f: F)
90-
where F: FnMut(hir::BindingMode, ast::NodeId, Span, &Spanned<ast::Name>),
90+
where F: FnMut(hir::BindingAnnotation, ast::NodeId, Span, &Spanned<ast::Name>),
9191
{
9292
self.walk(|p| {
9393
if let PatKind::Binding(binding_mode, _, ref pth, _) = p.node {
@@ -130,12 +130,10 @@ impl hir::Pat {
130130

131131
pub fn simple_name(&self) -> Option<ast::Name> {
132132
match self.node {
133-
PatKind::Binding(hir::BindByValue(..), _, ref path1, None) => {
134-
Some(path1.node)
135-
}
136-
_ => {
137-
None
138-
}
133+
PatKind::Binding(hir::BindingAnnotation::Unannotated, _, ref path1, None) |
134+
PatKind::Binding(hir::BindingAnnotation::Mutable, _, ref path1, None) =>
135+
Some(path1.node),
136+
_ => None,
139137
}
140138
}
141139

@@ -163,16 +161,22 @@ impl hir::Pat {
163161
}
164162

165163
/// Checks if the pattern contains any `ref` or `ref mut` bindings,
166-
/// and if yes whether its containing mutable ones or just immutables ones.
167-
pub fn contains_ref_binding(&self) -> Option<hir::Mutability> {
164+
/// and if yes whether it contains mutable or just immutables ones.
165+
///
166+
/// FIXME(tschottdorf): this is problematic as the HIR is being scraped,
167+
/// but ref bindings may be implicit after #42640.
168+
pub fn contains_explicit_ref_binding(&self) -> Option<hir::Mutability> {
168169
let mut result = None;
169-
self.each_binding(|mode, _, _, _| {
170-
if let hir::BindingMode::BindByRef(m) = mode {
171-
// Pick Mutable as maximum
172-
match result {
173-
None | Some(hir::MutImmutable) => result = Some(m),
174-
_ => (),
170+
self.each_binding(|annotation, _, _, _| {
171+
match annotation {
172+
hir::BindingAnnotation::Ref => {
173+
match result {
174+
None | Some(hir::MutImmutable) => result = Some(hir::MutImmutable),
175+
_ => (),
176+
}
175177
}
178+
hir::BindingAnnotation::RefMut => result = Some(hir::MutMutable),
179+
_ => (),
176180
}
177181
});
178182
result
@@ -182,9 +186,11 @@ impl hir::Pat {
182186
impl hir::Arm {
183187
/// Checks if the patterns for this arm contain any `ref` or `ref mut`
184188
/// bindings, and if yes whether its containing mutable ones or just immutables ones.
185-
pub fn contains_ref_binding(&self) -> Option<hir::Mutability> {
189+
pub fn contains_explicit_ref_binding(&self) -> Option<hir::Mutability> {
190+
// FIXME(tschottdorf): contains_explicit_ref_binding() must be removed
191+
// for #42640.
186192
self.pats.iter()
187-
.filter_map(|pat| pat.contains_ref_binding())
193+
.filter_map(|pat| pat.contains_explicit_ref_binding())
188194
.max_by_key(|m| match *m {
189195
hir::MutMutable => 1,
190196
hir::MutImmutable => 0,

‎src/librustc/hir/print.rs

+8-4
Original file line numberDiff line numberDiff line change
@@ -1651,12 +1651,16 @@ impl<'a> State<'a> {
16511651
PatKind::Wild => self.s.word("_")?,
16521652
PatKind::Binding(binding_mode, _, ref path1, ref sub) => {
16531653
match binding_mode {
1654-
hir::BindByRef(mutbl) => {
1654+
hir::BindingAnnotation::Ref => {
16551655
self.word_nbsp("ref")?;
1656-
self.print_mutability(mutbl)?;
1656+
self.print_mutability(hir::MutImmutable)?;
16571657
}
1658-
hir::BindByValue(hir::MutImmutable) => {}
1659-
hir::BindByValue(hir::MutMutable) => {
1658+
hir::BindingAnnotation::RefMut => {
1659+
self.word_nbsp("ref")?;
1660+
self.print_mutability(hir::MutMutable)?;
1661+
}
1662+
hir::BindingAnnotation::Unannotated => {}
1663+
hir::BindingAnnotation::Mutable => {
16601664
self.word_nbsp("mut")?;
16611665
}
16621666
}

‎src/librustc/ich/impls_hir.rs

+5-3
Original file line numberDiff line numberDiff line change
@@ -442,9 +442,11 @@ impl_stable_hash_for!(struct hir::FieldPat {
442442
is_shorthand
443443
});
444444

445-
impl_stable_hash_for!(enum hir::BindingMode {
446-
BindByRef(mutability),
447-
BindByValue(mutability)
445+
impl_stable_hash_for!(enum hir::BindingAnnotation {
446+
Unannotated,
447+
Mutable,
448+
Ref,
449+
RefMut
448450
});
449451

450452
impl_stable_hash_for!(enum hir::RangeEnd {

‎src/librustc/ich/impls_ty.rs

+2
Original file line numberDiff line numberDiff line change
@@ -616,6 +616,7 @@ for ty::TypeckTables<'tcx> {
616616
ref node_types,
617617
ref node_substs,
618618
ref adjustments,
619+
ref pat_binding_modes,
619620
ref upvar_capture_map,
620621
ref closure_tys,
621622
ref closure_kinds,
@@ -636,6 +637,7 @@ for ty::TypeckTables<'tcx> {
636637
ich::hash_stable_nodemap(hcx, hasher, node_types);
637638
ich::hash_stable_nodemap(hcx, hasher, node_substs);
638639
ich::hash_stable_nodemap(hcx, hasher, adjustments);
640+
ich::hash_stable_nodemap(hcx, hasher, pat_binding_modes);
639641
ich::hash_stable_hashmap(hcx, hasher, upvar_capture_map, |hcx, up_var_id| {
640642
let ty::UpvarId {
641643
var_id,

‎src/librustc/middle/expr_use_visitor.rs

+16-12
Original file line numberDiff line numberDiff line change
@@ -796,16 +796,19 @@ impl<'a, 'gcx, 'tcx> ExprUseVisitor<'a, 'gcx, 'tcx> {
796796
debug!("determine_pat_move_mode cmt_discr={:?} pat={:?}", cmt_discr,
797797
pat);
798798
return_if_err!(self.mc.cat_pattern(cmt_discr, pat, |cmt_pat, pat| {
799-
match pat.node {
800-
PatKind::Binding(hir::BindByRef(..), ..) =>
801-
mode.lub(BorrowingMatch),
802-
PatKind::Binding(hir::BindByValue(..), ..) => {
803-
match copy_or_move(&self.mc, self.param_env, &cmt_pat, PatBindingMove) {
804-
Copy => mode.lub(CopyingMatch),
805-
Move(..) => mode.lub(MovingMatch),
799+
if let PatKind::Binding(..) = pat.node {
800+
let bm = *self.mc.tables.pat_binding_modes.get(&pat.id)
801+
.expect("missing binding mode");
802+
match bm {
803+
ty::BindByReference(..) =>
804+
mode.lub(BorrowingMatch),
805+
ty::BindByValue(..) => {
806+
match copy_or_move(&self.mc, self.param_env, &cmt_pat, PatBindingMove) {
807+
Copy => mode.lub(CopyingMatch),
808+
Move(..) => mode.lub(MovingMatch),
809+
}
806810
}
807811
}
808-
_ => {}
809812
}
810813
}));
811814
}
@@ -818,8 +821,9 @@ impl<'a, 'gcx, 'tcx> ExprUseVisitor<'a, 'gcx, 'tcx> {
818821

819822
let ExprUseVisitor { ref mc, ref mut delegate, param_env } = *self;
820823
return_if_err!(mc.cat_pattern(cmt_discr.clone(), pat, |cmt_pat, pat| {
821-
if let PatKind::Binding(bmode, def_id, ..) = pat.node {
824+
if let PatKind::Binding(_, def_id, ..) = pat.node {
822825
debug!("binding cmt_pat={:?} pat={:?} match_mode={:?}", cmt_pat, pat, match_mode);
826+
let bm = *mc.tables.pat_binding_modes.get(&pat.id).expect("missing binding mode");
823827

824828
// pat_ty: the type of the binding being produced.
825829
let pat_ty = return_if_err!(mc.node_ty(pat.id));
@@ -832,14 +836,14 @@ impl<'a, 'gcx, 'tcx> ExprUseVisitor<'a, 'gcx, 'tcx> {
832836
}
833837

834838
// It is also a borrow or copy/move of the value being matched.
835-
match bmode {
836-
hir::BindByRef(m) => {
839+
match bm {
840+
ty::BindByReference(m) => {
837841
if let ty::TyRef(r, _) = pat_ty.sty {
838842
let bk = ty::BorrowKind::from_mutbl(m);
839843
delegate.borrow(pat.id, pat.span, cmt_pat, r, bk, RefBinding);
840844
}
841845
}
842-
hir::BindByValue(..) => {
846+
ty::BindByValue(..) => {
843847
let mode = copy_or_move(mc, param_env, &cmt_pat, PatBindingMove);
844848
debug!("walk_pat binding consuming pat");
845849
delegate.consume_pat(pat, cmt_pat, mode);

‎src/librustc/middle/mem_categorization.rs

+20-14
Original file line numberDiff line numberDiff line change
@@ -330,11 +330,12 @@ impl MutabilityCategory {
330330
ret
331331
}
332332

333-
fn from_local(tcx: TyCtxt, id: ast::NodeId) -> MutabilityCategory {
333+
fn from_local(tcx: TyCtxt, tables: &ty::TypeckTables, id: ast::NodeId) -> MutabilityCategory {
334334
let ret = match tcx.hir.get(id) {
335335
hir_map::NodeLocal(p) => match p.node {
336-
PatKind::Binding(bind_mode, ..) => {
337-
if bind_mode == hir::BindByValue(hir::MutMutable) {
336+
PatKind::Binding(..) => {
337+
let bm = *tables.pat_binding_modes.get(&p.id).expect("missing binding mode");
338+
if bm == ty::BindByValue(hir::MutMutable) {
338339
McDeclared
339340
} else {
340341
McImmutable
@@ -475,16 +476,21 @@ impl<'a, 'gcx, 'tcx> MemCategorizationContext<'a, 'gcx, 'tcx> {
475476
// *being borrowed* is. But ideally we would put in a more
476477
// fundamental fix to this conflated use of the node id.
477478
let ret_ty = match pat.node {
478-
PatKind::Binding(hir::BindByRef(_), ..) => {
479-
// a bind-by-ref means that the base_ty will be the type of the ident itself,
480-
// but what we want here is the type of the underlying value being borrowed.
481-
// So peel off one-level, turning the &T into T.
482-
match base_ty.builtin_deref(false, ty::NoPreference) {
483-
Some(t) => t.ty,
484-
None => {
485-
debug!("By-ref binding of non-derefable type {:?}", base_ty);
486-
return Err(());
479+
PatKind::Binding(..) => {
480+
let bm = *self.tables.pat_binding_modes.get(&pat.id).expect("missing binding mode");
481+
if let ty::BindByReference(_) = bm {
482+
// a bind-by-ref means that the base_ty will be the type of the ident itself,
483+
// but what we want here is the type of the underlying value being borrowed.
484+
// So peel off one-level, turning the &T into T.
485+
match base_ty.builtin_deref(false, ty::NoPreference) {
486+
Some(t) => t.ty,
487+
None => {
488+
debug!("By-ref binding of non-derefable type {:?}", base_ty);
489+
return Err(());
490+
}
487491
}
492+
} else {
493+
base_ty
488494
}
489495
}
490496
_ => base_ty,
@@ -659,7 +665,7 @@ impl<'a, 'gcx, 'tcx> MemCategorizationContext<'a, 'gcx, 'tcx> {
659665
id,
660666
span,
661667
cat: Categorization::Local(vid),
662-
mutbl: MutabilityCategory::from_local(self.tcx, vid),
668+
mutbl: MutabilityCategory::from_local(self.tcx, self.tables, vid),
663669
ty: expr_ty,
664670
note: NoteNone
665671
}))
@@ -711,7 +717,7 @@ impl<'a, 'gcx, 'tcx> MemCategorizationContext<'a, 'gcx, 'tcx> {
711717
let var_ty = self.node_ty(var_id)?;
712718

713719
// Mutability of original variable itself
714-
let var_mutbl = MutabilityCategory::from_local(self.tcx, var_id);
720+
let var_mutbl = MutabilityCategory::from_local(self.tcx, self.tables, var_id);
715721

716722
// Construct the upvar. This represents access to the field
717723
// from the environment (perhaps we should eventually desugar

‎src/librustc/middle/region.rs

+25-1
Original file line numberDiff line numberDiff line change
@@ -889,8 +889,32 @@ fn resolve_local<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>,
889889
/// | ( ..., P&, ... )
890890
/// | box P&
891891
fn is_binding_pat(pat: &hir::Pat) -> bool {
892+
// Note that the code below looks for *explicit* refs only, that is, it won't
893+
// know about *implicit* refs as introduced in #42640.
894+
//
895+
// This is not a problem. For example, consider
896+
//
897+
// let (ref x, ref y) = (Foo { .. }, Bar { .. });
898+
//
899+
// Due to the explicit refs on the left hand side, the below code would signal
900+
// that the temporary value on the right hand side should live until the end of
901+
// the enclosing block (as opposed to being dropped after the let is complete).
902+
//
903+
// To create an implicit ref, however, you must have a borrowed value on the RHS
904+
// already, as in this example (which won't compile before #42640):
905+
//
906+
// let Foo { x, .. } = &Foo { x: ..., ... };
907+
//
908+
// in place of
909+
//
910+
// let Foo { ref x, .. } = Foo { ... };
911+
//
912+
// In the former case (the implicit ref version), the temporary is created by the
913+
// & expression, and its lifetime would be extended to the end of the block (due
914+
// to a different rule, not the below code).
892915
match pat.node {
893-
PatKind::Binding(hir::BindByRef(_), ..) => true,
916+
PatKind::Binding(hir::BindingAnnotation::Ref, ..) |
917+
PatKind::Binding(hir::BindingAnnotation::RefMut, ..) => true,
894918

895919
PatKind::Struct(_, ref field_pats, _) => {
896920
field_pats.iter().any(|fp| is_binding_pat(&fp.node.pat))

0 commit comments

Comments
 (0)