rust-analyzer: Choose the library's display_name as much as possible.

[reiyw]

Thank you for the Rust Analyzer support without an explicit target list 8c05ac7. This greatly reduces our workload.

rules_rust/tools/rust_analyzer/aquery.rs

Lines 347 to 348 in 8c05ac7

	// TODO: It's probably better if the display name matches the library target
	// after the specs are consolidated.

As left as a TODO, when there is a CrateSpec with the same ID, the information of the first CrateSpec added to the BTreeSet takes precedence, regardless of whether it is a test or a library. The comment says "probably better if the display name matches the library target", but in fact, I had to make the display name matches the library target or Rust Analyzer returned an unresolved import error. I haven't looked at the source code of Rust Analyzer in detail, but from my experiments, it seems that display_name is used to match crate entries in rust-project.json against symbols in the source file.

In order to choose the library's display_name as much as possible when consolidating crate specs, I made the following change.

@@ -167,6 +167,9 @@ fn consolidate_crate_specs(crate_specs: Vec<CrateSpec>) -> anyhow::Result<BTreeS
         log::debug!("{:?}", spec);
         if let Some(existing) = consolidated_specs.get_mut(&spec.crate_id) {
             existing.deps.extend(spec.deps);
+            if existing.display_name.ends_with("_test") {
+                existing.display_name = spec.display_name;
+            }
         } else {
             consolidated_specs.insert(spec.crate_id.clone(), spec);
         }

This seems to have eliminated the unresolved import error in our repository and everything seems to be working fine, but I'm not sure how general this solution is. Can anyone come up with a better, more general solution? If this solution would suffice, I will submit a PR.

[UebelAndre]

Hi! Thanks for trying out the new rust-analyzer functionality! I think for this TODO it might be better to allow the aspect to generate an additional field that tracks whether or not the crate is a test target and allow non-test display names to always be written to the BTreeMap, overriding whatever was there before. Do you know if binary targets would share the same ID as a Library target?

[reiyw]

Thank you for your quick response! Your suggestion seems much more reasonable.

Do you know if binary targets would share the same ID as a Library target?

Yes. If a library and a test for the library are written in the same file (I guess this is a common pattern of unit test in Rust), they share the same ID since the ID is just a file path.

[reiyw]

I created a reproduction repository: https://github.com/reiyw/rules_rust_1032_repro

[reiyw]

CrateInfo already has is_test field, so all I had to do was to propagate that information:

diff --git a/rust/private/rust_analyzer.bzl b/rust/private/rust_analyzer.bzl
index 7c47235..ad92610 100644
--- a/rust/private/rust_analyzer.bzl
+++ b/rust/private/rust_analyzer.bzl
@@ -145,6 +145,7 @@ def _create_single_crate(ctx, info):
     crate["display_name"] = crate_name
     crate["edition"] = info.crate.edition
     crate["env"] = {}
+    crate["is_test"] = info.crate.is_test
 
     # Switch on external/ to determine if crates are in the workspace or remote.
     # TODO: Some folks may want to override this for vendored dependencies.
diff --git a/tools/rust_analyzer/aquery.rs b/tools/rust_analyzer/aquery.rs
index c75596a..db16b5a 100644
--- a/tools/rust_analyzer/aquery.rs
+++ b/tools/rust_analyzer/aquery.rs
@@ -51,6 +51,7 @@ pub struct CrateSpec {
     pub cfg: Vec<String>,
     pub env: BTreeMap<String, String>,
     pub target: String,
+    pub is_test: bool,
 }
 
 #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Deserialize)]
@@ -167,6 +168,9 @@ fn consolidate_crate_specs(crate_specs: Vec<CrateSpec>) -> anyhow::Result<BTreeS
         log::debug!("{:?}", spec);
         if let Some(existing) = consolidated_specs.get_mut(&spec.crate_id) {
             existing.deps.extend(spec.deps);
+            if existing.is_test {
+                existing.display_name = spec.display_name;
+            }
         } else {
             consolidated_specs.insert(spec.crate_id.clone(), spec);
         }

This change got back the functionality of rust-analyzer in my repro repository.

[UebelAndre]

Do you know if binary targets would share the same ID as a Library target?

Yes. If a library and a test for the library are written in the same file (I guess this is a common pattern of unit test in Rust), they share the same ID since the ID is just a file path.

That was t quite my question. I totally agree with the assessment of library and test targets. The thought was if library and binary (non-test) would match but I think I they don't.

I think your diff looks good! Would you be able to open a PR for it and update the unittests to account for the change? 🙏

[reiyw]

I realized the same problem occurs when a library and binary target share the same source 😅 . I guessed a more reliable way would be to update the display_name of the existing CrateSpec only when the consolidating crate is a library:

diff --git a/rust/private/rust_analyzer.bzl b/rust/private/rust_analyzer.bzl
index 7c47235..4a22f84 100644
--- a/rust/private/rust_analyzer.bzl
+++ b/rust/private/rust_analyzer.bzl
@@ -145,6 +145,7 @@ def _create_single_crate(ctx, info):
     crate["display_name"] = crate_name
     crate["edition"] = info.crate.edition
     crate["env"] = {}
+    crate["crate_type"] = info.crate.type
 
     # Switch on external/ to determine if crates are in the workspace or remote.
     # TODO: Some folks may want to override this for vendored dependencies.
diff --git a/tools/rust_analyzer/aquery.rs b/tools/rust_analyzer/aquery.rs
index c75596a..18559f7 100644
--- a/tools/rust_analyzer/aquery.rs
+++ b/tools/rust_analyzer/aquery.rs
@@ -51,6 +51,7 @@ pub struct CrateSpec {
     pub cfg: Vec<String>,
     pub env: BTreeMap<String, String>,
     pub target: String,
+    pub crate_type: String,
 }
 
 #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Deserialize)]
@@ -167,6 +168,9 @@ fn consolidate_crate_specs(crate_specs: Vec<CrateSpec>) -> anyhow::Result<BTreeS
         log::debug!("{:?}", spec);
         if let Some(existing) = consolidated_specs.get_mut(&spec.crate_id) {
             existing.deps.extend(spec.deps);
+            if spec.crate_type == "rlib" {
+                existing.display_name = spec.display_name;
+            }
         } else {
             consolidated_specs.insert(spec.crate_id.clone(), spec);
         }

For now, I will adopt this approach and send a PR with sufficient tests.

[UebelAndre]

For now, I will adopt this approach and send a PR with sufficient tests.

That would be awesome! I'd be very happy to review that 😄

I realized the same problem occurs when a library and binary target share the same source 😅

Very interesting, I wonder how rust-analyzer otherwise handles this. I'll have to do some reading some time this week 😄

[hlopko]

Yay I just hit the same problem. I'll continue on the PR.