Skip to content
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

Remove accidental cargo knowledge from CrateGraph deduplication #16170

Closed
Veykril opened this issue Dec 20, 2023 · 4 comments · Fixed by #16586
Closed

Remove accidental cargo knowledge from CrateGraph deduplication #16170

Veykril opened this issue Dec 20, 2023 · 4 comments · Fixed by #16586
Assignees
Labels
Broken Window Bugs / technical debt to be addressed immediately

Comments

@Veykril
Copy link
Member

Veykril commented Dec 20, 2023

.., but I would say this is a significant architectural bug.

By design, this layer of rust-analyzer knows nothing about cargo specific concepts. dev-vs-build-vs normal is 100% Cargo concept. rustc knows nothing about these words. As such, any special handling of these concepts should happen in workspace.rs, not here.

The motivation for this design is two-fold:

  • practically, Cargo is a build system, there might be others. In the core analysis parts, we shouldn't be prioritizing Cargo over other (current and future) build systems
  • theoretically, it is important that the internal objects in rust-analyzer's semantic model reflect the physical reality of rustc. In that physical reality, cargo packages do not exists (and -dev as a concept applies to a package), there are only units of compilation.

Originally posted by @matklad in #15754 (comment)

@Veykril Veykril changed the title Please don't take this too seriously, as I have retreated to my Zig ivory tower in Lisbon some years ago and am mostly out of touch with the current goings of rust-analyzer, but I would say this is a significant architectural bug. Remove accidental cargo knowledge from CrateGraph deduplication Dec 20, 2023
@Veykril Veykril added the Broken Window Bugs / technical debt to be addressed immediately label Dec 20, 2023
@alibektas
Copy link
Member

@rustbot claim

@alibektas
Copy link
Member

So what has been bugging me with this issue is

  1. We need to filter what is from Cargo's perspective a DevDependency out of a crate's dependencies so that our new deduplication mechanism works properly.
  2. DevDependency is a cargo property this as mentioned in the issue. So it is not a base-db matter it belongs to a less abstract layer.
  3. We do not have a multi-phased processing for crates : what we have at hand is immediately turned into a CrateData.

How can I ever make queries like "Get me all the dev-dependencies on this CrateData on this CrateGraph if CrateGraph is the one and only representation we have at our disposal? The only solution I can see to this problem is to have an intermediary phase that acts as a bridge between project-model and base-db::input and is 100% build system ( cargo ) specific.

@Veykril
Copy link
Member Author

Veykril commented Jan 9, 2024

(repeating from dms)

Idea would be to have Workspace::to_crate_graph spit out an additional mapping that reveals dev-dependencies in a some fashion (empty for anytihng but cargo workspaces), then CrateGraph::extend can take a call back that looks something like impl Fn(&CrateData, &CrateData) -> Option<CrateData> which is responsible for doing the merge/deduplication so the caller can decide whether to do so. That's where the map then comes in. That should allow lifting the knowledge out of the CrateGraph.

@Veykril
Copy link
Member Author

Veykril commented Jan 9, 2024

On a related note, CrateOrigin kind of has a similar issue, as it contains repo: Option<String> which is technically speaking also cargo specific I think (iirc we added that for lsif).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Broken Window Bugs / technical debt to be addressed immediately
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants