sp-backward-kill-word and ESS, part 2

[jabranham]

I assume this is somehow related to #813

Expected behavior

c("test", "one")

with point on the " after one" and run sp-backward-kill-word should kill one

Actual behavior

it kills test

Environment & version information

• smartparens version: latest git commit f41960f
• Emacs version (M-x emacs-version): GNU Emacs 25.3.1
• OS: gnu/linux

[jabranham]

"one two"

with point on the second "

sp-backward-kill-word in R-mode kills one instead of two (it behaves correctly in emacs-lisp-mode, for example). sp-backward-kill-word calls sp-backward-kill-symbol which in turn calls sp-get-symbol. The value of (sp-get-symbol t) in R-mode is (:beg 2 :end 5 :op "" :cl "" :prefix "" :suffix "") but in emacs-lisp-mode is (:beg 6 :end 9 :op "" :cl "" :prefix "" :suffix ""). This is because calling sp-skip-backward-to-symbol puts point over the n in one in R-mode but doesn't move point in emacs-lisp-mode. sp-skip-backward-to-symbol is defined by a macro sp--skip-to-symbol-1 which has the comment ;; TODO: get rid of the macro anyway, it's stupid! and is currently throwing the remaining two byte compiler warnings in #836.

[Fuco1]

I don't know what it is about R mode but it does some weird stuff to syntax tables :D Anyway the macro has been there pretty much from the start and it is quite useless but it is complicated and I don't understand how it works anymore.

The whole parsing thing has some parts in it which are all the way from 2012 when my elisp-fu was rather weak and so things that "just worked" were good enough. I have tried several times to do a rewrite but never managed because it's such a deep web to unspin I just don't have the power to do that.

But I think the problem at hand will have the cause somewhere else, namely in the syntax definitions... which we can either hack around or can't, I'll have a look.

At one point I was also hoping to have an architecture with pluggable major-mode-determined components (like the skippers, parsers for balanced pairs etc) but that would also need some more thought and time to be realized.

[jabranham]

This issue was introduced by commit e389877

EDIT: It's due to this snippet of code in sp--get-prefix:

(-if-let (mmode-prefix (cdr (assoc major-mode sp-sexp-prefix)))
                    (cond
                     ((and (eq (car mmode-prefix) 'regexp)
                           (sp--looking-back (cadr mmode-prefix)))
                      (match-string-no-properties 0))
                     ((eq (car mmode-prefix) 'syntax)
                      (skip-syntax-backward (cadr mmode-prefix))
                      (buffer-substring-no-properties (point) p))
                     (t ""))

EDIT2: Apparently smartparens-ess.el sets sp-sexp-prefix to this:

(add-to-list 'sp-sexp-prefix
             (list 'ess-mode 'regexp
                   (rx (zero-or-more (syntax symbol)))))

But I'm not sure that's needed. I've set it to nil and everything seems to work^{TM}

Can we just delete it @Fuco1?

EDIT3: No, we can't. Removing it makes a couple tests fail:

   FAILED  sp-test-ess-raise-sexp
   FAILED  sp-test-ess-slurp-forward

[jabranham]

Note that this bug also produces behavior like:

ggplot(mtcars, aes(mpg, am)) +
  facet_wrap|()

with | representing point, calling sp-backward-kill-word kills mtcars in the row above. Removing the entry for ess-mode from sp-sexp-prefix results in the expected behavior (killing wrap).

[Fuco1]

Hey Alex. Three years later and after a year of using R daily, I finally got to this :D

There were multiple bugs surrounding this code, which over the years grew quite complex to support all the different cases. The prefix configuration is needed in R to make slurping work properly (among some other things) but causes the issue you describe.

Using your analysis I narrowed it down and managed to fix three issues including this one.

[Fuco1]

I added the test cases from OP and the thread for regression testing, they all pass now.