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

[Philip Kaludercic <philipk@posteo.net>]

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