bug#29165: 26.0.90; can't use some code byte-compiled under emacs 24

[Noam Postavsky <npostavs@users.sourceforge.net>]

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

On Mon, Nov 6, 2017 at 2:16 PM, Noam Postavsky
<npostavs@users.sourceforge.net> wrote:
> On Mon, Nov 6, 2017 at 2:10 PM, Ken Raeburn <raeburn@permabit.com> wrote:
>
>> It appears that the emacs-26 version of defun* is happy with it (the
>> original Lisp code I posted, using &optional &key) as well, as long as I
>> provide the source, or a byte-compiled file from Emacs 25 or 26
>
> It looks like the cl-defun in newer Emacs throws away the &optional
> for you in this case.

I think we should make cl-defun reject this kind of code, to be
consistent with plain defun. See attached.

[-- Attachment #2: v1-0001-Mention-new-strictness-for-optional-rest-in-argli.patch --]
[-- Type: application/octet-stream, Size: 4689 bytes --]

From 82af67b84a663fa750b375e483d4b3e8e1f26a6e Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Mon, 13 Nov 2017 12:46:13 -0500
Subject: [PATCH v1] Mention new strictness for &optional, &rest in arglists
 (Bug#29165)

* etc/NEWS: Explain that '&optional' not followed by a variable is now
an error.
* lisp/emacs-lisp/cl-macs.el (cl--transform-lambda)
(cl--do-arglist): Also reject '&optional' or '&rest' not followed by a
variable for consistency.
* test/lisp/emacs-lisp/cl-macs-tests.el (cl-macs-bad-arglist): New
test.
---
 etc/NEWS                              | 11 +++++++++++
 lisp/emacs-lisp/cl-macs.el            | 16 +++++++++++-----
 test/lisp/emacs-lisp/cl-macs-tests.el | 25 +++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index f79c2cb..b7af6e0 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1468,6 +1468,17 @@ them through 'format' first.  Even that is discouraged: for ElDoc
 support, you should set 'eldoc-documentation-function' instead of
 calling 'eldoc-message' directly.
 
+---
+** Using '&rest' or '&optional' incorrectly is now an error.
+For example giving '&optional' without a following variable, or
+passing '&optional' multiple times:
+
+    (defun foo (&optional &rest x))
+    (defun bar (&optional &optional x))
+
+Previously, Emacs would just ignore the extra keyword, or give
+incorrect results in certain cases.
+
 \f
 * Lisp Changes in Emacs 26.1
 
diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index e313af2..fccf6da 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -281,8 +281,13 @@ cl--transform-lambda
                   (or (not optional)
                       ;; Optional args whose default is nil are simple.
                       (null (nth 1 (assq (car args) (cdr cl--bind-defs)))))
-                  (not (and (eq (car args) '&optional) (setq optional t)
-                            (car cl--bind-defs))))
+                  (not (and (eq (car args) '&optional)
+                            (progn
+                              (when (memq (cadr args)
+                                          '(nil &rest &body &key &aux))
+                                (error "Variable missing after &optional"))
+                              (setq optional t)
+                              (car cl--bind-defs)))))
         (push (pop args) simple-args))
       (when optional
         (if args (push '&optional args))
@@ -558,9 +563,10 @@ cl--do-arglist
 	  (keys nil)
 	  (laterarg nil) (exactarg nil) minarg)
       (or num (setq num 0))
-      (setq restarg (if (listp (cadr restarg))
-                        (make-symbol "--cl-rest--")
-                      (cadr restarg)))
+      (setq restarg (or (if (consp (cadr restarg))
+                            (make-symbol "--cl-rest--")
+                          (cadr restarg))
+                        (error "Variable missing after &rest")))
       (push (list restarg expr) cl--bind-lets)
       (if (eq (car args) '&whole)
 	  (push (list (cl--pop2 args) restarg) cl--bind-lets))
diff --git a/test/lisp/emacs-lisp/cl-macs-tests.el b/test/lisp/emacs-lisp/cl-macs-tests.el
index 575f170..e43ff23 100644
--- a/test/lisp/emacs-lisp/cl-macs-tests.el
+++ b/test/lisp/emacs-lisp/cl-macs-tests.el
@@ -497,4 +497,29 @@
                           vconcat (vector (1+ x)))
                  [2 3 4 5 6])))
 
+\f
+;;; cl-lib lambda list handling
+
+(ert-deftest cl-macs-bad-arglist ()
+  "Check that `cl-defun' and friends reject weird argument lists.
+See Bug#29165, and similar `eval-tests--bugs-24912-and-24913' in
+eval-tests.el."
+  (dolist (args (cl-mapcan
+                 ;; For every &rest variant, check also the same thing
+                 ;; with &key instead.
+                 (lambda (arglist) (if (memq '&rest arglist)
+                                  (list arglist (cl-subst '&key '&rest arglist))
+                                (list arglist)))
+                 '((&optional) (&rest) (&optional &rest) (&rest &optional)
+                   (&optional &rest _a) (&optional _a &rest)
+                   (&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 (cl-function (lambda ,args))) t))
+    (should-error (cl--transform-lambda (cons args t)))
+    (let ((byte-compile-debug t))
+      (should-error (eval `(byte-compile (cl-function (lambda ,args))) t)))))
+
 ;;; cl-macs-tests.el ends here
-- 
2.6.2.windows.1