* bug#24913: 25.1.50; Emacs accepts undocumented and confusing combinations of &optional and &rest in argument lists @ 2016-11-09 21:17 Philipp Stephani 2016-11-10 12:58 ` Philipp Stephani 0 siblings, 1 reply; 8+ messages in thread From: Philipp Stephani @ 2016-11-09 21:17 UTC (permalink / raw) To: 24913 For example: (funcall (lambda (&optional &rest &rest &optional x) (list x)) 'a) => ((a)) Obviously here the &rest keyword "wins", but I think that's overly confusing. Such an argument list is most likely a programmer mistake, and should signal an error to make the programmer aware of the mistake. In GNU Emacs 25.1.50.13 (x86_64-unknown-linux-gnu, GTK+ Version 3.10.8) of 2016-11-09 built on localhost Repository revision: eb364fddec1431f459166cebb36f09f6b371dd71 Windowing system distributor 'The X.Org Foundation', version 11.0.11501000 System Description: Ubuntu 14.04 LTS Configured using: 'configure --with-modules --enable-checking --enable-check-lisp-object-type 'CFLAGS=-ggdb3 -O0'' Configured features: XPM JPEG TIFF GIF PNG SOUND GSETTINGS NOTIFY GNUTLS FREETYPE XFT ZLIB TOOLKIT_SCROLL_BARS GTK3 X11 MODULES Important settings: value of $LANG: en_US.UTF-8 locale-coding-system: utf-8-unix Major mode: Lisp Interaction Minor modes in effect: tooltip-mode: t global-eldoc-mode: t electric-indent-mode: t mouse-wheel-mode: t tool-bar-mode: t menu-bar-mode: t file-name-shadow-mode: t global-font-lock-mode: t font-lock-mode: t blink-cursor-mode: t auto-composition-mode: t auto-encryption-mode: t auto-compression-mode: t line-number-mode: t transient-mark-mode: t Recent messages: For information about GNU Emacs and the GNU system, type C-h C-a. Load-path shadows: None found. Features: (shadow sort mail-extr emacsbug message dired format-spec rfc822 mml mml-sec password-cache epg epg-config gnus-util mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums mm-util help-fns help-mode easymenu cl-loaddefs pcase cl-lib mail-prsvr mail-utils time-date mule-util tooltip eldoc electric uniquify ediff-hook vc-hooks lisp-float-type mwheel x-win term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe tabulated-list newcomment elisp-mode lisp-mode prog-mode register page menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock font-lock syntax facemenu font-core frame cl-generic cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european ethiopic indian cyrillic chinese charscript case-table epa-hook jka-cmpr-hook help simple abbrev minibuffer cl-preloaded nadvice loaddefs button faces cus-face macroexp files text-properties overlay sha1 md5 base64 format env code-pages mule custom widget hashtable-print-readable backquote inotify dynamic-setting system-font-setting font-render-setting move-toolbar gtk x-toolkit x multi-tty make-network-process emacs) Memory information: ((conses 16 88939 12159) (symbols 48 19888 0) (miscs 40 343 148) (strings 32 14757 5052) (string-bytes 1 440451) (vectors 16 12779) (vector-slots 8 446082 5011) (floats 8 166 60) (intervals 56 209 0) (buffers 976 24) (heap 1024 23753 956)) -- Google Germany GmbH Erika-Mann-Straße 33 80636 München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind, leiten Sie diese bitte nicht weiter, informieren Sie den Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank. This e-mail is confidential. If you are not the right addressee please do not forward it, please inform the sender, and please erase this e-mail including any attachments. Thanks. ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#24913: 25.1.50; Emacs accepts undocumented and confusing combinations of &optional and &rest in argument lists 2016-11-09 21:17 bug#24913: 25.1.50; Emacs accepts undocumented and confusing combinations of &optional and &rest in argument lists Philipp Stephani @ 2016-11-10 12:58 ` Philipp Stephani 2016-11-10 16:56 ` Gemini Lasswell 2016-11-18 9:06 ` Eli Zaretskii 0 siblings, 2 replies; 8+ messages in thread From: Philipp Stephani @ 2016-11-10 12:58 UTC (permalink / raw) To: 24913 [-- Attachment #1.1: Type: text/plain, Size: 455 bytes --] Philipp Stephani <p.stephani2@gmail.com> schrieb am Mi., 9. Nov. 2016 um 22:19 Uhr: > > For example: > > (funcall (lambda (&optional &rest &rest &optional x) (list x)) 'a) > => ((a)) > > Obviously here the &rest keyword "wins", but I think that's overly > confusing. Such an argument list is most likely a programmer mistake, > and should signal an error to make the programmer aware of the mistake. > > Here's a patch that detects such argument lists. [-- Attachment #1.2: Type: text/html, Size: 990 bytes --] [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Prevent-dubious-argument-lists.txt --] [-- Type: text/plain; charset=US-ASCII; name="0001-Prevent-dubious-argument-lists.txt", Size: 3951 bytes --] From 185586a3377e166a5123407799ab7741d4627c52 Mon Sep 17 00:00:00 2001 From: Philipp Stephani <phst@google.com> Date: Wed, 9 Nov 2016 23:13:52 +0100 Subject: [PATCH] Prevent dubious argument lists See Bug#24912 and Bug#24913. * src/eval.c (funcall_lambda): Detect more dubious argument lists. * lisp/emacs-lisp/bytecomp.el (byte-compile-check-lambda-list): Detect more dubious argument lists. * test/src/eval-tests.el (eval-tests--bugs-24912-and-24913): Add unit test. --- lisp/emacs-lisp/bytecomp.el | 7 +++++-- src/eval.c | 18 +++++++++++++++--- test/src/eval-tests.el | 15 +++++++++++++++ 3 files changed, 35 insertions(+), 5 deletions(-) diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el index 428e21c..85daa43 100644 --- a/lisp/emacs-lisp/bytecomp.el +++ b/lisp/emacs-lisp/bytecomp.el @@ -2672,8 +2672,11 @@ byte-compile-check-lambda-list (when (cddr list) (error "Garbage following &rest VAR in lambda-list"))) ((eq arg '&optional) - (unless (cdr list) - (error "Variable name missing after &optional"))) + (when (or (null (cdr list)) + (memq (cadr list) '(&optional &rest))) + (error "Variable name missing after &optional")) + (when (memq '&optional (cddr list)) + (error "Duplicate &optional"))) ((memq arg vars) (byte-compile-warn "repeated variable %s in lambda-list" arg)) (t diff --git a/src/eval.c b/src/eval.c index caeb791..884e1eb 100644 --- a/src/eval.c +++ b/src/eval.c @@ -2888,6 +2888,7 @@ funcall_lambda (Lisp_Object fun, ptrdiff_t nargs, emacs_abort (); i = optional = rest = 0; + bool previous_optional_or_rest = false; for (; CONSP (syms_left); syms_left = XCDR (syms_left)) { QUIT; @@ -2897,9 +2898,19 @@ funcall_lambda (Lisp_Object fun, ptrdiff_t nargs, xsignal1 (Qinvalid_function, fun); if (EQ (next, Qand_rest)) - rest = 1; + { + if (rest || previous_optional_or_rest) + xsignal1 (Qinvalid_function, fun); + rest = 1; + previous_optional_or_rest = true; + } else if (EQ (next, Qand_optional)) - optional = 1; + { + if (optional || rest || previous_optional_or_rest) + xsignal1 (Qinvalid_function, fun); + optional = 1; + previous_optional_or_rest = true; + } else { Lisp_Object arg; @@ -2922,10 +2933,11 @@ funcall_lambda (Lisp_Object fun, ptrdiff_t nargs, else /* Dynamically bind NEXT. */ specbind (next, arg); + previous_optional_or_rest = false; } } - if (!NILP (syms_left)) + if (!NILP (syms_left) || previous_optional_or_rest) xsignal1 (Qinvalid_function, fun); else if (i < nargs) xsignal2 (Qwrong_number_of_arguments, fun, make_number (nargs)); diff --git a/test/src/eval-tests.el b/test/src/eval-tests.el index 75999e1..fe08506 100644 --- a/test/src/eval-tests.el +++ b/test/src/eval-tests.el @@ -32,4 +32,19 @@ ;; This should not crash. (should-error (funcall '(closure)) :type 'invalid-function)) +(ert-deftest eval-tests--bugs-24912-and-24913 () + "Checks that Emacs doesn’t accept weird argument lists. +Bug#24912 and Bug#24913." + (dolist (args '((&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 (lambda ,args)) t) :type 'invalid-function) + (should-error (byte-compile-check-lambda-list args)) + (let ((byte-compile-debug t)) + (should-error (eval `(byte-compile (lambda ,args)) t))))) + ;;; eval-tests.el ends here -- 2.8.0.rc3.226.g39d4020 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* bug#24913: 25.1.50; Emacs accepts undocumented and confusing combinations of &optional and &rest in argument lists 2016-11-10 12:58 ` Philipp Stephani @ 2016-11-10 16:56 ` Gemini Lasswell 2016-11-19 16:40 ` Philipp Stephani 2016-11-18 9:06 ` Eli Zaretskii 1 sibling, 1 reply; 8+ messages in thread From: Gemini Lasswell @ 2016-11-10 16:56 UTC (permalink / raw) To: Philipp Stephani, 24913 > Philipp Stephani <p.stephani2@gmail.com> schrieb am Mi., 9. Nov. 2016 um 22:19 Uhr: > > Here's a patch that detects such argument lists. > A more general solution would be to have the byte compiler try to match Edebug specs, and issue a warning or an error if it fails. That would help find errors in the invocations of all macros, not just ones with argument lists. ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#24913: 25.1.50; Emacs accepts undocumented and confusing combinations of &optional and &rest in argument lists 2016-11-10 16:56 ` Gemini Lasswell @ 2016-11-19 16:40 ` Philipp Stephani 2016-11-20 6:31 ` Gemini Lasswell 0 siblings, 1 reply; 8+ messages in thread From: Philipp Stephani @ 2016-11-19 16:40 UTC (permalink / raw) To: Gemini Lasswell, 24913 [-- Attachment #1: Type: text/plain, Size: 634 bytes --] Gemini Lasswell <gazally@runbox.com> schrieb am Do., 10. Nov. 2016 um 17:56 Uhr: > > > Philipp Stephani <p.stephani2@gmail.com> schrieb am Mi., 9. Nov. 2016 > um 22:19 Uhr: > > > > Here's a patch that detects such argument lists. > > > A more general solution would be to have the byte compiler try to match > Edebug specs, and issue a warning or an error if it fails. That would > help find errors in the invocations of all macros, not just ones with > argument lists. > I don't understand how this is related. This is only about function definitions in the evaluator and the byte compiler. I don't see how Edebug could help here. [-- Attachment #2: Type: text/html, Size: 1196 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#24913: 25.1.50; Emacs accepts undocumented and confusing combinations of &optional and &rest in argument lists 2016-11-19 16:40 ` Philipp Stephani @ 2016-11-20 6:31 ` Gemini Lasswell 2016-11-20 12:41 ` Philipp Stephani 0 siblings, 1 reply; 8+ messages in thread From: Gemini Lasswell @ 2016-11-20 6:31 UTC (permalink / raw) To: Philipp Stephani; +Cc: 24913 Philipp Stephani <p.stephani2@gmail.com> writes: > A more general solution would be to have the byte compiler try to match > Edebug specs, and issue a warning or an error if it fails. That would > help find errors in the invocations of all macros, not just ones with > argument lists. > > I don't understand how this is related. This is only about function > definitions in the evaluator and the byte compiler. I don't see how > Edebug could help here. Edebug specs describe the expected syntax for the arguments of a macro, including the macros which define functions, such as lambda, defun, defmacro etc. If Edebug can't match the actual arguments in a macro invocation to the Edebug spec for that macro, it will signal an error. For an example of Edebug catching a misplaced &optional in an argument list, see bug#24762. So part of my suggestion is that since there exists in Emacs a powerful mechanism for checking macro argument lists, it would be better to use it if we want to let programmers know that their macro invocations are incorrect, instead of adding error checking to individual macros on a case by case basis. Another thought going into this suggestion is my observation that it's not difficult to find bugs in Edebug specs in the Emacs sources right now. One cause of that could be the Edebug spec documentation, which could be improved. But I think the primary reason is that a macro with a broken Edebug spec won't cause an error until someone tries to use Edebug or Testcover on an invocation of that macro, which maybe isn't common practice when reviewing changes. But if the byte compiler checked Edebug specs and signaled errors, then at least those errors in Edebug specs which can be found statically would be noticed immediately. ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#24913: 25.1.50; Emacs accepts undocumented and confusing combinations of &optional and &rest in argument lists 2016-11-20 6:31 ` Gemini Lasswell @ 2016-11-20 12:41 ` Philipp Stephani 0 siblings, 0 replies; 8+ messages in thread From: Philipp Stephani @ 2016-11-20 12:41 UTC (permalink / raw) To: Gemini Lasswell; +Cc: 24913 [-- Attachment #1: Type: text/plain, Size: 2271 bytes --] Gemini Lasswell <gazally@runbox.com> schrieb am So., 20. Nov. 2016 um 07:31 Uhr: > Philipp Stephani <p.stephani2@gmail.com> writes: > > > A more general solution would be to have the byte compiler try to match > > Edebug specs, and issue a warning or an error if it fails. That would > > help find errors in the invocations of all macros, not just ones with > > argument lists. > > > > I don't understand how this is related. This is only about function > > definitions in the evaluator and the byte compiler. I don't see how > > Edebug could help here. > > Edebug specs describe the expected syntax for the arguments of a macro, > including the macros which define functions, such as lambda, defun, > defmacro etc. If Edebug can't match the actual arguments in a macro > invocation to the Edebug spec for that macro, it will signal an error. > For an example of Edebug catching a misplaced &optional in an argument > list, see bug#24762. > > So part of my suggestion is that since there exists in Emacs a powerful > mechanism for checking macro argument lists, it would be better to use > it if we want to let programmers know that their macro invocations are > incorrect, instead of adding error checking to individual macros on a > case by case basis. > > Another thought going into this suggestion is my observation that it's > not difficult to find bugs in Edebug specs in the Emacs sources right > now. One cause of that could be the Edebug spec documentation, which > could be improved. But I think the primary reason is that a macro with a > broken Edebug spec won't cause an error until someone tries to use > Edebug or Testcover on an invocation of that macro, which maybe isn't > common practice when reviewing changes. But if the byte compiler checked > Edebug specs and signaled errors, then at least those errors in Edebug > specs which can be found statically would be noticed immediately. > Integrating Edebug checks, byte-compiler checks, and evaluator checks sounds like the right thing to do, but also like a huge amount of work that is out of scope for this bug. Please create a new bug if you want that. Note that a generic checker would still need to catch cases such as (funcall (function (lambda (&rest))) ()) that don't involve any macros. [-- Attachment #2: Type: text/html, Size: 3414 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#24913: 25.1.50; Emacs accepts undocumented and confusing combinations of &optional and &rest in argument lists 2016-11-10 12:58 ` Philipp Stephani 2016-11-10 16:56 ` Gemini Lasswell @ 2016-11-18 9:06 ` Eli Zaretskii 2016-11-18 17:37 ` Philipp Stephani 1 sibling, 1 reply; 8+ messages in thread From: Eli Zaretskii @ 2016-11-18 9:06 UTC (permalink / raw) To: Philipp Stephani; +Cc: 24913 > From: Philipp Stephani <p.stephani2@gmail.com> > Date: Thu, 10 Nov 2016 12:58:39 +0000 > > Philipp Stephani <p.stephani2@gmail.com> schrieb am Mi., 9. Nov. 2016 um 22:19 Uhr: > > For example: > > (funcall (lambda (&optional &rest &rest &optional x) (list x)) 'a) > => ((a)) > > Obviously here the &rest keyword "wins", but I think that's overly > confusing. Such an argument list is most likely a programmer mistake, > and should signal an error to make the programmer aware of the mistake. > > Here's a patch that detects such argument lists. Thanks, please push to master. ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#24913: 25.1.50; Emacs accepts undocumented and confusing combinations of &optional and &rest in argument lists 2016-11-18 9:06 ` Eli Zaretskii @ 2016-11-18 17:37 ` Philipp Stephani 0 siblings, 0 replies; 8+ messages in thread From: Philipp Stephani @ 2016-11-18 17:37 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 24913-done [-- Attachment #1: Type: text/plain, Size: 702 bytes --] Eli Zaretskii <eliz@gnu.org> schrieb am Fr., 18. Nov. 2016 um 10:06 Uhr: > > From: Philipp Stephani <p.stephani2@gmail.com> > > Date: Thu, 10 Nov 2016 12:58:39 +0000 > > > > Philipp Stephani <p.stephani2@gmail.com> schrieb am Mi., 9. Nov. 2016 > um 22:19 Uhr: > > > > For example: > > > > (funcall (lambda (&optional &rest &rest &optional x) (list x)) 'a) > > => ((a)) > > > > Obviously here the &rest keyword "wins", but I think that's overly > > confusing. Such an argument list is most likely a programmer mistake, > > and should signal an error to make the programmer aware of the mistake. > > > > Here's a patch that detects such argument lists. > > Thanks, please push to master. > Done. [-- Attachment #2: Type: text/html, Size: 1583 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-11-20 12:41 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-09 21:17 bug#24913: 25.1.50; Emacs accepts undocumented and confusing combinations of &optional and &rest in argument lists Philipp Stephani 2016-11-10 12:58 ` Philipp Stephani 2016-11-10 16:56 ` Gemini Lasswell 2016-11-19 16:40 ` Philipp Stephani 2016-11-20 6:31 ` Gemini Lasswell 2016-11-20 12:41 ` Philipp Stephani 2016-11-18 9:06 ` Eli Zaretskii 2016-11-18 17:37 ` Philipp Stephani
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).