Fall back to fuzzy because LSIF data might be sparse

[chrismwendt]

Implements RFC 49 REVIEW: Fallback when LSIF data doesn't exist

Prior to this change, if there was no LSIF results for a code intel request, no code intel would be presented to the user.

After this change:

• Hovers: if LSIF returns no hover, it'll fall back to basic-code-intel
• Definitions: if LSIF returns no definition, it'll fall back to basic-code-intel
• References: both LSIF and text search results are shown, LSIF first

Related: RFC 89 WIP: Precise Code Intel Confidence outlines a plan to make the source of code intel data more transparent to the user so they understand why they're seeing fuzzy/precise results.

[felixfbecker]

I have a few thoughts here:

• I can understand the case and reasons where this is desirable - but I would not want to add this without also having the indicator that codeintel fell back to a different source. Otherwise it would drastically reduce my confidence in codeintel and I foresee a lot of frustration with getting results that are presented as if they were precise but are actually wild and wrong guesses that are completely unrelated (this happens a lot with basic code intel in my experience).
• If precise codeintel is available, why does it not fall back to that instead of basic code intel? As a user, I want to use LSIF for e.g. TypeScript because it is fast, but when global code intel is not available because the referenced repo does not have LSIF, I would want it to fall back to precise (even if that means no speed benefit) because basic code intel is usually useless. Otherwise I would rather not use LSIF at all.
• It seems like this is specifically about cross-repo code intel, because for the local case we already have fallback mechanisms. The case described is if the referenced external repo does not have LSIF, but the local repo does. But why does it matter for LSIF whether the referenced repo has LSIF data? One of the value props of LSIF was that you generate it in CI where you already have all dependencies available. For TypeScript, that should mean for all symbols referenced from dependencies, you should know what external repo, file, line, column it is defined in, the same way that the TypeScript language server knows.

[efritz]

I agree that we should hold off on merging until we have an implementation for RFC 89 ready as well. At least to minimize the time in which results get worse before they get better.

you should know what external repo, file, line, column it is defined in, the same way that the TypeScript language server knows.

No, we know what package and version it's defined in. We aren't able to correlate that with a repository known by Sourcegraph.

[ryan-blunden]

@chrismwendt Is it correct to say this PR would undoubtedly improve the user experience for the majority of repos/dirs using LSIF due to the possible (likely?) case of not all symbols/definitions being included?

@efritz While there is additional work to be done for sure but if this will give us an incremental win for 3.11, then I would question whether we need to wait for RFC 89 before merging.

cc @christinaforney

[felixfbecker]

No, we know what package and version it's defined in. We aren't able to correlate that with a repository known by Sourcegraph.

Which is the same as what the TypeScript language server knows. It then looks at the repository field in the package.json of the dependency to know the repository, the gitHead field to figure out the exact commit hash, and uses declaration maps to figure out the file path + position. The information is all there. In LSIF, I'd expect it to do the same, except that it's precomputed/indexed. It should probably be done in lsif-npm

If you wanna see how exactly it works I actually gave a talk on it this year: https://about.sourcegraph.com/blog/advanced-typescript-tooling-talk-fosdem-2019

[felixfbecker]

@ryan-blunden imo the frustration this can cause weighs higher than the incremental improvement

[efritz]

@ryan-blunden had swayed me with an in-person chat that this is an incremental net-win for for the product and the indication of fuzzy vs precise intel can happen on its own schedule.

The main takeway is that very few users will know about LSIF at this point, and even fewer would have enabled it for a small fraction of their repositories. Adding UI elements (outside of experimental and outside of admins-only) may cause users to question the precision of LSIF/LSP results until we get the UI correct. We want to iterate this on a different schedule to get it correct while still incrementally improving code intel results.

The results returned by this PR will be strictly better than sparse LSIF alone and strictly better than basic code intel alone. In the former, we will show complete results while ordering the more precise ones on top for references, and for definitions and hovers we will get some result when we would otherwise get none. In the latter, we will show more precise results first.

My 2¢ is to

[chrismwendt]

Product's assessment is that falling back is worth doing now.

Fallback to language servers in extensions that support them is planned to be implemented in the corresponding repositories after merging this PR.

Embedding cross-repository links directly into LSIF dumps would require an order of magnitude more effort than the appetite for this problem affords.