bug#50720: unnamed &rest broken

bug#50720: unnamed &rest broken

[Mattias Engdegård]

The interpreter and compiler allow &rest to be used without a variable name following but the generated byte code is completely broken:

(funcall (byte-compile (lambda (&rest) 'ta)))

crashes, and

(defun boo (a &rest)
  (if a a (list 1 2 3 4)))

(boo 'hiss)
=> hiss        ; interpreted
=> (1 2 3 4)   ; compiled

The reason is that the compiler generates code from the argument variable list but the byte-code interpreter will only look at the signature code which was generated from the actual signature:

(byte-compile (lambda (&rest) 'ta))
=> #[128 "\300\207" [ta] 1 "..."]

The 128 indicates zero positional parameters and a &rest argument, and the 1 is the maximum stack size required which is wrong; 2 stack slots are needed and that's what we get if naming the argument:

(byte-compile (lambda (&rest _r) 'ta))
=> #[128 "\300\207" [ta] 2 "..."]

In the `boo` case above, it is clear that the compiler doesn't expect any &rest param to have been pushed at all so the stack offsets are wrong.

Now, either we fix this bug or we stop pretending that unnamed &rest arguments work at all and signal an error, because it's clear from the above that they can't have seen much use.

bug#50720: bug#50268: 28.0.50; Assertion warning during native compilation

[Lars Ingebrigtsen]

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

> Now, either we fix this bug or we stop pretending that unnamed &rest
> arguments work at all and signal an error, because it's clear from the
> above that they can't have seen much use.

I did a quick grep through core and GNU ELPA, and I couldn't find any
instances of it being used, so I'd be fine with either solution (i.e.,
either fixing the bug or signalling an error).

Perhaps Eli has an opinion (added to the CCs).

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

bug#50268: 28.0.50; Assertion warning during native compilation

[Mattias Engdegård]

21 sep. 2021 kl. 18.11 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> I did a quick grep through core and GNU ELPA, and I couldn't find any
> instances of it being used, so I'd be fine with either solution (i.e.,
> either fixing the bug or signalling an error).

I know how to fix it but there's no really elegant way of doing it, and unless a convincing case can be made for permitting anonymous &rest, I'd favour disallowing it.

Unless I'm mistaken it was Noam who added it (1d47d777ef24c0be9153b0a1c8ba21918fa1025a), so let's ask him.

bug#50268: 28.0.50; Assertion warning during native compilation

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 50720@debbugs.gnu.org,  50268@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>
> Date: Tue, 21 Sep 2021 18:11:34 +0200
> 
> Mattias Engdegård <mattiase@acm.org> writes:
> 
> > Now, either we fix this bug or we stop pretending that unnamed &rest
> > arguments work at all and signal an error, because it's clear from the
> > above that they can't have seen much use.
> 
> I did a quick grep through core and GNU ELPA, and I couldn't find any
> instances of it being used, so I'd be fine with either solution (i.e.,
> either fixing the bug or signalling an error).
> 
> Perhaps Eli has an opinion (added to the CCs).

I think I'd like to at least see the prototype of the fix to have an
opinion.

bug#50268: 28.0.50; Assertion warning during native compilation

[Noam Postavsky]

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

> 21 sep. 2021 kl. 18.11 skrev Lars Ingebrigtsen <larsi@gnus.org>:
>
>> I did a quick grep through core and GNU ELPA, and I couldn't find any
>> instances of it being used, so I'd be fine with either solution (i.e.,
>> either fixing the bug or signalling an error).
>
> I know how to fix it but there's no really elegant way of doing it,
> and unless a convincing case can be made for permitting anonymous
> &rest, I'd favour disallowing it.
>
> Unless I'm mistaken it was Noam who added it
> (1d47d777ef24c0be9153b0a1c8ba21918fa1025a), so let's ask him.

It's been a while, but looking at Bug#29165, the reason I added it is
that it was supported in Emacs 25 and earlier.  Stefan M had some
justifications there for &optional without variables, but I think they
don't apply for &rest.

bug#50268: 28.0.50; Assertion warning during native compilation

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  Eli Zaretskii <eliz@gnu.org>,  50268@debbugs.gnu.org
> Date: Tue, 21 Sep 2021 13:09:24 -0400
> 
> Mattias Engdegård <mattiase@acm.org> writes:
> 
> > 21 sep. 2021 kl. 18.11 skrev Lars Ingebrigtsen <larsi@gnus.org>:
> >
> >> I did a quick grep through core and GNU ELPA, and I couldn't find any
> >> instances of it being used, so I'd be fine with either solution (i.e.,
> >> either fixing the bug or signalling an error).
> >
> > I know how to fix it but there's no really elegant way of doing it,
> > and unless a convincing case can be made for permitting anonymous
> > &rest, I'd favour disallowing it.
> >
> > Unless I'm mistaken it was Noam who added it
> > (1d47d777ef24c0be9153b0a1c8ba21918fa1025a), so let's ask him.
> 
> It's been a while, but looking at Bug#29165, the reason I added it is
> that it was supported in Emacs 25 and earlier.  Stefan M had some
> justifications there for &optional without variables, but I think they
> don't apply for &rest.

Stefan, can you please chime in?

bug#50268: 28.0.50; Assertion warning during native compilation

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

>> It's been a while, but looking at Bug#29165, the reason I added it is
>> that it was supported in Emacs 25 and earlier.  Stefan M had some
>> justifications there for &optional without variables, but I think they
>> don't apply for &rest.
> Stefan, can you please chime in?

With my type-systems-guy hat on, I'd say we want to detect those `&rest`
without a following variable and shame the author of the code.

But at the same time we could think of `&rest` as something similar to
`&optional` and decide that it can be handy for generated code to be
able to generate (x y &optional z &rest).

The main difference I see is that

    `(x y &optional ,@args)

makes a lot of sense and will naturally occasionally lead to `&optional`
not followed by anything, whereas `&rest` only expects 1 following
identifier so it seems much less likely that the code will sometimes put
0 vars in there.  After all, we do signal an error if there's more than
1 var after `&rest`, so I think it makes sense to follow my
types-systems-guy heart here.


        Stefan

bug#50268: 28.0.50; Assertion warning during native compilation

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Noam Postavsky <npostavs@gmail.com>,  mattiase@acm.org,  larsi@gnus.org,
>   50268@debbugs.gnu.org
> Date: Tue, 21 Sep 2021 15:32:32 -0400
> 
> With my type-systems-guy hat on, I'd say we want to detect those `&rest`
> without a following variable and shame the author of the code.
> 
> But at the same time we could think of `&rest` as something similar to
> `&optional` and decide that it can be handy for generated code to be
> able to generate (x y &optional z &rest).
> 
> The main difference I see is that
> 
>     `(x y &optional ,@args)
> 
> makes a lot of sense and will naturally occasionally lead to `&optional`
> not followed by anything, whereas `&rest` only expects 1 following
> identifier so it seems much less likely that the code will sometimes put
> 0 vars in there.  After all, we do signal an error if there's more than
> 1 var after `&rest`, so I think it makes sense to follow my
> types-systems-guy heart here.

Which basically means we should revert Noam's commit of yore, is that
right?

bug#50268: 28.0.50; Assertion warning during native compilation

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

> Which basically means we should revert Noam's commit of yore, is that
> right?

Not all of it, no.  Only the part which makes (&rest) acceptable.


        Stefan

bug#50268: 28.0.50; Assertion warning during native compilation

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: mattiase@acm.org,  larsi@gnus.org,  npostavs@gmail.com,
>   50268@debbugs.gnu.org
> Date: Wed, 22 Sep 2021 21:46:24 -0400
> 
> > Which basically means we should revert Noam's commit of yore, is that
> > right?
> 
> Not all of it, no.  Only the part which makes (&rest) acceptable.

That's what I meant, sorry for not being clear enough.

bug#50268: 28.0.50; Assertion warning during native compilation

[Mattias Engdegård]

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

23 sep. 2021 kl. 03.46 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> Not all of it, no.  Only the part which makes (&rest) acceptable.

Patch.


[-- Attachment #2: 0001-Renege-on-anonymous-rest-bug-50268-bug-50720.patch --]
[-- Type: application/octet-stream, Size: 7575 bytes --]

From c69c3b8c97e9aa9ff350b009c06b4e3e8842ba83 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Thu, 23 Sep 2021 12:43:41 +0200
Subject: [PATCH] Renege on anonymous &rest (bug#50268, bug#50720)

Allowing &rest without a variable name following turned out not to be
very useful, and it never worked properly.  Disallow it.

* lisp/emacs-lisp/bytecomp.el (byte-compile-check-lambda-list):
* src/eval.c (funcall_lambda):
Signal error for &rest without variable name.
* doc/lispref/functions.texi (Argument List): Adjust manual.
* etc/NEWS (file): Announce.
* test/src/eval-tests.el (eval-tests--bugs-24912-and-24913):
Extend test, also checking with and without lexical binding.
(eval-tests-accept-empty-optional-rest): Reduce to...
(eval-tests-accept-empty-optional): ...this, again checking
with and without lexical binding.
---
 doc/lispref/functions.texi  |  2 +-
 etc/NEWS                    |  7 +++++
 lisp/emacs-lisp/bytecomp.el |  2 ++
 src/eval.c                  |  9 ++++--
 test/src/eval-tests.el      | 57 +++++++++++++++++++++----------------
 5 files changed, 49 insertions(+), 28 deletions(-)

diff --git a/doc/lispref/functions.texi b/doc/lispref/functions.texi
index 77d1465c87..c856557c3c 100644
--- a/doc/lispref/functions.texi
+++ b/doc/lispref/functions.texi
@@ -378,7 +378,7 @@ Argument List
 @group
 (@var{required-vars}@dots{}
  @r{[}&optional @r{[}@var{optional-vars}@dots{}@r{]}@r{]}
- @r{[}&rest @r{[}@var{rest-var}@r{]}@r{]})
+ @r{[}&rest @var{rest-var}@r{]})
 @end group
 @end example
 
diff --git a/etc/NEWS b/etc/NEWS
index fb7a7f628a..cf3f90304d 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -3215,6 +3215,13 @@ local variable would not be heeded.  This has now changed, and a file
 with a 'lexical-binding' cookie is always heeded.  To revert to the
 old behavior, set 'permanently-enabled-local-variables' to nil.
 
++++
+** '&rest' in argument lists must always be followed by a variable name.
+Omitting the variable name after '&rest' was previously tolerated in
+some cases but not consistently so; it could lead to crashes or
+outright wrong results.  Since the utility was marginal at best, it is
+now an error to omit the variable.
+
 ---
 ** 'kill-all-local-variables' has changed how it handles non-symbol hooks.
 The function is documented to eliminate all buffer-local bindings
diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index be74195778..d7da7a2149 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -2930,6 +2930,8 @@ byte-compile-check-lambda-list
 		   (macroexp--const-symbol-p arg t))
 	       (error "Invalid lambda variable %s" arg))
 	      ((eq arg '&rest)
+               (unless (cdr list)
+                 (error "&rest without variable name"))
 	       (when (cddr list)
 		 (error "Garbage following &rest VAR in lambda-list"))
                (when (memq (cadr list) '(&optional &rest))
diff --git a/src/eval.c b/src/eval.c
index 2bb7cfe600..66d34808f8 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -3245,6 +3245,7 @@ funcall_lambda (Lisp_Object fun, ptrdiff_t nargs,
     emacs_abort ();
 
   i = optional = rest = 0;
+  bool previous_rest = false;
   for (; CONSP (syms_left); syms_left = XCDR (syms_left))
     {
       maybe_quit ();
@@ -3255,13 +3256,14 @@ funcall_lambda (Lisp_Object fun, ptrdiff_t nargs,
 
       if (EQ (next, Qand_rest))
         {
-          if (rest)
+          if (rest || previous_rest)
             xsignal1 (Qinvalid_function, fun);
           rest = 1;
+	  previous_rest = true;
         }
       else if (EQ (next, Qand_optional))
         {
-          if (optional || rest)
+          if (optional || rest || previous_rest)
             xsignal1 (Qinvalid_function, fun);
           optional = 1;
         }
@@ -3287,10 +3289,11 @@ funcall_lambda (Lisp_Object fun, ptrdiff_t nargs,
 	  else
 	    /* Dynamically bind NEXT.  */
 	    specbind (next, arg);
+	  previous_rest = false;
 	}
     }
 
-  if (!NILP (syms_left))
+  if (!NILP (syms_left) || previous_rest)
     xsignal1 (Qinvalid_function, fun);
   else if (i < nargs)
     xsignal2 (Qwrong_number_of_arguments, fun, make_fixnum (nargs));
diff --git a/test/src/eval-tests.el b/test/src/eval-tests.el
index b2b7dfefda..3c3e703341 100644
--- a/test/src/eval-tests.el
+++ b/test/src/eval-tests.el
@@ -39,31 +39,40 @@ byte-compile-debug
 (ert-deftest eval-tests--bugs-24912-and-24913 ()
   "Check that Emacs doesn't accept weird argument lists.
 Bug#24912 and Bug#24913."
-  (dolist (args '((&rest &optional)
-                  (&rest a &optional) (&rest &optional a)
-                  (&optional &optional) (&optional &optional a)
-                  (&optional a &optional b)
-                  (&rest &rest) (&rest &rest a)
-                  (&rest a &rest b)))
-    (should-error (eval `(funcall (lambda ,args)) t) :type 'invalid-function)
-    (should-error (byte-compile-check-lambda-list args))
-    (let ((byte-compile-debug t))
-      (ert-info ((format "bytecomp: args = %S" args))
-       (should-error (eval `(byte-compile (lambda ,args)) t))))))
-
-(ert-deftest eval-tests-accept-empty-optional-rest ()
-  "Check that Emacs accepts empty &optional and &rest arglists.
+  (dolist (lb '(t false))
+    (ert-info ((prin1-to-string lb) :prefix "lexical-binding: ")
+      (let ((lexical-binding lb))
+        (dolist (args '((&rest &optional)
+                        (&rest a &optional) (&rest &optional a)
+                        (&optional &optional) (&optional &optional a)
+                        (&optional a &optional b)
+                        (&rest &rest) (&rest &rest a)
+                        (&rest a &rest b)
+                        (&rest) (&optional &rest)
+                        ))
+          (ert-info ((prin1-to-string args) :prefix "args: ")
+            (should-error
+             (eval `(funcall (lambda ,args)) lb) :type 'invalid-function)
+            (should-error (byte-compile-check-lambda-list args))
+            (let ((byte-compile-debug t))
+              (should-error (eval `(byte-compile (lambda ,args)) lb)))))))))
+
+(ert-deftest eval-tests-accept-empty-optional ()
+  "Check that Emacs accepts empty &optional arglists.
 Bug#24912."
-  (dolist (args '((&optional) (&rest) (&optional &rest)
-                  (&optional &rest a) (&optional a &rest)))
-    (let ((fun `(lambda ,args 'ok)))
-      (ert-info ("eval")
-        (should (eq (funcall (eval fun t)) 'ok)))
-      (ert-info ("byte comp check")
-        (byte-compile-check-lambda-list args))
-      (ert-info ("bytecomp")
-        (let ((byte-compile-debug t))
-          (should (eq (funcall (byte-compile fun)) 'ok)))))))
+  (dolist (lb '(t false))
+    (ert-info ((prin1-to-string lb) :prefix "lexical-binding: ")
+      (let ((lexical-binding lb))
+        (dolist (args '((&optional) (&optional &rest a)))
+          (ert-info ((prin1-to-string args) :prefix "args: ")
+            (let ((fun `(lambda ,args 'ok)))
+              (ert-info ("eval")
+                (should (eq (funcall (eval fun lb)) 'ok)))
+              (ert-info ("byte comp check")
+                (byte-compile-check-lambda-list args))
+              (ert-info ("bytecomp")
+                (let ((byte-compile-debug t))
+                  (should (eq (funcall (byte-compile fun)) 'ok)))))))))))
 
 
 (dolist (form '(let let*))
-- 
2.21.1 (Apple Git-122.3)

bug#50268: 28.0.50; Assertion warning during native compilation

[Lars Ingebrigtsen]

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

> Allowing &rest without a variable name following turned out not to be
> very useful, and it never worked properly.  Disallow it.

Makes sense to me.  Since it's that non-invasive, I think it'd be fine
to push to Emacs 28.  Perhaps Eli has a different opinion, though.

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

bug#50268: 28.0.50; Assertion warning during native compilation

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  Eli Zaretskii
>  <eliz@gnu.org>,  Noam Postavsky <npostavs@gmail.com>,
>   50268@debbugs.gnu.org
> Date: Thu, 23 Sep 2021 23:03:04 +0200
> 
> Mattias Engdegård <mattiase@acm.org> writes:
> 
> > Allowing &rest without a variable name following turned out not to be
> > very useful, and it never worked properly.  Disallow it.
> 
> Makes sense to me.  Since it's that non-invasive, I think it'd be fine
> to push to Emacs 28.  Perhaps Eli has a different opinion, though.

Let's do it now.

Thanks.

bug#50268: 28.0.50; Assertion warning during native compilation

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> Makes sense to me.  Since it's that non-invasive, I think it'd be fine
>> to push to Emacs 28.  Perhaps Eli has a different opinion, though.
>
> Let's do it now.

OK, Mattias, go ahead and push.

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

bug#50268: 28.0.50; Assertion warning during native compilation

[Mattias Engdegård]

24 sep. 2021 kl. 08.07 skrev Eli Zaretskii <eliz@gnu.org>:

> Let's do it now.

Thank you, pushed to master. Closing.