Fix three bugs in php-complete function name completion - #825
Merged
Conversation
The predicate had two independent defects.
It looped over `values' -- a standard (Emacs 29+ obsolete) Emacs variable
that is nonetheless bound, normally to nil -- instead of over its own
argument `value'. `cl-loop for v in nil always ...' is vacuously true,
so the predicate returned t for every list, and a .dir-locals.el could
put arbitrary unvetted module names into the variable:
pred '(core) -> t
pred '(bogus-module) -> t ; should be nil
pred '("anything" 42) -> t ; should be nil
It also never ran successfully in the first place. Emacs decides whether
a .dir-locals.el value is safe while hacking local variables, which
happens before php-complete.el is loaded, so the predicate executes as
copied into php-mode-autoloads.el -- where neither cl-lib nor
php-defs-functions-alist exists yet:
php-complete-function-modules -> (void-function cl-loop)
safe-local-variable-p demotes that error and returns nil, so the variable
was treated as unsafe and prompted for confirmation anyway.
Add an autoloaded `php-defs-function-module-names' for the predicate to
consult -- a plain list of module symbols, rather than autoloading the
3800-line `php-defs-functions-alist' itself -- and rewrite the predicate
without cl-lib. `php-defs-function-module-names' also feeds the
defcustom `:type', and a test keeps it in sync with the alist.
`sort' is destructive, so sorting `php-complete-function-modules' to build the cache key reordered the value the user had set: after a single completion, a configured (pcntl bcmath core) came back as (bcmath core pcntl). Sort a copy instead.
An entry of `php-defs-functions-alist' is (MODULE . FUNCTION-NAMES), so appending the `assq' result spliced MODULE into the candidate list: with `php-complete-function-modules' set to (bcmath), the symbol `bcmath' showed up among the function name strings. Take the `cdr'. Also give `php-defs-functions-alist' the docstring it should have had as a public variable, spelling out that shape.
Emacs 28 was the first to copy a defcustom's `:safe' predicate into the generated autoloads file (autoload.el, "Propagate the :safe property to the loaddefs file"). Emacs 27's generator converts the defcustom into a plain defvar and discards `:safe' entirely, so the autoloads file has no `safe-local-variable' property to call and the subprocess died with (void-function nil) -- the CI failure on the 27.2 jobs. Skip the test when the generated file carries no such property, and check the subprocess output before its exit status so that a real failure reports what the subprocess complained about. Note in the CHANGELOG that this half of the fix needs Emacs 28 or later.
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.
Three independent bugs in
php-complete.el, each with a regression test.The
:safepredicate ofphp-complete-function-modulesaccepted anything, and never ranThe predicate looped over
values, a standard Emacs variable that is bound to nil, instead of over its own argumentvalue.cl-loop for v in nil always ...is true for an empty list, so any list passed as safe, including'(bogus-module)and'("anything" 42).It also failed before it could reject anything. Emacs checks
.dir-locals.elvalues before loadingphp-complete.el, so the predicate runs as copied intophp-mode-autoloads.el, where neithercl-loopnorphp-defs-functions-alistis available:safe-local-variable-pwraps predicates inwith-demoted-errors, which turns that error into nil, so Emacs asked for confirmation in every project that set the variable.This branch adds an autoloaded
php-defs-function-module-namesfor the predicate to consult, and rewrites the predicate without cl-lib. Autoloadingphp-defs-functions-alistitself would copy 3800 lines into the autoloads file, so the new variable holds the module symbols alone. It also feeds the defcustom:type, and a test keeps it in sync with the alist.php-complete--functionssorted a user option in placesortis destructive, so building the cache key reordered the value the user had set. A configured(pcntl bcmath core)came back as(bcmath core pcntl)after one completion. It now sorts a copy.Module names leaked into the completion candidates
Entries of
php-defs-functions-alistare(MODULE . FUNCTION-NAMES), and the code appended the whole entry, splicing MODULE in among the function names. Withphp-complete-function-modulesset to(bcmath), the symbolbcmathshowed up next to"bcadd". Taking thecdrfixes it.php-defs-functions-alistalso gets the docstring it should have had as a public variable, spelling out that shape.Verification
The 5 new ERT tests pass on this branch and fail on
master, including a subprocessemacs -Q --batch --load ...autoloadscheck that covers the cold path of the:safepredicate.eask test ert ./tests/php-mode-test.elreports 71 tests, 0 unexpected, 2 skipped. Byte compilation is clean, and checkdoc reports no new warnings.