Skip to content

Commit

Permalink
Always use related files API for Copilot completions (#12848)
Browse files Browse the repository at this point in the history
  • Loading branch information
@benmcmorran
benmcmorran authored Oct 17, 2024
1 parent 038a5ee commit 9df3a17
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 166 deletions.
31 changes: 0 additions & 31 deletions Extension/src/LanguageServer/copilotProviders.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,9 @@

import * as vscode from 'vscode';
import * as util from '../common';
import * as telemetry from '../telemetry';
import { ChatContextResult, GetIncludesResult } from './client';
import { getActiveClient } from './extension';

let isRelatedFilesApiEnabled: boolean | undefined;

export interface CopilotTrait {
name: string;
value: string;
Expand All @@ -31,10 +28,6 @@ export interface CopilotApi {
}

export async function registerRelatedFilesProvider(): Promise<void> {
if (!await getIsRelatedFilesApiEnabled()) {
return;
}

const api = await getCopilotApi();
if (util.extensionContext && api) {
try {
Expand Down Expand Up @@ -79,12 +72,6 @@ export async function registerRelatedFilesProvider(): Promise<void> {
}
}

export async function registerRelatedFilesCommands(commandDisposables: vscode.Disposable[], enabled: boolean): Promise<void> {
if (await getIsRelatedFilesApiEnabled()) {
commandDisposables.push(vscode.commands.registerCommand('C_Cpp.getIncludes', enabled ? (maxDepth: number) => getIncludes(maxDepth) : () => Promise.resolve()));
}
}

async function getIncludesWithCancellation(maxDepth: number, token: vscode.CancellationToken): Promise<GetIncludesResult> {
const activeClient = getActiveClient();
const includes = await activeClient.getIncludes(maxDepth, token);
Expand All @@ -98,24 +85,6 @@ async function getIncludesWithCancellation(maxDepth: number, token: vscode.Cance
return includes;
}

async function getIncludes(maxDepth: number): Promise<GetIncludesResult> {
const tokenSource = new vscode.CancellationTokenSource();
try {
const includes = await getIncludesWithCancellation(maxDepth, tokenSource.token);
return includes;
} finally {
tokenSource.dispose();
}
}

async function getIsRelatedFilesApiEnabled(): Promise<boolean> {
if (isRelatedFilesApiEnabled === undefined) {
isRelatedFilesApiEnabled = await telemetry.isExperimentEnabled("CppToolsRelatedFilesApi");
}

return isRelatedFilesApiEnabled;
}

export async function getCopilotApi(): Promise<CopilotApi | undefined> {
const copilotExtension = vscode.extensions.getExtension<CopilotApi>('github.copilot');
if (!copilotExtension) {
Expand Down
4 changes: 1 addition & 3 deletions Extension/src/LanguageServer/extension.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import * as telemetry from '../telemetry';
import { Client, DefaultClient, DoxygenCodeActionCommandArguments, openFileVersions } from './client';
import { ClientCollection } from './clientCollection';
import { CodeActionDiagnosticInfo, CodeAnalysisDiagnosticIdentifiersAndUri, codeAnalysisAllFixes, codeAnalysisCodeToFixes, codeAnalysisFileToCodeActions } from './codeAnalysis';
import { registerRelatedFilesCommands, registerRelatedFilesProvider } from './copilotProviders';
import { registerRelatedFilesProvider } from './copilotProviders';
import { CppBuildTaskProvider } from './cppBuildTaskProvider';
import { getCustomConfigProviders } from './customProviders';
import { getLanguageConfig } from './languageConfig';
Expand Down Expand Up @@ -411,8 +411,6 @@ export async function registerCommands(enabled: boolean): Promise<void> {
commandDisposables.push(vscode.commands.registerCommand('C_Cpp.ExtractToFreeFunction', enabled ? () => onExtractToFunction(true, false) : onDisabledCommand));
commandDisposables.push(vscode.commands.registerCommand('C_Cpp.ExtractToMemberFunction', enabled ? () => onExtractToFunction(false, true) : onDisabledCommand));
commandDisposables.push(vscode.commands.registerCommand('C_Cpp.ExpandSelection', enabled ? (r: Range) => onExpandSelection(r) : onDisabledCommand));

await registerRelatedFilesCommands(commandDisposables, enabled);
}

function onDisabledCommand() {
Expand Down
138 changes: 6 additions & 132 deletions Extension/test/scenarios/SingleRootProject/tests/copilotProviders.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,9 @@ import * as util from '../../../../src/common';
import { ChatContextResult, DefaultClient, GetIncludesResult } from '../../../../src/LanguageServer/client';
import { CopilotApi, CopilotTrait } from '../../../../src/LanguageServer/copilotProviders';
import * as extension from '../../../../src/LanguageServer/extension';
import * as telemetry from '../../../../src/telemetry';

describe('registerRelatedFilesProvider', () => {
let moduleUnderTest: any;
let getIsRelatedFilesApiEnabledStub: sinon.SinonStub;
let mockCopilotApi: sinon.SinonStubbedInstance<CopilotApi>;
let getActiveClientStub: sinon.SinonStub;
let activeClientStub: sinon.SinonStubbedInstance<DefaultClient>;
Expand All @@ -32,7 +30,6 @@ describe('registerRelatedFilesProvider', () => {
{} // Stub if you need to, or keep the object empty
);

getIsRelatedFilesApiEnabledStub = sinon.stub(telemetry, 'isExperimentEnabled');
sinon.stub(util, 'extensionContext').value({ extension: { id: 'test-extension-id' } });

class MockCopilotApi implements CopilotApi {
Expand Down Expand Up @@ -71,11 +68,10 @@ describe('registerRelatedFilesProvider', () => {
sinon.restore();
});

const arrange = ({ cppToolsRelatedFilesApi, vscodeExtension, getIncludeFiles, chatContext, rootUri, flags }:
{ cppToolsRelatedFilesApi: boolean; vscodeExtension?: vscode.Extension<unknown>; getIncludeFiles?: GetIncludesResult; chatContext?: ChatContextResult; rootUri?: vscode.Uri; flags?: Record<string, unknown> } =
{ cppToolsRelatedFilesApi: false, vscodeExtension: undefined, getIncludeFiles: undefined, chatContext: undefined, rootUri: undefined, flags: {} }
const arrange = ({ vscodeExtension, getIncludeFiles, chatContext, rootUri, flags }:
{ vscodeExtension?: vscode.Extension<unknown>; getIncludeFiles?: GetIncludesResult; chatContext?: ChatContextResult; rootUri?: vscode.Uri; flags?: Record<string, unknown> } =
{ vscodeExtension: undefined, getIncludeFiles: undefined, chatContext: undefined, rootUri: undefined, flags: {} }
) => {
getIsRelatedFilesApiEnabledStub.withArgs('CppToolsRelatedFilesApi').resolves(cppToolsRelatedFilesApi);
activeClientStub.getIncludes.resolves(getIncludeFiles);
activeClientStub.getChatContext.resolves(chatContext);
sinon.stub(activeClientStub, 'RootUri').get(() => rootUri);
Expand All @@ -95,31 +91,19 @@ describe('registerRelatedFilesProvider', () => {
vscodeGetExtensionsStub = sinon.stub(vscode.extensions, 'getExtension').returns(vscodeExtension);
};

it('should not register provider if CppToolsRelatedFilesApi is not enabled', async () => {
it('should register provider', async () => {
arrange(
{ cppToolsRelatedFilesApi: false }
{ vscodeExtension: vscodeExtension }
);

await moduleUnderTest.registerRelatedFilesProvider();

ok(getIsRelatedFilesApiEnabledStub.calledOnce, 'getIsRelatedFilesApiEnabled should be called once');
});

it('should register provider if CppToolsRelatedFilesApi is enabled', async () => {
arrange(
{ cppToolsRelatedFilesApi: true, vscodeExtension: vscodeExtension }
);

await moduleUnderTest.registerRelatedFilesProvider();

ok(getIsRelatedFilesApiEnabledStub.calledOnce, 'getIsRelatedFilesApiEnabled should be called once');
ok(vscodeGetExtensionsStub.calledOnce, 'vscode.extensions.getExtension should be called once');
ok(mockCopilotApi.registerRelatedFilesProvider.calledWithMatch(sinon.match({ extensionId: 'test-extension-id', languageId: sinon.match.in(['c', 'cpp', 'cuda-cpp']) })), 'registerRelatedFilesProvider should be called with the correct providerId and languageId');
});

it('should not add #cpp traits when ChatContext isn\'t available.', async () => {
arrange({
cppToolsRelatedFilesApi: true,
vscodeExtension: vscodeExtension,
getIncludeFiles: { includedFiles: ['c:\\system\\include\\vector', 'c:\\system\\include\\string', 'C:\\src\\my_project\\foo.h'] },
chatContext: undefined,
Expand All @@ -130,7 +114,6 @@ describe('registerRelatedFilesProvider', () => {

const result = await callbackPromise;

ok(getIsRelatedFilesApiEnabledStub.calledOnce, 'getIsRelatedFilesApiEnabled should be called once');
ok(vscodeGetExtensionsStub.calledOnce, 'vscode.extensions.getExtension should be called once');
ok(mockCopilotApi.registerRelatedFilesProvider.calledWithMatch(sinon.match({ extensionId: 'test-extension-id', languageId: sinon.match.in(['c', 'cpp', 'cuda-cpp']) })), 'registerRelatedFilesProvider should be called with the correct providerId and languageId');
ok(getActiveClientStub.callCount !== 0, 'getActiveClient should be called');
Expand All @@ -143,7 +126,6 @@ describe('registerRelatedFilesProvider', () => {

it('should not add #cpp traits when copilotcppTraits flag is false.', async () => {
arrange({
cppToolsRelatedFilesApi: true,
vscodeExtension: vscodeExtension,
getIncludeFiles: { includedFiles: ['c:\\system\\include\\vector', 'c:\\system\\include\\string', 'C:\\src\\my_project\\foo.h'] },
chatContext: {
Expand All @@ -160,7 +142,6 @@ describe('registerRelatedFilesProvider', () => {

const result = await callbackPromise;

ok(getIsRelatedFilesApiEnabledStub.calledOnce, 'getIsRelatedFilesApiEnabled should be called once');
ok(vscodeGetExtensionsStub.calledOnce, 'vscode.extensions.getExtension should be called once');
ok(mockCopilotApi.registerRelatedFilesProvider.calledWithMatch(sinon.match({ extensionId: 'test-extension-id', languageId: sinon.match.in(['c', 'cpp', 'cuda-cpp']) })), 'registerRelatedFilesProvider should be called with the correct providerId and languageId');
ok(getActiveClientStub.callCount !== 0, 'getActiveClient should be called');
Expand All @@ -173,7 +154,6 @@ describe('registerRelatedFilesProvider', () => {

it('should add #cpp traits when copilotcppTraits flag is true.', async () => {
arrange({
cppToolsRelatedFilesApi: true,
vscodeExtension: vscodeExtension,
getIncludeFiles: { includedFiles: ['c:\\system\\include\\vector', 'c:\\system\\include\\string', 'C:\\src\\my_project\\foo.h'] },
chatContext: {
Expand All @@ -190,7 +170,6 @@ describe('registerRelatedFilesProvider', () => {

const result = await callbackPromise;

ok(getIsRelatedFilesApiEnabledStub.calledOnce, 'getIsRelatedFilesApiEnabled should be called once');
ok(vscodeGetExtensionsStub.calledOnce, 'vscode.extensions.getExtension should be called once');
ok(mockCopilotApi.registerRelatedFilesProvider.calledThrice, 'registerRelatedFilesProvider should be called three times');
ok(mockCopilotApi.registerRelatedFilesProvider.calledWithMatch(sinon.match({ extensionId: 'test-extension-id', languageId: sinon.match.in(['c', 'cpp', 'cuda-cpp']) })), 'registerRelatedFilesProvider should be called with the correct providerId and languageId');
Expand Down Expand Up @@ -226,7 +205,6 @@ describe('registerRelatedFilesProvider', () => {
it('should exclude #cpp traits per copilotcppExcludeTraits.', async () => {
const excludeTraits = ['compiler', 'targetPlatform'];
arrange({
cppToolsRelatedFilesApi: true,
vscodeExtension: vscodeExtension,
getIncludeFiles: { includedFiles: ['c:\\system\\include\\vector', 'c:\\system\\include\\string', 'C:\\src\\my_project\\foo.h'] },
chatContext: {
Expand All @@ -243,7 +221,6 @@ describe('registerRelatedFilesProvider', () => {

const result = await callbackPromise;

ok(getIsRelatedFilesApiEnabledStub.calledOnce, 'getIsRelatedFilesApiEnabled should be called once');
ok(vscodeGetExtensionsStub.calledOnce, 'vscode.extensions.getExtension should be called once');
ok(mockCopilotApi.registerRelatedFilesProvider.calledThrice, 'registerRelatedFilesProvider should be called three times');
ok(mockCopilotApi.registerRelatedFilesProvider.calledWithMatch(sinon.match({ extensionId: 'test-extension-id', languageId: sinon.match.in(['c', 'cpp', 'cuda-cpp']) })), 'registerRelatedFilesProvider should be called with the correct providerId and languageId');
Expand All @@ -258,114 +235,11 @@ describe('registerRelatedFilesProvider', () => {
});

it('should handle errors during provider registration', async () => {
arrange(
{ cppToolsRelatedFilesApi: true }
);
arrange({});

await moduleUnderTest.registerRelatedFilesProvider();

ok(getIsRelatedFilesApiEnabledStub.calledOnce, 'getIsRelatedFilesApiEnabled should be called once');
ok(vscodeGetExtensionsStub.calledOnce, 'vscode.extensions.getExtension should be called once');
ok(mockCopilotApi.registerRelatedFilesProvider.notCalled, 'registerRelatedFilesProvider should not be called');
});
});

describe('registerRelatedFilesCommands', () => {
let moduleUnderTest: any;
let getIsRelatedFilesApiEnabledStub: sinon.SinonStub;
let registerCommandStub: sinon.SinonStub;
let commandDisposables: vscode.Disposable[];
let getActiveClientStub: sinon.SinonStub;
let activeClientStub: sinon.SinonStubbedInstance<DefaultClient>;
let getIncludesResult: Promise<GetIncludesResult | undefined> = Promise.resolve(undefined);

beforeEach(() => {
proxyquire.noPreserveCache(); // Tells proxyquire to not fetch the module from cache
// Ensures that each test has a freshly loaded instance of moduleUnderTest
moduleUnderTest = proxyquire(
'../../../../src/LanguageServer/copilotProviders',
{} // Stub if you need to, or keep the object empty
);

activeClientStub = sinon.createStubInstance(DefaultClient);
getActiveClientStub = sinon.stub(extension, 'getActiveClient').returns(activeClientStub);
activeClientStub.getIncludes.resolves({ includedFiles: [] });
getIsRelatedFilesApiEnabledStub = sinon.stub(telemetry, 'isExperimentEnabled');
registerCommandStub = sinon.stub(vscode.commands, 'registerCommand');
commandDisposables = [];
});

afterEach(() => {
sinon.restore();
});

const arrange = ({ cppToolsRelatedFilesApi, getIncludeFiles, rootUri }:
{ cppToolsRelatedFilesApi: boolean; getIncludeFiles?: GetIncludesResult; rootUri?: vscode.Uri } =
{ cppToolsRelatedFilesApi: false, getIncludeFiles: undefined, rootUri: undefined }
) => {
getIsRelatedFilesApiEnabledStub.withArgs('CppToolsRelatedFilesApi').resolves(cppToolsRelatedFilesApi);
activeClientStub.getIncludes.resolves(getIncludeFiles);
sinon.stub(activeClientStub, 'RootUri').get(() => rootUri);
registerCommandStub.callsFake((command: string, callback: (maxDepth: number) => Promise<GetIncludesResult>) => {
getIncludesResult = callback(1);
});
};

it('should register C_Cpp.getIncludes command if CppToolsRelatedFilesApi is enabled', async () => {
arrange({ cppToolsRelatedFilesApi: true });

await moduleUnderTest.registerRelatedFilesCommands(commandDisposables, true);

ok(getIsRelatedFilesApiEnabledStub.calledOnce, 'getIsRelatedFilesApiEnabled should be called once');
ok(registerCommandStub.calledOnce, 'registerCommand should be called once');
ok(commandDisposables.length === 1, 'commandDisposables should have one disposable');
ok(registerCommandStub.calledWithMatch('C_Cpp.getIncludes', sinon.match.func), 'registerCommand should be called with "C_Cpp.getIncludes" and a function');
});

it('should register C_Cpp.getIncludes command that can handle requests properly', async () => {
arrange({
cppToolsRelatedFilesApi: true,
getIncludeFiles: { includedFiles: ['c:\\system\\include\\vector', 'c:\\system\\include\\string', 'C:\\src\\my_project\\foo1.h', 'C:\\src\\my_project\\foo2.h'] },
rootUri: vscode.Uri.file('C:\\src\\my_project')
});
await moduleUnderTest.registerRelatedFilesCommands(commandDisposables, true);

const includesResult = await getIncludesResult;

ok(getIsRelatedFilesApiEnabledStub.calledOnce, 'getIsRelatedFilesApiEnabled should be called once');
ok(getActiveClientStub.calledOnce, 'getActiveClient should be called once');
ok(includesResult, 'includesResult should be defined');
ok(includesResult?.includedFiles.length === 2, 'includesResult should have 2 included files');
ok(includesResult?.includedFiles[0] === 'C:\\src\\my_project\\foo1.h', 'includesResult should have "C:\\src\\my_project\\foo1.h"');
ok(includesResult?.includedFiles[1] === 'C:\\src\\my_project\\foo2.h', 'includesResult should have "C:\\src\\my_project\\foo2.h"');
});

it('should not register C_Cpp.getIncludes command if CppToolsRelatedFilesApi is not enabled', async () => {
arrange({
cppToolsRelatedFilesApi: false
});

await moduleUnderTest.registerRelatedFilesCommands(commandDisposables, true);

ok(getIsRelatedFilesApiEnabledStub.calledOnce, 'getIsRelatedFilesApiEnabled should be called once');
ok(registerCommandStub.notCalled, 'registerCommand should not be called');
ok(commandDisposables.length === 0, 'commandDisposables should be empty');
});

it('should register C_Cpp.getIncludes as no-op command if enabled is false', async () => {
arrange({
cppToolsRelatedFilesApi: true,
getIncludeFiles: { includedFiles: ['c:\\system\\include\\vector', 'c:\\system\\include\\string', 'C:\\src\\my_project\\foo.h'] },
rootUri: vscode.Uri.file('C:\\src\\my_project')
});
await moduleUnderTest.registerRelatedFilesCommands(commandDisposables, false);

const includesResult = await getIncludesResult;

ok(getIsRelatedFilesApiEnabledStub.calledOnce, 'getIsRelatedFilesApiEnabled should be called once');
ok(registerCommandStub.calledOnce, 'registerCommand should be called once');
ok(commandDisposables.length === 1, 'commandDisposables should have one disposable');
ok(includesResult === undefined, 'includesResult should be undefined');
ok(registerCommandStub.calledWithMatch('C_Cpp.getIncludes', sinon.match.func), 'registerCommand should be called with "C_Cpp.getIncludes" and a function');
});
});

0 comments on commit 9df3a17

Please sign in to comment.