jupyter-lsp / jupyterlab-lsp

Coding assistance for JupyterLab (code navigation + hover suggestions + linters + autocompletion + rename) using Language Server Protocol
https://jupyterlab-lsp.readthedocs.io
BSD 3-Clause "New" or "Revised" License
1.78k stars 146 forks source link

Force virtual document to use additional file extension? #872

Open dleen opened 1 year ago

dleen commented 1 year ago

Description

I follow the instructions for the Scala Language Server (Metals) integration: https://jupyterlab-lsp.readthedocs.io/en/latest/Configuring.html#example-scala-language-server-metals-integration.

There weren't any errors, but the LSP wasn't returning any suggestions aside from the kernel ones. When testing using a Python kernel there were LSP suggestions.

Reproduce

Follow the instructions here: https://jupyterlab-lsp.readthedocs.io/en/latest/Configuring.html#example-scala-language-server-metals-integration.

I enabled debug traces for metals by creating the file:

2022.11.08 13:04:00 INFO  tracing is enabled: /Users/dleen/code/jupyterlab-lsp/.metals/lsp.trace.json
2022.11.08 13:04:02 INFO  logging to file /Users/dleen/code/jupyterlab-lsp/.virtual_documents/.metals/metals.log

Looking at the lsp.trace.json file while interacting with JupyterLab showed output like this:

[Trace - 01:08:07 PM] Received request 'textDocument/documentHighlight - (1)'
Params: {
  "textDocument": {
    "uri": "file:///Users/dleen/code/jupyterlab-lsp/.virtual_documents/Untitled.ipynb"
  },
  "position": {
    "line": 0,
    "character": 15
  }
}

[Trace - 01:08:07 PM] Sending response 'textDocument/documentHighlight - (1)'. Processing request took 9ms
Result: []

Notice that the result is empty [] and also notice that the uri is: file:///Users/dleen/code/jupyterlab-lsp/.virtual_documents/Untitled.ipynb which has file extension .ipynb.

After reading this doc: https://jupyterlab-lsp.readthedocs.io/en/latest/Configuring.html#making-custom-servers-work-with-notebooks

many language servers only handle files with specific file extensions and refuse to operate if not provided with such; the file extension of a native script for a given language (this is other than .ipynb), derived from file_extension field of language_info, will be added to the name of the notebook when communicating with the language server to satisfy the file extension check.

I guessed that the problem might be with the missing extension.

I commented out these lines: https://github.com/jupyter-lsp/jupyterlab-lsp/blob/master/packages/jupyterlab-lsp/src/virtual/document.ts#L755-L757

And the uri became: "uri": "file:///Users/dleen/code/jupyterlab-lsp/.virtual_documents/Untitled.ipynb.scala.sc", and the LSP started working correctly.

I'm not sure what the parent is controlling here or how to force the addition of the file extensions otherwise.

    if (!this.parent) {
      return encodedPath;
    }

Any pointers would be great! Thanks!

krassowski commented 1 year ago

I am a bit confused as there should be no this.parent for a notebook, unless you embed your Scala code in Python notebook via magics. Would you mind checking what this.parent refers too?

dleen commented 1 year ago

You're right - there is no 'this.parent' but that is causing the code to go into the branch which doesn't add the file extension. I can only get the Metals scala support to work if the file extension is added.

I'm not using any magics or anything like that just the Almond Scala kernel.

In this case the parent is null or undefined

dleen commented 1 year ago

@krassowski just to make sure we're on the same page: there is no this.parent for a notebook and because of that, the function referenced above exits early and never hits the line:

    return encodedPath + '.' + this.id_path + '.' + this.file_extension;

which means that the metals server is running against a file like foo.ipynb, and it doesn't seem to detect any Scala. When I force the filename to have the extension foo.ipynb.scala.sc by bypassing the parent check everything works as expected.

I'm a bit stuck at this point if you have any more tips...

joelschutz commented 1 year ago

I'm having the same issue in the server side, i'm creating a spec for a Go kernel and I needing a lot more them an extension append on the file. Maybe this should be dealt by the LanguageServerSession on the backend?

As I see, the specs used to inform the jupyter's environment how to initialize and use those servers. For the frontend the only concerne is the language itself. Since the notebook is a closed space, the server should care about the real files.

In Go, for example, I need to do a lot of manipulation in the files to make gopls recognize it as a module, including adding and removing lines, create directories, etc. I think that creating a way to extend the handler of the ShadowFileSystem and interface how the scripts should be presented to the language server as files.

I was working in a solution, but I don't think I found it yet. It needs some feedback.

dleen commented 1 year ago

I submitted a PR with my attempted solution, see #873. I'm still not happy with it but I had to make a fork and merge my change anyway as users wanted Scala support.

The reason it wasn't a backend change in this case was this line: https://github.com/jupyter-lsp/jupyterlab-lsp/blob/master/packages/jupyterlab-lsp/src/virtual/document.ts#L758 where the file extension is being manipulated already.

Even for the Scala there's more than just the extension being appended. If you look at the virtual documents it creates you'll see that the notebook content is being converted to a regular looking code file e.g. no json structure, just plain text. So there's definitely an opportunity for you to do more transformations - although maybe you've already found this in LanguageServerSession?

joelschutz commented 1 year ago

@dleen After wrestling with this think for a feel days now I think that the best way to go is to use the new LSP 3.17 notebook support. It's very new and not many(if any) language servers had implemented it yet.

Although is a more complicated approach, it's a more definitive and compatible solution, since is totally IDE independent. Once both the server and the client implements the protocol, all the current and future features of the server would be available for either type of file, since the new methods only concerne the file syncronization.

On top of that, I don't really like the idea of manipulate on disk files at didChange events. If the manipulation and analysis could all be done in memory it'd be much faster and without dunny files polluting the workspace.