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)