bug#74660: 31.0.50; bind-keys has unexpected behavior when evaluated with eval-defun

bug#74660: 31.0.50; bind-keys has unexpected behavior when evaluated with eval-defun

[Dale]

Start any Emacs that includes bind-key.el with "emacs -Q".  Then put the
following into an `emacs-lisp-mode' buffer (or anywhere C-M-x is bound
to `eval-defun'):

    (bind-keys :repeat-map foo-map
               ("n" . next-line)
               :exit
               ("q" . ignore))

Put the point anywhere in this form and press C-M-x (`eval-defun') twice.
Now look at the value of `foo-map'.

Expected value: (keymap (113 . ignore) (110 . next-line))
Both bindings are set.

Observed value: (keymap (113 . ignore))
Only the last binding is set.

I believe the problem is that `eval-defun' and friends treat `defvar`
specially via `elisp--eval-defun-1': normally `defvar' won't change the
value of SYMBOL if it is already bound, but `eval-defun' (and
`eval-last-sexp' and probably others) call `elisp--eval-defun-1' to
rewrite the `defvar' into a form that always sets ("re-initializes")
SYMBOL.

Additionally, `elisp--eval-defun-1' will recurse into `progn' forms to
apply this rewriting of `defvar'.

If you macroexpand the `bind-keys' form from my test case, you'll see
that `bind-keys' produces two `defvar' forms for the provided keymap:

    (progn
      (defvar foo-map (make-sparse-keymap))
      (put (function next-line) 'repeat-map 'foo-map)
      (bind-key "n" (function next-line) foo-map nil)
      (defvar foo-map (make-sparse-keymap))
      (bind-key "q" (function ignore) foo-map nil))

Normally this second `defvar' has no effect, but when you `eval-defun'
this `bind-keys' form, the special behavior from `elisp--eval-defun-1'
kicks in for `defvar', and so the second `defvar' clobbers the keymap
value that was set up by the first `defvar'.  (Actually, the first
`defvar' will also clobber the keymap, which may not be what the user
expects.)

You do have to `eval-defun' twice because `elisp--eval-defun-1' only
applies its special behavior when the variable being defined is unbound.
The first time through, `foo-map' is undefined, so `defvar' is left
alone, and the resulting value of `foo-map' is as expected.  The second
time through, `foo-map' is now bound, so the special behavior kicks in,
and both `defvar' forms set `foo-map' to an empty sparse keymap.

Normally I wouldn't open a bug for this, because this special behavior
for `eval-defun' is documented behavior.  However, I think the case of
`bind-keys' is problematic for two reasons:

1. It is not immediately obvious that the `bind-keys' macro is using
   `defvar'.

2. `bind-keys' is something users put in their init file, and so they're
   likely going to be using C-M-x or C-x C-e a lot while they're
   tweaking and testing their init file (exactly what happened to me).

Further notes:

I do find the special treatment of `defvar' (and `defcustom') with C-M-x
and C-x C-e useful, so I'm not proposing removing that. :)

Arguably, `elisp--eval-defun-1' should notice when there are more than
one `defvar' forms for the same variable, and only the first should
exhibit the special "always initialize SYMBOL" behavior.  I don't want
to make that argument, though, because I feel the special-casing of
`defvar' is surprising enough as-is.  I don't want to make that more
complex.

Maybe `bind-keys' shouldn't emit more than one `defvar' form for a
keymap?  Or maybe it shouldn't emit any `defvar' forms if it finds the
keymap is already bound?

Thank you for maintaining Emacs :)


In GNU Emacs 31.0.50 (build 1, aarch64-apple-darwin23.6.0, NS
appkit-2487.70 Version 14.7 (Build 23H124)) of 2024-10-12 built on
daleRepository revision: 05e418e0688f24b5518e38e54bde96a639f7c70d
Repository branch: master
Windowing system distributor 'Apple', version 10.3.2487
System Description:  macOS 14.7.1

Configured using:
 'configure --without-x --with-xwidgets --with-json --with-tree-sitter --without-imagemagick --with-xpm --with-jpeg --with-tiff --with-gif --with-png --with-rsvg --with-webp --with-sqlite3 --with-lcms2 --with-cairo --with-xml2 --with-gnutls --with-zlib --with-modules --with-threads --with-native-compilation --with-ns --enable-ns-self-contained 'CFLAGS= -D_DARWIN_UNLIMITED_SELECT=1 -DFD_SETSIZE=10240''