Make flycheck-phpstan a generic checker

`flycheck-define-checker' requires the car of `:command' to be a literal
string, and the program can only be overridden through the single string
variable `flycheck-CHECKER-executable'.  PHPStan does not fit that shape:
`phpstan-executable' may expand to a whole command line such as `docker
run --rm -v ...:/app IMAGE', chosen per project.

The checker worked around this by declaring a dummy "php" executable and
overwriting `flycheck-phpstan-executable' as a side effect of its
`:enabled' predicate, then having `phpstan-get-command-args' omit the
program from the arguments so flycheck would prepend the injected one.
That had two consequences beyond being hard to follow:

  * Flycheck wraps `:enabled' so that `flycheck-find-checker-executable'
    runs *before* the checker's own predicate.  At that point the
    variable was still nil, so it resolved the dummy "php" against
    `exec-path'.  A Docker-only setup with no local php could therefore
    never enable the checker.

  * `M-x flycheck-verify-setup' and the checker's documentation reported
    "php" as the executable.

Drive the process directly instead.  `:start' builds the command from
`phpstan-executable' with `:include-executable t' (the same call
`flymake-phpstan' already makes) and reports errors through the status
callback, so there is no dummy executable and no variable to inject.

This also removes the `:around' advice on `flycheck-finish-checker-process'
that suppressed "No files found to analyse" in a modified buffer.  The
advice worked by not calling the wrapped function at all, which means no
status was ever reported -- and as `flycheck-report-buffer-checker-status'
documents, a checker that never reports a finishing status leaves flycheck
stuck on the current check, silently killing checking for that buffer.
`flycheck-phpstan--finish' now reports an empty result instead.

Note that generic checkers default `:error-filter' to `identity', while
`flycheck-define-checker' installs `flycheck-sanitize-errors'.  It is
passed explicitly to keep the previous behaviour.

`:verify' is added to replace what `flycheck-verify-command-checker' gave
us for free; it now shows the real command line rather than "php".

Author: zonuexe

CHANGELOG.md

@@ -18,12 +18,18 @@ All notable changes of the `phpstan.el` are documented in this file using the [K
 ### Changed
 
 * `phpstan-copy-dumped-type` command now prioritizes `phpstan-hover-mode` data at point before falling back to dumped-type messages.
+* `flycheck-phpstan` is now a Flycheck *generic* checker instead of a command checker.
+  * The whole command line is built from `phpstan-executable`, so the checker no longer injects an executable into flycheck's `flycheck-phpstan-executable` variable as a side effect of its `:enabled` predicate, and no longer advises `flycheck-finish-checker-process`.
+  * `php` is no longer required in `exec-path` to enable the checker.  Previously the dummy `"php"` in `:command` was resolved by Flycheck *before* the `:enabled` predicate ran, so a Docker-only setup could not enable the checker at all.
+  * `M-x flycheck-verify-setup` now reports the real PHPStan command and configuration file instead of `"php"`.
+* `phpstan-flycheck-auto-set-executable` is obsolete and ignored.
 
 ### Fixed
 
 * Fix `phpstan-get-command-args` to keep `:options` in the correct position and pass target arguments correctly when editor mode options are used.
 * Fix `phpstan-executable` in the `(STRING . (ARGUMENTS ...))` form dropping the command name, which made the first *argument* run as the program (`("docker" "run" ...)` executed `run`).
 * Fix `phpstan-get-command-args` destructively modifying its inputs with `nconc`.  Each call appended the PHPStan arguments onto the caller's own list, growing `phpstan-executable` in the `(STRING . (ARGUMENTS ...))` form on every check, and appending `"--"` to `phpstan-generate-baseline-options` on every `phpstan-generate-baseline`.
+* Fix Flycheck getting stuck on a syntax check when PHPStan reported no files to analyse in a modified buffer.  The check now finishes with an empty result instead of never reporting a status.
 
 ## [0.9.0]
 

README.org

@@ -212,8 +212,8 @@ Rule level of PHPStan analysis.  Please see [[https://github.com/phpstan/phpstan
      - ex) ~("docker" "run" "--rm" "-v" "/path/to/project-dir/:/app" "your/docker-image")~
 - ~nil~ :: Auto detect ~phpstan~ executable file by composer dependencies of the project or executable command in ~PATH~ environment variable.
 
-*** Custom variable ~phpstan-flycheck-auto-set-executable~
-Set flycheck phpstan-executable automatically when non-NIL.
+*** Custom variable ~phpstan-flycheck-auto-set-executable~ (obsolete)
+Obsolete and ignored.  ~flycheck-phpstan~ now builds the whole command line from ~phpstan-executable~, so it no longer injects an executable into flycheck's ~flycheck-phpstan-executable~ variable.
 
 *** Custom variable ~phpstan-memory-limit~
 Use phpstan memory limit option when non-NIL.

flycheck-phpstan.el

@@ -36,15 +36,22 @@
 ;;
 ;;     (add-hook 'php-mode-hook 'my-php-mode-setup)
 ;;
+;; ## For Lisp maintainers
+;;
+;; This is a generic checker (`flycheck-define-generic-checker'), not a command
+;; checker (`flycheck-define-checker').  A command checker takes its executable
+;; from the car of `:command', which must be a literal string, overridable only
+;; through the single string variable `flycheck-CHECKER-executable'.  PHPStan
+;; does not fit that shape: `phpstan-executable' may expand to a whole command
+;; line such as `docker run --rm -v ...', and it is chosen per project.  So we
+;; drive the process ourselves and build the command from `phpstan-executable'.
 
 ;;; Code:
+(require 'cl-lib)
 (require 'flycheck)
 (require 'phpstan)
 
-;; Usually it is defined dynamically by flycheck
-(defvar flycheck-phpstan-executable)
 (defvar flycheck-phpstan--temp-buffer-name "*Flycheck PHPStan*")
-(defvar flycheck-phpstan--output-filter-added nil)
 (defconst flycheck-phpstan--nofiles-message (eval-when-compile (regexp-quote "[ERROR] No files found to analyse.")))
 
 (defcustom flycheck-phpstan-ignore-metadata-list nil
@@ -59,44 +66,7 @@
   :safe #'stringp
   :group 'phpstan)
 
-(defun flycheck-phpstan--suppress-no-files-error  (next checker exit-status files output callback cwd)
-  "Suppress Flycheck errors if PHPStan reports no files in a modified buffer.
-
-This function is intended to be used as an :around advice for
-`flycheck-finish-checker-process'.
-
-It prevents Flycheck from displaying an error when:
-- CHECKER is `phpstan',
-- the current buffer is modified,
-- and OUTPUT contains the message `flycheck-phpstan--nofiles-message'.
-
-NEXT, EXIT-STATUS, FILES, OUTPUT, CALLBACK, and CWD are the original arguments
-passed to `flycheck-finish-checker-process'."
-  (unless (and (eq checker 'phpstan)
-               (buffer-modified-p)
-               (string-match-p flycheck-phpstan--nofiles-message output))
-    (funcall next checker exit-status files output callback cwd)))
-
-(defun flycheck-phpstan--enabled-and-set-variable ()
-  "Return path to phpstan configure file, and set buffer execute in side effect."
-  (let ((enabled (phpstan-enabled)))
-    (prog1 enabled
-      (unless flycheck-phpstan--output-filter-added
-        (advice-add 'flycheck-finish-checker-process
-                    :around #'flycheck-phpstan--suppress-no-files-error)
-        (setq flycheck-phpstan--output-filter-added t))
-      (when (and enabled
-                 phpstan-flycheck-auto-set-executable
-                 (null (bound-and-true-p flycheck-phpstan-executable))
-                 (or (stringp phpstan-executable)
-                     (eq 'docker phpstan-executable)
-                     (and (eq 'root (car-safe phpstan-executable))
-                          (stringp (cdr-safe phpstan-executable)))
-                     (and (stringp (car-safe phpstan-executable))
-                          (listp (cdr-safe phpstan-executable)))
-                     (null phpstan-executable)))
-        (setq-local flycheck-phpstan-executable (car (phpstan-get-executable-and-args)))))))
-
+;; Parsing PHPStan output:
 (defun flycheck-phpstan-parse-output (output &optional _checker _buffer)
   "Parse PHPStan errors from OUTPUT."
   (let* ((json-buffer (with-current-buffer (flycheck-phpstan--temp-buffer)
@@ -143,21 +113,171 @@ passed to `flycheck-finish-checker-process'."
   "Return non-NIL if ORIGINAL is non-NIL and buffer is not modified."
   (and original (not (buffer-modified-p))))
 
-(flycheck-define-checker phpstan
+;; Running PHPStan:
+(defun flycheck-phpstan--command ()
+  "Return the whole PHPStan command line to check the current buffer.
+
+This has the side effect of saving the buffer to a temporary file, which is
+registered in `flycheck-temporaries' for later deletion."
+  (phpstan-get-command-args
+   :include-executable t
+   :format "json"
+   :editor (list
+            :analyze-original #'flycheck-phpstan-analyze-original
+            :original-file buffer-file-name
+            :temp-file (lambda () (flycheck-save-buffer-to-temp #'flycheck-temp-file-system))
+            :inplace (lambda () (flycheck-save-buffer-to-temp #'flycheck-temp-file-inplace)))))
+
+(defun flycheck-phpstan--start (checker callback)
+  "Start CHECKER, reporting the result to CALLBACK.
+
+Return the process, which Flycheck hands back to `:interrupt'."
+  (let (process)
+    (condition-case err
+        (let ((command (flycheck-phpstan--command))
+              ;; `flycheck-phpstan--nofiles-message' matches PHPStan's English
+              ;; output, so keep the checker process in the C locale.  Only
+              ;; LC_MESSAGES is set, to leave the encoding of the source alone.
+              (process-environment (cons "LC_MESSAGES=C" process-environment)))
+          (setq process
+                (make-process
+                 :name (format "flycheck-%s" checker)
+                 ;; Do not associate a buffer, to avoid the side effects of
+                 ;; attaching a process to the buffer being checked.
+                 :buffer nil
+                 :command command
+                 :noquery t
+                 :connection-type 'pipe
+                 :filter #'flycheck-phpstan--receive-output
+                 :sentinel #'flycheck-phpstan--handle-signal))
+          (process-put process 'flycheck-phpstan-checker checker)
+          (process-put process 'flycheck-phpstan-callback callback)
+          (process-put process 'flycheck-phpstan-buffer (current-buffer))
+          ;; Flycheck binds `default-directory' to `:working-directory' around
+          ;; this function, so remember it for resolving relative file names.
+          (process-put process 'flycheck-phpstan-cwd default-directory)
+          ;; Track the temporaries in the process itself, to get rid of the
+          ;; buffer-local state as soon as possible.
+          (process-put process 'flycheck-phpstan-temporaries flycheck-temporaries)
+          (setq flycheck-temporaries nil)
+          process)
+      (error
+       (flycheck-safe-delete-temporaries)
+       ;; Deleting the process triggers the sentinel, which deletes the
+       ;; temporary files of the process anyway.
+       (when process
+         (delete-process process))
+       (signal (car err) (cdr err))))))
+
+(defun flycheck-phpstan--interrupt (_checker process)
+  "Interrupt PROCESS."
+  ;; Deleting the process always triggers the sentinel, which does the cleanup.
+  (when process
+    (delete-process process)))
+
+(defun flycheck-phpstan--receive-output (process output)
+  "Accumulate OUTPUT of the PHPStan PROCESS for later parsing."
+  (process-put process 'flycheck-phpstan-pending-output
+               (cons output (process-get process 'flycheck-phpstan-pending-output))))
+
+(defun flycheck-phpstan--get-output (process)
+  "Return the complete output of the PHPStan PROCESS."
+  (with-demoted-errors "Error while retrieving process output: %S"
+    (apply #'concat (nreverse (process-get process 'flycheck-phpstan-pending-output)))))
+
+(defun flycheck-phpstan--handle-signal (process _event)
+  "Handle a signal from the PHPStan PROCESS.
+
+_EVENT is ignored."
+  (when (memq (process-status process) '(signal exit))
+    (let ((files (process-get process 'flycheck-phpstan-temporaries))
+          (buffer (process-get process 'flycheck-phpstan-buffer))
+          (callback (process-get process 'flycheck-phpstan-callback))
+          (cwd (process-get process 'flycheck-phpstan-cwd)))
+      (mapc #'flycheck-safe-delete files)
+      (when (buffer-live-p buffer)
+        (with-current-buffer buffer
+          (condition-case err
+              (pcase (process-status process)
+                (`signal
+                 (funcall callback 'interrupted))
+                (`exit
+                 (flycheck-phpstan--finish
+                  (process-get process 'flycheck-phpstan-checker)
+                  (process-exit-status process)
+                  files
+                  (flycheck-phpstan--get-output process)
+                  callback cwd)))
+            ((debug error)
+             (funcall callback 'errored (error-message-string err)))))))))
+
+(defun flycheck-phpstan--finish (checker exit-status files output callback cwd)
+  "Parse OUTPUT of CHECKER and report the result to CALLBACK.
+
+EXIT-STATUS is the exit status of the PHPStan process.  FILES is the list of
+temporary files given to PHPStan, used to map reported file names back onto
+the buffer.  Relative file names are resolved against CWD.
+
+CALLBACK is always invoked with a status that finishes the syntax check,
+because Flycheck gets stuck on the current check otherwise."
+  (if (and (buffer-modified-p)
+           (string-match-p flycheck-phpstan--nofiles-message output))
+      ;; PHPStan found nothing to analyse because the buffer is being edited.
+      ;; That is not a result worth showing, but the check must still finish.
+      (funcall callback 'finished nil)
+    (let ((errors (flycheck-phpstan-parse-output output checker (current-buffer))))
+      (when (and (not (equal exit-status 0)) (null errors))
+        ;; Warn about a suspicious result, but keep going: `suspicious' does
+        ;; not finish the syntax check on its own.
+        (funcall callback 'suspicious
+                 (format "Flycheck checker %S returned %S, but its output \
+contained no errors: %s\nTry installing a more recent version of PHPStan, and \
+please open a bug report if the issue persists in the latest release.  Thanks!"
+                         checker exit-status output)))
+      (funcall callback 'finished
+               ;; Fix error file names, by substituting them backwards from the
+               ;; temporaries.
+               (mapcar (lambda (e) (flycheck-fix-error-filename e files cwd))
+                       errors)))))
+
+(defun flycheck-phpstan--verify (_checker)
+  "Verify the PHPStan setup of the current buffer."
+  (let* ((executable-and-args (ignore-errors (phpstan-get-executable-and-args)))
+         (program (car executable-and-args))
+         (found (and program
+                     (if (file-name-absolute-p program)
+                         (and (file-executable-p program) program)
+                       (executable-find program))))
+         (config-file (phpstan-get-config-file)))
+    (list
+     (flycheck-verification-result-new
+      :label "executable"
+      :message (cond (found (format "Found at %s" found))
+                     (program (format "%s not found" program))
+                     (t "Not found"))
+      :face (if found 'success '(bold error)))
+     (flycheck-verification-result-new
+      :label "command"
+      :message (if executable-and-args
+                   (mapconcat #'shell-quote-argument executable-and-args " ")
+                 "Not available")
+      :face (if executable-and-args 'success 'warning))
+     (flycheck-verification-result-new
+      :label "configuration file"
+      :message (if config-file (format "Found at %S" config-file) "Not found")
+      :face (if config-file 'success 'warning)))))
+
+(flycheck-define-generic-checker 'phpstan
   "PHP static analyzer based on PHPStan."
-  :command ("php"
-            (eval
-             (phpstan-get-command-args
-              :format "json"
-              :editor (list
-                       :analyze-original #'flycheck-phpstan-analyze-original
-                       :original-file buffer-file-name
-                       :temp-file (lambda () (flycheck-save-buffer-to-temp #'flycheck-temp-file-system))
-                       :inplace (lambda () (flycheck-save-buffer-to-temp #'flycheck-temp-file-inplace))))))
+  :start #'flycheck-phpstan--start
+  :interrupt #'flycheck-phpstan--interrupt
+  :verify #'flycheck-phpstan--verify
+  ;; `flycheck-define-checker' installs this filter for command checkers, but
+  ;; generic checkers default to `identity'.
+  :error-filter #'flycheck-sanitize-errors
   :working-directory (lambda (_) (phpstan-get-working-dir))
-  :enabled (lambda () (flycheck-phpstan--enabled-and-set-variable))
-  :error-parser flycheck-phpstan-parse-output
-  :modes (php-mode php-ts-mode phps-mode))
+  :enabled (lambda () (phpstan-enabled))
+  :modes '(php-mode php-ts-mode phps-mode))
 
 (add-to-list 'flycheck-checkers 'phpstan t)
 (flycheck-add-next-checker 'php 'phpstan)

phpstan.el

@@ -77,8 +77,15 @@
   :link '(url-link :tag "phpstan.el" "https://github.com/emacs-php/phpstan.el"))
 
 (defcustom phpstan-flycheck-auto-set-executable t
-  "Set flycheck phpstan-executable automatically."
+  "Set flycheck phpstan-executable automatically.
+
+This variable no longer has any effect.  `flycheck-phpstan' now builds the
+whole command line from `phpstan-executable', so it never has to inject an
+executable into flycheck's own `flycheck-phpstan-executable' variable."
   :type 'boolean)
+(make-obsolete-variable 'phpstan-flycheck-auto-set-executable
+                        "the executable is always derived from `phpstan-executable'."
+                        "0.10.0")
 
 (defcustom phpstan-enable-on-no-config-file t
   "If T, activate config from composer even when `phpstan.neon' is not found."