master 750bc57cbb8: Don't rely on dynamic scoping to fix bug#59213

[Stefan Monnier]

branch: master
commit 750bc57cbb8d081566e671e8fc3e27a82588c197
Author: Stefan Monnier <monnier@iro.umontreal.ca>
Commit: Stefan Monnier <monnier@iro.umontreal.ca>

    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