bug#58790: Eglot URI parsing bug when using clojure-lsp server

["João Távora" <joaotavora@gmail.com>]

Danny Freeman <danny@dfreeman.email> writes:

> I have a new patch attached that does what João suggested in the 5
> point list,
> which I will respond to again
>
>> 2. Eglot converts the URI X into a some file designator Y, using
>>    eglot--uri-to-path.  Y may or may not be === X, as long as there is
>>    exactly one X for every Y.  Eglot makes xref-item objects from it
>>    using xref-make-match and xref-make-file-location.  The designator Y
>>    goes into the 'file' slot of the xref-file-location object.
>
> The patch leaves X === Y for `jar` type URIs. My updated packages now
> deals with
> the full jar URIs. No conversion are done. My new package can be found here:
> https://git.sr.ht/~dannyfreeman/jarchive/tree/9148ed7ada03ff2516f6e4ede20c453974d6da19/item/jarchive.el

Cool, I had a brief look, and it looks pretty reasonable.  There are
indeed many operations, but most of them seem trivial.  If you're
bothered by the repetitive structure, you can probably use sth like
cl-case or pcase to cut down a bit.

>> 5. Upon findin the "file", Eglot transmits a :textDocument/didOpen
>>    notification to the server.  This requires eglot--path-to-uri, the
>>    reciprocal of eglot--uri-to-path to convert the path Y to URI X
>>    again.  Again, maybe this conversion is just #'identity.
>>
>>    Eventually, the LSP server knows that the client is now working on a
>>    textDocument whose relationship to other opened documents in the
>>    workspace it understands (which may or may not be read-only).
>
> Sending the full jar uri back to the lsp server worked exactly as
> intended here.

Nice.

>> 6. xref-find-definition on any part of the B2 should now work similarly
>>    to this whole flow.
>
> It does!!!!!

Double nice.

> As for this question
>
>> I don't know what happens if another server also points to the same file.
>> Probably nothing very bad, but there may be some suprising behavior: I
>> haven't tested.
>
> The same file continues to work with respect to the original lsp server
> that was used to open it. The second lsp server is not conidered. I
> guess because eglot can only work with one lsp server at a time. If
> you want to
> associate the un-jar'd file with the second server, close it and
> xref-find-defintions to it from a file in the second server. That
> seems like a
> fine workaround to me, and its not something I've ever encountered in
> my routine
> development workflow.

This isn't specific to clojure.  I don't think it's a big issue: noone
has complained and your description of how it works suggests that it
doesn't do anything exceptionally silly.

> So, what do yall think about the patch? Is that something you might
> consider for
> eglot? I believe I could also accomplish something similar with
> "advice" in my
> package and leave eglot alone, but I would be poking at eglot
> internals which
> seems not ideal.

Yes, a patch is in order.  Comments below:

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index c587061837..b8f50e3cd8 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -231,7 +231,7 @@ eglot-server-programs
                                 (html-mode . ,(eglot-alternatives '(("vscode-html-language-server" "--stdio") ("html-languageserver" "--stdio"))))
                                 (dockerfile-mode . ("docker-langserver" "--stdio"))
                                 ((clojure-mode clojurescript-mode clojurec-mode)
-                                 . ("clojure-lsp"))
+                                 . ("clojure-lsp" :initializationOptions (:dependency-scheme "jar")))
                                 (csharp-mode . ("omnisharp" "-lsp"))
                                 (purescript-mode . ("purescript-language-server" "--stdio"))
                                 (perl-mode . ("perl"
                                 "-MPerl::LanguageServer" "-e"
                                 "Perl::LanguageServer::run"))

Didn't you say that "jar" is already the default for clojure-lsp?  Do we
really need to force it?  What can happen if we don't?

+(defvar eglot-preserve-jar-uri nil
+  "If non-nil, jar: type URIs will not be converted to paths.
+This means they will be provided to xref as URIs and not file paths.")

We don't need this variable.  Eglot should no nothing about jars (except
perhaps in only place where "clojure" is mentioned, which is in
eglot-server-programs).

I think it's better to patch eglot--uri-to-path so that if X looks
anything other than file://, Eglot leaves it untouched.  After all, it's
the only safe translation Eglot can make.

And in eglot--path-to-uri, we do likewise.  If the PATH argument already
looks vaguely URIish (say, it makes something like "^[[:alnum:]]+://")
we leave it unchanged.

Let me know if that makes sense and you can implement it, else I'll try
it myself.

Felicián also suggested that Eglot warns the user when it doesn't know
an URI scheme.  I think that can make sense in some situations, for now
let's assume it isn't as important as getting your new Jar
file-name-handler to integrate with Eglot.  Maybe in some later patch
Eglot can somehow predict if there is a file-name-handler entry for a
given URI and only warn if there isn't.

João