Activation refactor

[efritz]

This repo as well as the go extension and typescript extension share much of the same logic:

• falling back from LSIF to search-based results
• adding UI indicators when results are not precise
• tracing and event emission for precise/imprecise actions (coming up soon)

In each of these three efforts (the introduction of LSIF, the introduction of UI indicators, and the introduction of trace events) required symmetric work in order to maintain the same behavior across extensions.

This PR makes an effort to bring all of the common logic to one place. This will simplify the setup of the Go and Typescript extensions, as well as the extensions produced from ./template. This brings LSP providers into basic code intel as an optional concept, which will take precedence over search-based results (but not over LSIF results).

See sourcegraph/sourcegraph-go#117 and sourcegraph/sourcegraph-typescript#308 for how the other extensions will use this new initialization code.

This PR will be followed by a major cleanup.

[efritz]

Note: this logic isn't great but it's what currently exists for Go and Typescript. We should favor fast LSIF results first, then LSP results, then search results (if LSP is provided). This is possible now that we've updated our providers to be AsyncGenerators.

[efritz]

The following code was lifted from the typescript extension.

[chrismwendt]

I verified the following behavior remains unchanged:

• On a repo missing LSIF
  • Hover: an LSIF hover is attempted, fails, and falls back to search-based code intel
  • Definition: an LSIF definition is attempted, fails, and falls back to search-based code intel
  • References: both an LSIF references request and search request are made, LSIF fails and all results are search-based
• On a repo with LSIF
  • Hover: LSIF hover succeeds
  • Definition: LSIF definition succeeds
  • References: both an LSIF references request and search request are made, both succeed and LSIF results are shown first

(I'll do LSP checks for the Go/TypeScript repos)

[felixfbecker]

It would be nice if the functions etc had docblocks :)

[felixfbecker]

One big change I see in this is that extensions are no longer in control of provider registration, but can only pass handlers to here. I think this precludes extensions from registering providers dynamically. In LSP version 3.x, all capabilities are actually registered dynamically (which can be based on the project), which was also implemented in https://sourcegraph.com/github.com/sourcegraph/lsp-client by registering providers in response to language server registrations. The server can specify the document selector to register the capability for, so this maps nicely to registering providers directly and can save round trips as the main thread can already pre-filter whether a provider should be called or not.

A very simple use case for the dynamic registration is registering/unregistering all providers depending on whether a server is configured or not.

Maybe this could be written in a less imperative way, instead more reactive? E.g. an idea would be to allow Observables for each provider (that can emit new provider args or null if the provider should be unregistered).

Or, make the API basically compatible with sourcegaph.languages, so that it can be used to easily wrap any other code that currently registers providers directly with the Sourcegraph API.

[felixfbecker]

These interfaces all look very much the same. Is there a way to avoid the duplication?

[efritz]

They're different for now because we'll be adding support for LSIF pagination, etc, and it would be good to have the flexibility in the types.

[efritz]

They will be unified if they're obviously similar after doing some cleanup around reference merging (the current behavior of which is very suboptimal: it always uses LSP if a language server is configured, skipping LSIF entirely).

[efritz]

Or, make the API basically compatible with sourcegaph.languages, so that it can be used to easily wrap any other code that currently registers providers directly with the Sourcegraph API.

This sounds like a fairly easy solution.

[efritz]

One big change I see in this is that extensions are no longer in control of provider registration, but can only pass handlers to here. I think this precludes extensions from registering providers dynamically.

I've updated it so that only defs/refs/hovers need to be supplied, which are the only providers that use search-based fallbacks and the only providers for which LSIF is currently useful.

A very simple use case for the dynamic registration is registering/unregistering all providers depending on whether a server is configured or not.

Also related: your browser extension comment from the PR using this new interface in the TS extension.

@chrismwendt confirmed that an extension re-activates on each page reload, and each tab has its own extension instance.

He also noted that the extension client will not pass settings configured via the site-admin interface/user settings interface automatically, and only the things that are changed as part of a updateConfiguration invocation are pushed to the extension dynamically. So it appears that we need to check the {go,ts}.serverUrl config params on startup anyway, as that's the only value we get.

Is this incorrect?

Maybe this could be written in a less imperative way, instead more reactive? E.g. an idea would be to allow Observables for each provider (that can emit new provider args or null if the provider should be unregistered).

I'm seeing less utility for dynamic registration given my understanding in the point above. Why would you ever want to unregister or re-register a new LSP provider for defs/refs/hovers? (I've since removed the support for external refs/locations, and those belong solely to sourcegraph-{go,ts} extensions again, and they can be bound independently/dynamically by the extension as needed).

Or, make the API basically compatible with sourcegaph.languages, so that it can be used to easily wrap any other code that currently registers providers directly with the Sourcegraph API.

Tried to run through how we would do this. It would be simple to have a proxy defining all of the provide* methods that would inject LSIF before and search-based code after the LSP provider, but this gets really magic (in a bad way). We'd also need to be somewhat aware of how document selectors match registered providers, which would duplicate some of the logic inside sg/sg.

I'm not sure it's necessary given my understanding above, and if so it seems to be so at a rather high complexity cost.

---

I'm starting to see this as the first (tiny) step in a larger effort to unify all of our code intel extensions. This change does not seem to regress on any behavior, makes a step in the right direction to consolidate logic, and unblocks the steps necessary for code intel to progress toward their OKR goals. It's okay right now if it's not perfect and I do plan to make immediate additional effort into making larger steps to consolidate LSP logic (via lsp-client) as well.

cc @felixfbecker

[efritz]

Declaration of plans

I will be merging this PR, doing a bit of cleanup, and moving the code from the following PRs into this repository:

• Activation refactor sourcegraph-go#117
• Activation refactor sourcegraph-typescript#308

We can then mark the sourcegraph-go repository as deprecated, and (eventually) rework the sourcegraph-typescript repository to be either deprecated, or to contain only the language server (depending on how we most easily convert this to a monorepo for code intel extensions).

The next step will be to rewrite the remaining go/typescript extension code to use the lsp-client interfaces. This may take the form of moving the lsp-client code into this repo as well, or making necessary modifications to the lsp-client repository as a package. Again, whatever is easiest once we see the code all in one place.

I will merge this PR and begin this process unless there are concrete objections by 1/31 7am PT. I do not plan to publish any extensions from the new monorepo until both @chrismwendt and I have sufficiently smoke tested the new code in its new home.

cc @chrismwendt @creachadair @felixfbecker

[felixfbecker]

There's no action required on this comment, but I still owed you a response to your last comment :)

confirmed that an extension re-activates on each page reload, and each tab has its own extension instance.

He also noted that the extension client will not pass settings configured via the site-admin interface/user settings interface automatically, and only the things that are changed as part of a updateConfiguration invocation are pushed to the extension dynamically. So it appears that we need to check the {go,ts}.serverUrl config params on startup anyway, as that's the only value we get.

Is this incorrect?

Given you confirmed it this seems to be factually correct for the current state. It's not a general invariant of the system design though, i.e. this may not be true in the future (else we could have designed the extension API so that it is not possible to listen for config changes and only the state at activation is exposed). The current reactivation of all extensions for every new tab is not ideal for performance, and bad performance has been complained about by customers, so possible any improvement to this are "on our radar". The idea to move things to a SharedWorker and the background page in the browser extension has been floating around for longer (e.g. recently https://github.com/sourcegraph/sourcegraph/pull/8530#issuecomment-589255861), which would violate this assumption. It's currently backlogged though. Part of the reason why it wasn't done yet was that all components need to support this by not relying on those assumptions, so if we add more places that rely on them because we haven't done it yet, it becomes a chicken-and-egg situation :/
We also had inquiries from customers about powering code intelligence in the editor with our remote codeintel, where this assumption would also be wrong (also not on the roadmap right now though).
updateConfiguration may also be used for the serverUrl together with a prompt, e.g. to improve the setup experience for native code host integrations without requiring to go to the Sourcegraph instance and change settings. We use a similar flow for the Codecov extension. Again, not something currently planned.

Disclaimer: I wanna make it clear I'm not saying "we can't do this", I just want to point out that we'll have this to the tech debt stack we need to solve as soon as we want to make one of those changes that would violate the assumptions. Intentional design smells can be okay depending on the situation, I just want to prevent accidental design smells.

I'm seeing less utility for dynamic registration given my understanding in the point above. Why would you ever want to unregister or re-register a new LSP provider for defs/refs/hovers? (I've since removed the support for external refs/locations, and those belong solely to sourcegraph-{go,ts} extensions again, and they can be bound independently/dynamically by the extension as needed).

The ability to register providers dynamically (in the extension API, and in lsp-client) is there to mirror the VS Code API and the LSP support for dynamic registration (which was added later). Of course all dynamic registration can be "emulated" by simply having one provider that early-returns if a condition is not met. This is less efficient though, because it means that the provider (which may be one of many) is still called through the whole IPC stack from the main thread to the worker over WebSockets to the language server, just to return nothing. As opposed to unregistering it from the registry in the main thread, which means the main thread can skip it directly. So direct (un)registration is preferred (but not required).

As to why a language server would use this feature, I don't have a concrete example of a language server using this at hand, but I must assume it's useful for some or else it would not have been added to the spec in a later version. It would be interesting to have a repogroup on sourcegraph.com of all public language servers to make a code search for usages of the dynamic registration. I can imagine it's more useful if we can utilize long-running language servers (i.e. the SharedWorker stuff I talked about above) and multi-root workspaces, as then capabilities are more likely to depend on the workspace root that is open, and because the roots will change over the runtime, so would the capabilities.

Tried to run through how we would do this. It would be simple to have a proxy defining all of the provide* methods that would inject LSIF before and search-based code after the LSP provider, but this gets really magic (in a bad way). We'd also need to be somewhat aware of how document selectors match registered providers, which would duplicate some of the logic inside sg/sg.

I'm not sure it's necessary given my understanding above, and if so it seems to be so at a rather high complexity cost.

Yeah, it's tricky with the fallback logic. In lsp-client, I tried to forward language server registrations as raw as possible to avoid this duplication, but that predated the fallback logic/requirement. FWIW document selectors can be easily matched with minimatch.
The Disclaimer from above also applies here :)