bug#12299: 24.1; no byte compiler warning for inline function call with too few arguments

bug#12299: 24.1; no byte compiler warning for inline function call with too few arguments

[Roland Winkler]

cat > foo.el << EOF
(defsubst foo (a b))
(foo t)
EOF

Byte-compile the above file. The Emacs 24.1 byte compiler does not
complain that the inline function foo is called with too few arguments.
Emacs 23.1 complains

  foo.el:2:1:Warning: attempt to open-code `anonymous lambda' with
    too few arguments

till it aborts with

  foo.el:2:1:Error: Variable binding depth exceeds max-specpdl-size

which I do not find very appropriate either.


In GNU Emacs 24.1.1 (x86_64-unknown-linux-gnu, GTK+ Version 2.20.1)
 of 2012-06-10 on regnitz
Windowing system distributor `The X.Org Foundation', version 11.0.10706000

bug#12299: 24.1; no byte compiler warning for inline function call with too few arguments

[Lars Ingebrigtsen]

"Roland Winkler" <winkler@gnu.org> writes:

> cat > foo.el << EOF
> (defsubst foo (a b))
> (foo t)
> EOF
>
> Byte-compile the above file. The Emacs 24.1 byte compiler does not
> complain that the inline function foo is called with too few arguments.

I can confirm that this bug is still present in Emacs 27, but it's weird
that this hasn't come up more often.  I mean, defsubsts are commonly
used, and not having byte compilation warnings about the wrong number of
arguments is something you'd expect to come up more often.

> Emacs 23.1 complains
>
>   foo.el:2:1:Warning: attempt to open-code `anonymous lambda' with
>     too few arguments
>
> till it aborts with
>
>   foo.el:2:1:Error: Variable binding depth exceeds max-specpdl-size
>
> which I do not find very appropriate either.

Nope.

I tried briefly to follow the logic in the stuff that'd give a "attempt
to open-code", but it doesn't seem to be triggered at all in this case.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#12299: 24.1; no byte compiler warning for inline function call with too few arguments

[Lars Ingebrigtsen]

"Roland Winkler" <winkler@gnu.org> writes:

> cat > foo.el << EOF
> (defsubst foo (a b))
> (foo t)
> EOF
>
> Byte-compile the above file. The Emacs 24.1 byte compiler does not
> complain that the inline function foo is called with too few arguments.

This is still the case in Emacs 28, which is pretty surprising.  (Both
when doing dynamic and lexical compilation.)

I've added Stefan to the CCs -- perhaps he has some comments here.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#12299: 24.1; no byte compiler warning for inline function call with too few arguments

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

Lars Ingebrigtsen [2021-07-15 22:29:51] wrote:
> "Roland Winkler" <winkler@gnu.org> writes:
>> cat > foo.el << EOF
>> (defsubst foo (a b))
>> (foo t)
>> EOF
>>
>> Byte-compile the above file. The Emacs 24.1 byte compiler does not
>> complain that the inline function foo is called with too few arguments.
> This is still the case in Emacs 28, which is pretty surprising.  (Both
> when doing dynamic and lexical compilation.)
> I've added Stefan to the CCs -- perhaps he has some comments here.

No particular comments, no.  IIRC this part of the byte compiler is just
not making efforts to detect such errors, indeed.
It's probably not hard to fix.


        Stefan

bug#12299: 24.1; no byte compiler warning for inline function call with too few arguments

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> No particular comments, no.  IIRC this part of the byte compiler is just
> not making efforts to detect such errors, indeed.
> It's probably not hard to fix.

Right.  Well, let's hope that somebody feels up to poking at this
problem.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#12299: 24.1; no byte compiler warning for inline function call with too few arguments [PATCH]

[Mattias Engdegård]

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

Would this patch do?
For some reason that I haven't investigated, the usual warning-suppression mechanisms seem ineffective for these warnings. I'm probably doing something wrong. Even so, it's better to have arity checks that cannot be disabled than none at all.


[-- Attachment #2: 0001-Warn-about-arity-errors-in-inlining-calls-bug-12299.patch --]
[-- Type: application/octet-stream, Size: 6467 bytes --]

From cb9d873871eb445ad18b2a89f3c4ba7e6a4d3ef3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Thu, 22 Jul 2021 15:00:17 +0200
Subject: [PATCH] Warn about arity errors in inlining calls (bug#12299)

Wrong number of arguments in inlining function calls (to `defsubst` or
explicitly using `inline`) did not result in warnings, or in very
cryptic ones.

* lisp/emacs-lisp/byte-opt.el (byte-compile-inline-expand): Add calls
to `byte-compile--check-arity-bytecode`.
* lisp/emacs-lisp/bytecomp.el (byte-compile-emit-callargs-warn)
(byte-compile--check-arity-bytecode): New functions.
(byte-compile-callargs-warn): Use factored-out function.
* test/lisp/emacs-lisp/bytecomp-resources/warn-callargs-defsubst.el:
* test/lisp/emacs-lisp/bytecomp-tests.el ("warn-callargs-defsubst.el"):
New test case.
---
 lisp/emacs-lisp/byte-opt.el                   |  5 +-
 lisp/emacs-lisp/bytecomp.el                   | 58 +++++++++++++++----
 .../warn-callargs-defsubst.el                 |  5 ++
 test/lisp/emacs-lisp/bytecomp-tests.el        |  3 +
 4 files changed, 60 insertions(+), 11 deletions(-)
 create mode 100644 test/lisp/emacs-lisp/bytecomp-resources/warn-callargs-defsubst.el

diff --git a/lisp/emacs-lisp/byte-opt.el b/lisp/emacs-lisp/byte-opt.el
index 341643c7d1..ad9f827171 100644
--- a/lisp/emacs-lisp/byte-opt.el
+++ b/lisp/emacs-lisp/byte-opt.el
@@ -274,6 +274,7 @@ byte-compile-inline-expand
       ((pred byte-code-function-p)
        ;; (message "Inlining byte-code for %S!" name)
        ;; The byte-code will be really inlined in byte-compile-unfold-bcf.
+       (byte-compile--check-arity-bytecode form fn)
        `(,fn ,@(cdr form)))
       ((or `(lambda . ,_) `(closure . ,_))
        ;; While byte-compile-unfold-bcf can inline dynbind byte-code into
@@ -300,7 +301,9 @@ byte-compile-inline-expand
                ;; surrounded the `defsubst'.
                (byte-compile-warnings nil))
            (byte-compile name))
-         `(,(symbol-function name) ,@(cdr form))))
+         (let ((bc (symbol-function name)))
+           (byte-compile--check-arity-bytecode form bc)
+           `(,bc ,@(cdr form)))))
 
       (_ ;; Give up on inlining.
        form))))
diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index 2968f1af5d..a5cb5477d8 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -1477,6 +1477,51 @@ byte-compile-function-warn
           (push (list f byte-compile-last-position nargs)
                 byte-compile-unresolved-functions)))))
 
+(defun byte-compile-emit-callargs-warn (name actual-args min-args max-args)
+  (byte-compile-set-symbol-position name)
+  (byte-compile-warn
+   "%s called with %d argument%s, but %s %s"
+   name actual-args
+   (if (= 1 actual-args) "" "s")
+   (if (< actual-args min-args)
+       "requires"
+     "accepts only")
+   (byte-compile-arglist-signature-string (cons min-args max-args))))
+
+(defun byte-compile--check-arity-bytecode (form bytecode)
+  "Check that the call in FORM matches that allowed by BYTECODE."
+  (when (and (byte-code-function-p bytecode)
+             (byte-compile-warning-enabled-p 'callargs))
+    (let* ((desc (aref bytecode 0))
+           (actual-args (length (cdr form)))
+            min-args max-args)
+      (if (listp desc)
+        ;; Old-style bytecode: decode the signature.
+        (let ((n 0))
+          (setq max-args 0)
+          (while desc
+            (pcase (car desc)
+              ('&optional
+               (setq min-args n)
+               (setq desc (cdr desc)))
+              ('&rest
+               (unless min-args (setq min-args n))
+               (setq max-args nil)
+               (setq desc nil))
+              (_
+               (setq max-args (1+ max-args))
+               (setq n (1+ n))
+               (setq desc (cdr desc)))))
+          (unless min-args (setq min-args n)))
+        ;; Modern bytecode.
+        (setq min-args (logand desc 127))
+        (setq max-args (and (= (logand desc 128) 0)
+                            (ash desc -8))))
+      (when (or (< actual-args min-args)
+                (and max-args (> actual-args max-args)))
+        (byte-compile-emit-callargs-warn
+         (car form) actual-args min-args max-args)))))
+
 ;; Warn if the form is calling a function with the wrong number of arguments.
 (defun byte-compile-callargs-warn (form)
   (let* ((def (or (byte-compile-fdefinition (car form) nil)
@@ -1491,16 +1536,9 @@ byte-compile-callargs-warn
 	(setcdr sig nil))
     (if sig
 	(when (or (< ncall (car sig))
-		(and (cdr sig) (> ncall (cdr sig))))
-	  (byte-compile-set-symbol-position (car form))
-	  (byte-compile-warn
-	   "%s called with %d argument%s, but %s %s"
-	   (car form) ncall
-	   (if (= 1 ncall) "" "s")
-	   (if (< ncall (car sig))
-	       "requires"
-	     "accepts only")
-	   (byte-compile-arglist-signature-string sig))))
+		  (and (cdr sig) (> ncall (cdr sig))))
+          (byte-compile-emit-callargs-warn
+           (car form) ncall (car sig) (cdr sig))))
     (byte-compile-format-warn form)
     (byte-compile-function-warn (car form) (length (cdr form)) def)))
 
diff --git a/test/lisp/emacs-lisp/bytecomp-resources/warn-callargs-defsubst.el b/test/lisp/emacs-lisp/bytecomp-resources/warn-callargs-defsubst.el
new file mode 100644
index 0000000000..3a29128cf3
--- /dev/null
+++ b/test/lisp/emacs-lisp/bytecomp-resources/warn-callargs-defsubst.el
@@ -0,0 +1,5 @@
+;;; -*- lexical-binding: t -*-
+(defsubst warn-callargs-defsubst-f1 (_x)
+  nil)
+(defun warn-callargs-defsubst-f2 ()
+  (warn-callargs-defsubst-f1 1 2))
diff --git a/test/lisp/emacs-lisp/bytecomp-tests.el b/test/lisp/emacs-lisp/bytecomp-tests.el
index 33413f5a00..7c40f7ebca 100644
--- a/test/lisp/emacs-lisp/bytecomp-tests.el
+++ b/test/lisp/emacs-lisp/bytecomp-tests.el
@@ -700,6 +700,9 @@ "warn-autoload-not-on-top-level.el"
 (bytecomp--define-warning-file-test "warn-callargs.el"
                             "with 2 arguments, but accepts only 1")
 
+(bytecomp--define-warning-file-test "warn-callargs-defsubst.el"
+                            "with 2 arguments, but accepts only 1")
+
 (bytecomp--define-warning-file-test "warn-defcustom-nogroup.el"
                             "fails to specify containing group")
 
-- 
2.21.1 (Apple Git-122.3)

bug#12299: 24.1; no byte compiler warning for inline function call with too few arguments [PATCH]

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> Would this patch do?
> For some reason that I haven't investigated, the usual
> warning-suppression mechanisms seem ineffective for these
> warnings. I'm probably doing something wrong. Even so, it's better to
> have arity checks that cannot be disabled than none at all.

Yes, we can fix up suppression afterwards...

> From cb9d873871eb445ad18b2a89f3c4ba7e6a4d3ef3 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
> Date: Thu, 22 Jul 2021 15:00:17 +0200
> Subject: [PATCH] Warn about arity errors in inlining calls (bug#12299)

This leads to warnings like:

Warning: Eager macro-expansion skipped due to cycle:
  … => (load "byte-opt.el") => (macroexpand-all (defalias 'byte-decompile-bytecode-1 …)) => (macroexpand (cl-symbol-macrolet …)) => (load "byte-opt.el")

At least initially?  A "make bootstrap" made it go away.

It otherwise seems to work fine for the test example.

I wondered whether this added warning would find errors in the Emacs
tree, so I did a "make bootstrap" after applying the patch, but we
apparently have none of those, which surprised me.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#12299: 24.1; no byte compiler warning for inline function call with too few arguments [PATCH]

[Mattias Engdegård]

23 juli 2021 kl. 13.09 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> Yes, we can fix up suppression afterwards...

It's not a new problem -- there are plenty of warnings that can be generated during the source-level optimisation and suppressions don't work there right now.

> This leads to warnings like:
> 
> Warning: Eager macro-expansion skipped due to cycle:
>  … => (load "byte-opt.el") => (macroexpand-all (defalias 'byte-decompile-bytecode-1 …)) => (macroexpand (cl-symbol-macrolet …)) => (load "byte-opt.el")

Strange. I never saw anything like that. But if bootstrapping helped you, then I suppose it's fine.

> I wondered whether this added warning would find errors in the Emacs
> tree, so I did a "make bootstrap" after applying the patch, but we
> apparently have none of those, which surprised me.

Yes, I'm disappointed as well. Had hoped for some juicy pay-off, but no.

Thanks for testing it -- pushed to master, slightly simplified (somehow I completely missed the existence of `func-arity`)

bug#12299: 24.1; no byte compiler warning for inline function call with too few arguments [PATCH]

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

> diff --git a/lisp/emacs-lisp/byte-opt.el b/lisp/emacs-lisp/byte-opt.el
> index 341643c7d1..ad9f827171 100644
> --- a/lisp/emacs-lisp/byte-opt.el
> +++ b/lisp/emacs-lisp/byte-opt.el
> @@ -274,6 +274,7 @@ byte-compile-inline-expand
>        ((pred byte-code-function-p)
>         ;; (message "Inlining byte-code for %S!" name)
>         ;; The byte-code will be really inlined in byte-compile-unfold-bcf.
> +       (byte-compile--check-arity-bytecode form fn)
>         `(,fn ,@(cdr form)))

Hmm... wouldn't we want the warning to be emitted if the source code was itself
of the form constructed by `(,fn ,@(cdr form))?
IOW I think we want that `byte-compile--check-arity-bytecode` call to be
elsewhere (in the code that actually compiles the code constructed by
`(,fn ,@(cdr form))).

Also, as a general rule I think we should try and refrain from emitting
warnings from the code in byte-opt.el (warnings should be independent
from optimizations).

> -         `(,(symbol-function name) ,@(cdr form))))
> +         (let ((bc (symbol-function name)))
> +           (byte-compile--check-arity-bytecode form bc)
> +           `(,bc ,@(cdr form)))))

I think the same holds here.


        Stefan

bug#12299: 24.1; no byte compiler warning for inline function call with too few arguments [PATCH]

[Mattias Engdegård]

23 juli 2021 kl. 17.02 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> Hmm... wouldn't we want the warning to be emitted if the source code was itself
> of the form constructed by `(,fn ,@(cdr form))?

You mean when FN is a bytecode object? Sure, we can do that too. (We currently report an error when there are too many arguments but not too few.) However, it doesn't replace the newly inserted checks, because:

> IOW I think we want that `byte-compile--check-arity-bytecode` call to be
> elsewhere (in the code that actually compiles the code constructed by
> `(,fn ,@(cdr form))).

Tried that first, but then we no longer have the function name so the user sees

 Warning: ‹bytecode gibberish› called with 1 argument but requires 2

> Also, as a general rule I think we should try and refrain from emitting
> warnings from the code in byte-opt.el (warnings should be independent
> from optimizations).

Agree in general but for non-inlined calls we do the arity checks in the codegen, after optimising, and that's too late for inlined calls. We could do it in cconv, but then (a) we may not know the signature or (b) the call could be in dead code.

But this is good to have in mind -- we have a growing list of things to deal with in a compiler reorganisation. There is a lot of history in this code.

bug#12299: 24.1; no byte compiler warning for inline function call with too few arguments [PATCH]

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

>> IOW I think we want that `byte-compile--check-arity-bytecode` call to be
>> elsewhere (in the code that actually compiles the code constructed by
>> `(,fn ,@(cdr form))).
> Tried that first, but then we no longer have the function name so the user sees
>  Warning: ‹bytecode gibberish› called with 1 argument but requires 2

Ah, right, makes sense, thanks,


        Stefan

bug#12299: 24.1; no byte compiler warning for inline function call with too few arguments

[Stefan Kangas]

Mattias Engdegård <mattiase@acm.org> writes:

> Thanks for testing it -- pushed to master, slightly simplified
> (somehow I completely missed the existence of `func-arity`)

It seems like this bug was fixed, so I'm closing it now.

commit 109ca1bd00b56ba66b123b505d8c2187fded0ef7
Author: Mattias Engdegård <mattiase@acm.org>
Date:   Thu Jul 22 15:00:17 2021 +0200

    Warn about arity errors in inlining calls (bug#12299)