Auto merge of #32293 - nikomatsakis:incr-comp-def-path-munging, r=eddyb

Revamp symbol names for impls (and make them deterministic, etc)

This builds on @michaelwoerister's epic PR #31539 (note that his PR never landed, so I just incorporated it into this one). The main change here is that we remove the "name" from `DefPathData` for impls, since that name is synthetic and not sufficiently predictable for incr comp. However, just doing that would cause bad symbol names since those are based on the `DefPath`. Therefore, I introduce a new mechanism for getting symbol names (and also paths for user display) called `item_path`. This is kind of simplistic for now (based on strings) but I expect to expand it later to support richer types, hopefully generating C++-mangled names that gdb etc can understand. Along the way I cleaned up how we track the path that leads to an extern crate.

There is still some cleanup left undone here. Notably, I didn't remove the impl names altogether -- that would probably make sense. I also didn't try to remove the `item_symbols` vector. Mostly I want to unblock my other incr. comp. work. =)

r? @eddyb
cc @eddyb @alexcrichton @michaelwoerister

mk/tests.mk

@@ -383,7 +383,7 @@ $(3)/stage$(1)/test/$(4)test-$(2)$$(X_$(2)): \
 	@$$(call E, rustc: $$@)
 	$(Q)CFG_LLVM_LINKAGE_FILE=$$(LLVM_LINKAGE_PATH_$(2)) \
 	    $$(subst @,,$$(STAGE$(1)_T_$(2)_H_$(3))) -o $$@ $$< --test \
-		-L "$$(RT_OUTPUT_DIR_$(2))" \
+		-Cmetadata="test-crate" -L "$$(RT_OUTPUT_DIR_$(2))" \
 		$$(LLVM_LIBDIR_RUSTFLAGS_$(2)) \
 		$$(RUSTFLAGS_$(4))
 

src/compiletest/common.rs

@@ -69,10 +69,10 @@ impl fmt::Display for Mode {
 #[derive(Clone)]
 pub struct Config {
     // The library paths required for running the compiler
-    pub compile_lib_path: String,
+    pub compile_lib_path: PathBuf,
 
     // The library paths required for running compiled programs
-    pub run_lib_path: String,
+    pub run_lib_path: PathBuf,
 
     // The rustc executable
     pub rustc_path: PathBuf,

src/compiletest/compiletest.rs

@@ -117,9 +117,17 @@ pub fn parse_config(args: Vec<String> ) -> Config {
         }
     }
 
+    fn make_absolute(path: PathBuf) -> PathBuf {
+        if path.is_relative() {
+            env::current_dir().unwrap().join(path)
+        } else {
+            path
+        }
+    }
+
     Config {
-        compile_lib_path: matches.opt_str("compile-lib-path").unwrap(),
-        run_lib_path: matches.opt_str("run-lib-path").unwrap(),
+        compile_lib_path: make_absolute(opt_path(matches, "compile-lib-path")),
+        run_lib_path: make_absolute(opt_path(matches, "run-lib-path")),
         rustc_path: opt_path(matches, "rustc-path"),
         rustdoc_path: opt_path(matches, "rustdoc-path"),
         python: matches.opt_str("python").unwrap(),

src/compiletest/runtest.rs

@@ -316,7 +316,7 @@ fn run_pretty_test_revision(config: &Config,
                                      testpaths,
                                      pretty_type.to_owned()),
                         props.exec_env.clone(),
-                        &config.compile_lib_path,
+                        config.compile_lib_path.to_str().unwrap(),
                         Some(aux_dir.to_str().unwrap()),
                         Some(src))
     }
@@ -635,7 +635,7 @@ fn run_debuginfo_gdb_test(config: &Config, props: &TestProps, testpaths: &TestPa
                                                   testpaths,
                                                   proc_args,
                                                   environment,
-                                                  &config.run_lib_path,
+                                                  config.run_lib_path.to_str().unwrap(),
                                                   None,
                                                   None);
         }
@@ -1315,7 +1315,7 @@ fn exec_compiled_test(config: &Config, props: &TestProps,
                             testpaths,
                             make_run_args(config, props, testpaths),
                             env,
-                            &config.run_lib_path,
+                            config.run_lib_path.to_str().unwrap(),
                             Some(aux_dir.to_str().unwrap()),
                             None)
         }
@@ -1387,7 +1387,7 @@ fn compose_and_run_compiler(config: &Config, props: &TestProps,
                                      &aux_testpaths,
                                      aux_args,
                                      Vec::new(),
-                                     &config.compile_lib_path,
+                                     config.compile_lib_path.to_str().unwrap(),
                                      Some(aux_dir.to_str().unwrap()),
                                      None);
         if !auxres.status.success() {
@@ -1410,7 +1410,7 @@ fn compose_and_run_compiler(config: &Config, props: &TestProps,
                     testpaths,
                     args,
                     props.rustc_env.clone(),
-                    &config.compile_lib_path,
+                    config.compile_lib_path.to_str().unwrap(),
                     Some(aux_dir.to_str().unwrap()),
                     input)
 }

src/librbml/lib.rs

@@ -165,15 +165,15 @@ impl<'doc> Doc<'doc> {
         }
     }
 
-    pub fn get<'a>(&'a self, tag: usize) -> Doc<'a> {
+    pub fn get(&self, tag: usize) -> Doc<'doc> {
         reader::get_doc(*self, tag)
     }
 
     pub fn is_empty(&self) -> bool {
         self.start == self.end
     }
 
-    pub fn as_str_slice<'a>(&'a self) -> &'a str {
+    pub fn as_str_slice(&self) -> &'doc str {
         str::from_utf8(&self.data[self.start..self.end]).unwrap()
     }
 

src/librustc/front/map/collector.rs

@@ -14,7 +14,7 @@ use super::MapEntry::*;
 use rustc_front::hir::*;
 use rustc_front::util;
 use rustc_front::intravisit::{self, Visitor};
-use middle::def_id::{CRATE_DEF_INDEX, DefIndex};
+use middle::def_id::{CRATE_DEF_INDEX, DefId, DefIndex};
 use std::iter::repeat;
 use syntax::ast::{NodeId, CRATE_NODE_ID, DUMMY_NODE_ID};
 use syntax::codemap::Span;
@@ -50,6 +50,7 @@ impl<'ast> NodeCollector<'ast> {
                   parent: &'ast InlinedParent,
                   parent_node: NodeId,
                   parent_def_path: DefPath,
+                  parent_def_id: DefId,
                   map: Vec<MapEntry<'ast>>,
                   definitions: Definitions)
                   -> NodeCollector<'ast> {
@@ -60,8 +61,14 @@ impl<'ast> NodeCollector<'ast> {
             definitions: definitions,
         };
 
+        assert_eq!(parent_def_path.krate, parent_def_id.krate);
+        let root_path = Box::new(InlinedRootPath {
+            data: parent_def_path.data,
+            def_id: parent_def_id,
+        });
+
         collector.insert_entry(parent_node, RootInlinedParent(parent));
-        collector.create_def(parent_node, DefPathData::InlinedRoot(parent_def_path));
+        collector.create_def(parent_node, DefPathData::InlinedRoot(root_path));
 
         collector
     }
@@ -126,11 +133,16 @@ impl<'ast> Visitor<'ast> for NodeCollector<'ast> {
         // Pick the def data. This need not be unique, but the more
         // information we encapsulate into
         let def_data = match i.node {
-            ItemDefaultImpl(..) | ItemImpl(..) => DefPathData::Impl(i.name),
-            ItemEnum(..) | ItemStruct(..) | ItemTrait(..) => DefPathData::Type(i.name),
-            ItemExternCrate(..) | ItemMod(..) => DefPathData::Mod(i.name),
-            ItemStatic(..) | ItemConst(..) | ItemFn(..) => DefPathData::Value(i.name),
-            _ => DefPathData::Misc,
+            ItemDefaultImpl(..) | ItemImpl(..) =>
+                DefPathData::Impl,
+            ItemEnum(..) | ItemStruct(..) | ItemTrait(..) |
+            ItemExternCrate(..) | ItemMod(..) | ItemForeignMod(..) |
+            ItemTy(..) =>
+                DefPathData::TypeNs(i.name),
+            ItemStatic(..) | ItemConst(..) | ItemFn(..) =>
+                DefPathData::ValueNs(i.name),
+            ItemUse(..) =>
+                DefPathData::Misc,
         };
 
         self.insert_def(i.id, NodeItem(i), def_data);
@@ -195,7 +207,7 @@ impl<'ast> Visitor<'ast> for NodeCollector<'ast> {
     fn visit_foreign_item(&mut self, foreign_item: &'ast ForeignItem) {
         self.insert_def(foreign_item.id,
                         NodeForeignItem(foreign_item),
-                        DefPathData::Value(foreign_item.name));
+                        DefPathData::ValueNs(foreign_item.name));
 
         let parent_node = self.parent_node;
         self.parent_node = foreign_item.id;
@@ -215,8 +227,8 @@ impl<'ast> Visitor<'ast> for NodeCollector<'ast> {
 
     fn visit_trait_item(&mut self, ti: &'ast TraitItem) {
         let def_data = match ti.node {
-            MethodTraitItem(..) | ConstTraitItem(..) => DefPathData::Value(ti.name),
-            TypeTraitItem(..) => DefPathData::Type(ti.name),
+            MethodTraitItem(..) | ConstTraitItem(..) => DefPathData::ValueNs(ti.name),
+            TypeTraitItem(..) => DefPathData::TypeNs(ti.name),
         };
 
         self.insert(ti.id, NodeTraitItem(ti));
@@ -239,8 +251,8 @@ impl<'ast> Visitor<'ast> for NodeCollector<'ast> {
 
     fn visit_impl_item(&mut self, ii: &'ast ImplItem) {
         let def_data = match ii.node {
-            ImplItemKind::Method(..) | ImplItemKind::Const(..) => DefPathData::Value(ii.name),
-            ImplItemKind::Type(..) => DefPathData::Type(ii.name),
+            ImplItemKind::Method(..) | ImplItemKind::Const(..) => DefPathData::ValueNs(ii.name),
+            ImplItemKind::Type(..) => DefPathData::TypeNs(ii.name),
         };
 
         self.insert_def(ii.id, NodeImplItem(ii), def_data);

src/librustc/front/map/definitions.rs

@@ -59,23 +59,94 @@ pub struct DefData {
     pub node_id: ast::NodeId,
 }
 
-pub type DefPath = Vec<DisambiguatedDefPathData>;
+#[derive(Clone, Debug, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
+pub struct DefPath {
+    /// the path leading from the crate root to the item
+    pub data: Vec<DisambiguatedDefPathData>,
+
+    /// what krate root is this path relative to?
+    pub krate: ast::CrateNum,
+}
+
+impl DefPath {
+    pub fn is_local(&self) -> bool {
+        self.krate == LOCAL_CRATE
+    }
+
+    pub fn make<FN>(start_krate: ast::CrateNum,
+                    start_index: DefIndex,
+                    mut get_key: FN) -> DefPath
+        where FN: FnMut(DefIndex) -> DefKey
+    {
+        let mut krate = start_krate;
+        let mut data = vec![];
+        let mut index = Some(start_index);
+        loop {
+            let p = index.unwrap();
+            let key = get_key(p);
+            match key.disambiguated_data.data {
+                DefPathData::CrateRoot => {
+                    assert!(key.parent.is_none());
+                    break;
+                }
+                DefPathData::InlinedRoot(ref p) => {
+                    assert!(key.parent.is_none());
+                    assert!(!p.def_id.is_local());
+                    data.extend(p.data.iter().cloned().rev());
+                    krate = p.def_id.krate;
+                    break;
+                }
+                _ => {
+                    data.push(key.disambiguated_data);
+                    index = key.parent;
+                }
+            }
+        }
+        data.reverse();
+        DefPath { data: data, krate: krate }
+    }
+}
+
+/// Root of an inlined item. We track the `DefPath` of the item within
+/// the original crate but also its def-id. This is kind of an
+/// augmented version of a `DefPath` that includes a `DefId`. This is
+/// all sort of ugly but the hope is that inlined items will be going
+/// away soon anyway.
+///
+/// Some of the constraints that led to the current approach:
+///
+/// - I don't want to have a `DefId` in the main `DefPath` because
+///   that gets serialized for incr. comp., and when reloaded the
+///   `DefId` is no longer valid. I'd rather maintain the invariant
+///   that every `DefId` is valid, and a potentially outdated `DefId` is
+///   represented as a `DefPath`.
+///   - (We don't serialize def-paths from inlined items, so it's ok to have one here.)
+/// - We need to be able to extract the def-id from inline items to
+///   make the symbol name. In theory we could retrace it from the
+///   data, but the metadata doesn't have the required indices, and I
+///   don't want to write the code to create one just for this.
+/// - It may be that we don't actually need `data` at all. We'll have
+///   to see about that.
+#[derive(Clone, Debug, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
+pub struct InlinedRootPath {
+    pub data: Vec<DisambiguatedDefPathData>,
+    pub def_id: DefId,
+}
 
 #[derive(Clone, Debug, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
 pub enum DefPathData {
     // Root: these should only be used for the root nodes, because
     // they are treated specially by the `def_path` function.
     CrateRoot,
-    InlinedRoot(DefPath),
+    InlinedRoot(Box<InlinedRootPath>),
 
     // Catch-all for random DefId things like DUMMY_NODE_ID
     Misc,
 
     // Different kinds of items and item-like things:
-    Impl(ast::Name),
-    Type(ast::Name),
-    Mod(ast::Name),
-    Value(ast::Name),
+    Impl,
+    TypeNs(ast::Name), // something in the type NS
+    ValueNs(ast::Name), // something in the value NS
     MacroDef(ast::Name),
     ClosureExpr,
 
@@ -87,10 +158,6 @@ pub enum DefPathData {
     StructCtor, // implicit ctor for a tuple-like struct
     Initializer, // initializer for a const
     Binding(ast::Name), // pattern binding
-
-    // An external crate that does not have an `extern crate` in this
-    // crate.
-    DetachedCrate(ast::Name),
 }
 
 impl Definitions {
@@ -116,7 +183,7 @@ impl Definitions {
     /// will be the path of the item in the external crate (but the
     /// path will begin with the path to the external crate).
     pub fn def_path(&self, index: DefIndex) -> DefPath {
-        make_def_path(index, |p| self.def_key(p))
+        DefPath::make(LOCAL_CRATE, index, |p| self.def_key(p))
     }
 
     pub fn opt_def_index(&self, node: ast::NodeId) -> Option<DefIndex> {
@@ -175,20 +242,21 @@ impl DefPathData {
     pub fn as_interned_str(&self) -> InternedString {
         use self::DefPathData::*;
         match *self {
-            Impl(name) |
-            Type(name) |
-            Mod(name) |
-            Value(name) |
+            TypeNs(name) |
+            ValueNs(name) |
             MacroDef(name) |
             TypeParam(name) |
             LifetimeDef(name) |
             EnumVariant(name) |
-            DetachedCrate(name) |
             Binding(name) |
             Field(name) => {
                 name.as_str()
             }
 
+            Impl => {
+                InternedString::new("{{impl}}")
+            }
+
             // note that this does not show up in user printouts
             CrateRoot => {
                 InternedString::new("{{root}}")
@@ -222,29 +290,3 @@ impl DefPathData {
     }
 }
 
-pub fn make_def_path<FN>(start_index: DefIndex, mut get_key: FN) -> DefPath
-    where FN: FnMut(DefIndex) -> DefKey
-{
-    let mut result = vec![];
-    let mut index = Some(start_index);
-    while let Some(p) = index {
-        let key = get_key(p);
-        match key.disambiguated_data.data {
-            DefPathData::CrateRoot => {
-                assert!(key.parent.is_none());
-                break;
-            }
-            DefPathData::InlinedRoot(ref p) => {
-                assert!(key.parent.is_none());
-                result.extend(p.iter().cloned().rev());
-                break;
-            }
-            _ => {
-                result.push(key.disambiguated_data);
-                index = key.parent;
-            }
-        }
-    }
-    result.reverse();
-    result
-}