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

Virtual documents without file paths #122

Closed
jryans opened this issue Aug 24, 2023 · 10 comments · Fixed by #124
Closed

Virtual documents without file paths #122

jryans opened this issue Aug 24, 2023 · 10 comments · Fixed by #124

Comments

@jryans
Copy link
Contributor

jryans commented Aug 24, 2023

Some editors (at least VS Code) support "virtual" documents which are not saved anywhere on the filesystem. These include things like unsaved documents, notebook cells, etc. These virtual documents have some non-file URI scheme.

The language server currently assumes it is always working a document from the filesystem, as various components try to load the file from disk, split the path for directory traversal, etc. It would be great if the language server could be updated to work entirely from the document contents provided by the client without assuming it is stored in a filesystem somewhere.

As a workaround, the editor client can write these virtual documents to a temporary file and provide that to language server instead, but it would be much better to remove the filesystem requirement from the server entirely.

@dannypsnl
Copy link
Contributor

Didn’t know how DrRacket handle this?

@6cdh
Copy link
Contributor

6cdh commented Aug 25, 2023

DrRacket doesn't need the file path. It doesn't need to know the path if no relative path require, it only needs current working directory if there is relative path require.

@6cdh
Copy link
Contributor

6cdh commented Sep 6, 2023

I saw the notebook document uri looks like this:
vscode-notebook-cell:/home/lcdh/dots/work/racket-langserver/test.ipynb#W0sZmlsZQ%3D%3D

A simple fix could be

  1. remove all uri-is-path? guards
  2. make uri->path support not only file uri

@jryans
Copy link
Contributor Author

jryans commented Sep 6, 2023

A simple fix could be

  1. remove all uri-is-path? guards
  2. make uri->path support not only file uri

Do you mean support any URI (even for unsaved documents that have no file path at all), or only the subset like notebook cells, where there is something like a file path that could be extracted...?

I started to experiment with entirely removing all uses of the file path (storing only an opaque URI) before filing this issue... But then I noticed check-syntax seems to actually split the file path into directories for make-traversal and current-load-relative-directory, and I wasn't sure what might go wrong without supplying those... Also lsp-editor% has a method to load the file from its path. These things taken together made me concerned that file paths are required at some deeper level, so I stop experimenting and filed this issue.

Perhaps I was scared away too easily though, and those things can just be removed without issues...?

@dannypsnl
Copy link
Contributor

dannypsnl commented Sep 6, 2023

Do you think introduce a virtual filesystem for vscode-notebook-cell will work? It still have a path, just different from default filesystem.

@jryans
Copy link
Contributor Author

jryans commented Sep 6, 2023

Do you think introduce a virtual filesystem for vscode-notebook-cell will work? It still have a path, just different from default filesystem.

What do you mean by "virtual filesystem" here...? Just extracting the embedded file path, or something more?

The check-syntax and lsp-editor% code paths give the impression that file contents may be loaded from disk (as opposed to just using the over-the-wire content already provided by the LSP client). If that's actually happening, then loading the embedded *.ipynb file path is not likely to make sense to the Racket reader code.

Extracting the file path seems to imply maintaining some bit of code (either in clients or the server) for each possible URI scheme to work out a sensible file path from it (if one is even possible at all), and to me this seems like a somewhat unfortunate thing to maintain that would best be avoided (since new virtual URIs may appear at any time).

Ideally, we could move towards a solution that treats the URIs as opaque and avoids file paths entirely.

@dannypsnl
Copy link
Contributor

Because we can load syntax first and no provide path to make-traversal1, and hence, we can have self-maintained filesystem that be distinguished by internal tags.

Footnotes

  1. https://docs.racket-lang.org/drracket-tools/Accessing_Check_Syntax_Programmatically.html#%28def._%28%28lib._drracket%2Fcheck-syntax..rkt%29._make-traversal%29%29

@6cdh
Copy link
Contributor

6cdh commented Sep 6, 2023

Do you mean support any URI (even for unsaved documents that have no file path at all), or only the subset like notebook cells, where there is something like a file path that could be extracted...?

It's not any URI contains a file path. But it's not important, as the code never reads the current file, it only reads the other files it rely on. For example, a test in tests/textDocument/formatting.rkt specify a uri file:///test.rkt that is not exist, but it works as it doesn't rely on other files.

We only need to make sure we know its path or hypothetical path if there is a relative require in the document.
When this happens, this information can only be conveyed by URI or current workspace.
So we can just try to extract a path from the URI. It never fails so we can't fallback to current workspace.

It's not easy to replace all path with uri in the code. There are many places where assume the source of document is a path. I tried it today but can't make this change easily.

VFS is an ideal solution especially if we have, say https, ssh URI in the future. But it means more work than the simple fix. And probably no one will have these problems and report them in recent years.

@6cdh
Copy link
Contributor

6cdh commented Sep 7, 2023

I made a change: https://github.com/6cdh/racket-langserver/tree/remove-uri-limit

But I can't get it work with notebooks, it seems to require extra config in the extension.

please try this commit.

@jryans
Copy link
Contributor Author

jryans commented Sep 7, 2023

I made a change: 6cdh/racket-langserver@remove-uri-limit

Ah wow, seems like a clever approach! 😄

But I can't get it work with notebooks, it seems to require extra config in the extension.

Yes, there are several additional steps:

  1. You have to install IRacket to get VS Code notebook to support Racket cells
  2. The extension needs to be changed to remove the file scheme filter

I've done these things locally and tested it with your langserver branch, and overall it solves the file path issue, so that's great! 😄

Unsaved files now work as expected (once you manually mark them as Racket files).

For notebook cells, the langserver does run (which is the focus of this issue), but you then quickly encounter a separate problem that IRacket doesn't really want the notebook cell to have #lang (more details), but the langserver requires it to be there. Anyway, resolving that seems like a separate issue.

@6cdh, I'd recommend submitting a PR with your langserver branch. Thanks for working on this! 😄

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.

3 participants