Is the position in textDocument/completion guaranteed to be valid?

[davidanthoff]

We are seeing crash reports for our Julia extension where we get textDocument/completion requests for positions that don't exist in the document. For example, the document is empty (i.e. an empty string), and then we get a request with a position of line 4 character 11.

We did have some bugs in terms of text sync. They previously showed up when we compared our own copy of the document content with the value we received in didSave. But we haven't received a single a single crash report with this kind of discrepancy in didSave so we kind of assume that we fixed the text sync... (we have automatic crash reporting and many thousand users, so we should have pretty good coverage).

The question for us now is: is this another bug in our text sync implementation, or should we expect to get textDocument/completion requests for positions that don't exist? Or is this maybe a bug in what positions are sent from the VS Code client?

Looking at the node implementation here it looks like it would just tolerate such erroneous position requests, i.e. even if a request with a position that doesn't exist came in, it would not crash. So maybe the VS Code client is sending these positions that are not really valid, but it hasn't shown up as bugs because the "default" implementation can ignore this problem?

[dbaeumer]

They only way I can see this happening is if some extension tries to programmatically query for completions using VS Codes command API. The LSP client in VS Code doesn't have any protection code against it. If someone would need to protect against something like this it would be VS Code itself. Have you tried the following:

• create a simple VS Code extension that hooks code complete
• trigger code complete programmatically using API commands (see https://code.visualstudio.com/api/references/commands) and see what your extension receives.

[vscodebot]

This issue has been closed automatically because it needs more information and has not had recent activity. See also our issue reporting guidelines.

Happy Coding!

[davidanthoff]

@dbaeumer I finally found time to follow up on this. What I tried is this: I added a new command to our julia extension with the following code:

context.subscriptions.push(vscode.commands.registerCommand('language-julia.foo', async ()=>{        
    let uri = vscode.window.activeTextEditor.document.uri;
    let success = await vscode.commands.executeCommand('vscode.executeCompletionItemProvider', uri, new vscode.Position(10000, 10000));
    console.log(success);
}));

If I execute this on a small Julia file (i.e. one where this position doesn't exist), then the completion request gets passed through all the way to our language server and then the language server crashes because it received a request with a position that doesn't exist.

So I think your theory might well be spot on: one explanation why we see these crashes in our telemetry might be that some users have some third party extension installed that might be issuing such requests for positions that don't exist.

So the question is what to do about this :) I would love it if VS Code added some checks somewhere so that such requests never make it through to the language server and instead the extension that is causing these incorrect requests receives an error.

The other option would be that we in our language server don't crash in this situation but handle it more gracefully. I'm a bit hesitant to do that, though. Mainly because crashing on invalid input has helped us a lot to identify and fix legit bugs in our code, and I'm still afraid that there might also be some problem in our own code, that we might in the end just mask with a more graceful input validation story...

[dbaeumer]

VS Code and LSP can not guard against this. Main reason is that the protocol allows to send requests for file that are not open. Guarding would mean to open the file first, check if the position is valid and then do something.

It is absolutely fine if your server errors on that request. The error will then be propagated all the way back to the extension executing the command. If not that would be a bug in LSP client libs or VS Code.

[davidanthoff]

Main reason is that the protocol allows to send requests for file that are not open. Guarding would mean to open the file first, check if the position is valid and then do something.

Ah, interesting, I hadn't been aware of that!

Do you think we could add a check on the VS Code side in situations when the doc is open and then filter out these request in those situations? I understand that wouldn't be a guarantee that we don't receive such messages in general, but I kind of feel it would still be a useful "catch problems early" type check? And it should be cheap to do, I would think?

The main reason for this is that a) we are currently only receiving these invalid requests for open files, so it should in theory help with this problem and b) I'm still worried that we have some other mistake in our implementation of the protocoll (say the text sync part) and that these messages actually are valid. If we now just return an error message, then we can't really get to the bottom of that... On the other hand, if there was a filter on the client side, and after that we wouldn't see these error messages in our telemetry, then I could be pretty sure that this was not a problem in our code.

Or is there maybe some way that I could add some code in our extension that hooks into that, which would allow me to check whether we get requests for this with incorrect positions?

It is absolutely fine if your server errors on that request.

Would we send an error code InvalidParams in that case? Or something else?

[dbaeumer]

InvalidParams is the right error. I am fine if you want to provide a PR that checks for this in the LSP node modules with a flag for extensions to enable.

[davidanthoff]

Awesome. I'm not familiar with the LSP node codebase, could you maybe just point me to roughly where in the codebase one might add this? Just a single link I think would help me a lot to get started, and then I can figure out whether I can figure this out or not and open a PR :)

[dbaeumer]

https://github.com/microsoft/vscode-languageserver-node/blob/master/client/src/client.ts#L1512

[dbaeumer]

But this is only for completion. It needs to be done for all requests that take [text document, position]