From 57c1b237f87f517b48672c8aef1a019ce45ac820 Mon Sep 17 00:00:00 2001 From: Benjamin Christopher Simmonds <44439583+benibenj@users.noreply.github.com> Date: Mon, 5 Jan 2026 23:36:47 +0100 Subject: [PATCH] fix import suggestions (#2715) --- .../diagnosticsCompletions.ts | 4 + .../diagnosticsCompletionProcessor.ts | 103 ++++++------------ .../features/diagnosticsInlineEditProvider.ts | 63 ++++++----- .../vscode-node/inlineCompletionProvider.ts | 9 +- 4 files changed, 75 insertions(+), 104 deletions(-) diff --git a/extensions/copilot/src/extension/inlineEdits/vscode-node/features/diagnosticsBasedCompletions/diagnosticsCompletions.ts b/extensions/copilot/src/extension/inlineEdits/vscode-node/features/diagnosticsBasedCompletions/diagnosticsCompletions.ts index 42594622596..e1d4acfb2ad 100644 --- a/extensions/copilot/src/extension/inlineEdits/vscode-node/features/diagnosticsBasedCompletions/diagnosticsCompletions.ts +++ b/extensions/copilot/src/extension/inlineEdits/vscode-node/features/diagnosticsBasedCompletions/diagnosticsCompletions.ts @@ -105,6 +105,10 @@ export abstract class DiagnosticCompletionItem implements vscode.InlineCompletio protected _getDisplayLocation(): INextEditDisplayLocation | undefined { return undefined; } + + toString(): string { + return `DiagnosticCompletionItem(type=${this.type}, diagnostic=${this.diagnostic.toString()}, edit=${this._edit.toString()})`; + } } function displayLocationEquals(a: INextEditDisplayLocation | undefined, b: INextEditDisplayLocation | undefined): boolean { diff --git a/extensions/copilot/src/extension/inlineEdits/vscode-node/features/diagnosticsCompletionProcessor.ts b/extensions/copilot/src/extension/inlineEdits/vscode-node/features/diagnosticsCompletionProcessor.ts index 3b0a9918d46..d688028e84f 100644 --- a/extensions/copilot/src/extension/inlineEdits/vscode-node/features/diagnosticsCompletionProcessor.ts +++ b/extensions/copilot/src/extension/inlineEdits/vscode-node/features/diagnosticsCompletionProcessor.ts @@ -24,7 +24,7 @@ import { ThrottledDelayer } from '../../../../util/vs/base/common/async'; import { CancellationToken, CancellationTokenSource } from '../../../../util/vs/base/common/cancellation'; import { BugIndicatingError } from '../../../../util/vs/base/common/errors'; import { Emitter } from '../../../../util/vs/base/common/event'; -import { Disposable, DisposableStore } from '../../../../util/vs/base/common/lifecycle'; +import { Disposable } from '../../../../util/vs/base/common/lifecycle'; import { autorun, derived, IObservable, runOnChange } from '../../../../util/vs/base/common/observableInternal'; import { isEqual } from '../../../../util/vs/base/common/resources'; import { StringEdit } from '../../../../util/vs/editor/common/core/edits/stringEdit'; @@ -37,7 +37,7 @@ import { IVSCodeObservableDocument, VSCodeWorkspace } from '../parts/vscodeWorks import { toInternalPosition } from '../utils/translations'; import { AnyDiagnosticCompletionItem, AnyDiagnosticCompletionProvider } from './diagnosticsBasedCompletions/anyDiagnosticsCompletionProvider'; import { AsyncDiagnosticCompletionProvider } from './diagnosticsBasedCompletions/asyncDiagnosticsCompletionProvider'; -import { Diagnostic, DiagnosticCompletionItem, DiagnosticInlineEditRequestLogContext, distanceToClosestDiagnostic, IDiagnosticCompletionProvider, log, logList, sortDiagnosticsByDistance } from './diagnosticsBasedCompletions/diagnosticsCompletions'; +import { Diagnostic, DiagnosticCompletionItem, DiagnosticInlineEditRequestLogContext, IDiagnosticCompletionProvider, log, logList, sortDiagnosticsByDistance } from './diagnosticsBasedCompletions/diagnosticsCompletions'; import { ImportDiagnosticCompletionItem, ImportDiagnosticCompletionProvider } from './diagnosticsBasedCompletions/importDiagnosticsCompletionProvider'; interface IDiagnosticsCompletionState { @@ -143,6 +143,7 @@ export type DiagnosticCompletionState = { item: DiagnosticCompletionItem | undefined; telemetry: IDiagnosticsCompletionTelemetry; logContext: DiagnosticInlineEditRequestLogContext | undefined; + workInProgress?: boolean; }; export class DiagnosticsCompletionProcessor extends Disposable { @@ -275,7 +276,7 @@ export class DiagnosticsCompletionProcessor extends Disposable { const cursor = toInternalPosition(selection.start); const log = new DiagnosticInlineEditRequestLogContext(); - const { availableDiagnostics, relevantDiagnostics } = this._getDiagnostics(workspaceDocument, cursor, log); + const relevantDiagnostics = this._getDiagnostics(workspaceDocument, cursor, log); const diagnosticsSorted = sortDiagnosticsByDistance(workspaceDocument, relevantDiagnostics, cursor); if (this._currentDiagnostics.isEqualAndUpdate(diagnosticsSorted)) { @@ -284,14 +285,13 @@ export class DiagnosticsCompletionProcessor extends Disposable { this._tracer.trace('Scheduled update for diagnostics inline completion'); - await this._worker.schedule(async (token: CancellationToken) => this._runCompletionHandler(workspaceDocument, diagnosticsSorted, availableDiagnostics, cursor, log, token)); + await this._worker.schedule(async (token: CancellationToken) => this._runCompletionHandler(workspaceDocument, diagnosticsSorted, cursor, log, token)); } - private _getDiagnostics(workspaceDocument: IVSCodeObservableDocument, cursor: Position, logContext: DiagnosticInlineEditRequestLogContext): { availableDiagnostics: Diagnostic[]; relevantDiagnostics: Diagnostic[] } { + private _getDiagnostics(workspaceDocument: IVSCodeObservableDocument, cursor: Position, logContext: DiagnosticInlineEditRequestLogContext): Diagnostic[] { const availableDiagnostics = workspaceDocument.diagnostics.get().map(d => new Diagnostic(d)); - if (availableDiagnostics.length === 0) { - return { availableDiagnostics: [], relevantDiagnostics: [] }; + return []; } const filterDiagnosticsAndLog = (diagnostics: Diagnostic[], message: string, filterFn: (diagnostics: Diagnostic[]) => Diagnostic[]): Diagnostic[] => { @@ -311,10 +311,10 @@ export class DiagnosticsCompletionProcessor extends Disposable { relevantDiagnostics = filterDiagnosticsAndLog(relevantDiagnostics, 'Filtered by recent acceptance', ds => ds.filter(diagnostic => !this._hasDiagnosticRecentlyBeenAccepted(diagnostic))); relevantDiagnostics = filterDiagnosticsAndLog(relevantDiagnostics, 'Filtered by no recent edit', ds => this._filterDiagnosticsByRecentEditNearby(ds, workspaceDocument)); - return { availableDiagnostics, relevantDiagnostics }; + return relevantDiagnostics; } - private async _runCompletionHandler(workspaceDocument: IVSCodeObservableDocument, diagnosticsSorted: Diagnostic[], allDiagnostics: Diagnostic[], cursor: Position, log: DiagnosticInlineEditRequestLogContext, token: CancellationToken): Promise { + private async _runCompletionHandler(workspaceDocument: IVSCodeObservableDocument, diagnosticsSorted: Diagnostic[], cursor: Position, log: DiagnosticInlineEditRequestLogContext, token: CancellationToken): Promise { const telemetryBuilder = new DiagnosticsCompletionHandlerTelemetry(); let completionItem = null; @@ -327,20 +327,6 @@ export class DiagnosticsCompletionProcessor extends Disposable { this._tracer.trace('Diagnostic Providers returned completion item: ' + (completionItem ? completionItem.toString() : 'null')); - // Distance to the closest diagnostic which is not supported by any provider - const allNoneSupportedDiagnostics = allDiagnostics.filter(diagnostic => !diagnosticsSorted.includes(diagnostic)); - telemetryBuilder.setDistanceToUnknownDiagnostic(distanceToClosestDiagnostic(workspaceDocument, allNoneSupportedDiagnostics, cursor)); - - // Distance to the closest none result diagnostic - const allAlternativeDiagnostics = allDiagnostics.filter(diagnostic => !completionItem || !completionItem.diagnostic.equals(diagnostic)); - telemetryBuilder.setDistanceToAlternativeDiagnostic(distanceToClosestDiagnostic(workspaceDocument, allAlternativeDiagnostics, cursor)); - - if (completionItem) { - const hasDiagnosticForSameRange = allAlternativeDiagnostics.some(diagnostic => completionItem.diagnostic.range.equals(diagnostic.range)); - telemetryBuilder.setHasAlternativeDiagnosticForSameRange(hasDiagnosticForSameRange); - } - - // Todo: this should be handled on a lower level if (completionItem instanceof ImportDiagnosticCompletionItem) { telemetryBuilder.setImportTelemetry(completionItem); } @@ -354,43 +340,28 @@ export class DiagnosticsCompletionProcessor extends Disposable { const workspaceDocument = this._workspace.getDocument(docId); if (!workspaceDocument) { return { item: undefined, telemetry: new DiagnosticsCompletionHandlerTelemetry().addDroppedReason('WorkspaceDocumentNotFound').build(), logContext: undefined }; } - if (currentState === NoResultReason.HasNotRunYet) { + if (currentState === undefined) { return { item: undefined, telemetry: new DiagnosticsCompletionHandlerTelemetry().build(), logContext: undefined }; } - if (currentState === NoResultReason.WorkInProgress) { - return { item: undefined, telemetry: new DiagnosticsCompletionHandlerTelemetry().addDroppedReason(NoResultReason.WorkInProgress).build(), logContext: undefined }; - } const { telemetryBuilder, completionItem, logContext } = currentState; + const workInProgress = this._worker.workInProgress(); if (!completionItem) { - return { item: undefined, telemetry: telemetryBuilder.build(), logContext }; + return { item: undefined, telemetry: telemetryBuilder.build(), logContext, workInProgress }; } if (!this._isCompletionItemValid(completionItem, workspaceDocument, currentState.logContext, telemetryBuilder)) { - return { item: undefined, telemetry: telemetryBuilder.build(), logContext }; + return { item: undefined, telemetry: telemetryBuilder.build(), logContext, workInProgress }; } if (completionItem.documentId !== docId) { logContext.addLog('Dropped: wrong-document'); - return { item: undefined, telemetry: telemetryBuilder.addDroppedReason('wrong-document').build(), logContext }; + return { item: undefined, telemetry: telemetryBuilder.addDroppedReason('wrong-document').build(), logContext, workInProgress }; } log('following known diagnostics:\n' + this._currentDiagnostics.toString(), undefined, this._tracer); - return { item: completionItem, telemetry: telemetryBuilder.build(), logContext }; - } - - async getNextUpdatedState(docId: DocumentId, token: CancellationToken): Promise { - const disposables = new DisposableStore(); - - await new Promise((resolve) => { - disposables.add(token.onCancellationRequested(() => resolve())); - disposables.add(this._worker.onDidChange(() => resolve())); - }); - - disposables.dispose(); - - return this.getCurrentState(docId); + return { item: completionItem, telemetry: telemetryBuilder.build(), logContext, workInProgress }; } private async _getCompletionFromDiagnostics(workspaceDocument: IVSCodeObservableDocument, diagnosticsSorted: Diagnostic[], pos: Position, logContext: DiagnosticInlineEditRequestLogContext, token: CancellationToken, tb: DiagnosticsCompletionHandlerTelemetry): Promise { @@ -563,11 +534,6 @@ function isEditorFromEditorGrid(editor: vscode.TextEditor): boolean { return editor.viewColumn !== undefined; } -const enum NoResultReason { - WorkInProgress = 'work-in-progress', - HasNotRunYet = 'has-not-run-yet' -} - class AsyncWorker extends Disposable { private readonly _taskQueue: ThrottledDelayer; @@ -575,18 +541,18 @@ class AsyncWorker extends Disposable { readonly onDidChange = this._onDidChange.event; private _currentTokenSource: CancellationTokenSource | undefined = undefined; - private _activeWorkPromise: Promise | undefined = undefined; + private _activeWorkPromise: Promise | undefined = undefined; private __currentResult: T | undefined = undefined; private get _currentResult(): T | undefined { return this.__currentResult; } private set _currentResult(value: T) { - if (!this._taskQueue.isTriggered() && (this.__currentResult === undefined || !this._equals(value, this.__currentResult))) { + const changed = this.__currentResult === undefined || !this._equals(value, this.__currentResult); + this.__currentResult = value; + if (changed) { this._onDidChange.fire(value); } - - this.__currentResult = value; } constructor(delay: number, private readonly _equals: (a: T, b: T) => boolean) { @@ -599,47 +565,48 @@ class AsyncWorker extends Disposable { const activePromise = this._doSchedule(fn); this._activeWorkPromise = activePromise; - await activePromise; + const result = await activePromise; if (this._activeWorkPromise === activePromise) { this._activeWorkPromise = undefined; } + + if (result !== undefined) { + this._currentResult = result; + } } - private async _doSchedule(fn: (token: CancellationToken) => Promise): Promise { + private async _doSchedule(fn: (token: CancellationToken) => Promise): Promise { this._currentTokenSource?.dispose(true); this._currentTokenSource = new CancellationTokenSource(); const token = this._currentTokenSource.token; + let result; await this._taskQueue.trigger(async () => { if (token.isCancellationRequested) { return; } - const result = await fn(token); - - if (token.isCancellationRequested) { - return; - } - - this._currentResult = result; + result = await fn(token); }); + + return result; } // Get the active result if there is one currently // Return undefined if there is currently work being done - getCurrentResult(): T | NoResultReason { + getCurrentResult(): T | undefined { if (this._currentResult === undefined) { - return NoResultReason.HasNotRunYet; - } - - if (this._activeWorkPromise !== undefined) { - return NoResultReason.WorkInProgress; + return undefined; } return this._currentResult; } + workInProgress(): boolean { + return this._activeWorkPromise !== undefined; + } + override dispose(): void { if (this._currentTokenSource) { this._currentTokenSource.dispose(); diff --git a/extensions/copilot/src/extension/inlineEdits/vscode-node/features/diagnosticsInlineEditProvider.ts b/extensions/copilot/src/extension/inlineEdits/vscode-node/features/diagnosticsInlineEditProvider.ts index c41e02379e8..b3b081f108a 100644 --- a/extensions/copilot/src/extension/inlineEdits/vscode-node/features/diagnosticsInlineEditProvider.ts +++ b/extensions/copilot/src/extension/inlineEdits/vscode-node/features/diagnosticsInlineEditProvider.ts @@ -12,7 +12,7 @@ import { ShowNextEditPreference } from '../../../../platform/inlineEdits/common/ import { ILogService } from '../../../../platform/log/common/logService'; import * as errors from '../../../../util/common/errors'; import { createTracer, ITracer } from '../../../../util/common/tracing'; -import { timeout } from '../../../../util/vs/base/common/async'; +import { raceCancellation, timeout } from '../../../../util/vs/base/common/async'; import { CancellationToken } from '../../../../util/vs/base/common/cancellation'; import { BugIndicatingError } from '../../../../util/vs/base/common/errors'; import { Disposable } from '../../../../util/vs/base/common/lifecycle'; @@ -35,6 +35,7 @@ export class DiagnosticsNextEditResult implements INextEditResult { showRangePreference?: ShowNextEditPreference; action?: Command; } | undefined, + public workInProgress: boolean = false ) { } } @@ -68,18 +69,7 @@ export class DiagnosticsNextEditProvider extends Disposable implements INextEdit async getNextEdit(docId: DocumentId, context: NESInlineCompletionContext, logContext: InlineEditRequestLogContext, cancellationToken: CancellationToken, tb: DiagnosticsTelemetryBuilder): Promise { this._lastTriggerTime = Date.now(); - - if (cancellationToken.isCancellationRequested) { - this._tracer.trace('cancellationRequested before started'); - return new DiagnosticsNextEditResult(logContext.requestId, undefined); - } - - let diagnosticEditResult = this._diagnosticsCompletionHandler.getCurrentState(docId); - if (!diagnosticEditResult.item) { - diagnosticEditResult = await this._diagnosticsCompletionHandler.getNextUpdatedState(docId, cancellationToken); - } - - return this._createNextEditResult(diagnosticEditResult, logContext, tb); + throw new BugIndicatingError('DiagnosticsNextEditProvider does not support getNextEdit, use runUntilNextEdit instead'); } async runUntilNextEdit(docId: DocumentId, context: NESInlineCompletionContext, logContext: InlineEditRequestLogContext, delayStart: number, cancellationToken: CancellationToken, tb: DiagnosticsTelemetryBuilder): Promise { @@ -91,33 +81,42 @@ export class DiagnosticsNextEditProvider extends Disposable implements INextEdit } // Check if the last computed edit is still valid - let completionResult = this._diagnosticsCompletionHandler.getCurrentState(docId); - let telemetry = new DiagnosticsTelemetryBuilder(); - let diagnosticEditResult = this._createNextEditResult(completionResult, logContext, telemetry); - - // If the last computed edit is not valid, wait until the state is updated or the operation is cancelled - while (!diagnosticEditResult.result && !cancellationToken.isCancellationRequested) { - completionResult = await this._diagnosticsCompletionHandler.getNextUpdatedState(docId, cancellationToken); - telemetry = new DiagnosticsTelemetryBuilder(); - diagnosticEditResult = this._createNextEditResult(completionResult, logContext, telemetry); + const initialResult = this._getResultForCurrentState(docId, logContext, tb); + if (initialResult.result) { + return initialResult; } - telemetry.populate(tb); + const asyncResult = await raceCancellation(new Promise((resolve) => { + const onDidChangeDisposable = this._diagnosticsCompletionHandler.onDidChange((hasResult) => { + const completionResult = this._getResultForCurrentState(docId, logContext, tb); + if (completionResult.result || !completionResult.workInProgress) { + resolve(completionResult); + onDidChangeDisposable.dispose(); + } + }); + }), cancellationToken); - // TODO: Better incorporate diagnostics logging - if (completionResult.logContext) { - completionResult.logContext.getLogs().forEach(log => logContext.addLog(log)); - } - - return diagnosticEditResult; + return asyncResult ?? initialResult; } catch (error) { const errorMessage = `Error occurred while waiting for diagnostic edit: ${errors.toString(errors.fromUnknown(error))}`; logContext.addLog(errorMessage); this._tracer.trace(errorMessage); return new DiagnosticsNextEditResult(logContext.requestId, undefined); + } finally { + this._tracer.trace('DiagnosticsInlineCompletionProvider runUntilNextEdit complete' + (cancellationToken.isCancellationRequested ? ' (cancelled)' : '')); } } + private _getResultForCurrentState(docId: DocumentId, logContext: InlineEditRequestLogContext, tb: DiagnosticsTelemetryBuilder): DiagnosticsNextEditResult { + const completionResult = this._diagnosticsCompletionHandler.getCurrentState(docId); + const telemetry = new DiagnosticsTelemetryBuilder(); + const diagnosticEditResult = this._createNextEditResult(completionResult, logContext, telemetry); + if (diagnosticEditResult.result) { + telemetry.populate(tb); + } + return diagnosticEditResult; + } + private _createNextEditResult(diagnosticEditResult: DiagnosticCompletionState, logContext: InlineEditRequestLogContext, tb: DiagnosticsTelemetryBuilder): DiagnosticsNextEditResult { const { item, telemetry } = diagnosticEditResult; @@ -125,7 +124,7 @@ export class DiagnosticsNextEditProvider extends Disposable implements INextEdit if (item && this._hasRecentlyBeenAccepted(item)) { tb.addDroppedReason(`${item.type}:recently-accepted`); this._tracer.trace('recently accepted'); - return new DiagnosticsNextEditResult(logContext.requestId, undefined); + return new DiagnosticsNextEditResult(logContext.requestId, undefined, diagnosticEditResult.workInProgress); } telemetry.droppedReasons.forEach(reason => tb.addDroppedReason(reason)); @@ -133,7 +132,7 @@ export class DiagnosticsNextEditProvider extends Disposable implements INextEdit if (!item) { this._tracer.trace('no diagnostic edit result'); - return new DiagnosticsNextEditResult(logContext.requestId, undefined); + return new DiagnosticsNextEditResult(logContext.requestId, undefined, diagnosticEditResult.workInProgress); } tb.setType(item.type); @@ -145,7 +144,7 @@ export class DiagnosticsNextEditProvider extends Disposable implements INextEdit edit: item.toOffsetEdit(), displayLocation: item.nextEditDisplayLocation, item - }); + }, diagnosticEditResult.workInProgress); } handleShown(suggestion: DiagnosticsNextEditResult): void { } diff --git a/extensions/copilot/src/extension/inlineEdits/vscode-node/inlineCompletionProvider.ts b/extensions/copilot/src/extension/inlineEdits/vscode-node/inlineCompletionProvider.ts index b5652bc179c..88babdba2e3 100644 --- a/extensions/copilot/src/extension/inlineEdits/vscode-node/inlineCompletionProvider.ts +++ b/extensions/copilot/src/extension/inlineEdits/vscode-node/inlineCompletionProvider.ts @@ -30,6 +30,7 @@ import { raceCancellation, timeout } from '../../../util/vs/base/common/async'; import { CancellationTokenSource } from '../../../util/vs/base/common/cancellation'; import { Emitter, Event } from '../../../util/vs/base/common/event'; import { Disposable } from '../../../util/vs/base/common/lifecycle'; +import { clamp } from '../../../util/vs/base/common/numbers'; import { autorun, IObservable, observableFromEvent } from '../../../util/vs/base/common/observable'; import { basename } from '../../../util/vs/base/common/path'; import { StringEdit } from '../../../util/vs/editor/common/core/edits/stringEdit'; @@ -239,13 +240,13 @@ export class InlineCompletionProviderImpl extends Disposable implements InlineCo let [providerSuggestion, diagnosticsSuggestion] = await first; - const hasNonEmptyLlmNes = providerSuggestion && providerSuggestion.result !== undefined; - - const shouldGiveMoreTimeToDiagnostics = !hasNonEmptyLlmNes && this.model.diagnosticsBasedProvider; + const hasNonEmptyLlmNes = !!providerSuggestion && providerSuggestion.result !== undefined; + const shouldGiveMoreTimeToDiagnostics = !hasNonEmptyLlmNes && this.model.diagnosticsBasedProvider && !diagnosticsSuggestion; if (shouldGiveMoreTimeToDiagnostics) { tracer.trace('giving some more time to diagnostics provider'); - timeout(1000).then(() => requestCancellationTokenSource.cancel()); + const remainingTime = clamp(0, 1250 - (Date.now() - context.requestIssuedDateTime), 1250); + timeout(remainingTime).then(() => requestCancellationTokenSource.cancel()); [, diagnosticsSuggestion] = await all; }