Merge #8812

8812: fix: fix dependencies of build scripts r=jonas-schievink a=jonas-schievink

Previously, we added a dependency for all targets in a package to the package's library target. This is correct for most targets, except build scripts, which run before the library crate is built. This PR removes the incorrect dependency on the library target.

We also used to treat all dependencies the same, which led to build scripts being able to use regular dependencies as well as dev-dependencies. This is also fixed by this PR, and build scripts only depend on build-dependencies.

Incorrect dependency graph:

![screenshot-2021-05-11-23:35:01](https://user-images.githubusercontent.com/1786438/117975228-c2066a80-b32e-11eb-8f01-1e3ea904a608.png)

Fixed graph after this PR:

![screenshot-2021-05-12-14:29:31](https://user-images.githubusercontent.com/1786438/117975253-c9c60f00-b32e-11eb-8f6c-9e42d4e32468.png)


Co-authored-by: Jonas Schievink <[email protected]>

2 files changed, 56 insertions, 12 deletions

diff --git a/crates/project_model/src/cargo_workspace.rs b/crates/project_model/src/cargo_workspace.rs
index b18699b77..4a4996cf4 100644
--- a/crates/project_model/src/cargo_workspace.rs
+++ b/crates/project_model/src/cargo_workspace.rs
@@ -119,6 +119,32 @@ pub struct RustAnalyzerPackageMetaData {
 pub struct PackageDependency {
     pub pkg: Package,
     pub name: String,
+    pub kind: DepKind,
+}
+
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub enum DepKind {
+    /// Available to the library, binary, and dev targets in the package (but not the build script).
+    Normal,
+    /// Available only to test and bench targets (and the library target, when built with `cfg(test)`).
+    Dev,
+    /// Available only to the build script target.
+    Build,
+}
+
+impl DepKind {
+    fn new(list: &[cargo_metadata::DepKindInfo]) -> Self {
+        for info in list {
+            match info.kind {
+                cargo_metadata::DependencyKind::Normal => return Self::Normal,
+                cargo_metadata::DependencyKind::Development => return Self::Dev,
+                cargo_metadata::DependencyKind::Build => return Self::Build,
+                cargo_metadata::DependencyKind::Unknown => continue,
+            }
+        }
+
+        Self::Normal
+    }
 }
 
 /// Information associated with a package's target
@@ -144,6 +170,7 @@ pub enum TargetKind {
     Example,
     Test,
     Bench,
+    BuildScript,
     Other,
 }
 
@@ -155,6 +182,7 @@ impl TargetKind {
                 "test" => TargetKind::Test,
                 "bench" => TargetKind::Bench,
                 "example" => TargetKind::Example,
+                "custom-build" => TargetKind::BuildScript,
                 "proc-macro" => TargetKind::Lib,
                 _ if kind.contains("lib") => TargetKind::Lib,
                 _ => continue,
@@ -301,7 +329,11 @@ impl CargoWorkspace {
                         continue;
                     }
                 };
-                let dep = PackageDependency { name: dep_node.name, pkg };
+                let dep = PackageDependency {
+                    name: dep_node.name,
+                    pkg,
+                    kind: DepKind::new(&dep_node.dep_kinds),
+                };
                 packages[source].dependencies.push(dep);
             }
             packages[source].active_features.extend(node.features);
diff --git a/crates/project_model/src/workspace.rs b/crates/project_model/src/workspace.rs
index 761fbb3ab..607e62ea5 100644
--- a/crates/project_model/src/workspace.rs
+++ b/crates/project_model/src/workspace.rs
@@ -6,6 +6,7 @@ use std::{collections::VecDeque, fmt, fs, path::Path, process::Command};
 
 use anyhow::{Context, Result};
 use base_db::{CrateDisplayName, CrateGraph, CrateId, CrateName, Edition, Env, FileId, ProcMacro};
+use cargo_workspace::DepKind;
 use cfg::CfgOptions;
 use paths::{AbsPath, AbsPathBuf};
 use proc_macro_api::ProcMacroClient;
@@ -407,23 +408,25 @@ fn cargo_to_crate_graph(
                     }
                 }
 
-                pkg_crates.entry(pkg).or_insert_with(Vec::new).push(crate_id);
+                pkg_crates.entry(pkg).or_insert_with(Vec::new).push((crate_id, cargo[tgt].kind));
             }
         }
 
         // Set deps to the core, std and to the lib target of the current package
-        for &from in pkg_crates.get(&pkg).into_iter().flatten() {
+        for (from, kind) in pkg_crates.get(&pkg).into_iter().flatten() {
             if let Some((to, name)) = lib_tgt.clone() {
-                if to != from {
+                if to != *from && *kind != TargetKind::BuildScript {
+                    // (build script can not depend on its library target)
+
                     // For root projects with dashes in their name,
                     // cargo metadata does not do any normalization,
                     // so we do it ourselves currently
                     let name = CrateName::normalize_dashes(&name);
-                    add_dep(&mut crate_graph, from, name, to);
+                    add_dep(&mut crate_graph, *from, name, to);
                 }
             }
             for (name, krate) in public_deps.iter() {
-                add_dep(&mut crate_graph, from, name.clone(), *krate);
+                add_dep(&mut crate_graph, *from, name.clone(), *krate);
             }
         }
     }
@@ -434,8 +437,17 @@ fn cargo_to_crate_graph(
         for dep in cargo[pkg].dependencies.iter() {
             let name = CrateName::new(&dep.name).unwrap();
             if let Some(&to) = pkg_to_lib_crate.get(&dep.pkg) {
-                for &from in pkg_crates.get(&pkg).into_iter().flatten() {
-                    add_dep(&mut crate_graph, from, name.clone(), to)
+                for (from, kind) in pkg_crates.get(&pkg).into_iter().flatten() {
+                    if dep.kind == DepKind::Build && *kind != TargetKind::BuildScript {
+                        // Only build scripts may depend on build dependencies.
+                        continue;
+                    }
+                    if dep.kind != DepKind::Build && *kind == TargetKind::BuildScript {
+                        // Build scripts may only depend on build dependencies.
+                        continue;
+                    }
+
+                    add_dep(&mut crate_graph, *from, name.clone(), to)
                 }
             }
         }
@@ -472,7 +484,7 @@ fn handle_rustc_crates(
     pkg_to_lib_crate: &mut FxHashMap<la_arena::Idx<crate::PackageData>, CrateId>,
     public_deps: &[(CrateName, CrateId)],
     cargo: &CargoWorkspace,
-    pkg_crates: &FxHashMap<la_arena::Idx<crate::PackageData>, Vec<CrateId>>,
+    pkg_crates: &FxHashMap<la_arena::Idx<crate::PackageData>, Vec<(CrateId, TargetKind)>>,
 ) {
     let mut rustc_pkg_crates = FxHashMap::default();
     // The root package of the rustc-dev component is rustc_driver, so we match that
@@ -541,13 +553,13 @@ fn handle_rustc_crates(
                 if !package.metadata.rustc_private {
                     continue;
                 }
-                for &from in pkg_crates.get(&pkg).into_iter().flatten() {
+                for (from, _) in pkg_crates.get(&pkg).into_iter().flatten() {
                     // Avoid creating duplicate dependencies
                     // This avoids the situation where `from` depends on e.g. `arrayvec`, but
                     // `rust_analyzer` thinks that it should use the one from the `rustcSource`
                     // instead of the one from `crates.io`
-                    if !crate_graph[from].dependencies.iter().any(|d| d.name == name) {
-                        add_dep(crate_graph, from, name.clone(), to);
+                    if !crate_graph[*from].dependencies.iter().any(|d| d.name == name) {
+                        add_dep(crate_graph, *from, name.clone(), to);
                     }
                 }
             }