Fix resolving import completion item in LSP #69454
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| @@ -540,6 +540,10 @@ void M() | |||
| itemFromNS1.Data = completionResult.ItemDefaults.Data; | |||
| itemFromNS2.Data = completionResult.ItemDefaults.Data; | |||
|
|
|||
| // Remove the label details as this is the behavior of the VSCode client when resolving completion items. | |||
| itemFromNS1.LabelDetails = null; | |||
There was a problem hiding this comment.
ah this is why the unit tests didn't catch it?
There was a problem hiding this comment.
Yea. And I blame jetlag as the reason why it's not caught by manual testing
| @@ -80,8 +80,11 @@ public async Task<LSP.CompletionItem> HandleRequestAsync(LSP.CompletionItem comp | |||
|
|
|||
| private static bool MatchesLSPCompletionItem(LSP.CompletionItem lspCompletionItem, CompletionItem completionItem) | |||
| { | |||
| // We want to make sure we are resolving the same unimported item in case we have multiple with same name | |||
There was a problem hiding this comment.
Instead of doing this, we could consider adding information to the completion item resolve data that would definitively indicate which one we're resolving. Fine whichever you think is best.
There was a problem hiding this comment.
That does have the advantage of immune to change client might make to the resolve request in the future, with the cost of extra data on each unimported item. Let's take this fix for now, assuming such change would be very rare, but if it turns out not to be the case, we can switch to your proposal.
How do I add integration tests for vscode completion? |
Not setup yet (soonTM) - was more wishing |
ghost
added this to the 
Fix dotnet/vscode-csharp#6051
The bug was introduced in #69265, not sure how I didn't catch it during testing :(