matklad
commented
10 months ago @davidbarsky could you expand a little bit on what these extra fields actually do? This is out of my mind's L1 cache, so it's not immediately obvious to me how they could be used.
We've added these fields because determining the ManifestPath and the owning targets on the fly by querying buck adds some non-trivial, perceptible latencies
I am not entirely sure what this talks about (again, this is because at this point I remeber nothing :) ), but if this is about "what crate does foo.rs belong too" functionality (which is the first step of any analysis), that should be handled by source roots. See ProjectWorkspace::to_roots.
that a decent chunk of the information needed to successfully create a runnable
For this one in particular, I think we could do something super general, like in projec.json you could specify runnable_info: any for any target, and rust-analyzer then will just transfer this info as is over the LSP protocol for the frontent to deal with. If we want to re-use existing handling in the VS Code extension, we could be specific, and add somethign like
runnables: {
"check": ["buck", "check", "my-crate-name"],
"run": ["buck", "run", "my-crate-name"],
"test": ["buck", "test", "my-crate-name", "--"],
}where keys are a fixed set of commands with standrad semantics, and values are command-lines. note that I still believe that integrating that into flycheck would be fundamentally wrong, but if it's easy to do transparently, then we could do it (like, flycheck itself is fundamentally wrong, but we are still having it, cause its useful).
Exposing a dependency explorer
:+1: I think adding extra "display" information would be useful. Though, the danger here is mixing up things which are display only (and can be approximate fuzzy things, because human would sort them out) with things which have specific semantics (and where any fuzziness would confuse computers). We actually already have display_name in project.json for this purpose, and I think it would be fine to add display package or some such. Or, indeed, a path to manifest file, as long as it is DisplayPath(String), and not an actual path, as, I think, rust-analyzer shouldn't be interpreting that in any way.
davidbarsky
P1n3appl3
Motivations
Hi! I'm writing this issue not entirely convinced that it's the right approach, but I figured I'd at least open an issue for discussion/feedback. Anyways: We've added two fields to the rust-project.json format in the rust-analyzer/buck integration, which have been really useful. They are:
build_filecorresponding to theBUCKfile defining the crate(s), serving the same function that aManifestPathdoes today in rust-analyzer.labelcorresponding to the build system's name of the crate (details: https://buck2.build/docs/concepts/glossary/#target-label), which is roughly a Buck/Bazel-flavored URI for a build target or a more user-visible version thepackage_flagmethod incrates/project-mode/src/cargo_workspace/CargoWorkspace.We've added these fields because determining the
ManifestPathand the owning targets on the fly by querying buck adds some non-trivial, perceptible latencies (200-300 milliseconds at the average, with some substantially higher latencies at the tail) in what users would otherwise expect to be a near-instant response. Similar operations in rust-analyzer are in the low single-digit milliseconds, to its immense credit!I decided to open this issue to propose upstreaming these changes when I was working on adding generic runnable support[^1] and realized that a decent chunk of the information needed to successfully create a runnable (specifically, the manifest file) could be very easily added to a
CargoTargetSpecvia arust-project.json, enabling a set of long-tail features and functionality that I've wanted to tackle in a Buck context but was blocked by either too-high latencies or rust-analyzer's lack of visibility into this information. These features include:rust-project.json-based projects, which requireManifestPath(c.f., https://github.com/rust-lang/rust-analyzer/pull/14931)$saved_fileinterpolation is even more so), it might be feasible to replace that with building a specificCargoTargetSpec::package, which should have benefits for larger Cargo workspaces as well.Approach
I'd like to add an optional
TargetSpec(renamed fromCargoTargetSpec) toproject_model/src/project_json/Crate, where any build system that opts into providing aTargetSpecwould be required to provide all information thatTargetSpecexpects. My goal for providing this constraint is to minimize maintenance costs for rust-analyzer.Alternatives/Caveats.
I worry about proposing this feature because I worry:
rust-project.json(in that the rust-project.json format is the lowered version of a cargo graph, and this would be introducing something that is conceptually adjacent to it.)[^1]: Note that the current state of runnables can largely work if this line is commented out and the
rust-buck-companionextension queries the owner of the file in question, but that's not an approach that I'd like to take because latencies of querying a build system are too high.