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

[Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

> With an `add' instrumented for edebug, and evaluating `add', this causes
> edebug to create the form beginning "(function ...".  Ffunction in eval.c
> delegates the creation of a closure to cconv-make-interpreted-closure.
> That function analyses `add', decides that c is not used, and thus
> creates a lexical environment containing bindings only for a and b.
>
> This last is the error.  When instrumenting for edebug, EVERY lexical
> variable is potentially going to be read, so
> cconv-make-interpreted-closure should not remove any elements from the
> lexical environment.

Yup.

> The included patch fixes this: edebug binds the (new) variable
> cconv-dont-trim-unused-variables to non-nil around the generated calls to
> edebug-enter.  cconv-make-interpreted-closure tests this variable, and
> when non-nil it copies the lexical environment without change.
>
> Also, there's a consequential change in testcover.el, where it analyses
> the forms it is instrumenting, and needs to handle the new code around
> edebug-enter.
>
> This works.
>
> What do you think?

I think that's good enough for `emacs-29`, yes.
I don't like the use of a dynbound variable to control this, but it's
not clear how to do better.  One thing that occurred to me right now is
that we could mark the Edebug closures themselves, e.g. by replacing

    (function (lambda () ,@forms))

with

    (function (lambda () :closure-dont-trim-context ,@forms))

and then have `cconv-make-interpreted-closure` look for this
tell-tale sign.

A few minor comments about your patch below.

> +(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.")

This variable name sounds like it controls the closure conversion code
(e.g. `cconv-convert`).  I don't have a better suggestion, tho :-(

> @@ -875,15 +882,24 @@ cconv-fv
>          (cons fvs dyns)))))
>  
>  (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."
>    (cl-assert (eq (car-safe fun) 'lambda))
>    (let ((lexvars (delq nil (mapcar #'car-safe env))))
>      (if (null lexvars)
>          ;; The lexical environment is empty, so there's no need to
>          ;; look for free variables.
> +        ;; Attempting to replace ,(cdr fun) by a macroexpanded version
> +        ;; causes bootstrap to fail.
>          `(closure ,env . ,(cdr fun))
>        ;; We could try and cache the result of the macroexpansion and
>        ;; `cconv-fv' analysis.  Not sure it's worth the trouble.
> -      (let* ((form `#',fun)
> +      (let* (newenv
> +             (form `#',fun)
>               (expanded-form
>                (let ((lexical-binding t) ;; Tell macros which dialect is in use.
>  	            ;; Make the macro aware of any defvar declarations in scope.
> @@ -896,11 +912,14 @@ cconv-make-interpreted-closure
>                (pcase expanded-form
>                  (`#'(lambda . ,cdr) cdr)
>                  (_ (cdr fun))))
> -         
> -             (dynvars (delq nil (mapcar (lambda (b) (if (symbolp b) b)) env)))
> -             (fvs (cconv-fv expanded-form lexvars dynvars))
> -             (newenv (nconc (mapcar (lambda (fv) (assq fv env)) (car fvs))
> -                            (cdr fvs))))
> +
> +             (dynvars (delq nil (mapcar (lambda (b) (if (symbolp b) b)) env))))
> +        (if cconv-dont-trim-unused-variables
> +            (setq newenv (copy-alist env))
> +          (let ((fvs (cconv-fv expanded-form lexvars dynvars)))
> +            (setq newenv
> +                  (nconc (mapcar (lambda (fv) (assq fv env)) (car fvs))
> +                         (cdr fvs)))))
>          ;; Never return a nil env, since nil means to use the dynbind
>          ;; dialect of ELisp.
>          `(closure ,(or newenv '(t)) . ,expanded-fun-cdr)))))

Hmm... any reason why we can't just replace

    (if (null lexvars)

with

    (if (or cconv-dont-trim-unused-variables (null lexvars))

and be done with it?


        Stefan