emacs-evil / evil

The extensible vi layer for Emacs.
GNU General Public License v3.0
3.33k stars 281 forks source link

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

Open TheBB opened 9 years ago

TheBB commented 9 years ago

Originally reported by: Ken Okada (Bitbucket: kenoss, GitHub: 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.

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

TheBB commented 9 years ago

Original comment by Matthew Malcomson (Bitbucket: mmalcomson, GitHub: Unknown):


Issue #533 was marked as a duplicate of this issue.

TheBB commented 9 years ago

Original comment by Ken Okada (Bitbucket: kenoss, GitHub: kenoss):


FYI, I don't use the above magit configuration. So my priority is not high.