emacs-evil / emacs-evil/evil

`evil-delay` doesn't admit user use `evil-define-key` in `dolist`.

Open
#526 2 comments 0 reactions 0 assignees View on GitHub
bug major
Dominant language
Emacs Lisp
Stars
3.9k
Forks
307
PR merge metrics
No merged PRs in 30d

Description

Originally reported by: **Ken Okada (Bitbucket: [kenoss](https://bitbucket.org/kenoss), GitHub: [kenoss](https://github.com/kenoss))**

----------------------------------------

IMHO, using `eval` for forms in macros is definitely bad idea. It is used in `evil-define-keys`.

For example, see following configration and what happens.

``` emacs-lisp
(with-eval-after-load 'evil
(defvar magit-commit-mode-map-orig magit-commit-mode-map)
(defvar magit-status-mode-map-orig magit-status-mode-map)
(defvar magit-log-mode-map-orig magit-log-mode-map)
(defvar magit-mode-map-orig magit-mode-map)
(defvar git-rebase-mode-map-orig git-rebase-mode-map)
(dolist (mode '(git-commit-mode
magit-log-edit-mode))
(evil-set-initial-state mode 'insert))
(dolist (mode '(magit-mode
magit-status-mode
magit-commit-mode
magit-log-mode
magit-wassup-mode
git-rebase-mode))
(evil-set-initial-state mode 'normal)
(unless (memq mode '(magit-wassup-mode))
(let ((map (symbol-value (intern (concat (symbol-name mode) "-map"))))
(map-orig (symbol-value (intern (concat (symbol-name mode) "-map-orig")))))
(evil-make-overriding-map map 'normal)
(evil-define-key 'normal map
";" (lookup-key evil-motion-state-map ";")
"h" 'magit-goto-previous-section
"t" 'magit-goto-next-section
"H" 'magit-goto-previous-sibling-section
"T" 'magit-goto-next-sibling-section
(kbd "z") nil
(kbd "z z") 'magit-toggle-section
))))
)
```

This produce not expected result because `map` and `map-orig` remain in the original form.
This is an evil use of eval.

I guess Vegard had chosen it because "map variable" doesn't exist in the evaluation of this form,
without appropriate `eval-after-load`. I'm not positive that though. Because:

1. This contradicts minimal suprising principal.
2. I beleave that users should write appropriate `require` or `eval-after-load` themselves. Emacs Lisp compiler warns when undefined variables are used.
If user wants to use autoload, he should write `(eval-when-compile (require ...))` to make quiet compiler, and this solves the problem above.

Hence, I'd like you to fix this macro or `evil-define-key`.

Note:

1. You shouldn't generate intrinstic variables in macros. Macros should do only convert code.
2. I think we shouldn't use non-interned symbols for hooks. This makes tracing problems difficult. If you want to use volatile hook, use random numbers like gensym.
See also: https://github.com/kenoss/erfi/blob/85f38c0a08dd607ad4f0cb87e410ca594240447c/lisp/erfi-emacs.el#L32
(Do not fully trust this code. This is not effective for lots of call. But it was enough for the being.)

----------------------------------------
- Bitbucket: https://bitbucket.org/lyro/evil/issue/526

Contributor guide

Open the contributing guide

Research direction

The relevant entry points are evil-delay, evil-define-key, and evil-define-keys; read their macro expansions first, especially how forms inside dolist are evaluated. Reproduce the supplied Magit configuration and confirm that the key bindings use each loop's map values rather than the original forms.

Written by the indexing model from the issue text.

Assessment

Tech stack
emacs-lisp
Domain
tooling
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.