Refactor hir::lowering::Resolver

Author: jseyfried

src/librustc/hir/lowering.rs

@@ -44,7 +44,7 @@ use hir;
 use hir::map::Definitions;
 use hir::map::definitions::DefPathData;
 use hir::def_id::{DefIndex, DefId};
-use hir::def::{Def, DefMap, PathResolution};
+use hir::def::Def;
 
 use std::collections::BTreeMap;
 use std::iter;
@@ -68,15 +68,30 @@ pub struct LoweringContext<'a> {
     // the form of a DefIndex) so that if we create a new node which introduces
     // a definition, then we can properly create the def id.
     parent_def: Cell<Option<DefIndex>>,
-    resolver: Option<RefCell<&'a mut Resolver>>,
+    resolver: RefCell<&'a mut Resolver>,
 }
 
 pub trait Resolver {
+    // Resolve a global hir path generated by the lowerer when expanding `for`, `if let`, etc.
     fn resolve_generated_global_path(&mut self, path: &hir::Path, is_value: bool) -> Def;
 
-    fn def_map(&mut self) -> &mut DefMap;
+    // Record the resolution of a path or binding generated by the lowerer when expanding.
+    fn record_resolution(&mut self, id: NodeId, def: Def);
+
     // We must keep the set of definitions up to date as we add nodes that weren't in the AST.
-    fn definitions(&mut self) -> &mut Definitions;
+    // This should only return `None` during testing.
+    fn definitions(&mut self) -> Option<&mut Definitions>;
+}
+
+pub struct DummyResolver;
+impl Resolver for DummyResolver {
+    fn resolve_generated_global_path(&mut self, _path: &hir::Path, _is_value: bool) -> Def {
+        Def::Err
+    }
+    fn record_resolution(&mut self, _id: NodeId, _def: Def) {}
+    fn definitions(&mut self) -> Option<&mut Definitions> {
+        None
+    }
 }
 
 impl<'a, 'hir> LoweringContext<'a> {
@@ -98,18 +113,7 @@ impl<'a, 'hir> LoweringContext<'a> {
             crate_root: crate_root,
             id_assigner: id_assigner,
             parent_def: Cell::new(None),
-            resolver: Some(RefCell::new(resolver)),
-        }
-    }
-
-    // Only use this when you want a LoweringContext for testing and won't look
-    // up def ids for anything created during lowering.
-    pub fn testing_context(id_assigner: &'a NodeIdAssigner) -> LoweringContext<'a> {
-        LoweringContext {
-            crate_root: None,
-            id_assigner: id_assigner,
-            parent_def: Cell::new(None),
-            resolver: None,
+            resolver: RefCell::new(resolver),
         }
     }
 
@@ -127,37 +131,17 @@ impl<'a, 'hir> LoweringContext<'a> {
     }
 
     fn with_parent_def<T, F: FnOnce() -> T>(&self, parent_id: NodeId, f: F) -> T {
-        if self.resolver.is_none() {
-            // This should only be used for testing.
-            return f();
-        }
-
         let old_def = self.parent_def.get();
-        self.parent_def.set(Some(self.get_def(parent_id)));
+        self.parent_def.set(match self.resolver.borrow_mut().definitions() {
+            Some(defs) => Some(defs.opt_def_index(parent_id).unwrap()),
+            None => old_def,
+        });
+
         let result = f();
-        self.parent_def.set(old_def);
 
+        self.parent_def.set(old_def);
         result
     }
-
-    fn get_def(&self, id: NodeId) -> DefIndex {
-        let mut resolver = self.resolver.as_ref().unwrap().borrow_mut();
-        resolver.definitions().opt_def_index(id).unwrap()
-    }
-
-    fn record_def(&self, id: NodeId, def: Def) {
-        if let Some(ref resolver) = self.resolver {
-            resolver.borrow_mut().def_map().insert(id, PathResolution { base_def: def, depth: 0 });
-        }
-    }
-
-    fn resolve_generated_global_path(&self, path: &hir::Path, is_value: bool) -> Def {
-        if let Some(ref resolver) = self.resolver {
-            resolver.borrow_mut().resolve_generated_global_path(path, is_value)
-        } else {
-            Def::Err
-        }
-    }
 }
 
 pub fn lower_ident(_lctx: &LoweringContext, ident: Ident) -> hir::Ident {
@@ -1765,10 +1749,12 @@ fn expr_call(lctx: &LoweringContext,
 fn expr_ident(lctx: &LoweringContext, span: Span, id: hir::Ident,
               attrs: ThinAttributes, binding: NodeId) -> P<hir::Expr> {
     let expr = expr(lctx, span, hir::ExprPath(None, path_ident(span, id)), attrs);
-    if let Some(ref resolver) = lctx.resolver {
-        let def_id = resolver.borrow_mut().definitions().local_def_id(binding);
-        lctx.record_def(expr.id, Def::Local(def_id, binding));
-    }
+
+    let mut resolver = lctx.resolver.borrow_mut();
+    let def = resolver.definitions().map(|defs| Def::Local(defs.local_def_id(binding), binding))
+                                    .unwrap_or(Def::Err);
+    resolver.record_resolution(expr.id, def);
+
     expr
 }
 
@@ -1779,9 +1765,9 @@ fn expr_mut_addr_of(lctx: &LoweringContext, span: Span, e: P<hir::Expr>,
 
 fn expr_path(lctx: &LoweringContext, path: hir::Path,
              attrs: ThinAttributes) -> P<hir::Expr> {
-    let def = lctx.resolve_generated_global_path(&path, true);
+    let def = lctx.resolver.borrow_mut().resolve_generated_global_path(&path, true);
     let expr = expr(lctx, path.span, hir::ExprPath(None, path), attrs);
-    lctx.record_def(expr.id, def);
+    lctx.resolver.borrow_mut().record_resolution(expr.id, def);
     expr
 }
 
@@ -1811,9 +1797,9 @@ fn expr_struct(lctx: &LoweringContext,
                fields: hir::HirVec<hir::Field>,
                e: Option<P<hir::Expr>>,
                attrs: ThinAttributes) -> P<hir::Expr> {
-    let def = lctx.resolve_generated_global_path(&path, false);
+    let def = lctx.resolver.borrow_mut().resolve_generated_global_path(&path, false);
     let expr = expr(lctx, sp, hir::ExprStruct(path, fields, e), attrs);
-    lctx.record_def(expr.id, def);
+    lctx.resolver.borrow_mut().record_resolution(expr.id, def);
     expr
 
 }
@@ -1900,14 +1886,14 @@ fn pat_enum(lctx: &LoweringContext,
             path: hir::Path,
             subpats: hir::HirVec<P<hir::Pat>>)
             -> P<hir::Pat> {
-    let def = lctx.resolve_generated_global_path(&path, true);
+    let def = lctx.resolver.borrow_mut().resolve_generated_global_path(&path, true);
     let pt = if subpats.is_empty() {
         hir::PatKind::Path(path)
     } else {
         hir::PatKind::TupleStruct(path, Some(subpats))
     };
     let pat = pat(lctx, span, pt);
-    lctx.record_def(pat.id, def);
+    lctx.resolver.borrow_mut().record_resolution(pat.id, def);
     pat
 }
 
@@ -1929,14 +1915,13 @@ fn pat_ident_binding_mode(lctx: &LoweringContext,
 
     let pat = pat(lctx, span, pat_ident);
 
-    if let Some(ref resolver) = lctx.resolver {
-        let def_index =
-            resolver.borrow_mut().definitions()
-                                 .create_def_with_parent(lctx.parent_def.get(),
-                                                         pat.id,
-                                                         DefPathData::Binding(ident.name));
-        lctx.record_def(pat.id, Def::Local(DefId::local(def_index), pat.id));
-    }
+    let mut resolver = lctx.resolver.borrow_mut();
+    let def = resolver.definitions().map(|defs| {
+        let def_path_data = DefPathData::Binding(ident.name);
+        let def_index = defs.create_def_with_parent(lctx.parent_def.get(), pat.id, def_path_data);
+        Def::Local(DefId::local(def_index), pat.id)
+    }).unwrap_or(Def::Err);
+    resolver.record_resolution(pat.id, def);
 
     pat
 }

src/librustc_metadata/astencode.rs

@@ -57,7 +57,7 @@ use rustc_serialize::{Encodable, EncoderHelpers};
 #[cfg(test)] use syntax::parse;
 #[cfg(test)] use syntax::ast::NodeId;
 #[cfg(test)] use rustc::hir::print as pprust;
-#[cfg(test)] use rustc::hir::lowering::{lower_item, LoweringContext};
+#[cfg(test)] use rustc::hir::lowering::{lower_item, LoweringContext, DummyResolver};
 
 struct DecodeContext<'a, 'b, 'tcx: 'a> {
     tcx: &'a TyCtxt<'tcx>,
@@ -1325,6 +1325,14 @@ fn mk_ctxt() -> parse::ParseSess {
     parse::ParseSess::new()
 }
 
+#[cfg(test)]
+fn with_testing_context<T, F: FnOnce(LoweringContext) -> T>(f: F) -> T {
+    let assigner = FakeNodeIdAssigner;
+    let mut resolver = DummyResolver;
+    let lcx = LoweringContext::new(&assigner, None, &mut resolver);
+    f(lcx)
+}
+
 #[cfg(test)]
 fn roundtrip(in_item: hir::Item) {
     let mut wr = Cursor::new(Vec::new());
@@ -1338,34 +1346,34 @@ fn roundtrip(in_item: hir::Item) {
 #[test]
 fn test_basic() {
     let cx = mk_ctxt();
-    let fnia = FakeNodeIdAssigner;
-    let lcx = LoweringContext::testing_context(&fnia);
-    roundtrip(lower_item(&lcx, &quote_item!(&cx,
-        fn foo() {}
-    ).unwrap()));
+    with_testing_context(|lcx| {
+        roundtrip(lower_item(&lcx, &quote_item!(&cx,
+            fn foo() {}
+        ).unwrap()));
+    });
 }
 
 #[test]
 fn test_smalltalk() {
     let cx = mk_ctxt();
-    let fnia = FakeNodeIdAssigner;
-    let lcx = LoweringContext::testing_context(&fnia);
-    roundtrip(lower_item(&lcx, &quote_item!(&cx,
-        fn foo() -> isize { 3 + 4 } // first smalltalk program ever executed.
-    ).unwrap()));
+    with_testing_context(|lcx| {
+        roundtrip(lower_item(&lcx, &quote_item!(&cx,
+            fn foo() -> isize { 3 + 4 } // first smalltalk program ever executed.
+        ).unwrap()));
+    });
 }
 
 #[test]
 fn test_more() {
     let cx = mk_ctxt();
-    let fnia = FakeNodeIdAssigner;
-    let lcx = LoweringContext::testing_context(&fnia);
-    roundtrip(lower_item(&lcx, &quote_item!(&cx,
-        fn foo(x: usize, y: usize) -> usize {
-            let z = x + y;
-            return z;
-        }
-    ).unwrap()));
+    with_testing_context(|lcx| {
+        roundtrip(lower_item(&lcx, &quote_item!(&cx,
+            fn foo(x: usize, y: usize) -> usize {
+                let z = x + y;
+                return z;
+            }
+        ).unwrap()));
+    });
 }
 
 #[test]
@@ -1377,21 +1385,22 @@ fn test_simplification() {
             return alist {eq_fn: eq_int, data: Vec::new()};
         }
     ).unwrap();
-    let fnia = FakeNodeIdAssigner;
-    let lcx = LoweringContext::testing_context(&fnia);
-    let hir_item = lower_item(&lcx, &item);
-    let item_in = InlinedItemRef::Item(&hir_item);
-    let item_out = simplify_ast(item_in);
-    let item_exp = InlinedItem::Item(P(lower_item(&lcx, &quote_item!(&cx,
-        fn new_int_alist<B>() -> alist<isize, B> {
-            return alist {eq_fn: eq_int, data: Vec::new()};
+    let cx = mk_ctxt();
+    with_testing_context(|lcx| {
+        let hir_item = lower_item(&lcx, &item);
+        let item_in = InlinedItemRef::Item(&hir_item);
+        let item_out = simplify_ast(item_in);
+        let item_exp = InlinedItem::Item(P(lower_item(&lcx, &quote_item!(&cx,
+            fn new_int_alist<B>() -> alist<isize, B> {
+                return alist {eq_fn: eq_int, data: Vec::new()};
+            }
+        ).unwrap())));
+        match (item_out, item_exp) {
+            (InlinedItem::Item(item_out), InlinedItem::Item(item_exp)) => {
+                 assert!(pprust::item_to_string(&item_out) ==
+                         pprust::item_to_string(&item_exp));
+            }
+            _ => bug!()
         }
-    ).unwrap())));
-    match (item_out, item_exp) {
-      (InlinedItem::Item(item_out), InlinedItem::Item(item_exp)) => {
-        assert!(pprust::item_to_string(&item_out) ==
-                pprust::item_to_string(&item_exp));
-      }
-      _ => bug!()
-    }
+    });
 }

src/librustc_resolve/lib.rs

@@ -1098,11 +1098,12 @@ impl<'a> hir::lowering::Resolver for Resolver<'a> {
         }
     }
 
-    fn def_map(&mut self) -> &mut DefMap {
-        &mut self.def_map
+    fn record_resolution(&mut self, id: NodeId, def: Def) {
+        self.def_map.insert(id, PathResolution { base_def: def, depth: 0 });
     }
-    fn definitions(&mut self) -> &mut Definitions {
-        self.definitions
+
+    fn definitions(&mut self) -> Option<&mut Definitions> {
+        Some(self.definitions)
     }
 }