Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Register Copilot relatedFilesProvider for C# #7578

Merged
merged 3 commits into from
Sep 20, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions src/lsptoolshost/copilot.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
/*---------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import * as vscode from 'vscode';
import { CSharpExtensionId } from '../constants/csharpExtensionId';
import { CopilotRelatedDocumentsReport, CopilotRelatedDocumentsRequest } from './roslynProtocol';
import { RoslynLanguageServer } from './roslynLanguageServer';
import { UriConverter } from './uriConverter';
import { TextDocumentIdentifier } from 'vscode-languageserver-protocol';

export async function registerCopilotExtension(languageServer: RoslynLanguageServer, channel: vscode.OutputChannel) {
const ext = vscode.extensions.getExtension('github.copilot');
if (!ext) {
channel.appendLine('GitHub Copilot extension not installed. Skipping call to `registerRelatedFilesProvider`');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are technically user visible strings. Consider making it more evident want this actually means in terms of feature set

Copy link
Member Author

@genlu genlu Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, maybe I should move this to the trace log. This is intended to provide additional context implicitly (i.e. no user facing option to turn it on and off)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also fine with this being written when trace logging is enabled. I would still write to the C# output window though

e.g. https://github.com/dotnet/vscode-csharp/blob/main/src/lsptoolshost/roslynLanguageServer.ts#L614

return;
}
await ext.activate();
const relatedAPI = ext.exports as
| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you create a separate type definition for this? It's a little hard to read inline

registerRelatedFilesProvider(
providerId: { extensionId: string; languageId: string },
callback: (
uri: vscode.Uri
) => Promise<{ entries: vscode.Uri[]; traits?: { name: string; value: string }[] }>
): void;
}
| undefined;
if (!relatedAPI) {
channel.appendLine(
'Incompatible GitHub Copilot extension installed. Skipping call to `registerRelatedFilesProvider`'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible to show a notification asking the user to upgrade to a new version if you think it would be valuable. But I'd only do that if the extension is already installed

);
return;
}
channel.appendLine('registerRelatedFilesProvider succeeded.');
genlu marked this conversation as resolved.
Show resolved Hide resolved

const id = {
extensionId: CSharpExtensionId,
languageId: 'csharp',
};

relatedAPI.registerRelatedFilesProvider(id, async (uri) => {
const writeOutput = (output: CopilotRelatedDocumentsReport[], builder: vscode.Uri[] | null) => {
genlu marked this conversation as resolved.
Show resolved Hide resolved
if (output) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the output is actually nullable, it probably should be output?: CopilotRelatedDocumentsReport[]. Otherwise this check should be removed.

The client response type is defined as non-null in roslynProtocol, but the server defines the response as nullable -https://github.com/dotnet/roslyn/pull/74918/files#diff-0b28133c769dc0e92c52213acbee40648526328ab9cec6c7c0e2a9b5a43fa76bR70

It doesn't look like the server ever returns null though - so potentially the server annotation needs to be updated?

for (const report of output) {
if (report._vs_file_paths) {
for (const filePath of report._vs_file_paths) {
channel.appendLine('found related file: ' + filePath);
genlu marked this conversation as resolved.
Show resolved Hide resolved
builder?.push(vscode.Uri.file(filePath));
}
}
}
}
};

const relatedFiles: vscode.Uri[] = [];
const uriString = UriConverter.serialize(uri);
const textDocument = TextDocumentIdentifier.create(uriString);
const responsePromise = languageServer.sendRequestWithProgress(
CopilotRelatedDocumentsRequest.type,
{
_vs_textDocument: textDocument,
position: {
line: 0,
character: 0,
},
},
async (p) => {
writeOutput(p, relatedFiles);
}
);

await responsePromise.then(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect the other places using languageServer.sendRequestWithProgress need to be updated - but I think it should be fine to just await it directly (in a try catch if you want to handle errors).

(result) => {
writeOutput(result, null);
return;
},
(err) => {
channel.appendLine(err);
return;
}
);

return {
entries: relatedFiles,
};
});
}
2 changes: 2 additions & 0 deletions src/lsptoolshost/roslynLanguageServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ import { registerLanguageStatusItems } from './languageStatusBar';
import { ProjectContextService } from './services/projectContextService';
import { ProvideDynamicFileResponse } from '../razor/src/dynamicFile/provideDynamicFileResponse';
import { ProvideDynamicFileParams } from '../razor/src/dynamicFile/provideDynamicFileParams';
import { registerCopilotExtension } from './copilot';

let _channel: vscode.OutputChannel;
let _traceChannel: vscode.OutputChannel;
Expand Down Expand Up @@ -1036,6 +1037,7 @@ export async function activateRoslynLanguageServer(
);

registerLanguageStatusItems(context, languageServer, languageServerEvents);
await registerCopilotExtension(languageServer, _channel);

// Register any commands that need to be handled by the extension.
registerCommands(context, languageServer, hostExecutableResolver, _channel);
Expand Down
22 changes: 22 additions & 0 deletions src/lsptoolshost/roslynProtocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,16 @@ export interface ProjectNeedsRestoreName {
projectFilePaths: string[];
}

export interface CopilotRelatedDocumentsParams extends lsp.WorkDoneProgressParams, lsp.PartialResultParams {
_vs_textDocument: lsp.TextDocumentIdentifier;
position: lsp.Position;
}

export interface CopilotRelatedDocumentsReport {
_vs_resultId: string | null;
genlu marked this conversation as resolved.
Show resolved Hide resolved
_vs_file_paths: string[] | null;
}

export namespace WorkspaceDebugConfigurationRequest {
export const method = 'workspace/debugConfiguration';
export const messageDirection: lsp.MessageDirection = lsp.MessageDirection.clientToServer;
Expand Down Expand Up @@ -330,3 +340,15 @@ export namespace ProjectNeedsRestoreRequest {
export const messageDirection: lsp.MessageDirection = lsp.MessageDirection.serverToClient;
export const type = new lsp.RequestType<ProjectNeedsRestoreName, void, void>(method);
}

export namespace CopilotRelatedDocumentsRequest {
export const method = 'copilot/_related_documents';
export const messageDirection: lsp.MessageDirection = lsp.MessageDirection.clientToServer;
export const type = new lsp.ProtocolRequestType<
CopilotRelatedDocumentsParams,
CopilotRelatedDocumentsReport[],
CopilotRelatedDocumentsReport[],
void,
void
>(method);
}
Loading