Code completion on override method doesn't apply correctly #76141
Comments
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
arunchndr
added
VSCode
and removed
untriaged
|
While I myself am not seeing roslyn send any exceptions, I still have the same issue as you. This also extends to attempting to perform completion in xml comments. (Nevermind that, I wasnt seeing any exceptions because blink.cmp wasnt calling the lsp command via Excuse me for saying this, but while Im no Expert when it comes to the Language Server Protocol, it seems like Roslyn is once again making things unnecessarily difficult for external editors such as neovim. I have compared the messages for completion for roslyn and lua_ls: Roslyn output{
id = 1840,
jsonrpc = "2.0",
result = {
command = {
arguments = {
{
uri = "file:///...." -- Valid File URI
},
{
newText = "<!---->",
range = {
["end"] = {
character = 4,
line = 11
},
start = {
character = 4,
line = 11
}
}
},
false,
287
},
command = "roslyn.client.completionComplexEdit",
title = "CompleteComplexEditCommand"
},
commitCharacters = {},
data = {
ResultId = 44,
TextDocument = {
uri = "file:///...." -- Valid File URI
}
},
documentation = {
kind = "markdown",
value = ""
},
insertTextFormat = 1,
kind = 14,
label = "!--",
textEdit = {
newText = "",
range = {
["end"] = {
character = 4,
line = 11
},
start = {
character = 4,
line = 11
}
}
},
textEditText = ""
}
}lua_ls output{
id = 748,
jsonrpc = "2.0",
result = {
detail = "C:\\Users\\[myUsername]\\AppData\\Local\\nvim-data\\mason\\packages\\lua-language-server\\meta\\Lua 5.4 en-us utf8\\coroutine.lua",
documentation = {
kind = "markdown",
value = "* [[meta]](file:///c%3A/Users/[myUsername]/AppData/Local/nvim-data/mason/packages/lua-language-server/meta/Lua%205.4%20en-us%20utf8/coroutine.lua)"
},
insertTextFormat = 2,
kind = 17,
label = "coroutine",
score = 0,
sortText = "0001",
textEdit = {
newText = "coroutine",
range = {
["end"] = {
character = 9,
line = 0
},
start = {
character = 9,
line = 0
}
}
}
}
}The most notable difference here, is that lua_ls puts the completion data into the |
This is a complex edit, requiring a command, because not only do we have to insert the method body, we might also be adding imports at the top of the file. On top of that, we have to place the caret properly after inserting the full method body. This isn't possible with the simple textual API provided for completion.
Roslyn does this for most completions as well. However, as mentioned above, override completion is a complex case that goes beyond a simple text edit. |
At least partially incorrect.
While that is statistically correct, seeing that both XML-comment- and override-completion use the |
|
Furthermore, I am really curious to know why it is that the same functionality thats broken in neovim (and likely also other editors), works just fine in vscode. |
The above does not work here. Lsp requires things like the additional edits to be returned with the results of completion. They cannot be returned through a callback when completion is invoked. But this doesn't work for our complex edits because they are too computationally complex to compute up front (not hypothetical, we've measured this). Imagine a type with 100 complex overrides. Having to actually compute all the changes they all perform, and also see where they move the caret, and also then convert that to additional edits is not feasible within the time budget allowed.
You'd have to check with neovim. |
Override completion is a complex case. So we break it into the fast computation of what goes into the completion list, and the change that is made when completion is invoked. It is much more expensive than the other cases where we do use normal additional edits. |
Roslyn's responsibility is to follow the lsp spec. If we are doing that, then the bug lies in the consumer of it. If we are not following the lsp spec, then that's something we can fix. If you can examine the neovim side to see what's up, we can determine if it's an issue on their end our end. |
|
I've linked in the original post a video with the same behavior happening in Razor VSCode, so being a neovim specific issue is highly unlikely in my opinion. As for why it is not happening for normal C# code in VSCode, my best guess is that is was somehow "patched" client side in that scenario as a lot of things unfortunately are. At the very least on the specified Roslyn version I get the aforementioned exception from the server and I don't see anything wrong with the client request, at least to my limited understanding of how it should work If there is any specific additional info, testing, or something else required please feel free to ask. |
|
Wdym "patched client side"? |
|
@dibarbet do you know what would cause: |
Razor is not this repo. If they are mishandling things here, that would be GitHub.com/dotnet/razor :-) |
Well, it would be good to know why neovim borks on the lsp responses containing complex edits. |
That is a very good point, which I failed to consider. Sorry for the trouble that I may have caused. One last question on this topic though: how do XML comments fit into this? At least some XML completions, like |
As far as I could tell when I dug into Roslyns code (and from the call stack on the exception) it seemed like Roslyn was trying to initialize some lsp handler(s) (I believe?), which was/where left uninitialized in a |
Not trivial to say. The specification explicitly says:
XML completion very much manipulates the cursor. So it seems like it would be inappropriate to try to use additional text edits here. |
We will look into that. But why is that impacting neovim. If it did get a normal lsp competing completions result back, it should be fine. Unless it is explicitly the "completion item resolve" call back into Roslyn that is failing. |
Well, one part does anyway. Let me re-explain my previously mentioned idea with the XML comment as an example: |
As mentioned previously, its not the completion request, nor the completion resolve request, but the execute command request that is failing and throwing the exception |
Ok. We can look into that. |
Tbh, I don't believe so. For the reason above. It's Ill defined what a client would/should do here. For example, it could end up with the caret after the --> Furthermore, this just simply isn't conducive to how Roslyn is built. This information isn't in our completion item. Our own design is that when the item is selected, it going through a resolution step that performs all the computation and does things like set the caret. We'd have to move to a different model for all of these cases. Our model works, and is explicitly supported by lsp. It means we can have the same core base code for lsp and noon lsp hosts. Fundamentally, the rules are simple here. If we are not following the lsp spec, it's in us to fix. If we are, it's in the client. :-) |
|
As noted above, the cost of computing the full edits upfront is expensive - too expensive to provide upfront for all completion items in the list. Adding on to the problem, VSCode does not support lazily resolving the main Instead, we fill in the command field during This of course has its own issues (for example fast typing can cancel the application of the override item). But this is what we came up with that worked within the limitations of VSCode. I'm not 100% sure why this I am open to making things easier here for other clients. For example, if a client indicated in the completion capabilities that |
I already saw that, however I was unsure wether that actually got called as a result of the
That would be awesome, and is also pretty much what I was about to ask before I read that part of your comment. However, seeing as this isn't already implemented, is there more to this solution? (E.g. structure of the codebase making it difficult). Because I'd be willing to help |
I haven't dug into this bit of code for a bit, but it may be as simple as modifying this part of the code to check if the client supports modifying the textEdit field and updating the field instead of creating a command - https://github.com/dotnet/roslyn/blob/main/src/LanguageServer/Protocol/Handler/Completion/CompletionResultFactory.cs#L611 Note that of course whatever client is being used has to allow textEdit resolution on resolve and advertise the capability. If that's mainly all we need I think we'd be willing to accept that contribution. |
Ah, I see. This can in theory be replicated in neovim as this is already done in roslyn.nvim for code actions https://github.com/seblyng/roslyn.nvim/blob/main/lua/roslyn/lsp_commands.lua I've translated the vscode implementation you've linked in the plugin as a custom command here and verified that the command was indeed registered, but for some reason it never gets called. Well that is outside the scope of this issue, but my best guess is that it's somehow related to the exception which still happens.
Do you have any hints or ideas as to why that exception might be thrown? And as a sidenote, is there any list or friendlier ways to find out what functionalities the server expects to be done client side? Is such a search a reliable way to find out, or are there more? https://github.com/search?q=repo%3Adotnet%2Froslyn+roslyn.client&type=code |
Looks like the only test using this got disabled, and we don't really use server side workspace commands in VSCode today. So a bug slipped in. Tentative fix here - #77152. Do note that you'll still hit an exception if you try and execute the complex edit command on the server (just a different one). That command still needs to be executed client side (or pursue the path discussed above if the client supports text edit modification on resolve).
I think what you have there is the best way. We don't really have a centralized set of client side only commands, but they should be mostly prefixed with 'roslyn.client'. |
Discovered in #76141 The other test for this was disabled (feature not currently used in vscode), so this regression slipped in.
Version Used:
4.13.0-2.24558.12
Steps to Reproduce:
Expected Behavior:
The completion should apply with the appropriate selected method.
Actual Behavior:
The suggested methods are correct, but the completion is not. Instead of the selected method, a single character appears, or paranthesis, or none at all. This happens in:
I expect that it was fixed client-side in vscode for normal C# as it somehow seems to work in that scenario.
Attached one more example:
2024-11-29_00-05-55.mp4
From what I see relevant in the logs, the sent workspace/executeCommand is correct, but the server responds with a few exceptions:
The text was updated successfully, but these errors were encountered: