-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add C++ configuration as a language model tool #12577
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,7 +61,7 @@ import * as refs from './references'; | |
import { CppSettings, OtherSettings, SettingsParams, WorkspaceFolderSettingsParams, getEditorConfigSettings } from './settings'; | ||
import { SettingsTracker } from './settingsTracker'; | ||
import { ConfigurationType, LanguageStatusUI, getUI } from './ui'; | ||
import { handleChangedFromCppToC, makeLspRange, makeVscodeLocation, makeVscodeRange } from './utils'; | ||
import { handleChangedFromCppToC, makeLspRange, makeVscodeLocation, makeVscodeRange, withCancellation } from './utils'; | ||
import minimatch = require("minimatch"); | ||
|
||
function deepCopy(obj: any) { | ||
|
@@ -541,6 +541,14 @@ interface GetIncludesResult { | |
includedFiles: string[]; | ||
} | ||
|
||
export interface ChatContextResult { | ||
language: string; | ||
standardVersion: string; | ||
compiler: string; | ||
targetPlatform: string; | ||
targetArchitecture: string; | ||
} | ||
|
||
// Requests | ||
const PreInitializationRequest: RequestType<void, string, void> = new RequestType<void, string, void>('cpptools/preinitialize'); | ||
const InitializationRequest: RequestType<CppInitializationParams, void, void> = new RequestType<CppInitializationParams, void, void>('cpptools/initialize'); | ||
|
@@ -560,6 +568,7 @@ const GoToDirectiveInGroupRequest: RequestType<GoToDirectiveInGroupParams, Posit | |
const GenerateDoxygenCommentRequest: RequestType<GenerateDoxygenCommentParams, GenerateDoxygenCommentResult | undefined, void> = new RequestType<GenerateDoxygenCommentParams, GenerateDoxygenCommentResult, void>('cpptools/generateDoxygenComment'); | ||
const ChangeCppPropertiesRequest: RequestType<CppPropertiesParams, void, void> = new RequestType<CppPropertiesParams, void, void>('cpptools/didChangeCppProperties'); | ||
const IncludesRequest: RequestType<GetIncludesParams, GetIncludesResult, void> = new RequestType<GetIncludesParams, GetIncludesResult, void>('cpptools/getIncludes'); | ||
const CppContextRequest: RequestType<void, ChatContextResult, void> = new RequestType<void, ChatContextResult, void>('cpptools/getChatContext'); | ||
|
||
// Notifications to the server | ||
const DidOpenNotification: NotificationType<DidOpenTextDocumentParams> = new NotificationType<DidOpenTextDocumentParams>('textDocument/didOpen'); | ||
|
@@ -790,6 +799,7 @@ export interface Client { | |
setShowConfigureIntelliSenseButton(show: boolean): void; | ||
addTrustedCompiler(path: string): Promise<void>; | ||
getIncludes(maxDepth: number): Promise<GetIncludesResult>; | ||
getChatContext(token: vscode.CancellationToken): Promise<ChatContextResult | undefined>; | ||
} | ||
|
||
export function createClient(workspaceFolder?: vscode.WorkspaceFolder): Client { | ||
|
@@ -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); | ||
|
||
// sendRequest() won't throw on cancellation, but will return an | ||
// unexpected object with an error code and message. | ||
if (token.isCancellationRequested) { | ||
throw new vscode.CancellationError(); | ||
} | ||
|
||
return result; | ||
} | ||
|
||
/** | ||
* a Promise that can be awaited to know when it's ok to proceed. | ||
* | ||
|
@@ -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); } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this return 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for pointing this out. Looks like it is indeed still an issue. I noticed that there are existing code paths like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, that's bubbling all the way up from vscode-jsonrpc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed offline and there's a bug in the way errors are handled in the cpptools server today. Reported at #12591. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LSP cancellation should now be properly reflected by an exception (containing the JSON-RPC error object) from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this comment is still open to address the original question: Should this return Promise<ChatContextResult | undefined> to match the other definition? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Colengms @benmcmorran this one looks not addressed, but I thought it was reading the changes. Could you advice what changes are needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The line current is:
But the interface declares it as:
I'm just suggesting updating the derived class member function declaration be consistent with the base class, like other functions here. So:
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
/* -------------------------------------------------------------------------------------------- | ||
* Copyright (c) Microsoft Corporation. All Rights Reserved. | ||
* See 'LICENSE' in the project root for license information. | ||
* ------------------------------------------------------------------------------------------ */ | ||
'use strict'; | ||
|
||
import * as vscode from 'vscode'; | ||
import * as util from '../common'; | ||
import * as telemetry from '../telemetry'; | ||
import { ChatContextResult } from './client'; | ||
import { getClients } from './extension'; | ||
|
||
const knownValues: { [Property in keyof ChatContextResult]?: { [id: string]: string } } = { | ||
language: { | ||
'c': 'C', | ||
'cpp': 'C++', | ||
'cuda-cpp': 'CUDA C++' | ||
}, | ||
compiler: { | ||
'msvc': 'MSVC', | ||
'clang': 'Clang', | ||
'gcc': 'GCC' | ||
}, | ||
standardVersion: { | ||
'c++98': 'C++98', | ||
'c++03': 'C++03', | ||
'c++11': 'C++11', | ||
'c++14': 'C++14', | ||
'c++17': 'C++17', | ||
'c++20': 'C++20', | ||
'c++23': 'C++23', | ||
'c90': "C90", | ||
'c99': "C99", | ||
'c11': "C11", | ||
'c17': "C17", | ||
'c23': "C23" | ||
}, | ||
targetPlatform: { | ||
'windows': 'Windows', | ||
'Linux': 'Linux', | ||
'macos': 'macOS' | ||
} | ||
}; | ||
|
||
class StringLanguageModelToolResult implements vscode.LanguageModelToolResult | ||
{ | ||
public constructor(public readonly value: string) {} | ||
public toString(): string { return this.value; } | ||
} | ||
|
||
export class CppConfigurationLanguageModelTool implements vscode.LanguageModelTool | ||
{ | ||
public async invoke(_parameters: any, token: vscode.CancellationToken): Promise<vscode.LanguageModelToolResult> { | ||
return new StringLanguageModelToolResult(await this.getContext(token)); | ||
} | ||
|
||
private async getContext(token: vscode.CancellationToken): Promise<string> { | ||
const currentDoc = vscode.window.activeTextEditor?.document; | ||
if (!currentDoc || (!util.isCpp(currentDoc) && !util.isHeaderFile(currentDoc.uri))) { | ||
return 'The active document is not a C, C++, or CUDA file.'; | ||
} | ||
|
||
const chatContext: ChatContextResult | undefined = await (getClients()?.ActiveClient?.getChatContext(token) ?? undefined); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason the implementation of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. |
||
if (!chatContext) { | ||
return 'No configuration information is available for the active document.'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is actual content reporting absence of information needed? Could this be just an empty string? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks good |
||
} | ||
|
||
telemetry.logLanguageModelToolEvent( | ||
'cpp', | ||
{ | ||
"language": chatContext.language, | ||
"compiler": chatContext.compiler, | ||
"standardVersion": chatContext.standardVersion, | ||
"targetPlatform": chatContext.targetPlatform, | ||
"targetArchitecture": chatContext.targetArchitecture | ||
}); | ||
|
||
for (const key in knownValues) { | ||
const knownKey = key as keyof ChatContextResult; | ||
if (knownValues[knownKey] && chatContext[knownKey]) | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you run the formatter on this document? The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sean-mcmanus FYI the formatted is happy with @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. |
||
chatContext[knownKey] = knownValues[knownKey][chatContext[knownKey]] || chatContext[knownKey]; | ||
} | ||
} | ||
|
||
return `The user is working on a ${chatContext.language} project. The project uses language version ${chatContext.standardVersion}, compiles using the ${chatContext.compiler} compiler, targets the ${chatContext.targetPlatform} platform, and targets the ${chatContext.targetArchitecture} architecture.`; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 = ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.