Fix basic code intel definitions

[chrismwendt]

The problem seems to be that the Observable returned by provideDefinition and wrapped by distinctUntilChanged is hot, was emitting exactly once, and was emitting before the Sourcegraph extension API called provideDefinition a second time. On the second call, distinctUntilChanged returned the cached Observable which was done emitting and ended up preventing any location results from getting to the Sourcegraph extension API.

It seems as though https://github.com/sourcegraph/sourcegraph/issues/1321 is either not actually fixed, or the double-call has reappeared.

Fixes https://github.com/sourcegraph/sourcegraph/issues/5620

cc @sourcegraph/code-intel

[felixfbecker]

Kudos for finding this. However, the fix looks to me like it just piles another workaround on top of a workaround, and having the previous workaround was already (partly) the cause of this bug, so eventually this just leads to even more bugs.

The problem seems to be that the Observable returned by provideDefinition and wrapped by distinctUntilChanged is hot

This seems indeed unintentional and possible to fix. observableFromAsyncIterable() assumes that the passed Iterable is cold, but the AsyncIterable returned by a generator function is hot. This can be fixed by rewriting the function to take a generator function () => AsyncIterator<T>:

Diff

diff --git a/extension/src/extension.ts b/extension/src/extension.ts
index 16bd2d6..6dec706 100644
--- a/extension/src/extension.ts
+++ b/extension/src/extension.ts
@@ -83,7 +83,7 @@ import {
     asArray,
     distinctUntilChanged,
     flatMapConcurrent,
-    observableFromAsyncIterable,
+    observableFromAsyncGenerator,
     SourcegraphEndpoint,
     throwIfAbortError,
 } from './util'
@@ -540,7 +540,7 @@ export async function activate(ctx: sourcegraph.ExtensionContext): Promise<void>
         providers.add(
             sourcegraph.languages.registerHoverProvider(documentSelector, {
                 provideHover: distinctUntilChanged(areProviderParamsEqual, (textDocument, position) =>
-                    observableFromAsyncIterable(provideHover(textDocument, position))
+                    observableFromAsyncGenerator(provideHover.bind(null, textDocument, position))
                 ),
             })
         )
@@ -584,7 +584,7 @@ export async function activate(ctx: sourcegraph.ExtensionContext): Promise<void>
         providers.add(
             sourcegraph.languages.registerDefinitionProvider(documentSelector, {
                 provideDefinition: distinctUntilChanged(areProviderParamsEqual, (textDocument, position) =>
-                    observableFromAsyncIterable(provideDefinition(textDocument, position))
+                    observableFromAsyncGenerator(provideDefinition.bind(null, textDocument, position))
                 ),
             })
         )
@@ -594,7 +594,7 @@ export async function activate(ctx: sourcegraph.ExtensionContext): Promise<void>
             textDocument: sourcegraph.TextDocument,
             position: sourcegraph.Position,
             context: sourcegraph.ReferenceContext
-        ): AsyncIterable<sourcegraph.Location[]> =>
+        ): AsyncIterableIterator<sourcegraph.Location[]> =>
             traceAsyncGenerator('Provide references', tracer, undefined, async function*(span) {
                 if (canGenerateTraceUrl(span)) {
                     logger.log('References trace', span.generateTraceURL())
@@ -790,7 +790,8 @@ export async function activate(ctx: sourcegraph.ExtensionContext): Promise<void>
             })
         providers.add(
             sourcegraph.languages.registerReferenceProvider(documentSelector, {
-                provideReferences: (doc, pos, ctx) => observableFromAsyncIterable(provideReferences(doc, pos, ctx)),
+                provideReferences: (doc, pos, ctx) =>
+                    observableFromAsyncGenerator(provideReferences.bind(null, doc, pos, ctx)),
             })
         )
 
diff --git a/extension/src/tracing.ts b/extension/src/tracing.ts
index 8c3baa7..8dca73c 100644
--- a/extension/src/tracing.ts
+++ b/extension/src/tracing.ts
@@ -127,7 +127,7 @@ export async function* traceAsyncGenerator<T>(
     tracer: Tracer,
     childOf: Span | undefined,
     asyncGenerator: (span: Span) => AsyncIterable<T>
-): AsyncIterable<T> {
+): AsyncIterableIterator<T> {
     const span = tracer.startSpan(operationName, { childOf })
     try {
         yield* asyncGenerator(span)
diff --git a/extension/src/util.ts b/extension/src/util.ts
index 9d91bfe..9a0ebfa 100644
--- a/extension/src/util.ts
+++ b/extension/src/util.ts
@@ -24,9 +24,9 @@ export function throwIfAbortError(err: any): void {
 
 export const createAbortError = () => Object.assign(new Error('Aborted'), { name: 'AbortError' })
 
-export const observableFromAsyncIterable = <T>(iterable: AsyncIterable<T>): Observable<T> =>
+export const observableFromAsyncGenerator = <T>(generator: () => AsyncIterator<T>): Observable<T> =>
     new Observable(observer => {
-        const iterator = iterable[Symbol.asyncIterator]()
+        const iterator = generator()
         let unsubscribed = false
         let iteratorDone = false
         function next(): void {
@@ -66,7 +66,7 @@ export const observableFromAsyncIterable = <T>(iterable: AsyncIterable<T>): Obse
  */
 export const abortPrevious = <P extends any[], R>(
     fn: (...args: P) => AsyncIterable<R>
-): ((...args: P) => AsyncIterable<R>) => {
+): ((...args: P) => AsyncIterableIterator<R>) => {
     let abort = noop
     return async function*(...args) {
         abort()

That alone should fix the observed bug I think, but the values will still be double-computed (i.e. distinctUntilChanged() won't do what is expected), so adding the shareReplay() still makes sense. However, this seems to me like it should be applied to all providers, not just definition. Or is there any reason not to do so?

It seems as though sourcegraph/sourcegraph#1321 is either not actually fixed, or the double-call has reappeared.

This seems to be the actual bug that should be fixed to prevent more and more bugs caused by increased complexity through layered workarounds, and to prevent this bug for all extensions (including LSIF), not just sourcegraph-typescript. Any results of a root cause analysis here? Could we write a unit test for this?

[chrismwendt]

That patch kind of works: the panel appears but the buttons don't:

I added shareReplay to hovers (refs don't use distinctUntilChanged), too.

I haven't done an RCA on the double calls.

Want to fix your solution more or go with the PR as-is?

[chrismwendt]

Merging as-is because this at least fixes the release blocker.

[felixfbecker]

@chrismwendt could you own finding the root cause?

[chrismwendt]

Reopened the original issue for the bug https://github.com/sourcegraph/sourcegraph/issues/1321