From 750bc57cbb8d081566e671e8fc3e27a82588c197 Mon Sep 17 00:00:00 2001 From: Stefan Monnier Date: Sat, 18 Feb 2023 12:56:24 -0500 Subject: [PATCH] Don't rely on dynamic scoping to fix bug#59213 Rather than look up a dynamically scoped var to decide whether to trim closures, use an ad-hoc marker on those closures which should not be trimmed. * lisp/emacs-lisp/cconv.el (cconv-dont-trim-unused-variables): Delete var. (cconv-make-interpreted-closure): Use a `:closure-dont-trim-context` markers instead. * lisp/emacs-lisp/edebug.el (edebug-make-enter-wrapper): Use `:closure-dont-trim-context` rather than `cconv-dont-trim-unused-variables`. * lisp/emacs-lisp/testcover.el (testcover-analyze-coverage): Remove workaround for `cconv-dont-trim-unused-variables`. * test/lisp/emacs-lisp/cconv-tests.el (cconv-safe-for-space): New test. --- lisp/emacs-lisp/cconv.el | 15 +++++++++------ lisp/emacs-lisp/edebug.el | 22 ++++++++++++---------- lisp/emacs-lisp/testcover.el | 5 ----- test/lisp/emacs-lisp/cconv-tests.el | 13 +++++++++++++ 4 files changed, 34 insertions(+), 21 deletions(-) diff --git a/lisp/emacs-lisp/cconv.el b/lisp/emacs-lisp/cconv.el index b8121aeba55..940a1045625 100644 --- a/lisp/emacs-lisp/cconv.el +++ b/lisp/emacs-lisp/cconv.el @@ -113,10 +113,6 @@ is less than this number.") (defvar cconv--dynbound-variables nil "List of variables known to be dynamically bound.") -(defvar cconv-dont-trim-unused-variables nil - "When bound to non-nil, don't remove unused variables from the environment. -This is intended for use by edebug and similar.") - ;;;###autoload (defun cconv-closure-convert (form &optional dynbound-vars) "Main entry point for closure conversion. @@ -882,15 +878,22 @@ lexically and dynamically bound symbols actually used by FORM." (cons fvs dyns))))) (defun cconv-make-interpreted-closure (fun env) + ;; FIXME: I don't know what "This function is evaluated both at + ;; compile time and run time" is intended to mean here. "Make a closure for the interpreter. This function is evaluated both at compile time and run time. FUN, the closure's function, must be a lambda form. ENV, the closure's environment, is a mixture of lexical bindings of the form -(SYMBOL . VALUE) and symbols which indicate dynamic bindings of those +\(SYMBOL . VALUE) and symbols which indicate dynamic bindings of those symbols." (cl-assert (eq (car-safe fun) 'lambda)) (let ((lexvars (delq nil (mapcar #'car-safe env)))) - (if (or cconv-dont-trim-unused-variables (null lexvars)) + (if (or (null lexvars) + ;; Functions with a `:closure-dont-trim-context' marker + ;; should keep their whole context untrimmed (bug#59213). + (and (eq :closure-dont-trim-context (nth 2 fun)) + ;; Check the function doesn't just return the magic keyword. + (nthcdr 3 fun))) ;; The lexical environment is empty, or needs to be preserved, ;; so there's no need to look for free variables. ;; Attempting to replace ,(cdr fun) by a macroexpanded version diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el index 735a358cdba..552526b6efc 100644 --- a/lisp/emacs-lisp/edebug.el +++ b/lisp/emacs-lisp/edebug.el @@ -1217,16 +1217,18 @@ purpose by adding an entry to this alist, and setting (setq edebug-old-def-name nil)) (setq edebug-def-name (or edebug-def-name edebug-old-def-name (gensym "edebug-anon"))) - `(let ((cconv-dont-trim-unused-variables t)) - (edebug-enter - (quote ,edebug-def-name) - ,(if edebug-inside-func - `(list - ;; Doesn't work with more than one def-body!! - ;; But the list will just be reversed. - ,@(nreverse edebug-def-args)) - 'nil) - (function (lambda () ,@forms))))) + `(edebug-enter + (quote ,edebug-def-name) + ,(if edebug-inside-func + `(list + ;; Doesn't work with more than one def-body!! + ;; But the list will just be reversed. + ,@(nreverse edebug-def-args)) + 'nil) + ;; Make sure `forms' is not nil so we don't accidentally return + ;; the magic keyword. Mark the closure so we don't throw away + ;; unused vars (bug#59213). + #'(lambda () :closure-dont-trim-context ,@(or forms '(nil))))) (defvar edebug-form-begin-marker) ; the mark for def being instrumented diff --git a/lisp/emacs-lisp/testcover.el b/lisp/emacs-lisp/testcover.el index 1212905f08a..ed31b90ca32 100644 --- a/lisp/emacs-lisp/testcover.el +++ b/lisp/emacs-lisp/testcover.el @@ -442,11 +442,6 @@ or return multiple values." (let ((testcover-vector (get sym 'edebug-coverage))) (testcover-analyze-coverage-progn body))) - (`(let ((cconv-dont-trim-unused-variables t)) - (edebug-enter ',sym ,_ (function (lambda nil . ,body)))) - (let ((testcover-vector (get sym 'edebug-coverage))) - (testcover-analyze-coverage-progn body))) - (`(edebug-after ,(and before-form (or `(edebug-before ,before-id) before-id)) ,after-id ,wrapped-form) diff --git a/test/lisp/emacs-lisp/cconv-tests.el b/test/lisp/emacs-lisp/cconv-tests.el index 83013cf46a9..349ffeb7e47 100644 --- a/test/lisp/emacs-lisp/cconv-tests.el +++ b/test/lisp/emacs-lisp/cconv-tests.el @@ -364,5 +364,18 @@ (call-interactively f)) '((t 51696) (nil 51695) (t 51697))))))) +(ert-deftest cconv-safe-for-space () + (let* ((magic-string "This-is-a-magic-string") + (safe-p (lambda (x) (not (string-match magic-string (format "%S" x)))))) + (should (funcall safe-p (lambda (x) (+ x 1)))) + (should (funcall safe-p (eval '(lambda (x) (+ x 1)) + `((y . ,magic-string))))) + (should (funcall safe-p (eval '(lambda (x) :closure-dont-trim-context) + `((y . ,magic-string))))) + (should-not (funcall safe-p + (eval '(lambda (x) :closure-dont-trim-context (+ x 1)) + `((y . ,magic-string))))))) + + (provide 'cconv-tests) ;;; cconv-tests.el ends here -- 2.39.2