smartparens icon indicating copy to clipboard operation
smartparens copied to clipboard

sp-backward-delete-word causes an error `args-out-of-range` when the kill ring is empty

Open cordelia04m opened this issue 3 years ago • 7 comments

Expected behavior

There should be no error when calling sp-backward-delete-word when the kill ring is empty.

Actual behavior

An error happened and it took me into recursive edit.

Steps to reproduce the problem

  1. Clean the kill ring using this function
(defun clean-kill-ring ()
  "Use this to clean password after pasting it from a clipboard."
  (interactive)
  (setq kill-ring nil)
  (garbage-collect))
  1. Call sp-backward-delete-word.
  2. Call sp-backward-delete-word again. The error will happen this second time we call it.

Backtraces if necessary (M-x toggle-debug-on-error)

Debugger entered--Lisp error: (args-out-of-range 0 0)
  get-text-property(0 yank-handler nil)
  kill-append(#("aaa " 0 3 (fontified t)) t)
  kill-region(6 3)
  sp-backward-kill-symbol(1 t)
  sp-backward-delete-symbol(1 t)
  sp-backward-delete-word(1)
  funcall-interactively(sp-backward-delete-word 1)
  call-interactively(sp-backward-delete-word nil nil)
  command-execute(sp-backward-delete-word)

Environment & version information

  • smartparens version: 20211101.1101
  • Active major-mode: lisp-interaction-mode
  • Smartparens strict mode: nil
  • Emacs version (M-x emacs-version): GNU Emacs 27.2 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.27, cairo version 1.17.4) of 2021-03-27
  • Starterkit/Distribution: Vanilla
  • OS: gnu/linux

cordelia04m avatar Nov 28 '21 14:11 cordelia04m

I believe I know why this happens. I've had a related issue with deletion unexpectedly adding to the kill-ring. The sp-delete-* commands try to avoid touching the kill-ring by let-binding it, then delegating to kill-region.

On repeated deletes/kills the kill-region call chain eventually ends up in a call to add-to-history: (add-to-history 'kill-ring string kill-ring-max t). add-to-history looks up kill-ring's value with symbol-value, which bypasses lexical binding. So it ends up unexpectedly mangling the actual global kill-ring despite the let binding.

This is the same root cause, I believe, for #1040 and #1097. The best way to solve it is not clear. I have found saving (car kill-ring) and then actually setting it afterwards to work, but it seems drastic.

(let ((kill-ring-saved-car (car kill-ring)))
    (sp-kill-symbol arg word)
    (if (null kill-ring-saved-car)
        (setq kill-ring nil)
      (setcar kill-ring kill-ring-saved-car))

(Note I am writing this based on Emacs 27; I have not looked at other versions' kill-region implementations.)

woolsweater avatar Feb 19 '22 19:02 woolsweater

I'm still on E26 and I can't find the code you are refering to. Can you tell me what is the function/file where the add-to-history call is found? I wonder why the implementation seemingly changed.

Fuco1 avatar Feb 21 '22 09:02 Fuco1

Sure thing! Here's the call chain that I see:

kill-region checks whether the last command was also a kill, and if so calls kill-append: https://github.com/emacs-mirror/emacs/blob/emacs-27/lisp/simple.el#L4764-L4766

kill-append constructs the new kill ring value and calls kill-new: https://github.com/emacs-mirror/emacs/blob/emacs-27/lisp/simple.el#L4648-L4650

kill-new, if its replace argument is non-nil, calls add-to-history: https://github.com/emacs-mirror/emacs/blob/emacs-27/lisp/simple.el#L4624

Where symbol-value gets the dynamic kill-ring value: https://github.com/emacs-mirror/emacs/blob/emacs-27/lisp/subr.el#L1975

I will try to compare with the emacs-26 branch later this evening.

woolsweater avatar Feb 21 '22 16:02 woolsweater

I know I'm the devil for screenshotting code, but yea, here's the difference

image

Fuco1 avatar Feb 21 '22 18:02 Fuco1

I'm not sure what the best way to address this is. If you have a suggested or preferred approach, @Fuco1, I would be happy to try to come up with a patch.

woolsweater avatar Mar 06 '22 01:03 woolsweater

-snips something that was my fault-

I'm in the middle of updating my emacs configuration, I followed the steps in OP and could not replicate in emacs 28.1. But i also remember encountering issues with various commands when the kill-ring was empty back then.

In the end I made my own version of sp-backward-delete-word in one oy my libraries which just uses vanilla emacs commands:

`(defun dd-delete-word (&optional arg) "Delete characters forward until encountering the end of a word. With argument ARG, do this that many times." (interactive "p") (delete-region (point) (progn (forward-word (or arg 1)) (point))))

(defun dd-backward-delete-word (&optional arg) "Delete characters backward until encountering the beginning of a word. With argument ARG, do this that many times." (interactive "p") (dd-delete-word (- (or arg 1))))`

AkashaP avatar Apr 05 '22 12:04 AkashaP

Thanks, but your commands don't skip delimiters as smartparens does. With point at the beginning of '((foo bar) baz), smartparens forward delete produces

'(( bar) baz)

as opposed to dd-delete-word's

 bar) baz)

woolsweater avatar Apr 05 '22 15:04 woolsweater

I believe this is fixed by #1169 as I can no longer reproduce this and related kill/delete issues. I've added a test case for this issue so hopefully there won't be regressions.

Fuco1 avatar Oct 21 '23 12:10 Fuco1

The fix is working great, thank you, @Fuco1 !

woolsweater avatar Oct 25 '23 01:10 woolsweater