bug#49518: 28.0.50; electric-pair-mode skip-self fails for single-quotes in python-mode

bug#49518: 28.0.50; electric-pair-mode skip-self fails for single-quotes in python-mode

[Jim Porter]

To reproduce this issue:

  1.  emacs -Q --eval '(electric-pair-mode)' foo.py
  2a. Type " (double-quote); result should be "|", where | is the point
  2b. Type " (double-quote); result should be ""|
      (Note: This is the correct behavior, as I understand it.)
  3.  Delete everything (optional)
  4a. Type ' (single-quote); result should be '|'
  4b. Type ' (single-quote); result is ''|', but (I think) it should be ''|

Having looked through the source of both `electric-pair-mode' and
`python-mode', I believe I understand the problem.

First, some background: in `electric-pair-syntax-info', it checks
whether the point is inside a string or comment. If so, it uses
`electric-pair-text-syntax-table' (defaulting to
`prog-mode-syntax-table'); if not, it uses `python-mode-syntax-table'.
Normally, this means if you have a snippet like ''|', the point is
*not* inside string; it's after the end of one string and before the
start of the next.

However, Python has multiline strings, which causes an issue here.
When python-mode sees 3 quotation marks in a row, it doesn't see that
as an empty string followed by the start of a separate string; it sees
the start of a multiline string. Thus, when we have our snippet ''|',
the point is considered to be in a string. That means
`electric-pair-syntax-info' uses `electric-pair-text-syntax-table'.
Since the default prog-mode syntax table doesn't consider single-quote
to be a quote character, `electric-pair-syntax-info' ends up returning
nil.

Notably, this *isn't* an issue for double-quote. While we
(temporarily) have the state ""|", double-quote is a quote character
according to `electric-pair-text-syntax-table', and so skip-self
proceeds as expected.

(You might think the fix here would be to do like `emacs-lisp-mode'
and locally set `electric-pair-text-pairs' to include single-quote.
This wouldn't work though, since it would make it hard to type an
apostrophe in a word; `electric-pair-mode' would double it up.)

One possible fix would be for `python-mode' to locally set
`electric-pair-text-syntax-table' to `python-mode-syntax-table'. This
seems to work for me after some basic testing, but I'm not confident
it's the right thing to do here; I don't see any examples of other
modes that do this, and I haven't thought over all the possible
problems with it.

bug#49518: 28.0.50; electric-pair-mode skip-self fails for single-quotes in python-mode

[Stephen Berman]

On Sat, 10 Jul 2021 18:32:34 -0700 Jim Porter <jporterbugs@gmail.com> wrote:

> To reproduce this issue:
>
>   1.  emacs -Q --eval '(electric-pair-mode)' foo.py
>   2a. Type " (double-quote); result should be "|", where | is the point
>   2b. Type " (double-quote); result should be ""|
>       (Note: This is the correct behavior, as I understand it.)
>   3.  Delete everything (optional)
>   4a. Type ' (single-quote); result should be '|'
>   4b. Type ' (single-quote); result is ''|', but (I think) it should be ''|
[...]
> One possible fix would be for `python-mode' to locally set
> `electric-pair-text-syntax-table' to `python-mode-syntax-table'. This
> seems to work for me after some basic testing, but I'm not confident
> it's the right thing to do here; I don't see any examples of other
> modes that do this, and I haven't thought over all the possible
> problems with it.

One thing that's kinda nice about the current behavior of typing
single-quote in python-mode is that if you type it three times in a row
you get '''|''', so it's ready for typing a multiple string.  This
doesn't happen with double-quote, and after setting
electric-pair-text-syntax-table to python-mode-syntax-table it doesn't
happen any more with single-quote.

Steve Berman

bug#49518: 28.0.50; electric-pair-mode skip-self fails for single-quotes in python-mode

[Jim Porter]

On Sun, Jul 11, 2021 at 1:11 AM Stephen Berman <stephen.berman@gmx.net> wrote:
>
> One thing that's kinda nice about the current behavior of typing
> single-quote in python-mode is that if you type it three times in a row
> you get '''|''', so it's ready for typing a multiple string.  This
> doesn't happen with double-quote, and after setting
> electric-pair-text-syntax-table to python-mode-syntax-table it doesn't
> happen any more with single-quote.

That's true, and I've been working on a patch to make that behavior
more robust. The code for that is in
`python-electric-pair-string-delimiter', but I think an improved
implementation of that would depend on how this bug is fixed. I have a
local patch that works for me, but it depends on the other
electric-pair settings I use, so it couldn't merge as-is.

In particular, it would need to take into account the various possible
settings for `electric-pair-skip-self', since with that set to nil,
I'd generally expect `python-electric-pair-string-delimiter' to be a
no-op (we should already have three quote characters after point, so
no need to insert more).

bug#49518: 28.0.50; electric-pair-mode skip-self fails for single-quotes in python-mode

[Daniel Fleischer]

Jim Porter [2021-07-11 Sun 10:34] wrote:

> That's true, and I've been working on a patch to make that behavior
> more robust. The code for that is in
> `python-electric-pair-string-delimiter', but I think an improved
> implementation of that would depend on how this bug is fixed. I have a
> local patch that works for me, but it depends on the other
> electric-pair settings I use, so it couldn't merge as-is.

As I'm also bothered by this, is there some resolution?

BTW, my solution is hooking this code in 'inferior-python-mode':

(setq-local electric-pair-pairs
            (append electric-pair-pairs '((?' . ?'))))


-- 

Daniel Fleischer

bug#49518: 28.0.50; electric-pair-mode skip-self fails for single-quotes in python-mode

[Jim Porter]

(CCing João since he wanted to hear about `electric-pair-mode' issues[1].)

On 9/18/2021 5:55 AM, Daniel Fleischer wrote:
> Jim Porter [2021-07-11 Sun 10:34] wrote:
> 
>> That's true, and I've been working on a patch to make that behavior
>> more robust. The code for that is in
>> `python-electric-pair-string-delimiter', but I think an improved
>> implementation of that would depend on how this bug is fixed. I have a
>> local patch that works for me, but it depends on the other
>> electric-pair settings I use, so it couldn't merge as-is.
> 
> As I'm also bothered by this, is there some resolution?
> 
> BTW, my solution is hooking this code in 'inferior-python-mode':
> 
> (setq-local electric-pair-pairs
>              (append electric-pair-pairs '((?' . ?'))))

This isn't the right fix in general, but maybe it will help you. It 
works for me because I turn off `electric-pair-mode' inside comments and 
strings, but with the default configuration, it will pair single-quotes 
inside double-quoted strings (i.e. you won't be able to type an 
apostrophe). There's probably a more robust solution, but I haven't had 
time to investigate further...

----------------------------------------

   (defun user--python-electric-pair-string-delimiter ()
     (when (and electric-pair-mode
                (memq last-command-event '(?\" ?'))
                (let ((count 0))
                  (while (eq (char-before (- (point) count)) 
last-command-event)
                    (cl-incf count))
                  (= count 3)))
       (save-excursion (insert (make-string 3 last-command-event)))))

   (defun user--electric-pair-python-hook ()
     (setq-local electric-pair-text-syntax-table python-mode-syntax-table))

   (advice-add #'python-electric-pair-string-delimiter
               :override #'user--python-electric-pair-string-delimiter)
   (add-hook 'python-mode-hook #'user--electric-pair-python-hook)

----------------------------------------

[1] https://lists.gnu.org/archive/html/emacs-devel/2021-09/msg01313.html

bug#49518: 28.0.50; electric-pair-mode skip-self fails for single-quotes in python-mode

[João Távora]

Jim Porter <jporterbugs@gmail.com> writes:

> (CCing João since he wanted to hear about `electric-pair-mode' issues[1].)

Thanks you Jim,

I've read your original bug report.  It is very clear in reproduction
and also the analysis seems mostly correct.  I'll see what I can do.

The solution will probably go into python.el in terms of customization
of electric-pair-mode's variables.  The only question is whether it can
be made to serve Stephen's and your requirements.

In the meantime, if you're curious: this was one of the first things I
worked in with autopair.el. See
https://github.com/joaotavora/autopair/issues/6.

João

bug#49518: 28.0.50; electric-pair-mode skip-self fails for single-quotes in python-mode

[João Távora]

João Távora <joaotavora@gmail.com> writes:

> Jim Porter <jporterbugs@gmail.com> writes:
>
>> (CCing João since he wanted to hear about `electric-pair-mode' issues[1].)
>
> Thanks you Jim,
>
> I've read your original bug report.  It is very clear in reproduction
> and also the analysis seems mostly correct.  I'll see what I can do.
>
> The solution will probably go into python.el in terms of customization
> of electric-pair-mode's variables.  The only question is whether it can
> be made to serve Stephen's and your requirements.
>
> In the meantime, if you're curious: this was one of the first things I
> worked in with autopair.el. See
> https://github.com/joaotavora/autopair/issues/6.

So after having a look at this, I came up with a patch.  All tests
pass, and your original request is granted.  Unfortunately, Stephen's
desired triple-pair behaviour is also lost.  However, I think that
behaviour can be recovered via other more elegant, less accidental ways.
python.el has 'python-electric-pair-string-delimiter' for triple-pairing
that doesn't seem to be doing its thing.

Here is the patch.  Give it a shot.

diff --git a/lisp/elec-pair.el b/lisp/elec-pair.el
index d8c377a2ef..b8b8a97651 100644
--- a/lisp/elec-pair.el
+++ b/lisp/elec-pair.el
@@ -198,7 +198,9 @@ electric-pair-syntax-info
 inside a comment or string."
   (let* ((pre-string-or-comment (or (bobp)
                                     (nth 8 (save-excursion
-                                             (syntax-ppss (1- (point)))))))
+                                             (skip-chars-backward
+                                              (make-string 1 command-event))
+                                             (syntax-ppss (point))))))
          (post-string-or-comment (nth 8 (syntax-ppss (point))))
          (string-or-comment (and post-string-or-comment
                                  pre-string-or-comment))


The original idea of electric-pair-syntax-info is that
electric-pair-text-syntax-table is consulted if point is "well within" a
string or comment.  That's why it backtracks a character to establish
pre-string-or-comment.  But for strings started with multiple characters
it failed, as you well noticed.

So my patch makes it more likely that it understands if point is right
after the string start.  Maybe better less brittle solutions can
probably be found within the syntax tables framework.  If not, we can
make a variable.  I'll think about it better later.  Anyway, I'm not
entirely unhappy with this patch because all the tests pass, and they
are reasonably strict and sensitive to this stuff.  So no breakage there
is a very good sign.

João

bug#49518: 28.0.50; electric-pair-mode skip-self fails for single-quotes in python-mode

[Jim Porter]

On 9/18/2021 5:20 PM, João Távora wrote:
> So after having a look at this, I came up with a patch.  All tests
> pass, and your original request is granted.  Unfortunately, Stephen's
> desired triple-pair behaviour is also lost.  However, I think that
> behaviour can be recovered via other more elegant, less accidental ways.
> python.el has 'python-electric-pair-string-delimiter' for triple-pairing
> that doesn't seem to be doing its thing.
> 
> Here is the patch.  Give it a shot.
> 
> diff --git a/lisp/elec-pair.el b/lisp/elec-pair.el
> index d8c377a2ef..b8b8a97651 100644
> --- a/lisp/elec-pair.el
> +++ b/lisp/elec-pair.el
> @@ -198,7 +198,9 @@ electric-pair-syntax-info
>   inside a comment or string."
>     (let* ((pre-string-or-comment (or (bobp)
>                                       (nth 8 (save-excursion
> -                                             (syntax-ppss (1- (point)))))))
> +                                             (skip-chars-backward
> +                                              (make-string 1 command-event))
> +                                             (syntax-ppss (point))))))
>            (post-string-or-comment (nth 8 (syntax-ppss (point))))
>            (string-or-comment (and post-string-or-comment
>                                    pre-string-or-comment))
> 

Thanks, this patch works for me. It also works fine with my patch in 
bug#50538, so there's no conflict there.

> Maybe better less brittle solutions can probably be found within the
> syntax tables framework.

I agree that something less brittle would be nice, but that might be 
tricky. I had tried a few options when I filed this bug, but nothing 
worked quite right.

As for Stephen's desired behavior, the code snippet I posted above[1] 
mostly fixes it (just the `python-electric-pair-string-delimiter' part 
is necessary after your patch). However, that code doesn't work right if 
`electric-pair-skip-self' is set to nil, so I'd need to be a bit smarter 
with the implementation if I wanted to merge it into Emacs. Perhaps 
there's an even cleaner solution though.

[1] https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-09/msg01509.html

bug#49518: 28.0.50; electric-pair-mode skip-self fails for single-quotes in python-mode

[João Távora]

[Stefan, I'm CC-ing you to assist with a python.el syntax propertization
bug and a patch for it at the end of this mail]

Jim Porter <jporterbugs@gmail.com> writes:

> On 9/18/2021 5:20 PM, João Távora wrote:
>> Here is the patch.  Give it a shot.
>> diff --git a/lisp/elec-pair.el b/lisp/elec-pair.el
>> index d8c377a2ef..b8b8a97651 100644
>> --- a/lisp/elec-pair.el
>> +++ b/lisp/elec-pair.el
>> @@ -198,7 +198,9 @@ electric-pair-syntax-info
>>   inside a comment or string."
>>     (let* ((pre-string-or-comment (or (bobp)
>>                                       (nth 8 (save-excursion
>> -                                             (syntax-ppss (1- (point)))))))
>> +                                             (skip-chars-backward
>> +                                              (string command-event))
>> +                                             (syntax-ppss (point))))))
>>            (post-string-or-comment (nth 8 (syntax-ppss (point))))
>>            (string-or-comment (and post-string-or-comment
>>                                    pre-string-or-comment))
>> 
>
> Thanks, this patch works for me. 
>> Maybe better less brittle solutions can probably be found within the
>> syntax tables framework.
> I agree that something less brittle would be nice, but that might be
> tricky. I had tried a few options when I filed this bug, but nothing
> worked quite right.

The point of 'electric-pair-syntax-info's STRING-OR-COMMENT-START return
value is to tell if both the current and the pre-insertion point are and
were inside a string or a comment.  It seems reasonable (i.e. not crazy)
to skip the same syntax _and_ char backwards to discover that.  Ideally
this would be remembered from before the insertion, it's true.  But
again, the fact that the patch passes all demanding tests is a good
sign: they are quite strict.  Notice, for example, that if the patch
were the seemingly also reasonable:

       (let* ((pre-string-or-comment (or (bobp)
                                         (nth 8 (save-excursion
    -                                             (skip-chars-backward
    -                                              (string command-event))
    +                                             (skip-syntax-backward
    +                                              (string (char-syntax command-event)))
                                                  (syntax-ppss (point))))))
              (post-string-or-comment (nth 8 (syntax-ppss (point))))
              (string-or-comment (and post-string-or-comment

There would be one single test failure in a ruby-mode "mixed quote"
situation.

> As for Stephen's desired behavior, the code snippet I posted above[1]
> mostly fixes it (just the `python-electric-pair-string-delimiter' part
> is necessary after your patch). However, that code doesn't work right
> if `electric-pair-skip-self' is set to nil, so I'd need to be a bit
> smarter with the implementation if I wanted to merge it into
> Emacs. Perhaps there's an even cleaner solution though.

I think the cleaner solution is to fix a bug in python.el.  Here's the
reproduction for that bug (no electric pair mode involved)

    emacs -Q something.py
    type two single quotes
    M-: (nth 3 (syntax-ppss))
    notice how the return value says you're outside a string, correctly
    type another quote
    M-: (nth 3 (syntax-ppss))
    notice how the return value says you're inside a string, correctly
    backspace the quote just entered
    M-: (nth 3 (syntax-ppss))
    notice how the return value says you're inside a string, incorrectly

When this bug is fixed, I'm confident that Stephen's case will start
working.

In fact, it's possible that if this python.el bug were fixed, we
wouldn't need the above patch at all, becasue backtracking one char into
just before the third quote of a triple quote should _also_ yield a nil
(nth 3 (syntax-ppss)).

In fact, I have tried a patch for python.el that could fix _all_ of this
(but would need some heavy testing probably):
 
     (defvar python-mode-syntax-tablediff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
    index 19b79b6919..da7b92ae42 100644
    --- a/lisp/progmodes/python.el
    +++ b/lisp/progmodes/python.el
    @@ -775,12 +775,14 @@ python-syntax-stringify
                ;; The first quote is escaped, so it's not part of a triple quote!
                (goto-char (1+ quote-starting-pos)))
               ((null string-start)
    -           ;; This set of quotes delimit the start of a string.
    -           (put-text-property quote-starting-pos (1+ quote-starting-pos)
    +           ;; This set of quotes delimit the start of a string.  Put
    +           ;; the delimiter syntax in the last of the three quotes.
    +           (put-text-property (1- quote-ending-pos) quote-ending-pos
                                   'syntax-table (string-to-syntax "|")))
               (t
    -           ;; This set of quotes delimit the end of a string.
    -           (put-text-property (1- quote-ending-pos) quote-ending-pos
    +           ;; This set of quotes delimit the end of a string.  Put the
    +           ;; delimiter syntax in the first of the three quotess.
    +           (put-text-property quote-starting-pos (1+ quote-starting-pos)
                                   'syntax-table (string-to-syntax "|"))))))
     
     (defvar python-mode-syntax-table

João

bug#49518: 28.0.50; electric-pair-mode skip-self fails for single-quotes in python-mode

[João Távora]

João Távora <joaotavora@gmail.com> writes:

> [Stefan, I'm CC-ing you to assist with a python.el syntax propertization
> bug and a patch for it at the end of this mail]

...in fact, I've done a big of digging and it seems that the patch I
posted earlier fixes a longstanding failing test in python-mode.el as
well.  Opened as the result of #17912 more than 7 years ago (i'd totally
forgotten that)!  Some triple-pairing tests I added there should have
started failing, but didn't for some reason.  

Anyway, skipping and pairing, and triple-pairing are working with the
python.el, so I'm going to push the following fixes and tests soon,
unless there are major objections.  All python tests pass, too.

João

commit b0c34e3c207be0450fddd70620c600cd546751f2
Author: João Távora <joaotavora@gmail.com>
Date:   Sun Sep 19 17:08:41 2021 +0100

    Test electric-pair-mode more closely in python-mode, too (bug#49518)
    
    * test/lisp/electric-tests.el (define-electric-pair-test): Also run
    main tests for python-mode.  (pair-some-quotes-skip-others): Test
    another slightly different pairing.

diff --git a/test/lisp/electric-tests.el b/test/lisp/electric-tests.el
index 235f46056f..85a8e23bfa 100644
--- a/test/lisp/electric-tests.el
+++ b/test/lisp/electric-tests.el
@@ -174,7 +174,7 @@ define-electric-pair-test
           expected-string
           expected-point
           bindings
-          (modes '(quote (ruby-mode js-mode)))
+          (modes '(quote (ruby-mode js-mode python-mode)))
           (test-in-comments t)
           (test-in-strings t)
           (test-in-code t)
@@ -297,7 +297,7 @@ only-skip-over-at-least-partially-balanced-stuff
 ;;; Quotes
 ;;;
 (define-electric-pair-test pair-some-quotes-skip-others
-  " \"\"      " "-\"\"-----" :skip-pair-string "-ps------"
+  " \"\"      " "-\"\"\"\"---" :skip-pair-string "-ps-p----"
   :test-in-strings nil
   :bindings `((electric-pair-text-syntax-table
                . ,prog-mode-syntax-table)))

commit 44870df239ba681e826795fc54d69e8d9a517826
Author: João Távora <joaotavora@gmail.com>
Date:   Sun Sep 19 11:42:20 2021 +0100

    Make syntax-ppss more accurate for Python triple quotes (bug#49518)
    
    By putting delimiter syntax on the "inside" of Python triple-quoted
    strings, this makes syntax-ppss be more accurate and thus helps things
    like electric-pair-mode.  Also, the test
    python-syntax-after-python-backspace now passes, again.
    
    * lisp/progmodes/python.el (python-syntax-stringify): Put
    delimiter syntax in "inner" of the surrouding triple quotes.
    
    * test/lisp/progmodes/python-tests.el
    (python-syntax-after-python-backspace): Now passes.

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 19b79b6919..da7b92ae42 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -775,12 +775,14 @@ python-syntax-stringify
            ;; The first quote is escaped, so it's not part of a triple quote!
            (goto-char (1+ quote-starting-pos)))
           ((null string-start)
-           ;; This set of quotes delimit the start of a string.
-           (put-text-property quote-starting-pos (1+ quote-starting-pos)
+           ;; This set of quotes delimit the start of a string.  Put
+           ;; the delimiter syntax in the last of the three quotes.
+           (put-text-property (1- quote-ending-pos) quote-ending-pos
                               'syntax-table (string-to-syntax "|")))
           (t
-           ;; This set of quotes delimit the end of a string.
-           (put-text-property (1- quote-ending-pos) quote-ending-pos
+           ;; This set of quotes delimit the end of a string.  Put the
+           ;; delimiter syntax in the first of the three quotess.
+           (put-text-property quote-starting-pos (1+ quote-starting-pos)
                               'syntax-table (string-to-syntax "|"))))))
 
 (defvar python-mode-syntax-table
diff --git a/test/lisp/progmodes/python-tests.el b/test/lisp/progmodes/python-tests.el
index 1af579bb7a..a172f0f8e9 100644
--- a/test/lisp/progmodes/python-tests.el
+++ b/test/lisp/progmodes/python-tests.el
@@ -193,7 +193,6 @@ python-tests-look-at-2
 
 (ert-deftest python-syntax-after-python-backspace ()
   ;; `python-indent-dedent-line-backspace' garbles syntax
-  :expected-result :failed
   (python-tests-with-temp-buffer
       "\"\"\""
     (goto-char (point-max))

bug#49518: 28.0.50; electric-pair-mode skip-self fails for single-quotes in python-mode

[João Távora]

After some more testing, pushed the aforementioned simple fix to
master. Expanded the electric-pair-mode tests to python, too.

See commit 0646c6817139aa905a2f6079fdc82eb4be944de0
and commit 9ad962e118a17daf073ef5308233f9301755035d

Marking this bug done as all the problems mentioned here seem
to be solved.  I'd appreciate confirmation/feedback.

João

bug#49518: 28.0.50; electric-pair-mode skip-self fails for single-quotes in python-mode

[Jim Porter]

On 9/21/2021 2:59 AM, João Távora wrote:
> After some more testing, pushed the aforementioned simple fix to
> master. Expanded the electric-pair-mode tests to python, too.
> 
> See commit 0646c6817139aa905a2f6079fdc82eb4be944de0
> and commit 9ad962e118a17daf073ef5308233f9301755035d
> 
> Marking this bug done as all the problems mentioned here seem
> to be solved.  I'd appreciate confirmation/feedback.

I've tested things out with `emacs -Q', with my .emacs config, and with 
my patch in bug#50538, and everything appears to be working correctly. 
Thanks!