Add LSIF support #124
Add LSIF support #124
Conversation
There was a problem hiding this comment.
Approving as code changes generally LGTM. I'd like to raise a couple of items for discussion:
-
How do we feel about the fact that, with this PR in, the name of this package now belies the extent of its functionality? It would now provides basic, heuristic-based code intel, but also precise code intel when LSIF is available.
-
In the language extensions that can be backed by a language server, the activation flow is currently as follows:
if (hasLanguageServerURL) {
activateLanguageServer()
} else {
activateBasicCodeIntel()
}(see sourcegraph-go, sourcegraph-typescript)
This means that by default, language servers would essentially be prioritized over LSIF when available. This is the case on sourcegraph.com. Shouldn't it be the other way around? If LSIF dumps are available, they should be preferred to all other types of code intelligence, as they're guaranteed to be precise and fast. Yet if no LSIF dump is available for a given commit, we'd still want to fall back to using the language server.
I understand that, outside of sourcegraph.com, a situation where an instance has both language servers and LSIF set up will be excessively rare, but I still feel like the prioritization of the different types of code intelligence should be as obvious/intuitive as possible.
I'll rethink this after GopherCon. Ideas:
Oh yeah, I completely overlooked this, thanks for pointing it out! I'll figure out how to set the priority to: LSIF, language server, then basic-code-intel. |
| // Prevent leaking the name of a private repository to | ||
| // Sourcegraph.com by relying on the Sourcegraph extension host's | ||
| // private repository detection, which will throw an error when | ||
| // making a GraphQL request. |
There was a problem hiding this comment.
This part of the code is not clear to me.
- How would we leak private repo names? Wouldn't we always contact the private instance?
- Why would the extension host throw an error on any GraphQL request to the private instance? How would it even intercept GraphQL requests?
There was a problem hiding this comment.
Private repo names would have been leaked by a browser extension running on private GitHub.com repos.
There's already code in the extension host to detect if the page is a private repo. This logic triggers that code to get run at the expense of an extra GraphQL request. Do you have any ideas to improve this? Maybe the extension API could expose isPrivateRepository?
| return defaultR | ||
| } | ||
|
|
||
| export function initLSIF() { |
There was a problem hiding this comment.
Could it be that there is no TSLint setup in this repo? Because this function doesn't have a return type annotation, which would be rejected by our TSLint config (and also would have made it a lot easier to understand this code, as a reviewer)
This tries LSIF before falling back to basic-code-intel (symbol and text search).
Most of the code is copied from https://github.com/sourcegraph/sourcegraph/blob/598201b6678c4819bd0f63e9095809fc34de82a2/lsif/extension/src/lsif.ts Once this is merged into all of the individual language extensions, the
sourcegraph/lsifextension in sourcegraph/sourcegraph can be deleted.