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

[Augusto Stoffel <arstoffel@gmail.com>]

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