bug#59693: 29.0.50; treesitter in base buffer doesn't respond to modifications in indirect buffer correctly

bug#59693: 29.0.50; treesitter in base buffer doesn't respond to modifications in indirect buffer correctly

[miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

    *** Welcome to IELM ***  Type (describe-mode) or press C-h m for help.
    ELISP> (set-buffer (get-buffer-create "a"))
    ELISP> (insert "int main();")
    ELISP> (require 'treesit)
    ELISP> (treesit-node-children (treesit-node-child (treesit-buffer-root-node 'c) 0))
    (#<treesit-node
      (primitive_type)
      in 1-4> #<treesit-node
      (function_declarator)
      in 5-11> #<treesit-node ";" in 11-12>)

This is expected

    ELISP> (set-buffer (make-indirect-buffer "a" "b"))
    ELISP> (goto-char (point-min))
    ELISP> (insert " ")
    ELISP> (set-buffer "a")
    ELISP> (buffer-string)
    " int main();"

    ELISP> (treesit-node-children (treesit-node-child (treesit-buffer-root-node 'c) 0))
    (#<treesit-node
      (primitive_type)
      in 1-4> #<treesit-node
      (function_declarator)
      in 5-11> #<treesit-node
      (ERROR)
      in 11-12> #<treesit-node ";" in 12-13>)

This is unexpected. If we had called '(insert " ")' in the base buffer
"a", we would have got

    (#<treesit-node
      (primitive_type)
      in 2-5> #<treesit-node
      (function_declarator)
      in 6-12> #<treesit-node ";" in 12-13>)

Thanks for your hard work.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

bug#59693: 29.0.50; treesitter in base buffer doesn't respond to modifications in indirect buffer correctly

[Yuan Fu]

miha@kamnitnik.top writes:

>     *** Welcome to IELM ***  Type (describe-mode) or press C-h m for help.
>     ELISP> (set-buffer (get-buffer-create "a"))
>     ELISP> (insert "int main();")
>     ELISP> (require 'treesit)
>     ELISP> (treesit-node-children (treesit-node-child (treesit-buffer-root-node 'c) 0))
>     (#<treesit-node
>       (primitive_type)
>       in 1-4> #<treesit-node
>       (function_declarator)
>       in 5-11> #<treesit-node ";" in 11-12>)
>
> This is expected
>
>     ELISP> (set-buffer (make-indirect-buffer "a" "b"))
>     ELISP> (goto-char (point-min))
>     ELISP> (insert " ")
>     ELISP> (set-buffer "a")
>     ELISP> (buffer-string)
>     " int main();"
>
>     ELISP> (treesit-node-children (treesit-node-child (treesit-buffer-root-node 'c) 0))
>     (#<treesit-node
>       (primitive_type)
>       in 1-4> #<treesit-node
>       (function_declarator)
>       in 5-11> #<treesit-node
>       (ERROR)
>       in 11-12> #<treesit-node ";" in 12-13>)
>
> This is unexpected. If we had called '(insert " ")' in the base buffer
> "a", we would have got
>
>     (#<treesit-node
>       (primitive_type)
>       in 2-5> #<treesit-node
>       (function_declarator)
>       in 6-12> #<treesit-node ";" in 12-13>)
>
> Thanks for your hard work.

Thanks! I forgot about indirect buffers... We’ll need to make sure to
use the parsers in the original buffer when user edits the indirect
buffer, or something like that. I need to look into how does indirect
buffer works.

Yuan

bug#59693: 29.0.50; treesitter in base buffer doesn't respond to modifications in indirect buffer correctly

[Eli Zaretskii]

> Cc: 59693@debbugs.gnu.org
> From: Yuan Fu <casouri@gmail.com>
> Date: Wed, 30 Nov 2022 02:17:14 -0800
> 
> Thanks! I forgot about indirect buffers... We’ll need to make sure to
> use the parsers in the original buffer when user edits the indirect
> buffer, or something like that. I need to look into how does indirect
> buffer works.

In the insdel.c hooks where you record changes to buffer text, you should
see if the buffer has a base_buffer, and if so, update any parsers of the
base buffer as well.

bug#59693: 29.0.50; treesitter in base buffer doesn't respond to modifications in indirect buffer correctly

[Yuan Fu]

> On Nov 30, 2022, at 6:05 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> Cc: 59693@debbugs.gnu.org
>> From: Yuan Fu <casouri@gmail.com>
>> Date: Wed, 30 Nov 2022 02:17:14 -0800
>> 
>> Thanks! I forgot about indirect buffers... We’ll need to make sure to
>> use the parsers in the original buffer when user edits the indirect
>> buffer, or something like that. I need to look into how does indirect
>> buffer works.
> 
> In the insdel.c hooks where you record changes to buffer text, you should
> see if the buffer has a base_buffer, and if so, update any parsers of the
> base buffer as well.

Actually there’s a little bit of problem. When we edit the base buffer, we would want to update the parsers in all of its indirect buffers as well, and AFAICT there is no pointer from base buffer to the indirect buffer, only the other way around. 

We don’t want indirect buffer and base buffers to share parsers, since they can have different narrowing, and semantically indirect buffers should share anything but the text with the base buffer.

How about this: we change current_buffer->parser_list from a plain list of parsers to a cons (PARSER-LIST . INDIRECT-PARSER-LIST), where PARSER-LIST is as before. But for base buffers, INDIRECT-PARSER-LIST includes all the parsers of its indirect buffers; and for indirect buffers, INDIRECT-PARSER-LIST is nil.

Then base buffer can update all indirect buffers’ parsers, and indirect buffer can find its base buffer and update all the parsers, including the base buffer’s parsers and other indirect buffers’ parsers.

Of course, treesit-parser-create and treesit-parser-delete needs to do some extra work, but nothing complicated.

Yuan

bug#59693: 29.0.50; treesitter in base buffer doesn't respond to modifications in indirect buffer correctly

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Thu, 1 Dec 2022 21:05:43 -0800
> Cc: miha@kamnitnik.top,
>  59693@debbugs.gnu.org
> 
> > In the insdel.c hooks where you record changes to buffer text, you should
> > see if the buffer has a base_buffer, and if so, update any parsers of the
> > base buffer as well.
> 
> Actually there’s a little bit of problem. When we edit the base buffer, we would want to update the parsers in all of its indirect buffers as well, and AFAICT there is no pointer from base buffer to the indirect buffer, only the other way around. 

That's not the problem presented by the OP, though.

> We don’t want indirect buffer and base buffers to share parsers, since they can have different narrowing, and semantically indirect buffers should share anything but the text with the base buffer.

Yes, the parsers should not be shared.

> How about this: we change current_buffer->parser_list from a plain list of parsers to a cons (PARSER-LIST . INDIRECT-PARSER-LIST), where PARSER-LIST is as before. But for base buffers, INDIRECT-PARSER-LIST includes all the parsers of its indirect buffers; and for indirect buffers, INDIRECT-PARSER-LIST is nil.

You can maybe have the indirect buffers in the list, not their parsers.
That could make it easier to access other treesit-related information of the
indirect buffers, if needed.

bug#59693: 29.0.50; treesitter in base buffer doesn't respond to modifications in indirect buffer correctly

[Yuan Fu]

> On Dec 2, 2022, at 12:33 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Yuan Fu <casouri@gmail.com>
>> Date: Thu, 1 Dec 2022 21:05:43 -0800
>> Cc: miha@kamnitnik.top,
>> 59693@debbugs.gnu.org
>> 
>>> In the insdel.c hooks where you record changes to buffer text, you should
>>> see if the buffer has a base_buffer, and if so, update any parsers of the
>>> base buffer as well.
>> 
>> Actually there’s a little bit of problem. When we edit the base buffer, we would want to update the parsers in all of its indirect buffers as well, and AFAICT there is no pointer from base buffer to the indirect buffer, only the other way around. 
> 
> That's not the problem presented by the OP, though.

Yeah, but they are the same problem in spirit, ie, parser not updated when base/indirect buffer receive changes.

> 
>> We don’t want indirect buffer and base buffers to share parsers, since they can have different narrowing, and semantically indirect buffers should share anything but the text with the base buffer.
> 
> Yes, the parsers should not be shared.
> 
>> How about this: we change current_buffer->parser_list from a plain list of parsers to a cons (PARSER-LIST . INDIRECT-PARSER-LIST), where PARSER-LIST is as before. But for base buffers, INDIRECT-PARSER-LIST includes all the parsers of its indirect buffers; and for indirect buffers, INDIRECT-PARSER-LIST is nil.
> 
> You can maybe have the indirect buffers in the list, not their parsers.
> That could make it easier to access other treesit-related information of the
> indirect buffers, if needed.

Good idea, it’s easier to know when to remove the reference with buffers, aka when buffer is killed.

Yuan

bug#59693: 29.0.50; treesitter in base buffer doesn't respond to modifications in indirect buffer correctly

[Yuan Fu]

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



> On Dec 2, 2022, at 5:01 PM, Yuan Fu <casouri@gmail.com> wrote:
> 
> 
> 
>> On Dec 2, 2022, at 12:33 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> 
>>> From: Yuan Fu <casouri@gmail.com>
>>> Date: Thu, 1 Dec 2022 21:05:43 -0800
>>> Cc: miha@kamnitnik.top,
>>> 59693@debbugs.gnu.org
>>> 
>>>> In the insdel.c hooks where you record changes to buffer text, you should
>>>> see if the buffer has a base_buffer, and if so, update any parsers of the
>>>> base buffer as well.
>>> 
>>> Actually there’s a little bit of problem. When we edit the base buffer, we would want to update the parsers in all of its indirect buffers as well, and AFAICT there is no pointer from base buffer to the indirect buffer, only the other way around. 
>> 
>> That's not the problem presented by the OP, though.
> 
> Yeah, but they are the same problem in spirit, ie, parser not updated when base/indirect buffer receive changes.
> 
>> 
>>> We don’t want indirect buffer and base buffers to share parsers, since they can have different narrowing, and semantically indirect buffers should share anything but the text with the base buffer.
>> 
>> Yes, the parsers should not be shared.
>> 
>>> How about this: we change current_buffer->parser_list from a plain list of parsers to a cons (PARSER-LIST . INDIRECT-PARSER-LIST), where PARSER-LIST is as before. But for base buffers, INDIRECT-PARSER-LIST includes all the parsers of its indirect buffers; and for indirect buffers, INDIRECT-PARSER-LIST is nil.
>> 
>> You can maybe have the indirect buffers in the list, not their parsers.
>> That could make it easier to access other treesit-related information of the
>> indirect buffers, if needed.
> 
> Good idea, it’s easier to know when to remove the reference with buffers, aka when buffer is killed.

I now have a patch that fixes this problem. WDYT? I added a new buffer field since it’s cleaner than turning ts_parser_list into a cons, hopefully that’s not frowned upon. 


Yuan


[-- Attachment #2: indirect.patch --]
[-- Type: application/octet-stream, Size: 23043 bytes --]

From cf8b52f5053cc42fac0ddcefc1b2771c5838cb0d Mon Sep 17 00:00:00 2001
From: Yuan Fu <casouri@gmail.com>
Date: Sat, 3 Dec 2022 21:18:31 -0800
Subject: [PATCH 1/3] Refactor treesit_record_change

This is part of the multi-commit change to support indirect
buffers (bug#59693).

Besides moving the code to a new function, I also removed two lines:

-      CHECK_CONS (parser_list);
-      treesit_check_parser (lisp_parser);

because they could signal, and don't really make sense in a internal
function.  If we really want the checks, we could add easserts.

* src/treesit.c (treesit_record_change_1): New function.
(treesit_record_change): Move bulk of code to the new function.
---
 src/treesit.c | 139 ++++++++++++++++++++++++++------------------------
 1 file changed, 72 insertions(+), 67 deletions(-)

diff --git a/src/treesit.c b/src/treesit.c
index 4b150059fac..2ed9c2eafbe 100644
--- a/src/treesit.c
+++ b/src/treesit.c
@@ -690,6 +690,75 @@ treesit_tree_edit_1 (TSTree *tree, ptrdiff_t start_byte,
   ts_tree_edit (tree, &edit);
 }
 
+/* Update each parser in PARSER_LIST.  */
+static inline void
+treesit_record_change_1 (Lisp_Object parser_list, ptrdiff_t start_byte,
+			 ptrdiff_t old_end_byte, ptrdiff_t new_end_byte)
+{
+  FOR_EACH_TAIL_SAFE (parser_list)
+  {
+    Lisp_Object lisp_parser = XCAR (parser_list);
+    TSTree *tree = XTS_PARSER (lisp_parser)->tree;
+    /* See comment (ref:visible-beg-null) if you wonder why we don't
+       update visible_beg/end when tree is NULL.  */
+    if (tree != NULL)
+      {
+	eassert (start_byte <= old_end_byte);
+	eassert (start_byte <= new_end_byte);
+	/* Think the recorded change as a delete followed by an
+	   insert, and think of them as moving unchanged text back
+	   and forth.  After all, the whole point of updating the
+	   tree is to update the position of unchanged text.  */
+	ptrdiff_t visible_beg = XTS_PARSER (lisp_parser)->visible_beg;
+	ptrdiff_t visible_end = XTS_PARSER (lisp_parser)->visible_end;
+	eassert (visible_beg >= 0);
+	eassert (visible_beg <= visible_end);
+
+	/* AFFECTED_START/OLD_END/NEW_END are (0-based) offsets from
+	   VISIBLE_BEG.  min(visi_end, max(visi_beg, value)) clips
+	   value into [visi_beg, visi_end], and subtracting visi_beg
+	   gives the offset from visi_beg.  */
+	ptrdiff_t start_offset = (min (visible_end,
+				       max (visible_beg, start_byte))
+				  - visible_beg);
+	ptrdiff_t old_end_offset = (min (visible_end,
+					 max (visible_beg, old_end_byte))
+				    - visible_beg);
+	ptrdiff_t new_end_offset = (min (visible_end,
+					 max (visible_beg, new_end_byte))
+				    - visible_beg);
+	eassert (start_offset <= old_end_offset);
+	eassert (start_offset <= new_end_offset);
+
+	treesit_tree_edit_1 (tree, start_offset, old_end_offset,
+			     new_end_offset);
+	XTS_PARSER (lisp_parser)->need_reparse = true;
+	XTS_PARSER (lisp_parser)->timestamp++;
+
+	/* VISIBLE_BEG/END records tree-sitter's range of view in
+	   the buffer.  We need to adjust them when tree-sitter's
+	   view changes.  */
+	ptrdiff_t visi_beg_delta;
+	if (old_end_byte > new_end_byte)
+	  /* Move backward.  */
+	  visi_beg_delta = (min (visible_beg, new_end_byte)
+			    - min (visible_beg, old_end_byte));
+	else
+	  /* Move forward.  */
+	  visi_beg_delta = (old_end_byte < visible_beg
+			    ? new_end_byte - old_end_byte : 0);
+	XTS_PARSER (lisp_parser)->visible_beg = visible_beg + visi_beg_delta;
+	XTS_PARSER (lisp_parser)->visible_end = (visible_end
+						 + visi_beg_delta
+						 + (new_end_offset
+						    - old_end_offset));
+	eassert (XTS_PARSER (lisp_parser)->visible_beg >= 0);
+	eassert (XTS_PARSER (lisp_parser)->visible_beg
+		 <= XTS_PARSER (lisp_parser)->visible_end);
+      }
+  }
+}
+
 /* Update each parser's tree after the user made an edit.  This
    function does not parse the buffer and only updates the tree, so it
    should be very fast.  */
@@ -697,74 +766,10 @@ treesit_tree_edit_1 (TSTree *tree, ptrdiff_t start_byte,
 treesit_record_change (ptrdiff_t start_byte, ptrdiff_t old_end_byte,
 		       ptrdiff_t new_end_byte)
 {
-  Lisp_Object parser_list;
+  Lisp_Object parser_list = BVAR (current_buffer, ts_parser_list);
+  treesit_record_change_1 (parser_list, start_byte,
+			   old_end_byte, new_end_byte);
 
-  parser_list = BVAR (current_buffer, ts_parser_list);
-
-  FOR_EACH_TAIL_SAFE (parser_list)
-    {
-      CHECK_CONS (parser_list);
-      Lisp_Object lisp_parser = XCAR (parser_list);
-      treesit_check_parser (lisp_parser);
-      TSTree *tree = XTS_PARSER (lisp_parser)->tree;
-      /* See comment (ref:visible-beg-null) if you wonder why we don't
-      update visible_beg/end when tree is NULL.  */
-      if (tree != NULL)
-	{
-	  eassert (start_byte <= old_end_byte);
-	  eassert (start_byte <= new_end_byte);
-	  /* Think the recorded change as a delete followed by an
-	     insert, and think of them as moving unchanged text back
-	     and forth.  After all, the whole point of updating the
-	     tree is to update the position of unchanged text.  */
-	  ptrdiff_t visible_beg = XTS_PARSER (lisp_parser)->visible_beg;
-	  ptrdiff_t visible_end = XTS_PARSER (lisp_parser)->visible_end;
-	  eassert (visible_beg >= 0);
-	  eassert (visible_beg <= visible_end);
-
-	  /* AFFECTED_START/OLD_END/NEW_END are (0-based) offsets from
-	     VISIBLE_BEG.  min(visi_end, max(visi_beg, value)) clips
-	     value into [visi_beg, visi_end], and subtracting visi_beg
-	     gives the offset from visi_beg.  */
-	  ptrdiff_t start_offset = (min (visible_end,
-					 max (visible_beg, start_byte))
-				    - visible_beg);
-	  ptrdiff_t old_end_offset = (min (visible_end,
-					   max (visible_beg, old_end_byte))
-				      - visible_beg);
-	  ptrdiff_t new_end_offset = (min (visible_end,
-					   max (visible_beg, new_end_byte))
-				      - visible_beg);
-	  eassert (start_offset <= old_end_offset);
-	  eassert (start_offset <= new_end_offset);
-
-	  treesit_tree_edit_1 (tree, start_offset, old_end_offset,
-			       new_end_offset);
-	  XTS_PARSER (lisp_parser)->need_reparse = true;
-	  XTS_PARSER (lisp_parser)->timestamp++;
-
-	  /* VISIBLE_BEG/END records tree-sitter's range of view in
-	     the buffer.  We need to adjust them when tree-sitter's
-	     view changes.  */
-	  ptrdiff_t visi_beg_delta;
-	  if (old_end_byte > new_end_byte)
-	    /* Move backward.  */
-	    visi_beg_delta = (min (visible_beg, new_end_byte)
-			      - min (visible_beg, old_end_byte));
-	  else
-	    /* Move forward.  */
-	    visi_beg_delta = (old_end_byte < visible_beg
-			      ? new_end_byte - old_end_byte : 0);
-	  XTS_PARSER (lisp_parser)->visible_beg = visible_beg + visi_beg_delta;
-	  XTS_PARSER (lisp_parser)->visible_end = (visible_end
-						   + visi_beg_delta
-						   + (new_end_offset
-						      - old_end_offset));
-	  eassert (XTS_PARSER (lisp_parser)->visible_beg >= 0);
-	  eassert (XTS_PARSER (lisp_parser)->visible_beg
-		   <= XTS_PARSER (lisp_parser)->visible_end);
-	}
-    }
 }
 
 /* Comment (ref:visible-beg-null) The purpose of visible_beg/end is to
-- 
2.33.1


From 98623c2a5fa6776d63d341c9975f066d6849fff0 Mon Sep 17 00:00:00 2001
From: Yuan Fu <casouri@gmail.com>
Date: Sat, 3 Dec 2022 21:28:08 -0800
Subject: [PATCH 2/3] Rename ts_parser_list and add treesit_indirect_list

This is part of the multi-commit change to support indirect buffers in
tree-sitter (bug#59693).

ts_parser_list dodged the prefix rename from ts_ to treesit_, so I
took the opportunity to bring it to justice.

* src/buffer.c (bset_ts_parser_list): Rename.
(bset_treesit_indirect_list): New function.
(reset_buffer, init_buffer_once): Initialize treesit_indirect_list.
Rename ts_parser_list.

* src/buffer.h (struct buffer): Add treesit_indirect_list.  Rename
ts_parser_list.

* src/treesit.c (treesit_record_change_1)
(Ftreesit_parser_create)
(Ftreesit_parser_delete)
(Ftreesit_parser_list): Rename ts_parser_list.
---
 src/buffer.c  | 18 +++++++++++++-----
 src/buffer.h  |  7 ++++++-
 src/treesit.c | 13 +++++++------
 3 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index 71be7ed9e13..d7dd2c3a2d8 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -233,9 +233,14 @@ bset_extra_line_spacing (struct buffer *b, Lisp_Object val)
 }
 #ifdef HAVE_TREE_SITTER
 static void
-bset_ts_parser_list (struct buffer *b, Lisp_Object val)
+bset_treesit_parser_list (struct buffer *b, Lisp_Object val)
 {
-  b->ts_parser_list_ = val;
+  b->treesit_parser_list_ = val;
+}
+static void
+bset_treesit_indirect_list (struct buffer *b, Lisp_Object val)
+{
+  b->treesit_indirect_list_ = val;
 }
 #endif
 static void
@@ -1066,7 +1071,8 @@ reset_buffer (register struct buffer *b)
   bset_cursor_type (b, BVAR (&buffer_defaults, cursor_type));
   bset_extra_line_spacing (b, BVAR (&buffer_defaults, extra_line_spacing));
 #ifdef HAVE_TREE_SITTER
-  bset_ts_parser_list (b, Qnil);
+  bset_treesit_parser_list (b, Qnil);
+  bset_treesit_indirect_list (b, Qnil);
 #endif
 
   b->display_error_modiff = 0;
@@ -4692,7 +4698,8 @@ init_buffer_once (void)
   XSETFASTINT (BVAR (&buffer_local_flags, cursor_type), idx); ++idx;
   XSETFASTINT (BVAR (&buffer_local_flags, extra_line_spacing), idx); ++idx;
 #ifdef HAVE_TREE_SITTER
-  XSETFASTINT (BVAR (&buffer_local_flags, ts_parser_list), idx); ++idx;
+  XSETFASTINT (BVAR (&buffer_local_flags, treesit_parser_list), idx); ++idx;
+  XSETFASTINT (BVAR (&buffer_local_flags, treesit_indirect_list), idx); ++idx;
 #endif
   XSETFASTINT (BVAR (&buffer_local_flags, cursor_in_non_selected_windows), idx); ++idx;
 
@@ -4763,7 +4770,8 @@ init_buffer_once (void)
   bset_cursor_type (&buffer_defaults, Qt);
   bset_extra_line_spacing (&buffer_defaults, Qnil);
 #ifdef HAVE_TREE_SITTER
-  bset_ts_parser_list (&buffer_defaults, Qnil);
+  bset_treesit_parser_list (&buffer_defaults, Qnil);
+  bset_treesit_indirect_list (&buffer_defaults, Qnil);
 #endif
   bset_cursor_in_non_selected_windows (&buffer_defaults, Qt);
 
diff --git a/src/buffer.h b/src/buffer.h
index dded0cd98c1..984c5ee4437 100644
--- a/src/buffer.h
+++ b/src/buffer.h
@@ -575,7 +575,12 @@ #define BVAR(buf, field) ((buf)->field ## _)
 
 #ifdef HAVE_TREE_SITTER
   /* A list of tree-sitter parsers for this buffer.  */
-  Lisp_Object ts_parser_list_;
+  Lisp_Object treesit_parser_list_;
+  /* A list of indirect buffers of this buffer whose tree-sitter
+     parsers wants to be updated with buffer content changes.  This
+     list does not necessarily include all indirect buffers of this
+     buffer, and buffers in this list are not necessarily alive.  */
+  Lisp_Object treesit_indirect_list_;
 #endif
   /* Cursor type to display in non-selected windows.
      t means to use hollow box cursor.
diff --git a/src/treesit.c b/src/treesit.c
index 2ed9c2eafbe..9a076be67c5 100644
--- a/src/treesit.c
+++ b/src/treesit.c
@@ -766,7 +766,7 @@ treesit_record_change_1 (Lisp_Object parser_list, ptrdiff_t start_byte,
 treesit_record_change (ptrdiff_t start_byte, ptrdiff_t old_end_byte,
 		       ptrdiff_t new_end_byte)
 {
-  Lisp_Object parser_list = BVAR (current_buffer, ts_parser_list);
+  Lisp_Object parser_list = BVAR (current_buffer, treesit_parser_list);
   treesit_record_change_1 (parser_list, start_byte,
 			   old_end_byte, new_end_byte);
 
@@ -1279,7 +1279,7 @@ DEFUN ("treesit-parser-create",
   treesit_check_buffer_size (buf);
 
   /* See if we can reuse a parser.  */
-  for (Lisp_Object tail = BVAR (buf, ts_parser_list);
+  for (Lisp_Object tail = BVAR (buf, treesit_parser_list);
        NILP (no_reuse) && !NILP (tail);
        tail = XCDR (tail))
     {
@@ -1306,7 +1306,8 @@ DEFUN ("treesit-parser-create",
 						 language);
 
   /* Update parser-list.  */
-  BVAR (buf, ts_parser_list) = Fcons (lisp_parser, BVAR (buf, ts_parser_list));
+  Lisp_Object buffer_list = BVAR (buf, treesit_parser_list);
+  BVAR (buf, treesit_parser_list) = Fcons (lisp_parser, buffer_list);
 
   return lisp_parser;
 }
@@ -1323,8 +1324,8 @@ DEFUN ("treesit-parser-delete",
   Lisp_Object buffer = XTS_PARSER (parser)->buffer;
   struct buffer *buf = XBUFFER (buffer);
 
-  BVAR (buf, ts_parser_list)
-    = Fdelete (parser, BVAR (buf, ts_parser_list));
+  BVAR (buf, treesit_parser_list)
+    = Fdelete (parser, BVAR (buf, treesit_parser_list));
 
   XTS_PARSER (parser)->deleted = true;
   return Qnil;
@@ -1350,7 +1351,7 @@ DEFUN ("treesit-parser-list",
   Lisp_Object return_list = Qnil;
   Lisp_Object tail;
 
-  tail = BVAR (buf, ts_parser_list);
+  tail = BVAR (buf, treesit_parser_list);
 
   FOR_EACH_TAIL (tail)
     return_list = Fcons (XCAR (tail), return_list);
-- 
2.33.1


From 6f338f347f737531af8b29776f3745e653710f12 Mon Sep 17 00:00:00 2001
From: Yuan Fu <casouri@gmail.com>
Date: Sat, 3 Dec 2022 22:55:22 -0800
Subject: [PATCH 3/3] Update tree-sitter parser on both base and indirect
 buffers

This is part of the multi-commit change to support indirect buffers in
tree-sitter (bug#59693).

In this commit we finally share the buffer change across all base and
indirect buffers.  I also fixed a bug in Ftreesit_parser_create where
we unconditionally use the current buffer when creating the parser,
regardless of the value of the BUFFER parameter.

* src/treesit.c (treesit_reap_indirect_buffers): New function.
(treesit_record_change): Update all parsers in both the base buffer
and its indirect buffers.
(Ftreesit_parser_create): Add this buffer to its base buffer's
treesit_indirect_list.
(Ftreesit__indirect_buffer_list): New function.

* test/src/treesit-tests.el (treesit-indirect-buffers): New test.
---
 src/treesit.c             | 90 +++++++++++++++++++++++++++++++++++++--
 test/src/treesit-tests.el | 63 +++++++++++++++++++++++++++
 2 files changed, 149 insertions(+), 4 deletions(-)

diff --git a/src/treesit.c b/src/treesit.c
index 9a076be67c5..c6ee0c3322f 100644
--- a/src/treesit.c
+++ b/src/treesit.c
@@ -690,6 +690,27 @@ treesit_tree_edit_1 (TSTree *tree, ptrdiff_t start_byte,
   ts_tree_edit (tree, &edit);
 }
 
+/* Reap deleted buffers in the treesit_parser_list of BASE_BUFFER.  */
+static inline void
+treesit_reap_indirect_buffers (struct buffer *base_buffer)
+{
+  eassert (base_buffer->base_buffer == 0);
+  Lisp_Object buffer_list = BVAR (base_buffer, treesit_indirect_list);
+  Lisp_Object new_buffer_list = Qnil;
+
+  /* Go over the current buffer list and build a new list with only
+     live buffers, and replace the old list with the new list.  */
+  FOR_EACH_TAIL_SAFE (buffer_list)
+  {
+    Lisp_Object the_buffer = XCAR (buffer_list);
+    eassert (BUFFERP (the_buffer));
+    if (BUFFER_LIVE_P (XBUFFER (the_buffer)))
+      new_buffer_list = Fcons (the_buffer, new_buffer_list);
+  }
+  new_buffer_list = Fnreverse (new_buffer_list);
+  BVAR (base_buffer, treesit_indirect_list) = new_buffer_list;
+}
+
 /* Update each parser in PARSER_LIST.  */
 static inline void
 treesit_record_change_1 (Lisp_Object parser_list, ptrdiff_t start_byte,
@@ -766,10 +787,37 @@ treesit_record_change_1 (Lisp_Object parser_list, ptrdiff_t start_byte,
 treesit_record_change (ptrdiff_t start_byte, ptrdiff_t old_end_byte,
 		       ptrdiff_t new_end_byte)
 {
-  Lisp_Object parser_list = BVAR (current_buffer, treesit_parser_list);
+  /* We always act on the base buffer.  If this buffer is an indirect
+     buffer, switch to its base buffer.  */
+  struct buffer *base_buffer = current_buffer;
+  if (current_buffer->base_buffer)
+    base_buffer = current_buffer->base_buffer;
+
+  /* First update base buffer's parsers.  */
+  Lisp_Object parser_list = BVAR (base_buffer, treesit_parser_list);
   treesit_record_change_1 (parser_list, start_byte,
 			   old_end_byte, new_end_byte);
 
+  /* Then update parsers of indirect buffers.  */
+  Lisp_Object buffer_list = BVAR (base_buffer, treesit_indirect_list);
+  bool need_reap = false;
+  FOR_EACH_TAIL_SAFE (buffer_list)
+  {
+    struct buffer *buffer = XBUFFER (XCAR (buffer_list));
+    if (BUFFER_LIVE_P (buffer))
+      {
+	Lisp_Object parser_list = BVAR (buffer, treesit_parser_list);
+	treesit_record_change_1 (parser_list, start_byte,
+				 old_end_byte, new_end_byte);
+      }
+    else
+      need_reap = true;
+  }
+
+  /* If we encountered deleted indirect buffer above, remove it from
+     the indirect list.  */
+  if (need_reap)
+    treesit_reap_indirect_buffers (base_buffer);
 }
 
 /* Comment (ref:visible-beg-null) The purpose of visible_beg/end is to
@@ -1270,7 +1318,10 @@ DEFUN ("treesit-parser-create",
   CHECK_SYMBOL (language);
   struct buffer *buf;
   if (NILP (buffer))
-    buf = current_buffer;
+    {
+      buf = current_buffer;
+      XSETBUFFER (buffer, current_buffer);
+    }
   else
     {
       CHECK_BUFFER (buffer);
@@ -1301,14 +1352,26 @@ DEFUN ("treesit-parser-create",
   ts_parser_set_language (parser, lang);
 
   /* Create parser.  */
-  Lisp_Object lisp_parser = make_treesit_parser (Fcurrent_buffer (),
-						 parser, NULL,
+  Lisp_Object lisp_parser = make_treesit_parser (buffer, parser, NULL,
 						 language);
 
   /* Update parser-list.  */
   Lisp_Object buffer_list = BVAR (buf, treesit_parser_list);
   BVAR (buf, treesit_parser_list) = Fcons (lisp_parser, buffer_list);
 
+  /* If this buffer is an indirect buffer, add this buffer to its base
+     buffer's treesit_indirect_list.  */
+  if (buf->base_buffer)
+    {
+      struct buffer *base_buffer = buf->base_buffer;
+      Lisp_Object indirect_list = BVAR (base_buffer, treesit_indirect_list);
+      if (NILP (Fmemq (buffer, indirect_list)))
+	{
+	  indirect_list = Fcons (buffer, indirect_list);
+	  BVAR (base_buffer, treesit_indirect_list) = indirect_list;
+	}
+    }
+
   return lisp_parser;
 }
 
@@ -1382,6 +1445,24 @@ DEFUN ("treesit-parser-language",
   return XTS_PARSER (parser)->language_symbol;
 }
 
+DEFUN ("treesit--indirect-buffer-list",
+       Ftreesit__indirect_buffer_list, Streesit__indirect_buffer_list,
+       0, 1, 0,
+       doc: /* This INTERNAL function is used for testing only.
+It does NOT return all the indirect buffers of BUFFER.  */)
+  (Lisp_Object buffer)
+{
+  CHECK_BUFFER (buffer);
+  struct buffer *buf = XBUFFER (buffer);
+  Lisp_Object return_list = Qnil;
+  Lisp_Object tail = BVAR (buf, treesit_indirect_list);
+
+  FOR_EACH_TAIL (tail)
+    return_list = Fcons (XCAR (tail), return_list);
+
+  return Freverse (return_list);
+}
+
 /*** Parser API */
 
 DEFUN ("treesit-parser-root-node",
@@ -3105,6 +3186,7 @@ syms_of_treesit (void)
   defsubr (&Streesit_parser_list);
   defsubr (&Streesit_parser_buffer);
   defsubr (&Streesit_parser_language);
+  defsubr (&Streesit__indirect_buffer_list);
 
   defsubr (&Streesit_parser_root_node);
   /* defsubr (&Streesit_parse_string); */
diff --git a/test/src/treesit-tests.el b/test/src/treesit-tests.el
index 80fde408cd3..bc8502531dc 100644
--- a/test/src/treesit-tests.el
+++ b/test/src/treesit-tests.el
@@ -31,6 +31,7 @@
 (declare-function treesit-parser-create "treesit.c")
 (declare-function treesit-parser-delete "treesit.c")
 (declare-function treesit-parser-list "treesit.c")
+(declare-function treesit--indirect-buffer-list "treesit.c")
 (declare-function treesit-parser-buffer "treesit.c")
 (declare-function treesit-parser-language "treesit.c")
 
@@ -156,6 +157,68 @@ treesit-node-api
       (should (treesit-node-eq root-node root-node))
       (should (not (treesit-node-eq root-node doc-node))))))
 
+(ert-deftest treesit-indirect-buffers ()
+  "Test basic parsing routines."
+  (skip-unless (treesit-language-available-p 'json))
+  (let* ((base-buffer (get-buffer-create "*treesit test*"))
+         (indirect-1 (make-indirect-buffer base-buffer "*treesit test 1*"))
+         (indirect-2 (make-indirect-buffer base-buffer "*treesit test 2*")))
+    (unwind-protect
+        (progn
+          (treesit-parser-create 'json base-buffer)
+          (treesit-parser-create 'json indirect-1)
+          (treesit-parser-create 'json indirect-2)
+          ;; 1. Creating parsers in indirect buffers should add them
+          ;; to the base buffer's indirect list.
+          (should (equal (treesit--indirect-buffer-list base-buffer)
+                         (list indirect-2 indirect-1)))
+          ;; 2. Creating additional parsers in the indirect buffer
+          ;; should not add duplicate indirect buffers to the base
+          ;; buffer's indirect list.
+          (treesit-parser-create 'json indirect-2 t)
+          (should (equal (treesit--indirect-buffer-list base-buffer)
+                         (list indirect-2 indirect-1)))
+          ;; 3. Edit the base buffer...
+          (with-current-buffer base-buffer
+            (insert "[1,2,3]"))
+          ;; ...and parsers in the indirect buffer should be updated too.
+          (with-current-buffer indirect-1
+            (should (eq (treesit-node-end (treesit-buffer-root-node))
+                        8))
+            ;; 4. Edit the indirect buffer...
+            (goto-char 2)
+            (insert "0,"))
+          ;; ...and parsers in the base and other indirect buffers should be
+          ;; updated too.
+          (with-current-buffer base-buffer
+            (should (eq (treesit-node-end (treesit-buffer-root-node))
+                        10)))
+          (with-current-buffer indirect-2
+            (should (eq (treesit-node-end (treesit-buffer-root-node))
+                        10)))
+          ;; 5. Kill an indirect buffer, editing the base buffer and the
+          ;; other indirect buffer should be fine.
+          (kill-buffer indirect-1)
+          (with-current-buffer base-buffer
+            (delete-region (point-min) (point-max)))
+          (with-current-buffer indirect-2
+            (insert "[1,2,3]"))
+          (with-current-buffer base-buffer
+            (should (eq (treesit-node-end (treesit-buffer-root-node))
+                        8)))
+          (with-current-buffer indirect-2
+            (should (eq (treesit-node-end (treesit-buffer-root-node))
+                        8)))
+          ;; 6. The killed indirect buffer should be removed from the
+          ;; base buffer's indirect list.
+          (should (equal (treesit--indirect-buffer-list base-buffer)
+                         (list indirect-2)))
+          (kill-buffer indirect-2)
+          (kill-buffer base-buffer))
+      (kill-buffer base-buffer)
+      (kill-buffer base-buffer)
+      (kill-buffer base-buffer))))
+
 (ert-deftest treesit-query-api ()
   "Tests for query API."
   (skip-unless (treesit-language-available-p 'json))
-- 
2.33.1

bug#59693: 29.0.50; treesitter in base buffer doesn't respond to modifications in indirect buffer correctly

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Sat, 3 Dec 2022 23:20:59 -0800
> Cc: miha@kamnitnik.top,
>  59693@debbugs.gnu.org
> 
> >>> We don’t want indirect buffer and base buffers to share parsers, since they can have different narrowing, and semantically indirect buffers should share anything but the text with the base buffer.
> >> 
> >> Yes, the parsers should not be shared.
> >> 
> >>> How about this: we change current_buffer->parser_list from a plain list of parsers to a cons (PARSER-LIST . INDIRECT-PARSER-LIST), where PARSER-LIST is as before. But for base buffers, INDIRECT-PARSER-LIST includes all the parsers of its indirect buffers; and for indirect buffers, INDIRECT-PARSER-LIST is nil.
> >> 
> >> You can maybe have the indirect buffers in the list, not their parsers.
> >> That could make it easier to access other treesit-related information of the
> >> indirect buffers, if needed.
> > 
> > Good idea, it’s easier to know when to remove the reference with buffers, aka when buffer is killed.
> 
> I now have a patch that fixes this problem. WDYT? I added a new buffer field since it’s cleaner than turning ts_parser_list into a cons, hopefully that’s not frowned upon. 

Thanks.

If we are adding to the buffer object a field that holds the list of
indirect buffers, then:

  . the name of the field should not include "treesit" in it, and it
    shouldn't be conditioned on HAVE_TREE_SITTER
  . I wonder if a flat list is a good idea, i.e. scalable enough? also,
    treesit_reap_indirect_buffers conses a lot as result
  . I vaguely remember that adding built-in fields to the buffer object had
    some caveats, but I don't recall the details (did you bootstrap?)

Stefan, any comments on this?  Are there better ideas of how to track buffer
text changes in indirect buffers?

bug#59693: 29.0.50; treesitter in base buffer doesn't respond to modifications in indirect buffer correctly

[Yuan Fu]

> On Dec 3, 2022, at 11:46 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Yuan Fu <casouri@gmail.com>
>> Date: Sat, 3 Dec 2022 23:20:59 -0800
>> Cc: miha@kamnitnik.top,
>> 59693@debbugs.gnu.org
>> 
>>>>> We don’t want indirect buffer and base buffers to share parsers, since they can have different narrowing, and semantically indirect buffers should share anything but the text with the base buffer.
>>>> 
>>>> Yes, the parsers should not be shared.
>>>> 
>>>>> How about this: we change current_buffer->parser_list from a plain list of parsers to a cons (PARSER-LIST . INDIRECT-PARSER-LIST), where PARSER-LIST is as before. But for base buffers, INDIRECT-PARSER-LIST includes all the parsers of its indirect buffers; and for indirect buffers, INDIRECT-PARSER-LIST is nil.
>>>> 
>>>> You can maybe have the indirect buffers in the list, not their parsers.
>>>> That could make it easier to access other treesit-related information of the
>>>> indirect buffers, if needed.
>>> 
>>> Good idea, it’s easier to know when to remove the reference with buffers, aka when buffer is killed.
>> 
>> I now have a patch that fixes this problem. WDYT? I added a new buffer field since it’s cleaner than turning ts_parser_list into a cons, hopefully that’s not frowned upon. 
> 
> Thanks.
> 
> If we are adding to the buffer object a field that holds the list of
> indirect buffers, then:
> 
>  . the name of the field should not include "treesit" in it, and it
>    shouldn't be conditioned on HAVE_TREE_SITTER

I made it tree-sitter specific and stated that it doesn’t necessarily contain all indirect buffers to simply the implementation. Right now new indirect buffer are added to the list only when a parser is created in an indirect buffer. And dead indirect buffers are discarded only when treesit_record_change runs. Do we really need a proper indirect buffer list? After all, no one complained about its absence ever since indirect buffer were added.

>  . I wonder if a flat list is a good idea, i.e. scalable enough? also,
>    treesit_reap_indirect_buffers conses a lot as result

AFAIK in most cases only a handful of indirect buffer are created, so I didn’t worry about that. For tree-sitter’s case, we need to iterate through every indirect buffer anyway so that’s always O(n). Adding and removing a buffer from the list is O(n) but they are done infrequently. The add operation is called every time a parser is created, and the remove operation is called once when an indirect buffer is killed. 

We could use a hash table, but I doubt if that’s necessary, at least while the indirect buffer list is only used for tree-sitter. For tree-sitter, the common path is when we update each parser of buffer changes, which is always O(n) regardless of the data structure. 

>  . I vaguely remember that adding built-in fields to the buffer object had
>    some caveats, but I don't recall the details (did you bootstrap?)

I built from git clean, so yeah.

> 
> Stefan, any comments on this?  Are there better ideas of how to track buffer
> text changes in indirect buffers?

Specifically, when any one of the base and indirect buffers change, we want all parsers in all base and indirect buffers to be updated.

Yuan

bug#59693: 29.0.50; treesitter in base buffer doesn't respond to modifications in indirect buffer correctly

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Actually there’s a little bit of problem. When we edit the base buffer, we
> would want to update the parsers in all of its indirect buffers as well, and
> AFAICT there is no pointer from base buffer to the indirect buffer, only the
> other way around. 

Background note: I consider indirect buffers an attractive nuisance
and for this reason I think we should spend as little time as possible
improving them.  Instead we should encourage people to develop
alternative approaches.
I know we have a large number of bugs lurking in that area.

E.g.:

    Emacs -Q
    M-x clone-indirect-buffer RET
    M-x fundamental-mode RET

we see that `*scratch*` has lost its font-lock highlighting even though we
changed major mode in the "other" buffer.
Now go to `*scratch*` type some text: you see that font-lock properly
updates the new code's highlighting.
Then go to `*scratch*<2>` and type some more text: you should see that
font-lock does *not* properly update the new code's highlighting in the
base buffer.

AFAICT this is the exact same bug as what you're seeing, just with
font-lock instead of tree-sitter.  [ Of course, for font-lock it's worse
because font-lock uses text-properties (which are shared between the
base and the its indirect buffers) so it simply can't do its job
properly in indirect buffers.  ]

This bug has been with us since indirect buffers were invented, so
it has very low priority, AFAIC.

> Then base buffer can update all indirect buffers’ parsers, and indirect
> buffer can find its base buffer and update all the parsers, including the
> base buffer’s parsers and other indirect buffers’ parsers.

Don't bother, please.
Instead, I recommend you disallow the use of tree sitter in indirect
buffers.  And we should probably try and change `insdel.c` so it always
runs the `after/before-change-functions` in the base buffer (this should
fix the worst part of the problems).


        Stefan

bug#59693: 29.0.50; treesitter in base buffer doesn't respond to modifications in indirect buffer correctly

[Eli Zaretskii]

On December 5, 2022 5:49:14 AM GMT+02:00, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
> Don't bother, please.
> Instead, I recommend you disallow the use of tree sitter in indirect
> buffers.  And we should probably try and change `insdel.c` so it always
> runs the `after/before-change-functions` in the base buffer (this should
> fix the worst part of the problems).


Will this "hold water" vis-a-vis the expectations of various features that would like to use tree sitter related capabilities?  Regardless of your opinion on indirect buffers, many 3rd party packages and features use indirect buffers as the backbone of their implementation.  We don't currently have any alternatives to that, AFAIK.  So I'd expect a lot of disappointment if we declare that indirect buffers will not be supported by tree sitter as a matter of design decision.

Changing insdel.c to run stuff in base buffer could be a solution, but I don't feel we can make such changes on the release branch.  But maybe we can do that now only for treesit.c functions.  Yuan, would tgat solve this particular problem?

bug#59693: 29.0.50; treesitter in base buffer doesn't respond to modifications in indirect buffer correctly

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Will this "hold water" vis-a-vis the expectations of various features that
> would like to use tree sitter related capabilities?  Regardless of your
> opinion on indirect buffers, many 3rd party packages and features use
> indirect buffers as the backbone of their implementation.  We don't
> currently have any alternatives to that, AFAIK.  So I'd expect a lot of
> disappointment if we declare that indirect buffers will not be supported by
> tree sitter as a matter of design decision.

I can't see why: a package that uses indirect buffers can just as well
run its tree-sitter parsers in the base buffer.

> Changing insdel.c to run stuff in base buffer could be a solution, but
> I don't feel we can make such changes on the release branch.

Agreed.

> But maybe we can do that now only for treesit.c functions.

Sounds OK to me, yes.


        Stefan

bug#59693: 29.0.50; treesitter in base buffer doesn't respond to modifications in indirect buffer correctly

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Yuan Fu <casouri@gmail.com>,  59693@debbugs.gnu.org,  miha@kamnitnik.top
> Date: Mon, 05 Dec 2022 10:29:06 -0500
> 
> > Will this "hold water" vis-a-vis the expectations of various features that
> > would like to use tree sitter related capabilities?  Regardless of your
> > opinion on indirect buffers, many 3rd party packages and features use
> > indirect buffers as the backbone of their implementation.  We don't
> > currently have any alternatives to that, AFAIK.  So I'd expect a lot of
> > disappointment if we declare that indirect buffers will not be supported by
> > tree sitter as a matter of design decision.
> 
> I can't see why: a package that uses indirect buffers can just as well
> run its tree-sitter parsers in the base buffer.

I thought about those multi-mode modes, or features that show the same
buffer more than once with different modes.

> > Changing insdel.c to run stuff in base buffer could be a solution, but
> > I don't feel we can make such changes on the release branch.
> 
> Agreed.
> 
> > But maybe we can do that now only for treesit.c functions.
> 
> Sounds OK to me, yes.

OK, thanks.  Yuan, will this work for you? can you show a patch along these
lines?

bug#59693: 29.0.50; treesitter in base buffer doesn't respond to modifications in indirect buffer correctly

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> I can't see why: a package that uses indirect buffers can just as well
>> run its tree-sitter parsers in the base buffer.
> I thought about those multi-mode modes, or features that show the same
> buffer more than once with different modes.

Yes, that's exactly the case I'm thinking about: these packages should
have no trouble running all their parsers in the base buffer (even if
the rest of their work is done in indirect buffers).


        Stefan

bug#59693: 29.0.50; treesitter in base buffer doesn't respond to modifications in indirect buffer correctly

[Yuan Fu]

> On Dec 5, 2022, at 7:44 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: Yuan Fu <casouri@gmail.com>,  59693@debbugs.gnu.org,  miha@kamnitnik.top
>> Date: Mon, 05 Dec 2022 10:29:06 -0500
>> 
>>> Will this "hold water" vis-a-vis the expectations of various features that
>>> would like to use tree sitter related capabilities?  Regardless of your
>>> opinion on indirect buffers, many 3rd party packages and features use
>>> indirect buffers as the backbone of their implementation.  We don't
>>> currently have any alternatives to that, AFAIK.  So I'd expect a lot of
>>> disappointment if we declare that indirect buffers will not be supported by
>>> tree sitter as a matter of design decision.
>> 
>> I can't see why: a package that uses indirect buffers can just as well
>> run its tree-sitter parsers in the base buffer.
> 
> I thought about those multi-mode modes, or features that show the same
> buffer more than once with different modes.
> 
>>> Changing insdel.c to run stuff in base buffer could be a solution, but
>>> I don't feel we can make such changes on the release branch.
>> 
>> Agreed.
>> 
>>> But maybe we can do that now only for treesit.c functions.
>> 
>> Sounds OK to me, yes.
> 
> OK, thanks.  Yuan, will this work for you? can you show a patch along these
> lines?

That wouldn’t work, unfortunately. We need to update all the parsers in every indirect and base buffer, enforce all changes to happen in the base buffer doesn’t help with that: indirect buffers’ parsers would be out-of-sync.

I can think of two ways:

1. Only allow base buffer to have parsers, no change is needed for insdel.c, treesit_record_change can find the base buffer and update its parsers. We can ask indirect buffers to use their base buffer’s parser. Unless the base buffer is narrowed, I think it will work fine. 

I remember that there were a discussion along the lines of user-narrow vs low-level narrow, what was the outcome of that discussion?

2. The patch I sent earlier, which tracks indirect buffers so all the relevant buffer’s parser are updated at a buffer content change.

Yuan

bug#59693: 29.0.50; treesitter in base buffer doesn't respond to modifications in indirect buffer correctly

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 1. Only allow base buffer to have parsers, no change is needed for insdel.c,

That's the option I recommend, and I that's what I understood Eli was
agreeing we do.

> treesit_record_change can find the base buffer and update its parsers. We
> can ask indirect buffers to use their base buffer’s parser. Unless the base
> buffer is narrowed, I think it will work fine. 

Exactly.  Code that uses this can `widen` as needed as well.


        Stefan

bug#59693: 29.0.50; treesitter in base buffer doesn't respond to modifications in indirect buffer correctly

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Mon, 5 Dec 2022 18:15:07 -0800
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,
>  59693@debbugs.gnu.org,
>  miha@kamnitnik.top
> 
> 1. Only allow base buffer to have parsers, no change is needed for insdel.c, treesit_record_change can find the base buffer and update its parsers. We can ask indirect buffers to use their base buffer’s parser. Unless the base buffer is narrowed, I think it will work fine. 

I think this is fine, but we need to document it.

> I remember that there were a discussion along the lines of user-narrow vs low-level narrow, what was the outcome of that discussion?

Nothing in particular, and I don't think it's relevant.  If some mode needs
to widen, it can.

Thanks.

bug#59693: 29.0.50; treesitter in base buffer doesn't respond to modifications in indirect buffer correctly

[Yuan Fu]

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



> On Dec 6, 2022, at 4:17 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Yuan Fu <casouri@gmail.com>
>> Date: Mon, 5 Dec 2022 18:15:07 -0800
>> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,
>> 59693@debbugs.gnu.org,
>> miha@kamnitnik.top
>> 
>> 1. Only allow base buffer to have parsers, no change is needed for insdel.c, treesit_record_change can find the base buffer and update its parsers. We can ask indirect buffers to use their base buffer’s parser. Unless the base buffer is narrowed, I think it will work fine. 
> 
> I think this is fine, but we need to document it.
> 
>> I remember that there were a discussion along the lines of user-narrow vs low-level narrow, what was the outcome of that discussion?
> 
> Nothing in particular, and I don't think it's relevant.  If some mode needs
> to widen, it can.
> 
> Thanks.

Here is a patch that does #1.

Yuan


[-- Attachment #2: indirect.patch --]
[-- Type: application/octet-stream, Size: 8169 bytes --]

From 92f3fe1fe6cb021b24247b518fbe852592c0852c Mon Sep 17 00:00:00 2001
From: Yuan Fu <casouri@gmail.com>
Date: Wed, 7 Dec 2022 14:50:16 -0800
Subject: [PATCH] Make indirect buffers use tree-sitter parsers in their base
 buffer

* src/treesit.c (treesit_record_change): Always use the base buffer.
(Ftreesit_parser_create): Always use the base buffer.  Also change the
for loop into FOR_EACH_TAIL (stylistic change).
(Ftreesit_parser_list): Always use the base buffer.

* doc/lispref/parsing.texi (Using Parser): Update manual.
* test/src/treesit-tests.el (treesit-indirect-buffer): New test.
---
 doc/lispref/parsing.texi  | 12 ++++++++-
 src/treesit.c             | 56 +++++++++++++++++++++++++++------------
 test/src/treesit-tests.el | 34 ++++++++++++++++++++++++
 3 files changed, 84 insertions(+), 18 deletions(-)

diff --git a/doc/lispref/parsing.texi b/doc/lispref/parsing.texi
index 3223875320a..98582227fd7 100644
--- a/doc/lispref/parsing.texi
+++ b/doc/lispref/parsing.texi
@@ -409,6 +409,14 @@ Using Parser
 By default, this function reuses a parser if one already exists for
 @var{language} in @var{buffer}, but if @var{no-reuse} is
 non-@code{nil}, this function always creates a new parser.
+
+If @var{buffer} (or the current buffer) is an indirect buffer, its
+base buffer is used instead.  That is, indirect buffers uses their
+base buffer's parsers.  If the base buffer is narrowed, an indirect
+buffer might not be able to retrieve information of the portion of the
+buffer text that are invisible in the base buffer.  Lisp programs
+should widen as necessary should they want to use a parser in an
+indirect buffer.
 @end defun
 
 Given a parser, we can query information about it.
@@ -447,7 +455,9 @@ Using Parser
 @defun treesit-parser-list &optional buffer
 This function returns the parser list of @var{buffer}.  If
 @var{buffer} is @code{nil} or omitted, it defaults to the current
-buffer.
+buffer.  If @var{buffer} (or the current buffer) is an indirect
+buffer, its base buffer is used instead.  That is, indirect buffers
+uses their base buffer's parsers.
 @end defun
 
 @defun treesit-parser-delete parser
diff --git a/src/treesit.c b/src/treesit.c
index 8b485ca4ece..74370cd772d 100644
--- a/src/treesit.c
+++ b/src/treesit.c
@@ -384,7 +384,10 @@ #define ts_tree_root_node fn_ts_tree_root_node
    mysteriously drops.  3) what if a user uses so many stuff that the
    default cache size (20) is not enough and we end up thrashing?
    These are all imaginary scenarios but they are not impossible
-   :-) */
+   :-)
+
+   Parsers in indirect buffers: We make indirect buffers to share the
+   parser of its base buffer.  See bug#59693 for reasoning.  */
 
 \f
 /*** Initialization */
@@ -697,9 +700,10 @@ treesit_tree_edit_1 (TSTree *tree, ptrdiff_t start_byte,
 treesit_record_change (ptrdiff_t start_byte, ptrdiff_t old_end_byte,
 		       ptrdiff_t new_end_byte)
 {
-  Lisp_Object parser_list;
-
-  parser_list = BVAR (current_buffer, ts_parser_list);
+  struct buffer *base_buffer = current_buffer;
+  if (current_buffer->base_buffer)
+    base_buffer = current_buffer->base_buffer;
+  Lisp_Object parser_list = BVAR (base_buffer, ts_parser_list);
 
   FOR_EACH_TAIL_SAFE (parser_list)
     {
@@ -1252,12 +1256,19 @@ DEFUN ("treesit-parser-create",
        1, 3, 0,
        doc: /* Create and return a parser in BUFFER for LANGUAGE.
 
-The parser is automatically added to BUFFER's parser list, as
-returned by `treesit-parser-list'.
-LANGUAGE is a language symbol.  If BUFFER is nil or omitted, it
-defaults to the current buffer.  If BUFFER already has a parser for
-LANGUAGE, return that parser, but if NO-REUSE is non-nil, always
-create a new parser.  */)
+The parser is automatically added to BUFFER's parser list, as returned
+by `treesit-parser-list'.  LANGUAGE is a language symbol.  If BUFFER
+is nil or omitted, it defaults to the current buffer.  If BUFFER
+already has a parser for LANGUAGE, return that parser, but if NO-REUSE
+is non-nil, always create a new parser.
+
+If BUFFER (or the current buffer) is an indirect buffer, its base
+buffer is used instead.  That is, indirect buffers uses their base
+buffer's parsers.  If the base buffer is narrowed, an indirect buffer
+might not be able to retrieve information of the portion of the buffer
+text that are invisible in the base buffer.  Lisp programs should
+widen as necessary should they want to use a parser in an indirect
+buffer.  */)
   (Lisp_Object language, Lisp_Object buffer, Lisp_Object no_reuse)
 {
   treesit_initialize ();
@@ -1271,16 +1282,21 @@ DEFUN ("treesit-parser-create",
       CHECK_BUFFER (buffer);
       buf = XBUFFER (buffer);
     }
+  if (buf->base_buffer)
+    buf = buf->base_buffer;
+
   treesit_check_buffer_size (buf);
 
   /* See if we can reuse a parser.  */
-  for (Lisp_Object tail = BVAR (buf, ts_parser_list);
-       NILP (no_reuse) && !NILP (tail);
-       tail = XCDR (tail))
+  if (NILP (no_reuse))
     {
-      struct Lisp_TS_Parser *parser = XTS_PARSER (XCAR (tail));
-      if (EQ (parser->language_symbol, language))
-	return XCAR (tail);
+      Lisp_Object tail = BVAR (buf, ts_parser_list);
+      FOR_EACH_TAIL (tail)
+      {
+	struct Lisp_TS_Parser *parser = XTS_PARSER (XCAR (tail));
+	if (EQ (parser->language_symbol, language))
+	  return XCAR (tail);
+      }
     }
 
   /* Load language.  */
@@ -1329,7 +1345,10 @@ DEFUN ("treesit-parser-list",
        Ftreesit_parser_list, Streesit_parser_list,
        0, 1, 0,
        doc: /* Return BUFFER's parser list.
-BUFFER defaults to the current buffer.  */)
+
+BUFFER defaults to the current buffer.  If BUFFER (or the current
+buffer) is an indirect buffer, its base buffer is used instead.  That
+is, indirect buffers uses their base buffer's parsers.  */)
   (Lisp_Object buffer)
 {
   struct buffer *buf;
@@ -1340,6 +1359,9 @@ DEFUN ("treesit-parser-list",
       CHECK_BUFFER (buffer);
       buf = XBUFFER (buffer);
     }
+  if (buf->base_buffer)
+    buf = buf->base_buffer;
+
   /* Return a fresh list so messing with that list doesn't affect our
      internal data.  */
   Lisp_Object return_list = Qnil;
diff --git a/test/src/treesit-tests.el b/test/src/treesit-tests.el
index aba12759c34..1cc2217bd3b 100644
--- a/test/src/treesit-tests.el
+++ b/test/src/treesit-tests.el
@@ -161,6 +161,40 @@ treesit-node-api
       (should (treesit-node-eq root-node root-node))
       (should (not (treesit-node-eq root-node doc-node))))))
 
+(ert-deftest treesit-indirect-buffer ()
+  "Tests for indirect buffers."
+  (skip-unless (treesit-language-available-p 'json))
+  (let ((base (get-buffer-create "*treesit test*"))
+        parser indirect)
+    (unwind-protect
+        (progn
+          (with-current-buffer base
+            (setq indirect (clone-indirect-buffer "*treesit test 1*" nil)))
+          (with-current-buffer indirect
+            (setq parser (treesit-parser-create 'json)))
+          ;; 1. Parser created in the indirect buffer should be
+          ;; actually be created in the base buffer.
+          (with-current-buffer base
+            (should (equal (list parser)
+                           (treesit-parser-list)))
+            (insert "[1,2,3]"))
+          ;; Change in the base buffer should be reflected in the
+          ;; indirect buffer.
+          (with-current-buffer indirect
+            (should (eq (treesit-node-end
+                         (treesit-buffer-root-node))
+                        8))
+            (erase-buffer))
+          ;; Change in the indirect buffer should be reflected in the
+          ;; base buffer.
+          (with-current-buffer base
+            (should (eq (treesit-node-end
+                         (treesit-buffer-root-node))
+                        1))
+            (erase-buffer)))
+      (kill-buffer base)
+      (kill-buffer indirect))))
+
 (ert-deftest treesit-query-api ()
   "Tests for query API."
   (skip-unless (treesit-language-available-p 'json))
-- 
2.33.1


[-- Attachment #3: Type: text/plain, Size: 2 bytes --]

bug#59693: 29.0.50; treesitter in base buffer doesn't respond to modifications in indirect buffer correctly

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Wed, 7 Dec 2022 15:13:19 -0800
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,
>  59693@debbugs.gnu.org,
>  miha@kamnitnik.top
> 
> >> 1. Only allow base buffer to have parsers, no change is needed for insdel.c, treesit_record_change can find the base buffer and update its parsers. We can ask indirect buffers to use their base buffer’s parser. Unless the base buffer is narrowed, I think it will work fine. 
> > 
> > I think this is fine, but we need to document it.
> > 
> >> I remember that there were a discussion along the lines of user-narrow vs low-level narrow, what was the outcome of that discussion?
> > 
> > Nothing in particular, and I don't think it's relevant.  If some mode needs
> > to widen, it can.
> > 
> > Thanks.
> 
> Here is a patch that does #1.

Thanks, a few minor comments for documentation below.

> +If @var{buffer} (or the current buffer) is an indirect buffer, its
> +base buffer is used instead.  That is, indirect buffers uses their
                                          ^^^^^^^^^^^^^^^^^^^^^
"use", in plural.

> @@ -447,7 +455,9 @@ Using Parser
>  @defun treesit-parser-list &optional buffer
>  This function returns the parser list of @var{buffer}.  If
>  @var{buffer} is @code{nil} or omitted, it defaults to the current
> -buffer.
> +buffer.  If @var{buffer} (or the current buffer) is an indirect
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
I'd say more concisely

  If that buffer is an indirect buffer, ...

And please add a cross-reference to the node where indirect buffers
are described.

> +buffer, its base buffer is used instead.  That is, indirect buffers
> +uses their base buffer's parsers.
   ^^^^
"use".

> +   Parsers in indirect buffers: We make indirect buffers to share the
> +   parser of its base buffer.  See bug#59693 for reasoning.  */

I'd rather have a short summary of the reasoning here than ask the
readers to go to the bug tracker and read a long discussion.  Just
explain why indirect buffers present a problem for a parser, and then
say that we decided to do this as the easiest, simplest solution.

> +If BUFFER (or the current buffer) is an indirect buffer, its base
> +buffer is used instead.  That is, indirect buffers uses their base
                                                      ^^^^
"use"

> +buffer's parsers.  If the base buffer is narrowed, an indirect buffer
> +might not be able to retrieve information of the portion of the buffer
> +text that are invisible in the base buffer.  Lisp programs should
> +widen as necessary should they want to use a parser in an indirect
> +buffer.  */)

Here I would remove the second sentence: it is appropriate for the
manual, but is redundant in the doc string, since the next sentence
says it all.

> @@ -1329,7 +1345,10 @@ DEFUN ("treesit-parser-list",
>         Ftreesit_parser_list, Streesit_parser_list,
>         0, 1, 0,
>         doc: /* Return BUFFER's parser list.
> -BUFFER defaults to the current buffer.  */)
> +
> +BUFFER defaults to the current buffer.  If BUFFER (or the current
> +buffer) is an indirect buffer, its base buffer is used instead.  That
> +is, indirect buffers uses their base buffer's parsers.  */)
                        ^^^^
"use"

Otherwise, LGTM.

bug#59693: 29.0.50; treesitter in base buffer doesn't respond to modifications in indirect buffer correctly

[Yuan Fu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Yuan Fu <casouri@gmail.com>
>> Date: Wed, 7 Dec 2022 15:13:19 -0800
>> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,
>>  59693@debbugs.gnu.org,
>>  miha@kamnitnik.top
>> 
>> >> 1. Only allow base buffer to have parsers, no change is needed
>> >> for insdel.c, treesit_record_change can find the base buffer and
>> >> update its parsers. We can ask indirect buffers to use their base
>> >> buffer’s parser. Unless the base buffer is narrowed, I think it
>> >> will work fine.
>> > 
>> > I think this is fine, but we need to document it.
>> > 
>> >> I remember that there were a discussion along the lines of user-narrow vs low-level narrow, what was the outcome of that discussion?
>> > 
>> > Nothing in particular, and I don't think it's relevant.  If some mode needs
>> > to widen, it can.
>> > 
>> > Thanks.
>> 
>> Here is a patch that does #1.
>
> Thanks, a few minor comments for documentation below.
>
>> +If @var{buffer} (or the current buffer) is an indirect buffer, its
>> +base buffer is used instead.  That is, indirect buffers uses their
>                                           ^^^^^^^^^^^^^^^^^^^^^
> "use", in plural.
>
>> @@ -447,7 +455,9 @@ Using Parser
>>  @defun treesit-parser-list &optional buffer
>>  This function returns the parser list of @var{buffer}.  If
>>  @var{buffer} is @code{nil} or omitted, it defaults to the current
>> -buffer.
>> +buffer.  If @var{buffer} (or the current buffer) is an indirect
>             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> I'd say more concisely
>
>   If that buffer is an indirect buffer, ...
>
> And please add a cross-reference to the node where indirect buffers
> are described.
>
>> +buffer, its base buffer is used instead.  That is, indirect buffers
>> +uses their base buffer's parsers.
>    ^^^^
> "use".
>
>> +   Parsers in indirect buffers: We make indirect buffers to share the
>> +   parser of its base buffer.  See bug#59693 for reasoning.  */
>
> I'd rather have a short summary of the reasoning here than ask the
> readers to go to the bug tracker and read a long discussion.  Just
> explain why indirect buffers present a problem for a parser, and then
> say that we decided to do this as the easiest, simplest solution.
>
>> +If BUFFER (or the current buffer) is an indirect buffer, its base
>> +buffer is used instead.  That is, indirect buffers uses their base
>                                                       ^^^^
> "use"
>
>> +buffer's parsers.  If the base buffer is narrowed, an indirect buffer
>> +might not be able to retrieve information of the portion of the buffer
>> +text that are invisible in the base buffer.  Lisp programs should
>> +widen as necessary should they want to use a parser in an indirect
>> +buffer.  */)
>
> Here I would remove the second sentence: it is appropriate for the
> manual, but is redundant in the doc string, since the next sentence
> says it all.
>
>> @@ -1329,7 +1345,10 @@ DEFUN ("treesit-parser-list",
>>         Ftreesit_parser_list, Streesit_parser_list,
>>         0, 1, 0,
>>         doc: /* Return BUFFER's parser list.
>> -BUFFER defaults to the current buffer.  */)
>> +
>> +BUFFER defaults to the current buffer.  If BUFFER (or the current
>> +buffer) is an indirect buffer, its base buffer is used instead.  That
>> +is, indirect buffers uses their base buffer's parsers.  */)
>                         ^^^^
> "use"
>
> Otherwise, LGTM.

Cool, I fixed those and pushed.

Yuan