bug#67262: python-ts-mode cannot identify triple-quoted-strings

bug#67262: python-ts-mode cannot identify triple-quoted-strings

[JD Smith]

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


Inside this triple-quoted string, in a python buffer:

a = """This is a test"""

python-mode yields (python-info-triple-quoted-string-p)=t, whereas python-ts-mode gives nil, defeating the fancy doc string folding both modes implement.

The reason seems to be that (syntax-ppss) returns something different in position 3 (which is "non-nil if inside a string”) between these modes: 

t for python-mode (which signals a triple quote)
 ?34=" in python-ts-mode 

If you first load python-mode, then load python-ts-mode, the syntax parse becomes equal between the modes, and this bug vanishes.  

python.el v0.28, Emacs v29.1

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

bug#67262: python-ts-mode cannot identify triple-quoted-strings

[Eli Zaretskii]

> From: JD Smith <jdtsmith@gmail.com>
> Date: Sat, 18 Nov 2023 10:52:05 -0500
> 
> Inside this triple-quoted string, in a python buffer:
> 
> a = """This is a test"""
> 
> python-mode yields (python-info-triple-quoted-string-p)=t, whereas python-ts-mode gives nil,
> defeating the fancy doc string folding both modes implement.
> 
> The reason seems to be that (syntax-ppss) returns something different in position 3 (which is "non-nil
> if inside a string”) between these modes: 
> 
> * t for python-mode (which signals a triple quote)
> *  ?34=" in python-ts-mode 
> 
> If you first load python-mode, then load python-ts-mode, the syntax parse becomes equal between the
> modes, and this bug vanishes.  

Can you figure out which part of python-mode's initialization makes
the above work correctly, and why?  Then we could discuss whether
moving that part into python-base-mode is TRT.

Thanks.

bug#67262: python-ts-mode cannot identify triple-quoted-strings

[JD Smith]

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



> On Nov 18, 2023, at 11:29 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: JD Smith <jdtsmith@gmail.com>
>> Date: Sat, 18 Nov 2023 10:52:05 -0500
>> 
>> Inside this triple-quoted string, in a python buffer:
>> 
>> a = """This is a test"""
>> 
>> python-mode yields (python-info-triple-quoted-string-p)=t, whereas python-ts-mode gives nil,
> 
> Can you figure out which part of python-mode's initialization makes
> the above work correctly, and why?  Then we could discuss whether
> moving that part into python-base-mode is TRT.

I took a look. It’s `syntax-propertize-function' that is not being setup.  From a buffer in `python-ts-mode', this reenables triple-quote recognition:

(progn 
  (setq-local syntax-propertize-function python-syntax-propertize-function) 
  (syntax-ppss-flush-cache (point-min)))

Note that `python-syntax-propertize-function' mentions `python-syntax-stringify', which scans the syntax for triple quotes and marks their 'syntax-table.  I am not sure whether this was an oversight, or was omitted purposefully from the body of python-ts-mode.

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

bug#67262: python-ts-mode cannot identify triple-quoted-strings

[Dmitry Gutov]

On 18/11/2023 19:18, JD Smith wrote:
>    (setq-local syntax-propertize-function 
> python-syntax-propertize-function)

This is not a bad choice, but even better would be to write a s-p-f 
based on the tree-sitter parse tree.

There are examples in ruby-ts-mode and js-ts-mode.

bug#67262: python-ts-mode cannot identify triple-quoted-strings

[Eli Zaretskii]

> Date: Sun, 19 Nov 2023 00:52:06 +0200
> Cc: 67262@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 18/11/2023 19:18, JD Smith wrote:
> >    (setq-local syntax-propertize-function 
> > python-syntax-propertize-function)
> 
> This is not a bad choice, but even better would be to write a s-p-f 
> based on the tree-sitter parse tree.
> 
> There are examples in ruby-ts-mode and js-ts-mode.

JD, would you like to try writing such a syntax-propertize-function?

bug#67262: python-ts-mode cannot identify triple-quoted-strings

[JD Smith]

Bridging emacs syntax to treesitter in a robust way seems like it could be a subtle enterprise, so I’d prefer to leave that to one of the experts.  Right now the syntax-propertize-function in python-mode does one simple thing: ensure triple quotes are properly marked as strings.  Since the treesitter grammar doesn’t distinguish between different flavors of strings, something similar would still be needed, if we want to continue to treat various string flavors distinctly using syntax.  

Is moving all syntactification (beyond just font-lock) over to TS an explicit goal for all the *-ts-mode’s?

> On Nov 25, 2023, at 5:01 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> Date: Sun, 19 Nov 2023 00:52:06 +0200
>> Cc: 67262@debbugs.gnu.org
>> From: Dmitry Gutov <dmitry@gutov.dev>
>> 
>> On 18/11/2023 19:18, JD Smith wrote:
>>>   (setq-local syntax-propertize-function 
>>> python-syntax-propertize-function)
>> 
>> This is not a bad choice, but even better would be to write a s-p-f 
>> based on the tree-sitter parse tree.
>> 
>> There are examples in ruby-ts-mode and js-ts-mode.
> 
> JD, would you like to try writing such a syntax-propertize-function?

bug#67262: python-ts-mode cannot identify triple-quoted-strings

[Dmitry Gutov]

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

On 25/11/2023 16:42, JD Smith wrote:
> Bridging emacs syntax to treesitter in a robust way seems like it could be a subtle enterprise, so I’d prefer to leave that to one of the experts.  Right now the syntax-propertize-function in python-mode does one simple thing: ensure triple quotes are properly marked as strings.  Since the treesitter grammar doesn’t distinguish between different flavors of strings, something similar would still be needed, if we want to continue to treat various string flavors distinctly using syntax.
> 
> Is moving all syntactification (beyond just font-lock) over to TS an explicit goal for all the *-ts-mode’s?

It would make sense - since this way we would only have one source of 
syntax-recognition bugs, rather than two (both the grammar and the 
definition in Elisp).

Attached is a patch you can try (that uses treesit for s-p-f).

Unfortunately, it's not quite perfect (nor is python-syntax-stringify, 
according to its FIXME inside): after certain modifications, the 
syntax-table property is not applied.

I've done some print-debugging in python--treesit-parser-after-change, 
and it looks like the problem is this: in certain cases (e.g. when 
electric-pair-post-self-insert-function fires) the parser notifier fires 
only after syntax-propertize has been called -- and it fires inside of 
it. Meaning it's too late to flush the syntax-propertize cache at that 
point.

The reason for it is, overall, the fast that we're trigger parser's 
after-change notifiers lazily: only after some other feature has to 
initialize the parser, calling treesit_ensure_parsed from 
treesit-parser-root-node.

I think bug#66732 might also be a variation of this problem.

As for what to do about this one -- probably something involving 
syntax-propertize-extend-region-functions, adding an entry which would 
initialize the parser, but not call syntax-ppss-flush-cache directly (or 
at least not just that). It would signal the earlier position to extend 
to through some dynamic variable. This is getting tricky enough to move 
from the individual major modes into treesit.el proper, I think.

Yuan and others, thoughts welcome.

JD, I do believe the attached patch is TRT (or close to it), but 
depending on how it works for you, and how quickly we deal with the 
above problem, it might make sense to enact your original suggestion first.

And finally, here's the backtrace that led me to the above conclusions:

   backtrace()
   (message "in progress, backtrace %s" (backtrace))
   (progn (message "in progress, backtrace %s" (backtrace)))
   (if (syntax-propertize--in-process-p) (progn (message "in progress, 
backtrace %s" (backtrace))))
   (save-current-buffer (set-buffer (treesit-parser-buffer parser)) 
(message "flushing %s up to %s" ranges (let* ((--cl-var-- ranges) (r 
nil) (--cl-var-- nil)) (while (consp --cl-var--) (setq r (car 
--cl-var--)) (let* ((temp (car r))) (setq --cl-var-- (if --cl-var-- (min 
--cl-var-- temp) temp))) (setq --cl-var-- (cdr --cl-var--))) 
--cl-var--)) (syntax-ppss-flush-cache (let* ((--cl-var-- ranges) (r nil) 
(--cl-var-- nil)) (while (consp --cl-var--) (setq r (car --cl-var--)) 
(let* ((temp (car r))) (setq --cl-var-- (if --cl-var-- (min --cl-var-- 
temp) temp))) (setq --cl-var-- (cdr --cl-var--))) --cl-var--)) (if 
(syntax-propertize--in-process-p) (progn (message "in progress, 
backtrace %s" (backtrace)))) (message "flushed up to %d, %s" 
syntax-propertize--done syntax-ppss-wide))
   (progn (save-current-buffer (set-buffer (treesit-parser-buffer 
parser)) (message "flushing %s up to %s" ranges (let* ((--cl-var-- 
ranges) (r nil) (--cl-var-- nil)) (while (consp --cl-var--) (setq r (car 
--cl-var--)) (let* ((temp ...)) (setq --cl-var-- (if --cl-var-- ... 
temp))) (setq --cl-var-- (cdr --cl-var--))) --cl-var--)) 
(syntax-ppss-flush-cache (let* ((--cl-var-- ranges) (r nil) (--cl-var-- 
nil)) (while (consp --cl-var--) (setq r (car --cl-var--)) (let* ((temp 
...)) (setq --cl-var-- (if --cl-var-- ... temp))) (setq --cl-var-- (cdr 
--cl-var--))) --cl-var--)) (if (syntax-propertize--in-process-p) (progn 
(message "in progress, backtrace %s" (backtrace)))) (message "flushed up 
to %d, %s" syntax-propertize--done syntax-ppss-wide)))
   (if ranges (progn (save-current-buffer (set-buffer 
(treesit-parser-buffer parser)) (message "flushing %s up to %s" ranges 
(let* ((--cl-var-- ranges) (r nil) (--cl-var-- nil)) (while (consp 
--cl-var--) (setq r (car --cl-var--)) (let* (...) (setq --cl-var-- ...)) 
(setq --cl-var-- (cdr --cl-var--))) --cl-var--)) 
(syntax-ppss-flush-cache (let* ((--cl-var-- ranges) (r nil) (--cl-var-- 
nil)) (while (consp --cl-var--) (setq r (car --cl-var--)) (let* (...) 
(setq --cl-var-- ...)) (setq --cl-var-- (cdr --cl-var--))) --cl-var--)) 
(if (syntax-propertize--in-process-p) (progn (message "in progress, 
backtrace %s" (backtrace)))) (message "flushed up to %d, %s" 
syntax-propertize--done syntax-ppss-wide))))
   python--treesit-parser-after-change(((27 . 50)) #<treesit-parser for 
python>)
   treesit-buffer-root-node(python)
   treesit-node-at(42)
   (let ((node (treesit-node-at (point)))) (cond ((equal 
(treesit-node-type node) "string_content") (put-text-property (- (point) 
3) (- (point) 2) 'syntax-table (string-to-syntax "|"))) ((and (equal 
(treesit-node-type node) "\"") (= (treesit-node-start node) (- (point) 
3))) (put-text-property (1- (point)) (point) 'syntax-table 
(string-to-syntax "|")))))
   (cond (t (message "pt %s" (point)) (let ((node (treesit-node-at 
(point)))) (cond ((equal (treesit-node-type node) "string_content") 
(put-text-property (- (point) 3) (- (point) 2) 'syntax-table 
(string-to-syntax "|"))) ((and (equal (treesit-node-type node) "\"") (= 
(treesit-node-start node) (- ... 3))) (put-text-property (1- (point)) 
(point) 'syntax-table (string-to-syntax "|")))))))
   (while (and (< (point) end) (re-search-forward "\\(?:\"\"\"\\|'''\\)" 
end t)) (cond (t (message "pt %s" (point)) (let ((node (treesit-node-at 
(point)))) (cond ((equal (treesit-node-type node) "string_content") 
(put-text-property (- ... 3) (- ... 2) 'syntax-table (string-to-syntax 
"|"))) ((and (equal ... "\"") (= ... ...)) (put-text-property (1- ...) 
(point) 'syntax-table (string-to-syntax "|"))))))))
   (closure (t) (start end) (goto-char start) (while (and (< (point) 
end) (re-search-forward "\\(?:\"\"\"\\|'''\\)" end t)) (cond (t (message 
"pt %s" (point)) (let ((node ...)) (cond (... ...) (... ...)))))))(39 50)
   funcall((closure (t) (start end) (goto-char start) (while (and (< 
(point) end) (re-search-forward "\\(?:\"\"\"\\|'''\\)" end t)) (cond (t 
(message "pt %s" (point)) (let ((node ...)) (cond (... ...) (... 
...))))))) 39 50)
   python--treesit-syntax-propertize-function-1(39 50)
   syntax-propertize(42)
   syntax-ppss(42)
   electric-pair-syntax-info(39)
   electric-pair-post-self-insert-function()
   self-insert-command(1 39)
   funcall-interactively(self-insert-command 1 39)
   #<subr call-interactively>(self-insert-command nil nil)
   call-interactively@ido-cr+-record-current-command(#<subr 
call-interactively> self-insert-command nil nil)
   apply(call-interactively@ido-cr+-record-current-command #<subr 
call-interactively> (self-insert-command nil nil))
   call-interactively(self-insert-command nil nil)
   command-execute(self-insert-command)

[-- Attachment #2: python--treesit-syntax-propertize-function.diff --]
[-- Type: text/x-patch, Size: 1933 bytes --]

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index ab3bf1b4ec0..659def18999 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -1237,6 +1237,29 @@ python--treesit-fontify-variable
      (treesit-node-start node) (treesit-node-end node)
      'font-lock-variable-use-face override start end)))
 
+(defconst python--treesit-syntax-propertize-function
+  (syntax-propertize-rules
+   ((rx (or "\"\"\"" "'''"))
+    (0 (ignore
+        (let ((node (treesit-node-at (point))))
+          (cond
+           ((equal (treesit-node-type node) "string_content")
+            (put-text-property (- (point) 3) (- (point) 2)
+                               'syntax-table (string-to-syntax "|")))
+           ((and (equal (treesit-node-type node) "\"")
+                 (= (treesit-node-start node) (- (point) 3)))
+            (put-text-property (1- (point)) (point)
+                               'syntax-table (string-to-syntax "|"))))))))))
+
+(defun python--treesit-parser-after-change (ranges parser)
+  ;; Make sure we re-syntax-propertize the full node that is being
+  ;; edited.  For triple-quoted strings.
+  (when ranges
+    (with-current-buffer (treesit-parser-buffer parser)
+      (syntax-ppss-flush-cache (cl-loop for r in ranges
+                                        minimize (car r))))))
+
 \f
 ;;; Indentation
 
@@ -6851,6 +6874,14 @@ python-ts-mode
                                               "_definition"))
     (setq-local treesit-defun-name-function
                 #'python--treesit-defun-name)
+
+    (setq-local syntax-propertize-function
+                python--treesit-syntax-propertize-function)
+
+    ;; Make sure it's placed before font-lock's notifier.
+    (treesit-parser-add-notifier (car (treesit-parser-list))
+                                 #'python--treesit-parser-after-change)
+
     (treesit-major-mode-setup)
 
     (python-skeleton-add-menu-items)

bug#67262: python-ts-mode cannot identify triple-quoted-strings

[Dmitry Gutov]

On 26/11/2023 04:04, Dmitry Gutov wrote:
> As for what to do about this one -- probably something involving 
> syntax-propertize-extend-region-functions, adding an entry which would 
> initialize the parser, but not call syntax-ppss-flush-cache directly (or 
> at least not just that). It would signal the earlier position to extend 
> to through some dynamic variable. This is getting tricky enough to move 
> from the individual major modes into treesit.el proper, I think.

Alternatively, we'd trigger updates eagerly from within 
treesit_record_change -- that would make it slower, invalidating the 
comment above it. Not sure by how much, though.

bug#67262: python-ts-mode cannot identify triple-quoted-strings

[Yuan Fu]

On 11/26/23 6:58 AM, Dmitry Gutov wrote:
> On 26/11/2023 04:04, Dmitry Gutov wrote:
>> As for what to do about this one -- probably something involving 
>> syntax-propertize-extend-region-functions, adding an entry which 
>> would initialize the parser, but not call syntax-ppss-flush-cache 
>> directly (or at least not just that). It would signal the earlier 
>> position to extend to through some dynamic variable. This is getting 
>> tricky enough to move from the individual major modes into treesit.el 
>> proper, I think.
>
> Alternatively, we'd trigger updates eagerly from within 
> treesit_record_change -- that would make it slower, invalidating the 
> comment above it. Not sure by how much, though.

It seems to me that what we need is to force a re-parse at the beginning 
of syntax-propertize or in syntax-ppss-flush-cache; the re-parse would 
cause the notifier to run, which runs python--treesit-parser-after-change.

I'm not quite sure about how do we cause this re-parse. The 
straightforward approach would be calling treesit-force-reparse[1] in 
syntax-propertize/syntax-ppss-flush-cache. But ideally I'd like to keep 
tree-sitter transparent for syntax.el. Maybe we can add a hook in 
syntax-propertize/syntax-ppss-flush-cache.

[1] This function doesn't exist yet, but it's easy to define in lisp.

Yuan

bug#67262: python-ts-mode cannot identify triple-quoted-strings

[Dmitry Gutov]

On 27/11/2023 01:43, Yuan Fu wrote:
> 
> On 11/26/23 6:58 AM, Dmitry Gutov wrote:
>> On 26/11/2023 04:04, Dmitry Gutov wrote:
>>> As for what to do about this one -- probably something involving 
>>> syntax-propertize-extend-region-functions, adding an entry which 
>>> would initialize the parser, but not call syntax-ppss-flush-cache 
>>> directly (or at least not just that). It would signal the earlier 
>>> position to extend to through some dynamic variable. This is getting 
>>> tricky enough to move from the individual major modes into treesit.el 
>>> proper, I think.
>>
>> Alternatively, we'd trigger updates eagerly from within 
>> treesit_record_change -- that would make it slower, invalidating the 
>> comment above it. Not sure by how much, though.
> 
> It seems to me that what we need is to force a re-parse at the beginning 
> of syntax-propertize or in syntax-ppss-flush-cache; the re-parse would 
> cause the notifier to run, which runs python--treesit-parser-after-change.

syntax-ppss-flush-cache is called by edits (and by the re-parse). It 
seems like it will be odd to have execution the other way around and/or 
add some hook into it which would call the re-parse and extend the 
region to be invalidated.

syntax-propertize could have another hook added, yes. Or an advice.

But it seems better to reuse some of the existing hooks, such as 
syntax-propertize-extend-region-functions. It treesit.c provided a way 
to fetch the newly-invalidated region, the treesit-major-mode-setup 
could add a new function to syntax-propertize-extend-region-functions 
which would invoke that feature. But even now it can instantiate the 
parse, which would call treesit-force-reparse internally, and then 
collect the info from the callbacks.

And yet another way - is to extend the region to be propertized from 
inside the major mode's syntax-propertize-function, invalidating some 
earlier entries too. The main problem with that, I think, is that every 
ts mode will have to repeat that trick. And that authors would have to 
know to do that. How to make that easier and more obvious, is a question.

Finally, if I'm right that bug#66732 has a similar cause, then a shared 
solution that can be reused by syntax and font-lock (or preferably just 
fix both in the same place) would be ideal.

> I'm not quite sure about how do we cause this re-parse. The 
> straightforward approach would be calling treesit-force-reparse[1] in 
> syntax-propertize/syntax-ppss-flush-cache. But ideally I'd like to keep 
> tree-sitter transparent for syntax.el. Maybe we can add a hook in 
> syntax-propertize/syntax-ppss-flush-cache.
> 
> [1] This function doesn't exist yet, but it's easy to define in lisp.

treesit-parser-root-node calls it anyway and does little else, so we 
could get by with just using it.