bug#39149: 27.0.50; describe-buffer-bindings is calling :filter function in wrong buffer

[Stefan Kangas <stefankangas@gmail.com>]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> yyoncho <yyoncho@gmail.com> writes:
>
>> To reproduce (eval in scratch buffer)
>> (define-key global-map (kbd "C-c C-l r")
>>   `(menu-item "2" switch-to-buffer
>>               :filter ,(lambda (cmd)
>>                          (print (format "Called in %s" (current-buffer)))
>>                          cmd)))
>> (with-temp-buffer
>>   (with-output-to-string
>>     (describe-buffer-bindings (get-buffer "*scratch*") [3 12] t)))
>>
>> This outputs:
>>
>> \"Called in  *temp*\"
>
> I tried this recipe, but I'm not getting the :filter function called at
> all.  Do you have a recipe, starting from "emacs -Q", that reproduces
> this bug?

I have pushed a fix to master (f08e6538dc) ensuring that we correctly
run the :filter function.

The bug can now be reproduced (on master and 26.3) using:

(progn
  (require 'ert)
  (let (was-in-buffer)
    (unwind-protect
        (progn
          (define-key global-map (kbd "C-c C-l r")
            `(menu-item "2" identity
                        :filter ,(lambda (cmd)
                                   (setq was-in-buffer
                                         (current-buffer))
                                   cmd)))
          (with-temp-buffer
            (let ((buf (get-buffer "*scratch*")))
              (describe-buffer-bindings buf nil t)
              (should was-in-buffer)
              (should (eq was-in-buffer buf)))))
      (define-key global-map (kbd "C-c C-l r") nil)
      (define-key global-map (kbd "C-c C-l") nil))))

As Andreas Schwab has pointed out, the problem is that describe-map (and
describe_map before it) has no idea about the buffer argument to
describe-bindings.  So if the above is to be made to work, we have to
make sure that it does.

I see two ways of doing that:

a) Add a new argument to describe-map with the original buffer.  Ensure
   that the :filter function is run there.

b) Change describe-map to return its output as a string.

The problem with a) is that describe-map already has a pretty long
argument list.  It also seems to me that the resulting code would not be
very easy to follow.

My preference would therefore be looking into option b) first.
If we can make it work, we could then just change the buffer in
describe-buffer-bindings before calling describe-map.

(It is true that the old C function "describe_map" has always inserted
its output in the current buffer.  It also true that it has always been
an internal function, and never exposed to Lisp directly.  So we should
be free to change its behavior, and we should probably better do it
before Emacs 28.1.)

Thoughts?

---

Also, a tangent:

IMHO, using `menu-item' in this way works as it stands, but it is a bit
strange conceptually.  We are binding a key to a `menu-item' that is
never intended to be used for a menu.  Yet IIUC this is the recommended
way of using :filter.

It would perhaps be better to have a specific format that allows for
these extended features, without needing to provide a `menu-item'.

(See `(elisp) Format of Keymaps' for more.)