Implementation direction for shell-script-mode with tree-sitter

Implementation direction for shell-script-mode with tree-sitter

[João Paulo Labegalini de Carvalho]

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

Hi,

The tree-sitter-bash grammar does not include many reserved words and
builtin commands that are currently fontified by the regex based
fontication in shell-script-mode.

Here a list of the ones that tree-sitter-bash does not recognize:

("time" "coproc" "type" "trap" "exit" "exec" "continue" "break" "return"
"logout" "bye")

According to the Bash Reference Manual, all of the above are reserved words.

Should I make a PR to tree-sitter-bash to incorporate the missing keywords
or should I just filter them out of the list that I obtain through (and
other variables in `shell-script-mode'):

(append (sh-feature sh-leading-keywords)
        (sh-feature sh-other-keywords))

I am attaching the patch so everyone can see code and understand better
what I did. I welcome all criticism and feedback.

PS.: I am looking at the tree-sitter-bash and it does not seem very
complicated to extend it to recognize the missing keywords. But I can
definitely keep working independently of that.
-- 
João Paulo L. de Carvalho
Ph.D Computer Science |  IC-UNICAMP | Campinas , SP - Brazil
Postdoctoral Research Fellow | University of Alberta | Edmonton, AB - Canada
joao.carvalho@ic.unicamp.br
joao.carvalho@ualberta.ca

[-- Attachment #1.2: Type: text/html, Size: 1893 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: sh-script-treesit.patch --]
[-- Type: text/x-patch; charset="x-binaryenc"; name="sh-script-treesit.patch", Size: 5339 bytes --]

^[[33mdiff --git a/lisp/progmodes/sh-script.el b/lisp/progmodes/sh-script.el^[[m
^[[33mindex 558b62b20a..c7cc676843 100644^[[m
^[[33m--- a/lisp/progmodes/sh-script.el^[[m
^[[33m+++ b/lisp/progmodes/sh-script.el^[[m
^[[36m@@ -148,6 +148,7 @@^[[m
   (require 'let-alist)^[[m
   (require 'subr-x))^[[m
 (require 'executable)^[[m
^[[32m+^[[m^[[32m(require 'treesit)^[[m
 ^[[m
 (autoload 'comint-completion-at-point "comint")^[[m
 (autoload 'comint-filename-completion "comint")^[[m
^[[36m@@ -170,6 +171,12 @@^[[m ^[[msh-script^[[m
   :group 'sh^[[m
   :prefix "sh-")^[[m
 ^[[m
^[[32m+^[[m^[[32m(defcustom sh-script-use-tree-sitter nil^[[m
^[[32m+^[[m^[[32m  "If non-nil, `sh-script-mode' tries to use tree-sitter.^[[m
^[[32m+^[[m^[[32mCurrently `sh-script-mode' uses tree-sitter for font-locking, imenu,^[[m
^[[32m+^[[m^[[32mand movement functions."^[[m
^[[32m+^[[m^[[32m  :type 'boolean^[[m
^[[32m+^[[m^[[32m  :version "29.1")^[[m
 ^[[m
 (defcustom sh-ancestor-alist^[[m
   '((ash . sh)^[[m
^[[36m@@ -1534,13 +1541,24 @@^[[m ^[[msh-mode^[[m
   ;; we can't look if previous line ended with `\'^[[m
   (setq-local comint-prompt-regexp "^[ \t]*")^[[m
   (setq-local imenu-case-fold-search nil)^[[m
^[[31m-  (setq font-lock-defaults^[[m
^[[31m-	`((sh-font-lock-keywords^[[m
^[[31m-	   sh-font-lock-keywords-1 sh-font-lock-keywords-2)^[[m
^[[31m-	  nil nil^[[m
^[[31m-	  ((?/ . "w") (?~ . "w") (?. . "w") (?- . "w") (?_ . "w")) nil^[[m
^[[31m-	  (font-lock-syntactic-face-function^[[m
^[[31m-	   . ,#'sh-font-lock-syntactic-face-function)))^[[m
^[[32m+^[[m
^[[32m+^[[m^[[32m  (if (and sh-script-use-tree-sitter^[[m
^[[32m+^[[m^[[32m           (treesit-can-enable-p))^[[m
^[[32m+^[[m^[[32m      (progn^[[m
^[[32m+^[[m^[[32m        (setq-local font-lock-keywords-only t)^[[m
^[[32m+^[[m^[[32m        (setq-local treesit-font-lock-feature-list^[[m
^[[32m+^[[m^[[32m                    '((basic) (moderate) (elaborate)))^[[m
^[[32m+^[[m^[[32m        (setq-local treesit-font-lock-settings^[[m
^[[32m+^[[m^[[32m                    sh-script--treesit-settings)^[[m
^[[32m+^[[m^[[32m        (treesit-font-lock-enable))^[[m
^[[32m+^[[m^[[32m    (setq font-lock-defaults^[[m
^[[32m+^[[m^[[32m          `((sh-font-lock-keywords^[[m
^[[32m+^[[m^[[32m             sh-font-lock-keywords-1 sh-font-lock-keywords-2)^[[m
^[[32m+^[[m^[[32m            nil nil^[[m
^[[32m+^[[m^[[32m            ((?/ . "w") (?~ . "w") (?. . "w") (?- . "w") (?_ . "w")) nil^[[m
^[[32m+^[[m^[[32m            (font-lock-syntactic-face-function^[[m
^[[32m+^[[m^[[32m             . ,#'sh-font-lock-syntactic-face-function))))^[[m
^[[32m+^[[m
   (setq-local syntax-propertize-function #'sh-syntax-propertize-function)^[[m
   (add-hook 'syntax-propertize-extend-region-functions^[[m
             #'syntax-propertize-multiline 'append 'local)^[[m
^[[36m@@ -3191,6 +3209,51 @@^[[m ^[[msh-shellcheck-flymake^[[m
       (process-send-region sh--shellcheck-process (point-min) (point-max))^[[m
       (process-send-eof sh--shellcheck-process))))^[[m
 ^[[m
^[[31m-(provide 'sh-script)^[[m
^[[32m+^[[m^[[32m;;; Tree-sitter font-lock^[[m
^[[32m+^[[m
^[[32m+^[[m^[[32m(defvar sh-script--treesit-bash-keywords^[[m
^[[32m+^[[m^[[32m  '("case" "do" "done" "elif" "else" "esac" "export" "fi" "for"^[[m
^[[32m+^[[m^[[32m    "function" "if" "in" "unset" "while" "then"))^[[m
^[[32m+^[[m
^[[32m+^[[m^[[32m(defun sh-script--treesit-filtered-keywords (blacklist)^[[m
^[[32m+^[[m^[[32m  "Docstring goes here"^[[m
^[[32m+^[[m^[[32m  (let ((keywords (append (sh-feature sh-leading-keywords)^[[m
^[[32m+^[[m^[[32m                          (sh-feature sh-other-keywords)))^[[m
^[[32m+^[[m^[[32m        (filtered-list))^[[m
^[[32m+^[[m^[[32m    (dolist (item keywords filtered-list)^[[m
^[[32m+^[[m^[[32m      (if (not (member item blacklist))^[[m
^[[32m+^[[m^[[32m          (setq filtered-list (cons item filtered-list))^[[m
^[[32m+^[[m^[[32m        nil))))^[[m
^[[32m+^[[m
^[[32m+^[[m^[[32m(defvar sh-script--treesit-blacklisted-keywords^[[m
^[[32m+^[[m^[[32m  "Docstring goes here"^[[m
^[[32m+^[[m^[[32m  '("time" "coproc" "type" "trap" "exit" "exec" "continue" "break"^[[m
^[[32m+^[[m^[[32m  "return" "logout" "bye"))^[[m
^[[32m+^[[m
^[[32m+^[[m^[[32m(defvar sh-script--treesit-settings^[[m
^[[32m+^[[m^[[32m  (treesit-font-lock-rules^[[m
^[[32m+^[[m^[[32m   :language 'bash^[[m
^[[32m+^[[m^[[32m   :feature 'basic^[[m
^[[32m+^[[m^[[32m   '(;; Queries for function, strings, comments, and heredocs^[[m
^[[32m+^[[m^[[32m     (function_definition name: (word) @font-lock-function-name-face)^[[m
^[[32m+^[[m^[[32m     (comment) @font-lock-comment-face^[[m
^[[32m+^[[m^[[32m     [ (string) (raw_string)(heredoc_body) (heredoc_start) ] @font-lock-string-face)^[[m
^[[32m+^[[m^[[32m   :language 'bash^[[m
^[[32m+^[[m^[[32m   :feature 'moderate^[[m
^[[32m+^[[m^[[32m   :override t^[[m
^[[32m+^[[m^[[32m   `(;; Queries for keywords and builtin commands^[[m
^[[32m+^[[m^[[32m     [ ,@(sh-script--treesit-filtered-keywords sh-script--blacklisted-keywords) ] @font-lock-keyword-face^[[m
^[[32m+^[[m^[[32m     (command name: (command_name^[[m
^[[32m+^[[m^[[32m      ((word) @font-lock-builtin-face^[[m
^[[32m+^[[m^[[32m       (:match ,(let ((builtins (sh-feature sh-builtins)))^[[m
^[[32m+^[[m^[[32m                  (rx-to-string^[[m
^[[32m+^[[m^[[32m                   `(seq bol^[[m
^[[32m+^[[m^[[32m                         (or ,@builtins)^[[m
^[[32m+^[[m^[[32m                         eol)))^[[m
^[[32m+^[[m^[[32m               @font-lock-builtin-face))))^[[m
^[[32m+^[[m^[[32m     )^[[m
^[[32m+^[[m^[[32m   )^[[m
^[[32m+^[[m^[[32m  "Tree-sitter font-lock settings.")^[[m
 ^[[m
^[[32m+^[[m^[[32m(provide 'sh-script)^[[m
 ;;; sh-script.el ends here^[[m

Re: Implementation direction for shell-script-mode with tree-sitter

[Stefan Monnier]

> The tree-sitter-bash grammar does not include many reserved words and
> builtin commands that are currently fontified by the regex based
> fontication in shell-script-mode.
>
> Here a list of the ones that tree-sitter-bash does not recognize:
>
> ("time" "coproc" "type" "trap" "exit" "exec" "continue" "break" "return"
> "logout" "bye")

AFAIK `time` is clearly a bug in the tree-sitter grammar.
E.g. it probably causes a misparse for things like:

    time while ... do ... done

The same probably holds for `coproc`.
I suspect that for the other ones it doesn't make much difference, OTOH.
So maybe the other ones were deliberately omitted to keep
the grammar simpler.


        Stefan

Re: Implementation direction for shell-script-mode with tree-sitter

[João Paulo Labegalini de Carvalho]

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

> AFAIK `time` is clearly a bug in the tree-sitter grammar.
> E.g. it probably causes a misparse for things like:
>
>     time while ... do ... done
>

That was my impression as well.

The same probably holds for `coproc`.
> I suspect that for the other ones it doesn't make much difference, OTOH.
> So maybe the other ones were deliberately omitted to keep
> the grammar simpler.
>

So maybe I should just use a more generic query and use a regex to fontify
the keywords. Something like this:

(command_name ((word) @font-lock-keyword-face
               (:match <rx string regex here> @font-lock-keyword-face)))

-- 
João Paulo L. de Carvalho
Ph.D Computer Science |  IC-UNICAMP | Campinas , SP - Brazil
Postdoctoral Research Fellow | University of Alberta | Edmonton, AB - Canada
joao.carvalho@ic.unicamp.br
joao.carvalho@ualberta.ca

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

Re: Implementation direction for shell-script-mode with tree-sitter

[Po Lu]

João Paulo Labegalini de Carvalho <jaopaulolc@gmail.com> writes:

> Hi,
>
> The tree-sitter-bash grammar does not include many reserved words and
> builtin commands that are currently fontified by the regex based
> fontication in shell-script-mode.
>
> Here a list of the ones that tree-sitter-bash does not recognize:
>
> ("time" "coproc" "type" "trap" "exit" "exec" "continue" "break" "return" "logout" "bye")
>
> According to the Bash Reference Manual, all of the above are reserved words.
>
> Should I make a PR to tree-sitter-bash to incorporate the missing
> keywords or should I just filter them out of the list that I obtain
> through (and other variables in `shell-script-mode'):
>
> (append (sh-feature sh-leading-keywords)
>         (sh-feature sh-other-keywords))
>
> I am attaching the patch so everyone can see code and understand
> better what I did. I welcome all criticism and feedback.
>
> PS.: I am looking at the tree-sitter-bash and it does not seem very
> complicated to extend it to recognize the missing keywords. But I can
> definitely keep working independently of that.

N.B that shell-script-mode supports shells other than bash, so maybe a
tree-sitter-bash based variant should be spun off into its own
`bash-mode', as opposed to possibly interfering with the support for all
of rest?

Re: Implementation direction for shell-script-mode with tree-sitter

[João Paulo Labegalini de Carvalho]

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

N.B that shell-script-mode supports shells other than bash, so maybe a
> tree-sitter-bash based variant should be spun off into its own
> `bash-mode', as opposed to possibly interfering with the support for all
> of rest?
>

That is a very good point and that is the reason why I am pursuing an
implementation that reuses the work from the existing fontification code.

My goal is to use the `sh-feature' function to retrieve everything that
needs to be fontified for the buffer's shell variant.

It has worked well for most built-in commands (e.g. alias & source) but not
as great for reserved words like "time" and "coproc". As Stefen pointed
out, some reserved words complicate the grammar, so I believe that is the
reason why the tree-sitter-bash folks decided to keep them out of the
grammar.

This becomes a nuisance because then I have to use two queries to match
keywords. One simple query to match the recognized keywords and another
that matches all commands but only fontifies the ones that belong to the
un-recognized list. To build such a list I have to explicitly construct a
list of recognized keywords using literals and that goes against my goal of
reusing pre-existing functionality by relying on `sh-feature'.

Built-in commands are a lesser nuisance as some of them (e.g. local,
declare, and typeset) are not commands but declaration_commands in
tree-sitter-bash grammar. But for those I just have two queries and I don't
need to create a variable per shell variant.

Aside from that, I am trying to extract the keywords in `sh-font-lock-{var,
var-1, var-2}' variables to replicate the fontification based on the level
selected by the user. But parsing those is more intricate as the return is
a list where each element is either a list of the form:

   - (regex level font-face), or
   - (regex list [list]), where list is of the form (level face) or (level
   function function-args).

The logic to parse those I believe exists within font-lock-mode or
font-core, but I am not sure if I would be able to use the forms above as
they use functions that depend on variables that the user might tweak. I
don't know if the compiled queries in *-treesit-settings would be
recompiled to achieve the same flexibility as the existing fontification
code.

-- 
João Paulo L. de Carvalho
Ph.D Computer Science |  IC-UNICAMP | Campinas , SP - Brazil
Postdoctoral Research Fellow | University of Alberta | Edmonton, AB - Canada
joao.carvalho@ic.unicamp.br
joao.carvalho@ualberta.ca

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

Re: Implementation direction for shell-script-mode with tree-sitter

[Po Lu]

João Paulo Labegalini de Carvalho <jaopaulolc@gmail.com> writes:

> That is a very good point and that is the reason why I am pursuing an
> implementation that reuses the work from the existing fontification
> code.

Some shells, such as csh, are very syntactically different from
bash.

> My goal is to use the `sh-feature' function to retrieve everything
> that needs to be fontified for the buffer's shell variant.

So won't you end up, in effect, reproducing the existing font-lock code?

Re: Implementation direction for shell-script-mode with tree-sitter

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org,  Yuan Fu <casouri@gmail.com>,  Theodor Thornhill
>  <theo@thornhill.no>,  Eli Zaretskii <eliz@gnu.org>
> Date: Thu, 27 Oct 2022 08:54:01 +0800
> 
> > My goal is to use the `sh-feature' function to retrieve everything
> > that needs to be fontified for the buffer's shell variant.
> 
> So won't you end up, in effect, reproducing the existing font-lock code?

If that's so, I personally am not worried.

From where I stand, the single most important improvement from using
tree-sitter in shell-mode buffers is to solve the problems with
fontifications in tricky cases like here-documents etc.  These are a
source of constant stream of bug reports, when people expect us to do
a decent job in those cases.  I hope tree-sitter could solve that once
and for all.

Re: Implementation direction for shell-script-mode with tree-sitter

[João Paulo Labegalini de Carvalho]

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

>
> From where I stand, the single most important improvement from using
> tree-sitter in shell-mode buffers is to solve the problems with
> fontifications in tricky cases like here-documents etc.  These are a
> source of constant stream of bug reports, when people expect us to do
> a decent job in those cases.  I hope tree-sitter could solve that once
> and for all.
>

Eli, if you could send some of those bug reports, that would be great. I
can use them as test cases for what I am working on.
-- 
João Paulo L. de Carvalho
Ph.D Computer Science |  IC-UNICAMP | Campinas , SP - Brazil
Postdoctoral Research Fellow | University of Alberta | Edmonton, AB - Canada
joao.carvalho@ic.unicamp.br
joao.carvalho@ualberta.ca

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

Re: Implementation direction for shell-script-mode with tree-sitter

[Eli Zaretskii]

> From: João Paulo Labegalini de Carvalho <jaopaulolc@gmail.com>
> Date: Thu, 27 Oct 2022 08:23:12 -0600
> Cc: Po Lu <luangruo@yahoo.com>, emacs-devel@gnu.org, casouri@gmail.com, 
> 	theo@thornhill.no
> 
>  From where I stand, the single most important improvement from using
>  tree-sitter in shell-mode buffers is to solve the problems with
>  fontifications in tricky cases like here-documents etc.  These are a
>  source of constant stream of bug reports, when people expect us to do
>  a decent job in those cases.  I hope tree-sitter could solve that once
>  and for all.
> 
> Eli, if you could send some of those bug reports, that would be great. I can use them as test cases for what I
> am working on.

I don't have them ready, sorry.  You will need to search the debbugs
archives.

Re: Implementation direction for shell-script-mode with tree-sitter

[João Paulo Labegalini de Carvalho]

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

>
> Some shells, such as csh, are very syntactically different from
> bash.
>

That should not be a problem for tree-sitter as, once the csh tree-sitter
library is loaded, the queries will only match the particular keywords and
built-in commands of csh.

However, I am able to test other shell variants as there is no tree-sitter
grammar for them. The only one I have access to is for bash, but as I said
I am trying to keep the implementation as generic as possible so, once
those grammars are available the effort to use them should be low.


> So won't you end up, in effect, reproducing the existing font-lock code?
>

That seems to be the case for the keyword fontification generated from the
`sh-font-lock-var*' variables. The only way I found to avoid this is to
explicitly specify the same regex queries for fontification used currently
in sh-mode in the tree-sitter queries. This will create duplication of data
but avoids the undesirable duplication of font-lock code.

-- 
João Paulo L. de Carvalho
Ph.D Computer Science |  IC-UNICAMP | Campinas , SP - Brazil
Postdoctoral Research Fellow | University of Alberta | Edmonton, AB - Canada
joao.carvalho@ic.unicamp.br
joao.carvalho@ualberta.ca

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