search

emacs-evil / evil

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

kill-sexp missing point advice in normal/visual mode #1541

Open knusprjg opened 2 years ago

knusprjg commented 2 years ago

Issue type

Reproduction steps

Expected behavior

The whole line should be removed as it does when in insert mode.

Actual behavior

Result: (+ 1 )

Further notes

This looks to me that the underlying kill-sexp needs advising as is done with other functions already: (advice-add 'elisp--preceding-sexp :around 'evil--preceding-sexp '((name . evil))).

knusprjg commented 2 years ago

Added a pull request with my working solution: https://github.com/emacs-evil/evil/pull/1542

tomdl89 commented 2 years ago

I'm conflicted about this. If you have your cursor on the final paren on this line, which sexp do you want to kill?

(+ 1 (* 2 3))

~Your PR would mean it's not possible to kill the (* 2 3) sexp using backward-kill-sexp and I'm not sure that's a good thing.~ The typical approach to solving your problem is doing (setq evil-move-beyond-eol t) so you can get beyond the sexp in normal mode. Alternatively, there are a number of lisp-editing packages designed to work well with evil, such as lispyville and evil-cleverparens, so you can use text-objects.

tomdl89 commented 2 years ago

Just thought about this a little further - I suppose you'd delete the (* 2 3) sexp by placing your cursor on the penultimate paren? That makes sense to me. I'd still suggest the approach I suggested, but for consistency, I don't object to merging your PR.

tomdl89 commented 2 years ago

Sorry for spamming this issue, but I just tried your change, and I feel the behaviour on the penultimate paren is strange. It still deletes the whole line. Is this what you'd expect?

knusprjg commented 2 years ago

I will try to elaborate my intention a little bit more. In the following examples I'll mark the point of the cursor in insert mode with |, and surround the point in normal mode with two sticks. E.g. insert mode cursor after a a| and on a in normal mode: |a|.

So to my initial example: Insert mode: (+ 1 1)|

Pre PR behavior in normal mode: (+ 1 1|)|

To me that behavior felt inconsistent. In particular when I would like to write a function that replaces the last sexp with it's evaluation value (actually that is my main application of backward-kill-sexp ;) ). In that case I would have to write an exception for evil normal mode.

For your example this would be my expected results: (+ 1 (* 2 3)|)|

(+ 1 (* 2 3|)|)

I just tried your change, and I feel the behaviour on the penultimate paren is strange. It still deletes the whole line. Is this what you'd expect?

Well, no, but I tried it again here and it seems to be working fine..? To be honest I have not tried it with vanilla emacs but with two very different configs at least. So I would be surprised if the change would not work.

tomdl89 commented 2 years ago

OK I think I need to take a closer look at this. Thanks for your explanations.

tomdl89 commented 2 years ago

OK I've taken another look and can confirm that in normal mode in vanilla emacs with your PR, backward-kill-sexp deletes the entire line when on the penultimate paren, and deletes the (* 2 3) form when on the 3. This is not consistent with how eval-last-sexp behaves, which evals (* 2 3) when on the penultimate paren, and evals 3 when on 3. I agree that it makes the behaviour on the last paren consistent with eval-last-sexp but I don't think it's worth making 2+ things inconsistent to make one thing consistent. If you have time to look into this strangeness, please do, otherwise I'll keep this issue open until I have time to investigate properly.

knusprjg commented 2 years ago

I have now followed the recommendation from CONTRIBUTING.md: Go to evil workspace, make emacs. There I see exactly the behavior I was expecting. I'm using emacs-version 27.1. Does that mean that backward-kill-sexp and eval-last-sexp already behave in the same way in normal mode in your vanilla setup?

Frankly I have not much ideas where to go from here. If you have any ideas how I can help let me know.

tomdl89 commented 2 years ago

Ah I've just tried this in emacs 25 and it behaves like you describe. (I do most of my evil dev in emacs 28 but keep a vm with emacs 25 around for this sort of thing). So I guess backward-kill-sexp has changed between versions of emacs. Looking at the preceding-sexp advices and fn above your code change, it looks like we've had to handle inter-version differences before in this area. If you're interested, you can add to this version-conditional code, otherwise I'll take a look when I get a chance (which could be a while!) šŸ™

knusprjg commented 2 years ago

I think I figured it out... I looked at the emacs 28 code but could not find any relevant differences other than added error reporting. Therefore I installed 28, too, found the same issue and played around with the emacs debugger for the first time. Now I figured out that the error implementation is probably the actual problem here:

(defun kill-sexp (&optional arg interactive)
  "(...)"
  (interactive "p\nd")
  (if interactive
      (condition-case _
          (kill-sexp arg nil)                         <--- kill-sexp is called *recursively* a second time
        (scan-error (user-error (if (> arg 0)
                                    "No next sexp"
                                  "No previous sexp"))))
    (let ((opoint (point)))
      (forward-sexp (or arg 1))
      (kill-region opoint (point)))))

The problem is that to check if there is an error in interactive mode, kill-sexp calls itself a second time with interactive = nil. Hence the forward-char advice is applied a second time when entering the non-interactive kill-sexp where the actual thing happens.

I'm far from being an expert here but my feeling is that in fact the emacs code is a bit wonky at this point. I would say that the error handling should be wrapped around forward-sexp, because this evidently breaks advicing in interactive modes.

A workaround for now would be, to make the advice for kill-sexp conditionial on the value of interactive. Though I'm strongly considering to open an issue for emacs itself. I guess it's not intended to break advicing at such core functions for no obvious benefit.

Edit: And also I'm not sure if the topic of this discussion should not actually be that forward-sexp needs advicing, as this is the underlying function. That might even replace the advice on the currently used advice on elisp--preceding-sexp, because I could see that it also refers to forward-sexp.

tomdl89 commented 2 years ago

Great detective work @knusprjg! Yes the question of how advice should be handled for a function that calls itself is interesting... May indeed be worth raising with emacs devs. But as you said, there may be a quick way for us to handle this ourselves. After all, evil has to support older versions of emacs anyway.

knusprjg commented 2 years ago

Well, I've spent some more minutes thinking about this issue and created a bug report for emacs.

As for evil: I think I would fall back to add the advice for the moment only for backward-kill-sexp (no issues here) and wait for the outcome of the bug report. The workarounds I came up with so far feel to nasty to implement if this is only affecting a beta version of emacs.

knusprjg commented 2 years ago

Sorry for spamming. This issue is much wider than I initially assumed. The bug report can already be considered as dead, I guess. We need to find a way here for evil to implement this - or not. Here are my thoughts:

As I guessed from the code, this was introduced having only eval-last-sexp in mind: https://github.com/emacs-evil/evil/issues/373.

As soon as one starts using those other commands (forward-sexp, backward-sexp, backward-kill-sexp, backward-kill-sexp) things will get weird in evil. Examples:

| num | type         | Initial          | forward-sexp     | backward-sexp   | eval-last-sexp |
|-----+--------------+------------------+------------------+-----------------+----------------|
|   1 | emacs        | /(+ 1 (* 2 3))   | (+ 1 (* 2 3))/   | -> prev sexp    |                |
|   2 | evil (byeol) | /(/+ 1 (* 2 3))  | (+ 1 (* 2 3))/ / | -> prev sexp    |                |
|   3 | evil         | /(/+ 1 (* 2 3))  | (+ 1 (* 2 3)/)/  | -> prev sexp    |                |
|-----+--------------+------------------+------------------+-----------------+----------------|
|   4 | emacs        | (+ 1 (* 2 3))/   | -> next sexp     | /(+ 1 (* 2 3))  |              7 |
|   5 | evil (byeol) | (+ 1 (* 2 3))/ / | -> next sexp     | /(/+ 1 (* 2 3)) |              7 |
|   6 | evil         | (+ 1 (* 2 3)/)/  | (+ 1 (* 2 3)/)/  | (+ 1 /(/* 2 3)) |          7 (!) |

Now there are a lot of situations where this feels pretty counter intuitive and at least for the non-beyond-eol mode, forward/backward sexp is basically broken, because (6) is a dead end.

Here are the possible ways out of this ticket :)

1) We try to (somehow) rectify the situation for those motion-sexp commands - with advising or not. To make them feel more vim-ish, i.e. the pointer will always stop on the opening or closing paren. In that case it might be worse to consider mode specific functions like evil-forward-sexp, which correct the behaviour in normal/visual mode. 2) We actually remove the advising for eval-last-sexp, because this will make the behaviour at least consistent. But comes with the cost, that without beyond-eol, C-x C-e will be not helpful in normal state ((6) will output 6). 3) Screw this! We keep it as is meaning that eval-last-sexp works intuitive and the rest of the commands are not supported by evil and that's it. After all, the cost is only that using those motion-sexp commands will work just fine except for when: mixing with some evil commands in particular situations, or when using eval-last-sexp.

I personally would go with either 1) and mode specific functions or 3). And I think 3) is a very viable option as well because those emacs specific functions are probably not that much of interest for the average evil user. The only thing that makes me think having those mode specific functions available is because of the fact that all that stuff that I have figured out now is too much if one wants to simply create a function that jumps a few sexps (dependent on evil state, beyond-eol, beginning/end of sexp, ...).

tomdl89 commented 2 years ago

Thanks for the write-up @knusprjg! I'd like to think that most people who work with sexps often would be using lispyville or evil-cleverparens, so I'm not too inclined to put much effort into 1), when 3) seems like it suits people using these sorts of packages. Having said that, I think the current situation is a little inconsistent, so any effort you'd want to put into improving the advising would be welcome. I think specific functions like evil-forward-sexp sound too much like reinventing the cleverparens/lispyville wheel for me. Other than my own interests and clear-cut bugs, I prefer to only discuss/review around here, so feel free to make a PR if you're interested.

wrobell commented 1 year ago

I am vim/neovim user for years, trying to make Emacs work for me. evil is the important gateway.

I found this ticket when dealing with Emacs 29.1 Lisp modes. I am trying to keep my Emacs config and amount of packages installed to minimum for many reasons. Also lispyville seems to be out-of-date and not running anymore, evil-cleverparens seems to have similar issues like evil itself.

Unfortunately, for lisp-eval-last-sexp and eval-last-sexp I get inconsistent behaviour with evil. To be honest, I prefer @knusprjg suggestions to be implemented, but at least it would be nice to get some consistency here out of the box.

I suspect, similar problems might be reported with tree-sitter being integrated into Emacs - structural editing might become more popular with other languages.

tomdl89 commented 1 year ago

@wrobell when you say "evil-cleverparensĀ seems to have similar issues likeĀ evilĀ itself" can you be more specific? evil-cleverparens is now part of the emacs-evil org, and I'm actively interested in fixing such issues there.

wrobell commented 1 year ago

@tomdl89 This is what I tried.

I have installed evil-cleverparens, and run evil-cleverparens-mode using M-x. Emacs says the mode is enabled in current buffer.

In the buffer I have

(+ 1 1)

Then in normal mode, when cursor is over )

wrobell commented 1 year ago

So is that a bug, or I am doing something wrong?

tomdl89 commented 1 year ago

Sorry for the delay @wrobell. Neither really. It's not a bug, but you're not doing anything wrong. I don't think this is relevant to evil-cleverparens. Evil deals with this for emacs-lisp, but not for inf-lisp mode. I've made a change in master to address this. Let me know if that helps.

wrobell commented 1 year ago

It works. Have not realized it is so simple. Thanks.