Fuco1 / smartparens

Minor mode for Emacs that deals with parens pairs and tries to be smart about it.
GNU General Public License v3.0
1.8k stars 193 forks source link

`kill-ring` should not be `let-bind`ed to `kill-ring` itself on `sp(-backward)?-delete-symbol` #1169

Closed ROCKTAKEY closed 8 months ago

ROCKTAKEY commented 11 months ago

Hi, I fixed the bug that sp-delete-symbol and sp-backward-delete-symbol destruct kill-ring. Here is reproduction process:

(setq kill-ring '("kill-ring-value"))
; => ("kill-ring-value")
abc def ; At the beginning of this line, run `sp-delete-symbol' twice
kill-ring
; => (#("kill-ring-valuedef" 15 18 (fontified t)))

Explanation

Wrapping with (let ((kill-ring kill-ring) (select-enable-clipboard nil)) ...) is no mean, because the kill-* commands destructively edit the list which globally binds kill-ring, even with let-bind.

kill-ring is just a list. When list is passed to other variable, it is NOT copied but passed as reference. So the let-binding kill-ring to kill-ring does nothing, and destructive editing is not reverted. If we want to copy the list, you need to use purecopy.

Therefore, we can fix this bug by let-binding kill-ring to other list, like nil.

woolsweater commented 8 months ago

This is great! This will fix https://github.com/Fuco1/smartparens/issues/1115 and also, I believe https://github.com/Fuco1/smartparens/issues/1040 and https://github.com/Fuco1/smartparens/issues/1097 I've just tested it out on Emacs 29 and it seems to be working well there.

Fuco1 commented 8 months ago

This used to work because those operations did not use destructive edits :/ But yes, nil is definitely safer. I don't know why I didn't go with that in the first place.

Fuco1 commented 8 months ago

Actually, this seems to be even more complicated, because there now is a variable kill-ring-yank-pointer which "points" to the kill ring, but if we replace the kill ring with nil, it will save a pointer to that locally bound list (since it's just a cons). So we now need to also nil-bind this variable to shadow its dynamic binding. I'm making a patch.

ROCKTAKEY commented 8 months ago

Thanks, great work!!!