rules_rust icon indicating copy to clipboard operation
rules_rust copied to clipboard

Integration with rust-analyzer's automatic project discovery

Open davidbarsky opened this issue 1 year ago • 8 comments

Hi folks! I added Cargo-style automatic project discovery in rust-analyzer for non-Cargo build systems (Buck primarily, but it's designed to be usable by Bazel...) in https://github.com/rust-lang/rust-analyzer/pull/17246. The documentation for this feature is in the manual, under "rust-analyzer.workspace.discoverConfig". I've been using it with Buck for the last month and a half, and while some details are still likely to change, the general shape of the API is in a spot that I'm happy with. I think this would make the Rust IDE experience with Bazel a lot nicer and I'd be very happy to provide guidance on integrating this feature into rules_rust!

davidbarsky avatar Jul 22 '24 14:07 davidbarsky

For reference, the Buck-side implementation of this can be found here: https://github.com/facebook/buck2/tree/main/integrations/rust-project/src

jondo2010 avatar Jul 22 '24 14:07 jondo2010

Hi, @davidbarsky I read a few of the discussions on RAtoml (including: https://github.com/rust-lang/rust-analyzer/pull/17246). But I don't fully understand the concept/advantages of ra's automatic project discovery. Would you mind explaining? In what ways will it improve the Rust Bazel IDE experience?

otiv-emiel-vanseveren avatar Jul 23 '24 10:07 otiv-emiel-vanseveren

But I don't fully understand the concept/advantages of ra's automatic project discovery. Would you mind explaining? In what ways will it improve the Rust Bazel IDE experience?

I think it improves the situation in two major ways:

  1. For VS Code, Bazel directs users to setup a task that generates a rust-project.json for the entire workspace. In my opinion as a rust-analyzer team member, this has two downsides:
    1. Generating a rust-project.json for the entire workspace might not scale to larger workspaces. I don't believe this is an inherent limitation to @rules_rust//tools/rust_analyzer:gen_rust_project , but https://github.com/rust-lang/rust-analyzer/pull/17246 can be substantially more fine-grained with what is and isn't indexed.
    2. @rules_rust//tools/rust_analyzer:gen_rust_project runs on editor starting up, but doesn't necessarily refresh the workspace if dependencies are added or removed. The PR I linked to can reload when a BUILD file changes.
  2. Since the functionality lives in the rust-analyzer server, support across editors wouldn't require per-editor work—the required integrations are O(build_system), not O(build system * editor). I also think that the video I attached to the PR is pretty nice—it looks decently Cargo-like.

davidbarsky avatar Jul 23 '24 13:07 davidbarsky

Thanks for the explanation!

  1. Generating a rust-project.json for the entire workspace might not scale to larger workspaces. I don't believe this is an inherent limitation to @rules_rust//tools/rust_analyzer:gen_rust_project

Does this mean it could generate based on the open buffers / crate you're currently in ?

otiv-emiel-vanseveren avatar Jul 23 '24 13:07 otiv-emiel-vanseveren

Does this mean it could generate based on the open buffers / crate you're currently in ?

Yes, but you can still can still generate a rust-project.json for the entire workspace if somthing like /... is provided as an argument to the command—the scaling is just a bit more graceful.

davidbarsky avatar Jul 23 '24 14:07 davidbarsky

Thanks for the pointer @davidbarsky, this looks really nice!

I'd happily review a contribution if someone wanted to put one together :)

illicitonion avatar Jul 29 '24 14:07 illicitonion

Hi @davidbarsky, I’d like to contribute, but I’ll need some guidance. It looks like I'll need to update the existing rust-project-gen similar to the one Buck has (reference above)? mainly what buck's rust-project develop(+json) and check cmds implement? What else is needed?

Does rust-analyzer provide the currently edited files?

otiv-emiel-vanseveren avatar Aug 15 '24 17:08 otiv-emiel-vanseveren

Sorry, I thought I replied, @otiv-emiel-vanseveren.

It looks like I'll need to update the existing rust-project-gen similar to the one Buck has (reference above)?

Yup!

mainly what buck's rust-project develop(+json) and check cmds implement? What else is needed? That's all there is! In the manual, look for rust-analyzer.workspace.discoverConfig.

davidbarsky avatar Oct 11 '24 17:10 davidbarsky

This is great :-) I have a hacked up script that works-ish: https://gist.github.com/sam-mccall/1d0dedf61f45529394f4ae54345c0d25. It walks up from root/a/b/c/d.rs to find root/a/b/BUILD, and then runs gen_rust_project //a/b:all and returns the result to rust_analyzer.

The main awkwardness is relative paths: rust-analyzer interprets them relative to the BUILD file (virtual root/a/b/rust-project.json) Bazel would really prefer paths to be relative to the workspace root (virtual root/rust-project.json). The latter seems more natural because the paths aren't all in root/a/b/* but also dependencies root/c/d/* and also root/sysroot/*.

@davidbarsky I guess Buck has the same issue and forcing the paths to be relative anyway (../../sysroot/*) is the thing to do?

(My script just pokes around making all the paths absolute, which is ugly but works. We can't generate absolute paths in the first place, because the hermetic build doesn't know its location)

sam-mccall avatar Nov 29 '24 02:11 sam-mccall

@sam-mccall

As a preface, I don't really understand or know Blaze/Bazel, so I might say things that don't really make sense or aren't applicable.

I have a hacked up script that works-ish: https://gist.github.com/sam-mccall/1d0dedf61f45529394f4ae54345c0d25. It walks up from root/a/b/c/d.rs to find root/a/b/BUILD, and then runs gen_rust_project //a/b:all and returns the result to rust_analyzer.

Nice!

The main awkwardness is relative paths: rust-analyzer interprets them relative to the BUILD file (virtual root/a/b/rust-project.json) Bazel would really prefer paths to be relative to the workspace root (virtual root/rust-project.json). The latter seems more natural because the paths aren't all in root/a/b/* but also dependencies root/c/d/* and also root/sysroot/*.

A question and a comment:

  • Question: in those paths, root is @, right?
  • Comment: rust-analyzer can interpret paths in two ways: relative to the BUILD or as absolute paths. I could see an argument for a third (relative to @) inside of rust-analyzer for Bazel and Buck-style build systems, or for virtualized file systems, where you can't necessarily materialize files onto a traditional file system.

I guess Buck has the same issue and forcing the paths to be relative anyway (../../sysroot/*) is the thing to do?

It does have the same issue, but no: we force all paths to be absolute in two steps:

  1. When inside of Buck with a BXL script (think adhoc build graph analysis/exploration, but written in Starlark), we make all the paths relative to the the project root ("@").
    1. Buck's "project root" is equivalent to Bazel/Blaze's "workspace root". I don't know if Bazel/Blaze have this feature, but I can find Buck's project root by running buck root --kind=project.
  2. Once the BXL script returns to the rust-project Rust CLI that invoked it, the rust-project CLI typically joins the all the paths with the project root. We only keep the paths relative to the workspace root when we generate a Glean index.

This allows us to keep the core, heavy work of generating a rust-project.json hermetic, but have impure post-processing/telemetry/etc. live in something that doesn't care about hermeticity. Let me know if the thing I described can work for Bazel/Blaze: if not, we can explore adding a third option to rust-analyzer (relative to @).

davidbarsky avatar Nov 29 '24 17:11 davidbarsky

Thanks David!

I have a hacked up script that works-ish: https://gist.github.com/sam-mccall/1d0dedf61f45529394f4ae54345c0d25. It walks up from root/a/b/c/d.rs to find root/a/b/BUILD, and then runs gen_rust_project //a/b:all and returns the result to rust_analyzer.

  • Question: in those paths, root is @, right?

Yeah, I think technically root/ in the filesystem is @// in blaze-label terms (main repository, root). But I think multi-repo is not so relevant here, so I'd usually just call it // (current repository, root).

(To be honest, I live in our monorepo and don't understand multi-repo too well)

I guess Buck has the same issue and forcing the paths to be relative anyway (../../sysroot/*) is the thing to do?

It does have the same issue, but no: we force all paths to be absolute in two steps:

Ah, that seems easier. The existing gen_rust_project tool already does this as you describe for generated files (which aren't in the workspace) - the hermetic piece emits placeholders which the driver piece fills in. Reusing this for paths inside the workspace sounds good.

The structure we have is very similar to yours: hermetic piece of aspect/BXL, driver glue of gen_rust_project/rust-project.

I could see an argument for a third (relative to @) inside of rust-analyzer for Bazel and Buck-style build systems, or for virtualized file systems, where you can't necessarily materialize files onto a traditional file system. ... We only keep the paths relative to the workspace root when we generate a Glean index.

As it happens I'm also trying to set up rust-analyzer based static analysis, which will use VFS, but I think we can just pick a fixed concrete layout for the VFS (e.g. /src, /generated, /sysroot) and modify the driver to fill those into the placeholders.

sam-mccall avatar Dec 02 '24 02:12 sam-mccall

@otiv-emiel-vanseveren are you currently working on this?

It looks like there are a few separable pieces:

  • absolute file paths as discussed above (I sent #3033)
  • running a separate --output_base bazel instance so the tool doesn't fight with the user over the bazel lock
  • make the tool work outside bazel run so it can be invoked by rust-analyzer
  • speak the required argv/stdout protocol

I'd like to contribute whatever's needed to get this working. Please let me know if there are things you have in flight!

sam-mccall avatar Dec 02 '24 03:12 sam-mccall

Hi @sam-mccall, unfortunately I did not have the time yet. Looking forward to your contributions!

Please let me know if there are things you have in flight!

I initially started working on something very similar to buck's RA cli tool, the one in rules_rust is not that nice.

otiv-emiel-vanseveren avatar Dec 02 '24 17:12 otiv-emiel-vanseveren

@sam-mccall

Yeah, I think technically root/ in the filesystem is @// in blaze-label terms (main repository, root). But I think multi-repo is not so relevant here, so I'd usually just call it // (current repository, root).

(To be honest, I live in our monorepo and don't understand multi-repo too well)

Thankfully, it's not too hard to support multiple repos! We also technically support multiple repos, but if you know where your impure driver is being invoked from, it's generally pretty easy to normalize the paths to the correct repo root. We do this in Buck's rust-project because people might have multiple checkouts of fbsource for one reason or another, and multiple checkouts are cheap.

Ah, that seems easier. The existing gen_rust_project tool already does this as you describe for generated files (which aren't in the workspace) - the hermetic piece emits placeholders which the driver piece fills in. Reusing this for paths inside the workspace sounds good.

The structure we have is very similar to yours: hermetic piece of aspect/BXL, driver glue of gen_rust_project/rust-project.

Cool, yeah! I'm glad we converged on the same solution!

As it happens I'm also trying to set up rust-analyzer based static analysis, which will use VFS, but I think we can just pick a fixed concrete layout for the VFS (e.g. /src, /generated, /sysroot) and modify the driver to fill those into the placeholders.

Yeah, for Glean—code indexing—it's static analysis if you squint and we simply make it relative to the repo root. Everything is keyed off fbsource (e.g., fbsource/fbcode, fbsource/third-party, etc. I forget where the sysroot lives, but it's one of those folders and it's enough).

speak the required argv/stdout protocol

Lemme know how that protocol feels: I'm happy to change/evolve it in response to feedback! It was very Buck-centric, so if anything feels off, let me know!

davidbarsky avatar Dec 04 '24 15:12 davidbarsky