rust-analyzer
rust-analyzer copied to clipboard
rust-lang
feat: emit SCIP from rust-analyzer
hi rust-analyzer team
I'm one of the engineers at Sourcegraph (and have done a few small changes related to the LSIF work done in rust-analyzer). Recently, we've moved to a new protocol as the primary way to interact with Sourcegraph (LSIF is still possible to upload, so existing jobs will not stop working any time soon). This new protocol is SCIP (I linked a blog post below with more information).
I've implemented SCIP support (based largely on the existing LSIF support). In addition to supporting the existing features that rust-analyzer's LSIF support does, this PR adds the ability to move between crates on sourcegraph.com. So if both your project and a dependency are indexed, you would be able to hop to the particular version and view the source code. I'd be happy to record a demo of that on my local instance if you're interested.
There are a few TODO's left in the code (some that you might have insights on) which I'm happy to fix in this PR, but I just wanted to open this up for discussion first.
Thanks for your time :)
TJ
How are you planning to license scip? You might want to publish a version to crates.io and add a license descriptor.
That said, you might see some push-back against this. I don't have a strong opinion either way, though.
Does the protobuf crate require protoc to be installed? rust-analyzer is also built as part of rust-lang/rust, so we'd have to coordinate with t-infra.
This actually makes me wonder, is there any reason for us to have LSIF or SCIP be part of rust analyzer? To me it somewhat sounds like those are things that can just be built on top of the rust-analyzer libraries (assuming we expose whats required)? (Maybe this was asked in the LSIF introducing PR before though)
I am a bit reluctant here because I don't really see why r-a would ever have to rely (transitively or directly) on protobuf for example.
How are you planning to license
scip? You might want to publish a version to crates.io and add a license descriptor.
Currently, we have Apache License 2 ( https://github.com/sourcegraph/scip/blob/main/LICENSE ) for SCIP. We can try and get something published to crates.io, we just don't have an account for Sourcegraph there, etc. so it will require a little work on my end to make that happen (which is fine, especially if it ends up being blocking for the PR).
That said, you might see some push-back against this. I don't have a strong opinion either way, though.
Does the
protobufcrate requireprotocto be installed?rust-analyzeris also built as part ofrust-lang/rust, so we'd have to coordinate witht-infra.
As far as I know, it does not require protoc to be installed, but I can follow up on that. I think protobuf crate is pure Rust crate (at least from what I've seen, I'm not a protobuf expert).
This actually makes me wonder, is there any reason for us to have LSIF or SCIP be part of rust analyzer? To me it somewhat sounds like those are things that can just be built on top of the rust-analyzer libraries (assuming we expose whats required)? (Maybe this was asked in the LSIF introducing PR before though)
I would be perfectly fine doing this in an external crate (scip-rust) owned and maintained by Sourcegraph -- we would just have to talk about what crates need to be published. An alternative approach is we could maintain a fork of rust-analyzer for this project (I just prefer not doing that as it adds a lot of overhead work and confusion for people consuming the project).
I suppose one alternative as well would be hiding both SCIP and LSIF support behind a feature flag, so that most people would not compile the feature (and/or have to worry about the dependencies).
(also, if you'd prefer we can talk about this on Zulip and link to the thread from here for easier back-and-forth).
We try to publish most of the crates, although that's currently broken (but fixable, I suppose). I agree that doing this externally would be nicer.
We are publishing all the r-a crates, it's just that usually only what the other r-a crates need is publicly exposed, so some stuff might be private that LSIF/SCIP might need or not, in which case a PR to make something public/exposed in a way is fine. My main point is that this feels like a tool that is not really IDE specific, and hence I believe it has no need to be part of the rust-analyzer binary itself (LSIF might a bit different here but I don't remember, will need to read up on that again).
(also, if you'd prefer we can talk about this on Zulip and link to the thread from here for easier back-and-forth).
Either works, zulip is certainly more convenient than github.
We try to publish most of the crates, although that's currently broken (but fixable, I suppose). I agree that doing this externally would be nicer.
Didn't we fix this?
We did, but there some mismatch with the features added for the in-tree build. I haven't looked into it yet.
Given that @matklad reviewed the LSIF PR https://github.com/rust-lang/rust-analyzer/pull/10181 (with a lot of excitement), maybe my stance regarding this is wrong and this has a place here (as well as LSIF). Pinging so that you get to have a say here before any actual decisions happen :)
Maybe r-a becoming a big swiss army knife of tools is fine, in which case having these things live in the main binary works fine. But I personally don't really see a benefit in that as these things look like they can just live as their own binaries just fine as well. I'm a bit split on the matter to be completely honestly.
Edit: So LSIF is actually part of the lsp protocol (I forgot this), so with that said LSIF being part of r-a makes sense I suppose, given the same doesn't apply to SCIP I would say that one should probably live out of tree with us exposing all the tools needed to make it work.
So LSIF is actually part of the lsp protocol (I forgot this), so with that said LSIF being part of r-a makes sense I suppose, given the same doesn't apply to SCIP I would say that one should probably live out of tree with us exposing all the tools needed to make it work.
Cool, I will see if I can make it completely out of tree. We can close this (unless someone would like it in-tree). The reason I originally proposed this PR was because for LSIF, usage on Sourcegraph was explicitly mentioned as one of the reasons that people were excited about the rust-analyzer lsif ability. Totally fine having it live elsewhere though :)
Realized I never responded to this:
That being said, if we keep this in tree, than:
- it is on you to maintain it
- we don't give any compatibility guarantees, and in the future we might decide, that, in the end, it's better not to keep this in tree.
Sounds good to me. I don't expect r-a to make any fixes/improvements on the code here (and if new enums are added to definitions and other items, you can fallback to doing nothing with them for now and I could fix them later).
Also, if you end up deciding you want it in another repo that's totally fine too. I did an experiment with that yesterday and it was 95-ish% able to be done.
Also, I've published a release of scip on crates.io and removed the direct dependency on protobuf (so that it can't be used throughout r-a). Are there any other concerns you'd like me to tackle?
ah, woops, one sec (i am trying to rebase on top of master to make sure everything is OK)
:pushpin: Commit 50ecb09da460b58d1f62a2e0b8f7b4b52aa76139 has been approved by Veykril
It is now in the queue for this repository.
:hourglass: Testing commit 50ecb09da460b58d1f62a2e0b8f7b4b52aa76139 with merge e8a86b66c4c4578e7c430c0dc333b078b98f9a19...
:hourglass: Testing commit 50ecb09da460b58d1f62a2e0b8f7b4b52aa76139 with merge b2bf37cdde277472b9ad913cbf9ed28108d629ea...
:sunny: Test successful - checks-actions Approved by: Veykril Pushing b2bf37cdde277472b9ad913cbf9ed28108d629ea to master...