bug#58780: python.el infinite loop in info-current-defun

bug#58780: python.el infinite loop in info-current-defun

[JD Smith]

[-- Attachment #1: Type: text/plain, Size: 2218 bytes --]

I noticed python.el hangs hard on some of my files when `python-info-current-defun` is set up for use with which-function, and I open (but haven’t yet closed) a triple string: “”".  I tracked this down to a bug in `python-nav-end-of-statement` when an unclosed string is included in a file:

==========
def try():
    """Do the Foo

def a():
    """Do A's stuff"""
    a = True

===========

(Note: the final newline is important).  (python-info-current-defun) hangs on the unclosed docsig string of try().  

The reason why can be demonstrated by placing the cursor before a = True on the final line and (python-nav-end-of-statement).  Point moves to the end of the previous line!   Since `python-nav-end-of-defun` calls end-of-statement repeatedly looking for (eobp), this results in an infinite loop.  The problem is this call in end-of-statement:

  (re-search-forward (rx (syntax string-delimiter)) nil t)

Search starts at the single apostrophe in Do A’s stuff (the beginning of the apparent-but-incorrect string ppss has found), then searches forward to the triple quote at the end of the (prior) line.  

To reproduce this you need:

- an unclosed triple string above
- a triple string with another type of quote mark enclosed
- something after the final “”” (to prevent eobp). 

These are surprisingly common conditions to encounter given python docstring format.  A fix might be to insist that the `python-nav-end-of-statement` occurs at least at the end of the current line, or perhaps to improve the regex search for the end of string to match the opening string delimiter (although this could also be fooled I think).

This is Emacs 28, though aside from some additional commentary about such issues, end-of-statement hasn’t changed in the latest. 

As an aside, having stepped through this code, it seems python’s structural navigation and inspection are _very_ heavy, commonly traversing entire files one statement at a time to find the local function name, for example.  Due to their complexity, they are also susceptible to these types of infinite loops when syntax is in a temporarily broken state.  Good arguments for the inclusion of tree-sitter!


[-- Attachment #2: Type: text/html, Size: 4579 bytes --]

bug#58780: python.el infinite loop in info-current-defun

[Ryan B]

[-- Attachment #1: Type: text/plain, Size: 991 bytes --]

I think I have a repro for this with a single quote ' , as opposed to
triple string. I think I started seeing this hang after upgrading to Emacs
28.2. I now hit it very often, it's pretty painful. I don't know for sure
that it's the same bug, but it seems likely. Reproduces with emacs -Q:

======
class Foo():

  def __init__(self):
    '

  def bar(self):
    """Fetches posts and converts them to ActivityStreams activities.
    See source.Source.get_activities_response for details. app_id is
    ignored. min_id is translated to Twitter's since_id.
    """
    pass
======

The single quote and multiple lines in the docstring are necessary for the
repro. No extra newline at the end needed though.

JD, do you have a workaround for this? I may look into overriding
python-nav-end-of-defun until it's fixed. Any other ideas?

My emacs-version: GNU Emacs 28.2 (build 1, aarch64-apple-darwin21.1.0, NS
appkit-2113.00 Version 12.0.1 (Build 21A559)) of 2022-09-12

-- 
https://snarfed.org/

[-- Attachment #2: Type: text/html, Size: 1411 bytes --]

bug#58780: python.el infinite loop in info-current-defun

[Ryan B]

[-- Attachment #1: Type: text/plain, Size: 1368 bytes --]

Confirmed, probably the same root cause. When I brute force override
python-nav-end-of-statement like below, it doesn't happen any more.

(defun python-nav-end-of-statement (&optional noend)
  (interactive "^")
  (forward-line 1))


On Mon, Dec 5, 2022 at 6:44 AM Ryan B <pair@ryanb.org> wrote:

> I think I have a repro for this with a single quote ' , as opposed to
> triple string. I think I started seeing this hang after upgrading to Emacs
> 28.2. I now hit it very often, it's pretty painful. I don't know for sure
> that it's the same bug, but it seems likely. Reproduces with emacs -Q:
>
> ======
> class Foo():
>
>   def __init__(self):
>     '
>
>   def bar(self):
>     """Fetches posts and converts them to ActivityStreams activities.
>     See source.Source.get_activities_response for details. app_id is
>     ignored. min_id is translated to Twitter's since_id.
>     """
>     pass
> ======
>
> The single quote and multiple lines in the docstring are necessary for the
> repro. No extra newline at the end needed though.
>
> JD, do you have a workaround for this? I may look into overriding
> python-nav-end-of-defun until it's fixed. Any other ideas?
>
> My emacs-version: GNU Emacs 28.2 (build 1, aarch64-apple-darwin21.1.0, NS
> appkit-2113.00 Version 12.0.1 (Build 21A559)) of 2022-09-12
>
> --
> https://snarfed.org/
>


-- 
https://snarfed.org/

[-- Attachment #2: Type: text/html, Size: 2150 bytes --]

bug#58780: python.el infinite loop in info-current-defun

[Ryan B]

[-- Attachment #1: Type: text/plain, Size: 1638 bytes --]

I take it back, that override doesn't fully prevent it after all. 😢

On Mon, Dec 5, 2022 at 6:57 AM Ryan B <pair@ryanb.org> wrote:

> Confirmed, probably the same root cause. When I brute force override
> python-nav-end-of-statement like below, it doesn't happen any more.
>
> (defun python-nav-end-of-statement (&optional noend)
>   (interactive "^")
>   (forward-line 1))
>
>
> On Mon, Dec 5, 2022 at 6:44 AM Ryan B <pair@ryanb.org> wrote:
>
>> I think I have a repro for this with a single quote ' , as opposed to
>> triple string. I think I started seeing this hang after upgrading to Emacs
>> 28.2. I now hit it very often, it's pretty painful. I don't know for sure
>> that it's the same bug, but it seems likely. Reproduces with emacs -Q:
>>
>> ======
>> class Foo():
>>
>>   def __init__(self):
>>     '
>>
>>   def bar(self):
>>     """Fetches posts and converts them to ActivityStreams activities.
>>     See source.Source.get_activities_response for details. app_id is
>>     ignored. min_id is translated to Twitter's since_id.
>>     """
>>     pass
>> ======
>>
>> The single quote and multiple lines in the docstring are necessary for
>> the repro. No extra newline at the end needed though.
>>
>> JD, do you have a workaround for this? I may look into overriding
>> python-nav-end-of-defun until it's fixed. Any other ideas?
>>
>> My emacs-version: GNU Emacs 28.2 (build 1, aarch64-apple-darwin21.1.0, NS
>> appkit-2113.00 Version 12.0.1 (Build 21A559)) of 2022-09-12
>>
>> --
>> https://snarfed.org/
>>
>
>
> --
> https://snarfed.org/
>


-- 
https://snarfed.org/

[-- Attachment #2: Type: text/html, Size: 2702 bytes --]

bug#58780: python.el infinite loop in info-current-defun

[Ryan B]

[-- Attachment #1: Type: text/plain, Size: 2044 bytes --]

Advice seems to work better at preventing the hang:

(defun python-nav-end-of-statement--bug-override (orig-fn &optional noend)
  (forward-line 1))
(advice-add 'python-nav-end-of-statement :around
#'python-nav-end-of-statement--bug-override)

On Mon, Dec 5, 2022 at 7:01 AM Ryan B <pair@ryanb.org> wrote:

> I take it back, that override doesn't fully prevent it after all. 😢
>
> On Mon, Dec 5, 2022 at 6:57 AM Ryan B <pair@ryanb.org> wrote:
>
>> Confirmed, probably the same root cause. When I brute force override
>> python-nav-end-of-statement like below, it doesn't happen any more.
>>
>> (defun python-nav-end-of-statement (&optional noend)
>>   (interactive "^")
>>   (forward-line 1))
>>
>>
>> On Mon, Dec 5, 2022 at 6:44 AM Ryan B <pair@ryanb.org> wrote:
>>
>>> I think I have a repro for this with a single quote ' , as opposed to
>>> triple string. I think I started seeing this hang after upgrading to Emacs
>>> 28.2. I now hit it very often, it's pretty painful. I don't know for sure
>>> that it's the same bug, but it seems likely. Reproduces with emacs -Q:
>>>
>>> ======
>>> class Foo():
>>>
>>>   def __init__(self):
>>>     '
>>>
>>>   def bar(self):
>>>     """Fetches posts and converts them to ActivityStreams activities.
>>>     See source.Source.get_activities_response for details. app_id is
>>>     ignored. min_id is translated to Twitter's since_id.
>>>     """
>>>     pass
>>> ======
>>>
>>> The single quote and multiple lines in the docstring are necessary for
>>> the repro. No extra newline at the end needed though.
>>>
>>> JD, do you have a workaround for this? I may look into overriding
>>> python-nav-end-of-defun until it's fixed. Any other ideas?
>>>
>>> My emacs-version: GNU Emacs 28.2 (build 1, aarch64-apple-darwin21.1.0,
>>> NS appkit-2113.00 Version 12.0.1 (Build 21A559)) of 2022-09-12
>>>
>>> --
>>> https://snarfed.org/
>>>
>>
>>
>> --
>> https://snarfed.org/
>>
>
>
> --
> https://snarfed.org/
>


-- 
https://snarfed.org/

[-- Attachment #2: Type: text/html, Size: 3475 bytes --]

bug#58780: python.el infinite loop in info-current-defun

[kobarity]

[-- Attachment #1: Type: text/plain, Size: 3054 bytes --]


As JD Smith pointed out, this problem is caused by
`python-nav-end-of-statement' moving point backward.  A simple example
illustrating the problem of `python-nav-end-of-statement' is as
follows:

#+begin_src python
' """
a = 1
#+end_src

When point is placed on the second line and
`python-nav-end-of-statement' is executed, point will move to the end
of the first line.

Here is why this happens.

`python-nav-end-of-statement' moves point to the end of the second
line, and checks if it is in the middle of the string using:

(python-syntax-context 'string)

It tells that the point is within the string started at the
single-quote in the first line, and the variable `string-start' is set
to the start of the string (single-quote in the first line).  Then,
point is moved to the start of the string, and the end of the string
is searched for using:

(re-search-forward (rx (syntax string-delimiter)) nil t)

It finds the triple double-quotes and moves point immediately after
them.  The variable `last-string-end' is set to this position.

In the second iteration of the while loop, the variable `string-start'
is set to the single-quote in the first line again.  This results in
exiting the loop because the following condition is not met.

(>= string-start last-string-end)

There are several issues here:

1. Although `python-syntax-context' detects both string-quote
(single quote) and string-delimiter (triple quotes) as the start of
string, only string-delimiter is searched for as the end of string.
2. The triple double-quotes is marked as string-delimiter even though
they are within the string begins with the single-quote.
3. As the end of the first line is not escaped using a backslash, the
string begins with the single-quote should not be continued on the
second line.

I would like to discuss the second and third issues separately.  The
first issue is the main cause of Bug#58780.  This issue also causes
another type of problem, illustrated by the following example.

#+begin_src python
abc = 'a\
b\
c'
d = '''d'''
#+end_src

When `python-nav-end-of-statement' is executed with point located at
the second line, point is moved to the end of the fourth line instead
of the third line.  This is a wrong point move for a correct Python
program.

The attached
0001-Fix-searching-for-end-of-string-in-python-nav-end-of.patch is my
proposal to fix this issue with some ERTs.  `forward-sexp' (in fact
`python-nav--lisp-forward-sexp') is added to search for the end of the
string if the string begins with single single/double-quote (' or ").

I think this 0001 patch also fixes Bug#56271.  At least, it passes the
ERT python-nav-end-of-block-2 which is introduced in Bug#56271, even
the workaround introduced in Bug#56271 is reverted.  The attached
0002-Revert-workaround-introduced-in-Bug-56271.patch is a patch to
revert the workaround.  If the 0001 patch is accepted, the 0002 patch
can also be applied.  But if we want to be safer, we can leave the
workaround as it is by not applying the 0002 patch.

I look forward to your comments.

[-- Attachment #2: 0001-Fix-searching-for-end-of-string-in-python-nav-end-of.patch --]
[-- Type: application/octet-stream, Size: 3875 bytes --]

From 62cfa24a89fdbf90cbe866ad88ca635327eb1f49 Mon Sep 17 00:00:00 2001
From: kobarity <kobarity@gmail.com>
Date: Sun, 5 Mar 2023 17:06:26 +0900
Subject: [PATCH 1/2] Fix searching for end of string in
 python-nav-end-of-statement

* lisp/progmodes/python.el (python-nav-end-of-statement): Add
searching for corresponding string-quote.
* test/lisp/progmodes/python-tests.el (python-nav-end-of-statement-3)
(python-nav-end-of-statement-4, python-info-current-defun-4): New
tests. (Bug#58780)
---
 lisp/progmodes/python.el            | 14 ++++++---
 test/lisp/progmodes/python-tests.el | 44 +++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 1f970633bfc..cc4ece1669c 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -2076,10 +2076,16 @@ python-nav-end-of-statement
                            (goto-char (+ (point)
                                          (python-syntax-count-quotes
                                           (char-after (point)) (point))))
-                           (setq last-string-end
-                                 (or (re-search-forward
-                                      (rx (syntax string-delimiter)) nil t)
-                                     (goto-char (point-max)))))))
+                           (setq
+                            last-string-end
+                            (or (if (eq t (nth 3 (syntax-ppss)))
+                                    (re-search-forward
+                                     (rx (syntax string-delimiter)) nil t)
+                                  (ignore-error scan-error
+                                    (goto-char string-start)
+                                    (python-nav--lisp-forward-sexp)
+                                    (point)))
+                                (goto-char (point-max)))))))
                       ((python-syntax-context 'paren)
                        ;; The statement won't end before we've escaped
                        ;; at least one level of parenthesis.
diff --git a/test/lisp/progmodes/python-tests.el b/test/lisp/progmodes/python-tests.el
index 4f24c042c6a..e9df4a2c843 100644
--- a/test/lisp/progmodes/python-tests.el
+++ b/test/lisp/progmodes/python-tests.el
@@ -2943,6 +2943,36 @@ python-nav-end-of-statement-2
    "'\n''\n"
    (python-nav-end-of-statement)))
 
+(ert-deftest python-nav-end-of-statement-3 ()
+  "Test unmatched quotes (Bug#58780)."
+  (python-tests-with-temp-buffer
+   "
+' \"\"\"
+v = 1
+"
+   (python-tests-look-at "v =")
+   (should (= (save-excursion
+                (python-nav-end-of-statement)
+                (point))
+              (save-excursion
+                (point-max))))))
+
+(ert-deftest python-nav-end-of-statement-4 ()
+  (python-tests-with-temp-buffer
+   "
+abc = 'a\\
+b\\
+c'
+d = '''d'''
+"
+   (python-tests-look-at "b\\")
+   (should (= (save-excursion
+                (python-nav-end-of-statement)
+                (point))
+              (save-excursion
+                (python-tests-look-at "c'")
+                (pos-eol))))))
+
 (ert-deftest python-nav-forward-statement-1 ()
   (python-tests-with-temp-buffer
    "
@@ -5209,6 +5239,20 @@ python-info-current-defun-3
    (should (string= (python-info-current-defun t)
                     "def decoratorFunctionWithArguments"))))
 
+(ert-deftest python-info-current-defun-4 ()
+  "Ensure unmatched quotes do not cause hang (Bug#58780)."
+  (python-tests-with-temp-buffer
+   "
+def func():
+    ' \"\"\"
+    v = 1
+"
+   (python-tests-look-at "v = 1")
+   (should (string= (python-info-current-defun)
+                    "func"))
+   (should (string= (python-info-current-defun t)
+                    "def func"))))
+
 (ert-deftest python-info-current-symbol-1 ()
   (python-tests-with-temp-buffer
    "
-- 
2.34.1


[-- Attachment #3: 0002-Revert-workaround-introduced-in-Bug-56271.patch --]
[-- Type: application/octet-stream, Size: 1988 bytes --]

From f80cf3e53ceeaa940fa82a2e294dd43c6e74231b Mon Sep 17 00:00:00 2001
From: kobarity <kobarity@gmail.com>
Date: Sun, 5 Mar 2023 17:07:17 +0900
Subject: [PATCH 2/2] Revert workaround introduced in Bug#56271

* lisp/progmodes/python.el (python-nav-end-of-statement)
(python-nav-end-of-block): Revert workaround introduced in Bug#56271
as the bug is fixedin Bug#58780.
---
 lisp/progmodes/python.el | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index cc4ece1669c..aa0a043a275 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -2062,10 +2062,6 @@ python-nav-end-of-statement
                        ;; are somehow out of whack.  This has been
                        ;; observed when using `syntax-ppss' during
                        ;; narrowing.
-                       ;; It can also fail in cases where the buffer is in
-                       ;; the process of being modified, e.g. when creating
-                       ;; a string with `electric-pair-mode' disabled such
-                       ;; that there can be an unmatched single quote
                        (when (>= string-start last-string-end)
                          (goto-char string-start)
                          (if (python-syntax-context 'paren)
@@ -2154,10 +2150,7 @@ python-nav-end-of-block
       (while (and (forward-line 1)
                   (not (eobp))
                   (or (and (> (current-indentation) block-indentation)
-                           (let ((start (point)))
-                             (python-nav-end-of-statement)
-                             ;; must move forward otherwise infinite loop
-                             (> (point) start)))
+                           (or (python-nav-end-of-statement) t))
                       (python-info-current-line-comment-p)
                       (python-info-current-line-empty-p))))
       (python-util-forward-comment -1)
-- 
2.34.1

bug#58780: python.el infinite loop in info-current-defun

[Eli Zaretskii]

> Date: Sun, 05 Mar 2023 17:19:43 +0900
> From: kobarity <kobarity@gmail.com>
> 
> The attached
> 0001-Fix-searching-for-end-of-string-in-python-nav-end-of.patch is my
> proposal to fix this issue with some ERTs.  `forward-sexp' (in fact
> `python-nav--lisp-forward-sexp') is added to search for the end of the
> string if the string begins with single single/double-quote (' or ").
> 
> I think this 0001 patch also fixes Bug#56271.  At least, it passes the
> ERT python-nav-end-of-block-2 which is introduced in Bug#56271, even
> the workaround introduced in Bug#56271 is reverted.  The attached
> 0002-Revert-workaround-introduced-in-Bug-56271.patch is a patch to
> revert the workaround.  If the 0001 patch is accepted, the 0002 patch
> can also be applied.  But if we want to be safer, we can leave the
> workaround as it is by not applying the 0002 patch.
> 
> I look forward to your comments.

There were no more comments, so I've now installed both patches on the
emacs-29 branch, and I'm boldly closing this bug.

Thanks.