scip: Generate symbols for local crates.

[emilio]

Consider something like:

// a.rs
pub struct Foo { .. } // Foo is "local 1"

fn something() {
    crate::b::Bar::new() // Bar is "local 1", but of "b.rs"
}

// b.rs
pub struct Bar { .. } // "local 1"

Without this there's no way to disambiguate whether "local 1" references "Bar" or "Foo".

[emilio]

@tjdevries @Veykril can you review? Thanks!

[tjdevries]

@olafurpg / @varungandhi-src Is this generally how we deal w/ locals? I thought we restarted the local count for every file intentionally (particularly because of later goals of incremental compilation)

I think the actual problem must be that I missed some cases of when things should be exposed as non-local (i.e. a pub struct should not be a local, I think)

[emilio]

Right now ~everything defined in the current workspace uses a local symbol, I think. It makes sense to me off-hand, what should the symbol for Foo be in the test-case above otherwise? Happy to try to work on a better fix if given some pointers :)

[emilio]

(Given the rust compilation unit is the crate, not the file, it might be ~ok to do this regardless tho?)

[olafurpg]

Local symbols should only be used if the symbol cannot be referenced outside the file. In the case above, the pub struct Bar symbol should probably be global.

In scip-typescript, we emit global symbol for seemingly local variables. For example, see this test case where where we emit the definition of a global symbol for the key of a local object literal because the property a is part of the inferred type of the function

https://github.com/sourcegraph/scip-typescript/blob/main/snapshots/output/syntax/src/property-assignment.ts#L16

[emilio]

Ok, so I dug a bit and I think I found the root cause. The issue is that the moniker code was requiring a crate to have a repository in order to generate a moniker for it.

So if you're doing a scip index of a local crate which isn't published (which is our use case) then all symbols are incorrectly turned into local.

The fix I went for is to just allow the PackageInformation to not have a repo / version (which the lsif / scip formats allow afaict).

I added some tests to the scip code that fail without this patch.

[emilio]

An alternative would be to use unwrap_or_else(|| "file:///crate/path".into()) and unwrap_or_else(|| "v0.0.0".into()) or so.

Let me know if that's somehow preferable (though that would make the symbols for local crates non-deterministic, which would be somewhat unfortunate).

[olafurpg]

@emilio you can use a period character “.” for anonymous package names and versions. The only limitation from doing this is that you can’t support cross-repo navigation to your repo (only from your repo to external dependencies).

I can’t remember how well this is documented but scip-java uses this feature for almost all regular repositories (since cross-repo navigation works exclusively to packages where the name + version are easily accessible).

[emilio]

@emilio you can use a period character “.” for anonymous package names and versions. The only limitation from doing this is that you can’t support cross-repo navigation to your repo (only from your repo to external dependencies).

Cool, I've used the . placeholder for versionless crates, it's covered by the new tests. The code assumes all crates are named, and scip doesn't seem to use the repo url at the moment at least, so that should be all it's needed. Let me know if it looks good or more changes are needed :)

[emilio]

Gentle ping? Is there anything needed to get this reviewed? Thanks a lot in advance.

[Veykril]

I am not familiar with scip, so if anyone of the sourcegraph team could drop another small review that would be appreciated

[tjdevries]

One other option would be to allow passing the git hash of the project in for the current version -- not sure how much work that would be to do though.

Otherwise the PR looks good to me

[Veykril]

Ye I don't think we can easily do that, I'll merge this then, thanks!
@bors r+

[bors]

📌 Commit 8039a07 has been approved by Veykril

It is now in the queue for this repository.

[bors]

⌛ Testing commit 8039a07 with merge bbcb77e...

[bors]

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing bbcb77e to master...

[emilio]

Thanks!