Fallback to starting lsp-server in tmp dir

[julienvincent]

Currently we do not auto-start the lsp server if no valid clojure projects are found. This means that we won't have any lsp support for files in workspace folders that do not contain project files. This can be unexpected.

Additionally this means we don't support lsp operations on files opened which are not part of any workspace (for example when peeking or using File > Open on a random file on the host).

This commit makes two changes:

• Adds a fallback behaviour of provisioning an lsp server in a OS tmp directory if no valid project is found.
• Adds .lsp/config.edn as a heuristic for a valid clojure project.

Some misc changes:

• The way we store and reference clients needed to be refactored a bit to support the fallback client behaviour. This is the main reason for the large amount of changes.
• The status-bar item was not properly showing when a client was starting (spinning icon). This has been fixed as part of the refactor.

Fixes #2069

Checklist

• Read How to Contribute.
• Directed this pull request at the dev branch. (Or have specific reasons to target some other branch.)
• Made sure I have changed the PR base branch, so that it is not published. (Sorry for the nagging.)
• Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.
• Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and either tested it there if so, or mentioned it in the PR.
• Added to or updated docs in this branch, if appropriate
• Tests
  • Tested the particular change
  • Figured if the change might have some side effects and tested those as well.
• Referenced the issue I am fixing/addressing in a commit message for the pull request.
  • If I am fixing the issue, I have used GitHub's fixes/closes syntax
  • If I am fixing just part of the issue, I have just referenced it w/o any of the "fixes” keywords.
• Created the issue I am fixing/addressing, if it was not present.
• Formatted all JavaScript and TypeScript code that was changed. (use the prettier extension or run npm run prettier-format)
• Confirmed that there are no linter warnings or errors (use the eslint extension, run npm run eslint before creating your PR, or run npm run eslint-watch to eslint as you go).

[PEZ]

• Adds .lsp/config.edn as a heuristic for a valid clojure project.

Should we treat .clj-kondo/config.edn the same way? For me it is much more common with such a file.

[julienvincent]

Should we treat .clj-kondo/config.edn the same way? For me it is much more common with such a file.

Makes sense, added in ddefda8

[PEZ]

When I add a folder to the mono-repo workspace that does not have a Clojure project in it, a mysterious clojure-lsp server is started. Is this by design?

The server remains running when I remove the non-clojure folder.

When I add a clojure-project folder and then remove it, its server is also still running. I'm thinking this might be so in #2075 too, but have not yet tried it.

[PEZ]

I am guessing that the ”mystery” server is serving all non-clojure folders? I added an empty folder and that did not start a new one, and adding a clj file there I get both clojure-lsp and clj-kondo help there.

I think this might be fine. This server is not using much memory on my machine. Maybe we can add a description to it, so that we can tell the user why it is there. Then it wouldn't be a ”mystery”, I am thinking.

Is the server shared among all VS Code windows? If so it should probably always show up in the clojure-lsp menu when it is running. WDYT?

[PEZ]

After adding the empty folder, I have a few of these in the dev console:

[Extension Host] Failed to get results from completions provider 'lsp' Error: The request {:id 118, :method "textDocument/completion"} has been cancelled.
    at handleResponse (/Users/pez/Projects/calva/node_modules/vscode-jsonrpc/lib/common/connection.js:531:48)
    at processMessageQueue (/Users/pez/Projects/calva/node_modules/vscode-jsonrpc/lib/common/connection.js:327:17)
    at Immediate.<anonymous> (/Users/pez/Projects/calva/node_modules/vscode-jsonrpc/lib/common/connection.js:311:13)
    at processImmediate (node:internal/timers:466:21)
    at process.callbackTrampoline (node:internal/async_hooks:130:17) (at console.<anonymous> (/Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/api/node/extensionHostProcess.js:96:137522))

[PEZ]

I tried Calva: Fire up the Getting Started REPL from a new VS Code window and hoped a bit that the tmp server would be active there. It wasn't. You think it is possible to make it so? Doesn't have to be in this PR, but would be a great improvement of the Getting Started REPL experience.

[julienvincent]

When I add a folder to the mono-repo workspace that does not have a Clojure project in it, a mysterious clojure-lsp server is started. Is this by design?

Yes, that is what this PR is adding

I think this might be fine. This server is not using much memory on my machine. Maybe we can add a description to it, so that we can tell the user why it is there. Then it wouldn't be a ”mystery”, I am thinking.

Adding a label is probably a good idea yes, will see what I can do

Is the server shared among all VS Code windows? If so it should probably always show up in the clojure-lsp menu when it is running. WDYT?

No there isn't really a way to share a process between vscode windows like that I don't think. So new windows would start new processes, just as before.

After adding the empty folder, I have a few of these in the dev console:

[Extension Host] Failed to get results from completions provider 'lsp' Error: The request {:id 118, :method "textDocument/completion"} has been cancelled.
    at handleResponse (/Users/pez/Projects/calva/node_modules/vscode-jsonrpc/lib/common/connection.js:531:48)
    at processMessageQueue (/Users/pez/Projects/calva/node_modules/vscode-jsonrpc/lib/common/connection.js:327:17)
    at Immediate.<anonymous> (/Users/pez/Projects/calva/node_modules/vscode-jsonrpc/lib/common/connection.js:311:13)
    at processImmediate (node:internal/timers:466:21)
    at process.callbackTrampoline (node:internal/async_hooks:130:17) (at console.<anonymous> (/Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/api/node/extensionHostProcess.js:96:137522))

This wasn't introduced by this PR afaict. I have been intermittently getting this in my console since the start - I think it's just normal operation (vscode cancelling the request due to it being stale)

I tried Calva: Fire up the Getting Started REPL from a new VS Code window and hoped a bit that the tmp server would be active there. It wasn't. You think it is possible to make it so? Doesn't have to be in this PR, but would be a great improvement of the Getting Started REPL experience.

If you use "when-file-opened-use-furthest-project" then the lsp server is started and used in the getting started repl. It won't start when using the other setting enums as the getting started project is never added as a workspace folder to vscode.

We can improve on this by explicitly provisioning the fallback lsp server when running the "Fire up the Getting Started REPL" command - but lets play with that in a separate PR.

[PEZ]

Is the server shared among all VS Code windows? If so it should probably always show up in the clojure-lsp menu when it is running. WDYT?

No there isn't really a way to share a process between vscode windows like that I don't think. So new windows would start new processes, just as before.

We wouldn't need to share the process. Rather connect the client to an existing lsp server. Let's consider in a separate PR.

[julienvincent]

We wouldn't need to share the process. Rather connect the client to an existing lsp server. Let's consider in a separate PR.

While theoretically possible, I don't think the vscode lsp client library supports connecting to an independently running process, which would make implementing this hard (support would probably need to be added upstream)

[PEZ]

Can we connect several clients to the same server, @ericdallo ? I seem to recall having done this at some point...

[julienvincent]

@PEZ this wouldn't be a limitation of clojure-lsp but rather of the vscode client library. Though now looking more closely at the API it seems there might be a way to pass it a read/write stream instead of a process to start. So I guess this is possible.

[ericdallo]

Can we connect several clients to the same server, @ericdallo ? I seem to recall having done this at some point...

Hum, probably not, could you elaborate a real example why that would make sense?

[PEZ]

@ericdallo in this PR we are adding so that a general clojure-lsp server is started that serves files that are not part of a clojure project. E.g. if some of the workspace root folders are not Clojure projects (no deps.edn, etc), EDN and Clojure files in there can still get clojure-lsp help. All such folders will be served by the same clojure-lsp server. (Please correct me if I describe this in the wrong way, @julienvincent.)

If someone (like me) has 20 VS Code windows open, that can amount to 20 clojure-lsp servers started. It would be good if all VS Code windows could share the same ”general” clojure-lsp server. It's similar if the same Clojure project is opened in several Windows. That specific clojure-lsp server could also be shared then.

[PEZ]

it seems there might be a way to pass it a read/write stream instead of a process to start

Yes, that's what I have played with some year ago or so.

[ericdallo]

I see, for the same project open on multiple windows, that really makes sense and should work.
And for orphan clojure-lsp process (that threre is no root defined), should work with limited classpath/features, but should work as well, quite interesting PR

[julienvincent]

Maybe we can add a description to it, so that we can tell the user why it is there. Then it wouldn't be a ”mystery”, I am thinking.

Added a description to the fallback client in the lsp management menu - f6c2a6c

The server remains running when I remove the non-clojure folder.

When I add a clojure-project folder and then remove it, its server is also still running.

Fixed in fcb7b22

[julienvincent]

@PEZ I think this and #2075 are probably ready to go. Anything outstanding you would like me to address?

[julienvincent]

I have updated the documentation to include a section about the fallback server behaviour.

[PEZ]

I have updated the documentation to include a section about the fallback server behaviour.

🎉

@PEZ I think this and #2075 are probably ready to go. Anything outstanding you would like me to address?

I have the same concerns as with #2075 about if this is tested?

[julienvincent]

This is tested to the best of my ability, but I may have missed something. Perhaps you can think of some scenarios that I need to test specifically?

[julienvincent]

Tested this out on my windows VM and it seems to be working the same. The VM is super clunky which makes it difficult to test everything very well, but the temp directory stuff is working.

[PEZ]

Sorry for late reply. I totally missed you had been testing it, @julienvincent. (Thanks @bpringe for reminding me about it!)

Let's get this merged then! I'll read up on what order we decided to do it.

[PEZ]

I started here. 😄 Let me know when the next PR in the pack is ready for merging. You might wanna ping me on slack, my Github feed is a bit noisy and I miss stuff.