Add C++ configuration as a language model tool #12577

benmcmorran · 2024-08-15T22:35:09Z

This is essentially the same as #12044, but adapted to use the new proposed language model tools API instead of the chat variables API. I'm leaving it in a draft statue until the API is on a path to stabilization and we've resolved issues like microsoft/vscode#225737. I'm also guessing we'll want to add the ability to A/B experiment.

cc @lukka @sinemakinci1

Extension/package.json

Extension/src/LanguageServer/client.ts

Extension/src/LanguageServer/extension.ts

Colengms · 2024-08-16T21:24:54Z

Extension/src/LanguageServer/client.ts

@@ -4084,4 +4107,5 @@ class NullClient implements Client {
    setShowConfigureIntelliSenseButton(show: boolean): void { }
    addTrustedCompiler(path: string): Promise<void> { return Promise.resolve(); }
    getIncludes(): Promise<GetIncludesResult> { return Promise.resolve({} as GetIncludesResult); }
+    getChatContext(token: vscode.CancellationToken): Promise<ChatContextResult> { return Promise.resolve({} as ChatContextResult); }


Should this return Promise<ChatContextResult | undefined> to match the other definition?

It's a bit unusual to return an undefined result field in an LSP response. Have you confirmed that works as expected? A while back, there was some issue that prevented that, requiring us to instead always return an object, with a reason code or optional fields. (That may have been addressed when the lsp_manager was added.) If throwing CancellationError results in a better behavior than returning "No configuration information is available for the active document.", you might consider using server-side cancellation to indicate general failure. Or maybe plan ahead for other failure states and provide a field for that in the result object.

Thanks for pointing this out. Looks like it is indeed still an issue.

I noticed that there are existing code paths like GoToDirectiveInGroupRequest and IncludesRequest (on the native side) that also follow this pattern, so there might need to be a broader cleanup.

@benmcmorran I'm not sure what would surface that specific error into that UI. Is this an exception that is propagated all the up from the call to sendRequest?

Yep, that's bubbling all the way up from vscode-jsonrpc.

Discussed offline and there's a bug in the way errors are handled in the cpptools server today. Reported at #12591.

LSP cancellation should now be properly reflected by an exception (containing the JSON-RPC error object) from sendRequest. I think you still have an issue here if returning an undefined result, though.

I think this comment is still open to address the original question: Should this return Promise<ChatContextResult | undefined> to match the other definition?

@Colengms @benmcmorran this one looks not addressed, but I thought it was reading the changes. Could you advice what changes are needed?

Should this return Promise<ChatContextResult | undefined> to match the other definition?

Could you advice what changes are needed?

The line current is:

getChatContext(token: vscode.CancellationToken): Promise<ChatContextResult> { return Promise.resolve({} as ChatContextResult); }

But the interface declares it as:

getChatContext(token: vscode.CancellationToken): Promise<ChatContextResult | undefined>;

I'm just suggesting updating the derived class member function declaration be consistent with the base class, like other functions here. So:

getChatContext(token: vscode.CancellationToken): Promise<ChatContextResult | undefined> { return Promise.resolve({} as ChatContextResult); }

lukka · 2024-08-16T21:42:04Z

Extension/src/LanguageServer/lmTool.ts

+
+        const chatContext: ChatContextResult | undefined = await (getClients()?.ActiveClient?.getChatContext(token) ?? undefined);
+        if (!chatContext) {
+            return 'No configuration information is available for the active document.';


is actual content reporting absence of information needed? Could this be just an empty string?

sean-mcmanus · 2024-09-04T22:26:38Z

@benmcmorran Is this supposed to be out of draft now?

benmcmorran · 2024-09-05T00:54:09Z

@lukka is handling finalizing this PR, but yes we can take it out of draft. Ideally we'd like to get these changes in the next pre-release. Pending work:

Only enable when the experimental features setting is enabled.
Add runtime type checks for the API and wrap all in try/catch blocks.
Add notification if API interactions fail.

sean-mcmanus · 2024-09-05T02:16:43Z

Extension/src/LanguageServer/lmTool.ts

+        for (const key in knownValues) {
+            const knownKey = key as keyof ChatContextResult;
+            if (knownValues[knownKey] && chatContext[knownKey])
+            {


Can you run the formatter on this document? The if has { on a separate line.

@sean-mcmanus FYI the formatted is happy with { on the next line. I had to move this manually. The formatter used is "dbaeumer.vscode-eslint" as configured in settings.json.

@sean-mcmanus @Colengms @benmcmorran I addressed this comment in this other PR (topmost commit): #12685

I have no access to vscode-cpptools, hence I cant push on this PR.

Colengms · 2024-09-05T02:50:46Z

Extension/src/LanguageServer/client.ts

@@ -2204,6 +2214,19 @@ export class DefaultClient implements Client {
        return this.languageClient.sendRequest(IncludesRequest, params);
    }

+    public async getChatContext(token: vscode.CancellationToken): Promise<ChatContextResult | undefined> {
+        await withCancellation(this.ready, token);
+        const result = await this.languageClient.sendRequest(CppContextRequest, null, token);


Due to the changes to properly trigger exceptions when a request is cancelled, you'll need to use a try/catch block around the call to sendRequest now. i.e.:

let response: ChatContextResult; try { response = await this.client.languageClient.sendRequest(CppContextRequest, null, token); } catch (e: any) { if (e instanceof ResponseError && (e.code === RequestCancelled || e.code === ServerCancelled)) { throw new vscode.CancellationError(); } throw e; } if (token.isCancellationRequested) { throw new vscode.CancellationError(); }

Also, we try to explicitly provide the type when declaring a variable in TypeScript, unless otherwise explicitly stated in the same statement (i.e. = new Typename). I'm not sure why lint didn't complain here.

i.e. const result: ChatContextResult = ...

@Colengms @benmcmorran I addressed this comment in this other PR (topmost commit): #12685

I have no access to vscode-cpptools, hence I cant push on this PR.

Colengms · 2024-09-05T02:54:12Z

Extension/src/LanguageServer/client.ts

@@ -4084,4 +4107,5 @@ class NullClient implements Client {
    setShowConfigureIntelliSenseButton(show: boolean): void { }
    addTrustedCompiler(path: string): Promise<void> { return Promise.resolve(); }
    getIncludes(): Promise<GetIncludesResult> { return Promise.resolve({} as GetIncludesResult); }
+    getChatContext(token: vscode.CancellationToken): Promise<ChatContextResult> { return Promise.resolve({} as ChatContextResult); }


I think this comment is still open to address the original question: Should this return Promise<ChatContextResult | undefined> to match the other definition?

Colengms · 2024-09-05T02:57:49Z

Extension/src/LanguageServer/lmTool.ts

+            return 'The active document is not a C, C++, or CUDA file.';
+        }
+
+        const chatContext: ChatContextResult | undefined = await (getClients()?.ActiveClient?.getChatContext(token) ?? undefined);


Is there a reason the implementation of getChatContext needs to live in the client object? It seems like it might be appropriate here, whereas we're often needing to refactor things out of client.ts into more appropriate places.

@Colengms @benmcmorran I addressed this comment in this other PR (topmost commit): #12685

I have no access to vscode-cpptools, hence I cant push on this PR.

Extension/src/LanguageServer/utils.ts

Colengms · 2024-09-05T18:30:07Z

Extension/src/LanguageServer/utils.ts

+
+export async function withCancellation<T>(promise: Promise<T>, token: vscode.CancellationToken): Promise<T> {
+    return new Promise<T>((resolve, reject) => {
+        token.onCancellationRequested(() => reject(new vscode.CancellationError()));


Could you grab the Disposable that gets returned from this event registration, and dispose of it when resolving? That way, if this function is called multiple times with the same token, multiple event registrations won't get accumulated/leaked.

Colengms · 2024-09-05T18:42:53Z

Closing this one, as Luca has a new one here: #12685

lukka and others added 2 commits August 15, 2024 13:47

#cpp chat variable

9738558

Adapt to lmTool API

9845376

lukka mentioned this pull request Aug 15, 2024

Add context to the Copilot Chat #12044

Closed

Fix lint errors

366e3f5

Colengms requested changes Aug 15, 2024

View reviewed changes

Extension/package.json Outdated Show resolved Hide resolved

Extension/src/LanguageServer/client.ts Outdated Show resolved Hide resolved

Extension/src/LanguageServer/extension.ts Outdated Show resolved Hide resolved

Address PR comments

76ec4f7

Colengms reviewed Aug 16, 2024

View reviewed changes

lukka reviewed Aug 16, 2024

View reviewed changes

benmcmorran marked this pull request as ready for review September 5, 2024 00:54

sean-mcmanus requested changes Sep 5, 2024

View reviewed changes

Colengms requested changes Sep 5, 2024

View reviewed changes

lukka mentioned this pull request Sep 5, 2024

Add C++ configuration as a language model tool (luca) #12685

Merged

Colengms reviewed Sep 5, 2024

View reviewed changes

Colengms closed this Sep 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add C++ configuration as a language model tool #12577

Add C++ configuration as a language model tool #12577

benmcmorran commented Aug 15, 2024

Colengms Aug 16, 2024

benmcmorran Aug 19, 2024

Colengms Aug 19, 2024

benmcmorran Aug 19, 2024

benmcmorran Aug 20, 2024

Colengms Aug 21, 2024

Colengms Sep 5, 2024

lukka Sep 5, 2024

Colengms Sep 5, 2024

lukka Aug 16, 2024

lukka Sep 5, 2024

sean-mcmanus commented Sep 4, 2024

benmcmorran commented Sep 5, 2024

sean-mcmanus Sep 5, 2024

lukka Sep 5, 2024

Colengms Sep 5, 2024

Colengms Sep 5, 2024

lukka Sep 5, 2024

Colengms Sep 5, 2024

Colengms Sep 5, 2024

lukka Sep 5, 2024

Colengms Sep 5, 2024

Colengms commented Sep 5, 2024

Add C++ configuration as a language model tool #12577

Add C++ configuration as a language model tool #12577

Conversation

benmcmorran commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sean-mcmanus commented Sep 4, 2024

benmcmorran commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Colengms commented Sep 5, 2024