bug#59338: 29.0.50; Commit 1a2d603bb3 breaks Eglot on Windows

bug#59338: 29.0.50; Commit 1a2d603bb3 breaks Eglot on Windows

[Arash Esbati]

Hi all,

the commit 1a2d603bb3 was supposed to fix bug#58790, but it introduces
(or possibly surfaces) another one.  With Emacs (checkout c3b64985aa),
eval'ing the next 2 forms returned an URI:

  (require 'eglot)
  (insert "\n" (format "%s" (eglot--path-to-uri
                             "d:/digestif-test/tikz-test.tex")))
  => file:///d%3A/digestif-test/tikz-test.tex

With Emacs 623db40d, it looks like this:

  (require 'eglot)
  (insert "\n" (format "%s" (eglot--path-to-uri
                             "d:/digestif-test/tikz-test.tex")))
  => d:/digestif-test/tikz-test.tex

I think the underlying problem is with the return value of
`url-generic-parse-url', but I'm not familiar enough with it to make a
judgement.  As the result, digestif-LSP doesn't work with the current
Eglot on Windows -- checkout c3b64985aa does.

This is with:
In GNU Emacs 29.0.50 (build 1, x86_64-w64-mingw32) of 2022-11-17
Repository revision: 623db40dd1cd21623c5cecdc0abbf3ce885f92b1
Repository branch: master
System Description: Microsoft Windows 10 Pro

Best, Arash

bug#59338: 29.0.50; Commit 1a2d603bb3 breaks Eglot on Windows

[Eli Zaretskii]

> From: Arash Esbati <arash@gnu.org>
> Date: Thu, 17 Nov 2022 17:51:01 +0100
> 
> the commit 1a2d603bb3 was supposed to fix bug#58790, but it introduces
> (or possibly surfaces) another one.  With Emacs (checkout c3b64985aa),
> eval'ing the next 2 forms returned an URI:
> 
>   (require 'eglot)
>   (insert "\n" (format "%s" (eglot--path-to-uri
>                              "d:/digestif-test/tikz-test.tex")))
>   => file:///d%3A/digestif-test/tikz-test.tex
> 
> With Emacs 623db40d, it looks like this:
> 
>   (require 'eglot)
>   (insert "\n" (format "%s" (eglot--path-to-uri
>                              "d:/digestif-test/tikz-test.tex")))
>   => d:/digestif-test/tikz-test.tex

Both are wrong, right?  The correct URL should AFAIU be

  file:///d:/digestif-test/tikz-test.tex

IOW, the problem is that the URL is being run through url-encode-url,
which doesn't support file:// URLs on Windows properly.

bug#59338: 29.0.50; Commit 1a2d603bb3 breaks Eglot on Windows

[Augusto Stoffel]

On Thu, 17 Nov 2022 at 19:06, Eli Zaretskii wrote:

> Both are wrong, right?  The correct URL should AFAIU be
>
>   file:///d:/digestif-test/tikz-test.tex
>
> IOW, the problem is that the URL is being run through url-encode-url,
> which doesn't support file:// URLs on Windows properly.

While we are at it, note that

  (url-filename
    (url-generic-parse-url "file:///d:/digestif-test/tikz-test.tex"))
  => "/d:/digestif-test/tikz-test.tex"

is not the right file name under Windows.  Eglot treats this special
case correctly, but every package that deals with file URLs has to
repeat the work.  So there should be a helper function in Emacs for
this.

bug#59338: 29.0.50; Commit 1a2d603bb3 breaks Eglot on Windows

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

Augusto Stoffel <arstoffel@gmail.com> writes:

> On Thu, 17 Nov 2022 at 19:06, Eli Zaretskii wrote:
>
>> Both are wrong, right?  The correct URL should AFAIU be
>>
>>   file:///d:/digestif-test/tikz-test.tex
>>
>> IOW, the problem is that the URL is being run through url-encode-url,
>> which doesn't support file:// URLs on Windows properly.
>
> While we are at it, note that
>
>   (url-filename
>     (url-generic-parse-url "file:///d:/digestif-test/tikz-test.tex"))
>   => "/d:/digestif-test/tikz-test.tex"
>
> is not the right file name under Windows.  Eglot treats this special
> case correctly, but every package that deals with file URLs has to
> repeat the work.  So there should be a helper function in Emacs for
> this.

With the original problem: we're now getting a false positive in
eglot when checking for URLs being passed to `eglot--path-to-uri` when
the path is a windows path.

Is there something we can do to detect a windows path and continue
treating it as a path like we were before this change?

```
(defun eglot--path-to-uri (path)
  "URIfy PATH."
  (let ((truepath (file-truename path)))
    (if (and (url-type (url-generic-parse-url truepath))
             (NOT_WINDOWS_PATH truepath) ;; what would this be?
             )
      ;; ... blah blah blah
```

If there is no function available already, it may be enough to check if
the return value of `url-type` is not 1 character. Looking at this list
of what I believe are official URI schemes, all of them have at least
two characters: 
https://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml 

-- 
Danny Freeman

bug#59338: 29.0.50; Commit 1a2d603bb3 breaks Eglot on Windows

[Augusto Stoffel]

On Thu, 17 Nov 2022 at 17:27, Danny Freeman wrote:

> ```
> (defun eglot--path-to-uri (path)
>   "URIfy PATH."
>   (let ((truepath (file-truename path)))
>     (if (and (url-type (url-generic-parse-url truepath))
>              (NOT_WINDOWS_PATH truepath) ;; what would this be?
>              )
>       ;; ... blah blah blah
> ```
>
> If there is no function available already, it may be enough to check if
> the return value of `url-type` is not 1 character. Looking at this list
> of what I believe are official URI schemes, all of them have at least
> two characters: 
> https://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml 

I think that makes sense.  I find the above logic a bit funny, though.
What do you expect `truepath' to look like if `path' is actually an URI?
Shouldn't `path' be returned unchanged?

I also think that calling `url-generic-parse-url' might be overkill
here.  Based on
https://en.wikipedia.org/wiki/Uniform_Resource_Identifier#Syntax I would
just test if `path' matches "\\`[A-Za-z][+.0-9A-Za-z-]+:".

bug#59338: 29.0.50; Commit 1a2d603bb3 breaks Eglot on Windows

[Eli Zaretskii]

> From: Augusto Stoffel <arstoffel@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Arash Esbati <arash@gnu.org>,
>   59338@debbugs.gnu.org,  bug-gnu-emacs@gnu.org
> Date: Thu, 17 Nov 2022 18:12:05 -0500
> 
> On Thu, 17 Nov 2022 at 17:27, Danny Freeman wrote:
> 
> I also think that calling `url-generic-parse-url' might be overkill
> here.  Based on
> https://en.wikipedia.org/wiki/Uniform_Resource_Identifier#Syntax I would
> just test if `path' matches "\\`[A-Za-z][+.0-9A-Za-z-]+:".

Not sure I understand why this matters in the context of this
discussion.  We need to make eglot--path-to-uri produce valid file://
URL on MS-Windows and on Posix systems, right?  Then why does it
matter how URI schema are defined?  What am I missing?

bug#59338: 29.0.50; Commit 1a2d603bb3 breaks Eglot on Windows

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

Eli Zaretskii <eliz@gnu.org> writes:

>
> Not sure I understand why this matters in the context of this
> discussion.  We need to make eglot--path-to-uri produce valid file://
> URL on MS-Windows and on Posix systems, right?  Then why does it
> matter how URI schema are defined?  What am I missing?

The current eglot--path-to-uri implementation should produce a valid
file:// url unless what it receives is already a URL.

So it could receive something like:

/home/user/project/whatever.c
d:/what/home/is/on/windows/whatever.c

Both of which should be transformed into file:// URLs
OR what it receives may already be a URL like

zipfile:home/user/project.zip::/path/in/zip.c

If it receives a URL, we want to pass it along, and not transform it
into a file:// URL.

If it is a full windows path, we DO want to turn that into a file url.

So how do we detect that is is a windows path, and not a URL already?
That's what I was trying to get at in the other message you replied to.
Just checking the user's current OS is not enough, because this function
could also receive a URL on Windows.

-- 
Danny Freeman

bug#59338: 29.0.50; Commit 1a2d603bb3 breaks Eglot on Windows

[Eli Zaretskii]

> From: Danny Freeman <danny@dfreeman.email>
> Cc: Augusto Stoffel <arstoffel@gmail.com>, arash@gnu.org,
>  59338@debbugs.gnu.org, bug-gnu-emacs@gnu.org
> Date: Fri, 18 Nov 2022 08:39:13 -0500
> 
> The current eglot--path-to-uri implementation should produce a valid
> file:// url unless what it receives is already a URL.
> 
> So it could receive something like:
> 
> /home/user/project/whatever.c
> d:/what/home/is/on/windows/whatever.c
> 
> Both of which should be transformed into file:// URLs
> OR what it receives may already be a URL like
> 
> zipfile:home/user/project.zip::/path/in/zip.c
> 
> If it receives a URL, we want to pass it along, and not transform it
> into a file:// URL.
> 
> If it is a full windows path, we DO want to turn that into a file url.
> 
> So how do we detect that is is a windows path, and not a URL already?

You test that system-type is windows-nt AND that the
file-name-absolute-p returns non-nil for the argument.

bug#59338: 29.0.50; Commit 1a2d603bb3 breaks Eglot on Windows

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

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


Eli Zaretskii <eliz@gnu.org> writes:

>> If it is a full windows path, we DO want to turn that into a file url.
>> 
>> So how do we detect that is is a windows path, and not a URL already?
>
> You test that system-type is windows-nt AND that the
> file-name-absolute-p returns non-nil for the argument.

That is incredible simple. Maybe something like this patch will work:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Eglot-don-t-confuse-URLs-and-windows-file-paths.patch --]
[-- Type: text/x-patch, Size: 1419 bytes --]

From 85821844094e5d00ceb01902c55d9aa0fef73d17 Mon Sep 17 00:00:00 2001
From: dannyfreeman <danny@dfreeman.email>
Date: Fri, 18 Nov 2022 10:09:17 -0500
Subject: [PATCH] Eglot: don't confuse URLs and windows file paths

* lisp/progmodes/eglot.el (eglot--path-to-uri): check for windows path
---
 lisp/progmodes/eglot.el | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index c2d7fc309d..4661cf0e1a 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1502,11 +1502,15 @@ eglot--uri-path-allowed-chars
 (defun eglot--path-to-uri (path)
   "URIfy PATH."
   (let ((truepath (file-truename path)))
-    (if (url-type (url-generic-parse-url truepath))
+    (if (and (url-type (url-generic-parse-url path))
+             ;; It might be a windows path which includes a drive
+             ;; letter that looks like a URL scheme
+             (not (and (eq system-type 'windows-nt)
+                       (file-name-absolute-p 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
+        path
       (concat "file://"
               ;; Add a leading "/" for local MS Windows-style paths.
               (if (and (eq system-type 'windows-nt)
-- 
2.38.1


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


I can confirm it works on linux, but I don't have a windows machine.
Someone else would need to verify it there.

-- 
Danny Freeman

bug#59338: 29.0.50; Commit 1a2d603bb3 breaks Eglot on Windows

[Arash Esbati]

Danny Freeman <danny@dfreeman.email> writes:

> I can confirm it works on linux, but I don't have a windows machine.
> Someone else would need to verify it there.

Many thanks for working on this.  I tried your patch on my Windows box
and this is what I get:

  (require 'eglot)
  (insert "\n" (format "%s" (eglot--path-to-uri
                             "d:/digestif-test/tikz-test.tex")))
  => file:///d%3A/digestif-test/tikz-test.tex

  (insert "\n" (format "%s" (eglot--path-to-uri
                             "d:/digestif-test/tikz test.tex")))
  => file:///d%3A/digestif-test/tikz%20test.tex

As Eli already mentioned, %-escaping the colon seems to be wrong (I
couldn't find a definitive source for this, though), but it seems a
deliberate decision in eglot.el's `eglot--uri-path-allowed-chars', which
also references this GitHub issue[1].  My original problem is solved and
digestif-LSP works on Windows again.  So for now, I suggest to apply
your patch and close this report.

Best, Arash

Footnotes:
[1]  https://github.com/joaotavora/eglot/pull/639

bug#59338: 29.0.50; Commit 1a2d603bb3 breaks Eglot on Windows

[Eli Zaretskii]

> From: Danny Freeman <danny@dfreeman.email>
> Cc: Eli Zaretskii <eliz@gnu.org>, Arash Esbati <arash@gnu.org>,
>  59338@debbugs.gnu.org, bug-gnu-emacs@gnu.org
> Date: Thu, 17 Nov 2022 17:27:40 -0500
> 
> Is there something we can do to detect a windows path

You mean, a Windows-style file name?  You can detect that, but it is
easier to test system-type instead: these file names cannot happen on
any system except Windows, and if they do happen, they don't have the
same semantics (i.e., "d:/foo/bar" is NOT an absolute file name on
Posix systems).

Or maybe I don't understand the purpose of the test you have in mind?

> and continue treating it as a path like we were before this change?

I'd advise against such kludges.  If a function wants a file:// URL,
it should receive a valid file:// URL on all systems, and it should be
capable of handling file:// URLs on MS-Windows as well as on Posix
systems.  Likewise with functions which produce file:// URLs.  Letting
local file names into this is a clear path to future bugs, because
many people will not realize this subtlety, and will think they deal
with file:// URLs on all platforms.

> If there is no function available already, it may be enough to check if
> the return value of `url-type` is not 1 character. Looking at this list
> of what I believe are official URI schemes, all of them have at least
> two characters: 
> https://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml 

But hosts can have 1-character names (although that is unlikely).

Anyway, I'm against such kludges, especially since we don't need them
here.  We just need to make our functions that handle file:// URLs to
be capable of supporting file:// on MS-Windows.  It is not hard to do,
so let's do that.

bug#59338: 29.0.50; Commit 1a2d603bb3 breaks Eglot on Windows

[João Távora]

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

I just found out this bug was ongoing.

Eli, if you're proposing to fix url-parse.el to not be fooled by windows
file names, then I support that idea, and I think it's the correct
thing to do.

But.... we still need the eglot.el kludge installed because url-parse.el
is not distributed as an ELPA package and Eglot is. So users of
Emacs < 29 would not receive the fix and would have their
WIndows Eglot broken.

João

On Fri, Nov 18, 2022 at 7:13 AM Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Danny Freeman <danny@dfreeman.email>
> > Cc: Eli Zaretskii <eliz@gnu.org>, Arash Esbati <arash@gnu.org>,
> >  59338@debbugs.gnu.org, bug-gnu-emacs@gnu.org
> > Date: Thu, 17 Nov 2022 17:27:40 -0500
> >
> > Is there something we can do to detect a windows path
>
> You mean, a Windows-style file name?  You can detect that, but it is
> easier to test system-type instead: these file names cannot happen on
> any system except Windows, and if they do happen, they don't have the
> same semantics (i.e., "d:/foo/bar" is NOT an absolute file name on
> Posix systems).
>
> Or maybe I don't understand the purpose of the test you have in mind?
>
> > and continue treating it as a path like we were before this change?
>
> I'd advise against such kludges.  If a function wants a file:// URL,
> it should receive a valid file:// URL on all systems, and it should be
> capable of handling file:// URLs on MS-Windows as well as on Posix
> systems.  Likewise with functions which produce file:// URLs.  Letting
> local file names into this is a clear path to future bugs, because
> many people will not realize this subtlety, and will think they deal
> with file:// URLs on all platforms.
>
> > If there is no function available already, it may be enough to check if
> > the return value of `url-type` is not 1 character. Looking at this list
> > of what I believe are official URI schemes, all of them have at least
> > two characters:
> > https://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml
>
> But hosts can have 1-character names (although that is unlikely).
>
> Anyway, I'm against such kludges, especially since we don't need them
> here.  We just need to make our functions that handle file:// URLs to
> be capable of supporting file:// on MS-Windows.  It is not hard to do,
> so let's do that.
>
>
>
>

-- 
João Távora

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

bug#59338: 29.0.50; Commit 1a2d603bb3 breaks Eglot on Windows

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

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

> I just found out this bug was ongoing.
>
> Eli, if you're proposing to fix url-parse.el to not be fooled by windows
> file names, then I support that idea, and I think it's the correct
> thing to do.
>
> But.... we still need the eglot.el kludge installed because url-parse.el
> is not distributed as an ELPA package and Eglot is. So users of
> Emacs < 29 would not receive the fix and would have their
> WIndows Eglot broken.
>
> João
>

Should my patch for eglot be merged then?
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=59338#53

-- 
Danny Freeman

bug#59338: 29.0.50; Commit 1a2d603bb3 breaks Eglot on Windows

[João Távora]

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

I've just tested it on a Windows machine and pushed it, thanks.

Closing this.

João

On Thu, Nov 24, 2022 at 1:44 PM Danny Freeman <danny@dfreeman.email> wrote:

>
> João Távora <joaotavora@gmail.com> writes:
>
> > I just found out this bug was ongoing.
> >
> > Eli, if you're proposing to fix url-parse.el to not be fooled by windows
> > file names, then I support that idea, and I think it's the correct
> > thing to do.
> >
> > But.... we still need the eglot.el kludge installed because url-parse.el
> > is not distributed as an ELPA package and Eglot is. So users of
> > Emacs < 29 would not receive the fix and would have their
> > WIndows Eglot broken.
> >
> > João
> >
>
> Should my patch for eglot be merged then?
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=59338#53
>
> --
> Danny Freeman
>


-- 
João Távora

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

bug#59338: 29.0.50; Commit 1a2d603bb3 breaks Eglot on Windows

[Arash Esbati]

Eli Zaretskii <eliz@gnu.org> writes:

> Both are wrong, right?  The correct URL should AFAIU be
>
>   file:///d:/digestif-test/tikz-test.tex

Yes, this is also my understanding, after having a closer look.

> IOW, the problem is that the URL is being run through url-encode-url,
> which doesn't support file:// URLs on Windows properly.

As a note, texlab LSP-server still works with this version:

  => d:/digestif-test/tikz-test.tex

and digestif LSP-server accepts this version:

  => file:///d%3A/digestif-test/tikz-test.tex

Best, Arash