Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support for copilot-generated summaries in quick info. (On-the-fly docs) #12552
base: main
Are you sure you want to change the base?
Support for copilot-generated summaries in quick info. (On-the-fly docs) #12552
Changes from 11 commits
0eac8bf
93d7bb0
2667ec5
fb310c8
433f22d
7ec62d9
7a41841
db42258
c73a589
aa6779c
28390f1
3d5b7db
bd1eb43
d9c74c3
c31c4ab
9742465
3bb4f1f
1cf0785
6076ca9
fd7095d
9e1a547
d6a340f
f8cdde6
713f430
10fb3cf
3a0b8d8
1be1fcc
ceb9f8e
9376135
ba5f149
8707994
1b37fd1
f8fc500
223d52b
c055ce2
3af533d
ad991c6
76000c7
0fc52ed
1788813
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 thelsp_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).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.
I'm hoping that since we're no longer going back and forth from the lsp server, this shouldn't be an issue. I've added exception catching around the one call this still does make to the native side. Let me know if there is any additional cancellation I should be tracking still.
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.
It looks like this
sendRequest
was moved togetRequestInfo
and doesn't have a cancellation token passed to it. I think you'll want to capture the cancellation token from the original(?) hover request (or, maybe the subsequent hover request caused byshowCopilotContent
?), and pass that tosendRequest
to communicate that cancellation to the native process. Even if you're not explicitly checking for cancellation (!request.is_active()
) in your LSP message handler, this would allow the LSP manager itself to discard requests in queue that have already been cancelled and to discard the response for an operation already cancelled.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.
Similar feedback to the other call to
sendRequest
. Assuming this happens in the context of a hover operation, could you pass the associated cancellation token? For some of our calls tosendRequest
that are triggered by commands, we don't get a token from VS Code, so don't pass it along. But we should pass along cancellation tokens for any user operation that has one associated with it.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.
We don't directly have the token at call time, but we're relying on other stashed hover values so I've added similar logic for the cancellation token. Also, thanks for the heads up on the multiple send request calls added, this one isn't used anymore so I was able to remove it.
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.
Could you clarify? I would expect a hover-related operation to be conceptually cancelable/dismissible by the user. (It looks like you currently have a bug where, if a user has moved on to another task, a slow CopilotHover request can result in the cursor being repositioned, potentially even when the user is mid-typing?).
I think it's important that this feature handle cancellation. The
cpptools_getCopilotHoverInfo
handler on the native side will acquire a (potentially new) TU, which may impose a hefty perf impact. Even if the TU were already existing, if racing with edit, it may trigger a preparse. There could be other messages in queue, delaying delivery of yours, or piling up after yours.For a lot of users, IntelliSense is very slow. I have a project at home in which a TU can take more than a minute to perform a preparse. If racing with edits, we don't want to waste time fully completing an operation whose result won't be used. As soon as there are subsequent edits in queue, the preparse it performs potentially be entirely wasted.
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.
Sorry, I think my response was extra confusing since this code location doesn't make sense anymore (was removed).
For the other case where we're still calling to the native layer, we're not in the Copilot Hover's call stack directly, so we have to store the last hover cancellation token to use it later when called through the "onCopilotHover" command.
It looks like there's a bug with which token I was holding, so I'm fixing that now to make sure we have the most up-to-date hover cancellation token when checking it and passing it through to the native layer.
I agree that cancellation is important, trying to handle it from the command callback function has been a little more complicated since we're currently invoking multiple follow up hovers. Hopefully we can remove that need in a future update with better platform support, which would really help streamline the cancellation.
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.
In my last response, I was specifically looking at calls to our
languageClient.sendRequest
, trying to ensure they all had tokens passed to them. It looks like one of your last pushes addressed that. I may have been looking at stale code.One possible issue: In
onCopilotHover
, it looks like you're passing in a token from a new (not otherwise referenced) token source:I'm guessing maybe you had to pass a token (undefined wasn't accepted?), and didn't have a valid one?
Are those follow-ups all in the context of the same hover operation? If so, could the original hover operation's token be used for all of them?
Normally, I'd suggest passing an argument when programmatically invoking the command, but it looks like you're embedding the command as text into the hover text. Note that you can still pass an encoded argument to the command using syntax like so:
Follow the call into
onRescanCompilers
for an example of how to extract it. I don't think you'd be able to pass through an object reference, but you could pass through a key that you could associate a value with in a map.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.
Thanks! I'll push out my updated changes soon that seems to handle the cancellation much better by holding on to the hover's cancellation token and tracking which operations are changing the editor's focus more intentionally.
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.
Not sure what standard practice is in cpptools, but should we be sending some error telemetry here too?