bug#57325: 27.1; functions in ff-other-file-alist

bug#57325: 27.1; functions in ff-other-file-alist

[Felician Nemeth]

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

[There is a simple reproducer at end of the bug report.]

First, the documentation of ff-other-file-alist fails to mention that
the value of the variable does not have to be alist, it can be a symbol
as well.

More importantly, if the associated value is a function, then there's no
way for the function to signal that it cannot find a related file.

If the function returns "/nonexistent", then ff-find-the-other-file
(with the default settings) will try to create "/nonexistent".

If the function returns nil, then ff-find-the-other-file will call
ff-get-file-name and:

  (ff-get-file-name '("." "/usr/include" "/usr/local/include/*") nil nil)
  ==> "/home/nemethf/.emacs.d/News/drafts/drafts/679"

"emacs -Q -l bug.el" reproduces the problem by setting
uniquify-buffer-name-style.  However, my uniquify-buffer-name-style is
'forward and not nil.  Maybe Gnus changes uniquify-buffer-name-style
under the hood, because drafts/679 corresponds to a buffer named
"*sent wide reply to Somebody*<2>".

I tend to think this bug in line 577 of find-file.el:
http://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/find-file.el?id=958924a8126cf532d44c4b446d13ed744438cc9b#n577
But I don't understand the purpose of that string-match-p.

Thanks.


[-- Attachment #2: bug.el --]
[-- Type: application/emacs-lisp, Size: 448 bytes --]

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



In GNU Emacs 27.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.24, cairo version 1.16.0)
 of 2021-03-28, modified by Debian built on x86-conova-01
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Debian GNU/Linux 11 (bullseye)

bug#57325: 27.1; functions in ff-other-file-alist

[Eli Zaretskii]

> From: Felician Nemeth <felician.nemeth@gmail.com>
> Date: Sun, 21 Aug 2022 20:34:17 +0200
> 
> First, the documentation of ff-other-file-alist fails to mention that
> the value of the variable does not have to be alist, it can be a symbol
> as well.

Thanks, now fixed on the emacs-28 branch.

> More importantly, if the associated value is a function, then there's no
> way for the function to signal that it cannot find a related file.

That's on purpose: this package's design causes such a situation to
make no sense.  Imagine that I wrote a C source file and what now to
"find" the corresponding header file.  If that header file doesn't
exist, I need to write it and save it.  Which is why
ff-find-the-other-file behaves like it does: if the file doesn't
exist, it visits that file as a new one and lets me edit it.

What would you have the package do instead when "the other" file is
not found?

> If the function returns "/nonexistent", then ff-find-the-other-file
> (with the default settings) will try to create "/nonexistent".
> 
> If the function returns nil, then ff-find-the-other-file will call
> ff-get-file-name and:
> 
>   (ff-get-file-name '("." "/usr/include" "/usr/local/include/*") nil nil)
>   ==> "/home/nemethf/.emacs.d/News/drafts/drafts/679"
> 
> "emacs -Q -l bug.el" reproduces the problem by setting
> uniquify-buffer-name-style.  However, my uniquify-buffer-name-style is
> 'forward and not nil.  Maybe Gnus changes uniquify-buffer-name-style
> under the hood, because drafts/679 corresponds to a buffer named
> "*sent wide reply to Somebody*<2>".
> 
> I tend to think this bug in line 577 of find-file.el:
> http://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/find-file.el?id=958924a8126cf532d44c4b446d13ed744438cc9b#n577
> But I don't understand the purpose of that string-match-p.

It looks for buffers which were uniquified.  But I don't think I
understand what you don't understand there, nor how that part is
related to the issue you are raising, which AFAIU is that the function
cannot meaningfully signal a failure.  Please elaborate.

Thanks.

bug#57325: 27.1; functions in ff-other-file-alist

[Felician Nemeth]

> Thanks, now fixed on the emacs-28 branch.

Thank you.

>> More importantly, if the associated value is a function, then there's no
>> way for the function to signal that it cannot find a related file.

> What would you have the package do instead when "the other" file is
> not found?

Most probably I just failed to understand that my-find-related-file
should not return nil and, as you say, it cannot meaningfully signal a
failure.  If this is the case, this bug can be closed and I'm sorry for
the noise.

I got confused because if I remove the
(setq uniquify-buffer-name-style nil) line from bug.el, then
ff-find-related-file will open the parent directory of xref.el, which
feels correct.  However, with that line setting
uniquify-buffer-name-style, ff-find-related-file selects /tmp/dir/1234,
which feels wrong because that buffer has nothing to do with xref.el.

In my case, a file can contain a link to another file (a .toml file to a
schema file).  I wasn't sure what to do when the original file did not
contain a link.  Maybe my-find-related-file should ask the user what to
do in this case, or just do a (user-error "There's no related file.").

Thanks again.

bug#57325: 27.1; functions in ff-other-file-alist

[Eli Zaretskii]

> From: Felician Nemeth <felician.nemeth@gmail.com>
> Cc: 57325@debbugs.gnu.org
> Date: Sun, 21 Aug 2022 21:37:31 +0200
> 
> I got confused because if I remove the
> (setq uniquify-buffer-name-style nil) line from bug.el, then
> ff-find-related-file will open the parent directory of xref.el, which
> feels correct.  However, with that line setting
> uniquify-buffer-name-style, ff-find-related-file selects /tmp/dir/1234,
> which feels wrong because that buffer has nothing to do with xref.el.
> 
> In my case, a file can contain a link to another file (a .toml file to a
> schema file).  I wasn't sure what to do when the original file did not
> contain a link.  Maybe my-find-related-file should ask the user what to
> do in this case, or just do a (user-error "There's no related file.").

OK, I will look closer at this specific use case and see whether
there's some problem in find-file.el in that case.

bug#57325: 27.1; functions in ff-other-file-alist

[Eli Zaretskii]

> Cc: 57325@debbugs.gnu.org
> Date: Mon, 22 Aug 2022 14:44:47 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: Felician Nemeth <felician.nemeth@gmail.com>
> > Cc: 57325@debbugs.gnu.org
> > Date: Sun, 21 Aug 2022 21:37:31 +0200
> > 
> > I got confused because if I remove the
> > (setq uniquify-buffer-name-style nil) line from bug.el, then
> > ff-find-related-file will open the parent directory of xref.el, which
> > feels correct.  However, with that line setting
> > uniquify-buffer-name-style, ff-find-related-file selects /tmp/dir/1234,
> > which feels wrong because that buffer has nothing to do with xref.el.
> > 
> > In my case, a file can contain a link to another file (a .toml file to a
> > schema file).  I wasn't sure what to do when the original file did not
> > contain a link.  Maybe my-find-related-file should ask the user what to
> > do in this case, or just do a (user-error "There's no related file.").
> 
> OK, I will look closer at this specific use case and see whether
> there's some problem in find-file.el in that case.

AFAICT, what you saw is the consequence of one basic problem:
ff-find-the-other-file is unprepared to deal with a function that
returns nil (instead of a list of file-name extensions to try).  So it
tries to use that nil value as if it was a list of extensions, and the
result is basically random.

It should be easy to make ff-find-the-other-file detect the nil value
and handle it as if it found no match for the current buffer's file.
Do you think this would be better?  Or we could simply document that a
function in ff-other-file-alist must return a list of extensions.

WDYT?

bug#57325: 27.1; functions in ff-other-file-alist

[Felician Nemeth]

Eli Zaretskii <eliz@gnu.org> writes:

> AFAICT, what you saw is the consequence of one basic problem:
> ff-find-the-other-file is unprepared to deal with a function that
> returns nil (instead of a list of file-name extensions to try).  So it
> tries to use that nil value as if it was a list of extensions, and the
> result is basically random.
>
> It should be easy to make ff-find-the-other-file detect the nil value
> and handle it as if it found no match for the current buffer's file.
> Do you think this would be better?  Or we could simply document that a
> function in ff-other-file-alist must return a list of extensions.
>
> WDYT?

I think extending the documentation to explain what is expected from the
function is enough.  It would have helped me.  Also, when the function
returns an absolute file name, then the returned file should be already
opened, otherwise ff-find-other-file cannot find it.

Thank you.

bug#57325: 27.1; functions in ff-other-file-alist

[Eli Zaretskii]

> From: Felician Nemeth <felician.nemeth@gmail.com>
> Cc: 57325@debbugs.gnu.org
> Date: Mon, 29 Aug 2022 13:57:44 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > AFAICT, what you saw is the consequence of one basic problem:
> > ff-find-the-other-file is unprepared to deal with a function that
> > returns nil (instead of a list of file-name extensions to try).  So it
> > tries to use that nil value as if it was a list of extensions, and the
> > result is basically random.
> >
> > It should be easy to make ff-find-the-other-file detect the nil value
> > and handle it as if it found no match for the current buffer's file.
> > Do you think this would be better?  Or we could simply document that a
> > function in ff-other-file-alist must return a list of extensions.
> >
> > WDYT?
> 
> I think extending the documentation to explain what is expected from the
> function is enough.  It would have helped me.

I did that now.

> Also, when the function returns an absolute file name, then the
> returned file should be already opened, otherwise ff-find-other-file
> cannot find it.

Hmm... not sure how this is relevant.  The function should return a
list of extensions, not a file name.  What am I missing?

bug#57325: 27.1; functions in ff-other-file-alist

[Felician Nemeth]

>> Also, when the function returns an absolute file name, then the
>> returned file should be already opened, otherwise ff-find-other-file
>> cannot find it.
>
> Hmm... not sure how this is relevant.  The function should return a
> list of extensions, not a file name.  What am I missing?

In my case, the related file of foo.toml is bar.json.  From an extension
it is not possible to guess whether the related file is bar.json or
baz.json.  find-file.el of Emacs 27.1 has this in the Commentary
section:

;; These functions must return a list consisting of the possible names of the
;; corresponding file, with or without path.

That's why I thought my function could return an absolute file name.

bug#57325: 27.1; functions in ff-other-file-alist

[Eli Zaretskii]

> From: Felician Nemeth <felician.nemeth@gmail.com>
> Cc: 57325@debbugs.gnu.org
> Date: Mon, 29 Aug 2022 16:27:13 +0200
> 
> >> Also, when the function returns an absolute file name, then the
> >> returned file should be already opened, otherwise ff-find-other-file
> >> cannot find it.
> >
> > Hmm... not sure how this is relevant.  The function should return a
> > list of extensions, not a file name.  What am I missing?
> 
> In my case, the related file of foo.toml is bar.json.  From an extension
> it is not possible to guess whether the related file is bar.json or
> baz.json.  find-file.el of Emacs 27.1 has this in the Commentary
> section:
> 
> ;; These functions must return a list consisting of the possible names of the
> ;; corresponding file, with or without path.
> 
> That's why I thought my function could return an absolute file name.

If the returned list includes file names with leading directories,
find-file.el strips the leading directories and uses the basename to
match against.

Another thing to clarify in the docs?

bug#57325: 27.1; functions in ff-other-file-alist

[Felician Nemeth]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> >> Also, when the function returns an absolute file name, then the
>> >> returned file should be already opened, otherwise ff-find-other-file
>> >> cannot find it.
>> >
>> > Hmm... not sure how this is relevant.  The function should return a
>> > list of extensions, not a file name.  What am I missing?
>> 
>> In my case, the related file of foo.toml is bar.json.  From an extension
>> it is not possible to guess whether the related file is bar.json or
>> baz.json.  find-file.el of Emacs 27.1 has this in the Commentary
>> section:
>> 
>> ;; These functions must return a list consisting of the possible names of the
>> ;; corresponding file, with or without path.
>> 
>> That's why I thought my function could return an absolute file name.
>
> If the returned list includes file names with leading directories,
> find-file.el strips the leading directories and uses the basename to
> match against.

I think that not how it works.  Even if the attached file is in a
directory that contains a file named "hosts", when I

  1. emacs -Q test.el
  2. y
  3. M-x eval-buffer RET
  4. M-x ff-find-other-file RET

Emacs is going to switch to the buffer containing /etc/hosts.

> Another thing to clarify in the docs?

Playing with the implementation, I think that these functions shouldn't
return extensions.  They should return file names.


[-- Attachment #2: test.el --]
[-- Type: application/emacs-lisp, Size: 175 bytes --]

bug#57325: 27.1; functions in ff-other-file-alist

[Eli Zaretskii]

> From: Felician Nemeth <felician.nemeth@gmail.com>
> Cc: 57325@debbugs.gnu.org
> Date: Tue, 30 Aug 2022 10:46:22 +0200
> 
> > If the returned list includes file names with leading directories,
> > find-file.el strips the leading directories and uses the basename to
> > match against.
> 
> I think that not how it works.

Sorry, I misread the code.  You are right.

But then I think we should replace this code in ff-get-file-name:

              (setq file (concat dir "/" filename))
with
              (setq file (expand-file-name filename dir))

and then the code will work with absolute file names as well, even if
the file is not already visited in a buffer.  Right?

> Playing with the implementation, I think that these functions shouldn't
> return extensions.  They should return file names.

You are right.  I updated the doc string to say that.

bug#57325: 27.1; functions in ff-other-file-alist

[Felician Nemeth]

Eli Zaretskii <eliz@gnu.org> writes:

> But then I think we should replace this code in ff-get-file-name:
>
>               (setq file (concat dir "/" filename))
> with
>               (setq file (expand-file-name filename dir))
>
> and then the code will work with absolute file names as well, even if
> the file is not already visited in a buffer.  Right?

Yes, I think that's right.

The latest documentation still has this:

   Note: if an element of the alist names a FUNCTION as its cdr, that
   function must return a non-nil list of file-name extensions.  It
   cannot return nil, nor can it signal in any way a failure to find a
   suitable list of extensions.

The end of the first sentence should be "file-names."  Then I think this
bug report can be closed.

Thanks again.

bug#57325: 27.1; functions in ff-other-file-alist

[Eli Zaretskii]

> From: Felician Nemeth <felician.nemeth@gmail.com>
> Cc: 57325@debbugs.gnu.org
> Date: Tue, 30 Aug 2022 15:48:44 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > But then I think we should replace this code in ff-get-file-name:
> >
> >               (setq file (concat dir "/" filename))
> > with
> >               (setq file (expand-file-name filename dir))
> >
> > and then the code will work with absolute file names as well, even if
> > the file is not already visited in a buffer.  Right?
> 
> Yes, I think that's right.

Done.

> The latest documentation still has this:
> 
>    Note: if an element of the alist names a FUNCTION as its cdr, that
>    function must return a non-nil list of file-name extensions.  It
>    cannot return nil, nor can it signal in any way a failure to find a
>    suitable list of extensions.
> 
> The end of the first sentence should be "file-names."  Then I think this
> bug report can be closed.

Fixed, and closing.

Thanks a lot for your help.