GNU bug report logs - #60407
[PATCH] Update go-ts-mode to use Imenu facility

Previous Next

Full log

Message #26 received at 60407 <at> debbugs.gnu.org (full text, mbox):

From: Randy Taylor <dev <at> rjt.dev>
To: Evgeni Kolev <evgenysw <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Yuan Fu <casouri <at> gmail.com>,
 60407 <at> debbugs.gnu.org
Subject: Re: bug#60407: [PATCH] Update go-ts-mode to use Imenu facility
Date: Tue, 03 Jan 2023 14:30:59 +0000

On Tuesday, January 3rd, 2023 at 04:01, Evgeni Kolev <evgenysw <at> gmail.com> wrote:
>
> Hi Randy, thank you for your feedback!
> 
> I'm providing an updated patch below. I tested with "type Quack int"
> and other cases such as:
> type MyInt = int
> type Option func(s string)
> type List[T any] struct {
> head, tail *element[T]
> }
> type Number interface {
> int64 | float64
> }
> 
> After experimenting, I decided to add additional Imenu categories:
> "Alias" and "Type".
> So the final list of newly added categories is "Method", "Struct",
> "Interface", "Alias" and "Type".

Sounds good to me.

It looks like the alias type MyInt = int shows up in both categories: Alias and Type.
It should probably belong just in the Alias category.

> 
> Structs and interfaces are technically a private case for "Type", but
> are put in their own Imenu category.
> Hence the "Type" category now holds all types except structs,
> interfaces and aliases (aliases have their own tree sitter type
> defined in Go's grammar.js).
> 
> For reference, eglot's Imenu uses category "Class" instead of "Type".
> But I decided to not replicate this behavior - "Class" is not a widely
> used Go concept.
> However, I decided to replicate another eglot behaviour - prefixing
> the method names with the type of the receiver (for example,
> "(rect).area" for "func (r rect) area() float64...").

Great! I was going to suggest that but forgot.

> 
> I've also addressed the other comments. I'm a bit unsure how the git
> commit should be formatted - the part of the message which describes
> changed functions/files.
> 
> Please let me know if the patch can be improved. The patch is below.

Comments below.

> 
> commit a96e70052a79881ac666ab699ffd63ed916eaf83
> Author: Evgeni Kolev evgenysw <at> gmail.com
> 
> Date: Thu Dec 29 17:49:40 2022 +0200
> 
> Improve go-ts-mode Imenu

Maybe this should also say "and add navigation support" (or something similar)?

> 
> The Imenu items are extended to support "Method", "Struct",
> "Interface", "Alias" and "Type".
> 
> go-ts-mode is updated to use the Imenu facility added in commit
> b39dc7ab27a696a8607ab859aeff3c71509231f5.
> 
> * lisp/progmodes/go-ts-mode.el (go-ts-mode--imenu-1) (go-ts-mode--imenu):
> Remove functions.
> (go-ts-mode--defun-name) (go-ts-mode--interface-node-p)

I'm no commit format expert, but I think this can be (go-ts-mode--defun-name, go-ts-mode--interface-node-p)
Whenever it fits on a single line, you can combine them like that. Same for the line below.

> (go-ts-mode--struct-node-p) (go-ts-mode--other-type-node-p)
> (go-ts-mode--alias-node-p): New functions.
> (go-ts-mode): Improve Imenu settings.

I think the (go-ts-mode) part should mention that navigation support was added.

> 
> diff --git a/lisp/progmodes/go-ts-mode.el b/lisp/progmodes/go-ts-mode.el
> index 1d6a8a30db5..d91b555e03e 100644
> --- a/lisp/progmodes/go-ts-mode.el
> +++ b/lisp/progmodes/go-ts-mode.el
> @@ -173,44 +173,6 @@ go-ts-mode--font-lock-settings
> '((ERROR) @font-lock-warning-face))
> "Tree-sitter font-lock settings for `go-ts-mode'.") -(defun go-ts-mode--imenu () - "Return Imenu alist for the current buffer." - (let* ((node (treesit-buffer-root-node)) - (func-tree (treesit-induce-sparse-tree - node "function_declaration" nil 1000)) - (type-tree (treesit-induce-sparse-tree - node "type_spec" nil 1000)) - (func-index (go-ts-mode--imenu-1 func-tree)) - (type-index (go-ts-mode--imenu-1 type-tree))) - (append - (when func-index` (("Function" . ,func-index)))
> - (when type-index `(("Type" . ,type-index)))))) - -(defun go-ts-mode--imenu-1 (node) - "Helper for` go-ts-mode--imenu'.
> -Find string representation for NODE and set marker, then recurse
> -the subtrees."
> - (let* ((ts-node (car node))
> - (children (cdr node))
> - (subtrees (mapcan #'go-ts-mode--imenu-1
> - children))
> - (name (when ts-node
> - (treesit-node-text
> - (pcase (treesit-node-type ts-node)
> - ("function_declaration"
> - (treesit-node-child-by-field-name ts-node "name"))
> - ("type_spec"
> - (treesit-node-child-by-field-name ts-node "name"))))))
> - (marker (when ts-node
> - (set-marker (make-marker)
> - (treesit-node-start ts-node)))))
> - (cond
> - ((or (null ts-node) (null name)) subtrees)
> - (subtrees
> - `((,name ,(cons name marker) ,@subtrees))) - (t -` ((,name . ,marker))))))
> -
> ;;;###autoload
> (add-to-list 'auto-mode-alist '("\\.go\\'" . go-ts-mode))
> 
> @@ -228,9 +190,21 @@ go-ts-mode
> (setq-local comment-end "")
> (setq-local comment-start-skip (rx "//" (* (syntax whitespace))))
> 
> + ;; Navigation.
> + (setq-local treesit-defun-type-regexp
> + (regexp-opt '("method_declaration"
> + "function_declaration"
> + "type_declaration")))
> + (setq-local treesit-defun-name-function #'go-ts-mode--defun-name)
> +
> ;; Imenu.
> - (setq-local imenu-create-index-function #'go-ts-mode--imenu)
> - (setq-local which-func-functions nil)
> + (setq-local treesit-simple-imenu-settings
> + `(("Function" "\\\\`function_declaration\\'" nil nil)
> + ("Method" "\\`method_declaration\\\\'" nil nil) + ("Struct" "\\\\`type_declaration\\'"
> go-ts-mode--struct-node-p nil)
> + ("Interface" "\\`type_declaration\\\\'" go-ts-mode--interface-node-p nil) + ("Type" "\\\\`type_declaration\\'"
> go-ts-mode--other-type-node-p nil)
> + ("Alias" "\\`type_declaration\\'"
> go-ts-mode--alias-node-p nil)))
> 
> ;; Indent.
> (setq-local indent-tabs-mode t
> @@ -247,6 +221,53 @@ go-ts-mode
> 
> (treesit-major-mode-setup)))
> 
> +(defun go-ts-mode--defun-name (node)
> + "Return the defun name of NODE.
> +Return nil if there is no name or if NODE is not a defun node."
> + (pcase (treesit-node-type node)
> + ("function_declaration"
> + (treesit-node-text
> + (treesit-node-child-by-field-name
> + node "name")
> + t))
> + ("method_declaration"
> + (let* ((receiver-node (treesit-node-child-by-field-name node "receiver"))
> + (type-node (treesit-search-subtree receiver-node
> "type_identifier"))
> + (name-node (treesit-node-child-by-field-name node "name")))
> + (concat
> + "(" (treesit-node-text type-node) ")."
> + (treesit-node-text name-node))))
> + ("type_declaration"
> + (treesit-node-text
> + (treesit-node-child-by-field-name
> + (treesit-node-child node 0 t) "name")
> + t))))
> +
> +(defun go-ts-mode--interface-node-p (node)
> + "Return t if NODE is an interface."
> + (and
> + (string-equal "type_declaration" (treesit-node-type node))
> + (treesit-search-subtree node "interface_type" nil nil 2)))

I think you need to add (declare-function treesit-search-subtree "treesit.c") after the last one at the top of the file.

> +
> +(defun go-ts-mode--struct-node-p (node)
> + "Return t if NODE is a struct."
> + (and
> + (string-equal "type_declaration" (treesit-node-type node))
> + (treesit-search-subtree node "struct_type" nil nil 2)))
> +
> +(defun go-ts-mode--alias-node-p (node)
> + "Return t if NODE is a type alias."
> + (and
> + (string-equal "type_declaration" (treesit-node-type node))
> + (treesit-search-subtree node "type_alias" nil nil 1)))
> +
> +(defun go-ts-mode--other-type-node-p (node)
> + "Return t if NODE is a type, other than interface or struct."
> + (and
> + (string-equal "type_declaration" (treesit-node-type node))
> + (not (go-ts-mode--interface-node-p node))
> + (not (go-ts-mode--struct-node-p node))))

Here I guess we just need a (not alias) (and the docstring updated) to fix the issue mentioned above.

> +
> ;; go.mod support.
> 
> (defvar go-mod-ts-mode--syntax-table
>

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.