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

Jump to definition handling spaces in URI inconsistently #403

Closed
avaissi opened this issue Nov 9, 2020 · 4 comments · Fixed by #406
Closed

Jump to definition handling spaces in URI inconsistently #403

avaissi opened this issue Nov 9, 2020 · 4 comments · Fixed by #406

Comments

@avaissi
Copy link

avaissi commented Nov 9, 2020

Description

Jump to definition feature doesn't seem to handle spaces in base/parent path correctly. Jump to-feature behaves as if trying to perform a global jump using the .lsp_symlink, even though the target is in same directory than jump source. Getting similar results both in Windows 10 and Ubuntu 18.04:

image

Reproduce

Not entirely sure if prerequisite in reproducing the issue, but I have jupytext installed.

  1. Start jupyterlab server in directory, which has spaces in the name (might be reproducible with just space in anywhere in path)
  2. Create a target file (extension.py in my example) with function definition
  3. Create and open another python file in notebook mode and import function
  4. Jump to function definition

While debugging the issue I pinpointed the culprit to:
https://github.com/krassowski/jupyterlab-lsp/blob/fbc9d1c2f67842352acbbcb276347c6e65d51d29/packages/jupyterlab-lsp/src/utils.ts#L134
where child and parent:
child = "file:///c:/Users/avaissi/Documents/My own robot/.virtual_documents/tasks/extension.py"
parent = "file:///c:/Users/avaissi/Documents/My%20own%20robot"
thus failing the child.startsWith(parent) comparison.

When tried to wrap the parent || server_root_uri() with decodeURI() -> parent = decodeURI(parent || server_root_uri()); things worked nicely.

Expected behavior

I'm expecting the jump to complete to a file/location within same directory.
And furthermore, I'm expecting the URI to compare spaces (and possibly other special characters) either encoded or decoded.

Context

  • Windows 10, Ubuntu 18.04
  • Chrome: Version 86.0.4240.183 (Official Build) (64-bit)
  • JupyterLab version: 2.2.6
  • jupyterlab-lsp: 2.0.8
Required: installed server extensions
jupyter_lsp 0.9.2 ok
jupyterlab 2.2.6 ok
jupytext 1.5.0 ok
Required: installed lab extensions
@jupyter-widgets/jupyterlab-manager v2.0.0 enabled  ok
@krassowski/jupyterlab-lsp v2.0.8 enabled  ok
@robocorp/theme-robocodelab-extension v2.3.2 enabled  ok
jupyterlab-jupytext v1.2.1 enabled  ok
Browser Output (recommended for all interface issues)
Will jump to the first of suggested locations: Array(1)0: {uri: "file:///c:/Users/avaissi/Documents/My%20own%20robot/.virtual_documents/tasks/extension.py", range: {…}}length: 1__proto__: Array(0)
jump_to.js:77 Jumping to external file: file:///c:/Users/avaissi/Documents/My own robot/.virtual_documents/tasks/extension.py
jump_to.js:78 Jump target (source location): Object
api/contents/c%3A/Users/avaissi/Documents/My%20own%20robot/.virtual_documents/tasks/extension.py?content=0&1604934607968:1 Failed to load resource: the server responded with a status of 404 (Not Found)
jump_to.js:103 Error: c:/Users/avaissi/Documents/My own robot/.virtual_documents/tasks/extension.py is outside root contents directory
    at Function.create (serverconnection.js:106)
    at async Drive.get (index.js:496)
    at async jump_to_CMJumpToDefinition.handle_jump (jump_to.js:96)
    at async Object.execute (jump_to.js:137)
handle_jump @ jump_to.js:103
notebook.js:75 LSP: waiting for .lsp_symlink//c:Users/avaissi/Documents/My own robot/.virtual_documents/tasks/extension.py to fully load
api/contents/.lsp_symlink/c%3AUsers/avaissi/Documents/My%20own%20robot/.virtual_documents/tasks/extension.py?type=notebook&content=1&1604934607990:1 Failed to load resource: the server responded with a status of 404 (Not Found)
@bollwyvl
Copy link
Collaborator

bollwyvl commented Nov 9, 2020 via email

@bollwyvl
Copy link
Collaborator

Welp, over on #406, I'm not getting any particular insights back from running all of the acceptance tests from within %HOME%\nöte bòóks on all the platforms.

I'm investigating:

  • more path buggaboos, e.g. My%20 %20Folder
  • adding more cross-file jump tests in non-ascii folders in the notebook dir

If these come up without reproducible errors, I definitely will add the decodeUri, and see if that doesn't do weird double encoding... still would appreciate a (sanitized) dump of a pageConfig from the view:source:localhost:888/lab that exhibits the condition: if, as in the picture, your path is coming back c:Users, (as opposed to expected c:/Users, it's pretty much endgame. You mention this was reproducible on linux: if you could gin up a binder that exhibits the condition, that would be very useful indeed!

If the problems persist, there might indeed be deeper problems in jupytext vs jupyterlab-lsp: some race condition in the application of its monkeypatch vs when we read the information to pass to pageConfig... I do note that said monkeypatch does some odd stuff around windows paths... I haven't even looked at their typescript... and I'm not about to get into a patch-off.

What I don't foresee happening is us adding jupytext (or other import/file manager extensions a la importnb) to our test environment. This would open the doors for having to potentially support all kinds of things that don't mutually test against each other that might be actively trying to break API/protocol assumptions etc. which the language servers, and/or our current understanding of them, will have no idea what to do with. We're hustling to keep something that pretty much works at least 80% of the time with a vanilla install, and even that is a struggle 😫

@bollwyvl
Copy link
Collaborator

The lab terminal log will also show the root uri if you start with:

jupyter lab --debug

For example, from the latest CI test:


[D 06:10:19.415 LabApp] [lsp] rootUri will be file:///d:/a/jupyterlab-lsp/jupyterlab-lsp/atest/output/windows_38_1/home/n%C3%B6te%20b%C3%B2%C3%B3ks
[D 06:10:19.415 LabApp] [lsp] virtualDocumentsUri will be file:///d:/a/jupyterlab-lsp/jupyterlab-lsp/atest/output/windows_38_1/home/n%C3%B6te%20b%C3%B2%C3%B3ks/.virtual_documents

@avaissi
Copy link
Author

avaissi commented Nov 16, 2020

Sorry, missed the question about the pageConfig completely..

Didn't actually even notice the missing c:/ from the error picture, and following that trail it seems that global_jump() should handle it properly and one slash gets only dropped later from the path:
image

Collecting some more information here:

with jupyter lab --debug

[D 10:27:52.810 LabApp] [lsp] rootUri will be file:///c:/Users/avaissi/Documents/My%20own%20robot 
[D 10:27:52.810 LabApp] [lsp] virtualDocumentsUri will be file:///c:/Users/avaissi/Documents/My%20own%20robot/.virtual_documents

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 a pull request may close this issue.

2 participants