branch: elpa/flycheck
commit 7e44b59718318abb4bf9732de2538f4e0281ce49
Author: Bozhidar Batsov <[email protected]>
Commit: Bozhidar Batsov <[email protected]>
Harden the TRAMP remote-execution fixes
Address issues an adversarial review found in the previous two commits:
- flycheck-start-command-checker now errors cleanly when the checker
executable cannot be resolved. Forwarding a nil program to
start-file-process spawns a process that runs nothing and never exits,
which would hang the check forever; a cached-enabled checker whose
executable later vanishes now fails visibly instead. Add a spec.
- Rename the per-host caches flycheck--haskell-ghc-cache-directories and
flycheck--proselint-old-args-by-host. They changed from scalars to hash
tables; keeping the old names would let an in-session reload retain the
stale scalar value, so the next gethash would signal wrong-type-argument
until Emacs restarts. New names sidestep defvar's no-reassign rule.
---
flycheck.el | 39 +++++++++++++++++++++-------------
test/specs/languages/test-proselint.el | 2 +-
test/specs/test-command-checker.el | 10 +++++++++
3 files changed, 35 insertions(+), 16 deletions(-)
diff --git a/flycheck.el b/flycheck.el
index 2632086d95..8f1dcf4c01 100644
--- a/flycheck.el
+++ b/flycheck.el
@@ -7062,14 +7062,17 @@ PROCESS, and terminates standard input with EOF."
"Start a command CHECKER with CALLBACK."
(let (process)
(condition-case err
- (let* (;; `flycheck-find-checker-executable' may return a remote
+ (let* (;; `flycheck-find-checker-executable' may return nil for a
+ ;; cached-enabled checker whose executable later vanished.
+ ;; Fail the check cleanly rather than starting a process with
+ ;; a nil program, which never exits and hangs the check.
+ (executable (or (flycheck-find-checker-executable checker)
+ (error "Cannot find the executable of checker
%s"
+ checker)))
+ ;; `flycheck-find-checker-executable' may return a remote
;; (TRAMP) file name; the process program must be the plain
- ;; local name on the remote host. It may also return nil (a
- ;; cached-enabled checker whose executable later vanished), in
- ;; which case leave it nil, as before, rather than erroring in
- ;; `file-local-name'.
- (executable (flycheck-find-checker-executable checker))
- (program (and executable (file-local-name executable)))
+ ;; local name on the remote host.
+ (program (file-local-name executable))
(args (flycheck-checker-substituted-arguments checker))
(command (flycheck--wrap-command program args))
(sentinel-events nil)
@@ -10445,8 +10448,11 @@ pragma. Each extension is enabled via `-X'."
:safe #'flycheck-string-list-p
:package-version '(flycheck . "0.19"))
-(defvar flycheck-haskell-ghc-cache-directory (make-hash-table :test 'equal)
- "The cache directory for `ghc' output, keyed by host.
+;; A hash table (not the scalar of earlier versions -- hence the new name,
+;; so an in-session reload does not leave a stale non-table value that
+;; `gethash' would choke on).
+(defvar flycheck--haskell-ghc-cache-directories (make-hash-table :test 'equal)
+ "The cache directories for `ghc' output, keyed by host.
The key is the remote identifier of `default-directory' (see
`file-remote-p'), or nil for the local host, since `ghc' runs on
the host of the checked buffer and needs a cache directory there.")
@@ -10458,12 +10464,12 @@ If no cache directory exists yet for the current
host, create one
on that host and return it. Otherwise return the previously used
cache directory."
(let ((host (file-remote-p default-directory)))
- (or (gethash host flycheck-haskell-ghc-cache-directory)
+ (or (gethash host flycheck--haskell-ghc-cache-directories)
;; `make-nearby-temp-file' creates the directory on the host of
;; `default-directory', so `ghc' running there can write to it.
(puthash host
(make-nearby-temp-file "flycheck-haskell-ghc-cache"
'directory)
- flycheck-haskell-ghc-cache-directory))))
+ flycheck--haskell-ghc-cache-directories))))
(defun flycheck--locate-dominating-file-matching (directory regexp)
"Search for a file in directory hierarchy starting at DIRECTORY.
@@ -11384,7 +11390,10 @@ See URL `https://proselint.com/' for more information
about proselint."
(let-alist (car response)
.result.<stdin>.diagnostics)))))
-(defvar flycheck--proselint-use-old-args (make-hash-table :test 'equal)
+;; A hash table (not the scalar of earlier versions -- hence the new name,
+;; so an in-session reload does not leave a stale non-table value that
+;; `gethash' would choke on).
+(defvar flycheck--proselint-old-args-by-host (make-hash-table :test 'equal)
"Cache for proselint version detection, keyed by host.
The key is the remote identifier of `default-directory' (see
`file-remote-p'), or nil for the local host, since the proselint
@@ -11397,14 +11406,14 @@ absent from the table has not been probed yet.")
(defun flycheck--proselint-args ()
"Return command arguments for proselint, detecting the version once per
host."
(let ((host (file-remote-p default-directory)))
- (when (eq (gethash host flycheck--proselint-use-old-args 'unknown)
'unknown)
+ (when (eq (gethash host flycheck--proselint-old-args-by-host 'unknown)
'unknown)
(puthash host
;; Probe on the host the check will run on (remote over TRAMP).
(zerop (process-file
(or flycheck-proselint-executable "proselint")
nil nil nil "--version"))
- flycheck--proselint-use-old-args))
- (if (gethash host flycheck--proselint-use-old-args)
+ flycheck--proselint-old-args-by-host))
+ (if (gethash host flycheck--proselint-old-args-by-host)
;; Proselint versions <= 0.14.0:
(list "--json" "-")
;; Proselint versions >= 0.16.0
diff --git a/test/specs/languages/test-proselint.el
b/test/specs/languages/test-proselint.el
index 73c949d8fc..886c250a5e 100644
--- a/test/specs/languages/test-proselint.el
+++ b/test/specs/languages/test-proselint.el
@@ -7,7 +7,7 @@
(describe "flycheck--proselint-args"
(before-each
- (clrhash flycheck--proselint-use-old-args))
+ (clrhash flycheck--proselint-old-args-by-host))
(it "probes a host once and caches the detected version"
;; Exit 0 -> old proselint, which takes the "--json -" arguments.
diff --git a/test/specs/test-command-checker.el
b/test/specs/test-command-checker.el
index e8a8cdd504..c8432f4e3e 100644
--- a/test/specs/test-command-checker.el
+++ b/test/specs/test-command-checker.el
@@ -123,6 +123,16 @@ afterwards."
;; Called once for `emacs-lisp', and a second time for checkdoc
(expect was-called :to-equal 2)))
+ (it "signals when the executable cannot be found"
+ ;; A cached-enabled checker whose executable later vanished resolves to
+ ;; nil. Fail the check cleanly rather than starting a process with no
+ ;; program, which never exits and would hang the check forever.
+ (spy-on 'flycheck-find-checker-executable :and-return-value nil)
+ (spy-on 'start-file-process)
+ (flycheck-buttercup-with-temp-buffer
+ (expect (flycheck-start-command-checker 'emacs-lisp #'ignore)
:to-throw)
+ (expect 'start-file-process :not :to-have-been-called)))
+
(it "truncated stdin with errors"
;; Skip on Emacs 28 where large pipe writes can cause SIGPIPE
(assume (version<= "29" emacs-version))
