bug#60659: 30.0.50; tree-sitter: identical nodes are not `equal'

bug#60659: 30.0.50; tree-sitter: identical nodes are not `equal'

[Mickey Petersen]

In order to check for node equality, one must use `treesit-node-eq'.

But I see little reason why two identical nodes in in the same tree aren't `equal'?



In GNU Emacs 30.0.50 (build 6, x86_64-pc-linux-gnu, GTK+ Version
 3.24.20, cairo version 1.16.0) of 2023-01-02 built on mickey-work
Repository revision: c209802f7b3721a1b95113290934a23fee88f678
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12013000
System Description: Ubuntu 20.04.3 LTS

Configured using:
 'configure --with-native-compilation --with-json --with-mailutils
 --without-compress-install --with-imagemagick CC=gcc-10'

bug#60659: 30.0.50; tree-sitter: identical nodes are not `equal'

[Yuan Fu]

Mickey Petersen <mickey@masteringemacs.org> writes:

> In order to check for node equality, one must use `treesit-node-eq'.
>
> But I see little reason why two identical nodes in in the same tree aren't `equal'?

Indeed. I’ll see if I can extend equal to handle tree-sitter nodes.

Yuan

bug#60659: 30.0.50; tree-sitter: identical nodes are not `equal'

[Mickey Petersen]

Yuan Fu <casouri@gmail.com> writes:

> Mickey Petersen <mickey@masteringemacs.org> writes:
>
>> In order to check for node equality, one must use `treesit-node-eq'.
>>
>> But I see little reason why two identical nodes in in the same tree aren't `equal'?
>
> Indeed. I’ll see if I can extend equal to handle tree-sitter nodes.
>

Nice one. This will allow for membership checks with `member' and
such-like and it'll greatly cut down on custom code, particularly one
you start writing tests!

> Yuan

bug#60659: 30.0.50; tree-sitter: identical nodes are not `equal'

[Yuan Fu]

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


Mickey Petersen <mickey@masteringemacs.org> writes:

> Yuan Fu <casouri@gmail.com> writes:
>
>> Mickey Petersen <mickey@masteringemacs.org> writes:
>>
>>> In order to check for node equality, one must use `treesit-node-eq'.
>>>
>>> But I see little reason why two identical nodes in in the same tree aren't `equal'?
>>
>> Indeed. I’ll see if I can extend equal to handle tree-sitter nodes.
>>
>
> Nice one. This will allow for membership checks with `member' and
> such-like and it'll greatly cut down on custom code, particularly one
> you start writing tests!
>
>> Yuan

Eli, does this look right to you? In particular, I’m not sure if it’s ok to call a lisp function (Ftreesit_node_eq) inside internal_equal, even though it never signals. 

Yuan


[-- Attachment #2: equal.diff --]
[-- Type: application/octet-stream, Size: 878 bytes --]

diff --git a/src/fns.c b/src/fns.c
index 1aaf17914a2..c0ecc735bc5 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -38,6 +38,10 @@ Copyright (C) 1985-2023 Free Software Foundation, Inc.
 #include "puresize.h"
 #include "gnutls.h"
 
+#ifdef HAVE_TREE_SITTER
+#include "treesit.h"
+#endif
+
 enum equal_kind { EQUAL_NO_QUIT, EQUAL_PLAIN, EQUAL_INCLUDING_PROPERTIES };
 static bool internal_equal (Lisp_Object, Lisp_Object,
 			    enum equal_kind, int, Lisp_Object);
@@ -2822,6 +2826,10 @@ internal_equal (Lisp_Object o1, Lisp_Object o2, enum equal_kind equal_kind,
 		    && !memcmp (bool_vector_data (o1), bool_vector_data (o2),
 			        bool_vector_bytes (size)));
 	  }
+	if (TS_NODEP (o1))
+	  {
+	    return !NILP (Ftreesit_node_eq (o1, o2));
+	  }
 
 	/* Aside from them, only true vectors, char-tables, compiled
 	   functions, and fonts (font-spec, font-entity, font-object)

bug#60659: 30.0.50; tree-sitter: identical nodes are not `equal'

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Mon, 9 Jan 2023 21:56:19 -0800
> Cc: 60659@debbugs.gnu.org
> 
> Eli, does this look right to you? In particular, I’m not sure if it’s ok to call a lisp function (Ftreesit_node_eq) inside internal_equal, even though it never signals. 

It is better to make a C function from most of the body of
Ftreesit_node_eq, but without the CHECK_NODE parts, and make both
Fequal and Ftreesit_node_eq call that.  Because CHECK_NODE can signal,
right?  Also, you should make sure up front that _both_ o1 and o2 are
treesit nodes, otherwise they cannot be 'equal'.

Lars, Stefan: any comments/suggestions?

bug#60659: 30.0.50; tree-sitter: identical nodes are not `equal'

[Yuan Fu]

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


Eli Zaretskii <eliz@gnu.org> writes:

>> From: Yuan Fu <casouri@gmail.com>
>> Date: Mon, 9 Jan 2023 21:56:19 -0800
>> Cc: 60659@debbugs.gnu.org
>> 
>> Eli, does this look right to you? In particular, I’m not sure if
>> it’s ok to call a lisp function (Ftreesit_node_eq) inside
>> internal_equal, even though it never signals.
>
> It is better to make a C function from most of the body of
> Ftreesit_node_eq, but without the CHECK_NODE parts, and make both
> Fequal and Ftreesit_node_eq call that.  Because CHECK_NODE can signal,
> right?  Also, you should make sure up front that _both_ o1 and o2 are
> treesit nodes, otherwise they cannot be 'equal'.

I should’ve included some context. There is code that checks whether o1
and o2 have the same (pseudo vector) type and size[1], and returns early
if not, so I only need to check the type of o1. And since I checked the
type of o1, CHECK_NODE will always succeed.

But still, using a C functions is more correct. How about this?


[-- Attachment #2: equal.diff --]
[-- Type: application/octet-stream, Size: 2391 bytes --]

diff --git a/src/fns.c b/src/fns.c
index 1aaf17914a2..d5f7565d3d7 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -38,6 +38,10 @@ Copyright (C) 1985-2023 Free Software Foundation, Inc.
 #include "puresize.h"
 #include "gnutls.h"
 
+#ifdef HAVE_TREE_SITTER
+#include "treesit.h"
+#endif
+
 enum equal_kind { EQUAL_NO_QUIT, EQUAL_PLAIN, EQUAL_INCLUDING_PROPERTIES };
 static bool internal_equal (Lisp_Object, Lisp_Object,
 			    enum equal_kind, int, Lisp_Object);
@@ -2822,6 +2826,10 @@ internal_equal (Lisp_Object o1, Lisp_Object o2, enum equal_kind equal_kind,
 		    && !memcmp (bool_vector_data (o1), bool_vector_data (o2),
 			        bool_vector_bytes (size)));
 	  }
+	if (TS_NODEP (o1))
+	  {
+	    return treesit_node_eq (o1, o2);
+	  }
 
 	/* Aside from them, only true vectors, char-tables, compiled
 	   functions, and fonts (font-spec, font-entity, font-object)
diff --git a/src/treesit.c b/src/treesit.c
index 55463122d14..6a1a5c5ea2a 100644
--- a/src/treesit.c
+++ b/src/treesit.c
@@ -2154,6 +2154,16 @@ DEFUN ("treesit-node-descendant-for-range",
   return make_treesit_node (XTS_NODE (node)->parser, child);
 }
 
+/* Return true if NODE1 and NODE2 are the same node.  Assumes they are
+   TS_NODE type.  */
+bool treesit_node_eq (Lisp_Object node1, Lisp_Object node2)
+{
+  treesit_initialize ();
+  TSNode treesit_node_1 = XTS_NODE (node1)->node;
+  TSNode treesit_node_2 = XTS_NODE (node2)->node;
+  return ts_node_eq (treesit_node_1, treesit_node_2);
+}
+
 DEFUN ("treesit-node-eq",
        Ftreesit_node_eq,
        Streesit_node_eq, 2, 2, 0,
@@ -2166,12 +2176,7 @@ DEFUN ("treesit-node-eq",
   CHECK_TS_NODE (node1);
   CHECK_TS_NODE (node2);
 
-  treesit_initialize ();
-
-  TSNode treesit_node_1 = XTS_NODE (node1)->node;
-  TSNode treesit_node_2 = XTS_NODE (node2)->node;
-
-  bool same_node = ts_node_eq (treesit_node_1, treesit_node_2);
+  bool same_node = treesit_node_eq (node1, node2);
   return same_node ? Qt : Qnil;
 }
 
diff --git a/src/treesit.h b/src/treesit.h
index 909609737d3..5382bc58817 100644
--- a/src/treesit.h
+++ b/src/treesit.h
@@ -191,6 +191,7 @@ CHECK_TS_COMPILED_QUERY (Lisp_Object query)
 extern void treesit_delete_parser (struct Lisp_TS_Parser *);
 extern void treesit_delete_query (struct Lisp_TS_Query *);
 extern bool treesit_named_node_p (TSNode);
+extern bool treesit_node_eq (Lisp_Object, Lisp_Object);
 
 #endif	/* HAVE_TREE_SITTER */
 

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



Yuan

[1]

	ptrdiff_t size = ASIZE (o1);
	/* Pseudovectors have the type encoded in the size field, so this test
	   actually checks that the objects have the same type as well as the
	   same size.  */
	if (ASIZE (o2) != size)
	  return false;

bug#60659: 30.0.50; tree-sitter: identical nodes are not `equal'

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Tue, 10 Jan 2023 18:57:02 -0800
> Cc: larsi@gnus.org,
>  monnier@iro.umontreal.ca,
>  60659@debbugs.gnu.org
> 
> But still, using a C functions is more correct. How about this?

Yes, this is what I had in mind.  Thanks.

bug#60659: 30.0.50; tree-sitter: identical nodes are not `equal'

[Yuan Fu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Yuan Fu <casouri@gmail.com>
>> Date: Tue, 10 Jan 2023 18:57:02 -0800
>> Cc: larsi@gnus.org,
>>  monnier@iro.umontreal.ca,
>>  60659@debbugs.gnu.org
>> 
>> But still, using a C functions is more correct. How about this?
>
> Yes, this is what I had in mind.  Thanks.

Cool. I applied it.

Yuan