clojure-lsp / clojure-lsp

Clojure & ClojureScript Language Server (LSP) implementation
https://clojure-lsp.io
MIT License
1.16k stars 152 forks source link

Recondsider how navigation to dependencies that are not readily available on the client filesystem are handled #1385

Open dannyfreeman opened 1 year ago

dannyfreeman commented 1 year ago

Is your feature request related to a problem? Please describe. Currently clojure-lsp has a couple different ways of handling navigation to dependencies via methods like textDocument/definition.

If a definition is located inside the project, a file:// URL is provided to the client, which works fine.

If a definition is NOT inside the project, but instead in a jar then a couple of different things can happen:

  1. If the dependency is a clojure source file and the :dependency-scheme setting is not set or set to "zipfile", then a zipfile: URL is provided to the client. Clients need to understand the zipfile URI scheme to use this. Not all clients do. Some clients allow users to persist changes JAR dependencies (a footgun).
  2. If the dependency is a clojure source file and the :dependency-scheme setting is set to "jar", then a jar: URL is provided to the client. (spec). Clients either need to understand the jar URI scheme or make an extra call to the clojure/dependencyContents method with the jar URI to receive a response with the contents of the dependency. In theory, clients could allow users to persist changes in JAR dependencies.
  3. If the dependency is a .class file (java bytecode), then the file is unzipped, and de compiled into a temp directory, and clojure-lsp provides a file:// URL to the client. The client needs no special knowledge of clojure lsp to open the response.
  4. The dependency is external and NOT located in a jar file. Instead it is a regular file on the filesystem, referenced by something like a local maven coordinate in deps.edn. In this case the client receives a file:// URL and needs to special knowledge to open the response.

For this issue: I would like to find the optimal way to handle these dependencies. Some things that should be considered are

Describe the solution you'd like An ideal situation is one where clients do not need special code for dealing with clojure-lsp and clojure-lsp has a standard way of handling all external dependencies. This makes it easier for someone writing a new client, maybe a new editor, to work with clojure-lsp without having to implement special code. It also allows existing clients to simplify their implementations if they have already added the special code.

Describe alternatives you've considered Aside from clojure-lsp, other lsp servers in the JVM realm have their own way of handling these things.

Metals, a lsp for scala, will extract .scala source files from jars into a temp directory and provide a file URL. It will behave similarly to clojure-lsp when dealing with compiled java .class files.

jdt.ls, a java lsp, only provides a bespoke jdt URL for compiled .class files. It must be used in a second call to a custom LSP method to retrieve a decompiled source, similar to clojure/dependencyContents. It has no option for extracting to a temporary file, and the jdt URL scheme is not meant to be parsed by the client like a jar or zipfile URL can.

LSP servers have tried to solve this problem in a couple different ways. I believe it would benefit clojure-lsp and its clients if we consolidated on one strategy. What that strategy should be is hard to decide. To help us arrive at the best solution, an issue should be opened in the LSP specification. There we could see if the specification needs something more to deal with dependencies that are NOT readily available on the client's file system. Additionally, other lsp servers can also have a standard way of dealing with these types of situations.

I will try to open a LSP spec issue soon, and reference the issue in the microsoft repo from here.

Additional context This issue has been discussed in a number of other places: https://github.com/NoahTheDuke/coc-clojure/issues/8 https://github.com/joaotavora/eglot/issues/661 https://clojurians-log.clojureverse.org/lsp/2022-11-14

CC: @mainej @ericdallo

ericdallo commented 1 year ago

Great explanation of the problem @dannyfreeman, thank you! I find the "do everything on server" solution better to have better control and avoid different approaches on different clients. I think the upstream LSP spec issue may help a lot deciding what to do on clojure-lsp as all choices seem to have tradeoffs indeed.

Regarding "dependency-scheme", one thing we can already discuss is the zipfile support and if it should be the default or not in the server.

c/c @snoe

dannyfreeman commented 1 year ago

Regarding "dependency-scheme", one thing we can already discuss is the zipfile support and if it should be the default or not in the server.

It might help to know why the zipfile URI was added in the first place. Chesterton's fence and all that.

Some reasons I think it should be removed are:

ericdallo commented 1 year ago

The main reason I recall @snoe mentioning was that was allowed to edit the dependency on the fly, I personally dislike that but I can see people who think it's a good "feature".

mainej commented 1 year ago

To clarify a little... the zipfile scheme exists because some users want to be able to edit their .m2 directories. @snoe is a representative of this group.

However for other users, especially those who are using the zipfile scheme unintentionally, this "feature" can be a source of severe frustration. They "experiment" with editing a dependency file, thinking that it's just a harmless copy. Maybe they accidentally introduce a syntax error, or some snippet that only works in a REPL started from the current project. But they close their editor and think nothing more of it.

And then, sometimes months later, they discover that another project is broken. They have no breadcrumbs leading back to where the problem is. Version control is of no help. They might not even have touched the code in their broken project... it was working when they left it, and now it's not. If they're very experienced, clever, and lucky, they might think to delete their .m2, but probably only as a last resort. If they're not as experienced, clever or lucky, they're in for what may be days of debugging.

As @dannyfreeman more succinctly put it, the zipfile scheme is a footgun.

If I sound annoyed about this, it's because I've been that less-experienced, less-clever, less-lucky developer who had a problem magically fixed by deleting .m2. It's demoralizing to be turned away by your community with a "works for me". It's deeply unsatisfying to realize that rm -rf ~/.m2 could have avoided days of effort. It's guilt inducing to feel like you've wasted other's time. It's self-confidence eroding to see that your assumption that the problem must be in your code was just a form of blindness. I don't want clojure-lsp to inflict that experience on anyone, even accidentally.

So, I propose again that we deprecate and then remove the zipfile scheme from clojure-lsp. It's a bit buried, but @dannyfreeman has proposed two workarounds for users who like to be able to edit their dependencies:

  1. After being navigated to a dependency file, write a copy into the project's src/ directory. That is, save some/path/to/unzipped/lambdaisland/uri.clj to project-root/src/lambdaisland/uri.clj. The file can then be edited and evaluated, and will override whatever is in .m2. Changes can be committed as a project-specific patch.
  2. Check out the dependency's source code, and use :local/root to point to that local, editable copy. Changes can be committed and submitted upstream.

In neither case will edits to the dependency affect other projects on the same machine. If users want to make global changes that will affect multiple projects, despite those changes being hidden in non-version-controlled compressed files, that's their prerogative, but it's not a workflow I want to see clojure-lsp support.

If there are users who do want to know where .m2 files are, so they can build their own tooling to open those files, it'd be reasonable to request that clojure-lsp expose that. That information is accessible by clojure-lsp but is not especially easy to get to via other means. Technically, it's currently available via clojure/cursorInfo/raw, but it'd be reasonable to write a test to ensure we don't lose it. But I don't think that clojure-lsp should itself be involved in opening those files anymore. There are too many drawbacks.

Having written this, I see that it should be its own issue, with a formal proposal to remove the zipfile scheme. I'll write that up when I have a chance.

@snoe, as a final comment, you're the founder of clojure-lsp, so I personally owe you a lot of gratitude. I've found a lot of joy both using it and maintaining it. And so it feels bad to suggest taking away a feature that you like. I hope my doggedness asking to remove it doesn't come off as anything but a desire to see clojure-lsp have the most utility for the most people.

ericdallo commented 1 year ago

Thanks for the summary @mainej

After being navigated to a dependency file, write a copy into the project's src/ directory.

We need to take care with this, as having a copy of a dependency in a source-path may cause on didOpen we analyze that and then count 2 references on different files which may affect lens, references and other features, we could still change code to ignore those files, but sounds tricky.

Check out the dependency's source code, and use :local/root to point to that local, editable copy. Changes can be committed and submitted upstream.

This would mean we would need to reload classpath on the fly to re-consider the local/roots, right?

dannyfreeman commented 1 year ago

Check out the dependency's source code, and use :local/root to point to that local, editable copy. Changes can be committed and submitted upstream.

This would mean we would need to reload classpath on the fly to re-consider the local/roots, right?

When I originally suggested this alternative solution, I meant that the user update their deps.edn file themselves, then start clojure-lsp. Not as something that is done automatically or mid-flight. Just that the user change their dependencies.

mainej commented 1 year ago

Right... @ericdallo, I wasn't saying that clojure-lsp would participate, either in writing a file to src/, or in creating a directory that can be used as a :local/root. I was saying that users can do that themselves, IF they want to change the behavior of deps.

dannyfreeman commented 1 year ago

I have finally submitted an issue to the LSP spec: https://github.com/microsoft/language-server-protocol/issues/1595

ericdallo commented 1 year ago

Really appreciate your help @dannyfreeman! From linked issues, it seems csharp is one more use-case of this problem 😅

snoe commented 1 year ago

To clarify a little... the zipfile scheme exists because some users want to be able to edit their .m2 directories.

There are two reasons it exists. The most important is because it was an editor supported way to view the files. The second was the ability to edit that file.

However, I think we're conflating what belongs on the client vs the server. If a file IS available on the FS, I think the client should be pointed to it, rather than to an in-memory or tempfile copy. To me, vim's zipfile uri seems as good as any for reaching into jars. Then, it should be up to the lsp CLIENT whether or not to open that file in a readonly manner.

dannyfreeman commented 1 year ago

However, I think we're conflating what belongs on the client vs the server. If a file IS available on the FS, I think the client should be pointed to it, rather than to an in-memory or tempfile copy. To me, vim's zipfile uri seems as good as any for reaching into jars. Then, it should be up to the lsp CLIENT whether or not to open that file in a readonly manner.

The zipfile URI seems to only be understood natively by vim, and isn't a standardized URI. To me it seems like a worse option than the jar URI scheme. It at least has a standard that can be pointed to when telling client maintainers they have to support it. For that reason alone its at least worth making jar the default dependencyScheme option if removing it is off the table.

On the topic of what belongs on the client and what belongs on the server: I think that allowing clients to have control over how they open something is good. Thinking about that issue I've opened in the LSP spec repo, getting some kind of standardized exchange like the clojure/dependencyContents method into the LSP spec feels like the right move. Clients could opt in to using it. If they see some kind of scheme they want to handle on their own, like jar: or zipfile: then that's on them. And for things that they really can't open, like is the case zipped .class files, then all the clients can treat them the same.