Map JSON false and null to nil in phpstan--parse-json - #70
Merged
Conversation
PHPStan's `ignorable' flag is read in a truthy context, but both JSON parsers returned a truthy symbol for `false': `json-parse-buffer' gives `:false' and the `json-read-object' fallback gives `:json-false'. So a non-ignorable message (`"ignorable":false') was treated as ignorable by `flycheck-phpstan', `flymake-phpstan', and `phpstan-update-ignorebale-errors-from-json-buffer': its identifier was shown with the 🪪 prefix and `phpstan-insert-ignore' offered it, even though PHPStan says it cannot be ignored. `json-parse-buffer' also returns a truthy `:null' for JSON null, while the fallback returns nil, so the two paths disagreed -- and a `:null' reaching `concat' (say an absent tip) would error on a modern Emacs. Pass `:false-object nil :null-object nil' to `json-parse-buffer', and bind `json-false'/`json-null' to nil for the fallback, so both map `false' and `null' to nil. No consumer distinguishes `false' from an absent key (none use `plist-member'), and the remaining reads are numbers or strings guarded with `numberp', so nil is safe throughout. Tests cover the parse result, the ignorable-errors side effect, and both backends' identifier handling; each fails when the mapping is reverted.
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
1 participant
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.
Fixes a shared JSON-parsing bug that made non-ignorable PHPStan errors look ignorable — found while adding the flymake tests in #69.
The bug
phpstan--parse-jsonreturned a truthy symbol for JSONfalse:json-parse-buffergives:false, thejson-read-objectfallback gives:json-false. PHPStan'signorableflag is read in a truthy context, so a non-ignorable message ("ignorable":false, e.g. a parse error) was treated as ignorable by every consumer:flycheck-phpstanandflymake-phpstanappended the identifier with the 🪪 prefix, which tells the user "you can@phpstan-ignorethis";phpstan-update-ignorebale-errors-from-json-buffercollected it, sophpstan-insert-ignoreoffered to ignore an error PHPStan says cannot be ignored.json-parse-bufferalso returns a truthy:nullfor JSONnull, while the fallback returns nil — so the two paths disagreed, and a:nullreachingconcat(an absent tip, say) would error on a modern Emacs.The fix
Pass
:false-object nil :null-object niltojson-parse-buffer, and bindjson-false/json-nullto nil for thejson-read-objectfallback. Both paths now mapfalseandnullto nil.I checked every consumer first: nothing uses
plist-member, so no code distinguishesfalsefrom an absent key; the other JSON reads are numbers or strings guarded withnumberp(inphpstan-hover), so nil is safe throughout. This also makes the two parser paths agree onnull.Result
flycheck-phpstan: a non-ignorable parse error no longer shows 🪪; an ignorable error still does.flymake-phpstan: same.phpstan-insert-ignoreno longer offers non-ignorable identifiers.Tests
Four regression tests, each verified to fail when the mapping is reverted:
phpstan-test-parse-json-false-is-nil—false/nullread as nil,trueast.phpstan-test-update-ignorable-skips-non-ignorable—phpstan--ignorable-errorsexcludes the non-ignorable message.flycheck-phpstan-test-identifier-only-when-ignorableandflymake-phpstan-test-parse-skips-non-ignorable— the identifier appears only for the ignorable one.25 tests total pass on system Emacs and Emacs 27.2; compile is warning-free on both.