Rework #984

[justbur]

@wasamasa Here's a different version of #984. Changes are

1. I removed the customization variable. I think this is sufficiently correct functionality to make it the default even if it does modify someone's config slightly.

2. I now teach evil-initial-state to look at aliases for modes. This makes a little more sense to me and simplifies the code.

3. I detect the circular references and print a message as requested.
   Teach evil-initial-state to look at aliases for a mode when they exist and to
   handle nil for modes

Note that I still need to add tests for this.

[wasamasa]

Interestingly enough, this doesn't appear to handle circular references. Here's some sample code:

(define-derived-mode foo-mode prog-mode "Foo")
(define-derived-mode bar-mode foo-mode "Bar")
(put 'foo-mode 'derived-mode-parent 'bar-mode)
(foo-mode)

If I evaluate this in an Emacs using this branch's code, it displays the message and hangs. Make sure to run it in a separate instance.

[wasamasa]

Interestingly enough, this doesn't appear to handle circular references. Here's some sample code:

(define-derived-mode foo-mode prog-mode "Foo")
(define-derived-mode bar-mode foo-mode "Bar")
(put 'foo-mode 'derived-mode-parent 'bar-mode)
(foo-mode)

If I evaluate this in an Emacs using this branch's code, it displays the message and hangs. Make sure to run it in a separate instance.

[justbur]

Circular references cause bigger problems in Emacs it turns out. derived-mode-p easily gets caught in an infinite loop for example.

[justbur]

Circular references cause bigger problems in Emacs it turns out. derived-mode-p easily gets caught in an infinite loop for example.

[wasamasa]

You see, this is why I originally suggested to throw an error to abort execution completely.

[wasamasa]

You see, this is why I originally suggested to throw an error to abort execution completely.

[justbur]

I still don't like throwing an error. Take your example and enable global-paren-face-mode. If evil throws an error in the middle of initialization it leaves evil in a weird state (not fully initialized). Emacs still hangs due to a line like this in paren-face.

So the result is evil is not initialized (when it could be) and Emacs hangs anyway. If you have debug-on-error set then you might get a better indication of what's going on I guess.

[justbur]

I still don't like throwing an error. Take your example and enable global-paren-face-mode. If evil throws an error in the middle of initialization it leaves evil in a weird state (not fully initialized). Emacs still hangs due to a line like this in paren-face.

So the result is evil is not initialized (when it could be) and Emacs hangs anyway. If you have debug-on-error set then you might get a better indication of what's going on I guess.

[wasamasa]

I'm sorry, but I don't quite understand how this example supports your argument. The shown code calls derived-mode-p which will hang if there's a cyclical reference anywhere. How is this related to Evil? More importantly, how is a setting that affects the initial state of buffers relevant to load-up which only touches elisp files? It's not like your standard elisp file does nothing else than triggering as many different major modes as possible until it hits one triggering that corner case. I expect the error to happen as you're editing, open a file with a less well written major mode and run into it. Erroring out there makes more sense to me than making Emacs unusable.

[wasamasa]

I'm sorry, but I don't quite understand how this example supports your argument. The shown code calls derived-mode-p which will hang if there's a cyclical reference anywhere. How is this related to Evil? More importantly, how is a setting that affects the initial state of buffers relevant to load-up which only touches elisp files? It's not like your standard elisp file does nothing else than triggering as many different major modes as possible until it hits one triggering that corner case. I expect the error to happen as you're editing, open a file with a less well written major mode and run into it. Erroring out there makes more sense to me than making Emacs unusable.

[justbur]

@wasamasa I'll defer to you, but let me explain my reasoning. I have at least two minor modes (not including evil) which hang with circular references, so here is my experience

If we throw an error here:

1. Evil's initialization errors out.
2. Emacs hangs initializing another minor mode
3. If I quit I'm left with a buffer in which evil is uninitialized or maybe partially initialized, but doesn't work.

If we issue a message here:

1. Evil's initialization goes through fine with the default state being chosen for the buffer.
2. Emacs hangs initializing another minor mode
3. If I quit I'm left with a buffer in which evil is in a good state.

In either case, the message about the circular reference is there, but in the latter case I have a more functional editor (at least from the experience of an evil user).

[justbur]

@wasamasa I'll defer to you, but let me explain my reasoning. I have at least two minor modes (not including evil) which hang with circular references, so here is my experience

If we throw an error here:

1. Evil's initialization errors out.
2. Emacs hangs initializing another minor mode
3. If I quit I'm left with a buffer in which evil is uninitialized or maybe partially initialized, but doesn't work.

If we issue a message here:

1. Evil's initialization goes through fine with the default state being chosen for the buffer.
2. Emacs hangs initializing another minor mode
3. If I quit I'm left with a buffer in which evil is in a good state.

In either case, the message about the circular reference is there, but in the latter case I have a more functional editor (at least from the experience of an evil user).

[justbur]

@wasamasa I updated this to throw an error for the circular reference thing.

[justbur]

@wasamasa I updated this to throw an error for the circular reference thing.

[wasamasa]

Thanks, this leaves some tests which can be probably adapted from #904.

[wasamasa]

Thanks, this leaves some tests which can be probably adapted from #904.

[justbur]

@wasamasa added some tests

[justbur]

@wasamasa added some tests

[wasamasa]

I've found a case that doesn't work correctly, if I've configured Evil to only use prog-mode and text-mode for normal state and make Emacs state the default, this doesn't give me normal state when opening a json-mode buffer. It seems as if the alias detection doesn't work correctly yet. json-mode derives from javascript-mode which is an alias for js-mode, so I'd expect this PR's code to detect that and check js-mode and its parents until encountering prog-mode to reach normal state.

Here's my config:

(setq evil-default-state 'emacs
      evil-emacs-state-modes nil
      evil-insert-state-modes nil
      evil-motion-state-modes nil
      evil-normal-state-modes '(text-mode prog-mode fundamental-mode
                                          css-mode conf-mode
                                          TeX-mode LaTeX-mode
                                          diff-mode))

[wasamasa]

I've found a case that doesn't work correctly, if I've configured Evil to only use prog-mode and text-mode for normal state and make Emacs state the default, this doesn't give me normal state when opening a json-mode buffer. It seems as if the alias detection doesn't work correctly yet. json-mode derives from javascript-mode which is an alias for js-mode, so I'd expect this PR's code to detect that and check js-mode and its parents until encountering prog-mode to reach normal state.

Here's my config:

(setq evil-default-state 'emacs
      evil-emacs-state-modes nil
      evil-insert-state-modes nil
      evil-motion-state-modes nil
      evil-normal-state-modes '(text-mode prog-mode fundamental-mode
                                          css-mode conf-mode
                                          TeX-mode LaTeX-mode
                                          diff-mode))

[justbur]

Yep, I wasn't checking parents of the aliases, which is what your example shows. I changed the logic to do this now.

[justbur]

Yep, I wasn't checking parents of the aliases, which is what your example shows. I changed the logic to do this now.

[justbur]

@wasamasa seems like the travis segfaults are back.

[justbur]

@wasamasa seems like the travis segfaults are back.

[wasamasa]

Damnit. Code looks fine now, but I'll have to do some more testing before this is good to go.

[wasamasa]

Damnit. Code looks fine now, but I'll have to do some more testing before this is good to go.

[justbur]

I just rebased and the seg fault went away, so a good sign at least...

[justbur]

I just rebased and the seg fault went away, so a good sign at least...

[wasamasa]

That's no proof considering it went undetected for like dozens of test runs.

[wasamasa]

That's no proof considering it went undetected for like dozens of test runs.

[wasamasa]

Alright, I think this can be merged, that is, once you've fixed this (hopefully trivial) merge conflict. I'd fix it myself if this were a branch on here...

edit: https://travis-ci.org/emacs-evil/evil/builds/333000270

[wasamasa]

Alright, I think this can be merged, that is, once you've fixed this (hopefully trivial) merge conflict. I'd fix it myself if this were a branch on here...

edit: https://travis-ci.org/emacs-evil/evil/builds/333000270

[justbur]

@wasamasa done. I squashed two of the commits, too.

[justbur]

@wasamasa done. I squashed two of the commits, too.

[wasamasa]

Thanks!

[wasamasa]

Thanks!

[marktran]

This PR disables evil in org-mode for me. How do get it working with org-mode again?

cc @justbur @wasamasa

[marktran]

This PR disables evil in org-mode for me. How do get it working with org-mode again?

cc @justbur @wasamasa

[wasamasa]

@marktran First of all, the worst it could do is enabling Emacs state (as denoted by <E> in the mode line) which is not the same as disabling Evil. Second, I cannot reproduce that withing a make emacs session. It's therefore most probably something in your init file. Please open a more specific bug report once you have a minimal repro.

[wasamasa]

@marktran First of all, the worst it could do is enabling Emacs state (as denoted by <E> in the mode line) which is not the same as disabling Evil. Second, I cannot reproduce that withing a make emacs session. It's therefore most probably something in your init file. Please open a more specific bug report once you have a minimal repro.

[marktran]

Yes, please excuse my terminology.

I see that this PR makes org-mode derive its initial state from text-mode and my config had the following:

(loop for (mode . state) in '((inferior-emacs-lisp-mode . emacs)
                              (comint-mode              . emacs)
                              (eshell-mode              . emacs)
                              (occur-mode               . emacs)
                              (paradox-menu-mode        . emacs)
                              (special-mode             . emacs)
                              (sql-interactive-mode     . emacs)
                              (text-mode                . emacs))
      do (evil-set-initial-state mode state))

All fixed now. Thanks.

[marktran]

Yes, please excuse my terminology.

I see that this PR makes org-mode derive its initial state from text-mode and my config had the following:

(loop for (mode . state) in '((inferior-emacs-lisp-mode . emacs)
                              (comint-mode              . emacs)
                              (eshell-mode              . emacs)
                              (occur-mode               . emacs)
                              (paradox-menu-mode        . emacs)
                              (special-mode             . emacs)
                              (sql-interactive-mode     . emacs)
                              (text-mode                . emacs))
      do (evil-set-initial-state mode state))

All fixed now. Thanks.