bug#64442: 29.0.92; treesit-beginning-of-defun fails in DEFUN functions in C

bug#64442: 29.0.92; treesit-beginning-of-defun fails in DEFUN functions in C

[Eli Zaretskii]

To reproduce:

  emacs -Q
  C-x C-f src/dispnew.c RET
  M-x c-ts-mode RET
  C-u 220 M-g g
  C-M-a

Observe that instead of going to the beginning of
dump-redisplay-history, the function to which line 220 of dispnew.c
belongs, point goes to the beginning of the previous function, which
happens to be add_frame_display_history.

Can we please fix treesit-beginning-of-defun such that it recognizes C
functions defined via DEFUN?



In GNU Emacs 29.0.92 (build 20, i686-pc-mingw32) of 2023-07-02 built on
 HOME-C4E4A596F7
Repository revision: 15ff87617772c2a2c3d8a3a1e2ed7f96e527ad9e
Repository branch: emacs-29
Windowing system distributor 'Microsoft Corp.', version 5.1.2600
System Description: Microsoft Windows XP Service Pack 3 (v5.1.0.2600)

Configured using:
 'configure -C --prefix=/d/usr --with-wide-int
 --enable-checking=yes,glyphs 'CFLAGS=-O0 -gdwarf-4 -g3''

Configured features:
ACL GIF GMP GNUTLS HARFBUZZ JPEG JSON LCMS2 LIBXML2 MODULES NOTIFY
W32NOTIFY PDUMPER PNG RSVG SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP XPM ZLIB

Important settings:
  value of $LANG: ENU
  locale-coding-system: cp1255

Major mode: C/*

Minor modes in effect:
  bug-reference-prog-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date subr-x mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
misearch multi-isearch c-ts-mode c-ts-common treesit cl-seq vc-git
diff-mode easy-mmode vc vc-dispatcher bug-reference byte-opt gv bytecomp
byte-compile cc-mode cc-fonts cc-guess cc-menus cc-cmds cc-styles
cc-align cc-engine cc-vars cc-defs cl-loaddefs cl-lib rmc iso-transl
tooltip cconv eldoc paren electric uniquify ediff-hook vc-hooks
lisp-float-type elisp-mode mwheel dos-w32 ls-lisp disp-table
term/w32-win w32-win w32-vars term/common-win tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads w32notify w32 lcms2 multi-tty
make-network-process emacs)

Memory information:
((conses 16 81182 12528)
 (symbols 48 9662 0)
 (strings 16 28732 3398)
 (string-bytes 1 924785)
 (vectors 16 15907)
 (vector-slots 8 209599 17044)
 (floats 8 29 78)
 (intervals 40 1068 136)
 (buffers 888 11))

bug#64442: 29.0.92; treesit-beginning-of-defun fails in DEFUN functions in C

[Yuan Fu]

> On Jul 3, 2023, at 10:13 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> To reproduce:
> 
>  emacs -Q
>  C-x C-f src/dispnew.c RET
>  M-x c-ts-mode RET
>  C-u 220 M-g g
>  C-M-a
> 
> Observe that instead of going to the beginning of
> dump-redisplay-history, the function to which line 220 of dispnew.c
> belongs, point goes to the beginning of the previous function, which
> happens to be add_frame_display_history.
> 
> Can we please fix treesit-beginning-of-defun such that it recognizes C
> functions defined via DEFUN?

I’ve tried it when I was fixing fontification for DEFUN, but ultimately gave up. The tree-sitter defun movement functions searches for defun nodes bottom-up, and goes to the beginning or end of that node. 

The problem with DEFUN’s is that a DEFUN is really made of two nodes in the parse tree. One for the DEFUN part, one for the body, and there isn’t a parent node that encloses the two. 

The defun movement functions are not designed to handle a construct made of two adjacent nodes. They can find a node, go to the beginning/end of it; they can’t find a node, and go to the end of the next node.

It sounds easy to add some hack to handle it, but really isn’t. Defun movement need to support forward/backward to beg/end, that’s four movement types; on top of that you have nested defun’s.

Yuan

bug#64442: 29.0.92; treesit-beginning-of-defun fails in DEFUN functions in C

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Tue, 4 Jul 2023 01:41:22 -0700
> Cc: 64442@debbugs.gnu.org
> 
> The problem with DEFUN’s is that a DEFUN is really made of two nodes in the parse tree. One for the DEFUN part, one for the body, and there isn’t a parent node that encloses the two. 
> 
> The defun movement functions are not designed to handle a construct made of two adjacent nodes. They can find a node, go to the beginning/end of it; they can’t find a node, and go to the end of the next node.
> 
> It sounds easy to add some hack to handle it, but really isn’t. Defun movement need to support forward/backward to beg/end, that’s four movement types;

Why cannot we look for a top-level expression_statement node which is
a call_expression whose function identifier is "DEFUN" and whose
position is between the place where C-M-a was invoked and the place
where it does find a defun?

> on top of that you have nested defun’s.

DEFUN's cannot be nested, so we don't need to consider that.

Thanks.

bug#64442: 29.0.92; treesit-beginning-of-defun fails in DEFUN functions in C

[Yuan Fu]

> On Jul 4, 2023, at 4:39 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Yuan Fu <casouri@gmail.com>
>> Date: Tue, 4 Jul 2023 01:41:22 -0700
>> Cc: 64442@debbugs.gnu.org
>> 
>> The problem with DEFUN’s is that a DEFUN is really made of two nodes in the parse tree. One for the DEFUN part, one for the body, and there isn’t a parent node that encloses the two. 
>> 
>> The defun movement functions are not designed to handle a construct made of two adjacent nodes. They can find a node, go to the beginning/end of it; they can’t find a node, and go to the end of the next node.
>> 
>> It sounds easy to add some hack to handle it, but really isn’t. Defun movement need to support forward/backward to beg/end, that’s four movement types;
> 
> Why cannot we look for a top-level expression_statement node which is
> a call_expression whose function identifier is "DEFUN" and whose
> position is between the place where C-M-a was invoked and the place
> where it does find a defun?

It’s gonna be ugly, but I can take a jab at it this weekend. I’m thinking of a wrapper function that tries to detect DEFUN before falling back to the ordinary tree-sitter defun movement function.

> 
>> on top of that you have nested defun’s.
> 
> DEFUN's cannot be nested, so we don't need to consider that.

Yeah, in general C sources don’t have nested defuns, only C++ ones do. I was trying to illustrate that it’s hard to extend existing defun movement framework such that it handles this special case. The best solution I can think of is what I described above.

Yuan

bug#64442: 29.0.92; treesit-beginning-of-defun fails in DEFUN functions in C

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Thu, 6 Jul 2023 23:15:00 -0700
> Cc: 64442@debbugs.gnu.org
> 
> > On Jul 4, 2023, at 4:39 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> > 
> > Why cannot we look for a top-level expression_statement node which is
> > a call_expression whose function identifier is "DEFUN" and whose
> > position is between the place where C-M-a was invoked and the place
> > where it does find a defun?
> 
> It’s gonna be ugly, but I can take a jab at it this weekend. I’m thinking of a wrapper function that tries to detect DEFUN before falling back to the ordinary tree-sitter defun movement function.

Thanks.  let's see how ugly it is before deciding whether it's worth it.

> > DEFUN's cannot be nested, so we don't need to consider that.
> 
> Yeah, in general C sources don’t have nested defuns, only C++ ones do.

No, I meant the use of DEFUN macros in Emacs cannot be nested.

bug#64442: 29.0.92; treesit-beginning-of-defun fails in DEFUN functions in C

[Yuan Fu]

> On Jul 6, 2023, at 11:40 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Yuan Fu <casouri@gmail.com>
>> Date: Thu, 6 Jul 2023 23:15:00 -0700
>> Cc: 64442@debbugs.gnu.org
>> 
>>> On Jul 4, 2023, at 4:39 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>>> 
>>> Why cannot we look for a top-level expression_statement node which is
>>> a call_expression whose function identifier is "DEFUN" and whose
>>> position is between the place where C-M-a was invoked and the place
>>> where it does find a defun?
>> 
>> It’s gonna be ugly, but I can take a jab at it this weekend. I’m thinking of a wrapper function that tries to detect DEFUN before falling back to the ordinary tree-sitter defun movement function.
> 
> Thanks.  let's see how ugly it is before deciding whether it's worth it.
> 
>>> DEFUN's cannot be nested, so we don't need to consider that.
>> 
>> Yeah, in general C sources don’t have nested defuns, only C++ ones do.
> 
> No, I meant the use of DEFUN macros in Emacs cannot be nested.

Just an update. I didn’t forget about this, but it’s more harder than I thought and I’m still working on it :-(

Yuan

bug#64442: 29.0.92; treesit-beginning-of-defun fails in DEFUN functions in C

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Tue, 11 Jul 2023 19:10:01 -0700
> Cc: 64442@debbugs.gnu.org
> 
> 
> 
> > On Jul 6, 2023, at 11:40 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> > 
> >> From: Yuan Fu <casouri@gmail.com>
> >> Date: Thu, 6 Jul 2023 23:15:00 -0700
> >> Cc: 64442@debbugs.gnu.org
> >> 
> >>> On Jul 4, 2023, at 4:39 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> >>> 
> >>> Why cannot we look for a top-level expression_statement node which is
> >>> a call_expression whose function identifier is "DEFUN" and whose
> >>> position is between the place where C-M-a was invoked and the place
> >>> where it does find a defun?
> >> 
> >> It’s gonna be ugly, but I can take a jab at it this weekend. I’m thinking of a wrapper function that tries to detect DEFUN before falling back to the ordinary tree-sitter defun movement function.
> > 
> > Thanks.  let's see how ugly it is before deciding whether it's worth it.
> > 
> >>> DEFUN's cannot be nested, so we don't need to consider that.
> >> 
> >> Yeah, in general C sources don’t have nested defuns, only C++ ones do.
> > 
> > No, I meant the use of DEFUN macros in Emacs cannot be nested.
> 
> Just an update. I didn’t forget about this, but it’s more harder than I thought and I’m still working on it :-(

Any progress with this?  It would be good to have a solution in Emacs
29.2, if possible.

Thanks.

bug#64442: 29.0.92; treesit-beginning-of-defun fails in DEFUN functions in C

[Eli Zaretskii]

> Cc: 64442@debbugs.gnu.org
> Date: Sun, 30 Jul 2023 10:10:35 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: Yuan Fu <casouri@gmail.com>
> > Date: Tue, 11 Jul 2023 19:10:01 -0700
> > Cc: 64442@debbugs.gnu.org
> > 
> > 
> > 
> > > On Jul 6, 2023, at 11:40 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> > > 
> > >> From: Yuan Fu <casouri@gmail.com>
> > >> Date: Thu, 6 Jul 2023 23:15:00 -0700
> > >> Cc: 64442@debbugs.gnu.org
> > >> 
> > >>> On Jul 4, 2023, at 4:39 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> > >>> 
> > >>> Why cannot we look for a top-level expression_statement node which is
> > >>> a call_expression whose function identifier is "DEFUN" and whose
> > >>> position is between the place where C-M-a was invoked and the place
> > >>> where it does find a defun?
> > >> 
> > >> It’s gonna be ugly, but I can take a jab at it this weekend. I’m thinking of a wrapper function that tries to detect DEFUN before falling back to the ordinary tree-sitter defun movement function.
> > > 
> > > Thanks.  let's see how ugly it is before deciding whether it's worth it.
> > > 
> > >>> DEFUN's cannot be nested, so we don't need to consider that.
> > >> 
> > >> Yeah, in general C sources don’t have nested defuns, only C++ ones do.
> > > 
> > > No, I meant the use of DEFUN macros in Emacs cannot be nested.
> > 
> > Just an update. I didn’t forget about this, but it’s more harder than I thought and I’m still working on it :-(
> 
> Any progress with this?  It would be good to have a solution in Emacs
> 29.2, if possible.

Ping!

bug#64442: 29.0.92; treesit-beginning-of-defun fails in DEFUN functions in C

[Yuan Fu]

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



> On Aug 10, 2023, at 2:18 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> Cc: 64442@debbugs.gnu.org
>> Date: Sun, 30 Jul 2023 10:10:35 +0300
>> From: Eli Zaretskii <eliz@gnu.org>
>> 
>>> From: Yuan Fu <casouri@gmail.com>
>>> Date: Tue, 11 Jul 2023 19:10:01 -0700
>>> Cc: 64442@debbugs.gnu.org
>>> 
>>> 
>>> 
>>>> On Jul 6, 2023, at 11:40 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>>>> 
>>>>> From: Yuan Fu <casouri@gmail.com>
>>>>> Date: Thu, 6 Jul 2023 23:15:00 -0700
>>>>> Cc: 64442@debbugs.gnu.org
>>>>> 
>>>>>> On Jul 4, 2023, at 4:39 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>>>>>> 
>>>>>> Why cannot we look for a top-level expression_statement node which is
>>>>>> a call_expression whose function identifier is "DEFUN" and whose
>>>>>> position is between the place where C-M-a was invoked and the place
>>>>>> where it does find a defun?
>>>>> 
>>>>> It’s gonna be ugly, but I can take a jab at it this weekend. I’m thinking of a wrapper function that tries to detect DEFUN before falling back to the ordinary tree-sitter defun movement function.
>>>> 
>>>> Thanks.  let's see how ugly it is before deciding whether it's worth it.
>>>> 
>>>>>> DEFUN's cannot be nested, so we don't need to consider that.
>>>>> 
>>>>> Yeah, in general C sources don’t have nested defuns, only C++ ones do.
>>>> 
>>>> No, I meant the use of DEFUN macros in Emacs cannot be nested.
>>> 
>>> Just an update. I didn’t forget about this, but it’s more harder than I thought and I’m still working on it :-(
>> 
>> Any progress with this?  It would be good to have a solution in Emacs
>> 29.2, if possible.
> 
> Ping!

I still don’t have a good solution. But I just realized that we might be able to make a little compromise: what if Emacs recognizes DEFUN, but as two separate parts (the declaration and the body), rather than one? It’s hard to make it recognize DEFUN as a single defun, but making it recognize DEFUN as two parts is easy.

Try this patch and see if you like the behavior. Personally I find it quite alright.

Yuan


[-- Attachment #2: defun-nav.patch --]
[-- Type: application/octet-stream, Size: 5483 bytes --]

From b0e361b430650e95449c59b8643be58d598cde58 Mon Sep 17 00:00:00 2001
From: Yuan Fu <casouri@gmail.com>
Date: Thu, 10 Aug 2023 14:27:29 -0700
Subject: [PATCH] Support defun navigation for DEFUN in c-ts-mode (bug#64442)

Before this change, beginning/end-of-defun just ignores DEFUN in
c-ts-mode. After this change, beginning/end-of-defun can recognize
DEFUN, but a DEFUN definition is considered two defuns. Eg,
beginning/end-of-defun will stop at (1) (2) and (3) in the following
snippet:

(1)DEFUN ("treesit-node-parser",
       Ftreesit_node_parser, Streesit_node_parser,
       1, 1, 0,
       doc: /* Return the parser to which NODE belongs.  */)
  (Lisp_Object node)
(2){
  CHECK_TS_NODE (node);
  return XTS_NODE (node)->parser;
}
(3)

Ideally we want point to only stop at (1) and (3), but that'll be a
lot harder to do.

* lisp/progmodes/c-ts-mode.el:
(c-ts-mode--defun-valid-p): Refactor to take in account of DEFUN body.
(c-ts-mode--emacs-defun-body-p): New function.
(c-ts-base-mode): Add DEFUN and DEFUN body to recognized types.
---
 lisp/progmodes/c-ts-mode.el | 64 +++++++++++++++++++++++--------------
 1 file changed, 40 insertions(+), 24 deletions(-)

diff --git a/lisp/progmodes/c-ts-mode.el b/lisp/progmodes/c-ts-mode.el
index 7f4f6f11387..0eb1f2a17a7 100644
--- a/lisp/progmodes/c-ts-mode.el
+++ b/lisp/progmodes/c-ts-mode.el
@@ -882,29 +882,36 @@ c-ts-mode--defun-name
 (defun c-ts-mode--defun-valid-p (node)
   "Return non-nil if NODE is a valid defun node.
 Ie, NODE is not nested."
-  (or (c-ts-mode--emacs-defun-p node)
-      (not (or (and (member (treesit-node-type node)
-                            '("struct_specifier"
-                              "enum_specifier"
-                              "union_specifier"
-                              "declaration"))
-                    ;; If NODE's type is one of the above, make sure it is
-                    ;; top-level.
-                    (treesit-node-top-level
-                     node (rx (or "function_definition"
-                                  "type_definition"
-                                  "struct_specifier"
-                                  "enum_specifier"
-                                  "union_specifier"
-                                  "declaration"))))
-
-               (and (equal (treesit-node-type node) "declaration")
-                    ;; If NODE is a declaration, make sure it is not a
-                    ;; function declaration.
-                    (equal (treesit-node-type
-                            (treesit-node-child-by-field-name
-                             node "declarator"))
-                           "function_declarator"))))))
+  (let ((top-level-p (lambda (node)
+                       (not (treesit-node-top-level
+                             node (rx (or "function_definition"
+                                          "type_definition"
+                                          "struct_specifier"
+                                          "enum_specifier"
+                                          "union_specifier"
+                                          "declaration")))))))
+    (pcase (treesit-node-type node)
+      ;; The declaration part of a DEFUN.
+      ("expression_statement" (c-ts-mode--emacs-defun-p node))
+      ;; The body of a DEFUN.
+      ("compound_statement" (c-ts-mode--emacs-defun-body-p node))
+      ;; If NODE's type is one of these three, make sure it is
+      ;; top-level.
+      ((or "struct_specifier"
+           "enum_specifier"
+           "union_specifier")
+       (funcall top-level-p node))
+      ;; If NODE is a declaration, make sure it's not a function
+      ;; declaration (we only want function_definition) and is a
+      ;; top-level declaration.
+      ("declaration"
+       (and (not (equal (treesit-node-type
+                         (treesit-node-child-by-field-name
+                          node "declarator"))
+                        "function_declarator"))
+            (funcall top-level-p node)))
+      ;; Other types don't need further verification.
+      (_ t))))
 
 (defun c-ts-mode--defun-for-class-in-imenu-p (node)
   "Check if NODE is a valid entry for the Class subindex.
@@ -957,6 +964,11 @@ c-ts-mode--emacs-defun-p
                t)
               "DEFUN")))
 
+(defun c-ts-mode--emacs-defun-body-p (node)
+  "Return non-nil if NODE is the function body of a DEFUN."
+  (and (equal (treesit-node-type node) "compound_statement")
+       (c-ts-mode--emacs-defun-p (treesit-node-prev-sibling node))))
+
 (defun c-ts-mode--emacs-defun-at-point (&optional range)
   "Return the defun node at point.
 
@@ -1119,7 +1131,11 @@ c-ts-base-mode
                                   "enum_specifier"
                                   "union_specifier"
                                   "class_specifier"
-                                  "namespace_definition"))
+                                  "namespace_definition"
+                                  ;; DEFUN.
+                                  "expression_statement"
+                                  ;; DEFUN body.
+                                  "compound_statement"))
                     #'c-ts-mode--defun-valid-p))
   (setq-local treesit-defun-skipper #'c-ts-mode--defun-skipper)
   (setq-local treesit-defun-name-function #'c-ts-mode--defun-name)
-- 
2.41.0

bug#64442: 29.0.92; treesit-beginning-of-defun fails in DEFUN functions in C

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Thu, 10 Aug 2023 14:33:09 -0700
> Cc: 64442@debbugs.gnu.org
> 
> I still don’t have a good solution. But I just realized that we might be able to make a little compromise: what if Emacs recognizes DEFUN, but as two separate parts (the declaration and the body), rather than one? It’s hard to make it recognize DEFUN as a single defun, but making it recognize DEFUN as two parts is easy.
> 
> Try this patch and see if you like the behavior. Personally I find it quite alright.

I like this much better than what we have now, thanks.  But I have a
question: can we perhaps recognize the "function" of the body as such,
and then automatically move to the previous defun, which is the right
place?  The "defun" that is the body has no name, so maybe that could
be used as a sign?  That would allow "C-x 4 a" to work inside a DEFUN,
something that still works less reliably with this patch: you must be
in the "first defun" to get it to find the name of the function.

But if improving this is hard, I'll settle for what you have now,
thanks a lot.

bug#64442: 29.0.92; treesit-beginning-of-defun fails in DEFUN functions in C

[Yuan Fu]

> On Aug 12, 2023, at 7:59 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Yuan Fu <casouri@gmail.com>
>> Date: Thu, 10 Aug 2023 14:33:09 -0700
>> Cc: 64442@debbugs.gnu.org
>> 
>> I still don’t have a good solution. But I just realized that we might be able to make a little compromise: what if Emacs recognizes DEFUN, but as two separate parts (the declaration and the body), rather than one? It’s hard to make it recognize DEFUN as a single defun, but making it recognize DEFUN as two parts is easy.
>> 
>> Try this patch and see if you like the behavior. Personally I find it quite alright.
> 
> I like this much better than what we have now, thanks.  But I have a
> question: can we perhaps recognize the "function" of the body as such,
> and then automatically move to the previous defun, which is the right
> place?  The "defun" that is the body has no name, so maybe that could
> be used as a sign?  

We can easily tell the body from the declaration, but we can’t easily tell whether we should automatically move forward or backward. When point arrives at the point between the declaration and the body, should it move to the beginning of the next defun or the beginning of the declaration? This, plus it’s not straightforward to know whether we are in between a body and a declaration. I really don’t want to add even more cursed hacks into c-ts-mode.el :-)

> That would allow "C-x 4 a" to work inside a DEFUN,
> something that still works less reliably with this patch: you must be
> in the "first defun" to get it to find the name of the function.

C-x 4 a should’ve been fixed already. And it shouldn’t rely on this fix to work. Do you have a recipe for when it doesn’t work?

Yuan

bug#64442: 29.0.92; treesit-beginning-of-defun fails in DEFUN functions in C

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Sun, 13 Aug 2023 22:20:56 -0700
> Cc: 64442@debbugs.gnu.org
> 
> > I like this much better than what we have now, thanks.  But I have a
> > question: can we perhaps recognize the "function" of the body as such,
> > and then automatically move to the previous defun, which is the right
> > place?  The "defun" that is the body has no name, so maybe that could
> > be used as a sign?  
> 
> We can easily tell the body from the declaration, but we can’t easily tell whether we should automatically move forward or backward. When point arrives at the point between the declaration and the body, should it move to the beginning of the next defun or the beginning of the declaration? This, plus it’s not straightforward to know whether we are in between a body and a declaration. I really don’t want to add even more cursed hacks into c-ts-mode.el :-)

Too bad, but okay.

> > That would allow "C-x 4 a" to work inside a DEFUN,
> > something that still works less reliably with this patch: you must be
> > in the "first defun" to get it to find the name of the function.
> 
> C-x 4 a should’ve been fixed already. And it shouldn’t rely on this fix to work. Do you have a recipe for when it doesn’t work?

Just try it with your patch.  If point is inside the body, the
function's name is not captured by "C-x 4 a".

bug#64442: 29.0.92; treesit-beginning-of-defun fails in DEFUN functions in C

[Yuan Fu]

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



> On Aug 14, 2023, at 4:59 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Yuan Fu <casouri@gmail.com>
>> Date: Sun, 13 Aug 2023 22:20:56 -0700
>> Cc: 64442@debbugs.gnu.org
>> 
>>> I like this much better than what we have now, thanks.  But I have a
>>> question: can we perhaps recognize the "function" of the body as such,
>>> and then automatically move to the previous defun, which is the right
>>> place?  The "defun" that is the body has no name, so maybe that could
>>> be used as a sign?  
>> 
>> We can easily tell the body from the declaration, but we can’t easily tell whether we should automatically move forward or backward. When point arrives at the point between the declaration and the body, should it move to the beginning of the next defun or the beginning of the declaration? This, plus it’s not straightforward to know whether we are in between a body and a declaration. I really don’t want to add even more cursed hacks into c-ts-mode.el :-)
> 
> Too bad, but okay.
> 
>>> That would allow "C-x 4 a" to work inside a DEFUN,
>>> something that still works less reliably with this patch: you must be
>>> in the "first defun" to get it to find the name of the function.
>> 
>> C-x 4 a should’ve been fixed already. And it shouldn’t rely on this fix to work. Do you have a recipe for when it doesn’t work?
> 
> Just try it with your patch.  If point is inside the body, the
> function's name is not captured by "C-x 4 a”.

My bad, I must’ve been trying C-x 4 a in a different Emacs session, which worked. Anyway, I updated the patch and C-x 4 a should now work.

Yuan


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

From 779092b28e472d17adfff92c1b9d7403aad7a106 Mon Sep 17 00:00:00 2001
From: Yuan Fu <casouri@gmail.com>
Date: Thu, 10 Aug 2023 14:27:29 -0700
Subject: [PATCH] Support defun navigation for DEFUN in c-ts-mode (bug#64442)

Before this change, beginning/end-of-defun just ignores DEFUN in
c-ts-mode. After this change, beginning/end-of-defun can recognize
DEFUN, but a DEFUN definition is considered two defuns. Eg,
beginning/end-of-defun will stop at (1) (2) and (3) in the following
snippet:

(1)DEFUN ("treesit-node-parser",
       Ftreesit_node_parser, Streesit_node_parser,
       1, 1, 0,
       doc: /* Return the parser to which NODE belongs.  */)
  (Lisp_Object node)
(2){
  CHECK_TS_NODE (node);
  return XTS_NODE (node)->parser;
}
(3)

Ideally we want point to only stop at (1) and (3), but that'll be a
lot harder to do.

* lisp/progmodes/c-ts-mode.el:
(c-ts-mode--defun-valid-p): Refactor to take in account of DEFUN body.
(c-ts-mode--emacs-defun-body-p): New function.
(c-ts-base-mode): Add DEFUN and DEFUN body to recognized types.
(c-ts-mode--emacs-defun-at-point): Now that we recognize both parts of
a DEFUN as defun, c-ts-mode--emacs-defun-at-point needs to be updated
to adapt to it.
---
 lisp/progmodes/c-ts-mode.el | 115 +++++++++++++++++++-----------------
 1 file changed, 60 insertions(+), 55 deletions(-)

diff --git a/lisp/progmodes/c-ts-mode.el b/lisp/progmodes/c-ts-mode.el
index 98797bf3ce7..34a89b4dcad 100644
--- a/lisp/progmodes/c-ts-mode.el
+++ b/lisp/progmodes/c-ts-mode.el
@@ -880,29 +880,36 @@ c-ts-mode--defun-name
 (defun c-ts-mode--defun-valid-p (node)
   "Return non-nil if NODE is a valid defun node.
 Ie, NODE is not nested."
-  (or (c-ts-mode--emacs-defun-p node)
-      (not (or (and (member (treesit-node-type node)
-                            '("struct_specifier"
-                              "enum_specifier"
-                              "union_specifier"
-                              "declaration"))
-                    ;; If NODE's type is one of the above, make sure it is
-                    ;; top-level.
-                    (treesit-node-top-level
-                     node (rx (or "function_definition"
-                                  "type_definition"
-                                  "struct_specifier"
-                                  "enum_specifier"
-                                  "union_specifier"
-                                  "declaration"))))
-
-               (and (equal (treesit-node-type node) "declaration")
-                    ;; If NODE is a declaration, make sure it is not a
-                    ;; function declaration.
-                    (equal (treesit-node-type
-                            (treesit-node-child-by-field-name
-                             node "declarator"))
-                           "function_declarator"))))))
+  (let ((top-level-p (lambda (node)
+                       (not (treesit-node-top-level
+                             node (rx (or "function_definition"
+                                          "type_definition"
+                                          "struct_specifier"
+                                          "enum_specifier"
+                                          "union_specifier"
+                                          "declaration")))))))
+    (pcase (treesit-node-type node)
+      ;; The declaration part of a DEFUN.
+      ("expression_statement" (c-ts-mode--emacs-defun-p node))
+      ;; The body of a DEFUN.
+      ("compound_statement" (c-ts-mode--emacs-defun-body-p node))
+      ;; If NODE's type is one of these three, make sure it is
+      ;; top-level.
+      ((or "struct_specifier"
+           "enum_specifier"
+           "union_specifier")
+       (funcall top-level-p node))
+      ;; If NODE is a declaration, make sure it's not a function
+      ;; declaration (we only want function_definition) and is a
+      ;; top-level declaration.
+      ("declaration"
+       (and (not (equal (treesit-node-type
+                         (treesit-node-child-by-field-name
+                          node "declarator"))
+                        "function_declarator"))
+            (funcall top-level-p node)))
+      ;; Other types don't need further verification.
+      (_ t))))
 
 (defun c-ts-mode--defun-for-class-in-imenu-p (node)
   "Check if NODE is a valid entry for the Class subindex.
@@ -955,6 +962,11 @@ c-ts-mode--emacs-defun-p
                t)
               "DEFUN")))
 
+(defun c-ts-mode--emacs-defun-body-p (node)
+  "Return non-nil if NODE is the function body of a DEFUN."
+  (and (equal (treesit-node-type node) "compound_statement")
+       (c-ts-mode--emacs-defun-p (treesit-node-prev-sibling node))))
+
 (defun c-ts-mode--emacs-defun-at-point (&optional range)
   "Return the defun node at point.
 
@@ -969,31 +981,18 @@ c-ts-mode--emacs-defun-at-point
 If RANGE is non-nil, return (BEG . END) where BEG end END
 encloses the whole defun.  This is for when the entire defun
 is required, not just the declaration part for DEFUN."
-  (or (when-let ((node (treesit-defun-at-point)))
-        (if range
-            (cons (treesit-node-start node)
-                (treesit-node-end node))
-            node))
-      (and c-ts-mode-emacs-sources-support
-           (let ((candidate-1 ; For when point is in the DEFUN statement.
-                  (treesit-node-prev-sibling
-                   (treesit-node-top-level
-                    (treesit-node-at (point))
-                    "compound_statement")))
-                 (candidate-2 ; For when point is in the body.
-                  (treesit-node-top-level
-                   (treesit-node-at (point))
-                   "expression_statement")))
-             (when-let
-                 ((node (or (and (c-ts-mode--emacs-defun-p candidate-1)
-                                 candidate-1)
-                            (and (c-ts-mode--emacs-defun-p candidate-2)
-                                 candidate-2))))
-               (if range
-                   (cons (treesit-node-start node)
-                       (treesit-node-end
-                        (treesit-node-next-sibling node)))
-                   node))))))
+  (when-let* ((node (treesit-defun-at-point))
+              (defun-range (cons (treesit-node-start node)
+                                 (treesit-node-end node))))
+    ;; Make some adjustment for DEFUN.
+    (when c-ts-mode-emacs-sources-support
+      (cond ((c-ts-mode--emacs-defun-body-p node)
+             (setq node (treesit-node-prev-sibling node))
+             (setcar defun-range (treesit-node-start node)))
+            ((c-ts-mode--emacs-defun-p node)
+             (setcdr defun-range (treesit-node-end
+                                  (treesit-node-next-sibling node))))))
+    (if range defun-range node)))
 
 (defun c-ts-mode-indent-defun ()
   "Indent the current top-level declaration syntactically.
@@ -1111,13 +1110,19 @@ c-ts-base-mode
 
   ;; Navigation.
   (setq-local treesit-defun-type-regexp
-              (cons (regexp-opt '("function_definition"
-                                  "type_definition"
-                                  "struct_specifier"
-                                  "enum_specifier"
-                                  "union_specifier"
-                                  "class_specifier"
-                                  "namespace_definition"))
+              (cons (regexp-opt (append
+                                 '("function_definition"
+                                   "type_definition"
+                                   "struct_specifier"
+                                   "enum_specifier"
+                                   "union_specifier"
+                                   "class_specifier"
+                                   "namespace_definition")
+                                 (and c-ts-mode-emacs-sources-support
+                                      '(;; DEFUN.
+                                        "expression_statement"
+                                        ;; DEFUN body.
+                                        "compound_statement"))))
                     #'c-ts-mode--defun-valid-p))
   (setq-local treesit-defun-skipper #'c-ts-mode--defun-skipper)
   (setq-local treesit-defun-name-function #'c-ts-mode--defun-name)
-- 
2.41.0


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

bug#64442: 29.0.92; treesit-beginning-of-defun fails in DEFUN functions in C

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Tue, 15 Aug 2023 00:30:53 -0700
> Cc: 64442@debbugs.gnu.org
> 
> >>> That would allow "C-x 4 a" to work inside a DEFUN,
> >>> something that still works less reliably with this patch: you must be
> >>> in the "first defun" to get it to find the name of the function.
> >> 
> >> C-x 4 a should’ve been fixed already. And it shouldn’t rely on this fix to work. Do you have a recipe for when it doesn’t work?
> > 
> > Just try it with your patch.  If point is inside the body, the
> > function's name is not captured by "C-x 4 a”.
> 
> My bad, I must’ve been trying C-x 4 a in a different Emacs session, which worked. Anyway, I updated the patch and C-x 4 a should now work.

Thanks, LGTM.  Please install on the emacs-29 branch, and close the
bug when you do.

bug#64442: 29.0.92; treesit-beginning-of-defun fails in DEFUN functions in C

[Yuan Fu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Yuan Fu <casouri@gmail.com>
>> Date: Tue, 15 Aug 2023 00:30:53 -0700
>> Cc: 64442@debbugs.gnu.org
>> 
>> >>> That would allow "C-x 4 a" to work inside a DEFUN,
>> >>> something that still works less reliably with this patch: you
>> >>> must be
>> >>> in the "first defun" to get it to find the name of the function.
>> >> 
>> >> C-x 4 a should’ve been fixed already. And it shouldn’t rely on
>> >> this fix to work. Do you have a recipe for when it doesn’t work?
>> > 
>> > Just try it with your patch.  If point is inside the body, the
>> > function's name is not captured by "C-x 4 a”.
>> 
>> My bad, I must’ve been trying C-x 4 a in a different Emacs session,
>> which worked. Anyway, I updated the patch and C-x 4 a should now
>> work.
>
> Thanks, LGTM.  Please install on the emacs-29 branch, and close the
> bug when you do.

Pushed, thanks.

Yuan