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

[Augusto Stoffel <arstoffel@gmail.com>]

[-- 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