From e3f59aa88a0051996939979e518a5e77878e88ff Mon Sep 17 00:00:00 2001 From: Sean McManus Date: Thu, 21 Nov 2024 18:56:33 -0800 Subject: [PATCH 1/8] Fix an unnecessary cancel/re-request with GitHub Copilot requests. --- Extension/src/LanguageServer/client.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Extension/src/LanguageServer/client.ts b/Extension/src/LanguageServer/client.ts index 6dd00d741..70746c998 100644 --- a/Extension/src/LanguageServer/client.ts +++ b/Extension/src/LanguageServer/client.ts @@ -2318,9 +2318,13 @@ export class DefaultClient implements Client { } } + // Don't cancel if the result has already been computed, because the result might still be usable + // without needing to send an unnecessary re-request, which is the case for the callback to registerRelatedFilesProvider. + /* if (token.isCancellationRequested) { throw new vscode.CancellationError(); } + */ return result; } From e3e37b533cf61a430393f7131e39df71517a58f2 Mon Sep 17 00:00:00 2001 From: Sean McManus Date: Thu, 21 Nov 2024 19:11:38 -0800 Subject: [PATCH 2/8] Fix linter error. --- Extension/src/LanguageServer/client.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Extension/src/LanguageServer/client.ts b/Extension/src/LanguageServer/client.ts index 70746c998..023d0930a 100644 --- a/Extension/src/LanguageServer/client.ts +++ b/Extension/src/LanguageServer/client.ts @@ -2320,11 +2320,10 @@ export class DefaultClient implements Client { // Don't cancel if the result has already been computed, because the result might still be usable // without needing to send an unnecessary re-request, which is the case for the callback to registerRelatedFilesProvider. - /* if (token.isCancellationRequested) { - throw new vscode.CancellationError(); + return result; + // throw new vscode.CancellationError(); } - */ return result; } From 8e2e9775fede7bfaef01a61137e44b9c3575b550 Mon Sep 17 00:00:00 2001 From: Sean McManus Date: Thu, 21 Nov 2024 19:28:22 -0800 Subject: [PATCH 3/8] Minor change. --- Extension/src/LanguageServer/client.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Extension/src/LanguageServer/client.ts b/Extension/src/LanguageServer/client.ts index 023d0930a..5631f05c5 100644 --- a/Extension/src/LanguageServer/client.ts +++ b/Extension/src/LanguageServer/client.ts @@ -2321,8 +2321,7 @@ export class DefaultClient implements Client { // Don't cancel if the result has already been computed, because the result might still be usable // without needing to send an unnecessary re-request, which is the case for the callback to registerRelatedFilesProvider. if (token.isCancellationRequested) { - return result; - // throw new vscode.CancellationError(); + return result; // throw new vscode.CancellationError(); } return result; From d298adfbe9ceefe415db65674c31af1e1c9ddbd2 Mon Sep 17 00:00:00 2001 From: Sean McManus Date: Mon, 2 Dec 2024 19:54:26 -0800 Subject: [PATCH 4/8] Remove other cancel handling. --- Extension/src/LanguageServer/client.ts | 25 ++++++++----------- .../src/LanguageServer/copilotProviders.ts | 10 ++++---- Extension/src/LanguageServer/lmTool.ts | 4 +-- .../SingleRootProject/tests/lmTool.test.ts | 12 ++++----- 4 files changed, 23 insertions(+), 28 deletions(-) diff --git a/Extension/src/LanguageServer/client.ts b/Extension/src/LanguageServer/client.ts index ede821588..2cb7ee859 100644 --- a/Extension/src/LanguageServer/client.ts +++ b/Extension/src/LanguageServer/client.ts @@ -804,9 +804,9 @@ export interface Client { getShowConfigureIntelliSenseButton(): boolean; setShowConfigureIntelliSenseButton(show: boolean): void; addTrustedCompiler(path: string): Promise; - getIncludes(maxDepth: number, token: vscode.CancellationToken): Promise; + getIncludes(maxDepth: number): Promise; getChatContext(uri: vscode.Uri, token: vscode.CancellationToken): Promise; - getProjectContext(uri: vscode.Uri, token: vscode.CancellationToken): Promise; + getProjectContext(uri: vscode.Uri): Promise; } export function createClient(workspaceFolder?: vscode.WorkspaceFolder): Client { @@ -2228,11 +2228,10 @@ export class DefaultClient implements Client { await this.languageClient.sendNotification(DidOpenNotification, params); } - public async getIncludes(maxDepth: number, token: vscode.CancellationToken): Promise { + public async getIncludes(maxDepth: number): Promise { const params: GetIncludesParams = { maxDepth: maxDepth }; await this.ready; - return DefaultClient.withLspCancellationHandling( - () => this.languageClient.sendRequest(IncludesRequest, params, token), token); + return this.languageClient.sendRequest(IncludesRequest, params); } public async getChatContext(uri: vscode.Uri, token: vscode.CancellationToken): Promise { @@ -2242,11 +2241,10 @@ export class DefaultClient implements Client { () => this.languageClient.sendRequest(CppContextRequest, params, token), token); } - public async getProjectContext(uri: vscode.Uri, token: vscode.CancellationToken): Promise { + public async getProjectContext(uri: vscode.Uri): Promise { const params: TextDocumentIdentifier = { uri: uri.toString() }; - await withCancellation(this.ready, token); - return DefaultClient.withLspCancellationHandling( - () => this.languageClient.sendRequest(ProjectContextRequest, params, token), token); + await this.ready; + return this.languageClient.sendRequest(ProjectContextRequest, params); } /** @@ -2340,11 +2338,8 @@ export class DefaultClient implements Client { throw e; } } - - // Don't cancel if the result has already been computed, because the result might still be usable - // without needing to send an unnecessary re-request, which is the case for the callback to registerRelatedFilesProvider. if (token.isCancellationRequested) { - return result; // throw new vscode.CancellationError(); + throw new vscode.CancellationError(); } return result; @@ -4153,7 +4148,7 @@ class NullClient implements Client { getShowConfigureIntelliSenseButton(): boolean { return false; } setShowConfigureIntelliSenseButton(show: boolean): void { } addTrustedCompiler(path: string): Promise { return Promise.resolve(); } - getIncludes(maxDepth: number, token: vscode.CancellationToken): Promise { return Promise.resolve({} as GetIncludesResult); } + getIncludes(maxDepth: number): Promise { return Promise.resolve({} as GetIncludesResult); } getChatContext(uri: vscode.Uri, token: vscode.CancellationToken): Promise { return Promise.resolve({} as ChatContextResult); } - getProjectContext(uri: vscode.Uri, token: vscode.CancellationToken): Promise { return Promise.resolve({} as ProjectContextResult); } + getProjectContext(uri: vscode.Uri): Promise { return Promise.resolve({} as ProjectContextResult); } } diff --git a/Extension/src/LanguageServer/copilotProviders.ts b/Extension/src/LanguageServer/copilotProviders.ts index 2433e1695..653fad1d5 100644 --- a/Extension/src/LanguageServer/copilotProviders.ts +++ b/Extension/src/LanguageServer/copilotProviders.ts @@ -38,14 +38,14 @@ export async function registerRelatedFilesProvider(): Promise { for (const languageId of ['c', 'cpp', 'cuda-cpp']) { api.registerRelatedFilesProvider( { extensionId: util.extensionContext.extension.id, languageId }, - async (uri: vscode.Uri, context: { flags: Record }, token: vscode.CancellationToken) => { + async (uri: vscode.Uri, context: { flags: Record }) => { const start = performance.now(); const telemetryProperties: Record = {}; const telemetryMetrics: Record = {}; try { - const getIncludesHandler = async () => (await getIncludesWithCancellation(1, token))?.includedFiles.map(file => vscode.Uri.file(file)) ?? []; + const getIncludesHandler = async () => (await getIncludes(1))?.includedFiles.map(file => vscode.Uri.file(file)) ?? []; const getTraitsHandler = async () => { - const projectContext = await getProjectContext(uri, context, token); + const projectContext = await getProjectContext(uri, context); if (!projectContext) { return undefined; @@ -154,9 +154,9 @@ export async function registerRelatedFilesProvider(): Promise { } } -async function getIncludesWithCancellation(maxDepth: number, token: vscode.CancellationToken): Promise { +async function getIncludes(maxDepth: number): Promise { const activeClient = getActiveClient(); - const includes = await activeClient.getIncludes(maxDepth, token); + const includes = await activeClient.getIncludes(maxDepth); const wksFolder = activeClient.RootUri?.toString(); if (!wksFolder) { diff --git a/Extension/src/LanguageServer/lmTool.ts b/Extension/src/LanguageServer/lmTool.ts index 746ff3829..75ca8b994 100644 --- a/Extension/src/LanguageServer/lmTool.ts +++ b/Extension/src/LanguageServer/lmTool.ts @@ -127,11 +127,11 @@ function filterCompilerArguments(compiler: string, compilerArguments: string[], return result; } -export async function getProjectContext(uri: vscode.Uri, context: { flags: Record }, token: vscode.CancellationToken): Promise { +export async function getProjectContext(uri: vscode.Uri, context: { flags: Record }): Promise { const telemetryProperties: Record = {}; const telemetryMetrics: Record = {}; try { - const projectContext = await checkDuration(async () => await getClients()?.ActiveClient?.getProjectContext(uri, token) ?? undefined); + const projectContext = await checkDuration(async () => await getClients()?.ActiveClient?.getProjectContext(uri) ?? undefined); telemetryMetrics["duration"] = projectContext.duration; if (!projectContext.result) { return undefined; diff --git a/Extension/test/scenarios/SingleRootProject/tests/lmTool.test.ts b/Extension/test/scenarios/SingleRootProject/tests/lmTool.test.ts index f11c048e1..6370f9f8c 100644 --- a/Extension/test/scenarios/SingleRootProject/tests/lmTool.test.ts +++ b/Extension/test/scenarios/SingleRootProject/tests/lmTool.test.ts @@ -130,8 +130,8 @@ describe('CppConfigurationLanguageModelTool Tests', () => { }); const arrangeChatContextFromCppTools = ({ chatContextFromCppTools, isCpp, isHeaderFile }: - { chatContextFromCppTools?: ChatContextResult; isCpp?: boolean; isHeaderFile?: boolean } = - { chatContextFromCppTools: undefined, isCpp: undefined, isHeaderFile: false } + { chatContextFromCppTools?: ChatContextResult; isCpp?: boolean; isHeaderFile?: boolean } = + { chatContextFromCppTools: undefined, isCpp: undefined, isHeaderFile: false } ) => { activeClientStub.getChatContext.resolves(chatContextFromCppTools); sinon.stub(util, 'isCpp').returns(isCpp ?? true); @@ -139,8 +139,8 @@ describe('CppConfigurationLanguageModelTool Tests', () => { }; const arrangeProjectContextFromCppTools = ({ projectContextFromCppTools, isCpp, isHeaderFile }: - { projectContextFromCppTools?: ProjectContextResult; isCpp?: boolean; isHeaderFile?: boolean } = - { projectContextFromCppTools: undefined, isCpp: undefined, isHeaderFile: false } + { projectContextFromCppTools?: ProjectContextResult; isCpp?: boolean; isHeaderFile?: boolean } = + { projectContextFromCppTools: undefined, isCpp: undefined, isHeaderFile: false } ) => { activeClientStub.getProjectContext.resolves(projectContextFromCppTools); sinon.stub(util, 'isCpp').returns(isCpp ?? true); @@ -200,7 +200,7 @@ describe('CppConfigurationLanguageModelTool Tests', () => { } }); - const result = await getProjectContext(mockTextDocumentStub.uri, context, new vscode.CancellationTokenSource().token); + const result = await getProjectContext(mockTextDocumentStub.uri, context); ok(result, 'result should not be undefined'); ok(result.language === 'C++'); @@ -364,7 +364,7 @@ describe('CppConfigurationLanguageModelTool Tests', () => { } }); - const result = await getProjectContext(mockTextDocumentStub.uri, { flags: {} }, new vscode.CancellationTokenSource().token); + const result = await getProjectContext(mockTextDocumentStub.uri, { flags: {} }); ok(telemetryStub.calledOnce, 'Telemetry should be called once'); ok(telemetryStub.calledWithMatch('ProjectContext', sinon.match({ From de21b83dd6d0221828b3680f7a94a95525acebdd Mon Sep 17 00:00:00 2001 From: Sean McManus Date: Mon, 2 Dec 2024 20:25:53 -0800 Subject: [PATCH 5/8] Formatting. --- .../test/scenarios/SingleRootProject/tests/lmTool.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Extension/test/scenarios/SingleRootProject/tests/lmTool.test.ts b/Extension/test/scenarios/SingleRootProject/tests/lmTool.test.ts index 6370f9f8c..afcf90366 100644 --- a/Extension/test/scenarios/SingleRootProject/tests/lmTool.test.ts +++ b/Extension/test/scenarios/SingleRootProject/tests/lmTool.test.ts @@ -130,8 +130,8 @@ describe('CppConfigurationLanguageModelTool Tests', () => { }); const arrangeChatContextFromCppTools = ({ chatContextFromCppTools, isCpp, isHeaderFile }: - { chatContextFromCppTools?: ChatContextResult; isCpp?: boolean; isHeaderFile?: boolean } = - { chatContextFromCppTools: undefined, isCpp: undefined, isHeaderFile: false } + { chatContextFromCppTools?: ChatContextResult; isCpp?: boolean; isHeaderFile?: boolean } = + { chatContextFromCppTools: undefined, isCpp: undefined, isHeaderFile: false } ) => { activeClientStub.getChatContext.resolves(chatContextFromCppTools); sinon.stub(util, 'isCpp').returns(isCpp ?? true); @@ -139,8 +139,8 @@ describe('CppConfigurationLanguageModelTool Tests', () => { }; const arrangeProjectContextFromCppTools = ({ projectContextFromCppTools, isCpp, isHeaderFile }: - { projectContextFromCppTools?: ProjectContextResult; isCpp?: boolean; isHeaderFile?: boolean } = - { projectContextFromCppTools: undefined, isCpp: undefined, isHeaderFile: false } + { projectContextFromCppTools?: ProjectContextResult; isCpp?: boolean; isHeaderFile?: boolean } = + { projectContextFromCppTools: undefined, isCpp: undefined, isHeaderFile: false } ) => { activeClientStub.getProjectContext.resolves(projectContextFromCppTools); sinon.stub(util, 'isCpp').returns(isCpp ?? true); From 197855cdb4533f5139a96a491751a7bc016080ea Mon Sep 17 00:00:00 2001 From: Sean McManus Date: Tue, 3 Dec 2024 15:47:19 -0800 Subject: [PATCH 6/8] Add a comment block. --- Extension/src/LanguageServer/client.ts | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/Extension/src/LanguageServer/client.ts b/Extension/src/LanguageServer/client.ts index 81de99390..d937cff04 100644 --- a/Extension/src/LanguageServer/client.ts +++ b/Extension/src/LanguageServer/client.ts @@ -2228,12 +2228,26 @@ export class DefaultClient implements Client { await this.languageClient.sendNotification(DidOpenNotification, params); } + /** + * Copilot completion-related requests (e.g. getIncludes and getProjectContext) will have their cancellation tokens cancelled + * if the current request times out (showing the user completion results without context info), + * but the results can still be used for future requests (due to caching) so it's better to return results instead of cancelling. + * This is different behavior from the getChatContext, which does handle cancel requests, since the request blocks + * the UI results and always re-requests (no caching). + */ + public async getIncludes(maxDepth: number): Promise { const params: GetIncludesParams = { maxDepth: maxDepth }; await this.ready; return this.languageClient.sendRequest(IncludesRequest, params); } + public async getProjectContext(uri: vscode.Uri): Promise { + const params: TextDocumentIdentifier = { uri: uri.toString() }; + await this.ready; + return this.languageClient.sendRequest(ProjectContextRequest, params); + } + public async getChatContext(uri: vscode.Uri, token: vscode.CancellationToken): Promise { const params: TextDocumentIdentifier = { uri: uri.toString() }; await withCancellation(this.ready, token); @@ -2241,12 +2255,6 @@ export class DefaultClient implements Client { () => this.languageClient.sendRequest(CppContextRequest, params, token), token); } - public async getProjectContext(uri: vscode.Uri): Promise { - const params: TextDocumentIdentifier = { uri: uri.toString() }; - await this.ready; - return this.languageClient.sendRequest(ProjectContextRequest, params); - } - /** * a Promise that can be awaited to know when it's ok to proceed. * From a7839562dce02e3f1e5ec75ddca96ecb5cdaee0a Mon Sep 17 00:00:00 2001 From: Sean McManus Date: Wed, 4 Dec 2024 12:20:04 -0800 Subject: [PATCH 7/8] A couple bug fixes from the previous PR. --- Extension/src/LanguageServer/lmTool.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Extension/src/LanguageServer/lmTool.ts b/Extension/src/LanguageServer/lmTool.ts index 75ca8b994..50240fdd7 100644 --- a/Extension/src/LanguageServer/lmTool.ts +++ b/Extension/src/LanguageServer/lmTool.ts @@ -5,7 +5,7 @@ 'use strict'; import * as vscode from 'vscode'; -import { localize } from 'vscode-nls'; +import * as nls from 'vscode-nls'; import * as util from '../common'; import * as logger from '../logger'; import * as telemetry from '../telemetry'; @@ -13,6 +13,9 @@ import { ChatContextResult, ProjectContextResult } from './client'; import { getClients } from './extension'; import { checkDuration } from './utils'; +nls.config({ messageFormat: nls.MessageFormat.bundle, bundleFormat: nls.BundleFormat.standalone })(); +const localize: nls.LocalizeFunc = nls.loadMessageBundle(); + const MSVC: string = 'MSVC'; const Clang: string = 'Clang'; const GCC: string = 'GCC'; @@ -177,7 +180,7 @@ export async function getProjectContext(uri: vscode.Uri, context: { flags: Recor // Intentionally swallow any exception. } telemetryProperties["error"] = "true"; - return undefined; + throw exception; // Throw the exception for auto-retry. } finally { telemetry.logCopilotEvent('ProjectContext', telemetryProperties, telemetryMetrics); } From dc6e2599f6b1fe778b12648f4a9b729ade0f5f5f Mon Sep 17 00:00:00 2001 From: Sean McManus Date: Wed, 4 Dec 2024 12:33:04 -0800 Subject: [PATCH 8/8] Fix another loc case. --- Extension/src/LanguageServer/copilotProviders.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Extension/src/LanguageServer/copilotProviders.ts b/Extension/src/LanguageServer/copilotProviders.ts index 653fad1d5..a78fdfa5b 100644 --- a/Extension/src/LanguageServer/copilotProviders.ts +++ b/Extension/src/LanguageServer/copilotProviders.ts @@ -5,7 +5,7 @@ 'use strict'; import * as vscode from 'vscode'; -import { localize } from 'vscode-nls'; +import * as nls from 'vscode-nls'; import * as util from '../common'; import * as logger from '../logger'; import * as telemetry from '../telemetry'; @@ -13,6 +13,9 @@ import { GetIncludesResult } from './client'; import { getActiveClient } from './extension'; import { getCompilerArgumentFilterMap, getProjectContext } from './lmTool'; +nls.config({ messageFormat: nls.MessageFormat.bundle, bundleFormat: nls.BundleFormat.standalone })(); +const localize: nls.LocalizeFunc = nls.loadMessageBundle(); + export interface CopilotTrait { name: string; value: string;