bazelbuild / bazelbuild/rules_rust

[Bug/Question] Can we amend rust-analyzer generation crate consolidation to improve support for "cyclic" dependencies

Open
#1,589 1 comment 0 reactions 0 assignees View on GitHub
needs-triage rust-analyzer
Dominant language
Starlark
Stars
843
Forks
651
Avg merge
2d 18h
Merged PRs (30d)
15

Description

## Question
When we do crate consolidation here

https://github.com/bazelbuild/rules_rust/blob/d288ed634f9e149ceb0c81bda6cd93a810cf7e64/tools/rust_analyzer/aquery.rs#L163

on a build graph like so (`A`, `B`, `C` are crates)
```
rust_test(C) -> rust_library(A) -> rust_library(B) -> rust_library(C)
```

`rust_test(C)` and `rust_library(C)` get their dependencies merged, and the result of that is that we can't actually generate `rust-project.json`, as now we have a real dependency cycle.

Here's a reproducer - https://github.com/googleson78/rust-analyzer-bazel-gen-repro

It turns out that having both of them listed in `rust-project.json` is not a big issue and somewhat works (not only in my simple reproducer, but also in the bigger codebase I mention in the context below):

https://github.com/googleson78/rust-analyzer-bazel-gen-repro/blob/main/somewhat-working-rust-project.json

Can we somehow loosen consolidation so that we avoid this situation?

The solution I can think of that sounds the best is to add separate entries for `rust_test(C)` and `rust_library(C)` iff merging them would cause a circular dependency. This might not be that easy to implement however, it would require a lot of computing possibly.

The other extreme is to stop doing crate consolidation entirely. I couldn't find a reason for why it's done in the first place, so I've come to ask - can someone clarify this?

## Context

In the [Dfinity code base](https://github.com/dfinity/ic/tree/master/rs), the dependency situation from above happens, and prevents us from generating a `rust-project.json`

By replacing the implementation of `consolidate_crate_specs` with
```rust
fn consolidate_crate_specs(crate_specs: Vec) -> anyhow::Result> {
Ok(BTreeSet::from_iter(crate_specs))
}
```
generation now works, and the result appears to be at least partially usable - I tried it on a few files (including some of those which form a "cycle") and IDE features appear to work.

## Additional remark
By doing this generation we can even do better than rust-analyzer's builtin Cargo support!

We seemingly somehow support the cyclic dependency if we generate duplicate entries in `rust-project.json` - goto def works in all cases in the reproducer project, even from the "problematic" test in C, for which the builtin cargo based rust-analyzer support fails. See https://github.com/rust-lang/rust-analyzer/issues/13349 for more details.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.