Support for copilot-generated summaries in quick info. (On-the-fly docs) #12552

spebl · 2024-08-08T01:29:10Z

This introduces a new setting C_Cpp.onTheFlyDocsEnabled to control the display of the option to show copilot-generated summaries in the hover tooltip.
When enabled, and also authenticated with the vscode-copilot extension, the hover tooltip will display an option to generate a summary of the symbol with copilot.
The setting is defaulted to default which will check the feature flag control to determine if the feature should be enabled, which allows for slow rollout and the ability to rollback should any issues arise.

Updating the vscode requirement to 1.90.0 to support using copilot features with "vscode.lm".

The IntelliSense client changes supporting this feature are included in a separate PR against that repository.

Some workarounds were needed to support icon rendering in hover markdown and updating hover content dynamically which currently requires the hover to be closed and reopened. Potential fixes from proposals and unreleased patches are linked where applicable.

All feedback is very welcome!

…hen editor isnt selected

benmcmorran

Left a few wording comments but mostly looks good to me ✨. Leaving official sign-off for cpptools owners.

Extension/package.json

Extension/package.nls.json

Extension/src/LanguageServer/extension.ts

benmcmorran · 2024-08-08T16:33:29Z

Extension/src/LanguageServer/extension.ts

+        );
+    } catch (err) {
+        if (err instanceof vscode.LanguageModelError) {
+            console.log(err.message, err.code, err.cause);


Not sure what standard practice is in cpptools, but should we be sending some error telemetry here too?

Extension/src/LanguageServer/settings.ts

Extension/package.json

sean-mcmanus · 2024-08-08T17:48:15Z

@spebl Did you want this in our next 1.22.x or what release?

Extension/package.nls.json

spebl · 2024-08-08T17:55:37Z

@spebl Did you want this in our next 1.22.x or what release?

Not currently targeting a specific release, no need to block anything on this going in. I first wanted to get feedback on this approach, along with the upgrading of the vscode version. I'm taking a look now at how we can best keep support for the older versions while also using the new language model apis when available.

…s and cleanup based on feedback. add waiting spinner.

bobbrow · 2024-08-14T21:43:32Z

Extension/src/LanguageServer/extension.ts

+// This uses several workarounds for interacting with the hover feature.
+// A proposal for dynamic hover content would help, such as the one here (https://github.com/microsoft/vscode/issues/195394)
+async function onCopilotHover(): Promise<void> {
+    if (!vscode.window.activeTextEditor) { return; }


Does our linter allow this? I expected that it would want:

if (!vscode.window.activeTextEditor) { return; }

@bobbrow Our formatter recently changed and allows this currently. Maybe there's some setting we need to change to alter it

I can look into it later. I'm not a fan of this style.

I meant to update this to match the rest of the formatting, I'll update it now. I don't have any strong preference for one or the other.

Colengms · 2024-08-15T00:24:19Z

Extension/src/LanguageServer/client.ts

@@ -1536,7 +1556,7 @@ export class DefaultClient implements Client {
            resetDatabase: resetDatabase,
            edgeMessagesDirectory: path.join(util.getExtensionFilePath("bin"), "messages", getLocaleId()),
            localizedStrings: localizedStrings,
-            settings: this.getAllSettings()
+            settings: await this.getAllSettings()


I'm almost certain we avoided asyncs early in client creation code, as a fix for issues that occurs due to race conditions at startup. I don't recall the exact details at the moment. But the call to createLanguageClient is not awaited (it's called from a constructor, and constructors aren't async). So, with your change, it's possible for the client to get constructed and presumably calls to this.languageClient to occur before that variable is properly set. I think that's why there are no awaits there until after languageClient is set and everything has been hooked up.

Would it be possible to remove this async/await and deliver the setting value without awaiting to see if the flight is enabled? Then, gate the functionality later based on whether the flight is enabled?

Colengms · 2024-08-15T00:32:17Z

Extension/src/LanguageServer/protocolFilter.ts

@@ -89,7 +89,17 @@ export function createProtocolFilter(): Middleware {
        provideHover: async (document, position, token, next: (document: any, position: any, token: any) => any) => clients.ActiveClient.enqueue(async () => {
            const me: Client = clients.getClientFor(document.uri);
            if (me.TrackedDocuments.has(document.uri.toString())) {
-                return next(document, position, token);
+                // Currently needed to support icons.


This makes me nervous. It looks like you're potentially hijacking any hover response, if it happens to contain an array that has a markdown string in it.

I think it would be cleaner if we switched to using a HoverProvider instead of hacking around the LSP hover request. (Then you'd be more easily able to do pre/post processing of the request in TypeScript code. That may also help avoid that async/await to check if the flight is enabled, as that could happen in the a HoverProvider)

Also, I think having a hover provider would allow everything to be done in the same hover request, without needing to add additional custom LSP messages. Instead, you could add additional fields to a primary hover request message, and do any additional work immediately before or after forwarding that to the native process using sendRequest, like the other provider classes.

Colengms · 2024-08-15T00:36:32Z

Extension/src/LanguageServer/client.ts

+    public async showCopilotHover(content: string): Promise<ShowCopilotHoverResult> {
+        const params: ShowCopilotHoverParams = {content: content};
+        await this.ready;
+        return this.languageClient.sendRequest(ShowCopilotHoverRequest, params);


It's common for multiple hover requests to be issued and get backlogged as the user moves the mouse around. As each one is generated, the prior one is cancelled. I think the cancellation token needs to be plumbed up here, all the way to these calls to sendRequest, in order for the lsp_manager on the native side to handle cancellation.

Cancellation will now cause an exception to be thrown from sendRequest, which you could catch, check for the specific cancellation error code, and handle appropriately (i.e., to throw a CancellationError exception, if that is what calling code expects).

benmcmorran · 2024-08-21T22:56:35Z

Capturing notes from a quick discussion I had with @Colengms. He's going to go ahead and move from LSP-based hover to a HoverProvider which should make it easier for us to opt-in to icons and avoid async work during settings initialization. We also discussed a potential alternative using multiple HoverProviders where a provider that actually displays Copilot results simply blocks until the user clicks the Copilot link from the first provider. FYI @spebl

bobbrow · 2024-08-22T20:51:17Z

Extension/package.json

@@ -3286,6 +3286,17 @@
                        "default": false,
                        "markdownDescription": "%c_cpp.configuration.addNodeAddonIncludePaths.markdownDescription%",
                        "scope": "application"
+                    },
+                    "C_Cpp.copilotHover": {


This is being defined in the "Miscellaneous" section of settings. Just checking to make sure that's what you intended.

Colengms · 2024-08-22T21:02:43Z

Extension/src/LanguageServer/extension.ts

+    await vscode.window.showTextDocument(vscode.window.activeTextEditor.document, { preserveFocus: false, selection: new vscode.Selection(hoverPosition, hoverPosition) });
+
+    // Same workaround as above to force the editor to update it's content.
+    await clients.ActiveClient.showCopilotHover('$(loading~spin)');


It would be nice to have a way to do this other than wiggling the mouse.

VS Code generally merges results from multiple providers. I suspect if there are multiple HoverProviders and only one returns results (yet) it will show only one result. If that result had the link in it, and that link 'unblocked' a secondary HoverProvider from returning an AI result, I suspect VS Code would then populate a new 'section' of the tooltip with the additional provider result. That is just a guess, I haven't tried it. :)

I finally got a chance to try this out, and it almost works perfectly, but unfortunately while the provider is blocking waiting for the signal, VS Code reserves hover space for it and displays "Loading..." until it's finally unblocked and the Promise resolves.

If you have any ideas on a way to not show the "Loading..." message, I think everything else should work. So far everything I've tried has hit either that same issue or the issue of the Hover content not updating after being rendered.

Maybe we could put in a request to VS Code, to make this more perfect? :) Since this is AI related, they might consider it a priority. Hypothetically, if there were N hover providers, does it make sense for VS Code to show N Loading... messages? Seems like just one would suffice, and additional results could appear for additional providers, once they provide results. That might be easy for them to fix. I'd suggest opening an issue against VS Code and seeing what they think. https://github.com/microsoft/vscode/issues

sean-mcmanus · 2024-09-14T00:20:58Z

Extension/src/LanguageServer/extension.ts

+    await vscode.commands.executeCommand('editor.action.showHover', { focus: 'noAutoFocus' });
+
+    // Move back and show the correct hover.
+    await clients.ActiveClient.showCopilotHover('$(loading~spin)');


The active document and active client could change after returning from an await. I think this could be leading to bugs I'm seeing when switching between documents and hovering.

sean-mcmanus · 2024-09-14T00:24:09Z

Extension/src/LanguageServer/extension.ts

+// This uses several workarounds for interacting with the hover feature.
+// A proposal for dynamic hover content would help, such as the one here (https://github.com/microsoft/vscode/issues/195394)
+async function onCopilotHover(): Promise<void> {
+    if (!vscode.window.activeTextEditor) { return; }


Hover is supposed to work on a document that is not the active editor.

I see the hover disappear when Generate Copilot summary is clicked and there is no summary result.

sean-mcmanus · 2024-09-14T00:34:22Z

Extension/src/LanguageServer/extension.ts

+
+    // Workaround to force the editor to update it's content, needs to be called from another location first.
+    await vscode.commands.executeCommand('cursorMove', { to: 'right' });
+    await vscode.commands.executeCommand('editor.action.showHover', { focus: 'noAutoFocus' });


If the user changes the document and cursor, it can trigger a different hover provider -- I got GitLens hover to be invoked and the line changed to some random line. It seems like it should save the original document or something.

sean-mcmanus · 2024-09-14T00:39:00Z

Extension/src/LanguageServer/extension.ts

+    const hoverPosition = new vscode.Position(copilotHoverResult.hoverPos.line, copilotHoverResult.hoverPos.character);
+
+    // Make sure the editor has focus.
+    await vscode.window.showTextDocument(vscode.window.activeTextEditor.document, { preserveFocus: false, selection: new vscode.Selection(hoverPosition, hoverPosition) });


If the user switches active documents, it'll move the selection to an incorrect line (i.e. the line number of the previously active document), so they'll be at some random spot in the code which may not even have C++ hover, which is why I was getting only hover from GitLens in my other comment because it moved my cursor to hover over a space that had a GitLens inlay hint on it.

spebl added 7 commits August 5, 2024 12:47

initial on-the-fly docs implementation for vscode

0eac8bf

merge from main

93d7bb0

add localization support

2667ec5

add support for icons in hover

fb310c8

add support for feature flag control

433f22d

add setting and strings

7ec62d9

fix settings when copilot models load later and fix showing copilot w…

7a41841

…hen editor isnt selected

spebl added the Feature: Hover (Quick Info) An issue related to Hover (Quick Info) label Aug 8, 2024

spebl requested review from fearthecowboy, benmcmorran, sean-mcmanus, Colengms, browntarik and a team August 8, 2024 01:29

benmcmorran reviewed Aug 8, 2024

View reviewed changes

sean-mcmanus reviewed Aug 8, 2024

View reviewed changes

Extension/package.json Outdated Show resolved Hide resolved

sean-mcmanus reviewed Aug 8, 2024

View reviewed changes

Extension/package.nls.json Outdated Show resolved Hide resolved

spebl added 4 commits August 14, 2024 14:07

'otf docs' -> 'copilot hover' in naming conventions and strings. fixe…

db42258

…s and cleanup based on feedback. add waiting spinner.

fix spacing

c73a589

merge from main

aa6779c

reset vscode version

28390f1

bobbrow reviewed Aug 14, 2024

View reviewed changes

Colengms requested changes Aug 15, 2024

View reviewed changes

bobbrow reviewed Aug 22, 2024

View reviewed changes

Colengms reviewed Aug 22, 2024

View reviewed changes

bobbrow added this to the 1.22 milestone Aug 23, 2024

merge from main

3d5b7db

spebl added 2 commits September 3, 2024 12:51

merge from main

bd1eb43

merge from main

d9c74c3

sean-mcmanus reviewed Sep 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for copilot-generated summaries in quick info. (On-the-fly docs) #12552

Support for copilot-generated summaries in quick info. (On-the-fly docs) #12552

spebl commented Aug 8, 2024

benmcmorran left a comment

benmcmorran Aug 8, 2024

sean-mcmanus commented Aug 8, 2024

spebl commented Aug 8, 2024 •

edited

Loading

bobbrow Aug 14, 2024

sean-mcmanus Sep 14, 2024

bobbrow Sep 14, 2024

spebl Sep 14, 2024

Colengms Aug 15, 2024

Colengms Aug 15, 2024

Colengms Aug 15, 2024

Colengms Aug 15, 2024 •

edited

Loading

benmcmorran commented Aug 21, 2024

bobbrow Aug 22, 2024

Colengms Aug 22, 2024

spebl Sep 6, 2024

Colengms Sep 6, 2024

sean-mcmanus Sep 14, 2024

sean-mcmanus Sep 14, 2024

sean-mcmanus Sep 14, 2024

sean-mcmanus Sep 14, 2024 •

edited

Loading

sean-mcmanus Sep 14, 2024

Support for copilot-generated summaries in quick info. (On-the-fly docs) #12552

Are you sure you want to change the base?

Support for copilot-generated summaries in quick info. (On-the-fly docs) #12552

Conversation

spebl commented Aug 8, 2024

benmcmorran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sean-mcmanus commented Aug 8, 2024

spebl commented Aug 8, 2024 • edited Loading

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 Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

benmcmorran commented Aug 21, 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

sean-mcmanus Sep 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spebl commented Aug 8, 2024 •

edited

Loading

Colengms Aug 15, 2024 •

edited

Loading

sean-mcmanus Sep 14, 2024 •

edited

Loading