Skip to content

Commit

Permalink
Fix an unnecessary cancel/re-request with GitHub Copilot requests (an…
Browse files Browse the repository at this point in the history 
…d fix some other bugs with Copilot exception handling) (#12988)

* Fix an unnecessary cancel/re-request with GitHub Copilot requests.
* A couple bug fixes from the previous PR.
* Fix another loc case.
  • Loading branch information
@sean-mcmanus
sean-mcmanus authored Dec 4, 2024
1 parent 74d691f commit 052bcff
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 26 deletions.
33 changes: 19 additions & 14 deletions Extension/src/LanguageServer/client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -804,9 +804,9 @@ export interface Client {
getShowConfigureIntelliSenseButton(): boolean;
setShowConfigureIntelliSenseButton(show: boolean): void;
addTrustedCompiler(path: string): Promise<void>;
getIncludes(maxDepth: number, token: vscode.CancellationToken): Promise<GetIncludesResult>;
getIncludes(maxDepth: number): Promise<GetIncludesResult>;
getChatContext(uri: vscode.Uri, token: vscode.CancellationToken): Promise<ChatContextResult>;
getProjectContext(uri: vscode.Uri, token: vscode.CancellationToken): Promise<ProjectContextResult>;
getProjectContext(uri: vscode.Uri): Promise<ProjectContextResult>;
}

export function createClient(workspaceFolder?: vscode.WorkspaceFolder): Client {
Expand Down Expand Up @@ -2228,25 +2228,31 @@ export class DefaultClient implements Client {
await this.languageClient.sendNotification(DidOpenNotification, params);
}

public async getIncludes(maxDepth: number, token: vscode.CancellationToken): Promise<GetIncludesResult> {
/**
* 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<GetIncludesResult> {
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<ChatContextResult> {
public async getProjectContext(uri: vscode.Uri): Promise<ProjectContextResult> {
const params: TextDocumentIdentifier = { uri: uri.toString() };
await withCancellation(this.ready, token);
return DefaultClient.withLspCancellationHandling(
() => this.languageClient.sendRequest(CppContextRequest, params, token), token);
await this.ready;
return this.languageClient.sendRequest(ProjectContextRequest, params);
}

public async getProjectContext(uri: vscode.Uri, token: vscode.CancellationToken): Promise<ProjectContextResult> {
public async getChatContext(uri: vscode.Uri, token: vscode.CancellationToken): Promise<ChatContextResult> {
const params: TextDocumentIdentifier = { uri: uri.toString() };
await withCancellation(this.ready, token);
return DefaultClient.withLspCancellationHandling(
() => this.languageClient.sendRequest(ProjectContextRequest, params, token), token);
() => this.languageClient.sendRequest(CppContextRequest, params, token), token);
}

/**
Expand Down Expand Up @@ -2340,7 +2346,6 @@ export class DefaultClient implements Client {
throw e;
}
}

if (token.isCancellationRequested) {
throw new vscode.CancellationError();
}
Expand Down Expand Up @@ -4151,7 +4156,7 @@ class NullClient implements Client {
getShowConfigureIntelliSenseButton(): boolean { return false; }
setShowConfigureIntelliSenseButton(show: boolean): void { }
addTrustedCompiler(path: string): Promise<void> { return Promise.resolve(); }
getIncludes(maxDepth: number, token: vscode.CancellationToken): Promise<GetIncludesResult> { return Promise.resolve({} as GetIncludesResult); }
getIncludes(maxDepth: number): Promise<GetIncludesResult> { return Promise.resolve({} as GetIncludesResult); }
getChatContext(uri: vscode.Uri, token: vscode.CancellationToken): Promise<ChatContextResult> { return Promise.resolve({} as ChatContextResult); }
getProjectContext(uri: vscode.Uri, token: vscode.CancellationToken): Promise<ProjectContextResult> { return Promise.resolve({} as ProjectContextResult); }
getProjectContext(uri: vscode.Uri): Promise<ProjectContextResult> { return Promise.resolve({} as ProjectContextResult); }
}
15 changes: 9 additions & 6 deletions Extension/src/LanguageServer/copilotProviders.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,17 @@
'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';
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;
Expand All @@ -38,14 +41,14 @@ export async function registerRelatedFilesProvider(): Promise<void> {
for (const languageId of ['c', 'cpp', 'cuda-cpp']) {
api.registerRelatedFilesProvider(
{ extensionId: util.extensionContext.extension.id, languageId },
async (uri: vscode.Uri, context: { flags: Record<string, unknown> }, token: vscode.CancellationToken) => {
async (uri: vscode.Uri, context: { flags: Record<string, unknown> }) => {
const start = performance.now();
const telemetryProperties: Record<string, string> = {};
const telemetryMetrics: Record<string, number> = {};
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;
Expand Down Expand Up @@ -154,9 +157,9 @@ export async function registerRelatedFilesProvider(): Promise<void> {
}
}

async function getIncludesWithCancellation(maxDepth: number, token: vscode.CancellationToken): Promise<GetIncludesResult> {
async function getIncludes(maxDepth: number): Promise<GetIncludesResult> {
const activeClient = getActiveClient();
const includes = await activeClient.getIncludes(maxDepth, token);
const includes = await activeClient.getIncludes(maxDepth);
const wksFolder = activeClient.RootUri?.toString();

if (!wksFolder) {
Expand Down
11 changes: 7 additions & 4 deletions Extension/src/LanguageServer/lmTool.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,17 @@
'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';
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';
Expand Down Expand Up @@ -127,11 +130,11 @@ function filterCompilerArguments(compiler: string, compilerArguments: string[],
return result;
}

export async function getProjectContext(uri: vscode.Uri, context: { flags: Record<string, unknown> }, token: vscode.CancellationToken): Promise<ProjectContext | undefined> {
export async function getProjectContext(uri: vscode.Uri, context: { flags: Record<string, unknown> }): Promise<ProjectContext | undefined> {
const telemetryProperties: Record<string, string> = {};
const telemetryMetrics: Record<string, number> = {};
try {
const projectContext = await checkDuration<ProjectContextResult | undefined>(async () => await getClients()?.ActiveClient?.getProjectContext(uri, token) ?? undefined);
const projectContext = await checkDuration<ProjectContextResult | undefined>(async () => await getClients()?.ActiveClient?.getProjectContext(uri) ?? undefined);
telemetryMetrics["duration"] = projectContext.duration;
if (!projectContext.result) {
return undefined;
Expand Down Expand Up @@ -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);
}
Expand Down
4 changes: 2 additions & 2 deletions Extension/test/scenarios/SingleRootProject/tests/lmTool.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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++');
Expand Down Expand Up @@ -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({
Expand Down

0 comments on commit 052bcff

Please sign in to comment.