bug#72701: eglot crash when project-files-relative-names t

bug#72701: eglot crash when project-files-relative-names t

[Ship Mints]

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

project-files-relative-names was introduced a few months ago, so it's no
surprise that there are some side effects when set.

This affects the handler for workspace/didChangeWatchedFiles. The result is
that dirs-to-watch...

         (dirs-to-watch
          (delete-dups (mapcar #'file-name-directory
                               (project-files
                                (eglot--project server))))))

...is nil when project-files-relative-names is t, due to
file-name-directory failing to parse the directories it was expecting. This
results in file-readable-p failing with a nil dir:

         (watch-dir (dir)
           (when-let ((probe
                       (and (file-readable-p dir)

One solution which I've tested is simply the defensive:

         (project-files-relative-names nil) ; add this
         (dirs-to-watch
          (delete-dups (mapcar #'file-name-directory
                               (project-files
                                (eglot--project server))))))

-Stephane

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

bug#72701: eglot crash when project-files-relative-names t

[Ship Mints]

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

Greetings, just want to make sure this fix doesn't get lost...

-Stephane

On Sun, Aug 18, 2024 at 11:46 AM Ship Mints <shipmints@gmail.com> wrote:

> project-files-relative-names was introduced a few months ago, so it's no
> surprise that there are some side effects when set.
>
> This affects the handler for workspace/didChangeWatchedFiles. The result
> is that dirs-to-watch...
>
>          (dirs-to-watch
>           (delete-dups (mapcar #'file-name-directory
>                                (project-files
>                                 (eglot--project server))))))
>
> ...is nil when project-files-relative-names is t, due to
> file-name-directory failing to parse the directories it was expecting. This
> results in file-readable-p failing with a nil dir:
>
>          (watch-dir (dir)
>            (when-let ((probe
>                        (and (file-readable-p dir)
>
> One solution which I've tested is simply the defensive:
>
>          (project-files-relative-names nil) ; add this
>          (dirs-to-watch
>           (delete-dups (mapcar #'file-name-directory
>                                (project-files
>                                 (eglot--project server))))))
>
> -Stephane
>

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

bug#72701: eglot crash when project-files-relative-names t

[Eli Zaretskii]

> Cc: Dmitry Gutov <dmitry@gutov.dev>,
>  João Távora <joaotavora@gmail.com>
> From: Ship Mints <shipmints@gmail.com>
> Date: Thu, 22 Aug 2024 13:04:23 -0400
> 
> Greetings, just want to make sure this fix doesn't get lost...

Fear not, patches don't get lost here.

bug#72701: eglot crash when project-files-relative-names t

[Ship Mints]

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

Thank you. You're a machine!

On Thu, Aug 22, 2024 at 1:52 PM Eli Zaretskii <eliz@gnu.org> wrote:

> > Cc: Dmitry Gutov <dmitry@gutov.dev>,
> >  João Távora <joaotavora@gmail.com>
> > From: Ship Mints <shipmints@gmail.com>
> > Date: Thu, 22 Aug 2024 13:04:23 -0400
> >
> > Greetings, just want to make sure this fix doesn't get lost...
>
> Fear not, patches don't get lost here.
>

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

bug#72701: eglot crash when project-files-relative-names t

[João Távora]

If I understand the problem and the fix it's that `project-files` has
its return value controlled by a global.

Not sure that's a great API design (maybe it should change the end
result some interactive commands instead).
But adding the dynamic binding sounds fine, feel free to push, just
add a good explanation in the commit message.

João

On Thu, Aug 22, 2024 at 6:52 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > Cc: Dmitry Gutov <dmitry@gutov.dev>,
> >  João Távora <joaotavora@gmail.com>
> > From: Ship Mints <shipmints@gmail.com>
> > Date: Thu, 22 Aug 2024 13:04:23 -0400
> >
> > Greetings, just want to make sure this fix doesn't get lost...
>
> Fear not, patches don't get lost here.



-- 
João Távora

bug#72701: eglot crash when project-files-relative-names t

[Dmitry Gutov]

Hi!

On 18/08/2024 18:43, Ship Mints wrote:
> project-files-relative-names was introduced a few months ago, so it's no 
> surprise that there are some side effects when set.
> 
> This affects the handler for workspace/didChangeWatchedFiles. The result 
> is that dirs-to-watch...
> 
>           (dirs-to-watch
>            (delete-dups (mapcar #'file-name-directory
>                                 (project-files
>                                  (eglot--project server))))))
> 
> ...is nil when project-files-relative-names is t, due to 
> file-name-directory failing to parse the directories it was expecting. 
> This results in file-readable-p failing with a nil dir:

You're not really supposed to customize it as an end user (or at least 
not yet) - the caller code either sets it to t to indicate support for 
relative names, or not.

>           (watch-dir (dir)
>             (when-let ((probe
>                         (and (file-readable-p dir)
> 
> One solution which I've tested is simply the defensive:
> 
>           (project-files-relative-names nil) ; add this
>           (dirs-to-watch
>            (delete-dups (mapcar #'file-name-directory
>                                 (project-files
>                                  (eglot--project server))))))

...that said, someday the default value of this var will be flipped, and 
some changes like that might be needed.

The relative version is a bit faster, though, so maybe Eglot could 
switch to relative names too. If it makes sense.

bug#72701: eglot crash when project-files-relative-names t

[Ship Mints]

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

Glad I set project-files-relative-names to t and have been testing. This
side-effect in Eglot is sure to bite other people who are teased by the
NEWS of this new option, despite not being a defcustom.

If it is not intended to be set (yet? ever?), perhaps update the docstring
to indicate this. I do tend to agree with Joao that the API to
project-files probably could have an optional additional argument vs. a
global, especially if the intention is merely to influence a specific
call-site's expected results.

WRT to Eglot using relative as default, in this context it is looking for
directory components and relative names might need to be adorned at least
with "./" so that file-name-directory doesn't fail. Perhaps
project-files-relative-names could be set to a symbol such as
'retain-relative-directory-component.

On Thu, Aug 22, 2024 at 7:22 PM Dmitry Gutov <dmitry@gutov.dev> wrote:

> Hi!
>
> On 18/08/2024 18:43, Ship Mints wrote:
> > project-files-relative-names was introduced a few months ago, so it's no
> > surprise that there are some side effects when set.
> >
> > This affects the handler for workspace/didChangeWatchedFiles. The result
> > is that dirs-to-watch...
> >
> >           (dirs-to-watch
> >            (delete-dups (mapcar #'file-name-directory
> >                                 (project-files
> >                                  (eglot--project server))))))
> >
> > ...is nil when project-files-relative-names is t, due to
> > file-name-directory failing to parse the directories it was expecting.
> > This results in file-readable-p failing with a nil dir:
>
> You're not really supposed to customize it as an end user (or at least
> not yet) - the caller code either sets it to t to indicate support for
> relative names, or not.
>
> >           (watch-dir (dir)
> >             (when-let ((probe
> >                         (and (file-readable-p dir)
> >
> > One solution which I've tested is simply the defensive:
> >
> >           (project-files-relative-names nil) ; add this
> >           (dirs-to-watch
> >            (delete-dups (mapcar #'file-name-directory
> >                                 (project-files
> >                                  (eglot--project server))))))
>
> ...that said, someday the default value of this var will be flipped, and
> some changes like that might be needed.
>
> The relative version is a bit faster, though, so maybe Eglot could
> switch to relative names too. If it makes sense.
>

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

bug#72701: eglot crash when project-files-relative-names t

[Ship Mints]

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

João, I don't have Emacs git access so I'll write the equivalent comment
and commit message here.

-----
         (project-files-relative-names nil) ; override global to ensure
directory components are retained
         (dirs-to-watch
          (delete-dups (mapcar #'file-name-directory
                               (project-files
                                (eglot--project server))))))
-----

"project.el has a global, project-files-relative-names, which influences
project-files to return results without parent directory prefixes and this
causes file-name-directory to fail to detect directories at the root of the
parent. We disable project-files-relative-names to ensure complete results."

On Thu, Aug 22, 2024 at 4:37 PM João Távora <joaotavora@gmail.com> wrote:

> If I understand the problem and the fix it's that `project-files` has
> its return value controlled by a global.
>
> Not sure that's a great API design (maybe it should change the end
> result some interactive commands instead).
> But adding the dynamic binding sounds fine, feel free to push, just
> add a good explanation in the commit message.
>
> João
>
> On Thu, Aug 22, 2024 at 6:52 PM Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > > Cc: Dmitry Gutov <dmitry@gutov.dev>,
> > >  João Távora <joaotavora@gmail.com>
> > > From: Ship Mints <shipmints@gmail.com>
> > > Date: Thu, 22 Aug 2024 13:04:23 -0400
> > >
> > > Greetings, just want to make sure this fix doesn't get lost...
> >
> > Fear not, patches don't get lost here.
>
>
>
> --
> João Távora
>

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

bug#72701: eglot crash when project-files-relative-names t

[João Távora]

On Fri, Aug 23, 2024 at 3:56 PM Ship Mints <shipmints@gmail.com> wrote:
>
> Glad I set project-files-relative-names to t and have been testing. This side-effect in Eglot is sure to bite other people who are teased by the NEWS of this new option, despite not being a defcustom.

If it's not a defcustom and, as Dmitry said, not meant to be set by the user,
I suggest you _don't_ set it globally, but ask for it to be let-bound
in whatever
features you want to make use of it.

Eglot could be one of those features if there's a performance advantage.
But I doubt it, because server-supplied glob expressions may target the
full file name (indeed likely the truename).  Maybe it's worth it nevertheless,
dunno.  Anyway while let-binding p-f-r-names to nil in Eglot could work, I
don't think it's the right solution, especially since it probably triggers a
compilation warning in older Emacsen which don't have this.

Unrelated, there are other enhancements to Eglot's file-watching
machinery.  One of them is about excluding files from over-eager watching
by the server which could eventually bump into OS limits for these
OS objects.

João Távora

bug#72701: eglot crash when project-files-relative-names t

[Ship Mints]

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

Already disabled. But still glad I experimented.

On Fri, Aug 23, 2024 at 11:06 AM João Távora <joaotavora@gmail.com> wrote:

> On Fri, Aug 23, 2024 at 3:56 PM Ship Mints <shipmints@gmail.com> wrote:
> >
> > Glad I set project-files-relative-names to t and have been testing. This
> side-effect in Eglot is sure to bite other people who are teased by the
> NEWS of this new option, despite not being a defcustom.
>
> If it's not a defcustom and, as Dmitry said, not meant to be set by the
> user,
> I suggest you _don't_ set it globally, but ask for it to be let-bound
> in whatever
> features you want to make use of it.
>
> Eglot could be one of those features if there's a performance advantage.
> But I doubt it, because server-supplied glob expressions may target the
> full file name (indeed likely the truename).  Maybe it's worth it
> nevertheless,
> dunno.  Anyway while let-binding p-f-r-names to nil in Eglot could work, I
> don't think it's the right solution, especially since it probably triggers
> a
> compilation warning in older Emacsen which don't have this.
>
> Unrelated, there are other enhancements to Eglot's file-watching
> machinery.  One of them is about excluding files from over-eager watching
> by the server which could eventually bump into OS limits for these
> OS objects.
>
> João Távora
>

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

bug#72701: eglot crash when project-files-relative-names t

[João Távora]

Even without git access you can still submit a full patch prepared
with something like 'git format-patch'.

But this is enough, I think.  Thanks.  I think I'll leave it to Dmitry
to decide what to do here.

João

On Fri, Aug 23, 2024 at 4:08 PM Ship Mints <shipmints@gmail.com> wrote:
>
> João, I don't have Emacs git access so I'll write the equivalent comment and commit message here.
>
> -----
>          (project-files-relative-names nil) ; override global to ensure directory components are retained
>          (dirs-to-watch
>           (delete-dups (mapcar #'file-name-directory
>                                (project-files
>                                 (eglot--project server))))))
> -----
>
> "project.el has a global, project-files-relative-names, which influences project-files to return results without parent directory prefixes and this causes file-name-directory to fail to detect directories at the root of the parent. We disable project-files-relative-names to ensure complete results."
>
> On Thu, Aug 22, 2024 at 4:37 PM João Távora <joaotavora@gmail.com> wrote:
>>
>> If I understand the problem and the fix it's that `project-files` has
>> its return value controlled by a global.
>>
>> Not sure that's a great API design (maybe it should change the end
>> result some interactive commands instead).
>> But adding the dynamic binding sounds fine, feel free to push, just
>> add a good explanation in the commit message.
>>
>> João
>>
>> On Thu, Aug 22, 2024 at 6:52 PM Eli Zaretskii <eliz@gnu.org> wrote:
>> >
>> > > Cc: Dmitry Gutov <dmitry@gutov.dev>,
>> > >  João Távora <joaotavora@gmail.com>
>> > > From: Ship Mints <shipmints@gmail.com>
>> > > Date: Thu, 22 Aug 2024 13:04:23 -0400
>> > >
>> > > Greetings, just want to make sure this fix doesn't get lost...
>> >
>> > Fear not, patches don't get lost here.
>>
>>
>>
>> --
>> João Távora



-- 
João Távora

bug#72701: eglot crash when project-files-relative-names t

[Dmitry Gutov]

On 23/08/2024 18:08, João Távora wrote:

> Eglot could be one of those features if there's a performance advantage.
> But I doubt it, because server-supplied glob expressions may target the
> full file name (indeed likely the truename).

If the glob can match the full name, and it's hard to separate it into 
two matchers, I suppose there's not much that could be done.

The binding is probably and an improvement for some off-in-the-future 
scenario where somebody has Emacs 30 installed, but upgrades project.el 
to some yet-unreleased version where the variable's default is flipped.

> Maybe it's worth it nevertheless,
> dunno.  Anyway while let-binding p-f-r-names to nil in Eglot could work, I
> don't think it's the right solution, especially since it probably triggers a
> compilation warning in older Emacsen which don't have this.

A (defvar ...) at the top of the function's body would help.

bug#72701: eglot crash when project-files-relative-names t

[Eli Zaretskii]

Ping!  Is this issue resolved and can be closed, or do we need to do
anything else here?

> Cc: 72701@debbugs.gnu.org
> Date: Sat, 24 Aug 2024 02:51:16 +0300
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 23/08/2024 18:08, João Távora wrote:
> 
> > Eglot could be one of those features if there's a performance advantage.
> > But I doubt it, because server-supplied glob expressions may target the
> > full file name (indeed likely the truename).
> 
> If the glob can match the full name, and it's hard to separate it into 
> two matchers, I suppose there's not much that could be done.
> 
> The binding is probably and an improvement for some off-in-the-future 
> scenario where somebody has Emacs 30 installed, but upgrades project.el 
> to some yet-unreleased version where the variable's default is flipped.
> 
> > Maybe it's worth it nevertheless,
> > dunno.  Anyway while let-binding p-f-r-names to nil in Eglot could work, I
> > don't think it's the right solution, especially since it probably triggers a
> > compilation warning in older Emacsen which don't have this.
> 
> A (defvar ...) at the top of the function's body would help.
> 
> 
> 
> 

bug#72701: eglot crash when project-files-relative-names t

[Dmitry Gutov]

On 07/09/2024 10:20, Eli Zaretskii wrote:
> Ping!  Is this issue resolved and can be closed, or do we need to do
> anything else here?

I suggest installing the following. Not a hard necessity, but seems like 
an improvement:

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index acc197754db..e5b14ce9f80 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -3813,6 +3813,7 @@ eglot--code-action
  (cl-defmethod eglot-register-capability
    (server (method (eql workspace/didChangeWatchedFiles)) id &key watchers)
    "Handle dynamic registration of workspace/didChangeWatchedFiles."
+  (defvar project-files-relative-names)
    (eglot-unregister-capability server method id)
    (let* (success
           (globs (mapcar
@@ -3823,6 +3824,7 @@ eglot-register-capability
                           ;; (2), WatchKind.Delete (4)
                           (or kind 7)))
                   watchers))
+         (project-files-relative-names nil)
           (dirs-to-watch
            (delete-dups (mapcar #'file-name-directory
                                 (project-files
diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index c38d3f0048a..78f5c127900 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -331,7 +331,10 @@ project-files-relative-names
  The file names should be relative to the project root.  And this can
  only happen when all returned files are in the same directory.
  In other words, the DIRS argument of `project-files' has to be nil or a
-list of only one element.")
+list of only one element.
+
+This variable is only meant to be set by Lisp code, not customized by
+the user.")

  (cl-defgeneric project-files (project &optional dirs)
    "Return a list of files in directories DIRS in PROJECT.

bug#72701: eglot crash when project-files-relative-names t

[João Távora]

On Sun, Sep 8, 2024 at 3:24 AM Dmitry Gutov <dmitry@gutov.dev> wrote:
>
> On 07/09/2024 10:20, Eli Zaretskii wrote:
> > Ping!  Is this issue resolved and can be closed, or do we need to do
> > anything else here?
>
> I suggest installing the following. Not a hard necessity, but seems like
> an improvement:

Let's not, for all the reasons enunciated up-thread.

João

bug#72701: eglot crash when project-files-relative-names t

[Dmitry Gutov]

On 08/09/2024 13:56, João Távora wrote:
> On Sun, Sep 8, 2024 at 3:24 AM Dmitry Gutov <dmitry@gutov.dev> wrote:
>>
>> On 07/09/2024 10:20, Eli Zaretskii wrote:
>>> Ping!  Is this issue resolved and can be closed, or do we need to do
>>> anything else here?
>>
>> I suggest installing the following. Not a hard necessity, but seems like
>> an improvement:
> 
> Let's not, for all the reasons enunciated up-thread.

Very well - I've just installed the docstring update. Thanks, closing.