NoahTheDuke / coc-clojure

coc.nvim plugin for clojure-lsp
Mozilla Public License 2.0
32 stars 3 forks source link

Instruct clojure-lsp about how coc.nvim conveys URLs #8

Open mainej opened 2 years ago

mainej commented 2 years ago

coc.nvim is a bit different from other editors in terms of how it conveys URLs.

  1. It uses what clojure-lsp calls a "zipfile" scheme for JAR files: "zipfile://some.jar::xxx.clj". Other editors tend to follow a "jar" scheme: "jar:file:///some.jar!/xxx.clj"
  2. It escapes URI strings, even though the URIs are in JSON which doesn't need to be escaped. So when communicating about the aforementioned file, it will actually describe it as "zipfile://some.jar%3a%3axxx.clj" (I think. It may also encode the ://. Needs to be verified).

clojure-lsp needs to know how each client handles URIs, to make sure that it stores data internally in a way that the client can access.

So, this is a proposal to add a few options to the client config, to inform clojure-lsp about how coc.nvim likes to handle URIs.

First, to address 1) above, it'd be useful if coc-clojure set :dependency-scheme "zipfile". @ericdallo, clojure-lsp uses "zipfile" internally, but mentions "zip" and :zip in the docs. Can we settle on "zipfile"?

Second, to address 2) above, it'd be useful if coc-clojure set :uri-scheme "escaped-path", or something like that. This would have to be coordinated with @ericdallo, because clojure-lsp doesn't understand this setting yet. See https://github.com/clojure-lsp/clojure-lsp/issues/1287 and https://github.com/clojure-lsp/clojure-lsp/pull/1327 for more discussion.

:uri-scheme "escaped-path" is just a proposal. Other keys or values may be more appropriate depending on which components of the URI are escaped.

ericdallo commented 2 years ago

@ericdallo, clojure-lsp uses "zipfile" internally, but mentions "zip" and :zip in the docs. Can we settle on "zipfile"?

Sure.

I know you are already working on a fix on server side so maybe we don't need any changes on coc-clojure, but I'd like to bring a question: Why vim uses zipfile and not jar as default? I don't see how that change could affect user experience, and having all clients using the same scheme would help a lot avoiding bugs and code maintainability, WDYT @mainej @snoe @NoahTheDuke? Of course we would need to test and make sure jar scheme works seamless on coc/nvim client

mainej commented 2 years ago

maybe we don't need any changes on coc-clojure

That's right. I think this is fixed on the server side, so we don't need a coc-clojure change. I'd still appreciate it if coc-clojure set :dependency-scheme "zipfile" (although see below), but it's not breaking anything. @NoahTheDuke feel free to close this issue.

Why vim uses zipfile and not jar as default?

💯 I agree—supporting the zipfile scheme adds complexity to what is already a very complex part of clojure-lsp and it would be nice if we could remove it. I'm not sure we can, but we should try, IMO. (I accidentally forced coc.nvim to use the jar scheme today, and I think it broke definition navigation. Lots of other things were breaking navigation too, including clojure-lsp changes, so I'm not sure the jar scheme was to blame.)

NoahTheDuke commented 2 years ago

Vim uses zipfile because jars are just archives and uses the same functionality for both (seen here).

I also discovered this 2019 coc.nvim issue that relates to clojure-lsp (opened by fellow clojurian @snoe) which relates to the discussion y'all had about url escaping.

mainej commented 2 years ago

@NoahTheDuke thanks for doing some archeology. @snoe do you have any additional historical context?

I just did an experiment. I'm using a build of clojure-lsp from https://github.com/clojure-lsp/clojure-lsp/pull/1327. In ~/.config/clojure-lsp/config.edn I set :dependency-scheme "jar". Then I opened clojure-lsp/lib/src/clojure_lsp/kondo.clj with nvim. Using the LSP find-definition command I navigated into kondo.core, and from there into one of its impl namespaces. It appears that nvim is perfectly happy using the jar scheme:

nvim_jarfile

So while it's true that nvim handles clojure-lsp's default scheme of zipfile, it doesn't seem like it actually needs it. Perhaps this means we really could remove zipfile.

snoe commented 2 years ago

@mainej Using zipfile allows changing the jar on disk. To me, this is an advantage, others might not like it.

ericdallo commented 2 years ago

I suppose this should be pretty specific, not sure it's worth considering the tradeoff for only that feature

NoahTheDuke commented 2 years ago

@mainej Using zipfile allows changing the jar on disk. To me, this is an advantage, others might not like it.

omg this explains times I've broken things and fixed it by clearing my .m2 cache hahaha. Maybe I should change the default to jar and include config to keep it zipfile. Would that work for you?

mainej commented 2 years ago

I have to revise what I said earlier... for me, vim handles the jar scheme, but nvim doesn't. Earlier I must have been testing with vim, not nvim accidentally. I'm not sure what it means that the jar scheme works in one but not the other... perhaps there's a bug in nvim? (Also, I'm surprised to learn that coc.nvim and all the rest of @snoe's config seems to work in both vim and nvim.)

Using zipfile allows changing the jar on disk

@snoe, suppose nvim could deal with the jar scheme... should the server have to participate in order for the editor to be able to edit files? Shouldn't that be the editor's job? That is, wouldn't it be better to teach n/vim to edit files that use the jar scheme? Or perhaps write your own script that re-opened a jar scheme file as a zipfile?

I can see both sides to this. If those questions were being posed to me, I might answer "Yes... and... at the time, it was easier to teach clojure-lsp to use the zipfile scheme."

@NoahTheDuke I apologize—we've veered from the original topic, and I'm not sure coc-clojure has any role to play in this. If you'd like we can close this issue and continue the discussion elsewhere, perhaps in clojure-lsp.

ericdallo commented 2 years ago

I'd be nice if one could create a issue on nvim side to track/understand that better

snoe commented 1 year ago

@snoe, suppose nvim could deal with the jar scheme... should the server have to participate in order for the editor to be able to edit files? Shouldn't that be the editor's job? That is, wouldn't it be better to teach n/vim to edit files that use the jar scheme? Or perhaps write your own script that re-opened a jar scheme file as a zipfile?

@mainej So, this approach is what zipfile is imo - other editors are free to handle the scheme. jar, on the other hand changes the response from the server so that the server explicitly provides the file contents to the editor. LSP clients do work to provide an anonymous buffer to display that content. In effect with jar the server "owns" the file content and writing back may be possible, but it might violate assumptions that lsp clients make.

mainej commented 1 year ago

@snoe I think you're talking about "clojure/dependencyContents". I don't know that part of the code well. I think it's true that at some point it was put in place for clients to be able to request the file content of a jar scheme file. The client is told "here's a jar URI" and it then turns around and asks the server for the content. The server unzips the jar, finds the file, and returns the file content. Because the server provided the content, the editor has no way to save edits. Is all of that right?

@ericdallo is it also true that we're moving away from that approach, for example here, and instead asking the editors to take that responsibility? That is, we're moving toward a world where we tell clients "here's the URI for a jar file" and we expect them to know what to do... figure out where the jar is on disk, unzip it, show a buffer with one of the files in that archive, and if they so desire, support edits of that file by writing them back into the archive?

If it is true, and I'm not sure it is, then I personally support it! In my opinion those responsibilities—finding archive files, opening them, and editing them—are all editor responsibilities. I understand why clojure-lsp might have put in place workarounds for editors that couldn't do all of that. But to me, the long term solution is to improve the clients, not provide server-side workarounds. This is especially true if those workarounds aren't seamless and require client-side changes anyway. That is, rather than teach each client about our special "clojure/dependencyContents" JSON-RPC message, everyone's time would be better spent teaching the editors to handle the jar scheme natively. That way, other JVM language servers can benefit too. If the issue gets solved at the editor level, then it improves the client ecosystem too. Competing clients can also use the same code. That's the approach being taken in https://github.com/joaotavora/eglot/issues/661, which is filed for eglot but looks like it may be fixed in a way that benefits lsp-mode and even CIDER too.

I'd be willing to be corrected, but I think that Calva already understands the "jar" scheme without needing "clojure/dependencyContents". Emacs appears to be moving in that direction. That leaves nvim.

So, @NoahTheDuke perhaps this is an issue for coc-clojure, coc.nvim, or nvim after all. That is, if I understand correctly, @snoe you aren't attached to the "zipfile" scheme, but instead like the functionality it offers, functionality coc-clojure doesn't currently offer if you use the "jar" scheme. But you'd be happy if nvim could open "jar" scheme files and if those files were editable. Preferably the choice of whether archive files were read-only or editable would be a configuration option in coc-clojure or coc.nvim, but in the worst case it would be OK if nvim could be toggled from a read-only mode into an editable mode.

I don't understand the vim ecosystem very well, but I think that vim can open jar files (if not edit them), so it's arguably a bug that nvim can't. So is that the place to start, with a request that nvim learns how to open "jar" scheme files?

ericdallo commented 1 year ago

@mainej You are right, I agree entirely.

The spec doesn't say server should support reading URI content because I do think they agree that should be client responsibility.

snoe commented 1 year ago

@mainej that's all largely correct.

One of the big problems is that neither zip or jar uris are officially specced.

I used the zipfile scheme that vim and nvim both used and was also used by fireplace, in the hopes that emacs had or could add support for the same. Also, since jars ARE zipfiles, support for zip in general would naturally expand to cover jar.

I did survey in-the-wild jar uris, and found about 4 or 5 formats. How to address a file within an archive is poorly described by the URI rfcs and the jar ones were a reflection of that - some used # some used ; some used ?. Maybe that's consolidated in the last few years.

It would be interesting to know what the java-lsps do.

snoe commented 1 year ago

One other point, vimscript is probably the worst client plugin language in use, so that also played into the decision to lift their scheme, knowing that it would be far easier for elisp or js clients to translate into their native archive-file schemes.

NoahTheDuke commented 1 year ago

I did survey in-the-wild jar uris, and found about 4 or 5 formats. How to address a file within an archive is poorly described by the URI rfcs and the jar ones were a reflection of that - some used # some used ; some used ?. Maybe that's consolidated in the last few years.

This is a fuckin mess lol.

I've tried to keep up but a lot of this is far outside of my knowledge base so I don't have much to offer. I'm down to do what needs to be done, but so far, it seems like the answer is "nothing"?

dannyfreeman commented 1 year ago

@snoe

It would be interesting to know what the java-lsps do.

They are all different. I have looked into 3, but there are probably more language servers out there

There is nothing in the LSP spec to really help here either. I am looking into submitting an issue over there to see if something could be added to the spec to help mediate these kinds of exchanges where the source file isn't readily available as a file on disk.