zed icon indicating copy to clipboard operation
zed copied to clipboard

Add Clojure language support with tree-sitter and LSP

Open prcastro opened this issue 5 months ago • 20 comments

Current limitations:

  • Not able to navigate into JAR files

Release Notes:

  • Added Clojure language support

prcastro avatar Jan 29 '24 13:01 prcastro

We require contributors to sign our Contributor License Agreement, and we don't have @prcastro on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'.

cla-bot[bot] avatar Jan 29 '24 13:01 cla-bot[bot]

@cla-bot check

prcastro avatar Jan 29 '24 13:01 prcastro

The cla-bot has been summoned, and re-checked this pull request!

cla-bot[bot] avatar Jan 29 '24 13:01 cla-bot[bot]

Hey! I'd like some help with this PR. Currently the LSP is being downloaded and extracted correctly (at least it seems) but nothing LSP related it running on the editor.

How can I see the LSP logs within Zed?

prcastro avatar Jan 29 '24 14:01 prcastro

@prcastro you can view LSP logs by hitting cmd+shift+p and typing "debug: open language server logs". There are also logs in zed "zed: open log".

pocaeus avatar Jan 29 '24 16:01 pocaeus

Running "debug: open language server logs" makes Zed hang =s I'm checking with Eric how I can access the logs manually.

prcastro avatar Jan 29 '24 17:01 prcastro

@pseudomata I'm getting something like this:

[2024-01-29T18:28:03Z ERROR lsp] Cancelled LSP request task for "initialize" id 0 which took over 120s

Talking to @ericdallo we reached the conclusion the client is timing out before the LSP finishes opening the project (which includes even downloading the dependencies, afaik). Is there a way to increate the timeout of 120s?

prcastro avatar Jan 29 '24 18:01 prcastro

I am not sure if the timeout can be increased (or if that would be a desirable change) -- can you time how long clojure-lsp takes to start outside of Zed? I'm not super familiar, but does clojure-lsp use Graal (which AFAIK drastically improves startup time)

pocaeus avatar Jan 29 '24 18:01 pocaeus

It doesn't take long for small projects usually. However, it took a long time (more than 2min) to open a big project with a lot of dependencies to download and compile. About GraalVM, I'm not sure, maybe @ericdallo can answer that.

Also I made it work. Here are some limitations of my approach:

  • I had to increase the timeout to make it work (otherwise the LSP might not have enough time to start)
  • I currently cannot navigate into variables defined in JARs, because I receive the following error:
[2024-01-29T18:59:56Z ERROR project::worktree] failed to canonicalize root path: No such file or directory (os error 2)
[2024-01-29T18:59:56Z ERROR util] crates/editor/src/editor.rs:7045: ProjectPath { worktree_id: WorktreeId(30064771211), path: "" } opening failure: No such file or directory (os error 2)

For me those are not huge problems. WDYT?

prcastro avatar Jan 29 '24 19:01 prcastro

This looks close to be done, so it would be great to add a new docs entry to the https://github.com/zed-industries/zed/tree/main/docs/src/languages list before merging.

SomeoneToIgnore avatar Jan 29 '24 19:01 SomeoneToIgnore

This looks close to be done, so it would be great to add a new docs entry to the https://github.com/zed-industries/zed/tree/main/docs/src/languages list before merging.

Done!

prcastro avatar Jan 30 '24 00:01 prcastro

@osiewicz do you mind taking another look?

prcastro avatar Jan 31 '24 13:01 prcastro

Yup, LGTM on the first skim. I'll give it a go on some clojure project later.

osiewicz avatar Jan 31 '24 13:01 osiewicz

It looks like VSC does not have the problems with initialisation request (it completes within a reasonable time frame there), so there's definitely something Zed can do better there. I'm gonna try to work the differences out tomorrow. I am using Calva extension there and inspecting solely it's communication with clojure-lsp.

osiewicz avatar Jan 31 '24 21:01 osiewicz

One issue I've ran into is that clojure-lsp sends a request to the client to show a popup when it encounters issues during initialisation. e.g:

[Trace - 00:12:46] Received request 'window/showMessageRequest - (1)'.
Params: {
    "type": 2,
    "message": "LSP classpath lookup failed when running `lein with-profile +test,+dev classpath`. Some features may not work properly if ignored.\n\nError: Cannot run program \"lein\" (in directory \"/Users/hiro/Projects/throwaways/riemann\"): error=2, No such file or directory\n\nChoose an option:",
    "actions": [
        {
            "title": "Retry"
        },
        {
            "title": "Ignore"
        }
    ]
}


[Trace - 00:12:49] Sending response 'window/showMessageRequest - (1)'. Processing request took 3332ms
Result: {
    "title": "Ignore"
}

I don't have lein installed, hence the popup in Vscode. Funnily enough, response to initialize request comes right after that, so I suspect it's blocked on that response. We do not handle that request, hence clojure-lsp waits around forever. I don't believe we have API to display generic messages to user (in a way that they can choose an option), so we'd have to do a bit of work to handle the request proper. Alternatively, we could require that lein is installed on user machine (which we can do in fetch_server_binary or whatever) to kind of circumvent the issue. Opening Riemann in VSCode does indeed take 2 minutes to complete initialize request. Bummer.

osiewicz avatar Jan 31 '24 23:01 osiewicz

@osiewicz correct, the problem is that is not always lein, it depends on the type of clojure project, it could be lein, clojure, boot, babashka, npx, so not sure it's a good idea to require all those, the ideal would be to prompt to user

ericdallo avatar Feb 04 '24 20:02 ericdallo

Oh, I see; yeah, then we definitely need to prompt the user for an answer.

osiewicz avatar Feb 04 '24 21:02 osiewicz

At this point is it possible to evaluate expressions using interactive REPL? Similar to cider-jack-in using Emacs...

If not, how can this be implemented?

lanjoni avatar Feb 04 '24 23:02 lanjoni

Hey @osiewicz , just to understand the steps we need to take to get this merged:

  • Add dash to word characters
  • Implement a modal to send messages that require inputs to users
  • Implement a handler for the showMessageRequest that presents the modal to the user

Is that it? Any pointers to get started with step 2? I'll probably need a tip on how to get that started.

prcastro avatar Feb 05 '24 16:02 prcastro

Hey, I'd be down to pair on that sometime this week (preferably on Thursday) if needed. To display a modal, we could use something like WindowContext::prompt, which isn't really ideal (it'll look like the prompt you get for unsaved files); I think it's fine to start with that, we can touch up the prompts at some point down the line. For a showMessageRequest handler, I'd recommend using Workspace::show_notification - UI for that is in a good shape I believe, that will look like a update notification

osiewicz avatar Feb 05 '24 17:02 osiewicz

@osiewicz I would appreciate pairing on this issue on Thursday if you are available. Can you add me on Twitter so we can DM the details? It's @poliveiracastro over there

prcastro avatar Feb 06 '24 14:02 prcastro

Hey @osiewicz, I just finish what we started earlier today. Can you take a look and see if this PR is in an acceptable state now? Just to recap, we have some limitations:

  • Zed doesn't install lein for you when it's installing the LSP
  • We don't support navigating into a symbol (e.g. a function) defined in a JAR
  • We handle showMessageRequest by showing a WindowContext::prompt which is a macOS native prompt (not a prompt made in GPUI or anything).
  • We will still fail if the LSP takes too long to reply

prcastro avatar Feb 08 '24 20:02 prcastro

Yeah, I think that's fair; I think these can be fleshed out in a follow-up PR, this one relatively self-contained.

osiewicz avatar Feb 08 '24 20:02 osiewicz

@osiewicz Any idea why this is happening?

error[E0061]: this function takes 4 arguments but 3 arguments were supplied
  --> crates/zed/src/languages/clojure.rs:30:13
   |
30 |             latest_github_release("clojure-lsp/clojure-lsp", false, delegate.http_client()).await?;
   |             ^^^^^^^^^^^^^^^^^^^^^                                   ---------------------- an argument of type `bool` is missing

I could swear that this function call is correct

prcastro avatar Feb 08 '24 20:02 prcastro

I think that's due to https://github.com/zed-industries/zed/pull/7571/files#diff-c9e13c01768f22f905b924b7fc003f8288d1f6ab3d1b6f5232e178ef0162cd12 (which changed the signature of that function and landed on main today) - IIRC merges are tested against main (or on top of main should I say), so once you merge main into your branch, the code should stop compiling.

osiewicz avatar Feb 08 '24 21:02 osiewicz

Nice work, @prcastro.

Just a heads up - we are very soon going to be moving most of Zed's built-in languages into installable extensions (https://github.com/zed-industries/zed/issues/7096). Clojure is a popular enough language that I'm ok with merging this in the meantime. But once the extension system lands, we will probably remove this code and put it into a new clojure repository under the new zed-extensions GitHub org. We'll probably add you as a collaborator on that repo when we do it, if you're interested in helping maintain it.

maxbrunsfeld avatar Feb 10 '24 21:02 maxbrunsfeld

Thank you @prcastro and @osiewicz for your work on this! I’m using it and enjoying it. 🍻

aviflax avatar Feb 20 '24 18:02 aviflax