From d60e930d34fe0f4a88a790f98dcd43999327240c Mon Sep 17 00:00:00 2001 From: Stefan Monnier Date: Sat, 3 Sep 2022 10:46:46 -0400 Subject: * lisp/emacs-lisp/cl-macs.el: Use `define-symbol-prop` (bug#50869) (cl-define-compiler-macro, cl-defstruct, cl-deftype): Prefer `define-symbol-prop` over `put` so `unload-feature` can undo those definitions. --- lisp/emacs-lisp/cl-macs.el | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'lisp/emacs-lisp/cl-macs.el') diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el index 80ca43c902a..edd633675dc 100644 --- a/lisp/emacs-lisp/cl-macs.el +++ b/lisp/emacs-lisp/cl-macs.el @@ -3105,7 +3105,7 @@ To see the documentation for a defined struct type, use `(and ,pred-form t))) forms) (push `(eval-and-compile - (put ',name 'cl-deftype-satisfies ',predicate)) + (define-symbol-prop ',name 'cl-deftype-satisfies ',predicate)) forms)) (let ((pos 0) (descp descs)) (while descp @@ -3570,7 +3570,7 @@ and then returning foo." (cl-defun ,fname ,(if (memq '&whole args) (delq '&whole args) (cons '_cl-whole-arg args)) ,@body) - (put ',func 'compiler-macro #',fname)))) + (define-symbol-prop ',func 'compiler-macro #',fname)))) ;;;###autoload (defun cl-compiler-macroexpand (form) @@ -3679,8 +3679,8 @@ macro that returns its `&whole' argument." The type name can then be used in `cl-typecase', `cl-check-type', etc." (declare (debug cl-defmacro) (doc-string 3) (indent 2)) `(cl-eval-when (compile load eval) - (put ',name 'cl-deftype-handler - (cl-function (lambda (&cl-defs ('*) ,@arglist) ,@body))))) + (define-symbol-prop ',name 'cl-deftype-handler + (cl-function (lambda (&cl-defs ('*) ,@arglist) ,@body))))) (cl-deftype extended-char () '(and character (not base-char))) ;; Define fixnum so `cl-typep' recognize it and the type check emitted -- cgit v1.2.3 From 2dd1c2ab19f7fb99ecee60e27e63b2fb045f6970 Mon Sep 17 00:00:00 2001 From: Stefan Monnier Date: Sat, 3 Sep 2022 22:38:28 -0400 Subject: gv.el and cl-macs.el: Fix bug#57397 * lisp/emacs-lisp/gv.el (gv-get): Obey symbol macros. * lisp/emacs-lisp/cl-macs.el (cl--letf): Remove workaround placed to try and handle symbol macros. * test/lisp/emacs-lisp/cl-macs-tests.el (cl-macs-test--symbol-macrolet): Add new testcase. --- lisp/emacs-lisp/cl-macs.el | 2 +- lisp/emacs-lisp/gv.el | 6 +++++- test/lisp/emacs-lisp/cl-macs-tests.el | 15 ++++++++++++++- 3 files changed, 20 insertions(+), 3 deletions(-) (limited to 'lisp/emacs-lisp/cl-macs.el') diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el index edd633675dc..9755c2636de 100644 --- a/lisp/emacs-lisp/cl-macs.el +++ b/lisp/emacs-lisp/cl-macs.el @@ -2762,7 +2762,7 @@ Each PLACE may be a symbol, or any generalized variable allowed by `setf'. (funcall setter vold))) binds)))) (let* ((binding (car bindings)) - (place (macroexpand (car binding) macroexpand-all-environment))) + (place (car binding))) (gv-letplace (getter setter) place (macroexp-let2 nil vnew (cadr binding) (if (symbolp place) diff --git a/lisp/emacs-lisp/gv.el b/lisp/emacs-lisp/gv.el index eaab6439adb..1db9d96d999 100644 --- a/lisp/emacs-lisp/gv.el +++ b/lisp/emacs-lisp/gv.el @@ -87,7 +87,11 @@ with a (not necessarily copyable) Elisp expression that returns the value to set it to. DO must return an Elisp expression." (cond - ((symbolp place) (funcall do place (lambda (v) `(setq ,place ,v)))) + ((symbolp place) + (let ((me (macroexpand-1 place macroexpand-all-environment))) + (if (eq me place) + (funcall do place (lambda (v) `(setq ,place ,v))) + (gv-get me do)))) ((not (consp place)) (signal 'gv-invalid-place (list place))) (t (let* ((head (car place)) diff --git a/test/lisp/emacs-lisp/cl-macs-tests.el b/test/lisp/emacs-lisp/cl-macs-tests.el index 19ede627a13..2a647e08305 100644 --- a/test/lisp/emacs-lisp/cl-macs-tests.el +++ b/test/lisp/emacs-lisp/cl-macs-tests.el @@ -539,7 +539,20 @@ collection clause." ((p (gv-synthetic-place cl (lambda (v) `(setcar l ,v))))) (cl-incf p))) l) - '(1)))) + '(1))) + ;; Make sure `gv-synthetic-place' isn't macro-expanded before + ;; `cl-letf' gets to see its `gv-expander'. + (should (equal + (condition-case err + (let ((x 1)) + (list x + (cl-letf (((gv-synthetic-place (+ 1 2) + (lambda (v) `(setq x ,v))) + 7)) + x) + x)) + (error err)) + '(1 7 3)))) (ert-deftest cl-macs-loop-conditional-step-clauses () "These tests failed under the initial fixes in #bug#29799." -- cgit v1.2.3 From 2a78f06ef4d303b383749be3dabd0f9a68547e5e Mon Sep 17 00:00:00 2001 From: Stefan Monnier Date: Tue, 6 Sep 2022 00:08:35 -0400 Subject: cl-symbol-macrolet: Fix recent regression The recent fix for bug#57397 introduced a regression, breaking the `cl-lib-symbol-macrolet-hide` test. It turned out that the origin of the problem was that `gv.el` uses `macroexpand-1` which does not (can't) use `macroexpand` but `cl-symbol-macrolet` failed to advise `macroexpand-1` the way it advised `macroexpand`. To fix this, we change `cl-symbol-macrolet` so it advises both, and we do that with a new `macroexpand` advice which delegates the bulk of the work to `macroexpand-1`. Along the way, I bumped into another bug in the interaction between `cl-letf` and `cl-symbol-macrolet`, which I tried to fix in `cl-letf`. I hear the war on `cl-symbol-macrolet` was a failure. Maybe ... just say no? * lisp/emacs-lisp/cl-macs.el (cl--sm-macroexpand-1): New function, extracted from `cl--sm-macroexpand`. (cl--sm-macroexpand): Rewrite completely. (cl-symbol-macrolet): Advise both `macroexpand` and `macroexpand-1`. (cl--letf): Don't use the "simple variable" code for symbol macros. * test/lisp/emacs-lisp/cl-lib-tests.el (cl-lib-symbol-macrolet-hide): Revert last change because the test was right. * test/lisp/emacs-lisp/cl-macs-tests.el (cl-macs-test--symbol-macrolet): Add a test case. --- lisp/emacs-lisp/cl-macs.el | 266 +++++++++++++++++----------------- test/lisp/emacs-lisp/cl-lib-tests.el | 3 - test/lisp/emacs-lisp/cl-macs-tests.el | 9 +- 3 files changed, 141 insertions(+), 137 deletions(-) (limited to 'lisp/emacs-lisp/cl-macs.el') diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el index 9755c2636de..f8fdc50251f 100644 --- a/lisp/emacs-lisp/cl-macs.el +++ b/lisp/emacs-lisp/cl-macs.el @@ -2261,139 +2261,131 @@ This is like `cl-flet', but for macros instead of functions. (eval `(function (lambda ,@res)) t)) macroexpand-all-environment)))))) -(defun cl--sm-macroexpand (orig-fun exp &optional env) +(defun cl--sm-macroexpand (exp &optional env) + "Special macro expander used inside `cl-symbol-macrolet'." + ;; FIXME: Arguably, this should be the official definition of `macroexpand'. + (while (not (eq exp (setq exp (macroexpand-1 exp env))))) + exp) + +(defun cl--sm-macroexpand-1 (orig-fun exp &optional env) "Special macro expander advice used inside `cl-symbol-macrolet'. -This function extends `macroexpand' during macro expansion +This function extends `macroexpand-1' during macro expansion of `cl-symbol-macrolet' to additionally expand symbol macros." - (let ((macroexpand-all-environment env) + (let ((exp (funcall orig-fun exp env)) (venv (alist-get :cl-symbol-macros env))) - (while - (progn - (setq exp (funcall orig-fun exp env)) - (pcase exp - ((pred symbolp) - ;; Perform symbol-macro expansion. - (let ((symval (assq exp venv))) - (when symval - (setq exp (cadr symval))))) - (`(setq . ,args) - ;; Convert setq to setf if required by symbol-macro expansion. - (let ((convert nil) - (rargs nil)) - (while args - (let ((place (pop args))) - ;; Here, we know `place' should be a symbol. - (while - (let ((symval (assq place venv))) - (when symval - (setq place (cadr symval)) - (if (symbolp place) - t ;Repeat. - (setq convert t) - nil)))) - (push place rargs) - (push (pop args) rargs))) - (setq exp (cons (if convert 'setf 'setq) - (nreverse rargs))) - convert)) - ;; CL's symbol-macrolet used to treat re-bindings as candidates for - ;; expansion (turning the let into a letf if needed), contrary to - ;; Common-Lisp where such re-bindings hide the symbol-macro. - ;; Not sure if there actually is code out there which depends - ;; on this behavior (haven't found any yet). - ;; Such code should explicitly use `cl-letf' instead, I think. - ;; - ;; (`(,(or `let `let*) . ,(or `(,bindings . ,body) pcase--dontcare)) - ;; (let ((letf nil) (found nil) (nbs ())) - ;; (dolist (binding bindings) - ;; (let* ((var (if (symbolp binding) binding (car binding))) - ;; (sm (assq var venv))) - ;; (push (if (not (cdr sm)) - ;; binding - ;; (let ((nexp (cadr sm))) - ;; (setq found t) - ;; (unless (symbolp nexp) (setq letf t)) - ;; (cons nexp (cdr-safe binding)))) - ;; nbs))) - ;; (when found - ;; (setq exp `(,(if letf - ;; (if (eq (car exp) 'let) 'cl-letf 'cl-letf*) - ;; (car exp)) - ;; ,(nreverse nbs) - ;; ,@body))))) - ;; - ;; We implement the Common-Lisp behavior, instead (see bug#26073): - ;; The behavior of CL made sense in a dynamically scoped - ;; language, but nowadays, lexical scoping semantics is more often - ;; expected. - (`(,(or 'let 'let*) . ,(or `(,bindings . ,body) pcase--dontcare)) - (let ((nbs ()) (found nil)) - (dolist (binding bindings) - (let* ((var (if (symbolp binding) binding (car binding))) - (val (and found (consp binding) (eq 'let* (car exp)) - (list (macroexpand-all (cadr binding) - env))))) - (push (if (assq var venv) - ;; This binding should hide "its" surrounding - ;; symbol-macro, but given the way macroexpand-all - ;; works (i.e. the `env' we receive as input will - ;; be (re)applied to the code we return), we can't - ;; prevent application of `env' to the - ;; sub-expressions, so we need to α-rename this - ;; variable instead. - (let ((nvar (make-symbol (symbol-name var)))) - (setq found t) - (push (list var nvar) venv) - (push (cons :cl-symbol-macros venv) env) - (cons nvar (or val (cdr-safe binding)))) - (if val (cons var val) binding)) - nbs))) - (when found - (setq exp `(,(car exp) - ,(nreverse nbs) - ,@(macroexp-unprogn - (macroexpand-all (macroexp-progn body) - env))))) - nil)) - ;; Do the same as for `let' but for variables introduced - ;; via other means, such as `lambda' and `condition-case'. - (`(function (lambda ,args . ,body)) - (let ((nargs ()) (found nil)) - (dolist (var args) - (push (cond - ((memq var '(&optional &rest)) var) - ((assq var venv) - (let ((nvar (make-symbol (symbol-name var)))) - (setq found t) - (push (list var nvar) venv) - (push (cons :cl-symbol-macros venv) env) - nvar)) - (t var)) - nargs)) - (when found - (setq exp `(function - (lambda ,(nreverse nargs) - . ,(mapcar (lambda (exp) - (macroexpand-all exp env)) - body))))) - nil)) - ((and `(condition-case ,var ,exp . ,clauses) - (guard (assq var venv))) - (let ((nvar (make-symbol (symbol-name var)))) - (push (list var nvar) venv) - (push (cons :cl-symbol-macros venv) env) - (setq exp - `(condition-case ,nvar ,(macroexpand-all exp env) - . ,(mapcar - (lambda (clause) - `(,(car clause) - . ,(mapcar (lambda (exp) - (macroexpand-all exp env)) - (cdr clause)))) - clauses))) - nil)) - ))) - exp)) + (pcase exp + ((pred symbolp) + ;; Try symbol-macro expansion. + (let ((symval (assq exp venv))) + (if symval (cadr symval) exp))) + (`(setq . ,args) + ;; Convert setq to setf if required by symbol-macro expansion. + (let ((convert nil)) + (while args + (let* ((place (pop args)) + ;; Here, we know `place' should be a symbol. + (symval (assq place venv))) + (pop args) + (when symval + (setq convert t)))) + (if convert + (cons 'setf (cdr exp)) + exp))) + ;; CL's symbol-macrolet used to treat re-bindings as candidates for + ;; expansion (turning the let into a letf if needed), contrary to + ;; Common-Lisp where such re-bindings hide the symbol-macro. + ;; Not sure if there actually is code out there which depends + ;; on this behavior (haven't found any yet). + ;; Such code should explicitly use `cl-letf' instead, I think. + ;; + ;; (`(,(or `let `let*) . ,(or `(,bindings . ,body) pcase--dontcare)) + ;; (let ((letf nil) (found nil) (nbs ())) + ;; (dolist (binding bindings) + ;; (let* ((var (if (symbolp binding) binding (car binding))) + ;; (sm (assq var venv))) + ;; (push (if (not (cdr sm)) + ;; binding + ;; (let ((nexp (cadr sm))) + ;; (setq found t) + ;; (unless (symbolp nexp) (setq letf t)) + ;; (cons nexp (cdr-safe binding)))) + ;; nbs))) + ;; (when found + ;; (setq exp `(,(if letf + ;; (if (eq (car exp) 'let) 'cl-letf 'cl-letf*) + ;; (car exp)) + ;; ,(nreverse nbs) + ;; ,@body))))) + ;; + ;; We implement the Common-Lisp behavior, instead (see bug#26073): + ;; The behavior of CL made sense in a dynamically scoped + ;; language, but nowadays, lexical scoping semantics is more often + ;; expected. + (`(,(or 'let 'let*) . ,(or `(,bindings . ,body) pcase--dontcare)) + (let ((nbs ()) (found nil)) + (dolist (binding bindings) + (let* ((var (if (symbolp binding) binding (car binding))) + (val (and found (consp binding) (eq 'let* (car exp)) + (list (macroexpand-all (cadr binding) + env))))) + (push (if (assq var venv) + ;; This binding should hide "its" surrounding + ;; symbol-macro, but given the way macroexpand-all + ;; works (i.e. the `env' we receive as input will + ;; be (re)applied to the code we return), we can't + ;; prevent application of `env' to the + ;; sub-expressions, so we need to α-rename this + ;; variable instead. + (let ((nvar (make-symbol (symbol-name var)))) + (setq found t) + (push (list var nvar) venv) + (push (cons :cl-symbol-macros venv) env) + (cons nvar (or val (cdr-safe binding)))) + (if val (cons var val) binding)) + nbs))) + (if found + `(,(car exp) + ,(nreverse nbs) + ,@(macroexp-unprogn + (macroexpand-all (macroexp-progn body) + env))) + exp))) + ;; Do the same as for `let' but for variables introduced + ;; via other means, such as `lambda' and `condition-case'. + (`(function (lambda ,args . ,body)) + (let ((nargs ()) (found nil)) + (dolist (var args) + (push (cond + ((memq var '(&optional &rest)) var) + ((assq var venv) + (let ((nvar (make-symbol (symbol-name var)))) + (setq found t) + (push (list var nvar) venv) + (push (cons :cl-symbol-macros venv) env) + nvar)) + (t var)) + nargs)) + (if found + `(function + (lambda ,(nreverse nargs) + . ,(mapcar (lambda (exp) + (macroexpand-all exp env)) + body))) + exp))) + ((and `(condition-case ,var ,exp . ,clauses) + (guard (assq var venv))) + (let ((nvar (make-symbol (symbol-name var)))) + (push (list var nvar) venv) + (push (cons :cl-symbol-macros venv) env) + `(condition-case ,nvar ,(macroexpand-all exp env) + . ,(mapcar + (lambda (clause) + `(,(car clause) + . ,(mapcar (lambda (exp) + (macroexpand-all exp env)) + (cdr clause)))) + clauses)))) + (_ exp)))) ;;;###autoload (defmacro cl-symbol-macrolet (bindings &rest body) @@ -2412,7 +2404,8 @@ by EXPANSION, and (setq NAME ...) will act like (setf EXPANSION ...). (unwind-protect (progn (unless advised - (advice-add 'macroexpand :around #'cl--sm-macroexpand)) + (advice-add 'macroexpand :override #'cl--sm-macroexpand) + (advice-add 'macroexpand-1 :around #'cl--sm-macroexpand-1)) (let* ((venv (cdr (assq :cl-symbol-macros macroexpand-all-environment))) (expansion @@ -2428,7 +2421,8 @@ by EXPANSION, and (setq NAME ...) will act like (setf EXPANSION ...). expansion nil nil rev-malformed-bindings)) expansion))) (unless advised - (advice-remove 'macroexpand #'cl--sm-macroexpand))))) + (advice-remove 'macroexpand #'cl--sm-macroexpand) + (advice-remove 'macroexpand-1 #'cl--sm-macroexpand-1))))) ;;;###autoload (defmacro cl-with-gensyms (names &rest body) @@ -2765,8 +2759,14 @@ Each PLACE may be a symbol, or any generalized variable allowed by `setf'. (place (car binding))) (gv-letplace (getter setter) place (macroexp-let2 nil vnew (cadr binding) - (if (symbolp place) + (if (and (symbolp place) + ;; `place' could be some symbol-macro. + (eq place getter)) ;; Special-case for simple variables. + ;; FIXME: We currently only use this special case when `place' + ;; is a simple var. Should we also use it when the + ;; macroexpansion of `place' is a simple var (i.e. when + ;; getter+setter is the same as that of a simple var)? (cl--letf (cdr bindings) (cons `(,getter ,(if (cdr binding) vnew getter)) simplebinds) diff --git a/test/lisp/emacs-lisp/cl-lib-tests.el b/test/lisp/emacs-lisp/cl-lib-tests.el index 8d2b187e33a..b19494af746 100644 --- a/test/lisp/emacs-lisp/cl-lib-tests.el +++ b/test/lisp/emacs-lisp/cl-lib-tests.el @@ -511,9 +511,6 @@ (ert-deftest cl-lib-symbol-macrolet-hide () - :expected-result :failed - ;; FIXME -- it's unclear what the semantics here should be, but - ;; 2dd1c2ab19f7fb99ecee flipped them. ;; bug#26325, bug#26073 (should (equal (let ((y 5)) (cl-symbol-macrolet ((x y)) diff --git a/test/lisp/emacs-lisp/cl-macs-tests.el b/test/lisp/emacs-lisp/cl-macs-tests.el index 2a647e08305..68898720d9c 100644 --- a/test/lisp/emacs-lisp/cl-macs-tests.el +++ b/test/lisp/emacs-lisp/cl-macs-tests.el @@ -552,7 +552,14 @@ collection clause." x) x)) (error err)) - '(1 7 3)))) + '(1 7 3))) + (should (equal + (let ((x (list 42))) + (cl-symbol-macrolet ((m (car x))) + (list m + (cl-letf ((m 5)) m) + m))) + '(42 5 42)))) (ert-deftest cl-macs-loop-conditional-step-clauses () "These tests failed under the initial fixes in #bug#29799." -- cgit v1.2.3 From 6d8f5161ead689b7a2e44a7de0a695f0ab4c833b Mon Sep 17 00:00:00 2001 From: Philipp Stephani Date: Tue, 13 Sep 2022 17:11:53 +0200 Subject: Signal an error if a fallback cl-case is misplaced * lisp/emacs-lisp/cl-macs.el (cl-case): Warn if the user passes a nil key list (which would never match). Warn about quoted symbols that should probably be unquoted. * test/lisp/emacs-lisp/cl-macs-tests.el (cl-case-warning): New unit test (bug#51368). --- lisp/emacs-lisp/cl-macs.el | 9 +++++++-- test/lisp/emacs-lisp/cl-macs-tests.el | 11 +++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) (limited to 'lisp/emacs-lisp/cl-macs.el') diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el index f8fdc50251f..946d2c09a92 100644 --- a/lisp/emacs-lisp/cl-macs.el +++ b/lisp/emacs-lisp/cl-macs.el @@ -775,11 +775,16 @@ compared by `eql'. \(fn EXPR (KEYLIST BODY...)...)" (declare (indent 1) (debug (form &rest (sexp body)))) (macroexp-let2 macroexp-copyable-p temp expr - (let* ((head-list nil)) + (let* ((head-list nil) + (has-otherwise nil)) `(cond ,@(mapcar (lambda (c) - (cons (cond ((memq (car c) '(t otherwise)) t) + (cons (cond (has-otherwise + (error "Misplaced t or `otherwise' clause")) + ((memq (car c) '(t otherwise)) + (setq has-otherwise t) + t) ((eq (car c) 'cl--ecase-error-flag) `(error "cl-ecase failed: %s, %s" ,temp ',(reverse head-list))) diff --git a/test/lisp/emacs-lisp/cl-macs-tests.el b/test/lisp/emacs-lisp/cl-macs-tests.el index 68898720d9c..77817abd85c 100644 --- a/test/lisp/emacs-lisp/cl-macs-tests.el +++ b/test/lisp/emacs-lisp/cl-macs-tests.el @@ -747,4 +747,15 @@ collection clause." ;; Just make sure the forms can be instrumented. (eval-buffer)))) +(ert-deftest cl-case-error () + "Test that `cl-case' and `cl-ecase' signal an error if a t or +`otherwise' key is misplaced." + (dolist (form '((cl-case val (t 1) (123 2)) + (cl-ecase val (t 1) (123 2)) + (cl-ecase val (123 2) (t 1)))) + (ert-info ((prin1-to-string form) :prefix "Form: ") + (let ((error (should-error (macroexpand form)))) + (should (equal (cdr error) + '("Misplaced t or `otherwise' clause"))))))) + ;;; cl-macs-tests.el ends here -- cgit v1.2.3 From fffa53ff1afe097fe38f7664df5debe9811201d1 Mon Sep 17 00:00:00 2001 From: Philipp Stephani Date: Tue, 13 Sep 2022 17:12:57 +0200 Subject: Have 'cl-case' warn about suspicious cases * lisp/emacs-lisp/cl-macs.el (cl-case): Warn if the user passes a nil key list (which would never match). Warn about quoted symbols that should probably be unquoted. * test/lisp/emacs-lisp/cl-macs-tests.el (cl-case-warning): New unit test (bug#51368). --- lisp/emacs-lisp/cl-macs.el | 15 +++++++++++++++ test/lisp/emacs-lisp/cl-macs-tests.el | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) (limited to 'lisp/emacs-lisp/cl-macs.el') diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el index 946d2c09a92..5d330f32d66 100644 --- a/lisp/emacs-lisp/cl-macs.el +++ b/lisp/emacs-lisp/cl-macs.el @@ -788,6 +788,21 @@ compared by `eql'. ((eq (car c) 'cl--ecase-error-flag) `(error "cl-ecase failed: %s, %s" ,temp ',(reverse head-list))) + ((null (car c)) + (macroexp-warn-and-return + "Case nil will never match" + nil 'suspicious)) + ((and (consp (car c)) (not (cddar c)) + (memq (caar c) '(quote function))) + (macroexp-warn-and-return + (format-message + (concat "Case %s will match `%s'. If " + "that's intended, write %s " + "instead. Otherwise, don't " + "quote `%s'.") + (car c) (caar c) (list (cadar c) (caar c)) + (cadar c)) + `(cl-member ,temp ',(car c)) 'suspicious)) ((listp (car c)) (setq head-list (append (car c) head-list)) `(cl-member ,temp ',(car c))) diff --git a/test/lisp/emacs-lisp/cl-macs-tests.el b/test/lisp/emacs-lisp/cl-macs-tests.el index 77817abd85c..427b8f46893 100644 --- a/test/lisp/emacs-lisp/cl-macs-tests.el +++ b/test/lisp/emacs-lisp/cl-macs-tests.el @@ -25,6 +25,8 @@ (require 'cl-macs) (require 'edebug) (require 'ert) +(require 'ert-x) +(require 'pcase) ;;;; cl-loop tests -- many adapted from Steele's CLtL2 @@ -758,4 +760,34 @@ collection clause." (should (equal (cdr error) '("Misplaced t or `otherwise' clause"))))))) +(ert-deftest cl-case-warning () + "Test that `cl-case' and `cl-ecase' warn about suspicious +constructs." + (pcase-dolist (`(,case . ,message) + `((nil . "Case nil will never match") + ('nil . ,(concat "Case 'nil will match `quote'. " + "If that's intended, write " + "(nil quote) instead. " + "Otherwise, don't quote `nil'.")) + ('t . ,(concat "Case 't will match `quote'. " + "If that's intended, write " + "(t quote) instead. " + "Otherwise, don't quote `t'.")) + ('foo . ,(concat "Case 'foo will match `quote'. " + "If that's intended, write " + "(foo quote) instead. " + "Otherwise, don't quote `foo'.")) + (#'foo . ,(concat "Case #'foo will match " + "`function'. If that's " + "intended, write (foo function) " + "instead. Otherwise, don't " + "quote `foo'.")))) + (dolist (macro '(cl-case cl-ecase)) + (let ((form `(,macro val (,case 1)))) + (ert-info ((prin1-to-string form) :prefix "Form: ") + (ert-with-message-capture messages + (macroexpand form) + (should (equal messages + (concat "Warning: " message "\n"))))))))) + ;;; cl-macs-tests.el ends here -- cgit v1.2.3 From a71de4b52d3de14349ded7d88c4cae6e2a9376ae Mon Sep 17 00:00:00 2001 From: Philipp Stephani Date: Mon, 19 Sep 2022 13:34:51 +0200 Subject: Improve check for misleading 'cl-case' cases (Bug#57915). * lisp/emacs-lisp/cl-macs.el (cl-case): Check that the case is of the form (quote FOO), not just (quote). * test/lisp/emacs-lisp/cl-macs-tests.el (cl-case-no-warning): New unit test. --- lisp/emacs-lisp/cl-macs.el | 2 +- test/lisp/emacs-lisp/cl-macs-tests.el | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) (limited to 'lisp/emacs-lisp/cl-macs.el') diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el index 5d330f32d66..beafee1d631 100644 --- a/lisp/emacs-lisp/cl-macs.el +++ b/lisp/emacs-lisp/cl-macs.el @@ -792,7 +792,7 @@ compared by `eql'. (macroexp-warn-and-return "Case nil will never match" nil 'suspicious)) - ((and (consp (car c)) (not (cddar c)) + ((and (consp (car c)) (cdar c) (not (cddar c)) (memq (caar c) '(quote function))) (macroexp-warn-and-return (format-message diff --git a/test/lisp/emacs-lisp/cl-macs-tests.el b/test/lisp/emacs-lisp/cl-macs-tests.el index 83928775f18..f742637ee35 100644 --- a/test/lisp/emacs-lisp/cl-macs-tests.el +++ b/test/lisp/emacs-lisp/cl-macs-tests.el @@ -792,4 +792,15 @@ constructs." (should (equal messages (concat "Warning: " message "\n")))))))))) +(ert-deftest cl-case-no-warning () + "Test that `cl-case' and `cl-ecase' don't warn in some valid cases. +See Bug#57915." + (dolist (case '(quote (quote) function (function))) + (dolist (macro '(cl-case cl-ecase)) + (let ((form `(,macro val (,case 1)))) + (ert-info ((prin1-to-string form) :prefix "Form: ") + (ert-with-message-capture messages + (macroexpand form) + (should (string-empty-p messages)))))))) + ;;; cl-macs-tests.el ends here -- cgit v1.2.3