expez / evil-smartparens

Evil integration for Smartparens
GNU General Public License v3.0
134 stars 17 forks source link

Actions like dw and di" acting weirdly? #42

Closed dieggsy closed 6 years ago

dieggsy commented 6 years ago

With point at beginning of word:

Before Action After w/o evil-smartparens After w/ evil-smartparens
"hello" dw "" "o"
"hello" di" "" "o"
expez commented 6 years ago

Yeah, this is something I noticed too after updating emacs a few days ago.

I haven't made any changes to evil-smartparens in a while so this is likely caused by changes in either evil or smartparens.

I don't have much time for OSS lately, so if you want to investigate that would be quite welcome!

alexmurray commented 6 years ago

Am seeing this too - any pointers where to start looking?

expez commented 6 years ago

any pointers where to start looking?

evil-smartparens relies heavily on sp-region-ok-p found in smartparens. That function was changed earlier this year, and I'm guessing that's where the problem is.

alexmurray commented 6 years ago

If I manually test sp-region-ok-p it works fine - ie.

Insert "foo" in a buffer, and put point on f and mark on the last o - then eval M-: (sp-region-ok-p (mark) (point) - and this returns t.

However if you put point on f and do dw with evil-smartparens we are left with the last o and this is because in this context sp-region-ok-p has returned nil even though beg and end have the same values as (mark) and (point) above - so something about the evil-operator stuff or perhaps evil-smartparens makes sp-region-ok-p fail for the exact same values (I know the values are the same as I added a call to print them out in sp-region-ok-p - note the intermediate result variable and it being printed at the end):

(cl-defun sp-region-ok-p (start end)
  "Test if region between START and END is balanced.

A balanced region is one where all opening delimiters are matched
by closing delimiters.

This function does *not* check that the delimiters are correctly
ordered, that is [(]) is correct even though it is not logically
properly balanced."
  (save-excursion
    (save-restriction
      (let ((result nil))
        (narrow-to-region start end)
        (when (eq (sp-point-in-string start) (sp-point-in-string end))
          (let ((regex (sp--get-allowed-regexp (-difference sp-pair-list (sp--get-allowed-pair-list)))))
            (goto-char (point-min))
            (while (or (prog1 (sp-forward-sexp)
                         (sp-skip-forward-to-symbol))
                       ;; skip impossible delimiters
                       (when (looking-at-p regex)
                         (goto-char (match-end 0)))))
            (setq result (looking-at-p "[[:blank:]\n]*\\'"))))
        (message "(sp-region-ok-p %d %d) -> %s" start end result)
        result))))
alexmurray commented 6 years ago

If I remove the (narrow-to-region start end) call in (sp-region-ok-p) this seems to fix it from my limited testing - I think that by narrowing then (sp-point-in-string) gets confused since maybe we have lost context for syntax-ppss to return the correct syntax?

expez commented 6 years ago

Thanks for investigating, @alexmurray!

@Fuco1 does that sound right to you?

Fuco1 commented 6 years ago

@alexmurray I think that in evil the end of the region is shifted one point back to simulate the way vim behaves. Does this always behave in such a way that only the last character is left over? If so we could probably add a 1+ somewhere... but that sounds very brittle.

This can also depend on the major mode being used, as you said, we could possibly lose the context for sytnax-ppss (there is also some caching in SP to avoid calling it millions of times every time). Some major modes mark strings with syntax property overrides (python, rust, ruby I think come to mind), but I'm not sure if they actually take into account the narrowing (or rather, font lock which does that).

alexmurray commented 6 years ago

Sorry - we can't just remove (narrow-to-region start end) this is breaks later when (goto-char (point-min)) is called - so instead if we both remove the narrow-to-region and change the goto-char to (goto-char start) then it all appears to work - @Fuco1 can you comment on whether this is a reasonable approach - is the (narrow-to-region) strictly necessary?

alexmurray commented 6 years ago

Actually just moving the (narrow-to-region start end) to be the first statement inside (when (eq (sp-point-in-string start) (sp-point-in-string end)) seems to resolve this best.

alexmurray commented 6 years ago

With https://github.com/Fuco1/smartparens/pull/805 (and https://github.com/expez/evil-smartparens/pull/44) this is now working as expected for me.

expez commented 6 years ago

Fixed. New release cut.

Thanks again @alexmurray!

dieggsy commented 6 years ago

Thanks for looking into this @alexmurray. It does seem to be working now.

alexmurray commented 6 years ago

No worries