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

[Danny Freeman]

João Távora <joaotavora@gmail.com> writes:

> 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?

"jar" is not the default, "zipfile" is. The clojure-lsp maintainers want
to make jar the default but have not pulled the trigger on that yet.
Other lsp clients seem to be forcing "jar" in the initialization options
as well.

If we don't force it then I would be getting zipfile URIs, which is not
the end of the world, but I would prefer to only deal with the scheme
that the clojure-lsp devs want to use going forward. I could either set
the jar initialization option myself, tell the user to, or also support
the zipfile scheme. Setting it here is seems to be the easiest thing.

Including it does not have any downsides that I know of. Either way,
without some special file-name-handler dependencies in jars returned by
clojure-lsp cannot be opened. The end result is the same empty buffer.

> +(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.

This makes sense and seems very reasonable to me. I will send an updated
patch that tries to do this.

One question I have is, does it make sense to still parse the escape
sequences (via url-unhex-string) in the full URI? Or do we continue
to leave it as is? In my local testing I've left it as is but don't have
any jars that I can navigate to with special characters in the path.

> 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.

I think it would be pretty straight forward to see if something like
`find-file-name-handler` would pick up an unaltered URI for a common
operation that we know eglot will be using, like `expand-file-name`. If
it doesn't then a warning can be issued.