Auto merge of #33929 - petrochenkov:pathir, r=eddyb

Separate bindings from other patterns in HIR

Now when name resolution is done on AST, we can avoid dumping everything that looks like an identifier into `PatKind::Ident` in HIR.
`hir::PatKind::Ident` is removed, fresh bindings are now called `hir::PatKind::Binding`, everything else goes to `hir::PatKind::Path`.

I intend to do something with `PatKind::Path`/`PatKind::QPath` as well using resolution results, but it requires some audit and maybe some deeper refactoring of relevant resolution/type checking code to do it properly.
I'm submitting this part of the patch earlier to notify interested parties that I'm working on this.

cc @jseyfried
r? @eddyb

src/librustc/cfg/construct.rs

@@ -99,7 +99,7 @@ impl<'a, 'tcx> CFGBuilder<'a, 'tcx> {
 
     fn pat(&mut self, pat: &hir::Pat, pred: CFGIndex) -> CFGIndex {
         match pat.node {
-            PatKind::Ident(_, _, None) |
+            PatKind::Binding(_, _, None) |
             PatKind::Path(..) |
             PatKind::QPath(..) |
             PatKind::Lit(..) |
@@ -110,7 +110,7 @@ impl<'a, 'tcx> CFGBuilder<'a, 'tcx> {
 
             PatKind::Box(ref subpat) |
             PatKind::Ref(ref subpat, _) |
-            PatKind::Ident(_, _, Some(ref subpat)) => {
+            PatKind::Binding(_, _, Some(ref subpat)) => {
                 let subpat_exit = self.pat(&subpat, pred);
                 self.add_ast_node(pat.id, &[subpat_exit])
             }
@@ -456,8 +456,7 @@ impl<'a, 'tcx> CFGBuilder<'a, 'tcx> {
                     // Visit the guard expression
                     let guard_exit = self.expr(&guard, guard_start);
 
-                    let this_has_bindings = pat_util::pat_contains_bindings_or_wild(
-                        &self.tcx.def_map.borrow(), &pat);
+                    let this_has_bindings = pat_util::pat_contains_bindings_or_wild(&pat);
 
                     // If both this pattern and the previous pattern
                     // were free of bindings, they must consist only

src/librustc/hir/fold.rs

@@ -914,8 +914,8 @@ pub fn noop_fold_pat<T: Folder>(p: P<Pat>, folder: &mut T) -> P<Pat> {
             id: folder.new_id(id),
             node: match node {
                 PatKind::Wild => PatKind::Wild,
-                PatKind::Ident(binding_mode, pth1, sub) => {
-                    PatKind::Ident(binding_mode,
+                PatKind::Binding(binding_mode, pth1, sub) => {
+                    PatKind::Binding(binding_mode,
                              Spanned {
                                  span: folder.new_span(pth1.span),
                                  node: folder.fold_name(pth1.node),

src/librustc/hir/intravisit.rs

@@ -478,7 +478,7 @@ pub fn walk_pat<'v, V: Visitor<'v>>(visitor: &mut V, pattern: &'v Pat) {
         PatKind::Ref(ref subpattern, _) => {
             visitor.visit_pat(subpattern)
         }
-        PatKind::Ident(_, ref pth1, ref optional_subpattern) => {
+        PatKind::Binding(_, ref pth1, ref optional_subpattern) => {
             visitor.visit_name(pth1.span, pth1.node);
             walk_list!(visitor, visit_pat, optional_subpattern);
         }

src/librustc/hir/lowering.rs

@@ -866,14 +866,16 @@ impl<'a> LoweringContext<'a> {
                 PatKind::Wild => hir::PatKind::Wild,
                 PatKind::Ident(ref binding_mode, pth1, ref sub) => {
                     self.with_parent_def(p.id, |this| {
-                        let name = match this.resolver.get_resolution(p.id).map(|d| d.full_def()) {
-                            // Only pattern bindings are renamed
-                            None | Some(Def::Local(..)) => this.lower_ident(pth1.node),
-                            _ => pth1.node.name,
-                        };
-                        hir::PatKind::Ident(this.lower_binding_mode(binding_mode),
-                                            respan(pth1.span, name),
-                                            sub.as_ref().map(|x| this.lower_pat(x)))
+                        match this.resolver.get_resolution(p.id).map(|d| d.full_def()) {
+                            // `None` can occur in body-less function signatures
+                            None | Some(Def::Local(..)) => {
+                                hir::PatKind::Binding(this.lower_binding_mode(binding_mode),
+                                                      respan(pth1.span,
+                                                             this.lower_ident(pth1.node)),
+                                                      sub.as_ref().map(|x| this.lower_pat(x)))
+                            }
+                            _ => hir::PatKind::Path(hir::Path::from_name(pth1.span, pth1.node.name))
+                        }
                     })
                 }
                 PatKind::Lit(ref e) => hir::PatKind::Lit(self.lower_expr(e)),
@@ -1868,7 +1870,7 @@ impl<'a> LoweringContext<'a> {
 
     fn pat_ident_binding_mode(&mut self, span: Span, name: Name, bm: hir::BindingMode)
                               -> P<hir::Pat> {
-        let pat_ident = hir::PatKind::Ident(bm,
+        let pat_ident = hir::PatKind::Binding(bm,
                                             Spanned {
                                                 span: span,
                                                 node: name,

src/librustc/hir/map/collector.rs

@@ -165,7 +165,7 @@ impl<'ast> Visitor<'ast> for NodeCollector<'ast> {
     }
 
     fn visit_pat(&mut self, pat: &'ast Pat) {
-        let node = if let PatKind::Ident(..) = pat.node {
+        let node = if let PatKind::Binding(..) = pat.node {
             NodeLocal(pat)
         } else {
             NodePat(pat)

src/librustc/hir/map/def_collector.rs

@@ -396,7 +396,7 @@ impl<'ast> intravisit::Visitor<'ast> for DefCollector<'ast> {
     fn visit_pat(&mut self, pat: &'ast hir::Pat) {
         let parent_def = self.parent_def;
 
-        if let hir::PatKind::Ident(_, name, _) = pat.node {
+        if let hir::PatKind::Binding(_, name, _) = pat.node {
             let def = self.create_def(pat.id, DefPathData::Binding(name.node));
             self.parent_def = Some(def);
         }

src/librustc/hir/map/mod.rs

@@ -561,7 +561,7 @@ impl<'ast> Map<'ast> {
             NodeVariant(v) => v.node.name,
             NodeLifetime(lt) => lt.name,
             NodeTyParam(tp) => tp.name,
-            NodeLocal(&Pat { node: PatKind::Ident(_,l,_), .. }) => l.node,
+            NodeLocal(&Pat { node: PatKind::Binding(_,l,_), .. }) => l.node,
             NodeStructCtor(_) => self.name(self.get_parent(id)),
             _ => bug!("no name for {}", self.node_to_string(id))
         }

src/librustc/hir/mod.rs

@@ -466,7 +466,7 @@ impl Pat {
         }
 
         match self.node {
-            PatKind::Ident(_, _, Some(ref p)) => p.walk_(it),
+            PatKind::Binding(_, _, Some(ref p)) => p.walk_(it),
             PatKind::Struct(_, ref fields, _) => {
                 fields.iter().all(|field| field.node.pat.walk_(it))
             }
@@ -484,7 +484,7 @@ impl Pat {
             PatKind::Wild |
             PatKind::Lit(_) |
             PatKind::Range(_, _) |
-            PatKind::Ident(_, _, _) |
+            PatKind::Binding(..) |
             PatKind::Path(..) |
             PatKind::QPath(_, _) => {
                 true
@@ -524,15 +524,8 @@ pub enum PatKind {
     /// Represents a wildcard pattern (`_`)
     Wild,
 
-    /// A `PatKind::Ident` may either be a new bound variable,
-    /// or a unit struct/variant pattern, or a const pattern (in the last two cases
-    /// the third field must be `None`).
-    ///
-    /// In the unit or const pattern case, the parser can't determine
-    /// which it is. The resolver determines this, and
-    /// records this pattern's `NodeId` in an auxiliary
-    /// set (of "PatIdents that refer to unit patterns or constants").
-    Ident(BindingMode, Spanned<Name>, Option<P<Pat>>),
+    /// A fresh binding `ref mut binding @ OPT_SUBPATTERN`.
+    Binding(BindingMode, Spanned<Name>, Option<P<Pat>>),
 
     /// A struct or struct variant pattern, e.g. `Variant {x, y, ..}`.
     /// The `bool` is `true` in the presence of a `..`.
@@ -1144,7 +1137,7 @@ pub type ExplicitSelf = Spanned<SelfKind>;
 
 impl Arg {
     pub fn to_self(&self) -> Option<ExplicitSelf> {
-        if let PatKind::Ident(BindByValue(mutbl), name, _) = self.pat.node {
+        if let PatKind::Binding(BindByValue(mutbl), name, _) = self.pat.node {
             if name.node.unhygienize() == keywords::SelfValue.name() {
                 return match self.ty.node {
                     TyInfer => Some(respan(self.pat.span, SelfKind::Value(mutbl))),
@@ -1160,7 +1153,7 @@ impl Arg {
     }
 
     pub fn is_self(&self) -> bool {
-        if let PatKind::Ident(_, name, _) = self.pat.node {
+        if let PatKind::Binding(_, name, _) = self.pat.node {
             name.node.unhygienize() == keywords::SelfValue.name()
         } else {
             false

src/librustc/hir/pat_util.rs

@@ -10,14 +10,12 @@
 
 use hir::def::*;
 use hir::def_id::DefId;
+use hir::{self, PatKind};
 use ty::TyCtxt;
 use util::nodemap::FnvHashMap;
-
 use syntax::ast;
-use hir::{self, PatKind};
-use syntax::codemap::{respan, Span, Spanned, DUMMY_SP};
+use syntax::codemap::{Span, Spanned, DUMMY_SP};
 
-use std::cell::RefCell;
 use std::iter::{Enumerate, ExactSizeIterator};
 
 pub type PatIdMap = FnvHashMap<ast::Name, ast::NodeId>;
@@ -57,9 +55,9 @@ impl<T: ExactSizeIterator> EnumerateAndAdjustIterator for T {
 
 // This is used because same-named variables in alternative patterns need to
 // use the NodeId of their namesake in the first pattern.
-pub fn pat_id_map(dm: &RefCell<DefMap>, pat: &hir::Pat) -> PatIdMap {
+pub fn pat_id_map(pat: &hir::Pat) -> PatIdMap {
     let mut map = FnvHashMap();
-    pat_bindings(dm, pat, |_bm, p_id, _s, path1| {
+    pat_bindings(pat, |_bm, p_id, _s, path1| {
         map.insert(path1.node, p_id);
     });
     map
@@ -70,7 +68,6 @@ pub fn pat_is_refutable(dm: &DefMap, pat: &hir::Pat) -> bool {
         PatKind::Lit(_) | PatKind::Range(_, _) | PatKind::QPath(..) => true,
         PatKind::TupleStruct(..) |
         PatKind::Path(..) |
-        PatKind::Ident(_, _, None) |
         PatKind::Struct(..) => {
             match dm.get(&pat.id).map(|d| d.full_def()) {
                 Some(Def::Variant(..)) => true,
@@ -86,7 +83,6 @@ pub fn pat_is_variant_or_struct(dm: &DefMap, pat: &hir::Pat) -> bool {
     match pat.node {
         PatKind::TupleStruct(..) |
         PatKind::Path(..) |
-        PatKind::Ident(_, _, None) |
         PatKind::Struct(..) => {
             match dm.get(&pat.id).map(|d| d.full_def()) {
                 Some(Def::Variant(..)) | Some(Def::Struct(..)) | Some(Def::TyAlias(..)) => true,
@@ -99,7 +95,7 @@ pub fn pat_is_variant_or_struct(dm: &DefMap, pat: &hir::Pat) -> bool {
 
 pub fn pat_is_const(dm: &DefMap, pat: &hir::Pat) -> bool {
     match pat.node {
-        PatKind::Ident(_, _, None) | PatKind::Path(..) | PatKind::QPath(..) => {
+        PatKind::Path(..) | PatKind::QPath(..) => {
             match dm.get(&pat.id).map(|d| d.full_def()) {
                 Some(Def::Const(..)) | Some(Def::AssociatedConst(..)) => true,
                 _ => false
@@ -113,7 +109,7 @@ pub fn pat_is_const(dm: &DefMap, pat: &hir::Pat) -> bool {
 // returned instead of a panic.
 pub fn pat_is_resolved_const(dm: &DefMap, pat: &hir::Pat) -> bool {
     match pat.node {
-        PatKind::Ident(_, _, None) | PatKind::Path(..) | PatKind::QPath(..) => {
+        PatKind::Path(..) | PatKind::QPath(..) => {
             match dm.get(&pat.id)
                     .and_then(|d| if d.depth == 0 { Some(d.base_def) }
                                   else { None } ) {
@@ -125,46 +121,25 @@ pub fn pat_is_resolved_const(dm: &DefMap, pat: &hir::Pat) -> bool {
     }
 }
 
-pub fn pat_is_binding(dm: &DefMap, pat: &hir::Pat) -> bool {
-    match pat.node {
-        PatKind::Ident(..) => {
-            !pat_is_variant_or_struct(dm, pat) &&
-            !pat_is_const(dm, pat)
-        }
-        _ => false
-    }
-}
-
-pub fn pat_is_binding_or_wild(dm: &DefMap, pat: &hir::Pat) -> bool {
-    match pat.node {
-        PatKind::Ident(..) => pat_is_binding(dm, pat),
-        PatKind::Wild => true,
-        _ => false
-    }
-}
-
-/// Call `it` on every "binding" in a pattern, e.g., on `a` in
+/// Call `f` on every "binding" in a pattern, e.g., on `a` in
 /// `match foo() { Some(a) => (), None => () }`
-pub fn pat_bindings<I>(dm: &RefCell<DefMap>, pat: &hir::Pat, mut it: I) where
-    I: FnMut(hir::BindingMode, ast::NodeId, Span, &Spanned<ast::Name>),
+pub fn pat_bindings<F>(pat: &hir::Pat, mut f: F)
+    where F: FnMut(hir::BindingMode, ast::NodeId, Span, &Spanned<ast::Name>),
 {
     pat.walk(|p| {
-        match p.node {
-          PatKind::Ident(binding_mode, ref pth, _) if pat_is_binding(&dm.borrow(), p) => {
-            it(binding_mode, p.id, p.span, &respan(pth.span, pth.node));
-          }
-          _ => {}
+        if let PatKind::Binding(binding_mode, ref pth, _) = p.node {
+            f(binding_mode, p.id, p.span, pth);
         }
         true
     });
 }
 
 /// Checks if the pattern contains any patterns that bind something to
 /// an ident, e.g. `foo`, or `Foo(foo)` or `foo @ Bar(..)`.
-pub fn pat_contains_bindings(dm: &DefMap, pat: &hir::Pat) -> bool {
+pub fn pat_contains_bindings(pat: &hir::Pat) -> bool {
     let mut contains_bindings = false;
     pat.walk(|p| {
-        if pat_is_binding(dm, p) {
+        if let PatKind::Binding(..) = p.node {
             contains_bindings = true;
             false // there's at least one binding, can short circuit now.
         } else {
@@ -176,28 +151,25 @@ pub fn pat_contains_bindings(dm: &DefMap, pat: &hir::Pat) -> bool {
 
 /// Checks if the pattern contains any `ref` or `ref mut` bindings,
 /// and if yes whether its containing mutable ones or just immutables ones.
-pub fn pat_contains_ref_binding(dm: &RefCell<DefMap>, pat: &hir::Pat) -> Option<hir::Mutability> {
+pub fn pat_contains_ref_binding(pat: &hir::Pat) -> Option<hir::Mutability> {
     let mut result = None;
-    pat_bindings(dm, pat, |mode, _, _, _| {
-        match mode {
-            hir::BindingMode::BindByRef(m) => {
-                // Pick Mutable as maximum
-                match result {
-                    None | Some(hir::MutImmutable) => result = Some(m),
-                    _ => (),
-                }
+    pat_bindings(pat, |mode, _, _, _| {
+        if let hir::BindingMode::BindByRef(m) = mode {
+            // Pick Mutable as maximum
+            match result {
+                None | Some(hir::MutImmutable) => result = Some(m),
+                _ => (),
             }
-            hir::BindingMode::BindByValue(_) => { }
         }
     });
     result
 }
 
 /// Checks if the patterns for this arm contain any `ref` or `ref mut`
 /// bindings, and if yes whether its containing mutable ones or just immutables ones.
-pub fn arm_contains_ref_binding(dm: &RefCell<DefMap>, arm: &hir::Arm) -> Option<hir::Mutability> {
+pub fn arm_contains_ref_binding(arm: &hir::Arm) -> Option<hir::Mutability> {
     arm.pats.iter()
-            .filter_map(|pat| pat_contains_ref_binding(dm, pat))
+            .filter_map(|pat| pat_contains_ref_binding(pat))
             .max_by_key(|m| match *m {
                 hir::MutMutable => 1,
                 hir::MutImmutable => 0,
@@ -206,22 +178,23 @@ pub fn arm_contains_ref_binding(dm: &RefCell<DefMap>, arm: &hir::Arm) -> Option<
 
 /// Checks if the pattern contains any patterns that bind something to
 /// an ident or wildcard, e.g. `foo`, or `Foo(_)`, `foo @ Bar(..)`,
-pub fn pat_contains_bindings_or_wild(dm: &DefMap, pat: &hir::Pat) -> bool {
+pub fn pat_contains_bindings_or_wild(pat: &hir::Pat) -> bool {
     let mut contains_bindings = false;
     pat.walk(|p| {
-        if pat_is_binding_or_wild(dm, p) {
-            contains_bindings = true;
-            false // there's at least one binding/wildcard, can short circuit now.
-        } else {
-            true
+        match p.node {
+            PatKind::Binding(..) | PatKind::Wild => {
+                contains_bindings = true;
+                false // there's at least one binding/wildcard, can short circuit now.
+            }
+            _ => true
         }
     });
     contains_bindings
 }
 
 pub fn simple_name<'a>(pat: &'a hir::Pat) -> Option<ast::Name> {
     match pat.node {
-        PatKind::Ident(hir::BindByValue(_), ref path1, None) => {
+        PatKind::Binding(hir::BindByValue(..), ref path1, None) => {
             Some(path1.node)
         }
         _ => {
@@ -241,7 +214,6 @@ pub fn necessary_variants(dm: &DefMap, pat: &hir::Pat) -> Vec<DefId> {
         match p.node {
             PatKind::TupleStruct(..) |
             PatKind::Path(..) |
-            PatKind::Ident(_, _, None) |
             PatKind::Struct(..) => {
                 match dm.get(&p.id) {
                     Some(&PathResolution { base_def: Def::Variant(_, id), .. }) => {

src/librustc/hir/print.rs

@@ -1716,7 +1716,7 @@ impl<'a> State<'a> {
         // is that it doesn't matter
         match pat.node {
             PatKind::Wild => word(&mut self.s, "_")?,
-            PatKind::Ident(binding_mode, ref path1, ref sub) => {
+            PatKind::Binding(binding_mode, ref path1, ref sub) => {
                 match binding_mode {
                     hir::BindByRef(mutbl) => {
                         self.word_nbsp("ref")?;
@@ -2170,7 +2170,7 @@ impl<'a> State<'a> {
                 if let Some(eself) = input.to_self() {
                     self.print_explicit_self(&eself)?;
                 } else {
-                    let invalid = if let PatKind::Ident(_, name, _) = input.pat.node {
+                    let invalid = if let PatKind::Binding(_, name, _) = input.pat.node {
                         name.node == keywords::Invalid.name()
                     } else {
                         false