wrong encoding URI on Windows (breaks CudaText-lsp and VSCode-nim-lsp)

[veksha]

Hi. I tested nimlsp with VSCode-nim-lsp and CudaText-lsp on Windows 10. Paths with percent-encode were not working for me. Goto definition/references were totally broken. so I made this little patch for this proc:

https://github.com/PMunch/nimlsp/blob/d15a2b847ebe3a58babd024bb9c09f101eb9adb1/src/nimlsp.nim#L101-L112

@@ -102,6 +102,8 @@
   # This is a modified copy of encodeUrl in the uri module. This doesn't encode
   # the / character, meaning a full file path can be passed in without breaking
   # it.
+  if defined(windows):
+    return "/" & path.replace('\\','/')
   result = newStringOfCap(path.len + path.len shr 2) # assume 12% non-alnum-chars
   for c in path:
     case c

tested with filenames that contain spaces - working fine. so I guess it is a lot better then nothing. it plays nice with python's url2pathname(urlparse(uri).path) (this is how CudaText is parsing) vscode-nim-lsp extension is finally going to definitions and peeking references correctly. ("goto references" still broken?)

please consider this patch.

[PMunch]

Does this only happen for CudaText, or for VSCode as well? I haven't heard anyone else having this issue, so I'm not sure applying the patch to all Windows users would be a good idea.

[veksha]

I guess all VSCode users use this extension https://github.com/pragmagic/vscode-nim (which uses nimsuggest.exe and not nimlsp.exe)

This VSCode extension is working with nimlsp.exe: https://github.com/bung87/vscode-nim-lsp and this: https://github.com/junknet/vscode-nimlsp I tested them both.

correctly works only hover (because it is returned as simple text). Definition and references not working because URLs must be parsed by VSCode, and, for example, this URL can't be parsed correctly:

file://D%3A%5CProgramm%5CNim%5CNim-devel%5Clib%5Csystem.nim

2 problems:

• file:// is not enough, must be file:/// "for a local file, the hostname is omitted, but the slash is not" (c) https://en.wikipedia.org/wiki/File_URI_scheme
• backslashes must be converted to forward slashes. https://www.rfc-editor.org/rfc/rfc8089.html#appendix-E.4

the URL will look like this: file:///D%3A/Programm/Nim/Nim-devel/lib/system.nim and it will then be decoded by VSCode to file:///D:/Programm/Nim/Nim-devel/lib/system.nim this URL will work with VSCode just fine.

The same goes for CudaText's lsp-addon. it expects correct URL. otherwise it won't parse it and will fail.

With my patch all works correctly in CudaText. In VSCode it can work fine, but sometimes lsp extension crashes:

Details

```nim C:\Users\yura\.nimble\pkgs\nimlsp-0.3.2\src\nimlsp.nim(419) nimlsp C:\Users\yura\.nimble\pkgs\nimlsp-0.3.2\src\nimlsppkg\suggestlib.nim(110) outline D:\Programm\Nim\Nim-devel\nimsuggest\nimsuggest.nim(760) runCmd D:\Programm\Nim\Nim-devel\nimsuggest\nimsuggest.nim(194) executeNoHooks D:\Programm\Nim\Nim-devel\compiler\modules.nim(175) compileProject D:\Programm\Nim\Nim-devel\compiler\modules.nim(98) compileModule D:\Programm\Nim\Nim-devel\compiler\passes.nim(180) processModule D:\Programm\Nim\Nim-devel\compiler\passes.nim(73) processTopLevelStmt D:\Programm\Nim\Nim-devel\compiler\sem.nim(649) myProcess D:\Programm\Nim\Nim-devel\compiler\sem.nim(612) semStmtAndGenerateGenerics D:\Programm\Nim\Nim-devel\compiler\semstmts.nim(2335) semStmt D:\Programm\Nim\Nim-devel\compiler\semexprs.nim(1055) semExprNoType D:\Programm\Nim\Nim-devel\compiler\semexprs.nim(2970) semExpr D:\Programm\Nim\Nim-devel\compiler\semstmts.nim(2277) semStmtList D:\Programm\Nim\Nim-devel\compiler\semexprs.nim(2975) semExpr D:\Programm\Nim\Nim-devel\compiler\semstmts.nim(1449) semTypeSection D:\Programm\Nim\Nim-devel\compiler\semstmts.nim(1228) typeSectionRightSidePass D:\Programm\Nim\Nim-devel\compiler\semtypes.nim(2081) processMagicType D:\Programm\Nim\Nim-devel\lib\system\assertions.nim(39) failedAssertImpl D:\Programm\Nim\Nim-devel\lib\system\assertions.nim(29) raiseAssert D:\Programm\Nim\Nim-devel\lib\system\fatal.nim(53) sysFatal Error: unhandled exception: D:\Programm\Nim\Nim-devel\compiler\semtypes.nim(2081, 12) `c.graph.sysTypes[tySequence] == nil` [AssertionDefect] ```

I don't know why VSCode extension crashes and CudaText lsp-addon is not. But at least now they can go to definitions/references just fine because paths are correctly formatted.

I work in CudaText, so for me no crashes with patched nimlsp. the only thing that bothers is slow nimlsp loading time for every file I switch to.

[PMunch]

Hmm, curious. I know some people in the past have worked on the URI encoding system in NimLSP, and I believe this did work properly at some point. Maybe it got messed up through changes intended for Unix users. If you could fix it and create a PR that would be great. We really should create a test harness for NimLSP (which shouldn't be hard, it's just a matter of starting it and sending data over stdin and verify responses over stdout). This would allow us to set up some tests that could avoid issues like this in the future.

[veksha]

We really should create a test harness for NimLSP (which shouldn't be hard, it's just a matter of starting it and sending data over stdin and verify responses over stdout). This would allow us to set up some tests that could avoid issues like this in the future.

OK. I prepared new test "Nim LSP can request hover message". Edited tests/tnimlsp.nim file.

Details

```nim import unittest import os, osproc, streams, options, json, strutils import .. / src / nimlsppkg / baseprotocol include .. / src / nimlsppkg / messages suite "Nim LSP basic operation": setup: let nimlsp = parentDir(parentDir(currentSourcePath())) / "nimlsp" p = startProcess(nimlsp, options = {}) i = p.inputStream() o = p.outputStream() teardown: p.terminate() test "Nim LSP can be initialised": var ir = create(RequestMessage, "2.0", 0, "initialize", some( create(InitializeParams, processId = getCurrentProcessId(), rootPath = none(string), rootUri = "file:///tmp/", initializationOptions = none(JsonNode), capabilities = create(ClientCapabilities, workspace = none(WorkspaceClientCapabilities), textDocument = none(TextDocumentClientCapabilities), experimental = none(JsonNode) ), trace = none(string), workspaceFolders = none(seq[WorkspaceFolder]) ).JsonNode) ).JsonNode i.sendJson ir var frame = o.readFrame var message = frame.parseJson if message.isValid(ResponseMessage): var data = ResponseMessage(message) check data["id"].getInt == 0 echo data["result"] else: check false echo message test "Nim LSP can request hover message": let file = getCurrentDir()/"examples"/"test1_example.nim" var fileURI: string if defined(windows): #fileURI = "file://" & file # incorrect fileURI = "file:///" & file.replace("\\","/") # correct else: fileURI = "file://" & file echo "URL: " & fileURI var ir = create(RequestMessage, "2.0", 0, "initialize",none(JsonNode)).JsonNode i.sendJson ir ir = create(NotificationMessage, "2.0", "textDocument/didOpen", some( create(DidOpenTextDocumentParams, create(TextDocumentItem, uri = fileURI, languageId = "nim", version = 1, text = readfile(file) ) ).JsonNode) ).JsonNode i.sendJson ir ir = create(RequestMessage, "2.0", 1, "textDocument/hover", some( create(TextDocumentPositionParams, create(TextDocumentIdentifier, uri = fileURI ), create(Position, line = 0, character = 8 ) ).JsonNode) ).JsonNode i.sendJson ir var frame = o.readFrame var message = frame.parseJson if message.isValid(ResponseMessage): var data = ResponseMessage(message) check data["id"].getInt == 0 echo data["result"] else: check false frame = o.readFrame message = frame.parseJson if message.isValid(ResponseMessage): var data = ResponseMessage(message) check data["id"].getInt == 1 let contents = data["result"].get()["contents"] check contents[0]["value"].getStr == "os" check "basic operating system facilities" in contents[1]["value"].getStr else: check false echo message ```

it needs tests/examples/test1_example.nim file for the test. with this contents (single line):

import os

I can prepare a PR but there is a problem. If test fails it will just hang forever waiting for response message. so we need to prepare something to make this test possible. I can suggest adding new command line option to nimlsp like "-q" (quit on exception) or something like that and return some error code. (nimlsp not quitting on URI parse exception). This functionality will allow us to know if something went wrong immediately. any suggestions?

[AmjadHD]

FWIW, this doesn't work in sublime also (On windows), this is the result of definition request of parseEnum:

:: --> nimlsp textDocument/definition(59): {'textDocument': {'uri': 'file:///C:/Users/User/Exercism/nim/allergies/allergies.nim'}, 'position': {'character': 7, 'line': 21}, 'workDoneToken': 'wd59'}
:: <<< nimlsp 59: [{'uri': 'file://C%3A%5CUsers%5CUser%5C.choosenim%5Ctoolchains%5Cnim-1.4.8%5Clib%5Cpure%5Cstrutils.nim', 'range': {'end': {'character': 14, 'line': 1321}, 'start': {'character': 5, 'line': 1321}}}]

[AmjadHD]

This is how pathname2url and url2pathname are implemented in python: https://github.com/python/cpython/blob/main/Lib/urllib/request.py#L1678 on windows: https://github.com/python/cpython/blob/main/Lib/nturl2path.py#L45 on unix: https://github.com/python/cpython/blob/main/Lib/urllib/parse.py#L818

[AmjadHD]

I can confirm that the following diff works on Sublime and Windows:

@@ -103,10 +103,18 @@ proc pathToUri(path: string): string =
   # the / character, meaning a full file path can be passed in without breaking
   # it.
   result = newStringOfCap(path.len + path.len shr 2) # assume 12% non-alnum-chars
+  when defined(windows):
+    result.add '/'
   for c in path:
     case c
     # https://tools.ietf.org/html/rfc3986#section-2.3
     of 'a'..'z', 'A'..'Z', '0'..'9', '-', '.', '_', '~', '/': add(result, c)
+    of '\\':
+      when defined(windows):
+        result.add '/'
+      else:
+        result.add '%'
+        result.add toHex(ord(c), 2)
     else:
       add(result, '%')
       add(result, toHex(ord(c), 2))

[keiviv]

How is flying, @nosly? Maybe PR?

Would be really cool if it's finally fixed. Without the fix nothing URI-related works — references, renaming, etc.

[nosly]

How is flying, @nosly? Maybe PR?

Would be really cool if it's finally fixed. Without the fix nothing URI-related works — references, renaming, etc.

Unfortunately proposed patches didn't fix the problem for sublime4 on win10 21H1, tried both patches on 0.3.2 and 0.4.0, maybe win version plays a part in this

[veksha]

Unfortunately proposed patches didn't fix the problem for sublime on windows

tested my patch with sublime4 (windows11 x64). works fine. (i'm using v.0.3.2 branch of nimlsp, because latest nimlsp is not working for Windows at all!)

[keiviv]

Unfortunately proposed patches didn't fix the problem for sublime4 on win10

Same results — nothing works for me (Win 10 / Sublime 4). I don't understand how it works for 3 people and doesn't for 2 — this is not something that should do that.

[veksha]

Unfortunately proposed patches didn't fix the problem for sublime4 on win10

Same results — nothing works for me (Win 10 / Sublime 4). I don't understand how it works for 3 people and doesn't for 2 — this is not something that should do that.

perhaps something wrong with configuration. if you will outline your steps (from getting source from correct branch and patching it , to compilation flags, etc, lsp client configuration... setting system Path...) then maybe we can help you.

[keiviv]

Default configs, no extra LSP settings, correct sources. Besides patching, I've tried a direct clone of vanyle fork — just to be 100% sure. Even checked your repo to see if you have something special that makes it work.

To make extra sure there's no mistake on my end — I've also got nimble and changed its packages_official.json to build directly from the fork — so that there literally be no way to mess up the build with wrong flags, paths, etc.

And like @nosly said — it does not fix it: the URIs are simply not decoded to proper paths.

[veksha]

@keiviv i've compiled vanyle fork just now. I use nimble build -d:danger -d:debugLogging command line inside nimlsp sources folder to build an .exe (-d:danger will make it small and super fast)

how can you tell URIs are not decoded properly? show log file, please. I see urls like this: {"uri": "file:///D:/Programm/Nim/godot-nim-stub/src/utils.nim"}. this is correct and it's working.

[keiviv]

The machines at my work run a custom Win 10 LTSC fiddled with NTLite, etc. I just tried it on vanilla Win 10 and Win 11 boxes — and the patch worked.

So, false alarm on my part. Not sure why in @nosly case it's not working.

[PMunch]

Since it seems the patch is good I've merged it. And I'm closing this as well as this should be fixed now. Please comment if it isn't

[nosly]

Default configs, no extra LSP settings, correct sources. Besides patching, I've tried a direct clone of vanyle fork — just to be 100% sure. Even checked your repo to see if you have something special that makes it work.

To make extra sure there's no mistake on my end — I've also got nimble and changed its packages_official.json to build directly from the fork — so that there literally be no way to mess up the build with wrong flags, paths, etc.

And like @nosly said — it does not fix it: the URIs are simply not decoded to proper paths.

Have done things like this, but the problem remains. My win10 is 21H1 19043.1348. Besides sublime 4, also tried cudatext. All relevant settings are at their defaults.

When clicked "Definition" button in cudatext: NOTE: LSP: Nim - file does not exist: '', uri:'file://E%3A%5Cwork%5Cdev%5Cnim%5Chello.nim' When clicked "Reference" in sublime: \E%3A%5Cwork%5Cdev%5Cnim%5Chello.nim\:

Have tried both with & without the patches, results are the same. It still seems to be the initially identified issue of file:// should be file:/// on windows

[veksha]

Have tried both with & without the patches, results are the same. It still seems to be the initially identified issue of file:// should be file:/// on windows

result can't be the same. double-check where your nimlsp.exe is located. you can use software like Everything to search for it. perhaps Cuda or ST is using the old version of nimlsp.exe.

1. stop process in task manager
2. delete old files from disk.
3. recompile
4. ....

try to modify patch so that proc returns helloworld instead of file://c:/file.... to see if the new version is used.

[nosly]

Have tried both with & without the patches, results are the same. It still seems to be the initially identified issue of file:// should be file:/// on windows

result can't be the same. double-check where your nimlsp.exe is located. you can use software like Everything to search for it. perhaps Cuda or ST is using the old version of nimlsp.exe.

1. stop process in task manager

2. delete old files from disk.

3. recompile

4. ....

try to modify patch so that proc returns helloworld instead of file://c:/file.... to see if the new version is used. I use:

1. nimble uninstall nimlsp to remove current copy
2. nimble install https://github.com/nosly/nimlsp or nimble install nimlsp for another version
3. There is only one version installed under e.g. c:\Users\qm\.nimble\pkgs\nimlsp-0.4.0\, I even renamed this folder then use sublime to access (of course not accessable) to ensure this is the only version in play
4. I changed "file://" to "_*_" and recompile but still getting same results Strange

[veksha]

1. nimble uninstall nimlsp to remove current copy
2. nimble install https://github.com/nosly/nimlsp

now i see your problem, @nosly

nimble install command by default fetches latest tagged commit (this is not the most recent!) (read more https://github.com/nim-lang/nimble#nimble-install )

latest changes can be fetched like this: nimble install https://github.com/vanyle/nimlsp.git@#head

(added @#head according to nimble manual)

this is vanyle's fork that is compatible with Windows. your fork is v.0.4.0 - it's not compatible with Windows yet.

[veksha]

@nosly you can compile nimlsp without forking it on github and without nimble. just use commands like git clone ..., then nim c -d:release nimlsp.nim inside folder with nimlsp.nim file. this way it's easier.

[PMunch]

You can also use git clone ... and then nimble install in the folder you just cloned.

[nosly]

@veksha Yes, that solves my silly problem, thanks a lot. Btw, the cudatext lsp will always trigger a cmd window, any way to disable that?

[veksha]

@veksha Yes, that solves my silly problem, thanks a lot.

glad to help.

Btw, the cudatext lsp will always trigger a cmd window, any way to disable that?

it's fixed recently. use addon manager plugin to update CudaText lsp.

[nosly]

Btw, the cudatext lsp will always trigger a cmd window, any way to disable that?

it's fixed recently. use addon manager plugin to update CudaText lsp.

Yes, indeed, thanks