Make flycheck-phpstan a generic checker, add Apple container support - #62
Merged
Conversation
…mand
`phpstan-get-command-args' built its result with `nconc', but two of the
lists it links are owned by the caller rather than freshly consed:
* `executable-and-args', which for the `(STRING . (ARGUMENTS ...))'
form of `phpstan-executable' *is* the value of that variable, and
* `options', which `phpstan-generate-baseline' passes straight from
the `phpstan-generate-baseline-options' defcustom.
So every call appended the PHPStan arguments onto those lists. With
`(phpstan-executable . ("docker" "run" ...))' the variable grew by seven
elements per check, and each `M-x phpstan-generate-baseline' appended
another "--" to `phpstan-generate-baseline-options'. Use `append', which
copies all but the last list.
The same `(STRING . (ARGUMENTS ...))' form also returned `(cdr
phpstan-executable)' as the executable and arguments, so the command name
was dropped and the first argument ran as the program: `("docker" "run"
...)' executed `run'. This broke `flymake-phpstan' and
`phpstan-analyze-project', and made `flycheck-phpstan' set its executable
to "run". Return the whole list instead.
The form was also gated on `phpstan-flycheck-auto-set-executable', which
made `phpstan-get-executable-and-args' return nil for a valid executable
whenever that variable was nil. Resolving the executable has nothing to
do with how flycheck is configured, so drop the condition.
Apple container (https://github.com/apple/container) runs OCI images on macOS with the same `run --rm -v HOST:/app IMAGE' command line as Docker, so supporting it is a matter of picking a different command name. The image still comes from `phpstan-docker-image'. Introduce two predicates rather than one, because the existing `docker' handling conflated two questions: * `phpstan--container-runtime-command' answers "should phpstan.el build a `run' command line, and with which command?". Only the symbol forms say yes; the `(STRING . (ARGUMENTS ...))' form already supplies a complete command line and must not be rewritten. * `phpstan--container-executable-p' answers "does PHPStan see the project through a mount point?", which decides whether `phpstan-normalize-path' rewrites paths to /app. That is also true for `("docker" "run" ...)', which the old code special-cased by comparing the car against the literal "docker".
`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".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces the executable injection in
flycheck-phpstanwith a Flycheck generic checker, adds Apple container support, and fixes three bugs found on the way.Why
flycheck-define-checkerrequires the car of:commandto be a literal string, and the program can only be overridden through the single string variableflycheck-CHECKER-executable. PHPStan does not fit that shape:phpstan-executablemay expand to a whole command line such asdocker run --rm -v ...:/app IMAGE, chosen per project.So the checker declared a dummy
"php"executable, overwroteflycheck-phpstan-executableas a side effect of its:enabledpredicate, and hadphpstan-get-command-argsomit the program so flycheck would prepend the injected one.:startnow builds the command fromphpstan-executablewith:include-executable t— the same callflymake-phpstanalready makes — and reports errors through the status callback. No dummy executable, no variable injection, and no:aroundadvice onflycheck-finish-checker-process.Bugs fixed
All three were confirmed by execution, not by reading:
phpstan-get-command-argsdestructively modified its inputs. It built the result withnconc, but two of the lists it links are owned by the caller. With(phpstan-executable . ("docker" "run" ...))the variable grew by seven elements per check (6 → 13 → 20 → 27), and eachM-x phpstan-generate-baselineappended another"--"to thephpstan-generate-baseline-optionsdefcustom.The
(STRING . (ARGUMENTS ...))form dropped the command name. It returned(cdr phpstan-executable), so("docker" "run" ...)ranrunas the program. This brokeflymake-phpstanandphpstan-analyze-project, and madeflycheck-phpstanset its executable to"run".Flycheck could get stuck. The advice that suppressed "No files found to analyse" in a modified buffer worked by not calling the wrapped function at all, so no status was ever reported. As
flycheck-report-buffer-checker-statusdocuments, a checker that never reports a finishing status leaves flycheck stuck on the current check — silently killing checking for that buffer. It now finishes with an empty result.Dropping the dummy
"php"also fixes a fourth: Flycheck wraps:enabledso thatflycheck-find-checker-executableruns before the checker's own predicate, resolving"php"againstexec-pathwhile the injected variable was still nil. A Docker-only setup with no localphpcould never enable the checker.Apple container
phpstan-executableacceptscontainer. Apple container runs the same OCI images with the samerun --rm -v HOST:/app IMAGEcommand line as Docker, so only the command name differs; the image still comes fromphpstan-docker-image.The existing
dockerhandling conflated two questions, so this splits them:phpstan--container-runtime-command— should phpstan.el build aruncommand line? Only the symbol forms say yes;(STRING . (ARGUMENTS ...))already supplies a complete command line and must not be rewritten.phpstan--container-executable-p— does PHPStan see the project through a mount point, sophpstan-normalize-pathmust rewrite paths to/app? Also true for("docker" "run" ...).Verification
Ran Flycheck for real against local PHPStan 1.12.33, for both an unmodified buffer (analyzes the original) and a modified one (editor mode via
--tmp-file): errors reported correctly, no leftover temporaries, checker never stuck.container runwas executed directly against the phpstan image and returns the same output as Docker. Each commit byte-compiles on its own.Known issue, not fixed here
Docker and Apple container do not work through Flycheck, and did not before this PR either (verified against the original code). The analyzed source path is passed to the container unnormalized — the config file becomes
-c /app/tests/phpstan-docker.neoncorrectly, but the target file stays a host path, so PHPStan reportsPath /Users/... does not existand returns zero errors.flymake-phpstanavoids this by callingphpstan-normalize-pathitself.Related:
phpstan-editor-mode-available-prunsdocker --versionand parsesd1c06efout ofDocker version 29.5.3, build d1c06efas the PHPStan version, so editor mode is silently disabled for container users.Both are independent of this refactor and need their own design decisions (forcing in-place temp files inside the mount; how to version-check PHPStan inside a container), so they are left for follow-ups.
Compatibility
phpstan-flycheck-auto-set-executableis obsolete and ignored (make-obsolete-variable, 0.10.0).flycheck-phpstan-executableis gone; it was an undocumented baredefvarthat the package set itself, not a user-facing knob.