kill-sexp missing point advice in normal/visual mode

[knusprjg]

Issue type

• Bug report

Reproduction steps

• Start Emacs
• in scratch buffer type in new line:
• (+ 1 1)
• Move point to end of the line (.e.g. $)
• Use backward-kill-sexp in normal mode

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]

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

[tomdl89]

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]

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]

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]

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

• eval-last-sexp -> 2
• backward-kill-sexp -> empty line

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

• eval-last-sexp -> 2 (advice is already active here!)
• backward-kill-sexp -> (+ 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)|)|

• backward-kill-sexp -> empty line

(+ 1 (* 2 3|)|)

• backward-kill-sexp -> (+ 1 ), so the same behavior as I would expect in normal mode with the cursor after the last or penultimate paren. As I said, eval-last-sexp already behaves in that way.

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]

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

[tomdl89]

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]

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]

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]

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]

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]

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]

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]

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]

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]

@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]

@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 )

• run eval-last-sexp and Emacs shows 2
• run list-eval-last-sexp and I get 1 in *inferior-lisp* buffer

[wrobell]

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

[tomdl89]

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]

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