Auto merge of #34095 - petrochenkov:pathir2, r=jseyfried

Improvements to pattern resolution + some refactoring

Continuation of #33929
First commit is a careful rewrite of `resolve_pattern`, pattern path resolution and new binding creation logic is factored out in separate functions, some minor bugs are fixed. Also, `resolve_possibly_assoc_item` doesn't swallow modules now.
Later commits are refactorings, see the comment descriptions.

I intend to continue this work later with better support for `Def::Err` in patterns in post-resolve stages and cleanup of pattern resolution code in type checker.

Fixes #32086
Fixes #34047 ([breaking-change])
Fixes #34074

cc @jseyfried
r? @eddyb

src/librustc/cfg/construct.rs

@@ -574,8 +574,8 @@ impl<'a, 'tcx> CFGBuilder<'a, 'tcx> {
             return *self.loop_scopes.last().unwrap();
         }
 
-        match self.tcx.def_map.borrow().get(&expr.id).map(|d| d.full_def()) {
-            Some(Def::Label(loop_id)) => {
+        match self.tcx.expect_def(expr.id) {
+            Def::Label(loop_id) => {
                 for l in &self.loop_scopes {
                     if l.loop_id == loop_id {
                         return *l;

src/librustc/hir/def.rs

@@ -67,6 +67,10 @@ pub struct PathResolution {
 }
 
 impl PathResolution {
+    pub fn new(def: Def) -> PathResolution {
+        PathResolution { base_def: def, depth: 0 }
+    }
+
     /// Get the definition, if fully resolved, otherwise panic.
     pub fn full_def(&self) -> Def {
         if self.depth != 0 {
@@ -75,17 +79,11 @@ impl PathResolution {
         self.base_def
     }
 
-    /// Get the DefId, if fully resolved, otherwise panic.
-    pub fn def_id(&self) -> DefId {
-        self.full_def().def_id()
-    }
-
-    pub fn new(base_def: Def,
-               depth: usize)
-               -> PathResolution {
-        PathResolution {
-            base_def: base_def,
-            depth: depth,
+    pub fn kind_name(&self) -> &'static str {
+        if self.depth != 0 {
+            "associated item"
+        } else {
+            self.base_def.kind_name()
         }
     }
 }
@@ -161,8 +159,8 @@ impl Def {
             Def::Struct(..) => "struct",
             Def::Trait(..) => "trait",
             Def::Method(..) => "method",
-            Def::Const(..) => "const",
-            Def::AssociatedConst(..) => "associated const",
+            Def::Const(..) => "constant",
+            Def::AssociatedConst(..) => "associated constant",
             Def::TyParam(..) => "type parameter",
             Def::PrimTy(..) => "builtin type",
             Def::Local(..) => "local variable",

src/librustc/hir/lowering.rs

@@ -866,7 +866,7 @@ impl<'a> LoweringContext<'a> {
                 PatKind::Wild => hir::PatKind::Wild,
                 PatKind::Ident(ref binding_mode, pth1, ref sub) => {
                     self.with_parent_def(p.id, |this| {
-                        match this.resolver.get_resolution(p.id).map(|d| d.full_def()) {
+                        match this.resolver.get_resolution(p.id).map(|d| d.base_def) {
                             // `None` can occur in body-less function signatures
                             None | Some(Def::Local(..)) => {
                                 hir::PatKind::Binding(this.lower_binding_mode(binding_mode),
@@ -1238,14 +1238,10 @@ impl<'a> LoweringContext<'a> {
                             position: position,
                         }
                     });
-                    let rename = if path.segments.len() == 1 {
-                        // Only local variables are renamed
-                        match self.resolver.get_resolution(e.id).map(|d| d.full_def()) {
-                            Some(Def::Local(..)) | Some(Def::Upvar(..)) => true,
-                            _ => false,
-                        }
-                    } else {
-                        false
+                    // Only local variables are renamed
+                    let rename = match self.resolver.get_resolution(e.id).map(|d| d.base_def) {
+                        Some(Def::Local(..)) | Some(Def::Upvar(..)) => true,
+                        _ => false,
                     };
                     hir::ExprPath(hir_qself, self.lower_path_full(path, rename))
                 }

src/librustc/hir/pat_util.rs

@@ -53,16 +53,6 @@ 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(pat: &hir::Pat) -> PatIdMap {
-    let mut map = FnvHashMap();
-    pat_bindings(pat, |_bm, p_id, _s, path1| {
-        map.insert(path1.node, p_id);
-    });
-    map
-}
-
 pub fn pat_is_refutable(dm: &DefMap, pat: &hir::Pat) -> bool {
     match pat.node {
         PatKind::Lit(_) | PatKind::Range(_, _) | PatKind::QPath(..) => true,

src/librustc/infer/error_reporting.rs

@@ -1357,17 +1357,7 @@ impl<'a, 'gcx, 'tcx> Rebuilder<'a, 'gcx, 'tcx> {
                     ty_queue.push(&mut_ty.ty);
                 }
                 hir::TyPath(ref maybe_qself, ref path) => {
-                    let a_def = match self.tcx.def_map.borrow().get(&cur_ty.id) {
-                        None => {
-                            self.tcx
-                                .sess
-                                .fatal(&format!(
-                                        "unbound path {}",
-                                        pprust::path_to_string(path)))
-                        }
-                        Some(d) => d.full_def()
-                    };
-                    match a_def {
+                    match self.tcx.expect_def(cur_ty.id) {
                         Def::Enum(did) | Def::TyAlias(did) | Def::Struct(did) => {
                             let generics = self.tcx.lookup_item_type(did).generics;
 

src/librustc/middle/astconv_util.rs

@@ -65,13 +65,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
     /// to it.
     pub fn ast_ty_to_prim_ty(self, ast_ty: &ast::Ty) -> Option<Ty<'tcx>> {
         if let ast::TyPath(None, ref path) = ast_ty.node {
-            let def = match self.def_map.borrow().get(&ast_ty.id) {
-                None => {
-                    span_bug!(ast_ty.span, "unbound path {:?}", path)
-                }
-                Some(d) => d.full_def()
-            };
-            if let Def::PrimTy(nty) = def {
+            if let Def::PrimTy(nty) = self.expect_def(ast_ty.id) {
                 Some(self.prim_ty_to_ty(&path.segments, nty))
             } else {
                 None

src/librustc/middle/dead.rs

@@ -84,36 +84,35 @@ impl<'a, 'tcx> MarkSymbolVisitor<'a, 'tcx> {
         }
     }
 
-    fn lookup_and_handle_definition(&mut self, id: &ast::NodeId) {
+    fn lookup_and_handle_definition(&mut self, id: ast::NodeId) {
         use ty::TypeVariants::{TyEnum, TyStruct};
 
         // If `bar` is a trait item, make sure to mark Foo as alive in `Foo::bar`
-        self.tcx.tables.borrow().item_substs.get(id)
+        self.tcx.tables.borrow().item_substs.get(&id)
             .and_then(|substs| substs.substs.self_ty())
             .map(|ty| match ty.sty {
                 TyEnum(tyid, _) | TyStruct(tyid, _) => self.check_def_id(tyid.did),
                 _ => (),
             });
 
-        self.tcx.def_map.borrow().get(id).map(|def| {
-            match def.full_def() {
-                Def::Const(_) | Def::AssociatedConst(..) => {
-                    self.check_def_id(def.def_id());
-                }
-                _ if self.ignore_non_const_paths => (),
-                Def::PrimTy(_) => (),
-                Def::SelfTy(..) => (),
-                Def::Variant(enum_id, variant_id) => {
-                    self.check_def_id(enum_id);
-                    if !self.ignore_variant_stack.contains(&variant_id) {
-                        self.check_def_id(variant_id);
-                    }
-                }
-                _ => {
-                    self.check_def_id(def.def_id());
+        let def = self.tcx.expect_def(id);
+        match def {
+            Def::Const(_) | Def::AssociatedConst(..) => {
+                self.check_def_id(def.def_id());
+            }
+            _ if self.ignore_non_const_paths => (),
+            Def::PrimTy(_) => (),
+            Def::SelfTy(..) => (),
+            Def::Variant(enum_id, variant_id) => {
+                self.check_def_id(enum_id);
+                if !self.ignore_variant_stack.contains(&variant_id) {
+                    self.check_def_id(variant_id);
                 }
             }
-        });
+            _ => {
+                self.check_def_id(def.def_id());
+            }
+        }
     }
 
     fn lookup_and_handle_method(&mut self, id: ast::NodeId) {
@@ -138,10 +137,10 @@ impl<'a, 'tcx> MarkSymbolVisitor<'a, 'tcx> {
 
     fn handle_field_pattern_match(&mut self, lhs: &hir::Pat,
                                   pats: &[codemap::Spanned<hir::FieldPat>]) {
-        let def = self.tcx.def_map.borrow().get(&lhs.id).unwrap().full_def();
-        let pat_ty = self.tcx.node_id_to_type(lhs.id);
-        let variant = match pat_ty.sty {
-            ty::TyStruct(adt, _) | ty::TyEnum(adt, _) => adt.variant_of_def(def),
+        let variant = match self.tcx.node_id_to_type(lhs.id).sty {
+            ty::TyStruct(adt, _) | ty::TyEnum(adt, _) => {
+                adt.variant_of_def(self.tcx.expect_def(lhs.id))
+            }
             _ => span_bug!(lhs.span, "non-ADT in struct pattern")
         };
         for pat in pats {
@@ -272,7 +271,7 @@ impl<'a, 'tcx, 'v> Visitor<'v> for MarkSymbolVisitor<'a, 'tcx> {
             }
             _ if pat_util::pat_is_const(&def_map.borrow(), pat) => {
                 // it might be the only use of a const
-                self.lookup_and_handle_definition(&pat.id)
+                self.lookup_and_handle_definition(pat.id)
             }
             _ => ()
         }
@@ -283,12 +282,12 @@ impl<'a, 'tcx, 'v> Visitor<'v> for MarkSymbolVisitor<'a, 'tcx> {
     }
 
     fn visit_path(&mut self, path: &hir::Path, id: ast::NodeId) {
-        self.lookup_and_handle_definition(&id);
+        self.lookup_and_handle_definition(id);
         intravisit::walk_path(self, path);
     }
 
     fn visit_path_list_item(&mut self, path: &hir::Path, item: &hir::PathListItem) {
-        self.lookup_and_handle_definition(&item.node.id());
+        self.lookup_and_handle_definition(item.node.id());
         intravisit::walk_path_list_item(self, path, item);
     }
 }

src/librustc/middle/effect.rs

@@ -172,7 +172,7 @@ impl<'a, 'tcx, 'v> Visitor<'v> for EffectCheckVisitor<'a, 'tcx> {
                 self.require_unsafe(expr.span, "use of inline assembly");
             }
             hir::ExprPath(..) => {
-                if let Def::Static(_, true) = self.tcx.resolve_expr(expr) {
+                if let Def::Static(_, true) = self.tcx.expect_def(expr.id) {
                     self.require_unsafe(expr.span, "use of mutable static");
                 }
             }

src/librustc/middle/expr_use_visitor.rs

@@ -955,9 +955,9 @@ impl<'a, 'gcx, 'tcx> ExprUseVisitor<'a, 'gcx, 'tcx> {
         debug!("walk_pat cmt_discr={:?} pat={:?}", cmt_discr,
                pat);
 
+        let tcx = &self.tcx();
         let mc = &self.mc;
         let infcx = self.mc.infcx;
-        let def_map = &self.tcx().def_map;
         let delegate = &mut self.delegate;
         return_if_err!(mc.cat_pattern(cmt_discr.clone(), pat, |mc, cmt_pat, pat| {
             match pat.node {
@@ -972,8 +972,8 @@ impl<'a, 'gcx, 'tcx> ExprUseVisitor<'a, 'gcx, 'tcx> {
 
                     // Each match binding is effectively an assignment to the
                     // binding being produced.
-                    let def = def_map.borrow().get(&pat.id).unwrap().full_def();
-                    if let Ok(binding_cmt) = mc.cat_def(pat.id, pat.span, pat_ty, def) {
+                    if let Ok(binding_cmt) = mc.cat_def(pat.id, pat.span, pat_ty,
+                                                        tcx.expect_def(pat.id)) {
                         delegate.mutate(pat.id, pat.span, binding_cmt, MutateMode::Init);
                     }
 
@@ -1002,14 +1002,11 @@ impl<'a, 'gcx, 'tcx> ExprUseVisitor<'a, 'gcx, 'tcx> {
         // to the above loop's visit of than the bindings that form
         // the leaves of the pattern tree structure.
         return_if_err!(mc.cat_pattern(cmt_discr, pat, |mc, cmt_pat, pat| {
-            let def_map = def_map.borrow();
-            let tcx = infcx.tcx;
-
             match pat.node {
                 PatKind::Struct(..) | PatKind::TupleStruct(..) |
                 PatKind::Path(..) | PatKind::QPath(..) => {
-                    match def_map.get(&pat.id).map(|d| d.full_def()) {
-                        Some(Def::Variant(enum_did, variant_did)) => {
+                    match tcx.expect_def(pat.id) {
+                        Def::Variant(enum_did, variant_did) => {
                             let downcast_cmt =
                                 if tcx.lookup_adt_def(enum_did).is_univariant() {
                                     cmt_pat
@@ -1025,7 +1022,7 @@ impl<'a, 'gcx, 'tcx> ExprUseVisitor<'a, 'gcx, 'tcx> {
                             delegate.matched_pat(pat, downcast_cmt, match_mode);
                         }
 
-                        Some(Def::Struct(..)) | Some(Def::TyAlias(..)) => {
+                        Def::Struct(..) | Def::TyAlias(..) => {
                             // A struct (in either the value or type
                             // namespace; we encounter the former on
                             // e.g. patterns for unit structs).
@@ -1037,8 +1034,7 @@ impl<'a, 'gcx, 'tcx> ExprUseVisitor<'a, 'gcx, 'tcx> {
                             delegate.matched_pat(pat, cmt_pat, match_mode);
                         }
 
-                        Some(Def::Const(..)) |
-                        Some(Def::AssociatedConst(..)) => {
+                        Def::Const(..) | Def::AssociatedConst(..) => {
                             // This is a leaf (i.e. identifier binding
                             // or constant value to match); thus no
                             // `matched_pat` call.

src/librustc/middle/intrinsicck.rs

@@ -156,7 +156,7 @@ impl<'a, 'tcx, 'v> Visitor<'v> for ItemVisitor<'a, 'tcx> {
 impl<'a, 'gcx, 'tcx, 'v> Visitor<'v> for ExprVisitor<'a, 'gcx, 'tcx> {
     fn visit_expr(&mut self, expr: &hir::Expr) {
         if let hir::ExprPath(..) = expr.node {
-            match self.infcx.tcx.resolve_expr(expr) {
+            match self.infcx.tcx.expect_def(expr.id) {
                 Def::Fn(did) if self.def_id_is_transmute(did) => {
                     let typ = self.infcx.tcx.node_id_to_type(expr.id);
                     match typ.sty {

src/librustc/middle/liveness.rs

@@ -445,7 +445,7 @@ fn visit_expr(ir: &mut IrMaps, expr: &Expr) {
     match expr.node {
       // live nodes required for uses or definitions of variables:
       hir::ExprPath(..) => {
-        let def = ir.tcx.def_map.borrow().get(&expr.id).unwrap().full_def();
+        let def = ir.tcx.expect_def(expr.id);
         debug!("expr {}: path that leads to {:?}", expr.id, def);
         if let Def::Local(..) = def {
             ir.add_live_node_for_node(expr.id, ExprNode(expr.span));
@@ -695,8 +695,8 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> {
             Some(_) => {
                 // Refers to a labeled loop. Use the results of resolve
                 // to find with one
-                match self.ir.tcx.def_map.borrow().get(&id).map(|d| d.full_def()) {
-                    Some(Def::Label(loop_id)) => loop_id,
+                match self.ir.tcx.expect_def(id) {
+                    Def::Label(loop_id) => loop_id,
                     _ => span_bug!(sp, "label on break/loop \
                                         doesn't refer to a loop")
                 }
@@ -1269,7 +1269,7 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> {
 
     fn access_path(&mut self, expr: &Expr, succ: LiveNode, acc: u32)
                    -> LiveNode {
-        match self.ir.tcx.def_map.borrow().get(&expr.id).unwrap().full_def() {
+        match self.ir.tcx.expect_def(expr.id) {
           Def::Local(_, nid) => {
             let ln = self.live_node(expr.id, expr.span);
             if acc != 0 {
@@ -1534,9 +1534,7 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> {
     fn check_lvalue(&mut self, expr: &Expr) {
         match expr.node {
             hir::ExprPath(..) => {
-                if let Def::Local(_, nid) = self.ir.tcx.def_map.borrow().get(&expr.id)
-                                                                      .unwrap()
-                                                                      .full_def() {
+                if let Def::Local(_, nid) = self.ir.tcx.expect_def(expr.id) {
                     // Assignment to an immutable variable or argument: only legal
                     // if there is no later assignment. If this local is actually
                     // mutable, then check for a reassignment to flag the mutability

src/librustc/middle/mem_categorization.rs

@@ -517,8 +517,7 @@ impl<'a, 'gcx, 'tcx> MemCategorizationContext<'a, 'gcx, 'tcx> {
           }
 
           hir::ExprPath(..) => {
-            let def = self.tcx().def_map.borrow().get(&expr.id).unwrap().full_def();
-            self.cat_def(expr.id, expr.span, expr_ty, def)
+            self.cat_def(expr.id, expr.span, expr_ty, self.tcx().expect_def(expr.id))
           }
 
           hir::ExprType(ref e, _) => {
@@ -1106,18 +1105,10 @@ impl<'a, 'gcx, 'tcx> MemCategorizationContext<'a, 'gcx, 'tcx> {
 
         (*op)(self, cmt.clone(), pat);
 
-        let opt_def = if let Some(path_res) = self.tcx().def_map.borrow().get(&pat.id) {
-            if path_res.depth != 0 || path_res.base_def == Def::Err {
-                // Since patterns can be associated constants
-                // which are resolved during typeck, we might have
-                // some unresolved patterns reaching this stage
-                // without aborting
-                return Err(());
-            }
-            Some(path_res.full_def())
-        } else {
-            None
-        };
+        let opt_def = self.tcx().expect_def_or_none(pat.id);
+        if opt_def == Some(Def::Err) {
+            return Err(());
+        }
 
         // Note: This goes up here (rather than within the PatKind::TupleStruct arm
         // alone) because struct patterns can refer to struct types or

src/librustc/middle/reachable.rs

@@ -92,13 +92,7 @@ impl<'a, 'tcx, 'v> Visitor<'v> for ReachableContext<'a, 'tcx> {
     fn visit_expr(&mut self, expr: &hir::Expr) {
         match expr.node {
             hir::ExprPath(..) => {
-                let def = match self.tcx.def_map.borrow().get(&expr.id) {
-                    Some(d) => d.full_def(),
-                    None => {
-                        span_bug!(expr.span, "def ID not in def map?!")
-                    }
-                };
-
+                let def = self.tcx.expect_def(expr.id);
                 let def_id = def.def_id();
                 if let Some(node_id) = self.tcx.map.as_local_node_id(def_id) {
                     if self.def_id_represents_local_inlined_item(def_id) {