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

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

[Danny Freeman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1.1.1: Type: text/plain, Size: 2357 bytes --]

Hello,

I am submitting 2 patches that were discussed over on eglot's github
issue tracker: https://github.com/joaotavora/eglot/issues/661

Only one of the patches is needed to solve the issue, but I wanted to
present both options.

The problem occurs when using `xref-find-defintions` while eglot is
managing a clojure buffer with clojure-lsp. If the symbol that
xref-find-definitions is activated on is defined in a jar file,
clojure-lsp by default will provide a location with a response like the 
following

(:jsonrpc "2.0" :id 14 :result
   (:uri 
"zipfile:///Users/andreperictavares/.m2/repository/org/clojure/tools.namespace/0.3.1/tools.namespace-0.3.1.jar::clojure/tools/namespace/find.clj" 
... ))

However, there is a clojure-lsp setting that the maintainers of 
clojure-lsp would like to make default that changes the URI format to 
send a response like this

(:jsonrpc "2.0" :id 14 :result
   (:uri 
"jar:file:///Users/andreperictavares/.m2/repository/org/clojure/tools.namespace/0.3.1/tools.namespace-0.3.1.jar!clojure/tools/namespace/find.clj" 
... )).

This jar format URI is a common thing used in the JVM world apparently. 
It is a URI that contains a nested URI. If eglot is provided this kind 
of URI it needs to parse it TWICE before eglot dispatches the path. If 
it's only parsed once then emacs ends up trying to navigate using 
something roughly equivalent to `(find-file "file:///path/to/jar")` 
which does not work. That is what the patch titled 
`0001-Parse-jar-scheme-URIs-in-eglot-correctly.patch` fixes.

The other patch forces clojure-lsp to use the `zipfile` scheme and 
avoids the need to parse the URI twice. I prefer the double parsing 
patch though, as I believe if other language servers for JVM languages 
use this jar URI scheme, they would also be able to benefit from the patch.

I have a very simple repository here that can be used to test the 
problem here: 
https://git.sr.ht/~dannyfreeman/eglot-xref-to-jar-repo/tree/main/item/src/user.clj#L4

With clojure and clojure-lsp installed, eglot activated in a buffer 
visiting that `user.clj` file, the issue can be recreated.

This is my first time attempting to contribute to emacs. I am actively 
going through the process of copyright assignment right now in case it 
is necessary.

Thank you,
Danny Freeman

[-- Attachment #1.1.2: 0001-Initialize-clojure-lsp-with-the-zipfile-dependency-s.patch --]
[-- Type: text/x-patch, Size: 1945 bytes --]

From c20c0185929fbb3f5ca0101cab38721ddac412d6 Mon Sep 17 00:00:00 2001
From: dannyfreeman <danny@dfreeman.email>
Date: Tue, 25 Oct 2022 08:15:26 -0400
Subject: [PATCH] Initialize clojure-lsp with  the "zipfile" dependency-scheme
 option

When eglot is provided clojure dependencies that are located in external
jars, there are two formats they could be provided in.

The recommended format is "jar", which contains a jar URI like
"jar:file:///path/to/jar!/path/in/jar"
this URI contains a nested URI that eglot is not equipped to handle.

Setting this value to "zipfile" provides them in the following format
"zipfile:///path/to/jar::/path/in/jar"
which doesnt not contain any nested URIs.

This change ensures that we use zipfile so that the URI always correctly
has the scheme removed from the uri, and ends up being provided to xref
like "/path/to/jar::path/in/jar".
---
 lisp/progmodes/eglot.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index a28df6c2d5..c9d08de0d9 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -230,7 +230,7 @@ language-server/bin/php-language-server.php"))
                                 (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 "zipfile")))
                                 (csharp-mode . ("omnisharp" "-lsp"))
                                 (purescript-mode . ("purescript-language-server" "--stdio"))
                                 (perl-mode . ("perl" "-MPerl::LanguageServer" "-e" "Perl::LanguageServer::run"))
-- 
2.37.3


[-- Attachment #1.1.3: 0001-Parse-jar-scheme-URIs-in-eglot-correctly.patch --]
[-- Type: text/x-patch, Size: 2158 bytes --]

From f6e0a6d1ff557719b11cdf357fc24246c9da6a86 Mon Sep 17 00:00:00 2001
From: dannyfreeman <danny@dfreeman.email>
Date: Tue, 25 Oct 2022 08:22:20 -0400
Subject: [PATCH] Parse "jar" scheme URIs in eglot correctly

jar schemes contain nested URIs, like-this'
"jar:file:///home/user/some.jar!/path/in/jar" such that when eglot was
parsing these URIs provided by lsp servers, it ended up trying to
navigate to "file:///home/user/some.jar!/path/in/jar".

This change parses the URIs twice if the scheme is "jar", so that the
final parsed URI looks like "/home/user/some.jar!/path/in/jar" instead.

When providing this path to a jar to tools like xref, emacs will still
not correctly open the file within the jar, but this change makes that
work easier.
---
 lisp/progmodes/eglot.el | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index a28df6c2d5..549de5cba4 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1501,12 +1501,22 @@ If optional MARKER, return a marker instead"
              (directory-file-name (file-local-name truepath))
              eglot--uri-path-allowed-chars))))
 
+(defun eglot--parse-uri (uri)
+  (url-unhex-string
+   (url-filename
+    (let ((url (url-generic-parse-url uri)))
+      (if (string= "jar" (url-type url))
+          ;; jar: URIs can contain a nested URI, so we need to parse twice.
+          ;; For example, `jar:file:///home/user/some.jar!/path/in/jar'
+          (url-generic-parse-url (url-filename url))
+        url)))))
+
 (defun eglot--uri-to-path (uri)
   "Convert URI to file path, helped by `eglot--current-server'."
   (when (keywordp uri) (setq uri (substring (symbol-name uri) 1)))
   (let* ((server (eglot-current-server))
          (remote-prefix (and server (eglot--trampish-p server)))
-         (retval (url-unhex-string (url-filename (url-generic-parse-url uri))))
+         (retval (eglot--parse-uri uri))
          ;; Remove the leading "/" for local MS Windows-style paths.
          (normalized (if (and (not remote-prefix)
                               (eq system-type 'windows-nt)
-- 
2.37.3


[-- Attachment #1.1.4: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3185 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Stefan Kangas]

Danny Freeman via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

> I am submitting 2 patches that were discussed over on eglot's github
> issue tracker: https://github.com/joaotavora/eglot/issues/661
>
> Only one of the patches is needed to solve the issue, but I wanted to
> present both options.

Copying in João here.

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

[Danny Freeman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Carrying on a conversation from github

 > > Does eglot (or maybe project.el) have a mechanism for adding an
 > > external directory to the current project? I think that would be
 > > what I need to do this automatically.

 > I think project.el does. But then again this seems like a completely
 > different approach and seems to negate the need for the patches you
 > sent to Eglot.

The patches would still be necessary. The patches simply make eglot
correctly parse the "jar:" scheme URIs from lsp servers that use them.
It doesn't have much to do with how emacs is handling the file paths
once it is parsed.

 > In my mind, project.el should support adding jars as collections of
 > source files just like it supports adding directories as collections
 > of source files. Many years ago, Eclipse did this. You could add a jar
 > as a library or a file as a library: it's just a different
 > implementation detail.

Maybe so, it's unclear in my mind how that would work with lsp servers.
Without the files being extracted somewhere would they be able to
perform any analysis on them? Right now I don't think clojure-lsp is
capable, and I doubt other lsp servers are as well.

I think maybe there is some way I could extend the
`project-external-roots` after I've extracted the file somewhere to make
project (and thus eglot) consider the newly extracted to directory part
of the original project.

Either way, I think that is still work to do separately outside of eglot
right now. I'm sort of using that jarchive package I wrote as a place to
experiment with ideas like this.

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

[João Távora]

Danny Freeman <danny@dfreeman.email> writes:

> Danny: The patches would still be necessary. The patches simply make
> eglot Danny: correctly parse the "jar:" scheme URIs from lsp servers
> that use them.  Danny: It doesn't have much to do with how emacs is
> handling the file paths Danny: once it is parsed.

Indeed, I am in agreement with the last sentence.  And this means the
result of this parsing is arbitrary as long as it's a bijection.

>> In my mind, project.el should support adding jars as collections of
>> source files just like it supports adding directories as collections
>> of source files. Many years ago, Eclipse did this. You could add a jar
>> as a library or a file as a library: it's just a different
>> implementation detail.
>
> Danny: Maybe so, it's unclear in my mind how that would work with lsp
> Danny: servers.

Here's how I envision it working, from reading some of the source
involved, but with no testing:

1. In buffer B1, the user uses M-. (xref-find-definition).  Eglot
   requests :textDocument/definition from the LSP server, which returns
   a number of definitions.  Some of these look like

     jar:file:///home/user/.m2/repository/org/clojure/clojure/1.10.3/clojure-1.10.3.jar!clojure/core.clj

   Call this value X.

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.

   We'll see later if and how eglot--uri-to-path needs to be changed to
   do this conversion in a way that doesn't hurt all the other
   conversions it already does there.  One of the possibilities is just
   to leave X completely untouched and pass it onwards.

   The word "path" in eglot--uri-to-path should be understood as "file
   name" or "file designator" (depending on whether you are into Elisp
   or CL parlance).  It does not necessarily represent a filesystem
   path.  eglot--uri-to-path should probably be renamed, but that is a
   cosmetic tweak.

3. Using xref--show-location on one of these matches will eventually
   call xref-location-marker and invoke invoke find-file-noselect on Y.
   
   Here, an entry of file-name-handler-alist should know how to handle the
   format Y.  This is what Danny is working on as a separate package.

   If that entry exists, the process should land the user in some buffer
   B2 where buffer-file-name is set to Y.

4. B2 can be setup in a way so that project-current returns the same
   object which is returned in B.  If this is true,
   eglot--current-server discovers the same connection which also
   manages B1 and Eglot adds buffer B2 to the list of managed buffers in
   that connection.

   However, if eglot-extend-to-xref is non-nil, eglot--current-server
   should also discover the correct connection.  This is less ideal than
   making project-current know that the buffers of source code inside
   the jar are also in the same project, but it works.  I can explain
   the downsides, but it's not very relevant right now.

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

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

> Dmitry: Sounds like you want to advise project-vc-external-roots-function. Or
> Dmitry: change its whole value.

I don't understand that "vc" has to do with this.  The above
implementation should work with any project backend, not just VC.

To be clear, my suggestion was to add the ability to add a jar file to
project-external-roots.  Currently a root is a string representing a
directory, per its docstring.  But it could be generalized to a string
representing any container of files.

> Dmitry: Or create an Eglot-specific project backend.

I don't understand this suggestion either.  Normally Eglot is a client
of project information maintained by other project.el backends.  Very
commonly VC projects, but not always necessarily so.  That clashes with
the idea of making Eglot simultaneously a supplier of this information.

Please read the summary of the outlined above.  Maybe there's nothing to
be done in project.el if eglot-extend-to-xref is to be used.

Or maybe I'm misunderstanding the whole flow, or missing some detail, in
which case feel free to correct me.

> Danny: Without the files being extracted somewhere would they be able to
> Danny: perform any analysis on them?

Maybe Eglot just needs to be changed to _not_ strip the leading
"jar:file" and leave it completely unchanged.

So the server should be able to sort itself out, as long as you give
back the very same URI you got -- from the server itself -- to the
in-JAR source code.

João

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

[Dmitry Gutov]

On 27.10.2022 18:09, João Távora wrote:

> 4. B2 can be setup in a way so that project-current returns the same
>     object which is returned in B.  If this is true,
>     eglot--current-server discovers the same connection which also
>     manages B1 and Eglot adds buffer B2 to the list of managed buffers in
>     that connection.
> 
>     However, if eglot-extend-to-xref is non-nil, eglot--current-server
>     should also discover the correct connection.  This is less ideal than
>     making project-current know that the buffers of source code inside
>     the jar are also in the same project, but it works.  I can explain
>     the downsides, but it's not very relevant right now.

This point seems to be key, to be able to continue 'M-.' from inside the 
jar.

> 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).
> 
> 6. xref-find-definition on any part of the B2 should now work similarly
>     to this whole flow.
> 
>> Dmitry: Sounds like you want to advise project-vc-external-roots-function. Or
>> Dmitry: change its whole value.

FWIW, project-external-roots might be a red herring for this discussion. 
I just saw somebody mention project-external-roots, and the above would 
be a way to add to that list.

> I don't understand that "vc" has to do with this.  The above
> implementation should work with any project backend, not just VC.

Okay...

> To be clear, my suggestion was to add the ability to add a jar file to
> project-external-roots.  Currently a root is a string representing a
> directory, per its docstring.  But it could be generalized to a string
> representing any container of files.

Container or not, the return value of project-external-roots, just like 
that of project-root, is determined by the project backend, its internal 
structures, and how it is customized by the user.

We don't provide a setter for 'project-root', so I don't understand the 
expectation of being able to modify project-external-roots for an 
arbitrary project type either.

>> Dmitry: Or create an Eglot-specific project backend.
> 
> I don't understand this suggestion either.  Normally Eglot is a client
> of project information maintained by other project.el backends.  Very
> commonly VC projects, but not always necessarily so.  That clashes with
> the idea of making Eglot simultaneously a supplier of this information.

There is indeed certain tension, but if Eglot wants to decide stuff 
about the project (which, as I said in the past, could be a reasonable 
idea), then it could provide its own project backend. We're not 
necessarily at that point, though, because...

> Please read the summary of the outlined above.  Maybe there's nothing to
> be done in project.el if eglot-extend-to-xref is to be used.

...indeed you could stop at that.

Having the jars in project-external-roots could enable the users (after 
certain integration work) to search across their contents with 
project-or-external-find-regexp, or jump to files inside with 
project-or-external-find-file.

But as for xref-find-definitions, item 4 in your list should be enough 
(with either of the alternatives as underlying implementation).

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

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> We don't provide a setter for 'project-root', so I don't understand
> the expectation of being able to modify project-external-roots for an
> arbitrary project type either.

I think I understand your confusion.  I'm not suggesting that Eglot
modifies it.  I'll explain better below.

>>> Dmitry: Or create an Eglot-specific project backend.
>> I don't understand this suggestion either.  Normally Eglot is a
>> client
>> of project information maintained by other project.el backends.  Very
>> commonly VC projects, but not always necessarily so.  That clashes with
>> the idea of making Eglot simultaneously a supplier of this information.
>
> There is indeed certain tension, but if Eglot wants to decide stuff
> about the project (which, as I said in the past, could be a reasonable
> idea), then it could provide its own project backend. We're not
> necessarily at that point, though, because...

Eglot doesn't want to decide anything about project. Eglot justs wants
to go into user visible projects and answer the question:
(project-current).  It wants this because it maps project/major-mode
pairs to server connections. 

Eglot doesn't care if the project is of type 'vc, 'transient,
'visual-studio-solution-file, 'joes-complicated-project, etc.  

So Eglot providing a project backend doesn't make sense.  Maybe you
think I'm suggesting that Eglot that could collect these references to
jars coming from the LSP server and add them to project-external-roots
somehow.  I'm not suggesting that.  

It's just that an arbitrary project backend, other than 'vc or
'transient, could add a method to project-external-roots.  That would be
the user's job.  I suppose Clojure packages declare somewhere which jars
they use.  They probably store this information in a file.  Java used
some ghastly .xml Ant file or Maven or whatever.  A specialized project
backend could read the file and use it in an implementation of
project-external-roots.  At this this is how I interpret project.el's
CLOS-like protocol for defining new project backends.

>> Please read the summary of the outlined above.  Maybe there's nothing to
>> be done in project.el if eglot-extend-to-xref is to be used.
>
> ...indeed you could stop at that.

Maybe.  eglot-extend-to-xref works very well for non-jars, at least for
C++ and clangd.  Subsequent M-. work very well, too.

The downside is that once a system file discovered by the LSP server, it
is associated with a given server (_not project_) in Eglot.  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.

> Having the jars in project-external-roots could enable the users
> (after certain integration work) to search across their contents with
> project-or-external-find-regexp, or jump to files inside with
> project-or-external-find-file.

That's a very nice point.  I don't use Java fortunately, but when I did
a long time ago, I think I remember Eclipse let me do this.

> But as for xref-find-definitions, item 4 in your list should be enough
> (with either of the alternatives as underlying implementation).

Let's see what Danny says.

João

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

[Dmitry Gutov]

On 29.10.2022 05:02, João Távora wrote:

> Eglot doesn't want to decide anything about project. Eglot justs wants
> to go into user visible projects and answer the question:
> (project-current).  It wants this because it maps project/major-mode
> pairs to server connections.
> 
> Eglot doesn't care if the project is of type 'vc, 'transient,
> 'visual-studio-solution-file, 'joes-complicated-project, etc.
> 
> So Eglot providing a project backend doesn't make sense.  Maybe you
> think I'm suggesting that Eglot that could collect these references to
> jars coming from the LSP server and add them to project-external-roots
> somehow.  I'm not suggesting that.

That was my impression, yes.

> It's just that an arbitrary project backend, other than 'vc or
> 'transient, could add a method to project-external-roots.  That would be
> the user's job.  I suppose Clojure packages declare somewhere which jars
> they use.  They probably store this information in a file.  Java used
> some ghastly .xml Ant file or Maven or whatever.  A specialized project
> backend could read the file and use it in an implementation of
> project-external-roots.  At this this is how I interpret project.el's
> CLOS-like protocol for defining new project backends.

Okay: some new project type. Or a new feature that parses build files. 
Or etc. All of that could be reasonable, but is yet for somebody to 
design and implement.

IMHO a feature that takes up the goal of providing comprehensive IDE 
support could take up that responsibility too. But I'm fine either way.

That would also depend on whether the LSP protocol is ever going to be 
extended toward providing build file information, running build tasks, etc.

>>> Please read the summary of the outlined above.  Maybe there's nothing to
>>> be done in project.el if eglot-extend-to-xref is to be used.
>>
>> ...indeed you could stop at that.
> 
> Maybe.  eglot-extend-to-xref works very well for non-jars, at least for
> C++ and clangd.  Subsequent M-. work very well, too.
> 
> The downside is that once a system file discovered by the LSP server, it
> is associated with a given server (_not project_) in Eglot.  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 dilemma of having external files associated with just the latest 
server seems unavoidable.

Of course you could collect the full list of servers which the external 
file "belongs to", and then flat_map the requests through them all. But 
it's probably not what the user expects most of the time.

>> Having the jars in project-external-roots could enable the users
>> (after certain integration work) to search across their contents with
>> project-or-external-find-regexp, or jump to files inside with
>> project-or-external-find-file.
> 
> That's a very nice point.  I don't use Java fortunately, but when I did
> a long time ago, I think I remember Eclipse let me do this.

Maybe it's covered by existing LSP functionality, though.

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

[Felician Nemeth]

> The problem occurs when using `xref-find-defintions` while eglot is
> managing a clojure buffer with clojure-lsp. If the symbol that
> xref-find-definitions is activated on is defined in a jar file,
> clojure-lsp by default will provide a location with a response like
> the following
>
> (:jsonrpc "2.0" :id 14 :result
>   (:uri
>   "zipfile:///Users/andreperictavares/.m2/repository/org/clojure/tools.namespace/0.3.1/tools.namespace-0.3.1.jar::clojure/tools/namespace/find.clj"
> ... ))
>

Can you, please, consider merging something like github#854?
https://github.com/joaotavora/eglot/pull/854

Currently, Eglot assumes that the uri-scheme is always "file".  I think
Eglot should at least warn the user if this assumption is false instead
of silently failing as it currently does.

Thanks.

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

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]

Yes, i think that makes sense. Can't look at that pull request right now,
but my current thinking is to leave the URI unprocessed if the scheme is
anything other than file://.

Then file-name-handler-alist can decide what to do with it.

But warning the user on such occasions sounds a bit too noisy. Maybe a
one-time warning.

The only other thing to watch out for is that wherever find-file lands is,
its buffer-file-name should probably be left with the unchanged URI, so
that eglot--path-to-uri can return it and tell the LSP server that we're
now managing the buffer using the original URI.

João

On Sat, Oct 29, 2022, 16:36 Felician Nemeth <felician.nemeth@gmail.com>
wrote:

> > The problem occurs when using `xref-find-defintions` while eglot is
> > managing a clojure buffer with clojure-lsp. If the symbol that
> > xref-find-definitions is activated on is defined in a jar file,
> > clojure-lsp by default will provide a location with a response like
> > the following
> >
> > (:jsonrpc "2.0" :id 14 :result
> >   (:uri
> >
>  "zipfile:///Users/andreperictavares/.m2/repository/org/clojure/tools.namespace/0.3.1/tools.namespace-0.3.1.jar::clojure/tools/namespace/find.clj"
> > ... ))
> >
>
> Can you, please, consider merging something like github#854?
> https://github.com/joaotavora/eglot/pull/854
>
> Currently, Eglot assumes that the uri-scheme is always "file".  I think
> Eglot should at least warn the user if this assumption is false instead
> of silently failing as it currently does.
>
> Thanks.
>

[-- Attachment #2: Type: text/html, Size: 2238 bytes --]

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

[Danny Freeman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1.1.1: Type: text/plain, Size: 4336 bytes --]

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

I will look into seeing if everything can work where Y === X. What this 
means
is that the X will start with `jar:file://`, and the last time I attempted I
had to re-implement all of the file-name-handler-alist operations (there 
are a
LOT). I am not sure why stripping the URI information off made this not
necessary but it sure makes the implement a lot simpler. I still don't fully
understand why, but that is alright. It's just going to take me some time.

 > 4. B2 can be setup in a way so that project-current returns the same
 >    object which is returned in B.  If this is true,
 >    eglot--current-server discovers the same connection which also
 >    manages B1 and Eglot adds buffer B2 to the list of managed buffers in
 >    that connection.
 >
 >    However, if eglot-extend-to-xref is non-nil, eglot--current-server
 >    should also discover the correct connection.  This is less ideal than
 >    making project-current know that the buffers of source code inside
 >    the jar are also in the same project, but it works.  I can explain
 >    the downsides, but it's not very relevant right now.

Depending on the implementation of my file-name-handler-alist function, I
think my only option is to make project-current return the project of buffer
B. NOT doing this results in `eglot-current-server` returning a transient
project, I believe from `eglot--curent-project`, causing a new server to be
started by eglot. I need to spend some more time with this code as well to
make sure I understand how this is being triggered, but it probably 
comes from
my abuse of the file-name-handler-alist.

 > Maybe Eglot just needs to be changed to _not_ strip the leading
 > "jar:file" and leave it completely unchanged.

 > So the server should be able to sort itself out, as long as you give
 > back the very same URI you got -- from the server itself -- to the
 > in-JAR source code.

I hope this is the case. I guess it depends on the lsp server's
implementation. In the case of clojure-lsp, and probably other jvm 
languages I
suspect that it would automatically understand `jar:` URIs.

...

 > The downside is that once a system file discovered by the LSP server, it
 > is associated with a given server (_not project_) in Eglot.  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.


 > > Having the jars in project-external-roots could enable the users
 > > (after certain integration work) to search across their contents with
 > > project-or-external-find-regexp, or jump to files inside with
 > > project-or-external-find-file.
 >
 > That's a very nice point.  I don't use Java fortunately, but when I did
 > a long time ago, I think I remember Eclipse let me do this.

That might be nice. I think we would have to just extract the jar 
contents at
that point and use that. Not quite sure.

 > Okay: some new project type. Or a new feature that parses build 
files. Or etc. All of that could be reasonable, but is yet for somebody 
to design and implement.
 >
 > IMHO a feature that takes up the goal of providing comprehensive IDE 
support could take up that responsibility too. But I'm fine either way.
 >
 > That would also depend on whether the LSP protocol is ever going to 
be extended toward providing build file information, running build 
tasks, etc.

That I think this goes beyond the scope of what I want to do, and have the
ability to do. This is encroaching into cider's responsibilities I think. It
provides the full IDE expereince, and does so at the heavyweight cost of
running the actual project.

Thank you both for the input. It's given me a _lot_ to think about and
experiment with. It will probably take me some time to work through these
problems considering my day job and baby. Forgive me if it takes some 
time for
me to respond.


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3185 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Danny Freeman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 3644 bytes --]

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

 > 4. B2 can be setup in a way so that project-current returns the same
 >    object which is returned in B.  If this is true,
 >    eglot--current-server discovers the same connection which also
 >    manages B1 and Eglot adds buffer B2 to the list of managed buffers in
 >    that connection.
 >
 >    However, if eglot-extend-to-xref is non-nil, eglot--current-server
 >    should also discover the correct connection.  This is less ideal than
 >    making project-current know that the buffers of source code inside
 >    the jar are also in the same project, but it works.  I can explain
 >    the downsides, but it's not very relevant right now.

My newly updated package does nothing with `project-current`, as I have no
obvious way to access the previous project when extracting files from 
the jar.
(Extracting now happens in `insert-file-contents` instead of 
`get-file-buffer`)

BUT eglot-extend-to-xref works as it should with this, and I think I 
prefer that.
It allows people who don't want that behavior to disable 
`eglot-extend-to-xref`
and have eglot start up a new lsp server when visiting the file.
Idk why anyone would want that but you never know.

 > 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.
I am honestly surprised I didn't have to change anything in clojure-lsp. 
It can
do it's analysis on the file without issue.

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

It does!!!!!

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.


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.

[-- Attachment #2: 0001-Do-not-parse-jar-type-URIs-provided-by-lsp-servers-i.patch --]
[-- Type: text/x-patch, Size: 4931 bytes --]

From 671d3117bd9a810616dd5bbdfffa18e91a4d9896 Mon Sep 17 00:00:00 2001
From: dannyfreeman <danny@dfreeman.email>
Date: Mon, 31 Oct 2022 10:01:21 -0400
Subject: [PATCH] Do not parse jar type URIs provided by lsp servers in eglot

Clojure-lsp and possibly other lsp servers for JVM languages, can
provide file locations as jar scheme URIs for dependencies located
inside of jar archives. These URIs contain nested URIs that Emacs out of
the box does not know how to work with. Example:
  jar:file:///path/to/lib.jar!/path/inside/jar/source.ext

Even if they are parsed twice, the locations are of the format
  /path/to/lib.jar!/path/inside/jar/source.ext
which Emacs also does not know how to handle.

This commit introduces a var `eglot-preserve-jar-uri` that, when true,
will pass along jar URIs without parsing them. This allows other
packages to handle them through the `file-name-handler-alist` mechanism.
Additionally, any jars sent BACK to the lsp server by eglot will also
remain in the `jar` format, which at least clojure-lsp knows how to
interpret.

Given the correct file-name-handler-alist implementation, this change
allows xref to navigate to a file within a jar. When
`eglot-extend-to-xref` option is enabled, eglot can correctly connect to
the previous lsp server and continue to work in the newly opened file.

To ensure clojure-lsp servers always use the "jar" URI scheme for these
types of dependencies, an initialization option is set. The default
option, the "zipfile" scheme, is very similar to the jar
scheme. However, the "zipfile" scheme is something that the clojure-lsp
maintainers want to move away from and eventually remove.
---
 lisp/progmodes/eglot.el | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

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"))
@@ -1492,25 +1492,38 @@ eglot--uri-path-allowed-chars
     vec)
   "Like `url-path-allows-chars' but more restrictive.")
 
+(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.")
+
 (defun eglot--path-to-uri (path)
   "URIfy PATH."
-  (let ((truepath (file-truename path)))
-    (concat "file://"
-            ;; Add a leading "/" for local MS Windows-style paths.
-            (if (and (eq system-type 'windows-nt)
-                     (not (file-remote-p truepath)))
-                "/")
-            (url-hexify-string
-             ;; Again watch out for trampy paths.
-             (directory-file-name (file-local-name truepath))
-             eglot--uri-path-allowed-chars))))
+  (if (and eglot-preserve-jar-uri (equal "jar" (url-type (url-generic-parse-url path))))
+      path
+    (let ((truepath (file-truename path)))
+      (concat "file://"
+              ;; Add a leading "/" for local MS Windows-style paths.
+              (if (and (eq system-type 'windows-nt)
+                       (not (file-remote-p truepath)))
+                  "/")
+              (url-hexify-string
+               ;; Again watch out for trampy paths.
+               (directory-file-name (file-local-name truepath))
+               eglot--uri-path-allowed-chars)))))
+
+(defun eglot--parse-uri (uri)
+  "Try to parse a URI."
+  (let ((url (url-generic-parse-url uri)))
+    (if (and eglot-preserve-jar-uri (string= "jar" (url-type url)))
+        uri
+      (url-unhex-string (url-filename url)))))
 
 (defun eglot--uri-to-path (uri)
   "Convert URI to file path, helped by `eglot--current-server'."
   (when (keywordp uri) (setq uri (substring (symbol-name uri) 1)))
   (let* ((server (eglot-current-server))
          (remote-prefix (and server (eglot--trampish-p server)))
-         (retval (url-unhex-string (url-filename (url-generic-parse-url uri))))
+         (retval (eglot--parse-uri uri))
          ;; Remove the leading "/" for local MS Windows-style paths.
          (normalized (if (and (not remote-prefix)
                               (eq system-type 'windows-nt)
-- 
2.37.3

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

[Juri Linkov]

>> Please read the summary of the outlined above.  Maybe there's nothing to
>> be done in project.el if eglot-extend-to-xref is to be used.
>
> Having the jars in project-external-roots could enable the users (after
> certain integration work) to search across their contents with
> project-or-external-find-regexp, or jump to files inside with
> project-or-external-find-file.

FWIW, here's a possible way how to get everything to work,
so that 'C-x p G' works either when a file is visited
in a project dir or in a lib dir, and eglot is connected
to the project server by visiting any file in the lib dir
(that eglot-extend-to-xref can't do):

```
(defvar my-project-dir "...")
(defvar my-lib-dir "...")
(add-hook 'prog-mode-hook
          (lambda ()
            (when (string-match-p my-project-dir default-directory)
              (setq-local project-vc-external-roots-function
                          (lambda () (list my-lib-dir)))
              (eglot-ensure))))

(defun my-project-try-lib (dir)
  (when (string-match-p my-lib-dir dir)
    (if (bound-and-true-p eglot-lsp-context)
        ;; for eglot.el
        `(vc Git ,my-project-dir)
      ;; for project.el
      `(transient . ,my-lib-dir))))

(with-eval-after-load 'project
  (add-to-list 'project-find-functions 'my-project-try-lib))
```

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

[João Távora]

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

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

[Danny Freeman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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.

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

[Danny Freeman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 1247 bytes --]


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

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

I've attached patch
0001-Only-handle-file-type-URIs-in-eglot-explicitly.patch
to this email that does this pretty well

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

This is done by just trying to parse the path as a URL and checking the
url-type instead of checking a regex.

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

This change is only a matter of consulting the `find-file-name-handler`
function. I added another patch for that as
0002-Warn-when-eglot-receives-a-non-file-type-URI-that-Em.patch


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Only-handle-file-type-URIs-in-eglot-explicitly.patch --]
[-- Type: text/x-patch, Size: 3982 bytes --]

From 936034d0e72621815584680a9e75f44a4448ba9d Mon Sep 17 00:00:00 2001
From: dannyfreeman <danny@dfreeman.email>
Date: Thu, 3 Nov 2022 09:39:16 -0400
Subject: [PATCH 1/2] Only handle file:// type URIs in eglot explicitly

(Bug#58790)

This issue originated with clojure-lsp sending clients "jar": type URIs
that emacs is unable to handle out of the box. Before this change, jar:
URIs were parsed once, but since jar: URIs contain a nested URI,
this resulted in a file being dispatched with a partially parsed path
that looked like `file://path/to.jar!/path/in/jar`.

Now eglot will not attempt to parse URIs that are not file:// type at
all, instead let file-name-handler-alist entries to deal with them.
Not parsing them at all allows the file-name-handler-alist regexps to
identify them more accurately.

By also checking if eglot received a URI in eglot--path-to-uri, the
file-name-handler-alist can provide the non-file type URI back to the
lsp server, which presumably will know how to handle them since it is
also giving them out to clients.
---
 lisp/progmodes/eglot.el | 42 ++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 204121045a..b272d370ab 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1496,29 +1496,37 @@ eglot--uri-path-allowed-chars
 (defun eglot--path-to-uri (path)
   "URIfy PATH."
   (let ((truepath (file-truename path)))
-    (concat "file://"
-            ;; Add a leading "/" for local MS Windows-style paths.
-            (if (and (eq system-type 'windows-nt)
-                     (not (file-remote-p truepath)))
-                "/")
-            (url-hexify-string
-             ;; Again watch out for trampy paths.
-             (directory-file-name (file-local-name truepath))
-             eglot--uri-path-allowed-chars))))
+    (if (url-type (url-generic-parse-url truepath))
+        ;; Path is already a URI, forward it to the lsp server untouched
+        truepath
+      (concat "file://"
+              ;; Add a leading "/" for local MS Windows-style paths.
+              (if (and (eq system-type 'windows-nt)
+                       (not (file-remote-p truepath)))
+                  "/")
+              (url-hexify-string
+               ;; Again watch out for trampy paths.
+               (directory-file-name (file-local-name truepath))
+               eglot--uri-path-allowed-chars)))))
 
 (defun eglot--uri-to-path (uri)
   "Convert URI to file path, helped by `eglot--current-server'."
   (when (keywordp uri) (setq uri (substring (symbol-name uri) 1)))
   (let* ((server (eglot-current-server))
          (remote-prefix (and server (eglot--trampish-p server)))
-         (retval (url-unhex-string (url-filename (url-generic-parse-url uri))))
-         ;; Remove the leading "/" for local MS Windows-style paths.
-         (normalized (if (and (not remote-prefix)
-                              (eq system-type 'windows-nt)
-                              (cl-plusp (length retval)))
-                         (substring retval 1)
-                       retval)))
-    (concat remote-prefix normalized)))
+         (url (url-generic-parse-url uri)))
+    (if (string= "file" (url-type url))
+        (let* ((retval (url-unhex-string (url-filename url)))
+               ;; Remove the leading "/" for local MS Windows-style paths.
+               (normalized (if (and (not remote-prefix)
+                                    (eq system-type 'windows-nt)
+                                    (cl-plusp (length retval)))
+                               (substring retval 1)
+                             retval)))
+          (concat remote-prefix normalized))
+      ;; Leave non-file type URIs untouched, `file-name-handler-alist'
+      ;; handlers can be used to dispatch them properly.
+      uri)))
 
 (defun eglot--snippet-expansion-fn ()
   "Compute a function to expand snippets.
-- 
2.38.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Warn-when-eglot-receives-a-non-file-type-URI-that-Em.patch --]
[-- Type: text/x-patch, Size: 1258 bytes --]

From 4c5dbc458ca1b13a897b6f01f69cacd9490c1ee1 Mon Sep 17 00:00:00 2001
From: dannyfreeman <danny@dfreeman.email>
Date: Thu, 3 Nov 2022 09:57:45 -0400
Subject: [PATCH 2/2] Warn when eglot receives a non-file type URI that Emacs
 can't handle

(bug#58790)

The file-name-operation being checked, any-handler, has no significant
meaning, other than that it is not one that would be suppressed by
inhibit-file-name-operation. We just want to check that a handler exists
and has the potential to handle this URI, not actually dispatch a
file-name-operation right now.
---
 lisp/progmodes/eglot.el | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index b272d370ab..f0969290d0 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1524,6 +1524,8 @@ eglot--uri-to-path
                                (substring retval 1)
                              retval)))
           (concat remote-prefix normalized))
+      (unless (find-file-name-handler uri 'any-handler)
+        (eglot--message "Received URI with unexpected scheme: %s" uri))
       ;; Leave non-file type URIs untouched, `file-name-handler-alist'
       ;; handlers can be used to dispatch them properly.
       uri)))
-- 
2.38.0


[-- Attachment #4: Type: text/plain, Size: 19 bytes --]


-- 
Danny Freeman

bug#58790:

[Robert Brown]

[-- Attachment #1: Type: text/plain, Size: 371 bytes --]

I use a Java language server with Eglot that returns "jar:" URIs like
those discussed in this bug thread.
I took Danny's jarchive.el and combined it with the attached patch,
which modifies Eglot so that it does not convert non-file URIs into
file paths.
With this patch and jarchive.el, "goto definition" works as expected
when the Java source code is inside a jar file.

[-- Attachment #2: 0001-Do-not-convert-non-file-URIs-into-file-paths.patch --]
[-- Type: text/x-patch, Size: 3670 bytes --]

From 7a5e9959ca9c1989f4611199710d95a84ac9d26b Mon Sep 17 00:00:00 2001
From: Robert Brown <robert.brown@gmail.com>
Date: Tue, 8 Nov 2022 19:57:50 -0500
Subject: [PATCH] Do not convert non-file URIs into file paths.

---
 eglot.el | 57 ++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 22 deletions(-)

diff --git a/eglot.el b/eglot.el
index 901bf30..83b47c6 100644
--- a/eglot.el
+++ b/eglot.el
@@ -1483,31 +1483,44 @@ If optional MARKER, return a marker instead"
   "Like `url-path-allows-chars' but more restrictive.")
 
 (defun eglot--path-to-uri (path)
-  "URIfy PATH."
-  (let ((truepath (file-truename path)))
-    (concat "file://"
-            ;; Add a leading "/" for local MS Windows-style paths.
-            (if (and (eq system-type 'windows-nt)
-                     (not (file-remote-p truepath)))
-                "/")
-            (url-hexify-string
-             ;; Again watch out for trampy paths.
-             (directory-file-name (file-local-name truepath))
-             eglot--uri-path-allowed-chars))))
+  "When PATH is a file path derived from a file URI, convert it into a URI.
+Otherwise, PATH is already a URI, so just return it."
+  (let ((url (url-generic-parse-url path)))
+    (if (null (url-type url))
+        ;; PATH is a file path.  Convert it into a URI.
+        (let ((truepath (file-truename path)))
+          (concat "file://"
+                  ;; Add a leading "/" for local MS Windows-style paths.
+                  (if (and (eq system-type 'windows-nt)
+                           (not (file-remote-p truepath)))
+                      "/")
+                  (url-hexify-string
+                   ;; Again watch out for trampy paths.
+                   (directory-file-name (file-local-name truepath))
+                   eglot--uri-path-allowed-chars)))
+      ;; PATH is already a URI.
+      path)))
 
 (defun eglot--uri-to-path (uri)
-  "Convert URI to file path, helped by `eglot--current-server'."
+  "When URI is a file URI, convert it into a file path, helped by
+`eglot--current-server'.  Otherwise, just return the URI unchanged."
   (when (keywordp uri) (setq uri (substring (symbol-name uri) 1)))
-  (let* ((server (eglot-current-server))
-         (remote-prefix (and server (eglot--trampish-p server)))
-         (retval (url-unhex-string (url-filename (url-generic-parse-url uri))))
-         ;; Remove the leading "/" for local MS Windows-style paths.
-         (normalized (if (and (not remote-prefix)
-                              (eq system-type 'windows-nt)
-                              (cl-plusp (length retval)))
-                         (substring retval 1)
-                       retval)))
-    (concat remote-prefix normalized)))
+  (let ((url (url-generic-parse-url uri)))
+    (if (equal (url-type url) "file")
+        ;; Transform a file URI into a file path, which may be a
+        ;; remote tramp path.
+        (let* ((server (eglot-current-server))
+               (remote-prefix (and server (eglot--trampish-p server)))
+               (retval (url-unhex-string (url-filename url)))
+               ;; Remove the leading "/" for local MS Windows-style paths.
+               (normalized (if (and (not remote-prefix)
+                                    (eq system-type 'windows-nt)
+                                    (cl-plusp (length retval)))
+                               (substring retval 1)
+                             retval)))
+          (concat remote-prefix normalized))
+      ;; Non-file URIs are unmodifiled.
+      uri)))
 
 (defun eglot--snippet-expansion-fn ()
   "Compute a function to expand snippets.
-- 
2.25.1

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

[Eli Zaretskii]

Ping!  João, any comments on this patch?

> Cc: 58790@debbugs.gnu.org, felician.nemeth@gmail.com,
>  Stefan Kangas <stefankangas@gmail.com>, Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 03 Nov 2022 13:10:32 -0400
> From:  Danny Freeman via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> João Távora <joaotavora@gmail.com> writes:
> 
> > 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.
> 
> I've attached patch
> 0001-Only-handle-file-type-URIs-in-eglot-explicitly.patch
> to this email that does this pretty well
> 
> > 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.
> 
> This is done by just trying to parse the path as a URL and checking the
> url-type instead of checking a regex.
> 
> > 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.
> 
> This change is only a matter of consulting the `find-file-name-handler`
> function. I added another patch for that as
> 0002-Warn-when-eglot-receives-a-non-file-type-URI-that-Em.patch

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

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

> Ping!  João, any comments on this patch?

Hi Eli, thanks for pinging.  Yes, had a look and _both_ patches look
fine.

The first patch touches a sensitive part.  I hoped at this point to have
Eglot's automated test running in Emacs's repo, but I don't.  So I trust
Danny has given some minimal testing to this, hopefully also with some
other server, for good measure.

The second patch is a little riskier because it has the potential to
cause much warning noise, but I'd wait and see what it does in the wild.
If it does happen I think we have some means of toning down warnings
without making package's warning logic too complex.

So again, both patches should be pushed.  I'm in the middle of a
bisection here, so if you can commit and push the patches, I'd be
thankful.  Else, I will attend to that later.

João

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

[Danny Freeman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> Ping!  João, any comments on this patch?
>
> Hi Eli, thanks for pinging.  Yes, had a look and _both_ patches look
> fine.
>
> The first patch touches a sensitive part.  I hoped at this point to have
> Eglot's automated test running in Emacs's repo, but I don't.  So I trust
> Danny has given some minimal testing to this, hopefully also with some
> other server, for good measure.

If it gives you some piece of mind, I've been running this code for a
while in some large clojure, clojurescript, and typescript projects,
all without issue. 

I also ran through and did a quick test on a zig project (via zls) and
in the emacs C source (via clangd) this morning. I've not noticed any
issues there either, but also don't know much about either language. 

> The second patch is a little riskier because it has the potential to
> cause much warning noise, but I'd wait and see what it does in the wild.
> If it does happen I think we have some means of toning down warnings
> without making package's warning logic too complex.

I will keep an eye out in this mailing list and try to help resolve any
problems that might come up because of it.

> So again, both patches should be pushed.  I'm in the middle of a
> bisection here, so if you can commit and push the patches, I'd be
> thankful.  Else, I will attend to that later.
>
> João

Thank you!!
-- 
Danny Freeman

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

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: Danny Freeman <danny@dfreeman.email>,  58790@debbugs.gnu.org,
>   felician.nemeth@gmail.com,  stefankangas@gmail.com,  dgutov@yandex.ru
> Date: Thu, 10 Nov 2022 11:00:18 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Ping!  João, any comments on this patch?
> 
> Hi Eli, thanks for pinging.  Yes, had a look and _both_ patches look
> fine.

OK, then I have a few minor comments, before this can be installed:

> >From 936034d0e72621815584680a9e75f44a4448ba9d Mon Sep 17 00:00:00 2001
> From: dannyfreeman <danny@dfreeman.email>
> Date: Thu, 3 Nov 2022 09:39:16 -0400
> Subject: [PATCH 1/2] Only handle file:// type URIs in eglot explicitly
> 
> (Bug#58790)
> 
> This issue originated with clojure-lsp sending clients "jar": type URIs
> that emacs is unable to handle out of the box. Before this change, jar:
> URIs were parsed once, but since jar: URIs contain a nested URI,
> this resulted in a file being dispatched with a partially parsed path
> that looked like `file://path/to.jar!/path/in/jar`.
> 
> Now eglot will not attempt to parse URIs that are not file:// type at
> all, instead let file-name-handler-alist entries to deal with them.
> Not parsing them at all allows the file-name-handler-alist regexps to
> identify them more accurately.
> 
> By also checking if eglot received a URI in eglot--path-to-uri, the
> file-name-handler-alist can provide the non-file type URI back to the
> lsp server, which presumably will know how to handle them since it is
> also giving them out to clients.

This lacks ChangeLog-style parts which specify the file(s) and
function(s) which were changed.

Also, in the text above, please leave two spaces between sentences,
per our conventions, and refill the text to be at most 63 columns.

Finally, please consider moving some of the text above to comments
explaining why the code does what it does.

> >From 4c5dbc458ca1b13a897b6f01f69cacd9490c1ee1 Mon Sep 17 00:00:00 2001
> From: dannyfreeman <danny@dfreeman.email>
> Date: Thu, 3 Nov 2022 09:57:45 -0400
> Subject: [PATCH 2/2] Warn when eglot receives a non-file type URI that Emacs
>  can't handle
> 
> (bug#58790)
> 
> The file-name-operation being checked, any-handler, has no significant
> meaning, other than that it is not one that would be suppressed by
> inhibit-file-name-operation. We just want to check that a handler exists
> and has the potential to handle this URI, not actually dispatch a
> file-name-operation right now.

Same here.  And in this case, the text should definitely be in
comments.

Thanks.

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

[Danny Freeman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 1029 bytes --]


Eli Zaretskii <eliz@gnu.org> writes:

>
> OK, then I have a few minor comments, before this can be installed:
>
> This lacks ChangeLog-style parts which specify the file(s) and
> function(s) which were changed.

Thanks for your patience, I believe I've got that corrected now. See the
attachments.

> Also, in the text above, please leave two spaces between sentences,
> per our conventions, and refill the text to be at most 63 columns.

I cut it down to 63. I don't mind either way, but thought I would
mention that the CONTRIBUTE file mentions 79 columns, which is what I
normally default to in commit messages.

>> The file-name-operation being checked, any-handler, has no significant
>> meaning, other than that it is not one that would be suppressed by
>> inhibit-file-name-operation. We just want to check that a handler exists
>> and has the potential to handle this URI, not actually dispatch a
>> file-name-operation right now.
>
> Same here.  And in this case, the text should definitely be in
> comments.

Done!!!


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Only parse file: URIs --]
[-- Type: text/x-patch, Size: 3849 bytes --]

From a5b1b6b34d3d455e06757bbb1c10df8f81121f1f Mon Sep 17 00:00:00 2001
From: dannyfreeman <danny@dfreeman.email>
Date: Thu, 3 Nov 2022 09:39:16 -0400
Subject: [PATCH] Eglot: Only handle URIs with the file:// scheme

This fixes Bug#58790

* lisp/progmodes/eglot.el (eglot--path-to-uri, eglot--uri-to-path):
This issue originated with clojure-lsp sending clients "jar"
type URIs that Emacs is unable to handle out of the box.  Before
this change, jar: URIs were parsed once, but since jar: URIs
contain a nested URI, this resulted in a file being dispatched
with a partially parsed path that looked like
`file://path/to.jar!/path/in/jar`.  Now eglot will not attempt
to parse them at all.
---
 lisp/progmodes/eglot.el | 46 ++++++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 17 deletions(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index b707a6b059..11bf0884ac 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1500,29 +1500,41 @@ eglot--uri-path-allowed-chars
 (defun eglot--path-to-uri (path)
   "URIfy PATH."
   (let ((truepath (file-truename path)))
-    (concat "file://"
-            ;; Add a leading "/" for local MS Windows-style paths.
-            (if (and (eq system-type 'windows-nt)
-                     (not (file-remote-p truepath)))
-                "/")
-            (url-hexify-string
-             ;; Again watch out for trampy paths.
-             (directory-file-name (file-local-name truepath))
-             eglot--uri-path-allowed-chars))))
+    (if (url-type (url-generic-parse-url truepath))
+        ;; Path is already a URI, so forward it to the lsp server untouched.
+        ;; This assumes that the server can handle the same non-file
+        ;; scheme URIs that it provides to clients.
+        truepath
+      (concat "file://"
+              ;; Add a leading "/" for local MS Windows-style paths.
+              (if (and (eq system-type 'windows-nt)
+                       (not (file-remote-p truepath)))
+                  "/")
+              (url-hexify-string
+               ;; Again watch out for trampy paths.
+               (directory-file-name (file-local-name truepath))
+               eglot--uri-path-allowed-chars)))))
 
 (defun eglot--uri-to-path (uri)
   "Convert URI to file path, helped by `eglot--current-server'."
   (when (keywordp uri) (setq uri (substring (symbol-name uri) 1)))
   (let* ((server (eglot-current-server))
          (remote-prefix (and server (eglot--trampish-p server)))
-         (retval (url-unhex-string (url-filename (url-generic-parse-url uri))))
-         ;; Remove the leading "/" for local MS Windows-style paths.
-         (normalized (if (and (not remote-prefix)
-                              (eq system-type 'windows-nt)
-                              (cl-plusp (length retval)))
-                         (substring retval 1)
-                       retval)))
-    (concat remote-prefix normalized)))
+         (url (url-generic-parse-url uri)))
+    ;; Only attempt to parse URIs with the file scheme.
+    (if (string= "file" (url-type url))
+        (let* ((retval (url-unhex-string (url-filename url)))
+               ;; Remove the leading "/" for local MS Windows-style paths.
+               (normalized (if (and (not remote-prefix)
+                                    (eq system-type 'windows-nt)
+                                    (cl-plusp (length retval)))
+                               (substring retval 1)
+                             retval)))
+          (concat remote-prefix normalized))
+      ;; Leave non-file scheme URIs untouched.
+      ;; Handlers registered with `file-name-handler-alist' can
+      ;; identify these URIs and decide how to handle them.
+      uri)))
 
 (defun eglot--snippet-expansion-fn ()
   "Compute a function to expand snippets.
-- 
2.38.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Warn when unhandled file: URI is received --]
[-- Type: text/x-patch, Size: 1440 bytes --]

From 7fd1e7c4a264d74360d79c6cbee3f0e0aceeab5c Mon Sep 17 00:00:00 2001
From: dannyfreeman <danny@dfreeman.email>
Date: Thu, 10 Nov 2022 13:05:49 -0500
Subject: [PATCH] Eglot: warn when receiving a non-file scheme URI

Addresses bug#58790

* lisp/progmodes/eglot.el (eglot--uri-to-path): Write a message
when eglot encounters a non-file scheme URI that Emacs is not
capable of handling.
---
 lisp/progmodes/eglot.el | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 11bf0884ac..dfa18b885f 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1531,6 +1531,13 @@ eglot--uri-to-path
                                (substring retval 1)
                              retval)))
           (concat remote-prefix normalized))
+      ;; `any-handler', has no significant meaning, other than that it
+      ;; is not one that would be suppressed by `inhibit-file-name-operation'.
+      ;; We just want to check that a handler exists and has the potential
+      ;; to handle this URI, not actually dispatch a
+      ;; file-name-operation right now.
+      (unless (find-file-name-handler uri 'any-handler)
+        (eglot--message "Received URI with unexpected scheme: %s" uri))
       ;; Leave non-file scheme URIs untouched.
       ;; Handlers registered with `file-name-handler-alist' can
       ;; identify these URIs and decide how to handle them.
-- 
2.38.1


[-- Attachment #4: Type: text/plain, Size: 31 bytes --]



Thank you,
-- 
Danny Freeman

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

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 1620 bytes --]

I agree on the two comments about the ChangeLog style entries and the
spacing.

But when dealing with eglot.el, which I am the prime maintainer of,
I really prefer to see design described in the commit message where the
design was introduced. It plays very well with vc-region-history.

So Danny, if you don't mind, please revert that part.

João

On Thu, Nov 10, 2022 at 9:54 PM Danny Freeman <danny@dfreeman.email> wrote:

>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> >
> > OK, then I have a few minor comments, before this can be installed:
> >
> > This lacks ChangeLog-style parts which specify the file(s) and
> > function(s) which were changed.
>
> Thanks for your patience, I believe I've got that corrected now. See the
> attachments.
>
> > Also, in the text above, please leave two spaces between sentences,
> > per our conventions, and refill the text to be at most 63 columns.
>
> I cut it down to 63. I don't mind either way, but thought I would
> mention that the CONTRIBUTE file mentions 79 columns, which is what I
> normally default to in commit messages.
>
> >> The file-name-operation being checked, any-handler, has no significant
> >> meaning, other than that it is not one that would be suppressed by
> >> inhibit-file-name-operation. We just want to check that a handler exists
> >> and has the potential to handle this URI, not actually dispatch a
> >> file-name-operation right now.
> >
> > Same here.  And in this case, the text should definitely be in
> > comments.
>
> Done!!!
>
>
>
> Thank you,
> --
> Danny Freeman
>


-- 
João Távora

[-- Attachment #2: Type: text/html, Size: 2258 bytes --]

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

[Danny Freeman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 792 bytes --]


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

> I agree on the two comments about the ChangeLog style entries and the
> spacing.  
>
> But when dealing with eglot.el, which I am the prime maintainer of, 
> I really prefer to see design described in the commit message where the 
> design was introduced. It plays very well with vc-region-history.
>
> So Danny, if you don't mind, please revert that part.  
>
> João

Yall all really taking me for a ride on this lol.

In reviewing the comments over and over I've made a modification to the
second patch involving the warning message. Instead of using an
arbitrary symbol I realized that the function `find-file-name-handler'
works when a nil value for the second argument (operation).

Anyways, here they are again.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Eglot-Only-handle-URIs-with-the-file-scheme.patch --]
[-- Type: text/x-patch, Size: 4124 bytes --]

From e56e987cbf601f6caffac7edfdd580a0b7da921f Mon Sep 17 00:00:00 2001
From: dannyfreeman <danny@dfreeman.email>
Date: Thu, 3 Nov 2022 09:39:16 -0400
Subject: [PATCH 1/2] Eglot: Only handle URIs with the file:// scheme

This fixes Bug#58790

* lisp/progmodes/eglot.el (eglot--path-to-uri,
eglot--uri-to-path):

Eglot will not attempt to parse URIs that are not file:// type
at all, instead let `file-name-handler-alist' entries to deal
with them.  Not parsing them at all allows the
`file-name-handler-alist' regexps to identify them more
accurately.

By also checking if eglot received a URI in
`eglot--path-to-uri', the `file-name-handler-alist' can provide
the non-file type URI back to the lsp server, which presumably
will know how to handle them since it is also giving them out
to clients.

This issue originated with clojure-lsp sending clients "jar":
type URIs that emacs is unable to handle out of the box.
Before this change, jar: URIs were parsed once, but since jar:
URIs contain a nested URI, this resulted in a file being
dispatched with a partially parsed path that looked like
"file://path/to.jar!/path/in/jar".
---
 lisp/progmodes/eglot.el | 43 +++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 1b1302d689..2463f68f97 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1501,29 +1501,38 @@ eglot--uri-path-allowed-chars
 (defun eglot--path-to-uri (path)
   "URIfy PATH."
   (let ((truepath (file-truename path)))
-    (concat "file://"
-            ;; Add a leading "/" for local MS Windows-style paths.
-            (if (and (eq system-type 'windows-nt)
-                     (not (file-remote-p truepath)))
-                "/")
-            (url-hexify-string
-             ;; Again watch out for trampy paths.
-             (directory-file-name (file-local-name truepath))
-             eglot--uri-path-allowed-chars))))
+    (if (url-type (url-generic-parse-url truepath))
+        ;; Path is already a URI, so forward it to the lsp server untouched.
+        truepath
+      (concat "file://"
+              ;; Add a leading "/" for local MS Windows-style paths.
+              (if (and (eq system-type 'windows-nt)
+                       (not (file-remote-p truepath)))
+                  "/")
+              (url-hexify-string
+               ;; Again watch out for trampy paths.
+               (directory-file-name (file-local-name truepath))
+               eglot--uri-path-allowed-chars)))))
 
 (defun eglot--uri-to-path (uri)
   "Convert URI to file path, helped by `eglot--current-server'."
   (when (keywordp uri) (setq uri (substring (symbol-name uri) 1)))
   (let* ((server (eglot-current-server))
          (remote-prefix (and server (eglot--trampish-p server)))
-         (retval (url-unhex-string (url-filename (url-generic-parse-url uri))))
-         ;; Remove the leading "/" for local MS Windows-style paths.
-         (normalized (if (and (not remote-prefix)
-                              (eq system-type 'windows-nt)
-                              (cl-plusp (length retval)))
-                         (substring retval 1)
-                       retval)))
-    (concat remote-prefix normalized)))
+         (url (url-generic-parse-url uri)))
+    ;; Only attempt to parse URIs with the file scheme.
+    (if (string= "file" (url-type url))
+        (let* ((retval (url-unhex-string (url-filename url)))
+               ;; Remove the leading "/" for local MS Windows-style paths.
+               (normalized (if (and (not remote-prefix)
+                                    (eq system-type 'windows-nt)
+                                    (cl-plusp (length retval)))
+                               (substring retval 1)
+                             retval)))
+          (concat remote-prefix normalized))
+      ;; Leave non-file type URIs untouched, `file-name-handler-alist'
+      ;; handlers can be used to dispatch them properly.
+      uri)))
 
 (defun eglot--snippet-expansion-fn ()
   "Compute a function to expand snippets.
-- 
2.38.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Eglot-warn-when-receiving-a-non-file-scheme-URI.patch --]
[-- Type: text/x-patch, Size: 1375 bytes --]

From a2b92859d0a327c843bcd677eede72c0c100821f Mon Sep 17 00:00:00 2001
From: dannyfreeman <danny@dfreeman.email>
Date: Thu, 10 Nov 2022 13:05:49 -0500
Subject: [PATCH 2/2] Eglot: warn when receiving a non-file scheme URI

* lisp/progmodes/eglot.el (eglot--uri-to-path): Write a message
when eglot encounters a non-file scheme URI that Emacs is not
capable of handling.

Note that the file-name-handler operation checked is `nil'
because we don't want to use a real one that may be suppressed
by `inhibit-file-name-operation'.  We just want to check that a
handler exists and has the potential to handle this URI, not
actually dispatch a file-name-operation right now.

Addresses bug#58790
---
 lisp/progmodes/eglot.el | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 2463f68f97..1850334b84 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1530,6 +1530,8 @@ eglot--uri-to-path
                                (substring retval 1)
                              retval)))
           (concat remote-prefix normalized))
+      (unless (find-file-name-handler uri nil)
+        (eglot--message "Received URI with unexpected scheme: %s" uri))
       ;; Leave non-file type URIs untouched, `file-name-handler-alist'
       ;; handlers can be used to dispatch them properly.
       uri)))
-- 
2.38.1


[-- Attachment #4: Type: text/plain, Size: 19 bytes --]


-- 
Danny Freeman

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

[João Távora]

Danny Freeman <danny@dfreeman.email> writes:

> João Távora <joaotavora@gmail.com> writes:
>
>> I agree on the two comments about the ChangeLog style entries and the
>> spacing.  
>>
>> But when dealing with eglot.el, which I am the prime maintainer of, 
>> I really prefer to see design described in the commit message where the 
>> design was introduced. It plays very well with vc-region-history.
>>
>> So Danny, if you don't mind, please revert that part.  
>>
>> João
>
> Yall all really taking me for a ride on this lol.

Sorry.  The Emacs commit message format is a bit of work in the
beginning but it pays off in the end.  I will make minor adjustment to
your patches and push them now.

> In reviewing the comments over and over I've made a modification to the
> second patch involving the warning message. Instead of using an
> arbitrary symbol I realized that the function `find-file-name-handler'
> works when a nil value for the second argument (operation).

OK, I will look at this part, too.

João

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

[João Távora]

Danny Freeman <danny@dfreeman.email> writes:

> In reviewing the comments over and over I've made a modification to
> the second patch involving the warning message. Instead of using an
> arbitrary symbol I realized that the function `find-file-name-handler'
> works when a nil value for the second argument (operation).

I've now pushed the first patch, but I held off the second patch.

I don't see this OPERATION=nil option documented in the docstring of
find-file-name-handler, so I'm not sure we should rely on it.

I think we should provide OPERATION here, because what we want to check
is if we, Emacs, can actually visit the file designated by the URI.  So
I think passing something like 'access-file' as the OPERATION argument
makes more sense.  But Eli probably has more knowledge here.  Maybe
passing nil is indeed correct.

João

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

[Danny Freeman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

>
> Sorry.  The Emacs commit message format is a bit of work in the
> beginning but it pays off in the end.  I will make minor adjustment to
> your patches and push them now.

Its really no problem. I appreciate that so much thought is put into the
commit messages. Most repositories I deal with for work don't enforce
standards for commits at all, so this is a breath of fresh air!

> OK, I will look at this part, too.

Thanks again!

-- 
Danny Freeman

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

[Danny Freeman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

> I've now pushed the first patch, but I held off the second patch.
>
> I don't see this OPERATION=nil option documented in the docstring of
> find-file-name-handler, so I'm not sure we should rely on it.
>
> I think we should provide OPERATION here, because what we want to check
> is if we, Emacs, can actually visit the file designated by the URI.  So
> I think passing something like 'access-file' as the OPERATION argument
> makes more sense.  But Eli probably has more knowledge here.  Maybe
> passing nil is indeed correct.

If we want to not use nil I think a real operation argument like
`access-file' or `expand-file-name' would be more warranted then.
The made up one I initially used I was not a big fan of.

-- 
Danny Freeman

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

[Eli Zaretskii]

> From: Danny Freeman <danny@dfreeman.email>
> Cc: João Távora <joaotavora@gmail.com>,
>  58790@debbugs.gnu.org,
>  felician.nemeth@gmail.com, stefankangas@gmail.com, dgutov@yandex.ru
> Date: Thu, 10 Nov 2022 16:45:27 -0500
> 
> > Also, in the text above, please leave two spaces between sentences,
> > per our conventions, and refill the text to be at most 63 columns.
> 
> I cut it down to 63. I don't mind either way, but thought I would
> mention that the CONTRIBUTE file mentions 79 columns, which is what I
> normally default to in commit messages.

CONTRIBUTE describes the hard limit enforced by our Git commit hooks,
not the preferences for entries that will look good in the ChangeLog
files we produce from the Git logs.

I clarified this issue in CONTRIBUTE, thanks for pointing this out.

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

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  58790@debbugs.gnu.org,
>   felician.nemeth@gmail.com,  stefankangas@gmail.com,  dgutov@yandex.ru
> Date: Thu, 10 Nov 2022 22:48:20 +0000
> 
> Danny Freeman <danny@dfreeman.email> writes:
> 
> > In reviewing the comments over and over I've made a modification to
> > the second patch involving the warning message. Instead of using an
> > arbitrary symbol I realized that the function `find-file-name-handler'
> > works when a nil value for the second argument (operation).
> 
> I've now pushed the first patch, but I held off the second patch.
> 
> I don't see this OPERATION=nil option documented in the docstring of
> find-file-name-handler, so I'm not sure we should rely on it.
> 
> I think we should provide OPERATION here, because what we want to check
> is if we, Emacs, can actually visit the file designated by the URI.  So
> I think passing something like 'access-file' as the OPERATION argument
> makes more sense.  But Eli probably has more knowledge here.  Maybe
> passing nil is indeed correct.

I think you are right, but Michael (CC'ed) will know for sure.

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

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 10 Nov 2022 21:59:37 +0000
> Cc: Eli Zaretskii <eliz@gnu.org>, 58790@debbugs.gnu.org, felician.nemeth@gmail.com, 
> 	stefankangas@gmail.com, dgutov@yandex.ru
> 
> But when dealing with eglot.el, which I am the prime maintainer of, 
> I really prefer to see design described in the commit message where the 
> design was introduced. It plays very well with vc-region-history.

This is not the design, these are comments that explain why the code
does what it does the way it does that.  IOW, these are implementation
notes aimed at making the code more self-explanatory and easier to
maintain.

It is quite possible that you personally don't need these additional
comments because you are very familiar with the code and with LSP
servers in general.  But future Emacs developers and maintainers might
not have such insight, and I'm trying to represent them.  This is part
of my job and my responsibility.  And since comments can never
adversely affect code, I see no reason for you to object.

vc-region-history is a very useful command, but in my book the code
should explain itself; vc-region-history is for when one wants to
understand the reasons for a change, not when one wants to study the
code as a whole.

I'm not objected to having the explanations in the commit log
messages, but why would you object to having them in the code, even if
the commit log already says that?  That way everyone wins.

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

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

> It is quite possible that you personally don't need these additional
> comments because you are very familiar with the code and with LSP
> servers in general.  

Oh you're mistaken.  I do need this information at hand, I forget things
on a weekly basis.  Whether in comments or in commit messages, that's a
different decision.

>But future Emacs developers and maintainers might
> not have such insight, and I'm trying to represent them.  This is part
> of my job and my responsibility.  And since comments can never
> adversely affect code, I see no reason for you to object.

Comments are not a 0-cost thing, obviously They can hamper readability,
they contribute e.g. to a small function no longer fitting in one
screen.  They present a maintenance burden too and a source of
duplication.  These are their drawbacks, but they have many many
advantages too, of course.  You'll see me using comments very often.

I do this on a case-by-case basis.  I thought you were suggesting that
those big blocks of text about Clojure jar URI be moved to comments.
IMO they are out-of-place in source code, especially in such a short
function, but they are quite acceptable in commit messages.  It would
seem you weren't proposing that, so this was a misunderstanding.

Anyway, the comments Danny inserted:

  "Only attempt to parse URIs with the file scheme.".

and 
  
  "Leave non-file type URIs untouched, `file-name-handler-alist'
   handlers can be used to dispatch them properly."

Are quite fine.  I've condensed them and tweaked them to include a
reference to this bug#58790

Now if someone could fix 

  ;; bug-reference-bug-regexp: "\\(github#\\([0-9]+\\)\\)"
  ;; bug-reference-url-format: "https://github.com/joaotavora/eglot/issues/%s"

To work for both GitHub bugs and Emacs bug references in that file, it
would be just peachy.

João

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

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: danny@dfreeman.email,  58790@debbugs.gnu.org,
>   felician.nemeth@gmail.com,  stefankangas@gmail.com,  dgutov@yandex.ru
> Date: Fri, 11 Nov 2022 09:45:06 +0000
> 
> >But future Emacs developers and maintainers might
> > not have such insight, and I'm trying to represent them.  This is part
> > of my job and my responsibility.  And since comments can never
> > adversely affect code, I see no reason for you to object.
> 
> Comments are not a 0-cost thing, obviously They can hamper readability,
> they contribute e.g. to a small function no longer fitting in one
> screen.  They present a maintenance burden too and a source of
> duplication.

IME, these costs, while real, are dwarfed by the costs of making bad
changes due to misunderstanding of why the code does what it does.

And if I'm asking a contributor to add comments explaining why the
code does what it does, it's usually because I didn't understand that
by just looking at the code.  And if I don't immediately understand
that, there arew good chances quite a few others won't.

> These are their drawbacks, but they have many many
> advantages too, of course.  You'll see me using comments very often.

So we are in agreement?

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

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> These are their drawbacks, but they have many many
>> advantages too, of course.  You'll see me using comments very often.
>
> So we are in agreement?

You tell me.  The repo right now contains the original "jar:" motivation
story in the commit message and what I think are essential and useful
comments in the source file.  Is there anything you think is missing
somewhere to aid comprehension of this change?

João

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

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: danny@dfreeman.email,  58790@debbugs.gnu.org,
>   felician.nemeth@gmail.com,  stefankangas@gmail.com,  dgutov@yandex.ru
> Date: Fri, 11 Nov 2022 14:02:34 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> These are their drawbacks, but they have many many
> >> advantages too, of course.  You'll see me using comments very often.
> >
> > So we are in agreement?
> 
> You tell me.  The repo right now contains the original "jar:" motivation
> story in the commit message and what I think are essential and useful
> comments in the source file.  Is there anything you think is missing
> somewhere to aid comprehension of this change?

I already added what I thought was missing.

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

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: João Távora <joaotavora@gmail.com>
>> Cc: danny@dfreeman.email,  58790@debbugs.gnu.org,
>>   felician.nemeth@gmail.com,  stefankangas@gmail.com,  dgutov@yandex.ru
>> Date: Fri, 11 Nov 2022 14:02:34 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> These are their drawbacks, but they have many many
>> >> advantages too, of course.  You'll see me using comments very often.
>> >
>> > So we are in agreement?
>> 
>> You tell me.  The repo right now contains the original "jar:" motivation
>> story in the commit message and what I think are essential and useful
>> comments in the source file.  Is there anything you think is missing
>> somewhere to aid comprehension of this change?
>
> I already added what I thought was missing.

Thanks, it looks good.

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi,

>> > In reviewing the comments over and over I've made a modification to
>> > the second patch involving the warning message. Instead of using an
>> > arbitrary symbol I realized that the function `find-file-name-handler'
>> > works when a nil value for the second argument (operation).
>>
>> I've now pushed the first patch, but I held off the second patch.
>>
>> I don't see this OPERATION=nil option documented in the docstring of
>> find-file-name-handler, so I'm not sure we should rely on it.
>>
>> I think we should provide OPERATION here, because what we want to check
>> is if we, Emacs, can actually visit the file designated by the URI.  So
>> I think passing something like 'access-file' as the OPERATION argument
>> makes more sense.  But Eli probably has more knowledge here.  Maybe
>> passing nil is indeed correct.
>
> I think you are right, but Michael (CC'ed) will know for sure.

Well, using nil as operation might work in this special case, but it
disables an important feature of file name handlers: use of
inhibit-file-name-operation. That's why it shouldn't be documented as
such.

Using just an arbitrary symbol as operation looks better to me. If you
use an existing operation name, like access-file, it could conflict later
with the inhibit-file-name-operation/inhibit-file-name-handlers machinery.

This said, I don't understand why you need this check at all. But I
haven't followed the eglot discussion closely.

Best regards, Michael.

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

[Danny Freeman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 1208 bytes --]


Michael Albinus <michael.albinus@gmx.de> writes:

> Well, using nil as operation might work in this special case, but it
> disables an important feature of file name handlers: use of
> inhibit-file-name-operation. That's why it shouldn't be documented as
> such.
>
> Using just an arbitrary symbol as operation looks better to me. If you
> use an existing operation name, like access-file, it could conflict later
> with the inhibit-file-name-operation/inhibit-file-name-handlers machinery.
>
> This said, I don't understand why you need this check at all. But I
> haven't followed the eglot discussion closely.
>
> Best regards, Michael.

Thanks for your insights Michael! If you are curious, this code in eglot
is about to round-aboutly call `find-file-noselect' on a URI, which
Emacs will probably fail to open if there is no registered file name
handler. So before doing that we write a message warning the user if no
file name handler is registered. That's the idea.

With this info I've written up another patch that checks
find-file-name-handler with an arbitrary symbol (with a better name than
my first attempt, IMO). I've tried to strike a balance between inline
comments and the commit message.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: eglot file name handler warning patch --]
[-- Type: text/x-patch, Size: 1307 bytes --]

From 86a444be767a3e160f60e46086e38ef154134683 Mon Sep 17 00:00:00 2001
From: dannyfreeman <danny@dfreeman.email>
Date: Thu, 10 Nov 2022 19:07:46 -0500
Subject: [PATCH] Eglot: Warn when receiving a non-file type URI (bug#58790)

When this occurs, Emacs will probably not know how to open up the
URI. The warning may help users when investigating a potential
solution.

* lisp/progmodes/eglot.el (eglot--uri-to-path): warn on unhandled URI
---
 lisp/progmodes/eglot.el | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 97c674f7aa..97febdacc2 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1534,7 +1534,11 @@ If optional MARKER, return a marker instead"
                                (substring retval 1)
                              retval)))
           (concat remote-prefix normalized))
-
+      ;; Use a fake operation, eglot--fake-file-name-op because we
+      ;; don't want this check to be suppressed by `inhibit-file-name-operation'.
+      ;; We just want to see if a handler exists.
+      (unless (find-file-name-handler uri 'eglot--fake-file-name-op)
+        (eglot--message "Received URI with unexpected scheme: %s" uri))
       uri)))
 
 (defun eglot--snippet-expansion-fn ()
-- 
2.38.1


[-- Attachment #3: Type: text/plain, Size: 19 bytes --]


-- 
Danny Freeman

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

[Michael Albinus]

Danny Freeman <danny@dfreeman.email> writes:

Hi Danny,

> Thanks for your insights Michael! If you are curious, this code in eglot
> is about to round-aboutly call `find-file-noselect' on a URI, which
> Emacs will probably fail to open if there is no registered file name
> handler. So before doing that we write a message warning the user if no
> file name handler is registered. That's the idea.

Another approach is to simply require url-handlers in eglot.el, when it
comes to handle a URI. It shall do what you want.

(Untested, I haven't used url-handlers for a while).

> Danny Freeman

Best regards, Michael.

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

[Danny Freeman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Michael Albinus <michael.albinus@gmx.de> writes:

> Another approach is to simply require url-handlers in eglot.el, when it
> comes to handle a URI. It shall do what you want.
>
> (Untested, I haven't used url-handlers for a while).

That is a really interesting idea. I didn't know about the url-handlers
package. I think we would want the user to turn on that mode themselves
if they were expecting those types of URLs (http, ftp, etc). 

There are other URI schemes we have seen come through this function that
are outside the scope of the url-handlers package. The ones in this
particular thread are `jar` and `zipfile` URIs. I've seen other obscure
URIs as well that aren't really even standardized and only used by one
particular LSP server.

-- 
Danny Freeman

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

[Michael Albinus]

Danny Freeman <danny@dfreeman.email> writes:

Hi Danny,

>> Another approach is to simply require url-handlers in eglot.el, when it
>> comes to handle a URI. It shall do what you want.
>
> That is a really interesting idea. I didn't know about the url-handlers
> package. I think we would want the user to turn on that mode themselves
> if they were expecting those types of URLs (http, ftp, etc).

You could control this by a user option.

> There are other URI schemes we have seen come through this function that
> are outside the scope of the url-handlers package. The ones in this
> particular thread are `jar` and `zipfile` URIs. I've seen other obscure
> URIs as well that aren't really even standardized and only used by one
> particular LSP server.

url-handlers.el supports already non-canonical schemes, see
`url-tramp-protocols'. We could add "jar" and "zipfile" to another user
option, `url-archive-protocols', and let tramp-archive.el do the job.

The current restriction is, that tramp-archive.el works only on
GNU/Linux systems. When this is replaced by a native integration of
libarchive(3) into Emacs, it would work everywhere.

Best regards, Michael.

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

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 3120 bytes --]

I don't want to derail de discussion, which seems interesting.

I just want to make a few points:

* Eglot catches `file://` URI references coming from the LSP server
  and converts those -- and only those -- to file names.  It uses
  url-generic-parse for this.  The function eglot--uri-to-path handles
  a few more quirks but is not extraordinarily complex (about 15LOC).

* Eglot converts file names to URIs when it needs to tell the LSP
  about the files it is managing.  Here, too, conversion only happens
  if the PATH argument is not already an URI, in which case nothing happens.

* This logic is fairly simple.  Do you see anything to simplify in it,
Michael?
  Can `url-handlers` simplify the functions `eglot--uri-to-path` and
  `eglot--path-to-uri`?

* I didn't mention that sometimes the "file names" are actually "trampish"
  file names, depending on whether the M-x eglot command was invoked
  in file being visited remotely by the TRAMP facility.

* The only thing that's outstanding in the discussion, as I follow it,
  is that someone suggested that Eglot **warn the user** when the LSP
  server communicates to us (Eglot) a URI scheme that is not known
  by the current Emacs session, and as such `find-file` on it will fail.

* This is (or was) what Danny is asking for: A simple, robust way, for Eglot
  code to ask the current Emacs session if this URI scheme is supported
  downstream, and warn the user preemptively.

* If there's no excellent way to do the above, I think the code shouldn't
  be changed.  The user will eventually be confronted with the failure,
  and once could argue that this moment is when she should be made
  aware of the URI scheme that doesn't have a handler.

João




On Wed, Nov 16, 2022 at 7:53 AM Michael Albinus <michael.albinus@gmx.de>
wrote:

> Danny Freeman <danny@dfreeman.email> writes:
>
> Hi Danny,
>
> >> Another approach is to simply require url-handlers in eglot.el, when it
> >> comes to handle a URI. It shall do what you want.
> >
> > That is a really interesting idea. I didn't know about the url-handlers
> > package. I think we would want the user to turn on that mode themselves
> > if they were expecting those types of URLs (http, ftp, etc).
>
> You could control this by a user option.
>
> > There are other URI schemes we have seen come through this function that
> > are outside the scope of the url-handlers package. The ones in this
> > particular thread are `jar` and `zipfile` URIs. I've seen other obscure
> > URIs as well that aren't really even standardized and only used by one
> > particular LSP server.
>
> url-handlers.el supports already non-canonical schemes, see
> `url-tramp-protocols'. We could add "jar" and "zipfile" to another user
> option, `url-archive-protocols', and let tramp-archive.el do the job.
>
> The current restriction is, that tramp-archive.el works only on
> GNU/Linux systems. When this is replaced by a native integration of
> libarchive(3) into Emacs, it would work everywhere.
>
> Best regards, Michael.
>


-- 
João Távora

[-- Attachment #2: Type: text/html, Size: 4111 bytes --]

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

[Michael Albinus]

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

Hi João,

I must admit that I don't use eglot (except some basic tests to
understand how it works), so my comments might be not accurate.

> * Eglot catches `file://` URI references coming from the LSP server
>   and converts those -- and only those -- to file names.  It uses 
>   url-generic-parse for this.  The function eglot--uri-to-path handles
>   a few more quirks but is not extraordinarily complex (about 15LOC).
>
> * Eglot converts file names to URIs when it needs to tell the LSP
>   about the files it is managing.  Here, too, conversion only happens
>   if the PATH argument is not already an URI, in which case nothing
> happens.
>
> * This logic is fairly simple.  Do you see anything to simplify in it,
> Michael?

Both seem to be OK, although I'm not sure that it is the right approach
in eglot--path-to-uri just to concat "file://" and the file-local-name
part of a remote file name.

>   Can `url-handlers` simplify the functions `eglot--uri-to-path` and 
>   `eglot--path-to-uri`?

url-handlers do not convert between the different syntaxes. It is just a
package to implement a file name handler for URIs.

> * I didn't mention that sometimes the "file names" are actually
> "trampish"
>   file names, depending on whether the M-x eglot command was invoked 
>   in file being visited remotely by the TRAMP facility.
>
> * The only thing that's outstanding in the discussion, as I follow it,
>   is that someone suggested that Eglot **warn the user** when the LSP
>   server communicates to us (Eglot) a URI scheme that is not known
>   by the current Emacs session, and as such `find-file` on it will
> fail.

Yes, I understand it. But I don't understand why it is needed: if a URI
scheme is not supported, there will be an error, visible to the user. No
need to apply a check before, I believe.

But I haven't read the whole bug report, so I don't know why this check
is in place.

> * This is (or was) what Danny is asking for: A simple, robust way, for
> Eglot
>   code to ask the current Emacs session if this URI scheme is
> supported
>   downstream, and warn the user preemptively.  
>
> * If there's no excellent way to do the above, I think the code
> shouldn't
>   be changed.  The user will eventually be confronted with the
> failure,
>   and once could argue that this moment is when she should be made
>   aware of the URI scheme that doesn't have a handler.

Hmm, yes. All what I have commented about is the fact, that other
schemes but file:// could be handled by url-handlers. And if there is a
scheme not supported yet, it could be added.

It would be great if I could see a real use case of a URI not starting
with the file:// scheme. In that case I could try to debug and
understand what happens.

Do you (or Danny) have a recipe I could follow?

> João

Best regards, Michael.

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

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]

On Wed, Nov 16, 2022, 15:45 Michael Albinus <michael.albinus@gmx.de> wrote:

> João Távora <joaotavora@gmail.com> writes:
>
>
> Both seem to be OK, although I'm not sure that it is the right approach
> in eglot--path-to-uri just to concat "file://" and the file-local-name
> part of a remote file name.
>

Can you describe a case where this would be problematic? Remember that,
from the point of view of the server, the file is always local. That's
regardless of whether eglot invoked the server remotely or locally.

Yes, I understand it. But I don't understand why it is needed: if a URI
> scheme is not supported, there will be an error, visible to the user. No
> need to apply a check before, I believe.
>

I think you're right. Let's not do this patch
 It's not Eglot's responsibility, or at least there's nothing Eglot can
reasonably do about the problem that a later system can't, except maybe
informing that it was the LSP server who is the source of the unknown URL
scheme. I'm not sure it is worth the trouble, but let others speak their
mind.

But I haven't read the whole bug report, so I don't know why this check
> is in place.
>

It's not in place, we were discussing it.

Do you (or Danny) have a recipe I could follow?
>

Danny would, probably, but the recipe would involve a particular LSP server
and clojure toolchain, i think.

[-- Attachment #2: Type: text/html, Size: 2576 bytes --]

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

[Danny Freeman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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


Michael writes:
> url-handlers.el supports already non-canonical schemes, see
> `url-tramp-protocols'. We could add "jar" and "zipfile" to another user
> option, `url-archive-protocols', and let tramp-archive.el do the job.

I think this could be a nice thing to include in emacs itself.
I have already implemented this funcitonality in a package on elpa:
https://elpa.gnu.org/packages/jarchive.html if you want to look. 

João writes:
> I think you're right. Let's not do this patch
> It's not Eglot's responsibility, or at least there's nothing Eglot can reasonably do about the problem that a later system
> can't, except maybe informing that it was the LSP server who is the source of the unknown URL scheme. I'm not sure it
> is worth the trouble, but let others speak their mind.

This seems reasonable to me.

> > Do you (or Danny) have a recipe I could follow?
> Danny would, probably, but the recipe would involve a particular LSP server and clojure toolchain, i think.

Indeed I do :)

From emacs master branch. The only extra package that needs to be
installed is clojure-mode.

I have a repository here that should be cloned
https://git.sr.ht/~dannyfreeman/eglot-xref-to-jar-repo

It contains a nixos shell to install everything automatically if you use
that kind of thing.

If not, you need to have a recent version of openjdk installed, anything
over version 11 will do.  

The clojure command line tool needs to be installed:
https://clojure.org/guides/install_clojure (it's also in most linux
distro package managers)

Clojure-lsp needs to be installed: https://clojure-lsp.io/installation/


Even if you use the nixos shell, this following step is required:
Running the `clojure -Stree` command in that git repo to install clojure
dependencies.

From there, opening the repository in emacs to this line:
https://git.sr.ht/~dannyfreeman/eglot-xref-to-jar-repo/tree/main/item/src/user.clj#L4

Running `M-x eglot`
Then `M-x xref-find-definitions` over the symbol `inc` should reproduce
the error.

I have not found any other lsp servers that do this kind of thing,
besides a java one that has even more problems than clojure-lsp. So
apologies for the complicated recipe.

-- 
Danny Freeman

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

[Michael Albinus]

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

Hi João,

>     Both seem to be OK, although I'm not sure that it is the right
>     approach in eglot--path-to-uri just to concat "file://" and the
>     file-local-name part of a remote file name.
>
> Can you describe a case where this would be problematic? Remember
> that, from the point of view of the server, the file is always local.
> That's regardless of whether eglot invoked the server remotely or
> locally.

Got it.

Best regards, Michael.

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

[Richard Copley]

On 22/11/2022 14:30, Michael Albinus wrote:
> João Távora <joaotavora@gmail.com> writes:
> 
> Hi João,
> 
>>      Both seem to be OK, although I'm not sure that it is the right
>>      approach in eglot--path-to-uri just to concat "file://" and the
>>      file-local-name part of a remote file name.
>>
>> Can you describe a case where this would be problematic? Remember
>> that, from the point of view of the server, the file is always local.
>> That's regardless of whether eglot invoked the server remotely or
>> locally.
> 
> Got it.
> 
> Best regards, Michael.

Hi All,

For file names with a Windows drive letter and forward slashes (as 
emitted by CMAKE_EXPORT_COMPILE_COMMANDS), the drive letter is 
misinterpreted as a URL type, leading to repeated errors,
   "clangd only supports 'file' URI scheme for workspace files".


(let ((path "c:/projects/awesome-project/source/main.cpp"))
   (message "type %s, url %s"
            (url-type (url-generic-parse-url path))
            (eglot--path-to-uri path)))

;; => "type c, url c:/projects/awesome-project/source/main.cpp"

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

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 1995 bytes --]

Yes,I think I follow.  To be clear I think the problem is somewhere in

(defun eglot--path-to-uri (path)
  "URIfy PATH."
  (let ((truepath (file-truename path)))
    (if (url-type (url-generic-parse-url truepath))
        ;; Path is already a URI, so forward it to the LSP server
        ;; untouched.  The server should be able to handle it, since
        ;; it provided this URI to clients in the first place.
        truepath
      ...)

So either url-generic-parse-url and url-type is fixed in url-parse.el, or
we must add some Windows-specific guards in eglot.el.  Or likely
both, since url-parse.el is not a :core ELPA package.

Richard/Danny, can you perhaps come up with some patch?

João

On Wed, Nov 23, 2022 at 11:55 AM Richard Copley <rcopley@gmail.com> wrote:

> On 22/11/2022 14:30, Michael Albinus wrote:
> > João Távora <joaotavora@gmail.com> writes:
> >
> > Hi João,
> >
> >>      Both seem to be OK, although I'm not sure that it is the right
> >>      approach in eglot--path-to-uri just to concat "file://" and the
> >>      file-local-name part of a remote file name.
> >>
> >> Can you describe a case where this would be problematic? Remember
> >> that, from the point of view of the server, the file is always local.
> >> That's regardless of whether eglot invoked the server remotely or
> >> locally.
> >
> > Got it.
> >
> > Best regards, Michael.
>
> Hi All,
>
> For file names with a Windows drive letter and forward slashes (as
> emitted by CMAKE_EXPORT_COMPILE_COMMANDS), the drive letter is
> misinterpreted as a URL type, leading to repeated errors,
>    "clangd only supports 'file' URI scheme for workspace files".
>
>
> (let ((path "c:/projects/awesome-project/source/main.cpp"))
>    (message "type %s, url %s"
>             (url-type (url-generic-parse-url path))
>             (eglot--path-to-uri path)))
>
> ;; => "type c, url c:/projects/awesome-project/source/main.cpp"
>
>

-- 
João Távora

[-- Attachment #2: Type: text/html, Size: 2826 bytes --]

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

[Arash Esbati]

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

> Yes,I think I follow.  To be clear I think the problem is somewhere in 
>
> (defun eglot--path-to-uri (path)
>   "URIfy PATH."
>   (let ((truepath (file-truename path)))
>     (if (url-type (url-generic-parse-url truepath))
>         ;; Path is already a URI, so forward it to the LSP server
>         ;; untouched.  The server should be able to handle it, since
>         ;; it provided this URI to clients in the first place.
>         truepath
>       ...)
>
> So either url-generic-parse-url and url-type is fixed in url-parse.el, or 
> we must add some Windows-specific guards in eglot.el.  Or likely
> both, since url-parse.el is not a :core ELPA package.
>
> Richard/Danny, can you perhaps come up with some patch?

Have a look at bug#59338: Danny came up with a patch for eglot.el.

  https://lists.gnu.org/archive/html/bug-gnu-emacs/2022-11/msg01510.html

Best, Arash

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

[Richard Copley]

On Wed, 23 Nov 2022 at 12:43, Arash Esbati <arash@gnu.org> wrote:
>
> João Távora <joaotavora@gmail.com> writes:
>
> > Yes,I think I follow.  To be clear I think the problem is somewhere in
> >
> > (defun eglot--path-to-uri (path)
> >   "URIfy PATH."
> >   (let ((truepath (file-truename path)))
> >     (if (url-type (url-generic-parse-url truepath))
> >         ;; Path is already a URI, so forward it to the LSP server
> >         ;; untouched.  The server should be able to handle it, since
> >         ;; it provided this URI to clients in the first place.
> >         truepath
> >       ...)

Thanks, yes.

> > So either url-generic-parse-url and url-type is fixed in url-parse.el, or
> > we must add some Windows-specific guards in eglot.el.  Or likely
> > both, since url-parse.el is not a :core ELPA package.

I don't think "url-generic-parse-url" needs any fix. It is for parsing a URL,
not deciding if something is a URL.

> Have a look at bug#59338: Danny came up with a patch for eglot.el.
>
>   https://lists.gnu.org/archive/html/bug-gnu-emacs/2022-11/msg01510.html
>
> Best, Arash

Excellent.

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

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 704 bytes --]

On Wed, Nov 23, 2022 at 12:49 PM Richard Copley <rcopley@gmail.com> wrote:

>
> Thanks, yes.
>
> > > So either url-generic-parse-url and url-type is fixed in url-parse.el,
> or
> > > we must add some Windows-specific guards in eglot.el.  Or likely
> > > both, since url-parse.el is not a :core ELPA package.
>
> I don't think "url-generic-parse-url" needs any fix. It is for parsing a
> URL,
> not deciding if something is a URL.
>

That's... arguable. But OK, if we go with that, then we need a function to
decide if something is an URL, because it's exactly what we're
aiming for to do here.

But let's continue in the other bug report: I've CC'ed you in my last
message.

João

[-- Attachment #2: Type: text/html, Size: 1136 bytes --]

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

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Wed, 23 Nov 2022 12:36:37 +0000
> Cc: Danny Freeman <danny@dfreeman.email>, Felician Nemeth <felician.nemeth@gmail.com>, 
> 	58790@debbugs.gnu.org, Stefan Kangas <stefankangas@gmail.com>, 
> 	Dmitry Gutov <dgutov@yandex.ru>, Eli Zaretskii <eliz@gnu.org>, 
> 	Michael Albinus <michael.albinus@gmx.de>
> 
> Yes,I think I follow.  To be clear I think the problem is somewhere in 
> 
> (defun eglot--path-to-uri (path)
>   "URIfy PATH."
>   (let ((truepath (file-truename path)))
>     (if (url-type (url-generic-parse-url truepath))
>         ;; Path is already a URI, so forward it to the LSP server
>         ;; untouched.  The server should be able to handle it, since
>         ;; it provided this URI to clients in the first place.
>         truepath
>       ...)
> 
> So either url-generic-parse-url and url-type is fixed in url-parse.el, or 
> we must add some Windows-specific guards in eglot.el.  Or likely
> both, since url-parse.el is not a :core ELPA package.
> 
> Richard/Danny, can you perhaps come up with some patch?

From where I stand, the solution was already proposed here:

  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=59338#53

You said you wanted to augment it with something that didn't depend on
url-parse.el, because it wasn't in Emacs < 29, but I don't think I
understand the concern, since AFAICT url-parse.el is in all versions of
Emacs since at least Emacs 23.  So what am I missing?

In any case, I suggest that a solution be based on the patch shown in that
bug#59338 discussion.

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

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 2420 bytes --]

On Wed, Nov 23, 2022 at 1:34 PM Eli Zaretskii <eliz@gnu.org> wrote:

> > From: João Távora <joaotavora@gmail.com>
> > Date: Wed, 23 Nov 2022 12:36:37 +0000
> > Cc: Danny Freeman <danny@dfreeman.email>, Felician Nemeth <
> felician.nemeth@gmail.com>,
> >       58790@debbugs.gnu.org, Stefan Kangas <stefankangas@gmail.com>,
> >       Dmitry Gutov <dgutov@yandex.ru>, Eli Zaretskii <eliz@gnu.org>,
> >       Michael Albinus <michael.albinus@gmx.de>
> >
> > Yes,I think I follow.  To be clear I think the problem is somewhere in
> >
> > (defun eglot--path-to-uri (path)
> >   "URIfy PATH."
> >   (let ((truepath (file-truename path)))
> >     (if (url-type (url-generic-parse-url truepath))
> >         ;; Path is already a URI, so forward it to the LSP server
> >         ;; untouched.  The server should be able to handle it, since
> >         ;; it provided this URI to clients in the first place.
> >         truepath
> >       ...)
> >
> > So either url-generic-parse-url and url-type is fixed in url-parse.el,
> or
> > we must add some Windows-specific guards in eglot.el.  Or likely
> > both, since url-parse.el is not a :core ELPA package.
> >
> > Richard/Danny, can you perhaps come up with some patch?
>
> From where I stand, the solution was already proposed here:
>
>   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=59338#53


Right. When I wrote that, I was unaware of this second bug report.


> You said you wanted to augment it with something that didn't depend on
> url-parse.el, because it wasn't in Emacs < 29, but I don't think I
> understand the concern, since AFAICT url-parse.el is in all versions of
> Emacs since at least Emacs 23.  So what am I missing?
>

Maybe I misunderstood, but it seemed like you were proposing that
we change url-parse.el so that  it isn't fooled into thinking windows path
names are URLs with the drive letter as the 'url-type'.

If that were the correct solution (I tend to think it is), then it would
_not_ suffice to fix Eglot on Emacsen older than 29, because url-parse.el
is not distributed separately as a :core ELPA package.

So unless we made url-parse.el such a package, we would need a Danny's
"kludgy" patch to eglot.el anyway.

In any case, I suggest that a solution be based on the patch shown in that
> bug#59338 discussion.
>

Having now read Danny's patch, I think it's good to commit.

João

[-- Attachment #2: Type: text/html, Size: 3980 bytes --]

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

[Danny Freeman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

> Maybe I misunderstood, but it seemed like you were proposing that
> we change url-parse.el so that  it isn't fooled into thinking windows path
> names are URLs with the drive letter as the 'url-type'.
>
> If that were the correct solution (I tend to think it is), then it would
> _not_ suffice to fix Eglot on Emacsen older than 29, because url-parse.el
> is not distributed separately as a :core ELPA package.
>
> So unless we made url-parse.el such a package, we would need a Danny's
> "kludgy" patch to eglot.el anyway.
>
> In any case, I suggest that a solution be based on the patch shown in that
>> bug#59338 discussion.
>>
>
> Having now read Danny's patch, I think it's good to commit.
>
> João

Aside from the windows path bug, which is addressed in the other tick,
is this ticket safe to close? Eglot now tries to avoid parsing non-file
type URLs, and we decided against the warning message. Is there anything
else to do?

-- 
Danny Freeman

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

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 207 bytes --]

> type URLs, and we decided against the warning message. Is there anything
> else to do?

Not really. I think this bug is closed (in fact I think it was already, but
I'm
not sure anymore). Closing it again.

[-- Attachment #2: Type: text/html, Size: 259 bytes --]

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

[Michael Albinus]

Danny Freeman <danny@dfreeman.email> writes:

Hi Danny,

Sorry for being late, it took me a while to find a free time slot.

>> url-handlers.el supports already non-canonical schemes, see
>> `url-tramp-protocols'. We could add "jar" and "zipfile" to another user
>> option, `url-archive-protocols', and let tramp-archive.el do the job.
>
> I think this could be a nice thing to include in emacs itself.
> I have already implemented this funcitonality in a package on elpa:
> https://elpa.gnu.org/packages/jarchive.html if you want to look.

Well, the jar: and zipfile: semantics is a little bit different from
what url-handlers supports. So let's continue with your package ATM; we
could replace it by an implementation in url-handlers later on if we
like.

I gave your package a short review, it looks nice. Testing it together
with eglot and the recipe below works nicely. Just one recommendation: I
would add in jarchive--file-name-handler

--8<---------------cut here---------------start------------->8---
          ((eq op 'abbreviate-file-name) uri)
          ((eq op 'make-auto-save-file-name) nil)
          ((eq op 'vc-registered) nil)
--8<---------------cut here---------------end--------------->8---

Eval prior your tests

--8<---------------cut here---------------start------------->8---
(trace-function 'jarchive--file-name-handler)
--8<---------------cut here---------------end--------------->8---

and you'll see what I mean in buffer *trace-output*.

>> Danny would, probably, but the recipe would involve a particular LSP server and clojure toolchain, i think.
>
> Indeed I do :)

I've prepared the test as indicated.

> Running `M-x eglot`
> Then `M-x xref-find-definitions` over the symbol `inc` should reproduce
> the error.

Well, running the test together with your package looks fine. What I
don't understand is the following sequence:

--8<---------------cut here---------------start------------->8---
[client-request] (id:6) Fri Dec  2 17:04:01 2022:
(:jsonrpc "2.0" :id 6 :method "textDocument/definition" :params
	  (:textDocument
	   (:uri "file:///usr/local/src/eglot-xref-to-jar-repo/src/user.clj")
	   :position
	   (:line 3 :character 3)))
[server-reply] (id:6) Fri Dec  2 17:04:01 2022:
(:jsonrpc "2.0" :id 6 :result
	  (:uri "jar:file:///home/albinus/.m2/repository/org/clojure/clojure/1.10.3/clojure-1.10.3.jar!/clojure/core.clj" :range
		(:start
		 (:line 923 :character 6)
		 :end
		 (:line 923 :character 9))))
--8<---------------cut here---------------end--------------->8---

That means, that the client (my local Emacs) has asked for
"textDocument/definition" on the file user.clj, as indicated by your
recipe. The remote server has returned as answer, that this definition is in
"jar:file:///home/albinus/.m2/repository/org/clojure/clojure/1.10.3/clojure-1.10.3.jar!/clojure/core.clj". This
file must be located on the server, because the server cannot know
anything about my local configuration, right?

But your jarchive package opens this file locally. It works, because on
my local machine I have the same file, but it looks confusing to
me. What if the file doesn't exist locally?

Best regards, Michael.

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

[Danny Freeman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Michael Albinus <michael.albinus@gmx.de> writes:

> Danny Freeman <danny@dfreeman.email> writes:
>
> Hi Danny,
>
> Sorry for being late, it took me a while to find a free time slot.
>

No worries, we are all busy! I understand.


> Well, the jar: and zipfile: semantics is a little bit different from
> what url-handlers supports. So let's continue with your package ATM; we
> could replace it by an implementation in url-handlers later on if we
> like.
>
> I gave your package a short review, it looks nice. Testing it together
> with eglot and the recipe below works nicely. Just one recommendation: I
> would add in jarchive--file-name-handler
>
>           ((eq op 'abbreviate-file-name) uri)
>           ((eq op 'make-auto-save-file-name) nil)
>           ((eq op 'vc-registered) nil)
>
>
> Eval prior your tests
>
> (trace-function 'jarchive--file-name-handler)
>
>
> and you'll see what I mean in buffer *trace-output*.

I do see that, I'll be publishing a change that includes this later
today. I'll have to remember to use trace-function more often. What a
nifty tool!

>
> Well, running the test together with your package looks fine. What I
> don't understand is the following sequence:
>
> [client-request] (id:6) Fri Dec  2 17:04:01 2022:
> (:jsonrpc "2.0" :id 6 :method "textDocument/definition" :params
> 	  (:textDocument
> 	   (:uri "file:///usr/local/src/eglot-xref-to-jar-repo/src/user.clj")
> 	   :position
> 	   (:line 3 :character 3)))
> [server-reply] (id:6) Fri Dec  2 17:04:01 2022:
> (:jsonrpc "2.0" :id 6 :result
> 	  (:uri "jar:file:///home/albinus/.m2/repository/org/clojure/clojure/1.10.3/clojure-1.10.3.jar!/clojure/core.clj" :range
> 		(:start
> 		 (:line 923 :character 6)
> 		 :end
> 		 (:line 923 :character 9))))
>
> That means, that the client (my local Emacs) has asked for
> "textDocument/definition" on the file user.clj, as indicated by your
> recipe. The remote server has returned as answer, that this definition is in
> "jar:file:///home/albinus/.m2/repository/org/clojure/clojure/1.10.3/clojure-1.10.3.jar!/clojure/core.clj". This
> file must be located on the server, because the server cannot know
> anything about my local configuration, right?

The server should be running on your machine locally, and have access to
everything Emacs does. It knows the location of that jar because it uses
the clojure build tool to create a classpath for the project, which
contains the full location of the clojure-1.10.3.jar file.

From what the clojure-lsp maintainers tell me, it will not run on a
remote machine and be able to work with a project on your local machine.
I'm not sure what it would return for a definition if it was able to run
on a remote machine.

> But your jarchive package opens this file locally. It works, because on
> my local machine I have the same file, but it looks confusing to
> me. What if the file doesn't exist locally?
> Best regards, Michael.

I did some testing with this and it depends. Clojure-lsp needs the jar
dependencies to do it's analysis, so it will use whatever clojure build
tool the project uses to download them on startup if they do not exist
(given that it is listed as a dependency in the project file, deps.edn
in the case of my recipe).

If it doesn't exist locally after clojure-lsp has started then it will
return the same jar path, but opening the jar file will of course fail.
If this happens it's because the user or some other process deleted the
jar. There isn't much to be done about that.

-- 
Danny Freeman

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

[Michael Albinus]

Danny Freeman <danny@dfreeman.email> writes:

Hi Danny,

>> Well, running the test together with your package looks fine. What I
>> don't understand is the following sequence:
>>
>> [client-request] (id:6) Fri Dec  2 17:04:01 2022:
>> (:jsonrpc "2.0" :id 6 :method "textDocument/definition" :params
>> 	  (:textDocument
>> 	   (:uri "file:///usr/local/src/eglot-xref-to-jar-repo/src/user.clj")
>> 	   :position
>> 	   (:line 3 :character 3)))
>> [server-reply] (id:6) Fri Dec  2 17:04:01 2022:
>> (:jsonrpc "2.0" :id 6 :result
>> 	  (:uri "jar:file:///home/albinus/.m2/repository/org/clojure/clojure/1.10.3/clojure-1.10.3.jar!/clojure/core.clj" :range
>> 		(:start
>> 		 (:line 923 :character 6)
>> 		 :end
>> 		 (:line 923 :character 9))))
>>
>> That means, that the client (my local Emacs) has asked for
>> "textDocument/definition" on the file user.clj, as indicated by your
>> recipe. The remote server has returned as answer, that this definition is in
>> "jar:file:///home/albinus/.m2/repository/org/clojure/clojure/1.10.3/clojure-1.10.3.jar!/clojure/core.clj". This
>> file must be located on the server, because the server cannot know
>> anything about my local configuration, right?
>
> The server should be running on your machine locally, and have access to
> everything Emacs does. It knows the location of that jar because it uses
> the clojure build tool to create a classpath for the project, which
> contains the full location of the clojure-1.10.3.jar file.

???

I'm speaking about a clojure file which is located on a remote machine,
accessed via Tramp. I thought that Eglot uses an LSP server on that
remote machine then.

> From what the clojure-lsp maintainers tell me, it will not run on a
> remote machine and be able to work with a project on your local machine.
> I'm not sure what it would return for a definition if it was able to run
> on a remote machine.

The LSP server shouldn't care. It returns a local file name, like
"jar:file:///home/albinus/.m2/repository/org/clojure/clojure/1.10.3/clojure-1.10.3.jar!/clojure/core.clj". This
is because the file is local from the server's pov.

It is the client, Eglot, which must be able to access this file on the
remote machine. A task for your jarchive file name handler, I believe.

Or do I misunderstand the architecture?

Best regards, Michael.

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

[Danny Freeman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Michael Albinus <michael.albinus@gmx.de> writes:

>
> ???
>
> I'm speaking about a clojure file which is located on a remote machine,
> accessed via Tramp. I thought that Eglot uses an LSP server on that
> remote machine then.

I wasn't aware you were using tramp, I thought you were attempting to
run clojure-lsp on a remote server to work with a project on a local
machine, which shouldn't work. clojure-lsp needs access to the files,
including jar files, to work properly.

>> From what the clojure-lsp maintainers tell me, it will not run on a
>> remote machine and be able to work with a project on your local machine.
>> I'm not sure what it would return for a definition if it was able to run
>> on a remote machine.
>
> The LSP server shouldn't care. It returns a local file name, like
> "jar:file:///home/albinus/.m2/repository/org/clojure/clojure/1.10.3/clojure-1.10.3.jar!/clojure/core.clj". This
> is because the file is local from the server's pov.

> It is the client, Eglot, which must be able to access this file on the
> remote machine. A task for your jarchive file name handler, I believe.
>
> Or do I misunderstand the architecture?

I think I am just confused at this point. Can you explain more about
what your current setup is for this question? In trying to understand,
it sounds like you are using Emacs to connect to a remote machine
somewhere via tramp. On the remote machine is a clojure project and a
running clojure-lsp server. When you try to navigate to a definition in
a jar, it is returning a jar URI pointing to something on your local
machine, not the remote machine like it should? Does that sound right?

Fwiw, I have never used tramp before, so I'm not sure how Eglot or
clojure-lsp will behave in that kind of environment.

-- 
Danny Freeman

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

[Michael Albinus]

Danny Freeman <danny@dfreeman.email> writes:

Hi Danny,

>> The LSP server shouldn't care. It returns a local file name, like
>> "jar:file:///home/albinus/.m2/repository/org/clojure/clojure/1.10.3/clojure-1.10.3.jar!/clojure/core.clj". This
>> is because the file is local from the server's pov.
>
>> It is the client, Eglot, which must be able to access this file on the
>> remote machine. A task for your jarchive file name handler, I believe.
>>
>> Or do I misunderstand the architecture?
>
> I think I am just confused at this point. Can you explain more about
> what your current setup is for this question? In trying to understand,
> it sounds like you are using Emacs to connect to a remote machine
> somewhere via tramp. On the remote machine is a clojure project and a
> running clojure-lsp server. When you try to navigate to a definition in
> a jar, it is returning a jar URI pointing to something on your local
> machine, not the remote machine like it should? Does that sound right?

Yes, this is what it looks like. I don't believe it is an error in the
LSP server.

The eglot architecture, AFAIU, runs always the the LSP server on the
same machine as the file under investigation is located on. For local
files it is obvious: in eglot--connect, make-process is called, which
connects to the LSP server on the local machine.

If you have a remote file, say
/ssh:user@host:/usr/local/src/emacs/src/dbusbind.c (in order to use a
more simple LSP server like clangd), they same happens: make-process is
called. *But* make-process realizes that default-directory is a remote
one (/ssh:user@host:/usr/local/src/emacs/src//), and so it calls clangd
on that remote host.

That LSP server doesn't care where the client is called from. It still
does

--8<---------------cut here---------------start------------->8---
[client-request] (id:20) Fri Dec  9 16:47:05 2022:
(:jsonrpc "2.0" :id 20 :method "textDocument/definition" :params
	  (:textDocument
	   (:uri "file:///usr/local/src/emacs/src/dbusbind.c")
	   :position
	   (:line 171 :character 0)))
[server-reply] (id:20) Fri Dec  9 16:47:05 2022:
(:id 20 :jsonrpc "2.0" :result
     [(:range
       (:end
	(:character 22 :line 172)
	:start
	(:character 0 :line 172))
       :uri "file:///usr/local/src/emacs/src/dbusbind.c")])
--8<---------------cut here---------------end--------------->8---

So the client requests to get a definition in (the server local) file
"file:///usr/local/src/emacs/src/dbusbind.c" at a given position, and
the server replies with a pointer to the (server local) file
"file:///usr/local/src/emacs/src/dbusbind.c". It is up to the client
(eglot), to translate this information into remote file name syntax
"/ssh:user@host:/usr/local/src/emacs/src/dbusbind.c".

> Fwiw, I have never used tramp before, so I'm not sure how Eglot or
> clojure-lsp will behave in that kind of environment.

I suppose for clojure the same spplies. If the LSP server on that remote
host returns something like a (server local) file
"jar:file:///home/albinus/.m2/repository/org/clojure/clojure/1.10.3/clojure-1.10.3.jar!/clojure/core.clj",
it is up to the (eglot) client to recognize this as a pointer to the
remote file at the given location. This problem is not tackled yet by
your jarchive package.

Best regards, Michael.

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

[Danny Freeman via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Michael Albinus <michael.albinus@gmx.de> writes:

> Yes, this is what it looks like. I don't believe it is an error in the
> LSP server.
>
> The eglot architecture, AFAIU, runs always the the LSP server on the
> same machine as the file under investigation is located on. For local
> files it is obvious: in eglot--connect, make-process is called, which
> connects to the LSP server on the local machine.
>
> If you have a remote file, say
> /ssh:user@host:/usr/local/src/emacs/src/dbusbind.c (in order to use a
> more simple LSP server like clangd), they same happens: make-process is
> called. *But* make-process realizes that default-directory is a remote
> one (/ssh:user@host:/usr/local/src/emacs/src//), and so it calls clangd
> on that remote host.
>
> That LSP server doesn't care where the client is called from. It still
> does
>
> [client-request] (id:20) Fri Dec  9 16:47:05 2022:
> (:jsonrpc "2.0" :id 20 :method "textDocument/definition" :params
> 	  (:textDocument
> 	   (:uri "file:///usr/local/src/emacs/src/dbusbind.c")
> 	   :position
> 	   (:line 171 :character 0)))
> [server-reply] (id:20) Fri Dec  9 16:47:05 2022:
> (:id 20 :jsonrpc "2.0" :result
>      [(:range
>        (:end
> 	(:character 22 :line 172)
> 	:start
> 	(:character 0 :line 172))
>        :uri "file:///usr/local/src/emacs/src/dbusbind.c")])
>
> So the client requests to get a definition in (the server local) file
> "file:///usr/local/src/emacs/src/dbusbind.c" at a given position, and
> the server replies with a pointer to the (server local) file
> "file:///usr/local/src/emacs/src/dbusbind.c". It is up to the client
> (eglot), to translate this information into remote file name syntax
> "/ssh:user@host:/usr/local/src/emacs/src/dbusbind.c".
>

>
> I suppose for clojure the same spplies. If the LSP server on that remote
> host returns something like a (server local) file
> "jar:file:///home/albinus/.m2/repository/org/clojure/clojure/1.10.3/clojure-1.10.3.jar!/clojure/core.clj",
> it is up to the (eglot) client to recognize this as a pointer to the
> remote file at the given location. This problem is not tackled yet by
> your jarchive package.
>
> Best regards, Michael.

Thanks for taking the time to explain more. I understand now, and have
enough information to re-create this scenario myself. Once I do that
I'll see how I can account for this scenario in jarchive. When I do I
will post an update.

-- 
Danny Freeman

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

[Michael Albinus]

Danny Freeman <danny@dfreeman.email> writes:

Hi Danny,

> Thanks for taking the time to explain more. I understand now, and have
> enough information to re-create this scenario myself. Once I do that
> I'll see how I can account for this scenario in jarchive. When I do I
> will post an update.

Just some random thoughts. In jarchive, I believe you have two options:

- Use tramp-archive. This is restricted to local GNU/Linux systems(*),
  but it shall work out of the box. If you have a local jar archive,
  just map the file name to something like

  jar:file:///home/albinus/.m2/repository/org/clojure/clojure/1.10.3/clojure-1.10.3.jar!/clojure/core.clj
  => /home/albinus/.m2/repository/org/clojure/clojure/1.10.3/clojure-1.10.3.jar/clojure/core.clj

  Try it out, it will work out of the box :-)

  If the jar archive is located on a remote host, say /ssh:user@host:,
  just prepend that remote file name location:

  /ssh:user@host:/home/albinus/.m2/repository/org/clojure/clojure/1.10.3/clojure-1.10.3.jar/clojure/core.clj

  Try it out, it will work out of the box :-)

- Use jarchive as it is, but on a local cop< if it is located on a
  remote host. If the jarchive is located on, /ssh:user@host:, copy it
  locally:

  (copy-file "/ssh:user@host:/home/albinus/.m2/repository/org/clojure/clojure/1.10.3/clojure-1.10.3.jar"
            "/tmp/home_albinus_.m2_repository_org_clojure_clojure_1.10.3_clojure-1.10.3.jar"

  Let your jarchive file name handler run on this temp file /tmp/home_albinus_.m2_repository_org_clojure_clojure_1.10.3_clojure-1.10.3.jar!/clojure/core.clj

(*): There are plans to integrate libarchive(3) natively into Emacs. Then
     this would work on all systems.

Best regards, Michael.