bug#71379: 29.3; Elisp compiler: warnings for formats given insufficiently many arguments

bug#71379: 29.3; Elisp compiler: warnings for formats given insufficiently many arguments

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

The following example code has been put in my ~/foo.el

(defun check-value (v)
   ;; Let's demonstrate different treament of formats by elisp compiler
   (cond (v
	 (print (format "Congratulations! v=%S is non-NIL."))
	 ;; oops, we forgot to pass v in `format's &rest args
	 t)
	(t ;; otherwise
	 (user-error "Try again. v=%S is null")
	 ;; same error, here when calling `user-error'
	 nil)))


When I byte-compile it with M-x byte-compile-file I get the following 
warning message:

      In check-value:
      foo.el:4:18: Warning: ‘format’ called with 0 args to fill 1 format 
field(s)

which is very useful.

But why don't I get a similar error message for my call to user-error ? 
It would be useful too.

Note that when I evaluate (check-value nil) I get an error with the 
following *Backtrace*, showing that my call to
`user-error' fails for exactly the same reason that evaluating 
`(check-value t)' fails.

Debugger entered--Lisp error: (error "Not enough arguments for format 
string")
   format-message("Try again. v=%S is null")
   apply(format-message "Try again. v=%S is null" nil)
   user-error("Try again. v=%S is null")
   (if v (print (format "Congratulations! v=%S is non-NIL.")) 
(user-error "Try again. v=%S is null"))
   check-value(nil)
   (progn (check-value nil))
   eval((progn (check-value nil)) t)
   elisp--eval-last-sexp(t)
   eval-last-sexp(t)
   eval-print-last-sexp(nil)
   funcall-interactively(eval-print-last-sexp nil)
   call-interactively(eval-print-last-sexp nil nil)

I thought you'd like to know.

Thanks for your time and dedication in developing GNU Emacs !

--philippe

In GNU Emacs 29.3 (build 1, aarch64-apple-darwin21.6.0, NS 
appkit-2113.60 Version 12.6.6 (Build 21G646)) of
  2024-03-24 built on armbob.lan
Windowing system distributor 'Apple', version 10.3.2487
System Description:  macOS 14.5

Configured using:
  'configure --with-ns '--enable-locallisppath=/Library/Application
  Support/Emacs/${version}/site-lisp:/Library/Application 
Support/Emacs/site-lisp' --with-modules
  'CFLAGS=-DFD_SETSIZE=10000 -DDARWIN_UNLIMITED_SELECT' --with-x-toolkit=no'

bug#71379: 29.3; Elisp compiler: warnings for formats given insufficiently many arguments

[Mattias Engdegård]

I stumbled on this report by accident:

> But why don't I get a similar error message for my call to user-error ?

That's a clear oversight, thank you for reporting it. Will fix.

Should be trivial enough for master (and I have made a full bootstrap) but I'm asking Eli for confirmation just in case.

bug#71379: 29.3; Elisp compiler: warnings for formats given insufficiently many arguments

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Wed, 5 Jun 2024 18:56:52 +0200
> Cc: 71379@debbugs.gnu.org,
>  Eli Zaretskii <eliz@gnu.org>
> 
> I stumbled on this report by accident:
> 
> > But why don't I get a similar error message for my call to user-error ?
> 
> That's a clear oversight, thank you for reporting it. Will fix.
> 
> Should be trivial enough for master (and I have made a full bootstrap) but I'm asking Eli for confirmation just in case.

Thanks, I'd like to see the changes before making up my mind about
that.  The problem is definitely not serious, so the fix should be
safe enough to qualify.

bug#71379: 29.3; Elisp compiler: warnings for formats given insufficiently many arguments

[Mattias Engdegård]

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

5 juni 2024 kl. 19.48 skrev Eli Zaretskii <eliz@gnu.org>:

> Thanks, I'd like to see the changes before making up my mind about
> that.  The problem is definitely not serious, so the fix should be
> safe enough to qualify.

Sorry, meant to attach the patch.


[-- Attachment #2: 0001-Format-string-warnings-for-more-functions-bug-71379.patch --]
[-- Type: application/octet-stream, Size: 1098 bytes --]

From 6f758e82a37a6b62abc71459c8878de7217834ae Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Wed, 5 Jun 2024 18:28:59 +0200
Subject: [PATCH] Format string warnings for more functions (bug#71379)

* lisp/emacs-lisp/bytecomp.el (byte-compile-format-like):
Add message-box, message-or-box, warn and user-error.
---
 lisp/emacs-lisp/bytecomp.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index 03cfbe6f4c9..d59e89bd7de 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -1643,7 +1643,8 @@ byte-compile-format-warn
          nargs (if (= nargs 1) "" "s")
          nfields (if (= nfields 1) "" "s"))))))
 
-(dolist (elt '(format message format-message error))
+(dolist (elt '( format message format-message message-box message-or-box
+                warn error user-error))
   (put elt 'byte-compile-format-like t))
 
 ;; Warn if the function or macro is being redefined with a different
-- 
2.32.0 (Apple Git-132)

bug#71379: 29.3; Elisp compiler: warnings for formats given insufficiently many arguments

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Wed, 5 Jun 2024 21:20:53 +0200
> Cc: schnoebelen.ph@gmail.com,
>  71379@debbugs.gnu.org
> 
> 5 juni 2024 kl. 19.48 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > Thanks, I'd like to see the changes before making up my mind about
> > that.  The problem is definitely not serious, so the fix should be
> > safe enough to qualify.
> 
> Sorry, meant to attach the patch.

On second thought, this is still fine to go in now, but I wonder
whether we should move these to the places where the corresponding
functions are defined, and leave in bytecomp.c only those which are
primitives defined in C?  That would mean 'warn' and 'user-error'
should be moved to their respective Lisp files.  WDYT?

bug#71379: 29.3; Elisp compiler: warnings for formats given insufficiently many arguments

[Mattias Engdegård]

6 juni 2024 kl. 06.38 skrev Eli Zaretskii <eliz@gnu.org>:

> On second thought, this is still fine to go in now, but I wonder
> whether we should move these to the places where the corresponding
> functions are defined, and leave in bytecomp.c only those which are
> primitives defined in C?  That would mean 'warn' and 'user-error'
> should be moved to their respective Lisp files.

Indeed it's a question I always ask myself every so often, and sometimes we distribute properties to definitions. 
However, in this case it seems better to keep them in one place: it's only a small handful of functions, and this way the properties are kept local to the byte-compiler.

Thank you, the change is now on master. Closing.

bug#71379: 29.3; Elisp compiler: warnings for formats given insufficiently many arguments

[Stefan Kangas]

Mattias Engdegård <mattias.engdegard@gmail.com> writes:

> 6 juni 2024 kl. 06.38 skrev Eli Zaretskii <eliz@gnu.org>:
>
>> On second thought, this is still fine to go in now, but I wonder
>> whether we should move these to the places where the corresponding
>> functions are defined, and leave in bytecomp.c only those which are
>> primitives defined in C?  That would mean 'warn' and 'user-error'
>> should be moved to their respective Lisp files.
>
> Indeed it's a question I always ask myself every so often, and sometimes we distribute properties to definitions.
> However, in this case it seems better to keep them in one place: it's only a small handful of functions, and this way the properties are kept local to the byte-compiler.

No strong opinion, but I think this cuts both ways: keeping it local to
the definition of the functions themselves makes it clear to anyone
looking there for inspiration how to add such warnings to their own
`format` functions.

bug#71379: 29.3; Elisp compiler: warnings for formats given insufficiently many arguments

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Fri, 7 Jun 2024 01:22:49 +0200
> Cc: 71379@debbugs.gnu.org, philippe schnoebelen <schnoebelen.ph@gmail.com>
> 
> Mattias Engdegård <mattias.engdegard@gmail.com> writes:
> 
> > 6 juni 2024 kl. 06.38 skrev Eli Zaretskii <eliz@gnu.org>:
> >
> >> On second thought, this is still fine to go in now, but I wonder
> >> whether we should move these to the places where the corresponding
> >> functions are defined, and leave in bytecomp.c only those which are
> >> primitives defined in C?  That would mean 'warn' and 'user-error'
> >> should be moved to their respective Lisp files.
> >
> > Indeed it's a question I always ask myself every so often, and sometimes we distribute properties to definitions.
> > However, in this case it seems better to keep them in one place: it's only a small handful of functions, and this way the properties are kept local to the byte-compiler.
> 
> No strong opinion, but I think this cuts both ways: keeping it local to
> the definition of the functions themselves makes it clear to anyone
> looking there for inspiration how to add such warnings to their own
> `format` functions.

Indeed.  In addition, some functions marked with this property already
do that in their own files.  Which was why I asked that question in
the first place.

bug#71379: 29.3; Elisp compiler: warnings for formats given insufficiently many arguments

[Mattias Engdegård]

7 juni 2024 kl. 01.22 skrev Stefan Kangas <stefankangas@gmail.com>:

> No strong opinion, but I think this cuts both ways: keeping it local to
> the definition of the functions themselves makes it clear to anyone
> looking there for inspiration how to add such warnings to their own
> `format` functions.

Yes, it's a matter of judgement which has to be reconsidered from time to time. I see no reason for doing anything about this right now, but this may change in the future. The reasons for not distributing function properties can be quite strong as well.

bug#71379: 29.3; Elisp compiler: warnings for formats given insufficiently many arguments

[Andrea Corallo]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Kangas <stefankangas@gmail.com>
>> Date: Fri, 7 Jun 2024 01:22:49 +0200
>> Cc: 71379@debbugs.gnu.org, philippe schnoebelen <schnoebelen.ph@gmail.com>
>> 
>> Mattias Engdegård <mattias.engdegard@gmail.com> writes:
>> 
>> > 6 juni 2024 kl. 06.38 skrev Eli Zaretskii <eliz@gnu.org>:
>> >
>> >> On second thought, this is still fine to go in now, but I wonder
>> >> whether we should move these to the places where the corresponding
>> >> functions are defined, and leave in bytecomp.c only those which are
>> >> primitives defined in C?  That would mean 'warn' and 'user-error'
>> >> should be moved to their respective Lisp files.
>> >
>> > Indeed it's a question I always ask myself every so often, and
>> > sometimes we distribute properties to definitions.
>> > However, in this case it seems better to keep them in one place:
>> > it's only a small handful of functions, and this way the
>> > properties are kept local to the byte-compiler.
>> 
>> No strong opinion, but I think this cuts both ways: keeping it local to
>> the definition of the functions themselves makes it clear to anyone
>> looking there for inspiration how to add such warnings to their own
>> `format` functions.
>
> Indeed.  In addition, some functions marked with this property already
> do that in their own files.  Which was why I asked that question in
> the first place.

Agreed.  Maybe in the future even worth having a declare for it?  We
have them already for things like ex 'important-return-value'.

  Andrea

bug#71379: 29.3; Elisp compiler: warnings for formats given insufficiently many arguments

[Eli Zaretskii]

> From: Andrea Corallo <acorallo@gnu.org>
> Cc: Stefan Kangas <stefankangas@gmail.com>,  71379@debbugs.gnu.org,
>   schnoebelen.ph@gmail.com,  mattias.engdegard@gmail.com
> Date: Fri, 07 Jun 2024 09:55:08 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> No strong opinion, but I think this cuts both ways: keeping it local to
> >> the definition of the functions themselves makes it clear to anyone
> >> looking there for inspiration how to add such warnings to their own
> >> `format` functions.
> >
> > Indeed.  In addition, some functions marked with this property already
> > do that in their own files.  Which was why I asked that question in
> > the first place.
> 
> Agreed.  Maybe in the future even worth having a declare for it?  We
> have them already for things like ex 'important-return-value'.

Yes, could be a good idea.

bug#71379: 29.3; Elisp compiler: warnings for formats given insufficiently many arguments

[Andrea Corallo]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrea Corallo <acorallo@gnu.org>
>> Cc: Stefan Kangas <stefankangas@gmail.com>,  71379@debbugs.gnu.org,
>>   schnoebelen.ph@gmail.com,  mattias.engdegard@gmail.com
>> Date: Fri, 07 Jun 2024 09:55:08 -0400
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> No strong opinion, but I think this cuts both ways: keeping it local to
>> >> the definition of the functions themselves makes it clear to anyone
>> >> looking there for inspiration how to add such warnings to their own
>> >> `format` functions.
>> >
>> > Indeed.  In addition, some functions marked with this property already
>> > do that in their own files.  Which was why I asked that question in
>> > the first place.
>> 
>> Agreed.  Maybe in the future even worth having a declare for it?  We
>> have them already for things like ex 'important-return-value'.
>
> Yes, could be a good idea.

Okay, will do after we branch emacs30.

  Andrea

bug#71379: 29.3; Elisp compiler: warnings for formats given insufficiently many arguments

[Mattias Engdegård]

8 juni 2024 kl. 20.15 skrev Andrea Corallo <acorallo@gnu.org>:

>>> Agreed.  Maybe in the future even worth having a declare for it?  We
>>> have them already for things like ex 'important-return-value'.
>> 
>> Yes, could be a good idea.
> 
> Okay, will do after we branch emacs30.

Again, I'm not very fond of the idea (or I would have done it long ago myself).

bug#71379: 29.3; Elisp compiler: warnings for formats given insufficiently many arguments

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Sat, 8 Jun 2024 20:18:09 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>,
>  stefankangas@gmail.com,
>  71379@debbugs.gnu.org,
>  schnoebelen.ph@gmail.com
> 
> 8 juni 2024 kl. 20.15 skrev Andrea Corallo <acorallo@gnu.org>:
> 
> >>> Agreed.  Maybe in the future even worth having a declare for it?  We
> >>> have them already for things like ex 'important-return-value'.
> >> 
> >> Yes, could be a good idea.
> > 
> > Okay, will do after we branch emacs30.
> 
> Again, I'm not very fond of the idea (or I would have done it long ago myself).

Why not?  Without a rationale, how can we discuss this?

bug#71379: 29.3; Elisp compiler: warnings for formats given insufficiently many arguments

[Mattias Engdegård]

8 juni 2024 kl. 20.32 skrev Eli Zaretskii <eliz@gnu.org>:

> Why not?

As I said, I think the drawbacks outweigh the advantages right now.
It's much easier to keep track of the functions that are subject to the warning when they are gathered in a single place, and there is an advantage in keeping the mechanism local.

bug#71379: 29.3; Elisp compiler: warnings for formats given insufficiently many arguments

[Andrea Corallo]

Mattias Engdegård <mattias.engdegard@gmail.com> writes:

> 8 juni 2024 kl. 20.32 skrev Eli Zaretskii <eliz@gnu.org>:
>
>> Why not?
>
> As I said, I think the drawbacks outweigh the advantages right now.
> It's much easier to keep track of the functions that are subject to the warning when they are gathered in a single place, and there is an advantage in keeping the mechanism local.

But this is not local only already, see 'info-xref-output' and
'info-xref-output-error'.  There might be other occurencies outside
emacs.git as well.

  Andrea

bug#71379: 29.3; Elisp compiler: warnings for formats given insufficiently many arguments

[Mattias Engdegård]

10 juni 2024 kl. 10.11 skrev Andrea Corallo <acorallo@gnu.org>:

> But this is not local only already, see 'info-xref-output' and
> 'info-xref-output-error'.

Very much aware of that, and don't think it's worth doing anything about.

>  There might be other occurencies outside
> emacs.git as well.

Already checked that as well, and there were almost none -- all I could find was a single occurrence in one package. Not that we really care since it's an internal and undocumented interface.

bug#71379: 29.3; Elisp compiler: warnings for formats given insufficiently many arguments

[Andrea Corallo]

Mattias Engdegård <mattias.engdegard@gmail.com> writes:

> 10 juni 2024 kl. 10.11 skrev Andrea Corallo <acorallo@gnu.org>:
>
>> But this is not local only already, see 'info-xref-output' and
>> 'info-xref-output-error'.
>
> Very much aware of that, and don't think it's worth doing anything about.
>
>>  There might be other occurencies outside
>> emacs.git as well.
>
> Already checked that as well, and there were almost none -- all I
> could find was a single occurrence in one package. Not that we really
> care since it's an internal and undocumented interface.

You can indeed have your opinions, but again this shows that the
mechanism is not local only already, and that it's already in use
outside as there was probably need for it.

bug#71379: 29.3; Elisp compiler: warnings for formats given insufficiently many arguments

[Mattias Engdegård]

10 juni 2024 kl. 23.14 skrev Andrea Corallo <acorallo@gnu.org>:

> You can indeed have your opinions, but again this shows that the
> mechanism is not local only already, and that it's already in use
> outside as there was probably need for it.

No it doesn't.

I don't know what is special about this warning, enough for you to argue strongly about it, but maybe you don't either -- once we take a strong position about something, even by accident, it's difficult to stop, and to admit that it's perhaps not that important after all.

So let's give it a rest. I will keep revisiting this again, as I have many times before, and of course so may you. If we do something, let's make sure we know why, not just because we have made up our minds about it.