dev-dependencies on workspace member cause cyclic dependency issues

[Veykril]

Note: We (rust-analyzer) consider this issue resolved in terms of what we can do here.

This issue is spread across several others, so let's make one big one out of it to better track it.

#2414 (comment)

Yeah, the problem is that a crate can have a dev-dep onto itself:

[package]
name = "foo"
version = "0.1.0"
edition = "2018"

[dev-dependencies]
foo = { path = "." }

The solution here is to start "duplicating" crates when lowering cargo-metada output to CrateGraph. In the above example, we should construct two foo crates, one with cfg(test) and one without, and make the required depednency edge.

Issues in question:
#8330
#2414
#12407
#9574
#11410

[GilShoshan94]

Is it the same issue for crates that export proc_macro with other crates ?

I have a main crate foo, and another one foo-derive with the proc_macro.
I wanted to test foo-derive inside itself and to not have the tests moved to foo for the derive macro only.
But I need foo in the tests since the generate code used structs and traits from foo public API.

In foo-derive Cargo.toml:

[package]
name = "foo-derive"
version = "0.1.0"
edition = "2021"

[lib]
proc-macro = true

[dependencies]
proc-macro2 = "1"
quote = "1"
syn = { version = "1", features = ["full"] }

[dev-dependencies]
foo = { path = "../foo" }
trybuild = "1"

Is it inherently wrong to do that ?

Beside an error (appears twice in a row) in the output of Rust Analyzer Language Server in vscode

[ERROR project_model::workspace] cyclic deps: foo_derive(CrateId(76)) -> foo(CrateId(69)), alternative path: foo(CrateId(69)) -> foo_derive(CrateId(76))

Everything seems to work well.

[Veykril]

It is not a wrong thing to do that, it's just r-a struggling with it (so yes, you are hitting this here). If everything works fine you can safely ignore the error.

[Veykril]

So duplicating the crates causes a bunch of issues, especially for traits and their implementations, as we can possibly have types in scope that implement the original trait, but have the duplicated on in scope. And a lot of other issues similar to that.

So now I'm wondering, the reason for why we want to prevent cycles is obviously graph traversal which makes me wonder if wen can lower dev dependencies to some kind of "weak edge" that is skipped when traversing the graph except for name resolution.

[Veykril]

Hmm, so that approach runs into the problem that we'll have a cycle in the defmap creation ...

[flodiebold]

Yeah. 🤔 How exactly do cyclic dev dependencies work with Cargo? I'm not sure why it doesn't run into the same problem?

[Veykril]

Right, that should occur there as well then 🤔. Also would be interesting to hear how the jetbrains plugin deals with this. @Undin @vlad20012 I hope you don't mind the ping, but if you could shed some light on how intellij-rust deals with workspace dev dependency cycles that would be appreciated :) (assuming it deals with them at all)

@matklad maybe you have some more ideas here as well

[matklad]

How exactly do cyclic dev dependencies work with Cargo? I'm not sure why it doesn't run into the same problem?

So the core thing here is that there are packages and crates. We physically can not have cyclic dependencies between creates, cause they need to be compiled in topological order.

I believe cargo has some logic to prevent cyclic dependencies between packages (eg, if you literally add a cyclic dep, cargo will bail out), but that can be circumvented via a dev dependency. Adding a dev-dep on oneslef is forbidden (an explicit check in cargo), but adding a self-dev-dep via a third package works.

I wonder if we can just, like, ban this in cargo in an edition boundary? Like, this 100% works today, and people are relying on that, but semantics are confusing (you basically have two copies of every trait/type in scope), and compilation-time hit is big.

My gut feeling is that we should add an off-by-default flag, which models cargo semantics precisely, by essentially duplicating the crate graph (every crate gets variants with and without —-test). This we can use as a testbed to iron out any issues with the single file belonging to more than one crate. Yes, that’s going to be hugely confusing, because there’s going to be two copies of every crate and what not, but that’s what is actually physically happening. With that flag in pace, we can have a better error message here saying “hey, we noticed you have cyclic dependencies, would you like to fix your code, or to check this checkbox?”

[matklad]

you basically have two copies of every trait/type in scope

Perhaps more importantly, you also literally have two independent copies of the crate in the final binary

[vlad20012]

Also would be interesting to hear how the jetbrains plugin deals with this

We just carefully remove such dev-dependencies graph edges that lead to a cycle (during the lowering from a cargo project model to a crate graph). Then we suppress errors in cfg(test) items and #[test] functions inside a crate with a removed dev dependency

[bragov4ik]

Related to rust-lang/rust#79381
Just thought it makes sense to link it here :)

[Veykril]

Fyi, #16871 followed by #16886 implement the approach of preferring non-dev depedency edges if possible, minimizing the problems we encounter with this

[ian-h-chamberlain]

Hi, just found this issue as I was trying to figure out why I was seeing an unresolved-extern-crate error in a project. I think this is what it comes down to:

The Bathwater: Cycles in dev deps is not that useful for unit tests, which is also where the problem lies. This can be safely thrown out, I think.

In our case, we actually do rely on this kind of cycle in https://github.com/rust3ds/ctru-rs, but maybe there's a way we could work around it? Here's how our (trimmed for clarity) dependency tree looks:

ctru-sys v0.5.0
└── libc v0.2.154
[dev-dependencies]
├── shim-3ds v0.1.0 (https://github.com/rust3ds/shim-3ds.git#98015084)
│   ├── ctru-sys v0.5.0 (*)
│   └── libc v0.2.154
└── test-runner v0.1.0
    ├── ctru-rs v0.7.1
    │   ├── ctru-sys v0.5.0 (*)
    └── ctru-sys v0.5.0 (*)

There are two cases here:

test-runner, which is a helper crate for use with custom_test_frameworks, is required for our unit tests to work properly — but it in turn relies on the ctru-sys crate (directly and indirectly) to implement its functionality. This works fine as r-a doesn't seem to know or care about the custom test framework.

shim-3ds is the crate which triggers unresolved-extern-crate. We use it to fill in some libc definitions at link time, so the only usage is (still needed for unit tests) just this:

#[cfg(test)]
extern crate shim_3ds;

In both cases, we use [patch] in Cargo.toml to ensure the local versions of ctru-rs/ctru-sys are used in the graph, meaning we end up with a #[cfg(test)] and a #[cfg(not(test))] version of ctru-sys, but it works for our testing!

---

From what I can tell, this is the kind of use case that is being considered as "not really useful", is that right? Is there maybe some other way we can achieve the same thing that would play nicer with r-a, or something? I guess something like #[cfg(all(test, not(rust_analyzer)))] might work just to get rid of the squiggly, but otherwise I'm not sure if there's a simple alternative to our current setup.

[Manishearth]

From what I can tell, this is the kind of use case that is being considered as "not really useful", is that right?

No, this was out of scope for my analysis, mostly because custom test frameworks are even weirder and already not supported by rust-analyzer. This may be a legitimate use case but it's legitimate because it's via custom test frameworks.

shim-3ds is the crate which triggers unresolved-extern-crate. We use it to fill in some libc definitions at link time, so the only usage is (still needed for unit tests) just this

This seems less good, since you end up with multiple pairs of types. This is probably better served by slicing the deptree a bit more.

[ian-h-chamberlain]

This seems less good, since you end up with multiple pairs of types. This is probably better served by slicing the deptree a bit more.

Hmm, do you mean like if we tried to re-export the types or something, we'd end up with two different versions of the crate's types/functions? In our case that's fine, because we don't actually call/reference any of those transitively via shim-3ds.

As a small workaround without requiring full support from r-a, I wonder if our particular situation could be helped out by allowing dependencies up to the point that a cycle is created, rather than eliminating the dependency branch entirely? Without knowing too much about the implementation, it seems like r-a is disabling this edge (marked X) when it detects the cycle:

ctru-sys v0.5.0
[dev-dependencies]
└─X shim-3ds v0.1.0 (https://github.com/rust3ds/shim-3ds.git#98015084)
    ├── ctru-sys v0.5.0 (*)
    └── libc v0.2.154

Would it be possible instead to ignore this one instead? It would allow for more symbols to be resolved, and in our crate just silence the one error we have for extern crate shim_3ds:

ctru-sys v0.5.0
[dev-dependencies]
└── shim-3ds v0.1.0 (https://github.com/rust3ds/shim-3ds.git#98015084)
    ├─X ctru-sys v0.5.0 (*)
    └── libc v0.2.154

It probably is possible for us to restructure our crates to avoid this cycle, but just wondering if that would be a more general "partial solution" for cases similar to ours where Cargo does allow a cycle like that.

[Manishearth]

It probably is possible for us to restructure our crates to avoid this cycle

I think there are a lot of benefits of doing that anyway.

OTOH I don't see a huge benefit of making this rather niche case work.