Skip to content
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

Tolerate "processId":null that may be send by Emacs LSP client #1

Closed
wants to merge 1 commit into from

Conversation

michaliskambi
Copy link

Emacs LSP client sends the "initialize" JSON request containing null in place of the processId:

{....,"params":{"processId":null,"rootPath":"....","rootUri":"...."}...}

(The full sample JSON send by Emacs client is in the attached test application.)

This causes JSON reading in Initialize to fail to read the following information, in particular it fails to read the RootUri. More precisely, the loop

while (Reader.Advance <> jsDictEnd) and Reader.Key(Key) do

ends prematurely, before we've actually seeen all parameters. Briefly looking at TJsonReader implementation, I understand that the expectation is that one should call Reader.Null in case the value may be null in JSON, to allow TJsonReader.Null to do StackPop; Reduce;.

I'm attaching test_json.lpr, a simple JSON reader test (using only JSONStream unit, nothing else). I used this to confirm the issue and test the solution. It contains hardcoded JSON requests I logged from Emacs and VS Code (for comparison) LSP clients. If you comment out the

if Key = 'processId' then
  Reader.Null;

inside and run it, you will get:

$ fpc test_json.lpr
Free Pascal Compiler version 3.2.2-rrelease_3_2_2-0-g0d122c4953 [2022/06/17] for x86_64
...
$ ./test_json
Test on VS Code JSON:
Has JSON key: jsonrpc
Has JSON key: id
Has JSON key: method
Has JSON key: params
Has JSON key in params: processId
Has JSON key in params: clientInfo
Has JSON key in params: rootPath
Has JSON key in params: rootUri
Has JSON key in params: capabilities
Has JSON key in params: initializationOptions
Has JSON key in params: trace
Has JSON key in params: workspaceFolders
Test on Emacs JSON:
Has JSON key: jsonrpc
Has JSON key: method
Has JSON key: params
Has JSON key in params: processId
Has JSON key: rootPath
Has JSON key: clientInfo
Has JSON key: rootUri
Has JSON key: capabilities
Has JSON key: initializationOptions

So in case of Emacs JSON, the only key found in params was processId.

Uncommenting the code doing Reader.Null, we get the desired result, and rootUri is correctly parsed for both VS Code client and Emacs client:

$ fpc test_json.lpr
Free Pascal Compiler version 3.2.2-rrelease_3_2_2-0-g0d122c4953 [2022/06/17] for x86_64
...
$ ./test_json
Test on VS Code JSON:
Has JSON key: jsonrpc
Has JSON key: id
Has JSON key: method
Has JSON key: params
Has JSON key in params: processId
Has JSON key in params: clientInfo
Has JSON key in params: rootPath
Has JSON key in params: rootUri
Has JSON key in params: capabilities
Has JSON key in params: initializationOptions
Has JSON key in params: trace
Has JSON key in params: workspaceFolders
Test on Emacs JSON:
Has JSON key: jsonrpc
Has JSON key: method
Has JSON key: params
Has JSON key in params: processId
Has JSON key in params: rootPath
Has JSON key in params: clientInfo
Has JSON key in params: rootUri
Has JSON key in params: capabilities
Has JSON key in params: initializationOptions
Has JSON key: workDoneToken

test_json.lpr.txt

The lack of RootUri seems to have dire consequences for pasls: It reports then that it cannot find units from LCL, like Laz_AVL_Tree from LazUtils package, in my test on michaliskambi/elisp#1 . Despite the log showing it can find the lazutils.lpk correctly. In effect, code completion in Emacs with this LSP server was failing for me when some LCL unit like Laz_AVL_Tree was used. I didn't analyze further (how lack of RootUri is causing this, despite lazutils.lpk being found), but making RootUri correctly detected fixed everything :)

Side investigation: Why does Emacs LSP client send "processId":null?

michaliskambi added a commit to michaliskambi/elisp that referenced this pull request Nov 10, 2022
Isopod added a commit that referenced this pull request Nov 10, 2022
Fixes a bug with JSON parsing

See also:
  #1
@Isopod
Copy link
Owner

Isopod commented Nov 10, 2022

Thanks for your investigation and the patch!

You have actually discovered a bug in my json library: Values that are not consumed are supposed to be skipped. So in this case, since we are not explicitly handling "processId", the value should just be ignored and parsing should continue with the next item. But this did not work correctly for null values.

It should be fixed now in master, so I'm closing this. Please reopen if you still get the error.

@Isopod Isopod closed this Nov 10, 2022
@michaliskambi
Copy link
Author

Cool, I see Isopod/jsonstream@c9601be and confirm that it also fixes the issue (both in my test program test_json.lpr and in LSP server when used from Emacs, without my PR addition).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants