Skip to content

Commit

Permalink
Autocomplete: Fix suggestion event over counting (#649)
Browse files Browse the repository at this point in the history 
This PR fixes an issue that sometimes caused completion suggestion
events to not fire correctly. The issue here was that we decided wether
or not a completion is visible in the `getInlineCompletions` code that
was now change to _still yield completions_.

The yielding is fine, we have relied on these results to be used for the
last candidate cache. However, this would also mean that we would store
a log id that was _deemed as not visible_ but that never reconsidered
this condition. This meant that such a completion could become visible
later and we would be able to accept it, even though it was never logged
as suggested.

To fix this, we move this logic into the VS Code specific part (which
conceptually makes more sense anyways since it's full of VS Code
specific logic) and also test the visibility after every cache
retrieval.

## Test plan

I've added three conditions to test invariants that should not be
possible. To test locally, I've added debugger break points into either
of them and repeatedly triggered a bazillion of completions. After these
changes, I don't seem to be able to trigger them anymore. I’m leaving
some logs in there, though, so we can see if they happen on production.

<!-- Required. See
https://docs.sourcegraph.com/dev/background-information/testing_principles.
-->
  • Loading branch information
@philipp-spiess
philipp-spiess authored Aug 10, 2023
1 parent 7bdce68 commit a1cb359
Show file tree
Hide file tree
Showing 7 changed files with 247 additions and 169 deletions.
1 change: 1 addition & 0 deletions vscode/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ Starting from `0.2.0`, Cody is using `major.EVEN_NUMBER.patch` for release versi
- Update file link color to match buttons. [pull/600](https://github.com/sourcegraph/cody/pull/600)
- Handle `socket hung up` errors that are not caused by the `stop generating` button. [pull/598](https://github.com/sourcegraph/cody/pull/598)
- Fix "Reload Window" appearing in all VS Code views. [pull/603](https://github.com/sourcegraph/cody/pull/603)
- Fixes issues where in some instances, suggested autocomplete events were under counted. [pull/649](https://github.com/sourcegraph/cody/pull/649)

### Changed

Expand Down
67 changes: 9 additions & 58 deletions vscode/src/completions/getInlineCompletions.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import dedent from 'dedent'
import { describe, expect, test, vi } from 'vitest'
import { describe, expect, test } from 'vitest'
import { Range } from 'vscode'
import { URI } from 'vscode-uri'

Expand All @@ -12,13 +12,13 @@ import {
import { vsCodeMocks } from '../testutils/mocks'
import { range } from '../testutils/textDocument'

import { getCurrentDocContext } from './document'
import {
getInlineCompletions as _getInlineCompletions,
InlineCompletionsParams,
InlineCompletionsResultSource,
LastInlineCompletionCandidate,
} from './getInlineCompletions'
import * as CompletionsLogger from './logger'
import { createProviderConfig } from './providers/anthropic'
import { RequestManager } from './request-manager'
import { completion, documentAndPosition } from './testHelpers'
Expand Down Expand Up @@ -47,7 +47,7 @@ function params(
selectedCompletionInfo: undefined,
},
...params
}: Partial<Omit<InlineCompletionsParams, 'document' | 'position'>> & {
}: Partial<Omit<InlineCompletionsParams, 'document' | 'position' | 'docContext'>> & {
languageId?: string
onNetworkRequest?: (params: CompletionParameters) => void
} = {}
Expand All @@ -68,13 +68,17 @@ function params(

const { document, position } = documentAndPosition(code, languageId, URI_FIXTURE.toString())

const docContext = getCurrentDocContext(document, position, 1000, 1000)
if (docContext === null) {
throw new Error()
}

return {
document,
position,
context,
docContext,
promptChars: 1000,
maxPrefixChars: 1000,
maxSuffixChars: 1000,
isEmbeddingsContextEnabled: true,
providerConfig,
responsePercentage: 0.4,
Expand Down Expand Up @@ -202,59 +206,6 @@ describe('getInlineCompletions', () => {
})
})

describe('logger', () => {
test('logs a completion as shown', async () => {
const spy = vi.spyOn(CompletionsLogger, 'suggested')
await getInlineCompletions(params('foo = █', [completion`bar`]))
expect(spy).toHaveBeenCalled()
})

test('does not log a completion when the abort handler was triggered after a network fetch', async () => {
const spy = vi.spyOn(CompletionsLogger, 'suggested')
const abortController = new AbortController()
await getInlineCompletions({
...params('const x = █', [completion`├1337┤`], { onNetworkRequest: () => abortController.abort() }),
abortSignal: abortController.signal,
})
expect(spy).not.toHaveBeenCalled()
})

test('does not log a completion if it does not overlap the completion popup', async () => {
const spy = vi.spyOn(CompletionsLogger, 'suggested')
const abortController = new AbortController()
await getInlineCompletions({
...params('console.█', [completion`├log()┤`], {
context: {
triggerKind: vsCodeMocks.InlineCompletionTriggerKind.Automatic,
selectedCompletionInfo: { text: 'dir', range: new vsCodeMocks.Range(0, 6, 0, 8) },
},
}),
abortSignal: abortController.signal,
})
expect(spy).not.toHaveBeenCalled()
})

test('log a completion if the suffix is inside the completion', async () => {
const spy = vi.spyOn(CompletionsLogger, 'suggested')
const abortController = new AbortController()
await getInlineCompletions({
...params('const a = [1, █];', [completion`├2] ;┤`]),
abortSignal: abortController.signal,
})
expect(spy).toHaveBeenCalled()
})

test('does not log a completion if the suffix does not match', async () => {
const spy = vi.spyOn(CompletionsLogger, 'suggested')
const abortController = new AbortController()
await getInlineCompletions({
...params('const a = [1, █)(123);', [completion`├2];┤`]),
abortSignal: abortController.signal,
})
expect(spy).not.toHaveBeenCalled()
})
})

describe('same line suffix behavior', () => {
test('does not trigger when there are alphanumeric chars in the line suffix', async () =>
expect(await getInlineCompletions(params('foo = █ // x', []))).toBeNull())
Expand Down
100 changes: 4 additions & 96 deletions vscode/src/completions/getInlineCompletions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { debug } from '../log'

import { GetContextOptions, GetContextResult } from './context/context'
import { DocumentHistory } from './context/history'
import { DocumentContext, getCurrentDocContext } from './document'
import { DocumentContext } from './document'
import * as CompletionLogger from './logger'
import { detectMultiline } from './multiline'
import { CompletionProviderTracer, Provider, ProviderConfig, ProviderOptions } from './providers/provider'
Expand All @@ -22,11 +22,10 @@ export interface InlineCompletionsParams {
document: vscode.TextDocument
position: vscode.Position
context: vscode.InlineCompletionContext
docContext: DocumentContext

// Prompt parameters
promptChars: number
maxPrefixChars: number
maxSuffixChars: number
providerConfig: ProviderConfig
responsePercentage: number
prefixPercentage: number
Expand Down Expand Up @@ -132,9 +131,8 @@ async function doGetInlineCompletions({
document,
position,
context,
docContext,
promptChars,
maxPrefixChars,
maxSuffixChars,
providerConfig,
responsePercentage,
prefixPercentage,
Expand All @@ -153,11 +151,6 @@ async function doGetInlineCompletions({
}: InlineCompletionsParams): Promise<InlineCompletionsResult | null> {
tracer?.({ params: { document, position, context } })

const docContext = getCurrentDocContext(document, position, maxPrefixChars, maxSuffixChars)
if (!docContext) {
return null
}

// If we have a suffix in the same line as the cursor and the suffix contains any word
// characters, do not attempt to make a completion. This means we only make completions if
// we have a suffix in the same line for special characters like `)]}` etc.
Expand Down Expand Up @@ -255,7 +248,7 @@ async function doGetInlineCompletions({
? InlineCompletionsResultSource.CacheAfterRequestStart
: InlineCompletionsResultSource.Network

logCompletions(logId, completions, document, docContext, context, providerConfig, source, abortSignal)
CompletionLogger.loaded(logId)

return {
logId,
Expand Down Expand Up @@ -375,88 +368,3 @@ function createCompletionProviderTracer(
}
)
}

function logCompletions(
logId: string,
completions: InlineCompletionItem[],
document: vscode.TextDocument,
docContext: DocumentContext,
context: vscode.InlineCompletionContext,
providerConfig: ProviderConfig,
source: InlineCompletionsResultSource,
abortSignal: AbortSignal | undefined
): void {
CompletionLogger.loaded(logId)

// There are these cases when a completion is being returned here but won't
// be displayed by VS Code.
//
// - When the abort signal was already triggered and a new completion
// request was stared.
// - When the VS Code completion popup is open and we suggest a completion
// that does not match the currently selected completion. For now we make
// sure to not log these completions as displayed.
// TODO: Take this into account when creating the completion prefix.
// - When no completions contains characters in the current line that are
// not in the current line suffix. Since VS Code will try to merge
// completion with the suffix, we have to do a per-character diff to test
// this.
const isAborted = abortSignal ? abortSignal.aborted : false
const isMatchingPopupItem = completionMatchesPopupItem(completions, document, context)
const isMatchingSuffix = completionMatchesSuffix(completions, docContext, providerConfig)
const isVisible = !isAborted && isMatchingPopupItem && isMatchingSuffix

if (isVisible) {
if (completions.length > 0) {
CompletionLogger.suggested(logId, InlineCompletionsResultSource[source])
} else {
CompletionLogger.noResponse(logId)
}
}
}

function completionMatchesPopupItem(
completions: InlineCompletionItem[],
document: vscode.TextDocument,
context: vscode.InlineCompletionContext
): boolean {
if (context.selectedCompletionInfo) {
const currentText = document.getText(context.selectedCompletionInfo.range)
const selectedText = context.selectedCompletionInfo.text
if (completions.length > 0 && !(currentText + completions[0].insertText).startsWith(selectedText)) {
return false
}
}
return true
}

function completionMatchesSuffix(
completions: InlineCompletionItem[],
docContext: DocumentContext,
providerConfig: ProviderConfig
): boolean {
// Models that support infilling do not replace an existing suffix but
// instead insert the completion only at the current cursor position. Thus,
// we do not need to compare the suffix
if (providerConfig.supportsInfilling) {
return true
}

const suffix = docContext.currentLineSuffix

for (const completion of completions) {
const insertion = completion.insertText
let j = 0
// eslint-disable-next-line @typescript-eslint/prefer-for-of
for (let i = 0; i < insertion.length; i++) {
if (insertion[i] === suffix[j]) {
j++
}
}
if (j === suffix.length) {
return true
}
}

return false
}
12 changes: 12 additions & 0 deletions vscode/src/completions/logger.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,18 @@ export function accept(id: string, lines: number): void {
return
}

// Some additional logging to ensure the invariant is correct. I expect these branches to never
// hit but if they do, they might help debug analytics issues
if (!completionEvent.loadedAt) {
logCompletionEvent('unexpectedNotLoaded')
}
if (!completionEvent.startLoggedAt) {
logCompletionEvent('unexpectedNotStarted')
}
if (!completionEvent.suggestedAt) {
logCompletionEvent('unexpectedNotSuggested')
}

completionEvent.acceptedAt = performance.now()

logSuggestionEvents()
Expand Down
Loading

0 comments on commit a1cb359

Please sign in to comment.