bug#57884: [PATCH] Flymake backend using the shellcheck program

bug#57884: [PATCH] Flymake backend using the shellcheck program

[Augusto Stoffel]

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

Tags: patch

See https://www.shellcheck.net/ for details on the ShellCheck program.
It seems to be the most usual linter for shell scripts.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-New-Flymake-backend-using-the-shellcheck-program.patch --]
[-- Type: text/patch, Size: 4613 bytes --]

From 7571d833179fa49871a23607dc34f17d380eb147 Mon Sep 17 00:00:00 2001
From: Augusto Stoffel <arstoffel@gmail.com>
Date: Sat, 17 Sep 2022 18:30:04 +0200
Subject: [PATCH] New Flymake backend using the shellcheck program

* lisp/progmodes/sh-script.el (sh-shellcheck-program,
sh--shellcheck-process, sh-shellcheck-flymake): Variables and function
defining a Flymake backend.
(sh-mode): Add it to 'flymake-diagnostic-functions'.
---
 lisp/progmodes/sh-script.el | 70 +++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/lisp/progmodes/sh-script.el b/lisp/progmodes/sh-script.el
index 517fbbd8e7..242afccb3f 100644
--- a/lisp/progmodes/sh-script.el
+++ b/lisp/progmodes/sh-script.el
@@ -31,6 +31,9 @@
 ;; available for filenames, variables known from the script, the shell and
 ;; the environment as well as commands.
 
+;; A Flymake backend using the ShellCheck program is provided.  See
+;; https://www.shellcheck.net/ for installation instructions.
+
 ;;; Known Bugs:
 
 ;; - In Bourne the keyword `in' is not anchored to case, for, select ...
@@ -1580,6 +1583,7 @@ sh-mode
 	 ((equal (file-name-nondirectory buffer-file-name) ".profile") "sh")
          (t sh-shell-file))
    nil nil)
+  (add-hook 'flymake-diagnostic-functions #'sh-shellcheck-flymake nil t)
   (add-hook 'hack-local-variables-hook
     #'sh-after-hack-local-variables nil t))
 
@@ -3103,6 +3107,72 @@ sh-delete-backslash
  	    (delete-region (1+ (point))
  			   (progn (skip-chars-backward " \t") (point)))))))
 
+;;; Flymake backend
+
+(defcustom sh-shellcheck-command '("shellcheck")
+  "The shellcheck program followed by extra arguments."
+  :type 'string)
+
+(defvar-local sh--shellcheck-process nil)
+
+(defun sh-shellcheck-flymake (report-fn &rest _args)
+  "Flymake backend using the shellcheck program.
+Takes a Flymake callback REPORT-FN as argument, as expected of a
+member of `flymake-diagnostic-functions'."
+  (when (process-live-p sh--shellcheck-process)
+    (kill-process sh--shellcheck-process))
+  (let* ((source (current-buffer))
+         (dialect (named-let dialect ((s sh-shell))
+                    (pcase s
+                      ((or 'bash 'dash 'sh) (symbol-name s))
+                      ('ksh88 "ksh")
+                      ((guard s)
+                       (dialect (alist-get s sh-ancestor-alist))))))
+         (pattern "^-:\\([0-9]+\\):\\([0-9]+\\): \\([^:]+\\): \\(.*\\)$")
+         (sentinel
+          (lambda (proc _event)
+            (when (memq (process-status proc) '(exit signal))
+              (unwind-protect
+                  (if (with-current-buffer source
+                        (not (eq proc sh--shellcheck-process)))
+                      (flymake-log :warning "Canceling obsolete check %s" proc)
+                    (with-current-buffer (process-buffer proc)
+                      (goto-char (point-min))
+                      (cl-loop
+                       while (search-forward-regexp pattern nil t)
+                       for msg = (match-string 4)
+                       for (beg . end) = (flymake-diag-region
+                                          source
+                                          (string-to-number (match-string 1))
+                                          (string-to-number (match-string 2)))
+                       for type = (pcase (match-string 3)
+                                    ("error" :error)
+                                    ("warning" :warning)
+                                    (_ :note))
+                       when (and beg end)
+                       collect (flymake-make-diagnostic source beg end type msg)
+                       into diags
+                       finally (funcall report-fn diags))))
+                (kill-buffer (process-buffer proc)))))))
+    (unless dialect
+      (error "`sh-shellcheck-flymake' is not suitable for shell type `%s'"
+             sh-shell))
+    (setq sh--shellcheck-process
+          (make-process
+           :name "luacheck" :noquery t :connection-type 'pipe
+           :buffer (generate-new-buffer " *flymake-luacheck*")
+           :command `(,(car sh-shellcheck-command)
+                      "--format=gcc"
+                      "--color=never"
+                      "-s" ,dialect
+                      ,@(cdr sh-shellcheck-command)
+                      "-")
+           :sentinel sentinel))
+    (save-restriction
+      (widen)
+      (process-send-region sh--shellcheck-process (point-min) (point-max))
+      (process-send-eof sh--shellcheck-process))))
+
 (provide 'sh-script)
 
 ;;; sh-script.el ends here
-- 
2.37.3

bug#57884: [PATCH] Flymake backend using the shellcheck program

[Eli Zaretskii]

> From: Augusto Stoffel <arstoffel@gmail.com>
> Date: Sat, 17 Sep 2022 18:48:19 +0200
> 
> See https://www.shellcheck.net/ for details on the ShellCheck program.
> It seems to be the most usual linter for shell scripts.

Thanks.

> * lisp/progmodes/sh-script.el (sh-shellcheck-program,
> sh--shellcheck-process, sh-shellcheck-flymake): Variables and function
> defining a Flymake backend.
> (sh-mode): Add it to 'flymake-diagnostic-functions'.

Please mention the bug number in the log message.

> +(defcustom sh-shellcheck-command '("shellcheck")
> +  "The shellcheck program followed by extra arguments."
> +  :type 'string)

This lacks the :version tag.

Also, wouldn't it be better to have separate user options for the
program and the switches?  That way, there won't be a need to require
users to provide a list as the value of the option.

> +(defun sh-shellcheck-flymake (report-fn &rest _args)
> +  "Flymake backend using the shellcheck program.

The first line of a function's doc string should mention its
arguments.

And I think this warrants a NEWS entry.

bug#57884: [PATCH] Flymake backend using the shellcheck program

[Augusto Stoffel]

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

I've attached a new patch.  See my comments below.

On Sat, 17 Sep 2022 at 20:05, Eli Zaretskii wrote:

>> From: Augusto Stoffel <arstoffel@gmail.com>
>> Date: Sat, 17 Sep 2022 18:48:19 +0200
>> 
>> See https://www.shellcheck.net/ for details on the ShellCheck program.
>> It seems to be the most usual linter for shell scripts.
>
> Thanks.
>
>> * lisp/progmodes/sh-script.el (sh-shellcheck-program,
>> sh--shellcheck-process, sh-shellcheck-flymake): Variables and function
>> defining a Flymake backend.
>> (sh-mode): Add it to 'flymake-diagnostic-functions'.
>
> Please mention the bug number in the log message.

Done.

>> +(defcustom sh-shellcheck-command '("shellcheck")
>> +  "The shellcheck program followed by extra arguments."
>> +  :type 'string)
>
> This lacks the :version tag.

Done (and fixed the :type).

> Also, wouldn't it be better to have separate user options for the
> program and the switches?  That way, there won't be a need to require
> users to provide a list as the value of the option.

I could split this into two defcustoms if you feel strongly about it,
but it seems a bit of overengineering to me.  Not many people will want
to customize the program name, and the switches would have to be
provided as a list anyway, since we're not going to call this through a
shell.

Let me know what you think.

>> +(defun sh-shellcheck-flymake (report-fn &rest _args)
>> +  "Flymake backend using the shellcheck program.
>
> The first line of a function's doc string should mention its
> arguments.

It didn't fit the first line, so as per standard procedure it's in the
body of the docstring.

> And I think this warrants a NEWS entry.

Done.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-New-Flymake-backend-using-the-shellcheck-program.patch --]
[-- Type: text/x-patch, Size: 5107 bytes --]

From 7598653c31bc72a30bc7ed7ba28cc9b6c6b6c843 Mon Sep 17 00:00:00 2001
From: Augusto Stoffel <arstoffel@gmail.com>
Date: Sat, 17 Sep 2022 18:30:04 +0200
Subject: [PATCH] New Flymake backend using the shellcheck program

See bug#57884.

* lisp/progmodes/sh-script.el (sh-shellcheck-program,
sh--shellcheck-process, sh-shellcheck-flymake): Variables and function
defining a Flymake backend.
(sh-mode): Add it to 'flymake-diagnostic-functions'.
---
 etc/NEWS                    |  5 +++
 lisp/progmodes/sh-script.el | 71 +++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+)

diff --git a/etc/NEWS b/etc/NEWS
index a6a8883593..60e171ad13 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1320,6 +1320,11 @@ This controls how statements like the following are indented:
     foo &&
         bar
 
++++
+*** New Flymake backend using the ShellCheck program
+It is enable by default, only requiring that ShellCheck is installed.
+See https://www.shellcheck.net/ for details.
+
 ** Cperl Mode
 
 ---
diff --git a/lisp/progmodes/sh-script.el b/lisp/progmodes/sh-script.el
index 517fbbd8e7..9c0f6dabd5 100644
--- a/lisp/progmodes/sh-script.el
+++ b/lisp/progmodes/sh-script.el
@@ -31,6 +31,9 @@
 ;; available for filenames, variables known from the script, the shell and
 ;; the environment as well as commands.
 
+;; A Flymake backend using the ShellCheck program is provided.  See
+;; https://www.shellcheck.net/ for installation instructions.
+
 ;;; Known Bugs:
 
 ;; - In Bourne the keyword `in' is not anchored to case, for, select ...
@@ -1580,6 +1583,7 @@ sh-mode
 	 ((equal (file-name-nondirectory buffer-file-name) ".profile") "sh")
          (t sh-shell-file))
    nil nil)
+  (add-hook 'flymake-diagnostic-functions #'sh-shellcheck-flymake nil t)
   (add-hook 'hack-local-variables-hook
     #'sh-after-hack-local-variables nil t))
 
@@ -3103,6 +3107,73 @@ sh-delete-backslash
  	    (delete-region (1+ (point))
  			   (progn (skip-chars-backward " \t") (point)))))))
 
+;;; Flymake backend
+
+(defcustom sh-shellcheck-command '("shellcheck")
+  "The shellcheck program followed by extra arguments."
+  :type '(repeat string)
+  :version "29.1")
+
+(defvar-local sh--shellcheck-process nil)
+
+(defun sh-shellcheck-flymake (report-fn &rest _args)
+  "Flymake backend using the shellcheck program.
+Takes a Flymake callback REPORT-FN as argument, as expected of a
+member of `flymake-diagnostic-functions'."
+  (when (process-live-p sh--shellcheck-process)
+    (kill-process sh--shellcheck-process))
+  (let* ((source (current-buffer))
+         (dialect (named-let dialect ((s sh-shell))
+                    (pcase s
+                      ((or 'bash 'dash 'sh) (symbol-name s))
+                      ('ksh88 "ksh")
+                      ((guard s)
+                       (dialect (alist-get s sh-ancestor-alist))))))
+         (pattern "^-:\\([0-9]+\\):\\([0-9]+\\): \\([^:]+\\): \\(.*\\)$")
+         (sentinel
+          (lambda (proc _event)
+            (when (memq (process-status proc) '(exit signal))
+              (unwind-protect
+                  (if (with-current-buffer source
+                        (not (eq proc sh--shellcheck-process)))
+                      (flymake-log :warning "Canceling obsolete check %s" proc)
+                    (with-current-buffer (process-buffer proc)
+                      (goto-char (point-min))
+                      (cl-loop
+                       while (search-forward-regexp pattern nil t)
+                       for msg = (match-string 4)
+                       for (beg . end) = (flymake-diag-region
+                                          source
+                                          (string-to-number (match-string 1))
+                                          (string-to-number (match-string 2)))
+                       for type = (pcase (match-string 3)
+                                    ("error" :error)
+                                    ("warning" :warning)
+                                    (_ :note))
+                       when (and beg end)
+                       collect (flymake-make-diagnostic source beg end type msg)
+                       into diags
+                       finally (funcall report-fn diags))))
+                (kill-buffer (process-buffer proc)))))))
+    (unless dialect
+      (error "`sh-shellcheck-flymake' is not suitable for shell type `%s'"
+             sh-shell))
+    (setq sh--shellcheck-process
+          (make-process
+           :name "luacheck" :noquery t :connection-type 'pipe
+           :buffer (generate-new-buffer " *flymake-luacheck*")
+           :command `(,(car sh-shellcheck-command)
+                      "--format=gcc"
+                      "--color=never"
+                      "-s" ,dialect
+                      ,@(cdr sh-shellcheck-command)
+                      "-")
+           :sentinel sentinel))
+    (save-restriction
+      (widen)
+      (process-send-region sh--shellcheck-process (point-min) (point-max))
+      (process-send-eof sh--shellcheck-process))))
+
 (provide 'sh-script)
 
 ;;; sh-script.el ends here
-- 
2.37.3

bug#57884: [PATCH] Flymake backend using the shellcheck program

[Eli Zaretskii]

> From: Augusto Stoffel <arstoffel@gmail.com>
> Cc: 57884@debbugs.gnu.org
> Date: Sat, 17 Sep 2022 19:32:44 +0200
> 
> > Also, wouldn't it be better to have separate user options for the
> > program and the switches?  That way, there won't be a need to require
> > users to provide a list as the value of the option.
> 
> I could split this into two defcustoms if you feel strongly about it,
> but it seems a bit of overengineering to me.  Not many people will want
> to customize the program name, and the switches would have to be
> provided as a list anyway, since we're not going to call this through a
> shell.
> 
> Let me know what you think.

If we don't expect users to customize this, maybe this shouldn't be a
defcustom at all?  If and when we learn about an alternative program,
we could at that time decide how best to allow its customization.  For
example, if both programs don't need any command-line switches, we
could offer a defcustom only for the program.

Anyway, let's hear what Lars and others think.

bug#57884: [PATCH] Flymake backend using the shellcheck program

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> If we don't expect users to customize this, maybe this shouldn't be a
> defcustom at all?  If and when we learn about an alternative program,
> we could at that time decide how best to allow its customization.

Makes sense.

> For example, if both programs don't need any command-line switches, we
> could offer a defcustom only for the program.

Maybe we should just make the flags into a defcustom, because there are
some useful ones like, from shellcheck(1):

   -i CODE1[,CODE2...], --include=CODE1[,CODE2...]
          Explicitly include only the specified codes in the report

   -e CODE1[,CODE2...], --exclude=CODE1[,CODE2...]
          Explicitly  exclude the specified codes from the report.

   --norc Don't try to look for .shellcheckrc configuration files.

   -s shell, --shell=shell
          Specify Bourne shell dialect.  Valid values are sh,  bash,  dash
          and  ksh.

bug#57884: [PATCH] Flymake backend using the shellcheck program

[Eli Zaretskii]

> From: Augusto Stoffel <arstoffel@gmail.com>
> Cc: 57884@debbugs.gnu.org
> Date: Sat, 17 Sep 2022 19:32:44 +0200
> 
> ++++
> +*** New Flymake backend using the ShellCheck program
> +It is enable by default, only requiring that ShellCheck is installed.
         ^^^^^^
"enabled"

> +(defun sh-shellcheck-flymake (report-fn &rest _args)
> +  "Flymake backend using the shellcheck program.

I'd use

  Flymake backend using the shellcheck program and REPORT-FN as callback.

bug#57884: [PATCH] Flymake backend using the shellcheck program

[Stefan Kangas]

Augusto Stoffel <arstoffel@gmail.com> writes:

> I could split this into two defcustoms if you feel strongly about it,
> but it seems a bit of overengineering to me.  Not many people will want
> to customize the program name, and the switches would have to be
> provided as a list anyway, since we're not going to call this through a
> shell.

FWIW, I tend to agree, unless we can point to any other "shellcheck"
compatible programs out there.  For now, the code assumes that's what
we're using, right?

> ++++

Should probably be "---" as you didn't add documentation for it.

> +*** New Flymake backend using the ShellCheck program
> +It is enable by default, only requiring that ShellCheck is installed.
> +See https://www.shellcheck.net/ for details.

(Typo: "enabled".)

I'd suggest:

It is enabled by default, but requires that the external "shellcheck"
command is installed.

> +;; A Flymake backend using the ShellCheck program is provided.  See
> +;; https://www.shellcheck.net/ for installation instructions.

Maybe this should be "shellcheck" too?  The capitalization is not
necessarily helpful for finding the correct package to install.

bug#57884: [PATCH] Flymake backend using the shellcheck program

[Augusto Stoffel]

On Sat, 17 Sep 2022 at 14:04, Stefan Kangas wrote:

> Augusto Stoffel <arstoffel@gmail.com> writes:
>
>> I could split this into two defcustoms if you feel strongly about it,
>> but it seems a bit of overengineering to me.  Not many people will want
>> to customize the program name, and the switches would have to be
>> provided as a list anyway, since we're not going to call this through a
>> shell.
>
> FWIW, I tend to agree, unless we can point to any other "shellcheck"
> compatible programs out there.  For now, the code assumes that's what
> we're using, right?

Yes, this Flymake backend is specific to this particular program because
1. the output pattern is probably more or less unique, 2. it needs a
dynamically computed argument and 3. Flymake allows several backends to
in parallel, so if a new linter comes into being, it's up to the user to
decide whether to run one or the other or both.

bug#57884: [PATCH] Flymake backend using the shellcheck program

[Philip Kaludercic]

Augusto Stoffel <arstoffel@gmail.com> writes:

>
> I could split this into two defcustoms if you feel strongly about it,
> but it seems a bit of overengineering to me.  Not many people will want
>to customize the program name, and the switches would have to be
> provided as a list anyway, since we're not going to call this through a
> shell.

I think it would be good, because then you could make the flags file
local in case you need something special, without having to worry about
some evil file that sets the command to

     '("rm" "-rf" "--no-preserve-root" "/")

> Let me know what you think.
>
>>> +(defun sh-shellcheck-fmake (report-fn &rest _args)
>>> +  "Flymake backend using the shellchecprogram.
>>
>> The first line of a function's doc string should mtion its
>> arguments.
>
> It didn't fit the first line, so as per standd proceu it's in the
> body of the dctring.
>
>> And I think this warrants a NEWS entry.
>
> Dne.
>
>>From 7598653c31bc72a30bc7ed7ba28cc9b6c6b6c843Mo Sep 17 00:00:00 2001
> From: Augusto Stoffel <arstoffel@gmail.om>
> Date: St17 Sep 2022 18:30:04 +0200
> Subject: [PATCH] New Flymake backend usinghe shellcheck program
>>See bug#57884.
>
> * lisp/progmodes/sh-scrpel (shsellcheck-program,
> sh--shellcheck-process, sh-shellcheck-flymake): Variles and function
> defining a Flymake backen
> (sh-mode): Add it to 'flymake-diagntic-functions'.
> ---
>  etc/NEWS                    |  5 +++
>  lsprogmodes/sh-sci.el | 71 +++++++++++++++++++++++++++++++++++++
>  2 fis changed, 76 insertions(+)
>
> diff --git a/etc/NEWS b/etc/NEWS
> indea6a8883593..60e171ad13 10064> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -1320,6 +1320, @@ is controls how statements like the foowing are indented:
>      foo &&
>          bar
>  
> ++++
> +*** New Fmake backend using the ShellCheck poam
> +It is enable by default, on requiring that ShellCheck is instald.
> +See https/www.shellchecket/ for details.
> +
>  ** Cperl Mode
>  
>  ---
> diff --git a/lisp/progmodes/sh-scrt.el b/lisp/ogmodes/sh-scptl
> iex 517fbbd8e7..9c0f6dabd5 100644
> --- a/lisp/progmodesh-script.el
> +++ b/lisp/progmodes/sh-script.el
> @@ -31,6 +31,9 @@
>  ;; available for filenames, variables known fromhe script, the shell and
>  ;; the environment as well as commands.
>  
> +;; A Flymake backend using the ShellCheck program is provided.  See
> +;; https://www.shellecnet/ for instalti instctions.
> +
>  ;;; Known Bugs:
>  
>  ;; - In Bourne the keyword `in' inot anchored to case, for, select ..> @@ -1580,6 +1583,7 @@ sh-mode
>  ((equal (file-name-nondirectory bfer-file-name) ".pfile") "sh")
>           (t sh-shell-file))
>     nil nil)
> +  (add-hook 'fmake-diagnostic-functions #'sh-shellchecklyke nil t)
>    (add-hook 'hack-local-variables-hook
>      #'sh-afterack-local-variables nil t))
>  
> @@ -3103,6 +3107,73 @@ sh-dele-ckslash
>   	    ele-region (1+ (point))
>   			   (progn (skip-chars-backward " \t") (point)))))
>  
> +;;; Flymake backend
> +
> +(defcustom sh-shellcheck-command '("shellcheck")
> +  "The shellcheck program followed by extra arguments."
> +  :type '(repeat string)
> +  :version "29.1")
> +
> +(defvar-local sh--shellcheck-process nil)
> +
> +(defun sh-shellcheck-flymake (report-fn &rest _args)
> +  "Flymake backend using the shellcheck program.
> +Takes a Flymake callback REPORT-FN as argument, as expected of a
> +member of `flymake-diagnostic-functions'."
> +  (when (process-live-p sh--shellcheck-process)
> +    (kill-process sh--shellcheck-process))
> +  (let* ((source (current-buffer))
> +         (dialect (named-let dialect ((s sh-shell))
> +                    (pcase s
> +                      ((or 'bash 'dash 'sh) (symbol-name s))
> +                      ('ksh88 "ksh")
> +                      ((guard s)
> +                       (dialect (alist-get s sh-ancestor-alist))))))

I don't use `named-let' that much, but calling both the result and the
recursive function `dialect' seems confusing to me.

> +         (pattern "^-:\\([0-9]+\\):\\([0-9]+\\): \\([^:]+\\): \\(.*\\)$")

Do you think that that using `rx' would make this pattern more maintainable?

> +         (sentinel
> +          (lambda (proc _event)

Wouldn't it be cleaner to pull this lambda out into a separate named function?

> +            (when (memq (process-status proc) '(exit signal))
> +              (unwind-protect
> +                  (if (with-current-buffer source
> +                        (not (eq proc sh--shellcheck-process)))
> +                      (flymake-log :warning "Canceling obsolete check %s" proc)
> +                    (with-current-buffer (process-buffer proc)
> +                      (goto-char (point-min))
> +                      (cl-loop
> +                       while (search-forward-regexp pattern nil t)
> +                       for msg = (match-string 4)
> +                       for (beg . end) = (flymake-diag-region
> +                                          source
> +                                          (string-to-number (match-string 1))
> +                                          (string-to-number (match-string 2)))
> +                       for type = (pcase (match-string 3)
> +                                    ("error" :error)
> +                                    ("warning" :warning)
> +                                    (_ :note))
> +                       when (and beg end)
> +                       collect (flymake-make-diagnostic source beg end type msg)
> +                       into diags
> +                       finally (funcall report-fn diags))))
> +                (kill-buffer (process-buffer proc)))))))
> +    (unless dialect
> +      (error "`sh-shellcheck-flymake' is not suitable for shell type `%s'"
> +             sh-shell))
> +    (setq sh--shellcheck-process
> +          (make-process
> +           :name "luacheck" :noquery t :connection-type 'pipe
                    ^
                    Typo?

> +           :buffer (generate-new-buffer " *flymake-luacheck*")
                                                      ^
                                                      same here
> +           :command `(,(car sh-shellcheck-command)
> +                      "--format=gcc"
> +                      "--color=never"
> +                      "-s" ,dialect
> +                      ,@(cdr sh-shellcheck-command)
> +                      "-")
> +           :sentinel sentinel))
> +    (save-restriction
> +      (widen)
> +      (process-send-region sh--shellcheck-process (point-min) (point-max))
> +      (process-send-eof sh--shellcheck-process))))
> +
>  (provide 'sh-script)
>  
>  ;;; sh-script.el ends here

bug#57884: [PATCH] Flymake backend using the shellcheck program

[Augusto Stoffel]

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

With the attached patch I believe I've addressed all comments from this
and previous messages.  (In case I missed some detail, the committer
should feel free to make any desired copyediting.)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-New-Flymake-backend-using-the-shellcheck-program.patch --]
[-- Type: text/x-patch, Size: 5186 bytes --]

From 1cadf763114eb733036a1c8182900fe2df17092c Mon Sep 17 00:00:00 2001
From: Augusto Stoffel <arstoffel@gmail.com>
Date: Sat, 17 Sep 2022 18:30:04 +0200
Subject: [PATCH] New Flymake backend using the shellcheck program

See bug#57884.

* lisp/progmodes/sh-script.el (sh-shellcheck-program,
sh--shellcheck-process, sh-shellcheck-flymake): Variables and function
defining a Flymake backend.
(sh-mode): Add it to 'flymake-diagnostic-functions'.
---
 etc/NEWS                    |  4 ++
 lisp/progmodes/sh-script.el | 76 +++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+)

diff --git a/etc/NEWS b/etc/NEWS
index a6a8883593..5b2ed16063 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1320,6 +1320,10 @@ This controls how statements like the following are indented:
     foo &&
         bar
 
+*** New Flymake backend using the ShellCheck program
+It is enabled by default, but requires that the external "shellcheck"
+command is installed.
+
 ** Cperl Mode
 
 ---
diff --git a/lisp/progmodes/sh-script.el b/lisp/progmodes/sh-script.el
index 517fbbd8e7..dd36f92766 100644
--- a/lisp/progmodes/sh-script.el
+++ b/lisp/progmodes/sh-script.el
@@ -31,6 +31,9 @@
 ;; available for filenames, variables known from the script, the shell and
 ;; the environment as well as commands.
 
+;; A Flymake backend using the "shellcheck" program is provided.  See
+;; https://www.shellcheck.net/ for installation instructions.
+
 ;;; Known Bugs:
 
 ;; - In Bourne the keyword `in' is not anchored to case, for, select ...
@@ -1580,6 +1583,7 @@ sh-mode
 	 ((equal (file-name-nondirectory buffer-file-name) ".profile") "sh")
          (t sh-shell-file))
    nil nil)
+  (add-hook 'flymake-diagnostic-functions #'sh-shellcheck-flymake nil t)
   (add-hook 'hack-local-variables-hook
     #'sh-after-hack-local-variables nil t))
 
@@ -3103,6 +3107,78 @@ sh-delete-backslash
  	    (delete-region (1+ (point))
  			   (progn (skip-chars-backward " \t") (point)))))))
 
+;;; Flymake backend
+
+(defcustom sh-shellcheck-program "shellcheck"
+  "Name of the shellcheck executable."
+  :type 'string
+  :version "29.1")
+
+(defcustom sh-shellcheck-arguments nil
+  "Additional arguments to the shellcheck program."
+  :type '(repeat string)
+  :version "29.1")
+
+(defvar-local sh--shellcheck-process nil)
+
+(defun sh-shellcheck-flymake (report-fn &rest _args)
+  "Flymake backend using the shellcheck program.
+Takes a Flymake callback REPORT-FN as argument, as expected of a
+member of `flymake-diagnostic-functions'."
+  (when (process-live-p sh--shellcheck-process)
+    (kill-process sh--shellcheck-process))
+  (let* ((source (current-buffer))
+         (dialect (named-let dialect ((s sh-shell))
+                    (pcase s
+                      ((or 'bash 'dash 'sh) (symbol-name s))
+                      ('ksh88 "ksh")
+                      ((guard s)
+                       (dialect (alist-get s sh-ancestor-alist))))))
+         (pattern "^-:\\([0-9]+\\):\\([0-9]+\\): \\([^:]+\\): \\(.*\\)$")
+         (sentinel
+          (lambda (proc _event)
+            (when (memq (process-status proc) '(exit signal))
+              (unwind-protect
+                  (if (with-current-buffer source
+                        (not (eq proc sh--shellcheck-process)))
+                      (flymake-log :warning "Canceling obsolete check %s" proc)
+                    (with-current-buffer (process-buffer proc)
+                      (goto-char (point-min))
+                      (cl-loop
+                       while (search-forward-regexp pattern nil t)
+                       for msg = (match-string 4)
+                       for (beg . end) = (flymake-diag-region
+                                          source
+                                          (string-to-number (match-string 1))
+                                          (string-to-number (match-string 2)))
+                       for type = (pcase (match-string 3)
+                                    ("error" :error)
+                                    ("warning" :warning)
+                                    (_ :note))
+                       when (and beg end)
+                       collect (flymake-make-diagnostic source beg end type msg)
+                       into diags
+                       finally (funcall report-fn diags))))
+                (kill-buffer (process-buffer proc)))))))
+    (unless dialect
+      (error "`sh-shellcheck-flymake' is not suitable for shell type `%s'"
+             sh-shell))
+    (setq sh--shellcheck-process
+          (make-process
+           :name "shellcheck" :noquery t :connection-type 'pipe
+           :buffer (generate-new-buffer " *flymake-shellcheck*")
+           :command `(,sh-shellcheck-program
+                      "--format=gcc"
+                      "--color=never"
+                      "-s" ,dialect
+                      ,@sh-shellcheck-arguments
+                      "-")
+           :sentinel sentinel))
+    (save-restriction
+      (widen)
+      (process-send-region sh--shellcheck-process (point-min) (point-max))
+      (process-send-eof sh--shellcheck-process))))
+
 (provide 'sh-script)
 
 ;;; sh-script.el ends here
-- 
2.37.3


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


On Sun, 18 Sep 2022 at 11:55, Philip Kaludercic wrote:

> Augusto Stoffel <arstoffel@gmail.com> writes:
>
>>
>> I could split this into two defcustoms if you feel strongly about it,
>> but it seems a bit of overengineering to me.  Not many people will want
>>to customize the program name, and the switches would have to be
>> provided as a list anyway, since we're not going to call this through a
>> shell.
>
> I think it would be good, because then you could make the flags file
> local in case you need something special, without having to worry about
> some evil file that sets the command to
>
>      '("rm" "-rf" "--no-preserve-root" "/")
>

Well, fine then.  I've split this into two variables.

> I don't use `named-let' that much, but calling both the result and the
> recursive function `dialect' seems confusing to me.
>

Welcome to Lisp-2...

>> +         (pattern "^-:\\([0-9]+\\):\\([0-9]+\\): \\([^:]+\\): \\(.*\\)$")
>
> Do you think that that using `rx' would make this pattern more maintainable?
>

I use rx often, but I think this is still at a level where rx doesn't
really help much.

>> +         (sentinel
>> +          (lambda (proc _event)
>
> Wouldn't it be cleaner to pull this lambda out into a separate named function?
>

That's not possible, it needs to be in that lexical scope.

>> +    (setq sh--shellcheck-process
>> +          (make-process
>> +           :name "luacheck" :noquery t :connection-type 'pipe
>                     ^
>                     Typo?
>
>> +           :buffer (generate-new-buffer " *flymake-luacheck*")
>                                                       ^
>                                                       same here

Ouch, fixed.

> Also what happens if someone doesn't have shellcheck installed?

Then Flymake logs a warning and disables the backend. The error message
will be whatever make-process gives, which I find more than good enough.

bug#57884: [PATCH] Flymake backend using the shellcheck program

[Philip Kaludercic]

Augusto Stoffel <arstoffel@gmail.com> writes:

> With the attached patch I believe I've addressed all comments from this
> and previous messages.  (In case I missed some detail, the committer
> should feel free to make any desired copyediting.)
>
>
>
> On Sun, 18 Sep 2022 at 11:55, Philip Kaludercic wrote:
>
>> Augusto Stoffel <arstoffel@gmail.com> writes:
>>
>>>
>>> I could split this into two defcustoms if you feel strongly about it,
>>> but it seems a bit of overengineering to me.  Not many people will want
>>>to customize the program name, and the switches would have to be
>>> provided as a list anyway, since we're not going to call this through a
>>> shell.
>>
>> I think it would be good, because then you could make the flags file
>> local in case you need something special, without having to worry about
>> some evil file that sets the command to
>>
>>      '("rm" "-rf" "--no-preserve-root" "/")
>>
>
> Well, fine then.  I've split this into two variables.
>
>> I don't use `named-let' that much, but calling both the result and the
>> recursive function `dialect' seems confusing to me.
>>
>
> Welcome to Lisp-2...

It is not so much that they share the same name, but that I don't
understand why you close to give the function the name "dialect".  From
what I have seen, named-let is usually given a self-referential call
like "next", "recur", "loop", etc.

>>> +         (pattern "^-:\\([0-9]+\\):\\([0-9]+\\): \\([^:]+\\): \\(.*\\)$")
>>
>> Do you think that that using `rx' would make this pattern more maintainable?
>>
>
> I use rx often, but I think this is still at a level where rx doesn't
> really help much.

OK.

>>> +         (sentinel
>>> +          (lambda (proc _event)
>>
>> Wouldn't it be cleaner to pull this lambda out into a separate named function?
>>
>
> That's not possible, it needs to be in that lexical scope.

Not if you use `process-put' and `process-get'.  I recently reworked
flymake-proselint and did the same thing.  IMO it doesn't look that bad:

https://git.sr.ht/~manuel-uberti/flymake-proselint/commit/30c4baa08db32e73d956c978c81a9f79062c2e1d

>>> +    (setq sh--shellcheck-process
>>> +          (make-process
>>> +           :name "luacheck" :noquery t :connection-type 'pipe
>>                     ^
>>                     Typo?
>>
>>> +           :buffer (generate-new-buffer " *flymake-luacheck*")
>>                                                       ^
>>                                                       same here
>
> Ouch, fixed.

1+

>> Also what happens if someone doesn't have shellcheck installed?
>
> Then Flymake logs a warning and disables the backend. The error message
> will be whatever make-process gives, which I find more than good enough.

But isn't that rather late to detect the error.  Couldn't you also check
if "shellcheck" (or whatever the new option name will be) can be found
in the path before adding the hook in `shell-mode'?

bug#57884: [PATCH] Flymake backend using the shellcheck program

[Augusto Stoffel]

On Sun, 18 Sep 2022 at 12:50, Philip Kaludercic wrote:

>>> I don't use `named-let' that much, but calling both the result and the
>>> recursive function `dialect' seems confusing to me.
>>>
>>
>> Welcome to Lisp-2...
>
> It is not so much that they share the same name, but that I don't
> understand why you close to give the function the name "dialect".  From
> what I have seen, named-let is usually given a self-referential call
> like "next", "recur", "loop", etc.

Yeah, I have no strong opinion about the name.

>>> Wouldn't it be cleaner to pull this lambda out into a separate named function?
>>>
>>
>> That's not possible, it needs to be in that lexical scope.
>
> Not if you use `process-put' and `process-get'.  I recently reworked
> flymake-proselint and did the same thing.  IMO it doesn't look that bad:
>
> https://git.sr.ht/~manuel-uberti/flymake-proselint/commit/30c4baa08db32e73d956c978c81a9f79062c2e1d

Honestly, I much prefer to have some state localized in a closure than
in what is effective a global variable...

I like in your backend that you read a JSON output, which presumably
provides the start _and end_ of the diagnostic region.  How did you
convert from line/column to buffer position?

I think it would be nice to extend flymake-diag-region to have signature

   (flymake-diag-region BUFFER LINE &optional COL LINE-END COL-END)

If you provide LINE-END and COL-END, then those are converted to buffer
positions and there's no guessing as to what they should be.

>>> Also what happens if someone doesn't have shellcheck installed?
>>
>> Then Flymake logs a warning and disables the backend. The error message
>> will be whatever make-process gives, which I find more than good enough.
>
> But isn't that rather late to detect the error.  Couldn't you also check
> if "shellcheck" (or whatever the new option name will be) can be found
> in the path before adding the hook in `shell-mode'?

I think this is worse than not doing anything.  If you don't check and
let the backend fail, it gets displayed as a disabled backend, and the
user has the chance to look at the log buffer and try to figure out why
it's not working.  If we did what we suggest, we'd have to reinvent the
wheel to convey that information to the user, I think.

bug#57884: [PATCH] Flymake backend using the shellcheck program

[Philip Kaludercic]

Augusto Stoffel <arstoffel@gmail.com> writes:

> On Sun, 18 Sep 2022 at 12:50, Philip Kaludercic wrote:
>
>>>> I don't use `named-let' that much, but calling both the result and the
>>>> recursive function `dialect' seems confusing to me.
>>>>
>>>
>>> Welcome to Lisp-2...
>>
>> It is not so much that they share the same name, but that I don't
>> understand why you close to give the function the name "dialect".  From
>> what I have seen, named-let is usually given a self-referential call
>> like "next", "recur", "loop", etc.
>
> Yeah, I have no strong opinion about the name.
>
>>>> Wouldn't it be cleaner to pull this lambda out into a separate named function?
>>>>
>>>
>>> That's not possible, it needs to be in that lexical scope.
>>
>> Not if you use `process-put' and `process-get'.  I recently reworked
>> flymake-proselint and did the same thing.  IMO it doesn't look that bad:
>>
>> https://git.sr.ht/~manuel-uberti/flymake-proselint/commit/30c4baa08db32e73d956c978c81a9f79062c2e1d
>
> Honestly, I much prefer to have some state localized in a closure than
> in what is effective a global variable...

Not quite, you can only access the value if you have access to the
process object.  I guess you can argue that the process object is stored
as a file local variable which sort of makes it global...

> I like in your backend that you read a JSON output, which presumably
> provides the start _and end_ of the diagnostic region.  How did you
> convert from line/column to buffer position?

I didn't do that, Flymake takes care of that if you give it the buffer
position (unless I misunderstood something, but it appears to be working
for me).

> I think it would be nice to extend flymake-diag-region to have signature
>
>    (flymake-diag-region BUFFER LINE &optional COL LINE-END COL-END)
>
> If you provide LINE-END and COL-END, then those are converted to buffer
> positions and there's no guessing as to what they should be.
>
>>>> Also what happens if someone doesn't have shellcheck installed?
>>>
>>> Then Flymake logs a warning and disables the backend. The error message
>>> will be whatever make-process gives, which I find more than good enough.
>>
>> But isn't that rather late to detect the error.  Couldn't you also check
>> if "shellcheck" (or whatever the new option name will be) can be found
>> in the path before adding the hook in `shell-mode'?
>
> I think this is worse than not doing anything.  If you don't check and
> let the backend fail, it gets displayed as a disabled backend, and the
> user has the chance to look at the log buffer and try to figure out why
> it's not working.  If we did what we suggest, we'd have to reinvent the
> wheel to convey that information to the user, I think.

On the other hand there might be people who don't care about shellcheck
(say they just open a shell script once a year) and any error message
would just be an annoyance.

The most important part is having the ability to use shellcheck
prominently documented, the rest is probably just a matter of taste.

bug#57884: [PATCH] Flymake backend using the shellcheck program

[Augusto Stoffel]

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

On Sun, 18 Sep 2022 at 13:46, Philip Kaludercic wrote:

>> I like in your backend that you read a JSON output, which presumably
>> provides the start _and end_ of the diagnostic region.  How did you
>> convert from line/column to buffer position?
>
> I didn't do that, Flymake takes care of that if you give it the buffer
> position (unless I misunderstood something, but it appears to be working
> for me).

Okay, perhaps the docstring of `flymake-make-diagnostic' is easy to
misinterpret, but apparently it works fine if one passes a (LINE . COL)
cons for BEG and END, even if LOCUS is a buffer.  This would also mean
that the call to `flymake-diag-region` in the example backend of the
Flymake manual is unnecessary, so I'm a bit suspicious here.

Anyway, I rewrote the backend to use the JSON output of shellcheck,
which has the advantage that it provides the end position of each
diagnostic, so Flymake doesn't have to guess it (which is by nature
sometimes inaccurate).  Let me know what you think.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-New-Flymake-backend-using-the-shellcheck-program.patch --]
[-- Type: text/x-patch, Size: 5799 bytes --]

From 49fbfaf98cb6d11b877eaa4a72b116aa61646d75 Mon Sep 17 00:00:00 2001
From: Augusto Stoffel <arstoffel@gmail.com>
Date: Sat, 17 Sep 2022 18:30:04 +0200
Subject: [PATCH] New Flymake backend using the shellcheck program

See bug#57884.

* lisp/progmodes/sh-script.el: Require let-alist and subr-x when
compiling.
(sh--json-read): Helper function to deal with possible absence of
json-parse-buffer.
(sh-shellcheck-program, sh--shellcheck-process,
sh-shellcheck-flymake): Variables and function defining a Flymake
backend.
(sh-mode): Add it to 'flymake-diagnostic-functions'.
---
 etc/NEWS                    |  4 ++
 lisp/progmodes/sh-script.el | 90 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 93 insertions(+), 1 deletion(-)

diff --git a/etc/NEWS b/etc/NEWS
index a6a8883593..5b2ed16063 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1320,6 +1320,10 @@ This controls how statements like the following are indented:
     foo &&
         bar
 
+*** New Flymake backend using the ShellCheck program
+It is enabled by default, but requires that the external "shellcheck"
+command is installed.
+
 ** Cperl Mode
 
 ---
diff --git a/lisp/progmodes/sh-script.el b/lisp/progmodes/sh-script.el
index 517fbbd8e7..558b62b20a 100644
--- a/lisp/progmodes/sh-script.el
+++ b/lisp/progmodes/sh-script.el
@@ -31,6 +31,9 @@
 ;; available for filenames, variables known from the script, the shell and
 ;; the environment as well as commands.
 
+;; A Flymake backend using the "shellcheck" program is provided.  See
+;; https://www.shellcheck.net/ for installation instructions.
+
 ;;; Known Bugs:
 
 ;; - In Bourne the keyword `in' is not anchored to case, for, select ...
@@ -141,7 +144,9 @@
 (eval-when-compile
   (require 'skeleton)
   (require 'cl-lib)
-  (require 'comint))
+  (require 'comint)
+  (require 'let-alist)
+  (require 'subr-x))
 (require 'executable)
 
 (autoload 'comint-completion-at-point "comint")
@@ -1580,6 +1585,7 @@ sh-mode
 	 ((equal (file-name-nondirectory buffer-file-name) ".profile") "sh")
          (t sh-shell-file))
    nil nil)
+  (add-hook 'flymake-diagnostic-functions #'sh-shellcheck-flymake nil t)
   (add-hook 'hack-local-variables-hook
     #'sh-after-hack-local-variables nil t))
 
@@ -3103,6 +3109,88 @@ sh-delete-backslash
  	    (delete-region (1+ (point))
  			   (progn (skip-chars-backward " \t") (point)))))))
 
+;;; Flymake backend
+
+(defcustom sh-shellcheck-program "shellcheck"
+  "Name of the shellcheck executable."
+  :type 'string
+  :version "29.1")
+
+(defcustom sh-shellcheck-arguments nil
+  "Additional arguments to the shellcheck program."
+  :type '(repeat string)
+  :version "29.1")
+
+(defvar-local sh--shellcheck-process nil)
+
+(defalias 'sh--json-read
+  (if (fboundp 'json-parse-buffer)
+      (lambda () (json-parse-buffer :object-type 'alist))
+    (require 'json)
+    'json-read))
+
+(defun sh-shellcheck-flymake (report-fn &rest _args)
+  "Flymake backend using the shellcheck program.
+Takes a Flymake callback REPORT-FN as argument, as expected of a
+member of `flymake-diagnostic-functions'."
+  (when (process-live-p sh--shellcheck-process)
+    (kill-process sh--shellcheck-process))
+  (let* ((source (current-buffer))
+         (dialect (named-let recur ((s sh-shell))
+                    (pcase s
+                      ((or 'bash 'dash 'sh) (symbol-name s))
+                      ('ksh88 "ksh")
+                      ((guard s)
+                       (recur (alist-get s sh-ancestor-alist))))))
+         (sentinel
+          (lambda (proc _event)
+            (when (memq (process-status proc) '(exit signal))
+              (unwind-protect
+                  (if (with-current-buffer source
+                        (not (eq proc sh--shellcheck-process)))
+                      (flymake-log :warning "Canceling obsolete check %s" proc)
+                    (with-current-buffer (process-buffer proc)
+                      (goto-char (point-min))
+                      (thread-last
+                        (sh--json-read)
+                        (alist-get 'comments)
+                        (seq-filter
+                         (lambda (item)
+                           (let-alist item (string= .file "-"))))
+                        (mapcar
+                         (lambda (item)
+                           (let-alist item
+                             (flymake-make-diagnostic
+                              source
+                              (cons .line .column)
+                              (unless (and (eq .line .endLine)
+                                           (eq .column .endColumn))
+                                (cons .endLine .endColumn))
+                              (pcase .level
+                                ("error" :error)
+                                ("warning" :warning)
+                                (_ :note))
+                              (format "SC%s: %s" .code .message)))))
+                        (funcall report-fn))))
+                (kill-buffer (process-buffer proc)))))))
+    (unless dialect
+      (error "`sh-shellcheck-flymake' is not suitable for shell type `%s'"
+             sh-shell))
+    (setq sh--shellcheck-process
+          (make-process
+           :name "shellcheck" :noquery t :connection-type 'pipe
+           :buffer (generate-new-buffer " *flymake-shellcheck*")
+           :command `(,sh-shellcheck-program
+                      "--format=json1"
+                      "-s" ,dialect
+                      ,@sh-shellcheck-arguments
+                      "-")
+           :sentinel sentinel))
+    (save-restriction
+      (widen)
+      (process-send-region sh--shellcheck-process (point-min) (point-max))
+      (process-send-eof sh--shellcheck-process))))
+
 (provide 'sh-script)
 
 ;;; sh-script.el ends here
-- 
2.37.3


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


PS: What is the best practice w.r.t. the fact that json-serialize might
not be available?  It seems that every library that needs to parse JSON
has to define a new alias similar to sh--json-read here.  This is rather
suboptimal...

bug#57884: [PATCH] Flymake backend using the shellcheck program

[Philip Kaludercic]

Augusto Stoffel <arstoffel@gmail.com> writes:

> On Sun, 18 Sep 2022 at 13:46, Philip Kaludercic wrote:
>
>>> I like in your backend that you read a JSON output, which presumably
>>> provides the start _and end_ of the diagnostic region.  How did you
>>> convert from line/column to buffer position?
>>
>> I didn't do that, Flymake takes care of that if you give it the buffer
>> position (unless I misunderstood something, but it appears to be working
>> for me).
>
> Okay, perhaps the docstring of `flymake-make-diagnostic' is easy to
> misinterpret, but apparently it works fine if one passes a (LINE . COL)
> cons for BEG and END, even if LOCUS is a buffer.  This would also mean
> that the call to `flymake-diag-region` in the example backend of the
> Flymake manual is unnecessary, so I'm a bit suspicious here.

The example backend in the manual has had issues before so I wouldn't be
surprised if this were the case.

> Anyway, I rewrote the backend to use the JSON output of shellcheck,
> which has the advantage that it provides the end position of each
> diagnostic, so Flymake doesn't have to guess it (which is by nature
> sometimes inaccurate).  Let me know what you think.

LGTM, but I haven't tested it yet.

> PS: What is the best practice w.r.t. the fact that json-serialize might
> not be available?  It seems that every library that needs to parse JSON
> has to define a new alias similar to sh--json-read here.  This is rather
> suboptimal...

All I can add is this: I've re-implemented the native-json functions in
Elisp for Compat[0], and it turns out to be more complicated than I had
initially expected -- though not impossible.  If anyone has Compat
installed these definitions will also be loaded even if the user is
running Emacs 27.1+, since the test case it uses to check if the
definition should be installed is:

        (not (condition-case nil
                 (equal (json-serialize '()) "{}")
               (:success t)
               (void-function nil)
               (json-unavailable nil)))

As Compat is part of GNU ELPA, we could just copy these definitions as
fallback into json.el and replace the tests with (fboundp ...) checks.

[0] https://git.sr.ht/~pkal/compat/tree/e827a9f3e2ba585de8b07cd246534a15ec414a8b/item/compat-27.el#L137

bug#57884: [PATCH] Flymake backend using the shellcheck program

[Philip Kaludercic]

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

Philip Kaludercic <philipk@posteo.net> writes:

>> Anyway, I rewrote the backend to use the JSON output of shellcheck,
>> which has the advantage that it provides the end position of each
>> diagnostic, so Flymake doesn't have to guess it (which is by nature
>> sometimes inaccurate).  Let me know what you think.
>
> LGTM, but I haven't tested it yet.

I just tried it out and it behaves the way you advertised it.

BTW, this diff describes the changes required if you were to pull out
the sentinel definition into a named function:


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

diff --git a/lisp/progmodes/sh-script.el b/lisp/progmodes/sh-script.el
index 558b62b20a..d52e385b36 100644
--- a/lisp/progmodes/sh-script.el
+++ b/lisp/progmodes/sh-script.el
@@ -3129,50 +3129,49 @@ 'sh--json-read
     (require 'json)
     'json-read))
 
+(defun sh-shellcheck-sentinel (proc _event)
+  (when (memq (process-status proc) '(exit signal))
+    (unwind-protect
+        (if (with-current-buffer (process-get proc 'source)
+              (not (eq proc sh--shellcheck-process)))
+            (flymake-log :warning "Canceling obsolete check %s" proc)
+          (with-current-buffer (process-buffer proc)
+            (goto-char (point-min))
+            (thread-last
+              (sh--json-read)
+              (alist-get 'comments)
+              (seq-filter
+               (lambda (item)
+                 (let-alist item (string= .file "-"))))
+              (mapcar
+               (lambda (item)
+                 (let-alist item
+                   (flymake-make-diagnostic
+                    (process-get proc 'source)
+                    (cons .line .column)
+                    (unless (and (eq .line .endLine)
+                                 (eq .column .endColumn))
+                      (cons .endLine .endColumn))
+                    (pcase .level
+                      ("error" :error)
+                      ("warning" :warning)
+                      (_ :note))
+                    (format "SC%s: %s" .code .message)))))
+              (funcall (process-get proc 'report-fn)))))
+      (kill-buffer (process-buffer proc)))))
+
 (defun sh-shellcheck-flymake (report-fn &rest _args)
   "Flymake backend using the shellcheck program.
 Takes a Flymake callback REPORT-FN as argument, as expected of a
 member of `flymake-diagnostic-functions'."
   (when (process-live-p sh--shellcheck-process)
     (kill-process sh--shellcheck-process))
-  (let* ((source (current-buffer))
-         (dialect (named-let recur ((s sh-shell))
+  (let* ((dialect (named-let recur ((s sh-shell))
                     (pcase s
                       ((or 'bash 'dash 'sh) (symbol-name s))
                       ('ksh88 "ksh")
                       ((guard s)
-                       (recur (alist-get s sh-ancestor-alist))))))
-         (sentinel
-          (lambda (proc _event)
-            (when (memq (process-status proc) '(exit signal))
-              (unwind-protect
-                  (if (with-current-buffer source
-                        (not (eq proc sh--shellcheck-process)))
-                      (flymake-log :warning "Canceling obsolete check %s" proc)
-                    (with-current-buffer (process-buffer proc)
-                      (goto-char (point-min))
-                      (thread-last
-                        (sh--json-read)
-                        (alist-get 'comments)
-                        (seq-filter
-                         (lambda (item)
-                           (let-alist item (string= .file "-"))))
-                        (mapcar
-                         (lambda (item)
-                           (let-alist item
-                             (flymake-make-diagnostic
-                              source
-                              (cons .line .column)
-                              (unless (and (eq .line .endLine)
-                                           (eq .column .endColumn))
-                                (cons .endLine .endColumn))
-                              (pcase .level
-                                ("error" :error)
-                                ("warning" :warning)
-                                (_ :note))
-                              (format "SC%s: %s" .code .message)))))
-                        (funcall report-fn))))
-                (kill-buffer (process-buffer proc)))))))
+                       (recur (alist-get s sh-ancestor-alist)))))))
     (unless dialect
       (error "`sh-shellcheck-flymake' is not suitable for shell type `%s'"
              sh-shell))
@@ -3185,7 +3184,9 @@ sh-shellcheck-flymake
                       "-s" ,dialect
                       ,@sh-shellcheck-arguments
                       "-")
-           :sentinel sentinel))
+           :sentinel #'sh-shellcheck-sentinel))
+    (process-put sh--shellcheck-process 'source (current-buffer))
+    (process-put sh--shellcheck-process 'report-fn report-fn)
     (save-restriction
       (widen)
       (process-send-region sh--shellcheck-process (point-min) (point-max))

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


I still don't think it looks that bad, but I don't insist on it.

bug#57884: [PATCH] Flymake backend using the shellcheck program

[Augusto Stoffel]

On Sun, 18 Sep 2022 at 21:18, Philip Kaludercic wrote:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>>> Anyway, I rewrote the backend to use the JSON output of shellcheck,
>>> which has the advantage that it provides the end position of each
>>> diagnostic, so Flymake doesn't have to guess it (which is by nature
>>> sometimes inaccurate).  Let me know what you think.
>>
>> LGTM, but I haven't tested it yet.
>
> I just tried it out and it behaves the way you advertised it.

Okay, so for whoever feels inclined to merge this, please consider the
patch in my message of Sun, 18 Sep 2022 21:38:17 +0200.

> BTW, this diff describes the changes required if you were to pull out
> the sentinel definition into a named function:
> [...]
> I still don't think it looks that bad, but I don't insist on it.

I still don't understand the motivation for this.  Closures are the
perfect tool for this job, and you seem to be just reinventing them. If
I wanted to create a helper function (which I don't really find
necessary from the code organization perspective in this case, although
it might well be in the proselint case), then I'd pass this as :sentinel
argument to make-process:

  (lambda (proc _event) (helper-function proc source report-fn))

bug#57884: [PATCH] Flymake backend using the shellcheck program

[Philip Kaludercic]

Augusto Stoffel <arstoffel@gmail.com> writes:

> On Sun, 18 Sep 2022 at 21:18, Philip Kaludercic wrote:
>
>> BTW, this diff describes the changes required if you were to pull out
>> the sentinel definition into a named function:
>> [...]
>> I still don't think it looks that bad, but I don't insist on it.
>
> I still don't understand the motivation for this.  Closures are the
> perfect tool for this job, and you seem to be just reinventing them. If
> I wanted to create a helper function (which I don't really find
> necessary from the code organization perspective in this case, although
> it might well be in the proselint case), then I'd pass this as :sentinel
> argument to make-process:
>
>   (lambda (proc _event) (helper-function proc source report-fn))

It is just a optical-stylistic manner, nothing technical. Flatter
functions and less indentation, that is all.

bug#57884: [PATCH] Flymake backend using the shellcheck program

[Augusto Stoffel]

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

On Sun, 18 Sep 2022 at 21:18, Philip Kaludercic wrote:

> I just tried it out and it behaves the way you advertised it.

Could someone merge this patch then, so it gets some testing before
Emacs 29 is cut?

I've attached the patch again for your reference.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-New-Flymake-backend-using-the-shellcheck-program.patch --]
[-- Type: text/x-patch, Size: 5799 bytes --]

From 49fbfaf98cb6d11b877eaa4a72b116aa61646d75 Mon Sep 17 00:00:00 2001
From: Augusto Stoffel <arstoffel@gmail.com>
Date: Sat, 17 Sep 2022 18:30:04 +0200
Subject: [PATCH] New Flymake backend using the shellcheck program

See bug#57884.

* lisp/progmodes/sh-script.el: Require let-alist and subr-x when
compiling.
(sh--json-read): Helper function to deal with possible absence of
json-parse-buffer.
(sh-shellcheck-program, sh--shellcheck-process,
sh-shellcheck-flymake): Variables and function defining a Flymake
backend.
(sh-mode): Add it to 'flymake-diagnostic-functions'.
---
 etc/NEWS                    |  4 ++
 lisp/progmodes/sh-script.el | 90 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 93 insertions(+), 1 deletion(-)

diff --git a/etc/NEWS b/etc/NEWS
index a6a8883593..5b2ed16063 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1320,6 +1320,10 @@ This controls how statements like the following are indented:
     foo &&
         bar
 
+*** New Flymake backend using the ShellCheck program
+It is enabled by default, but requires that the external "shellcheck"
+command is installed.
+
 ** Cperl Mode
 
 ---
diff --git a/lisp/progmodes/sh-script.el b/lisp/progmodes/sh-script.el
index 517fbbd8e7..558b62b20a 100644
--- a/lisp/progmodes/sh-script.el
+++ b/lisp/progmodes/sh-script.el
@@ -31,6 +31,9 @@
 ;; available for filenames, variables known from the script, the shell and
 ;; the environment as well as commands.
 
+;; A Flymake backend using the "shellcheck" program is provided.  See
+;; https://www.shellcheck.net/ for installation instructions.
+
 ;;; Known Bugs:
 
 ;; - In Bourne the keyword `in' is not anchored to case, for, select ...
@@ -141,7 +144,9 @@
 (eval-when-compile
   (require 'skeleton)
   (require 'cl-lib)
-  (require 'comint))
+  (require 'comint)
+  (require 'let-alist)
+  (require 'subr-x))
 (require 'executable)
 
 (autoload 'comint-completion-at-point "comint")
@@ -1580,6 +1585,7 @@ sh-mode
 	 ((equal (file-name-nondirectory buffer-file-name) ".profile") "sh")
          (t sh-shell-file))
    nil nil)
+  (add-hook 'flymake-diagnostic-functions #'sh-shellcheck-flymake nil t)
   (add-hook 'hack-local-variables-hook
     #'sh-after-hack-local-variables nil t))
 
@@ -3103,6 +3109,88 @@ sh-delete-backslash
  	    (delete-region (1+ (point))
  			   (progn (skip-chars-backward " \t") (point)))))))
 
+;;; Flymake backend
+
+(defcustom sh-shellcheck-program "shellcheck"
+  "Name of the shellcheck executable."
+  :type 'string
+  :version "29.1")
+
+(defcustom sh-shellcheck-arguments nil
+  "Additional arguments to the shellcheck program."
+  :type '(repeat string)
+  :version "29.1")
+
+(defvar-local sh--shellcheck-process nil)
+
+(defalias 'sh--json-read
+  (if (fboundp 'json-parse-buffer)
+      (lambda () (json-parse-buffer :object-type 'alist))
+    (require 'json)
+    'json-read))
+
+(defun sh-shellcheck-flymake (report-fn &rest _args)
+  "Flymake backend using the shellcheck program.
+Takes a Flymake callback REPORT-FN as argument, as expected of a
+member of `flymake-diagnostic-functions'."
+  (when (process-live-p sh--shellcheck-process)
+    (kill-process sh--shellcheck-process))
+  (let* ((source (current-buffer))
+         (dialect (named-let recur ((s sh-shell))
+                    (pcase s
+                      ((or 'bash 'dash 'sh) (symbol-name s))
+                      ('ksh88 "ksh")
+                      ((guard s)
+                       (recur (alist-get s sh-ancestor-alist))))))
+         (sentinel
+          (lambda (proc _event)
+            (when (memq (process-status proc) '(exit signal))
+              (unwind-protect
+                  (if (with-current-buffer source
+                        (not (eq proc sh--shellcheck-process)))
+                      (flymake-log :warning "Canceling obsolete check %s" proc)
+                    (with-current-buffer (process-buffer proc)
+                      (goto-char (point-min))
+                      (thread-last
+                        (sh--json-read)
+                        (alist-get 'comments)
+                        (seq-filter
+                         (lambda (item)
+                           (let-alist item (string= .file "-"))))
+                        (mapcar
+                         (lambda (item)
+                           (let-alist item
+                             (flymake-make-diagnostic
+                              source
+                              (cons .line .column)
+                              (unless (and (eq .line .endLine)
+                                           (eq .column .endColumn))
+                                (cons .endLine .endColumn))
+                              (pcase .level
+                                ("error" :error)
+                                ("warning" :warning)
+                                (_ :note))
+                              (format "SC%s: %s" .code .message)))))
+                        (funcall report-fn))))
+                (kill-buffer (process-buffer proc)))))))
+    (unless dialect
+      (error "`sh-shellcheck-flymake' is not suitable for shell type `%s'"
+             sh-shell))
+    (setq sh--shellcheck-process
+          (make-process
+           :name "shellcheck" :noquery t :connection-type 'pipe
+           :buffer (generate-new-buffer " *flymake-shellcheck*")
+           :command `(,sh-shellcheck-program
+                      "--format=json1"
+                      "-s" ,dialect
+                      ,@sh-shellcheck-arguments
+                      "-")
+           :sentinel sentinel))
+    (save-restriction
+      (widen)
+      (process-send-region sh--shellcheck-process (point-min) (point-max))
+      (process-send-eof sh--shellcheck-process))))
+
 (provide 'sh-script)
 
 ;;; sh-script.el ends here
-- 
2.37.3


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


Thanks!

bug#57884: [PATCH] Flymake backend using the shellcheck program

[Philip Kaludercic]

Augusto Stoffel <arstoffel@gmail.com> writes:

> On Sun, 18 Sep 2022 at 21:18, Philip Kaludercic wrote:
>
>> I just tried it out and it behaves the way you advertised it.
>
> Could someone merge this patch then, so it gets some testing before
> Emacs 29 is cut?

As I assume that everything has been settled, I've pushed the patch.

bug#57884: [PATCH] Flymake backend using the shellcheck program

[Philip Kaludercic]

Philip Kaludercic <philipk@posteo.net> writes:

> Augusto Stoffel <arstoffel@gmail.com> writes:
>
>> On Sun, 18 Sep 2022 at 21:18, Philip Kaludercic wrote:
>>
>>> I just tried it out and it behaves the way you advertised it.
>>
>> Could someone merge this patch then, so it gets some testing before
>> Emacs 29 is cut?
>
> As I assume that everything has been settled, I've pushed the patch.

Looks like I forgot to close the report after pushing the patch, so I am
doing it now.

bug#57884: [PATCH] Flymake backend using the shellcheck program

[Philip Kaludercic]

Augusto Stoffel <arstoffel@gmail.com> writes:

> +  (add-hook 'flymake-diagnostic-functions #'sh-shellcheck-flymake nil t)

[...]

> +    (setq sh--shellcheck-process
> +          (make-process
> +           :name "luacheck" :noquery t :connection-type 'pipe
> +           :buffer (generate-new-buffer " *flymake-luacheck*")
> +           :command `(,(car sh-shellcheck-command)
> +                      "--format=gcc"
> +                      "--color=never"
> +                      "-s" ,dialect
> +                      ,@(cdr sh-shellcheck-command)
> +                      "-")
> +           :sentinel sentinel))

Also what happens if someone doesn't have shellcheck installed?