bug#59213: Emacs 29: Edebug fails to instrument a parameter whose name begins with _

[Alan Mackenzie <acm@muc.de>]

Hello, Stefan,

Sorry you needed to ping me to get an answer to this.

On Tue, Feb 14, 2023 at 17:19:12 -0500, Stefan Monnier wrote:
> Hi Alan,

> >  (defun cconv-make-interpreted-closure (fun env)
> > +  "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
> > +symbols."

> BTW, what did you mean by "This function is evaluated both at compile
> time and run time"?

It was my state of gradually diminishing confusion (which started at a
high level) as I worked through the bug.  It took me some while before it
occurred to me that the creation of closures was happening at run time,
not compile time.  Of course, it's got to, because the lexical
environment only exists at run time.  But I would have got through all
this faster if there had been a comment saying
cconv-make-interpreted-closure is called at run time.  So I put such a
comment in for the next highly confused hacker.

As for the "at compile time", I saw this happening whilst I was running
things in gdb.  I'm not actually sure about this anymore, since these
calls to c-m-i-closure might well have been run time for bits of the
compiler, not compile time.  So maybe the words "both compile time and"
should be removed.  What do you say?

> Also, I append the current state of the patch I plan to install on
> `master`.

Thanks.  I'm not sure what advantages this approach has, but it certainly
gets rid of the ugly cconv-dont-trim-unused-variables.  I'm sure it will
work, too, which is the main thing.

>         Stefan


> diff --git a/lisp/emacs-lisp/cconv.el b/lisp/emacs-lisp/cconv.el
> index b8121aeba55..d055026cb1a 100644
> --- a/lisp/emacs-lisp/cconv.el
> +++ b/lisp/emacs-lisp/cconv.el
> @@ -113,10 +113,6 @@ cconv--interactive-form-funs
>  (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.
> @@ -886,11 +882,16 @@ cconv-make-interpreted-closure
>  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 of the form (lambda (..) :closure-dont-trim-context ..)
> +            ;; 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..4b625dd076e 100644
> --- a/lisp/emacs-lisp/edebug.el
> +++ b/lisp/emacs-lisp/edebug.el
> @@ -1217,16 +1217,17 @@ edebug-make-enter-wrapper
>      (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.
> +    #'(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 @@ testcover-analyze-coverage
>       (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 @@ cconv-tests--intern-all
>                             (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

-- 
Alan Mackenzie (Nuremberg, Germany).