fix: Split toolchain and datalayout out of CrateData

Author: Veykril

crates/base-db/src/input.rs

@@ -11,7 +11,6 @@ use std::{fmt, mem, ops, str::FromStr};
 use cfg::CfgOptions;
 use la_arena::{Arena, Idx, RawIdx};
 use rustc_hash::{FxHashMap, FxHashSet};
-use semver::Version;
 use syntax::SmolStr;
 use triomphe::Arc;
 use vfs::{file_set::FileSet, AbsPathBuf, AnchoredPath, FileId, VfsPath};
@@ -292,16 +291,11 @@ pub struct CrateData {
     pub dependencies: Vec<Dependency>,
     pub origin: CrateOrigin,
     pub is_proc_macro: bool,
-    // FIXME: These things should not be per crate! These are more per workspace crate graph level
-    // things. This info does need to be somewhat present though as to prevent deduplication from
-    // happening across different workspaces with different layouts.
-    pub target_layout: TargetLayoutLoadResult,
-    pub toolchain: Option<Version>,
 }
 
 impl CrateData {
     /// Check if [`other`] is almost equal to [`self`] ignoring `CrateOrigin` value.
-    pub fn eq_ignoring_origin_and_deps(&self, other: &CrateData, ignore_dev_deps: bool) -> bool {
+    fn eq_ignoring_origin_and_deps(&self, other: &CrateData, ignore_dev_deps: bool) -> bool {
         // This method has some obscure bits. These are mostly there to be compliant with
         // some patches. References to the patches are given.
         if self.root_file_id != other.root_file_id {
@@ -353,10 +347,6 @@ impl CrateData {
 
         slf_deps.eq(other_deps)
     }
-
-    pub fn channel(&self) -> Option<ReleaseChannel> {
-        self.toolchain.as_ref().and_then(|v| ReleaseChannel::from_str(&v.pre))
-    }
 }
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
@@ -439,8 +429,6 @@ impl CrateGraph {
         env: Env,
         is_proc_macro: bool,
         origin: CrateOrigin,
-        target_layout: Result<Arc<str>, Arc<str>>,
-        toolchain: Option<Version>,
     ) -> CrateId {
         let data = CrateData {
             root_file_id,
@@ -452,9 +440,7 @@ impl CrateGraph {
             env,
             dependencies: Vec::new(),
             origin,
-            target_layout,
             is_proc_macro,
-            toolchain,
         };
         self.arena.alloc(data)
     }
@@ -524,6 +510,10 @@ impl CrateGraph {
         self.arena.is_empty()
     }
 
+    pub fn len(&self) -> usize {
+        self.arena.len()
+    }
+
     pub fn iter(&self) -> impl Iterator<Item = CrateId> + '_ {
         self.arena.iter().map(|(idx, _)| idx)
     }
@@ -624,13 +614,16 @@ impl CrateGraph {
     ///
     /// This will deduplicate the crates of the graph where possible.
     /// Note that for deduplication to fully work, `self`'s crate dependencies must be sorted by crate id.
-    /// If the crate dependencies were sorted, the resulting graph from this `extend` call will also have the crate dependencies sorted.
+    /// If the crate dependencies were sorted, the resulting graph from this `extend` call will also
+    /// have the crate dependencies sorted.
+    ///
+    /// Returns a mapping from `other`'s crate ids to the new crate ids in `self`.
     pub fn extend(
         &mut self,
         mut other: CrateGraph,
         proc_macros: &mut ProcMacroPaths,
-        on_finished: impl FnOnce(&FxHashMap<CrateId, CrateId>),
-    ) {
+        may_merge: impl Fn((CrateId, &CrateData), (CrateId, &CrateData)) -> bool,
+    ) -> FxHashMap<CrateId, CrateId> {
         let topo = other.crates_in_topological_order();
         let mut id_map: FxHashMap<CrateId, CrateId> = FxHashMap::default();
         for topo in topo {
@@ -639,6 +632,10 @@ impl CrateGraph {
             crate_data.dependencies.iter_mut().for_each(|dep| dep.crate_id = id_map[&dep.crate_id]);
             crate_data.dependencies.sort_by_key(|dep| dep.crate_id);
             let res = self.arena.iter().find_map(|(id, data)| {
+                if !may_merge((id, &data), (topo, &crate_data)) {
+                    return None;
+                }
+
                 match (&data.origin, &crate_data.origin) {
                     (a, b) if a == b => {
                         if data.eq_ignoring_origin_and_deps(crate_data, false) {
@@ -663,8 +660,7 @@ impl CrateGraph {
                 None
             });
 
-            if let Some((res, should_update_lib_to_local)) = res {
-                id_map.insert(topo, res);
+            let new_id = if let Some((res, should_update_lib_to_local)) = res {
                 if should_update_lib_to_local {
                     assert!(self.arena[res].origin.is_lib());
                     assert!(crate_data.origin.is_local());
@@ -673,16 +669,17 @@ impl CrateGraph {
                     // Move local's dev dependencies into the newly-local-formerly-lib crate.
                     self.arena[res].dependencies = crate_data.dependencies.clone();
                 }
+                res
             } else {
-                let id = self.arena.alloc(crate_data.clone());
-                id_map.insert(topo, id);
-            }
+                self.arena.alloc(crate_data.clone())
+            };
+            id_map.insert(topo, new_id);
         }
 
         *proc_macros =
             mem::take(proc_macros).into_iter().map(|(id, macros)| (id_map[&id], macros)).collect();
 
-        on_finished(&id_map);
+        id_map
     }
 
     fn find_path(
@@ -889,8 +886,6 @@ mod tests {
             Env::default(),
             false,
             CrateOrigin::Local { repo: None, name: None },
-            Err("".into()),
-            None,
         );
         let crate2 = graph.add_crate_root(
             FileId::from_raw(2u32),
@@ -902,8 +897,6 @@ mod tests {
             Env::default(),
             false,
             CrateOrigin::Local { repo: None, name: None },
-            Err("".into()),
-            None,
         );
         let crate3 = graph.add_crate_root(
             FileId::from_raw(3u32),
@@ -915,8 +908,6 @@ mod tests {
             Env::default(),
             false,
             CrateOrigin::Local { repo: None, name: None },
-            Err("".into()),
-            None,
         );
         assert!(graph
             .add_dep(
@@ -951,8 +942,6 @@ mod tests {
             Env::default(),
             false,
             CrateOrigin::Local { repo: None, name: None },
-            Err("".into()),
-            None,
         );
         let crate2 = graph.add_crate_root(
             FileId::from_raw(2u32),
@@ -964,8 +953,6 @@ mod tests {
             Env::default(),
             false,
             CrateOrigin::Local { repo: None, name: None },
-            Err("".into()),
-            None,
         );
         assert!(graph
             .add_dep(
@@ -994,8 +981,6 @@ mod tests {
             Env::default(),
             false,
             CrateOrigin::Local { repo: None, name: None },
-            Err("".into()),
-            None,
         );
         let crate2 = graph.add_crate_root(
             FileId::from_raw(2u32),
@@ -1007,8 +992,6 @@ mod tests {
             Env::default(),
             false,
             CrateOrigin::Local { repo: None, name: None },
-            Err("".into()),
-            None,
         );
         let crate3 = graph.add_crate_root(
             FileId::from_raw(3u32),
@@ -1020,8 +1003,6 @@ mod tests {
             Env::default(),
             false,
             CrateOrigin::Local { repo: None, name: None },
-            Err("".into()),
-            None,
         );
         assert!(graph
             .add_dep(
@@ -1050,8 +1031,6 @@ mod tests {
             Env::default(),
             false,
             CrateOrigin::Local { repo: None, name: None },
-            Err("".into()),
-            None,
         );
         let crate2 = graph.add_crate_root(
             FileId::from_raw(2u32),
@@ -1063,8 +1042,6 @@ mod tests {
             Env::default(),
             false,
             CrateOrigin::Local { repo: None, name: None },
-            Err("".into()),
-            None,
         );
         assert!(graph
             .add_dep(

crates/base-db/src/lib.rs

@@ -62,6 +62,20 @@ pub trait SourceDatabase: FileLoader + std::fmt::Debug {
     /// The crate graph.
     #[salsa::input]
     fn crate_graph(&self) -> Arc<CrateGraph>;
+
+    // FIXME: Consider removing this, making HirDatabase::target_data_layout an input query
+    #[salsa::input]
+    fn data_layout(&self, krate: CrateId) -> TargetLayoutLoadResult;
+
+    #[salsa::input]
+    fn toolchain(&self, krate: CrateId) -> Option<Version>;
+
+    #[salsa::transparent]
+    fn toolchain_channel(&self, krate: CrateId) -> Option<ReleaseChannel>;
+}
+
+fn toolchain_channel(db: &dyn SourceDatabase, krate: CrateId) -> Option<ReleaseChannel> {
+    db.toolchain(krate).as_ref().and_then(|v| ReleaseChannel::from_str(&v.pre))
 }
 
 fn parse(db: &dyn SourceDatabase, file_id: FileId) -> Parse<ast::SourceFile> {

crates/hir-expand/src/change.rs

@@ -1,6 +1,10 @@
 //! Defines a unit of change that can applied to the database to get the next
 //! state. Changes are transactional.
-use base_db::{salsa::Durability, CrateGraph, FileChange, SourceDatabaseExt, SourceRoot};
+use base_db::{
+    salsa::Durability, CrateGraph, CrateId, FileChange, SourceDatabaseExt, SourceRoot,
+    TargetLayoutLoadResult, Version,
+};
+use la_arena::RawIdx;
 use span::FileId;
 use triomphe::Arc;
 
@@ -10,6 +14,8 @@ use crate::{db::ExpandDatabase, proc_macro::ProcMacros};
 pub struct Change {
     pub source_change: FileChange,
     pub proc_macros: Option<ProcMacros>,
+    pub toolchains: Option<Vec<Option<Version>>>,
+    pub target_data_layouts: Option<Vec<TargetLayoutLoadResult>>,
 }
 
 impl Change {
@@ -22,6 +28,24 @@ impl Change {
         if let Some(proc_macros) = self.proc_macros {
             db.set_proc_macros_with_durability(Arc::new(proc_macros), Durability::HIGH);
         }
+        if let Some(target_data_layouts) = self.target_data_layouts {
+            for (id, val) in target_data_layouts.into_iter().enumerate() {
+                db.set_data_layout_with_durability(
+                    CrateId::from_raw(RawIdx::from(id as u32)),
+                    val,
+                    Durability::HIGH,
+                );
+            }
+        }
+        if let Some(toolchains) = self.toolchains {
+            for (id, val) in toolchains.into_iter().enumerate() {
+                db.set_toolchain_with_durability(
+                    CrateId::from_raw(RawIdx::from(id as u32)),
+                    val,
+                    Durability::HIGH,
+                );
+            }
+        }
     }
 
     pub fn change_file(&mut self, file_id: FileId, new_text: Option<Arc<str>>) {
@@ -36,6 +60,14 @@ impl Change {
         self.proc_macros = Some(proc_macros);
     }
 
+    pub fn set_toolchains(&mut self, toolchains: Vec<Option<Version>>) {
+        self.toolchains = Some(toolchains);
+    }
+
+    pub fn set_target_data_layouts(&mut self, target_data_layouts: Vec<TargetLayoutLoadResult>) {
+        self.target_data_layouts = Some(target_data_layouts);
+    }
+
     pub fn set_roots(&mut self, roots: Vec<SourceRoot>) {
         self.source_change.set_roots(roots)
     }

crates/hir-expand/src/declarative.rs

@@ -31,7 +31,7 @@ impl DeclarativeMacroExpander {
         call_id: MacroCallId,
     ) -> ExpandResult<tt::Subtree> {
         let loc = db.lookup_intern_macro_call(call_id);
-        let toolchain = &db.crate_graph()[loc.def.krate].toolchain;
+        let toolchain = db.toolchain(loc.def.krate);
         let new_meta_vars = toolchain.as_ref().map_or(false, |version| {
             REQUIREMENT.get_or_init(|| VersionReq::parse(">=1.76").unwrap()).matches(
                 &base_db::Version {
@@ -67,7 +67,7 @@ impl DeclarativeMacroExpander {
         krate: CrateId,
         call_site: Span,
     ) -> ExpandResult<tt::Subtree> {
-        let toolchain = &db.crate_graph()[krate].toolchain;
+        let toolchain = db.toolchain(krate);
         let new_meta_vars = toolchain.as_ref().map_or(false, |version| {
             REQUIREMENT.get_or_init(|| VersionReq::parse(">=1.76").unwrap()).matches(
                 &base_db::Version {
@@ -119,7 +119,7 @@ impl DeclarativeMacroExpander {
                 _ => None,
             }
         };
-        let toolchain = crate_data.toolchain.as_ref();
+        let toolchain = db.toolchain(def_crate);
         let new_meta_vars = toolchain.as_ref().map_or(false, |version| {
             REQUIREMENT.get_or_init(|| VersionReq::parse(">=1.76").unwrap()).matches(
                 &base_db::Version {

crates/hir-ty/src/layout/target.rs

@@ -11,10 +11,8 @@ pub fn target_data_layout_query(
     db: &dyn HirDatabase,
     krate: CrateId,
 ) -> Result<Arc<TargetDataLayout>, Arc<str>> {
-    let crate_graph = db.crate_graph();
-    let res = crate_graph[krate].target_layout.as_deref();
-    match res {
-        Ok(it) => match TargetDataLayout::parse_from_llvm_datalayout_string(it) {
+    match db.data_layout(krate) {
+        Ok(it) => match TargetDataLayout::parse_from_llvm_datalayout_string(&it) {
             Ok(it) => Ok(Arc::new(it)),
             Err(e) => {
                 Err(match e {
@@ -44,6 +42,6 @@ pub fn target_data_layout_query(
                 }.into())
             }
         },
-        Err(e) => Err(Arc::from(&**e)),
+        Err(e) => Err(e),
     }
 }

crates/hir-ty/src/layout/tests.rs

@@ -27,7 +27,7 @@ fn current_machine_data_layout() -> String {
 fn eval_goal(ra_fixture: &str, minicore: &str) -> Result<Arc<Layout>, LayoutError> {
     let target_data_layout = current_machine_data_layout();
     let ra_fixture = format!(
-        "{minicore}//- /main.rs crate:test target_data_layout:{target_data_layout}\n{ra_fixture}",
+        "//- target_data_layout: {target_data_layout}\n{minicore}//- /main.rs crate:test\n{ra_fixture}",
     );
 
     let (db, file_ids) = TestDB::with_many_files(&ra_fixture);
@@ -76,7 +76,7 @@ fn eval_goal(ra_fixture: &str, minicore: &str) -> Result<Arc<Layout>, LayoutErro
 fn eval_expr(ra_fixture: &str, minicore: &str) -> Result<Arc<Layout>, LayoutError> {
     let target_data_layout = current_machine_data_layout();
     let ra_fixture = format!(
-        "{minicore}//- /main.rs crate:test target_data_layout:{target_data_layout}\nfn main(){{let goal = {{{ra_fixture}}};}}",
+        "//- target_data_layout: {target_data_layout}\n{minicore}//- /main.rs crate:test\nfn main(){{let goal = {{{ra_fixture}}};}}",
     );
 
     let (db, file_id) = TestDB::with_single_file(&ra_fixture);

crates/ide-completion/src/context.rs

@@ -717,7 +717,7 @@ impl<'a> CompletionContext<'a> {
         let krate = scope.krate();
         let module = scope.module();
 
-        let toolchain = db.crate_graph()[krate.into()].channel();
+        let toolchain = db.toolchain_channel(krate.into());
         // `toolchain == None` means we're in some detached files. Since we have no information on
         // the toolchain being used, let's just allow unstable items to be listed.
         let is_nightly = matches!(toolchain, Some(base_db::ReleaseChannel::Nightly) | None);

crates/ide/src/doc_links.rs

@@ -501,7 +501,7 @@ fn get_doc_base_urls(
     let Some(krate) = def.krate(db) else { return Default::default() };
     let Some(display_name) = krate.display_name(db) else { return Default::default() };
     let crate_data = &db.crate_graph()[krate.into()];
-    let channel = crate_data.channel().unwrap_or(ReleaseChannel::Nightly).as_str();
+    let channel = db.toolchain_channel(krate.into()).unwrap_or(ReleaseChannel::Nightly).as_str();
 
     let (web_base, local_base) = match &crate_data.origin {
         // std and co do not specify `html_root_url` any longer so we gotta handwrite this ourself.

crates/ide/src/lib.rs

@@ -253,11 +253,11 @@ impl Analysis {
             Env::default(),
             false,
             CrateOrigin::Local { repo: None, name: None },
-            Err("Analysis::from_single_file has no target layout".into()),
-            None,
         );
         change.change_file(file_id, Some(Arc::from(text)));
         change.set_crate_graph(crate_graph);
+        change.set_target_data_layouts(vec![Err("fixture has no layout".into())]);
+        change.set_toolchains(vec![None]);
         host.apply_change(change);
         (host.analysis(), file_id)
     }