Skip to content

Only add dev dependencies to the dependency list of dev targets #14166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from

Conversation

Liamolucko
Copy link

Fixes #9574
Fixes #11410

rust-analyzer already counts all of the different targets of a crate as different crates, so it was pretty easy to just exclude dev dependencies from the dependency lists of targets other than test, dev and bench when ingesting cargo metadata's output. A similar thing is already done for build dependencies.

It's possible there might be something I'm missing here, since this is quite a long-standing issue which I expected to be much more complicated to solve; but, this seems to work. The circular dependency errors that rust-analyzer used to throw on wasm-bindgen went away after switching to this patched version.

Fixes rust-lang#9574
Fixes rust-lang#11410

`rust-analyzer` already counts all of the different targets of a crate as different crates, so it was pretty easy to just exclude dev dependencies from the dependency lists of the targets other than test, dev and bench when ingesting `cargo metadata`'s output. A similar thing is already done for build dependencies.

It's possible there might be something I'm missing here, since this is quite a long-standing issue which I expected to be much more complicated to solve; but, this seems to work. The circular dependency errors that `rust-analyzer` used to throw on `wasm-bindgen` went away after switching to this patched version.
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 16, 2023
@Veykril
Copy link
Member

Veykril commented Feb 16, 2023

I don't think this works, because we analyze library targets with their tests where the test code can rely on dev-dependencies. A quick check on the impact of the analysis stats from this also seems to show this:
Baseline:

❯ cargo run --release -p rust-analyzer -q analysis-stats --memory-usage .
Database loaded:     2.38s, 217mb (metadata 279.34ms, 21mb; build 1.55s, 412kb)
  crates: 44, mods: 907, decls: 19232, fns: 14376
Item Collection:     14.20s, 471mb
  exprs: 409952, ??ty: 69 (0%), ?ty: 141 (0%), !ty: 1
Inference:           45.32s, 732mb
Total:               59.52s, 1203mb

This PR:

❯ cargo run --release -p rust-analyzer -q analysis-stats --memory-usage .
Database loaded:     1.15s, 217mb (metadata 294.54ms, 21mb; build 292.44ms, 388kb)
  crates: 44, mods: 907, decls: 19231, fns: 14376
Item Collection:     14.30s, 471mb
  exprs: 391236, ??ty: 2641 (0%), ?ty: 491 (0%), !ty: 6
Inference:           41.19s, 673mb
Total:               55.49s, 1144mb

Note the increase of unknown types (??ty).

@Veykril
Copy link
Member

Veykril commented Feb 16, 2023

I closed the linked issues (and some others) in favor of #14167, to have a more concrete tracking issue about this. In general in one of the related issues the proper fix was described which is now noted down in that issue.

I also had to change the logic for whether they get put into test mode or not:
* Bin crates now only get put into test mode if they're a part of the current workspace, because otherwise dev dependencies don't work
    * this didn't actually change anything because bin crates outside the current workspace are already ignored but idk it's better to make it explicit?
* The version of a lib crate with test mode enabled is now separate from the normal version of the lib crate, since it's allowed to depend on the normal version as a dev dependency.
  (as suggested in rust-lang#2414 (comment))
* The test-mode version of the lib crate is also only created if it's a part of the current workspace.
@Liamolucko
Copy link
Author

I tried to follow the suggestion in #14167 to treat lib crates compiled for testing separately from their normal versions, but it doesn't work very well, as rust-analyzer doesn't seem to be designed to have the same file be part of multiple crates right now. Sections with #[cfg(test)] are greyed out but still have diagnostics, the little 'run test' code lenses aren't showing up because the code that produces them is only checking the first crate in the list which isn't the test version, and probably other things that I haven't noticed.

I don't think I know enough about rust-analyzer to be the one who gets this working properly, so I'm going to close this here.

@Liamolucko Liamolucko closed this Feb 18, 2023
@Veykril
Copy link
Member

Veykril commented Feb 18, 2023

Thanks for having looked into it though, it's still appreciated :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
3 participants