Re: emacs-30 b585826a65e: ; * lisp/files.el (require-with-check): Fix doc string and error text.

Re: emacs-30 b585826a65e: ; * lisp/files.el (require-with-check): Fix doc string and error text.

[Pip Cet]

"Eli Zaretskii" <eliz@gnu.org> writes:

> branch: emacs-30
> commit b585826a65ebfb58d3fe4744f0f8f9b5f3fc08cc
> Author: Eli Zaretskii <eliz@gnu.org>
> Commit: Eli Zaretskii <eliz@gnu.org>
>
>     ; * lisp/files.el (require-with-check): Fix doc string and error text.
> ---
>  lisp/files.el | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/lisp/files.el b/lisp/files.el
> index 73ad85ce854..dc796fffaa1 100644
> --- a/lisp/files.el
> +++ b/lisp/files.el
> @@ -1258,11 +1258,11 @@ See `load-file' for a different interface to `load'."
>  (defun require-with-check (feature &optional filename noerror)
>    "If FEATURE is not already loaded, load it from FILENAME.
>  This is like `require' except if FEATURE is already a member of the list
> -`features’, then we check if this was provided by a different file than the
> -one that we would load now (presumably because `load-path' has been
> -changed since the file was loaded).
> -If it's the case, we either signal an error (the default), or forcibly reload
> -the new file (if NOERROR is equal to `reload'), or otherwise emit a warning."
> +`features’, then check if it was provided by a different file than the
> +one that is about to be loaded now (presumably because `load-path' has
> +been changed since FILENAME was loaded).  If that is the case, either
> +signal an error (the default), or forcibly reload the new file (if
> +NOERROR is equal to `reload'), or otherwise emit a warning."
>    (let ((lh load-history)
>          (res (require feature filename (if (eq noerror 'reload) nil noerror))))
>      ;; If the `feature' was not yet provided, `require' just loaded the right
> @@ -1275,7 +1275,7 @@ the new file (if NOERROR is equal to `reload'), or otherwise emit a warning."
>           ((assoc fn load-history) nil)  ;We loaded the right file.
>           ((eq noerror 'reload) (load fn nil 'nomessage))
>           (t (funcall (if noerror #'warn #'error)
> -                     "Feature provided by other file: %S" feature)))))
> +                     "Feature `%S' is now provided by a different file %s" fn)))))

That doesn't look right to me. Indeed,

(require-with-check 'nonexistent "nonexistent.el" t)

now throws a "Not enough arguments for format string" error (fn is nil
at that point).

Pip

Re: emacs-30 b585826a65e: ; * lisp/files.el (require-with-check): Fix doc string and error text.

[Eli Zaretskii]

> Date: Mon, 12 Aug 2024 20:25:30 +0000
> From: Pip Cet <pipcet@protonmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>
> 
> That doesn't look right to me. Indeed,
> 
> (require-with-check 'nonexistent "nonexistent.el" t)
> 
> now throws a "Not enough arguments for format string" error (fn is nil
> at that point).

Thanks, fixed.

Re: emacs-30 b585826a65e: ; * lisp/files.el (require-with-check): Fix doc string and error text.

[Pip Cet]

"Eli Zaretskii" <eliz@gnu.org> writes:

>> Date: Mon, 12 Aug 2024 20:25:30 +0000
>> From: Pip Cet <pipcet@protonmail.com>
>> Cc: Eli Zaretskii <eliz@gnu.org>
>>
>> That doesn't look right to me. Indeed,
>>
>> (require-with-check 'nonexistent "nonexistent.el" t)
>>
>> now throws a "Not enough arguments for format string" error (fn is nil
>> at that point).
>
> Thanks, fixed.

I'm not sure "Feature `nonexistent' is now provided by a different file
nil" is a helpful message, to be honest.  In fact, I don't see how 'fn'
is ever a useful addition to the error message; it doesn't tell us the
new file, does it?

Pip

Re: emacs-30 b585826a65e: ; * lisp/files.el (require-with-check): Fix doc string and error text.

[Eli Zaretskii]

> Date: Tue, 13 Aug 2024 07:08:48 +0000
> From: Pip Cet <pipcet@protonmail.com>
> Cc: emacs-devel@gnu.org
> 
> "Eli Zaretskii" <eliz@gnu.org> writes:
> 
> >> Date: Mon, 12 Aug 2024 20:25:30 +0000
> >> From: Pip Cet <pipcet@protonmail.com>
> >> Cc: Eli Zaretskii <eliz@gnu.org>
> >>
> >> That doesn't look right to me. Indeed,
> >>
> >> (require-with-check 'nonexistent "nonexistent.el" t)
> >>
> >> now throws a "Not enough arguments for format string" error (fn is nil
> >> at that point).
> >
> > Thanks, fixed.
> 
> I'm not sure "Feature `nonexistent' is now provided by a different file
> nil" is a helpful message, to be honest.

Is it less helpful than the original one?  It went like this:

  Feature provided by other file: nonexistent

We can, of course, try to make the message even more useful, but it's
strange to hear that you like the original one better.  To me, it is
completely obfuscated (is "nonexistent" the name of a file or is it
something else? and what does "by other file" mean, if "nonexistent"
is not a file name?).

> In fact, I don't see how 'fn' is ever a useful addition to the error
> message; it doesn't tell us the new file, does it?

Yes, it does, AFAIU.

Re: emacs-30 b585826a65e: ; * lisp/files.el (require-with-check): Fix doc string and error text.

[Pip Cet]

"Eli Zaretskii" <eliz@gnu.org> writes:

>> Date: Tue, 13 Aug 2024 07:08:48 +0000
>> From: Pip Cet <pipcet@protonmail.com>
>> Cc: emacs-devel@gnu.org
>>
>> "Eli Zaretskii" <eliz@gnu.org> writes:
>>
>> >> Date: Mon, 12 Aug 2024 20:25:30 +0000
>> >> From: Pip Cet <pipcet@protonmail.com>
>> >> Cc: Eli Zaretskii <eliz@gnu.org>
>> >>
>> >> That doesn't look right to me. Indeed,
>> >>
>> >> (require-with-check 'nonexistent "nonexistent.el" t)
>> >>
>> >> now throws a "Not enough arguments for format string" error (fn is nil
>> >> at that point).
>> >
>> > Thanks, fixed.
>>
>> I'm not sure "Feature `nonexistent' is now provided by a different file
>> nil" is a helpful message, to be honest.
>
> Is it less helpful than the original one?  It went like this:
>
>   Feature provided by other file: nonexistent

That is definitely not a helpful message.

> We can, of course, try to make the message even more useful, but it's
> strange to hear that you like the original one better.

I'm sorry if I said that.  I don't like the original message any better.

> To me, it is
> completely obfuscated (is "nonexistent" the name of a file or is it
> something else? and what does "by other file" mean, if "nonexistent"
> is not a file name?).

I think the main problem is the "by other file" or "by a different file",
which suggests we know that there is another file providing the
'nonexistent feature. We don't, of course, because there isn't.

The old message, I think, meant to say

Feature FEATURE has already been provided by a different file than
(require FEATURE FILENAME) would attempt to load now. We won't tell you
which file that is.

>> In fact, I don't see how 'fn' is ever a useful addition to the error
>> message; it doesn't tell us the new file, does it?
>
> Yes, it does, AFAIU.

We don't know whether we found a file at all (fn might be nil) and, even
if we found one, we don't know whether it provides the feature.

Pip

Re: emacs-30 b585826a65e: ; * lisp/files.el (require-with-check): Fix doc string and error text.

[Eli Zaretskii]

> Date: Tue, 13 Aug 2024 16:08:34 +0000
> From: Pip Cet <pipcet@protonmail.com>
> Cc: emacs-devel@gnu.org, Stefan Monnier <monnier@iro.umontreal.ca>
> 
> > To me, it is
> > completely obfuscated (is "nonexistent" the name of a file or is it
> > something else? and what does "by other file" mean, if "nonexistent"
> > is not a file name?).
> 
> I think the main problem is the "by other file" or "by a different file",
> which suggests we know that there is another file providing the
> 'nonexistent feature. We don't, of course, because there isn't.
> 
> The old message, I think, meant to say
> 
> Feature FEATURE has already been provided by a different file than
> (require FEATURE FILENAME) would attempt to load now. We won't tell you
> which file that is.
> 
> >> In fact, I don't see how 'fn' is ever a useful addition to the error
> >> message; it doesn't tell us the new file, does it?
> >
> > Yes, it does, AFAIU.
> 
> We don't know whether we found a file at all (fn might be nil) and, even
> if we found one, we don't know whether it provides the feature.

I'm not sure I see any direction here towards improving the
diagnostic.  Did you mean to suggest something concrete?

Re: emacs-30 b585826a65e: ; * lisp/files.el (require-with-check): Fix doc string and error text.

[Pip Cet]

"Eli Zaretskii" <eliz@gnu.org> writes:

>> Date: Tue, 13 Aug 2024 16:08:34 +0000
>> From: Pip Cet <pipcet@protonmail.com>
>> Cc: emacs-devel@gnu.org, Stefan Monnier <monnier@iro.umontreal.ca>
>>
>> > To me, it is
>> > completely obfuscated (is "nonexistent" the name of a file or is it
>> > something else? and what does "by other file" mean, if "nonexistent"
>> > is not a file name?).
>>
>> I think the main problem is the "by other file" or "by a different file",
>> which suggests we know that there is another file providing the
>> 'nonexistent feature. We don't, of course, because there isn't.
>>
>> The old message, I think, meant to say
>>
>> Feature FEATURE has already been provided by a different file than
>> (require FEATURE FILENAME) would attempt to load now. We won't tell you
>> which file that is.
>>
>> >> In fact, I don't see how 'fn' is ever a useful addition to the error
>> >> message; it doesn't tell us the new file, does it?
>> >
>> > Yes, it does, AFAIU.
>>
>> We don't know whether we found a file at all (fn might be nil) and, even
>> if we found one, we don't know whether it provides the feature.
>
> I'm not sure I see any direction here towards improving the
> diagnostic.  Did you mean to suggest something concrete?

How about just saying what we know, that loading the file failed to
provide the feature, for whatever reason?  Alternatively, we can keep
your message, like this, but restrict it to when we (almost) know it's
accurate.

diff --git a/lisp/files.el b/lisp/files.el
index eadb4a9d0b1..629e7841af4 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -1274,9 +1274,14 @@ require-with-check
         (cond
          ((assoc fn load-history) nil)  ;We loaded the right file.
          ((eq noerror 'reload) (load fn nil 'nomessage))
-         (t (funcall (if noerror #'warn #'error)
-                     "Feature `%S' is now provided by a different file %s"
-                     feature fn)))))
+         ((and fn (memq feature features))
+          (funcall (if noerror #'warn #'error)
+                   "Feature `%S' is now provided by a different file %s"
+                   feature fn))
+         ((funcall (if noerror #'warn #'error)
+                   "Could not load file %s to provide feature `%S'"
+                   (or filename (symbol-name feature))
+                   feature)))))
     res))
 
 (defun file-remote-p (file &optional identification connected)

Re: emacs-30 b585826a65e: ; * lisp/files.el (require-with-check): Fix doc string and error text.

[Eli Zaretskii]

> Date: Tue, 13 Aug 2024 17:28:09 +0000
> From: Pip Cet <pipcet@protonmail.com>
> Cc: emacs-devel@gnu.org, monnier@iro.umontreal.ca
> 
> "Eli Zaretskii" <eliz@gnu.org> writes:
> 
> > I'm not sure I see any direction here towards improving the
> > diagnostic.  Did you mean to suggest something concrete?
> 
> How about just saying what we know, that loading the file failed to
> provide the feature, for whatever reason?  Alternatively, we can keep
> your message, like this, but restrict it to when we (almost) know it's
> accurate.
> 
> diff --git a/lisp/files.el b/lisp/files.el
> index eadb4a9d0b1..629e7841af4 100644
> --- a/lisp/files.el
> +++ b/lisp/files.el
> @@ -1274,9 +1274,14 @@ require-with-check
>          (cond
>           ((assoc fn load-history) nil)  ;We loaded the right file.
>           ((eq noerror 'reload) (load fn nil 'nomessage))
> -         (t (funcall (if noerror #'warn #'error)
> -                     "Feature `%S' is now provided by a different file %s"
> -                     feature fn)))))
> +         ((and fn (memq feature features))
> +          (funcall (if noerror #'warn #'error)
> +                   "Feature `%S' is now provided by a different file %s"
> +                   feature fn))
> +         ((funcall (if noerror #'warn #'error)
> +                   "Could not load file %s to provide feature `%S'"
> +                   (or filename (symbol-name feature))
> +                   feature)))))

The second message is not accurate, AFAIU.  If NOEROR is equal to
'reload', and the file for FEATURE could not be found,
require-with-check will signal an error from here:

  (let ((lh load-history)
        (res (require feature filename (if (eq noerror 'reload) nil noerror))))

Also, if FILENAME was found, but it doesn't provide FEATURE, the
message text again doesn't provide accurate diagnostic.

So I think we need 3 different messages, not 2.

Re: emacs-30 b585826a65e: ; * lisp/files.el (require-with-check): Fix doc string and error text.

[Pip Cet]

"Eli Zaretskii" <eliz@gnu.org> writes:

>> Date: Tue, 13 Aug 2024 17:28:09 +0000
>> From: Pip Cet <pipcet@protonmail.com>
>> Cc: emacs-devel@gnu.org, monnier@iro.umontreal.ca
>>
>> "Eli Zaretskii" <eliz@gnu.org> writes:
>>
>> > I'm not sure I see any direction here towards improving the
>> > diagnostic.  Did you mean to suggest something concrete?
>>
>> How about just saying what we know, that loading the file failed to
>> provide the feature, for whatever reason?  Alternatively, we can keep
>> your message, like this, but restrict it to when we (almost) know it's
>> accurate.
>>
>> diff --git a/lisp/files.el b/lisp/files.el
>> index eadb4a9d0b1..629e7841af4 100644
>> --- a/lisp/files.el
>> +++ b/lisp/files.el
>> @@ -1274,9 +1274,14 @@ require-with-check
>>          (cond
>>           ((assoc fn load-history) nil)  ;We loaded the right file.
>>           ((eq noerror 'reload) (load fn nil 'nomessage))
>> -         (t (funcall (if noerror #'warn #'error)
>> -                     "Feature `%S' is now provided by a different file %s"
>> -                     feature fn)))))
>> +         ((and fn (memq feature features))
>> +          (funcall (if noerror #'warn #'error)
>> +                   "Feature `%S' is now provided by a different file %s"
>> +                   feature fn))
>> +         ((funcall (if noerror #'warn #'error)
>> +                   "Could not load file %s to provide feature `%S'"
>> +                   (or filename (symbol-name feature))
>> +                   feature)))))
>
> The second message is not accurate, AFAIU.

Can you provide an example for a case in which the message is emitted
but is not accurate?

> If NOEROR is equal to
> 'reload', and the file for FEATURE could not be found,
> require-with-check will signal an error from here:
>
>   (let ((lh load-history)
>         (res (require feature filename (if (eq noerror 'reload) nil noerror))))

Correct, so we would never reach the code emitting the new message.

> Also, if FILENAME was found, but it doesn't provide FEATURE, the
> message text again doesn't provide accurate diagnostic.

If FILENAME exists and is loadable, but doesn't provide FEATURE,
'require' will throw an error (even if NOERROR is t).  So, again, we'd
never reach the code emitting the new message.

> So I think we need 3 different messages, not 2.

I don't understand what the third message would be.

Pip

Re: emacs-30 b585826a65e: ; * lisp/files.el (require-with-check): Fix doc string and error text.

[Eli Zaretskii]

> Date: Wed, 14 Aug 2024 05:56:21 +0000
> From: Pip Cet <pipcet@protonmail.com>
> Cc: emacs-devel@gnu.org, monnier@iro.umontreal.ca
> 
> "Eli Zaretskii" <eliz@gnu.org> writes:
> 
> >> +         ((and fn (memq feature features))
> >> +          (funcall (if noerror #'warn #'error)
> >> +                   "Feature `%S' is now provided by a different file %s"
> >> +                   feature fn))
> >> +         ((funcall (if noerror #'warn #'error)
> >> +                   "Could not load file %s to provide feature `%S'"
> >> +                   (or filename (symbol-name feature))
> >> +                   feature)))))
> >
> > The second message is not accurate, AFAIU.
> 
> Can you provide an example for a case in which the message is emitted
> but is not accurate?

When fn is nil.

> > If NOEROR is equal to
> > 'reload', and the file for FEATURE could not be found,
> > require-with-check will signal an error from here:
> >
> >   (let ((lh load-history)
> >         (res (require feature filename (if (eq noerror 'reload) nil noerror))))
> 
> Correct, so we would never reach the code emitting the new message.

But the message text mainly describes this situation: "Could not load
file".

> > Also, if FILENAME was found, but it doesn't provide FEATURE, the
> > message text again doesn't provide accurate diagnostic.
> 
> If FILENAME exists and is loadable, but doesn't provide FEATURE,
> 'require' will throw an error (even if NOERROR is t).  So, again, we'd
> never reach the code emitting the new message.

And again, the error message will not describe the situation.

> > So I think we need 3 different messages, not 2.
> 
> I don't understand what the third message would be.

For starters, that no FILENAME could be found for FEATURE.

Re: emacs-30 b585826a65e: ; * lisp/files.el (require-with-check): Fix doc string and error text.

[Pip Cet]

"Eli Zaretskii" <eliz@gnu.org> writes:

>> Date: Wed, 14 Aug 2024 05:56:21 +0000
>> From: Pip Cet <pipcet@protonmail.com>
>> Cc: emacs-devel@gnu.org, monnier@iro.umontreal.ca
>>
>> "Eli Zaretskii" <eliz@gnu.org> writes:
>>
>> >> +         ((and fn (memq feature features))
>> >> +          (funcall (if noerror #'warn #'error)
>> >> +                   "Feature `%S' is now provided by a different file %s"
>> >> +                   feature fn))
>> >> +         ((funcall (if noerror #'warn #'error)
>> >> +                   "Could not load file %s to provide feature `%S'"
>> >> +                   (or filename (symbol-name feature))
>> >> +                   feature)))))
>> >
>> > The second message is not accurate, AFAIU.
>>
>> Can you provide an example for a case in which the message is emitted
>> but is not accurate?
>
> When fn is nil.

How is it inaccurate then? It states which file we tried to load, that
we couldn't, and which feature we were looking for. If you think it
should be more precise, in stating that the reason we couldn't load the
file is that it is missing (more accurately, that it couldn't be
located), I'm perfectly okay with that version, though.

>> > If NOEROR is equal to
>> > 'reload', and the file for FEATURE could not be found,
>> > require-with-check will signal an error from here:
>> >
>> >   (let ((lh load-history)
>> >         (res (require feature filename (if (eq noerror 'reload) nil noerror))))
>>
>> Correct, so we would never reach the code emitting the new message.
>
> But the message text mainly describes this situation: "Could not load
> file".

Yes, the message text also applies to situations in which it isn't
printed, but a more precise message is.

>> > Also, if FILENAME was found, but it doesn't provide FEATURE, the
>> > message text again doesn't provide accurate diagnostic.
>>
>> If FILENAME exists and is loadable, but doesn't provide FEATURE,
>> 'require' will throw an error (even if NOERROR is t).  So, again, we'd
>> never reach the code emitting the new message.
>
> And again, the error message will not describe the situation.

I think the error message that is actually printed describes the
situation fairly well:

     error ("Loading file %s failed to provide feature `%s'",

>> > So I think we need 3 different messages, not 2.
>>
>> I don't understand what the third message would be.
>
> For starters, that no FILENAME could be found for FEATURE.

How's this?

diff --git a/lisp/files.el b/lisp/files.el
index eadb4a9d0b1..6749056fe64 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -1274,9 +1274,17 @@ require-with-check
         (cond
          ((assoc fn load-history) nil)  ;We loaded the right file.
          ((eq noerror 'reload) (load fn nil 'nomessage))
-         (t (funcall (if noerror #'warn #'error)
-                     "Feature `%S' is now provided by a different file %s"
-                     feature fn)))))
+         ((and fn (memq feature features))
+          (funcall (if noerror #'warn #'error)
+                   "Feature `%S' is now provided by a different file %s"
+                   feature fn))
+         (fn
+          (funcall (if noerror #'warn #'error)
+                   "Could not load file %s to provide feature `%S'"
+                   fn feature))
+         ((funcall (if noerror #'warn #'error)
+                   "Could not locate file %s in load path"
+                   (or filename (symbol-name feature)))))))
     res))
 
 (defun file-remote-p (file &optional identification connected)

Re: emacs-30 b585826a65e: ; * lisp/files.el (require-with-check): Fix doc string and error text.

[Eli Zaretskii]

> Date: Wed, 14 Aug 2024 14:22:20 +0000
> From: Pip Cet <pipcet@protonmail.com>
> Cc: emacs-devel@gnu.org, monnier@iro.umontreal.ca
> 
> "Eli Zaretskii" <eliz@gnu.org> writes:
> 
> >> Date: Wed, 14 Aug 2024 05:56:21 +0000
> >> From: Pip Cet <pipcet@protonmail.com>
> >> Cc: emacs-devel@gnu.org, monnier@iro.umontreal.ca
> >>
> >> "Eli Zaretskii" <eliz@gnu.org> writes:
> >>
> >> >> +         ((and fn (memq feature features))
> >> >> +          (funcall (if noerror #'warn #'error)
> >> >> +                   "Feature `%S' is now provided by a different file %s"
> >> >> +                   feature fn))
> >> >> +         ((funcall (if noerror #'warn #'error)
> >> >> +                   "Could not load file %s to provide feature `%S'"
> >> >> +                   (or filename (symbol-name feature))
> >> >> +                   feature)))))
> >> >
> >> > The second message is not accurate, AFAIU.
> >>
> >> Can you provide an example for a case in which the message is emitted
> >> but is not accurate?
> >
> > When fn is nil.
> 
> How is it inaccurate then?

Oh, come on!  We will then say

  Could not load file WHATSITSNAME to provide feature `FOOBAR'

Is that even helpful, let alone accurate?  The file couldn't be found,
so its name is a myth, it most probably doesn't exist.  We didn't even
try loading it, so "Could not load" is a lie.  We should instead say
that we couldn't find FILENAME, which is all we know.

> diff --git a/lisp/files.el b/lisp/files.el
> index eadb4a9d0b1..6749056fe64 100644
> --- a/lisp/files.el
> +++ b/lisp/files.el
> @@ -1274,9 +1274,17 @@ require-with-check
>          (cond
>           ((assoc fn load-history) nil)  ;We loaded the right file.
>           ((eq noerror 'reload) (load fn nil 'nomessage))
> -         (t (funcall (if noerror #'warn #'error)
> -                     "Feature `%S' is now provided by a different file %s"
> -                     feature fn)))))
> +         ((and fn (memq feature features))
> +          (funcall (if noerror #'warn #'error)
> +                   "Feature `%S' is now provided by a different file %s"
> +                   feature fn))
> +         (fn
> +          (funcall (if noerror #'warn #'error)
> +                   "Could not load file %s to provide feature `%S'"

"Could not load file to provide feature" is ambiguous wrt what
exactly failed: the loading or the providing.

> +                   fn feature))
> +         ((funcall (if noerror #'warn #'error)
> +                   "Could not locate file %s in load path"
> +                   (or filename (symbol-name feature)))))))

You've lost t there.

Re: emacs-30 b585826a65e: ; * lisp/files.el (require-with-check): Fix doc string and error text.

[Pip Cet]

"Eli Zaretskii" <eliz@gnu.org> writes:

>> Date: Wed, 14 Aug 2024 14:22:20 +0000
>> From: Pip Cet <pipcet@protonmail.com>
>> Cc: emacs-devel@gnu.org, monnier@iro.umontreal.ca
>>
>> "Eli Zaretskii" <eliz@gnu.org> writes:
>>
>> >> Date: Wed, 14 Aug 2024 05:56:21 +0000
>> >> From: Pip Cet <pipcet@protonmail.com>
>> >> Cc: emacs-devel@gnu.org, monnier@iro.umontreal.ca
>> >>
>> >> "Eli Zaretskii" <eliz@gnu.org> writes:
>> >>
>> >> >> +         ((and fn (memq feature features))
>> >> >> +          (funcall (if noerror #'warn #'error)
>> >> >> +                   "Feature `%S' is now provided by a different file %s"
>> >> >> +                   feature fn))
>> >> >> +         ((funcall (if noerror #'warn #'error)
>> >> >> +                   "Could not load file %s to provide feature `%S'"
>> >> >> +                   (or filename (symbol-name feature))
>> >> >> +                   feature)))))
>> >> >
>> >> > The second message is not accurate, AFAIU.
>> >>
>> >> Can you provide an example for a case in which the message is emitted
>> >> but is not accurate?
>> >
>> > When fn is nil.
>>
>> How is it inaccurate then?
>
> Oh, come on!  We will then say
>
>   Could not load file WHATSITSNAME to provide feature `FOOBAR'
>
> Is that even helpful, let alone accurate?  The file couldn't be found,
> so its name is a myth, it most probably doesn't exist.

Indeed it doesn't, which is why we should let the user know which file
we tried, and why.  So, yes, I'd consider that helpful, but not as
helpful as the improved message you suggested.

> We didn't even
> try loading it, so "Could not load" is a lie.

(I think calling Fload counts as trying to load it, even if it returned
nil.)

> We should instead say
> that we couldn't find FILENAME, which is all we know.

Again, no objections there, but it is a stronger statement than the one
we're replacing.

>> diff --git a/lisp/files.el b/lisp/files.el
>> index eadb4a9d0b1..6749056fe64 100644
>> --- a/lisp/files.el
>> +++ b/lisp/files.el
>> @@ -1274,9 +1274,17 @@ require-with-check
>>          (cond
>>           ((assoc fn load-history) nil)  ;We loaded the right file.
>>           ((eq noerror 'reload) (load fn nil 'nomessage))
>> -         (t (funcall (if noerror #'warn #'error)
>> -                     "Feature `%S' is now provided by a different file %s"
>> -                     feature fn)))))
>> +         ((and fn (memq feature features))
>> +          (funcall (if noerror #'warn #'error)
>> +                   "Feature `%S' is now provided by a different file %s"
>> +                   feature fn))
>> +         (fn
>> +          (funcall (if noerror #'warn #'error)
>> +                   "Could not load file %s to provide feature `%S'"
>
> "Could not load file to provide feature" is ambiguous wrt what
> exactly failed: the loading or the providing.

Correct.  Reading over the Frequire code, I think we can make that
unambiguous and say it is the load that failed.

>
>> +                   fn feature))
>> +         ((funcall (if noerror #'warn #'error)
>> +                   "Could not locate file %s in load path"
>> +                   (or filename (symbol-name feature)))))))
>
> You've lost t there.

Thanks, fixed.  Tangentially-related question:

Are we moving to the more verbose (but arguably clearer) form of

(cond ... (t ELSE-CLAUSE))

rather than the old-fashioned

(cond ... (ELSE-CLAUSE))

?  I'll try to make sure I use the former, then, of course.

diff --git a/lisp/files.el b/lisp/files.el
index eadb4a9d0b1..6cbb1b5c632 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -1274,9 +1274,17 @@ require-with-check
         (cond
          ((assoc fn load-history) nil)  ;We loaded the right file.
          ((eq noerror 'reload) (load fn nil 'nomessage))
-         (t (funcall (if noerror #'warn #'error)
-                     "Feature `%S' is now provided by a different file %s"
-                     feature fn)))))
+         ((and fn (memq feature features))
+          (funcall (if noerror #'warn #'error)
+                   "Feature `%S' is now provided by a different file %s"
+                   feature fn))
+         (fn
+          (funcall (if noerror #'warn #'error)
+                   "Could not load file %s" fn))
+         (t
+          (funcall (if noerror #'warn #'error)
+                   "Could not locate file %s in load path"
+                   (or filename (symbol-name feature)))))))
     res))
 
 (defun file-remote-p (file &optional identification connected)

Pip

Re: emacs-30 b585826a65e: ; * lisp/files.el (require-with-check): Fix doc string and error text.

[Eli Zaretskii]

> Date: Wed, 14 Aug 2024 16:31:53 +0000
> From: Pip Cet <pipcet@protonmail.com>
> Cc: emacs-devel@gnu.org, monnier@iro.umontreal.ca
> 
> >> +                   fn feature))
> >> +         ((funcall (if noerror #'warn #'error)
> >> +                   "Could not locate file %s in load path"
> >> +                   (or filename (symbol-name feature)))))))
> >
> > You've lost t there.
> 
> Thanks, fixed.  Tangentially-related question:
> 
> Are we moving to the more verbose (but arguably clearer) form of
> 
> (cond ... (t ELSE-CLAUSE))
> 
> rather than the old-fashioned
> 
> (cond ... (ELSE-CLAUSE))
> 
> ?  I'll try to make sure I use the former, then, of course.

I prefer to have an explicit t, yes.  I think most, if not all, of our
sources use that.

> diff --git a/lisp/files.el b/lisp/files.el
> index eadb4a9d0b1..6cbb1b5c632 100644
> --- a/lisp/files.el
> +++ b/lisp/files.el
> @@ -1274,9 +1274,17 @@ require-with-check
>          (cond
>           ((assoc fn load-history) nil)  ;We loaded the right file.
>           ((eq noerror 'reload) (load fn nil 'nomessage))
> -         (t (funcall (if noerror #'warn #'error)
> -                     "Feature `%S' is now provided by a different file %s"
> -                     feature fn)))))
> +         ((and fn (memq feature features))
> +          (funcall (if noerror #'warn #'error)
> +                   "Feature `%S' is now provided by a different file %s"
> +                   feature fn))
> +         (fn
> +          (funcall (if noerror #'warn #'error)
> +                   "Could not load file %s" fn))
> +         (t
> +          (funcall (if noerror #'warn #'error)
> +                   "Could not locate file %s in load path"
> +                   (or filename (symbol-name feature)))))))
>      res))
>  
>  (defun file-remote-p (file &optional identification connected)

LGTM, thanks.

Re: emacs-30 b585826a65e: ; * lisp/files.el (require-with-check): Fix doc string and error text.

[Pip Cet]

"Eli Zaretskii" <eliz@gnu.org> writes:

>> Date: Wed, 14 Aug 2024 16:31:53 +0000
>> From: Pip Cet <pipcet@protonmail.com>
>> Cc: emacs-devel@gnu.org, monnier@iro.umontreal.ca
>>
>> >> +                   fn feature))
>> >> +         ((funcall (if noerror #'warn #'error)
>> >> +                   "Could not locate file %s in load path"
>> >> +                   (or filename (symbol-name feature)))))))
>> >
>> > You've lost t there.
>>
>> Thanks, fixed.  Tangentially-related question:
>>
>> Are we moving to the more verbose (but arguably clearer) form of
>>
>> (cond ... (t ELSE-CLAUSE))
>>
>> rather than the old-fashioned
>>
>> (cond ... (ELSE-CLAUSE))
>>
>> ?  I'll try to make sure I use the former, then, of course.
>
> I prefer to have an explicit t, yes.  I think most, if not all, of our
> sources use that.

Certainly not all of them. It's easy enough to find counterexamples with
a heuristic such as:

diff --git a/lisp/emacs-lisp/byte-opt.el b/lisp/emacs-lisp/byte-opt.el
index d8dbfa62bf9..8dd49410cc3 100644
--- a/lisp/emacs-lisp/byte-opt.el
+++ b/lisp/emacs-lisp/byte-opt.el
@@ -1331,6 +1331,10 @@ byte-optimize-cond
     (setq form (remq nil form))
     (setq rest form)
     (while (setq rest (cdr rest))
+      (and (car rest)
+           (not (cdr-safe rest))
+           (not (cdr-safe (car rest)))
+           (message "singleton clause %S" (car rest)))
       (cond ((byte-compile-trueconstp (car-safe (car rest)))
              ;; This branch will always be taken: kill the subsequent ones.
 	     (cond ((eq rest (cdr form)) ;First branch of `cond'.

That finds three non-explicit 't's in files.el, for example.

>> diff --git a/lisp/files.el b/lisp/files.el
>> index eadb4a9d0b1..6cbb1b5c632 100644
>> --- a/lisp/files.el
>> +++ b/lisp/files.el
>> @@ -1274,9 +1274,17 @@ require-with-check
>>          (cond
>>           ((assoc fn load-history) nil)  ;We loaded the right file.
>>           ((eq noerror 'reload) (load fn nil 'nomessage))
>> -         (t (funcall (if noerror #'warn #'error)
>> -                     "Feature `%S' is now provided by a different file %s"
>> -                     feature fn)))))
>> +         ((and fn (memq feature features))
>> +          (funcall (if noerror #'warn #'error)
>> +                   "Feature `%S' is now provided by a different file %s"
>> +                   feature fn))
>> +         (fn
>> +          (funcall (if noerror #'warn #'error)
>> +                   "Could not load file %s" fn))
>> +         (t
>> +          (funcall (if noerror #'warn #'error)
>> +                   "Could not locate file %s in load path"
>> +                   (or filename (symbol-name feature)))))))
>>      res))
>>
>>  (defun file-remote-p (file &optional identification connected)
>
> LGTM, thanks.

Is this okay for emacs-30 or master? (I'd prefer just applying this to
master for now)

Pip

Re: emacs-30 b585826a65e: ; * lisp/files.el (require-with-check): Fix doc string and error text.

[Eli Zaretskii]

> Date: Thu, 15 Aug 2024 16:13:58 +0000
> From: Pip Cet <pipcet@protonmail.com>
> Cc: emacs-devel@gnu.org, monnier@iro.umontreal.ca
> 
> >> diff --git a/lisp/files.el b/lisp/files.el
> >> index eadb4a9d0b1..6cbb1b5c632 100644
> >> --- a/lisp/files.el
> >> +++ b/lisp/files.el
> >> @@ -1274,9 +1274,17 @@ require-with-check
> >>          (cond
> >>           ((assoc fn load-history) nil)  ;We loaded the right file.
> >>           ((eq noerror 'reload) (load fn nil 'nomessage))
> >> -         (t (funcall (if noerror #'warn #'error)
> >> -                     "Feature `%S' is now provided by a different file %s"
> >> -                     feature fn)))))
> >> +         ((and fn (memq feature features))
> >> +          (funcall (if noerror #'warn #'error)
> >> +                   "Feature `%S' is now provided by a different file %s"
> >> +                   feature fn))
> >> +         (fn
> >> +          (funcall (if noerror #'warn #'error)
> >> +                   "Could not load file %s" fn))
> >> +         (t
> >> +          (funcall (if noerror #'warn #'error)
> >> +                   "Could not locate file %s in load path"
> >> +                   (or filename (symbol-name feature)))))))
> >>      res))
> >>
> >>  (defun file-remote-p (file &optional identification connected)
> >
> > LGTM, thanks.
> 
> Is this okay for emacs-30 or master? (I'd prefer just applying this to
> master for now)

Why not emacs-30?  It just improves the error message text, that's
all.