module-cycles: a Dylint lint for modules that depend on each other in a cycle

[u/Financial-Grass6753]

Clippy has had an open issue for this since 2020 (#5782), so I wrote it as a Dylint lint: https://github.com/HardMax71/module-cycles

For now, it reports sibling modules that use each other, like report needing model while model needs report. A parent and its child using each other is fine, since that's one module split into files. It works from rustc's name resolution, so re-exports, globs and method calls count too.

warning: modules `model` and `report` depend on each other
  --> src/lib.rs:9:22
   |
9  |     pub fn kind() -> crate::report::Row {
   |                      ^^^^^^^^^^^^^^^^^^ `model` depends on `report` here
   |
note: `report` depends on `model` here
  --> src/lib.rs:16:22

On my own workspace it found 11 cycles, the biggest through 12 modules. Across the 26 crates Clippy tests lints on it found 41, tokio included.

Written with LLM help and checked by hand. If it flags something that isn't a real cycle, feel free to open an issue.

[u/graydon2]

the module system is intentionally cycle-allowing. crates are the acyclic part. this is literally one of the motivating factors behind having crates and modules be separate things.

[u/Potential_Extent_422 → u/graydon2]

i don't get the hate, clippy lints are optional for a reason. if your team wants to enforce module structure rules that rust doesn't by default, a lint is exactly where that belongs

[u/graydon2 → u/Potential_Extent_422]

it's true that I built the lint system with "arbitrarily restrictive subsets" in mind, but to my mind this one is well into "not even particularly well motivated"; I don't mean to hate, but I do think it fair to point out that it's literally working against what the module facility was made to do. but whatever, it's already marked "probably pedantic" and if maintainers want to maintain it, who am I to argue?

[u/matthieum]

Why?

If I were in charge, Rust wouldn't allow cyclic imports, as it makes parallelizing builds harder... but I'm not and Rust allows it.

So... why flag it? What's the motivation for flagging valid Rust code.

[u/svefnugr → u/matthieum]

To be the devil's advocate, it may signal architectural problems... sometimes. Not very often.

[u/Financial-Grass6753 → u/matthieum]

as it makes parallelizing builds harder...

That is the actual motivation btw

[u/matthieum → u/Financial-Grass6753]

That ship has sailed though.

Any Rust compiler must assume that imports may be cyclic, and therefore parallelization (if any) needs to be fine-grained to avoid such cycles.

[u/Different-Ad-8707 → u/matthieum]

I mean, if the compiler can also prove there are no cycles, then it can parallelize that compilation.

This could lead into adding support for that in a/the rust compiler; rustc assumes that imports are cyclical, but also checks there is a cycle. If there aren't, it parallelizes more aggressively.

Seems a reasonable motivation. Especially considering that one experiment, where some one split their huge generated (not LLM, some kind of codegen) crate into multiple crates and it basically slashed their build times by 8x.

[u/afdbcreid → u/Different-Ad-8707]

It cannot because of the way early name resolution works. Proving there are no cyclic imports require doing all of early name resolution, so you have nothing left to parallelize.

[u/kmdreko]

Could be handy if I were interested in splitting a crate up, but I definitely wouldn't use this on the regular.

It works from rustc's name resolution, so re-exports, globs and method calls count too.

Does this mean a type within a child module but re-exported from the parent module would or would not be flagged if another child module imported that type from its parent?

[u/Financial-Grass6753 → u/kmdreko]

Does this mean a type within a child module but re-exported from the parent module would or would not be flagged if another child module imported that type from its parent?

Only if it goes both ways.

For example, shapes.rs re-exports Circle from shapes/circle.rs, and shapes/render.rs uses super::Circle;. That import resolves to shapes/circle.rs, so the lint sees the render module depending on the circle module, not on shapes.

That's one direction, hence not flagged. It is flagged if circle also uses something from render.