Code Quality: Annotate three impure functions for PHPStan - #13074
Code Quality: Annotate three impure functions for PHPStan#13074tstokes8040 wants to merge 2 commits into
Conversation
PHPStan infers `current_user_can()`, `WP_Filesystem_Base::chmod()` and `WP_HTML_Processor::next_token()` as pure, so it remembers what each returned and reuses that value at a later call in the same scope. All three can legitimately answer differently the second time: `current_user_can()` reads the current user and applies the `user_has_cap` filter, `chmod()` changes the permissions that a following `is_writable()` reports, and `next_token()` advances the parser so the token accessors describe a different token.
The annotations go on the mutating call rather than on the accessors PHPStan's tips name. `is_writable()`, `is_tag_closer()`, `expects_closer()`, `get_current_depth()` and `set_bookmark()` are read-only; what invalidates a remembered value is the mutation sitting between the two reads.
This resolves 16 baselined errors across seven identifiers, among them the five `! current_user_can()` guards that re-check a capability already tested earlier in the same file, and the `is_writable()` and `copy()` re-checks that follow a `chmod()` in `copy_dir()`, `WP_Upgrader` and `Language_Pack_Upgrader`. The baselines were regenerated with:
{{{
composer phpstan:baselines -- --identifier=booleanNot.alwaysTrue,booleanNot.alwaysFalse,booleanAnd.leftAlwaysTrue,booleanOr.alwaysTrue,deadCode.unreachable,if.alwaysTrue,ternary.alwaysTrue
}}}
Props tstokes8040.
See #65817.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
PHPStan infers
current_user_can(),WP_Filesystem_Base::chmod()andWP_HTML_Processor::next_token()as pure, so it remembers what each returned and reuses that value at a later call in the same scope. All three can legitimately answer differently the second time:current_user_can()user_has_capfilter, so a plugin hooked in between can change the answerWP_Filesystem_Base::chmod()is_writable()reportsWP_HTML_Processor::next_token()The clearest case is
wp-admin/link-manager.php. Line 11 bails out unless the user hasmanage_links, and line 91 checks the same capability again afteradmin-header.phphas been required and its hooks have fired. PHPStan carried the line 11 answer forward and reported the line 91 guard as dead.Annotating the mutation, not the accessor
PHPStan's tips name
is_writable(),is_tag_closer(),expects_closer(),get_current_depth()andset_bookmark()as candidates. Those are read-only accessors and annotating them would be inaccurate. What actually invalidates a remembered value is the mutating call sitting between the two reads, so the annotation belongs there: inWP_Upgrader::install_package()it is thechmod()between twois_writable()calls, and inWP_Blockit is thenext_token()loop between twois_tag_closer()calls.WP_Filesystem_Base::copy()was annotated at first and then removed after re-running the analysis with and without it produced identical results;chmod()alone covers thecopy_dir()case.WP_User::has_cap(), where theuser_has_capfilter is actually applied, was also tried and cleared nothing, so the annotation sits oncurrent_user_can()itself.Impact
16 errors resolved across seven identifiers, with no new error of any identifier anywhere in the codebase. Eleven are in the boolean baselines this targets:
booleanNot.alwaysFalsebooleanNot.alwaysTruebooleanAnd.leftAlwaysTruebooleanOr.alwaysTrueThe other five fall outside those baselines and come along for free:
wp-admin/my-sites.phpandwp-admin/upload.php(×2) inif.alwaysTrue,wp-admin/theme-install.phpinternary.alwaysTrue, and adeadCode.unreachableinclass-wp-block.phpthat was only unreachable because of the boolean error above it.The baselines are generated, not hand-edited. No baseline reaches zero here, so none is deleted and
phpstan.neon.distis unchanged.Testing instructions
npm run typecheck:phpontrunkreports[OK] No errors, because the occurrences are baselined.npm run typecheck:phpreports[OK] No errorsacross 1289 files with the seven baselines regenerated.class-wp-html-processor.phpshifts every line below it, which makes a naive line-keyed diff report roughly 40 spurious changes. Compared correctly, exactly 16 errors disappear and none appears.npm run test:php— 30871 tests, 4558975 assertions, 86 warnings, 44 skipped, and one failure:Tests_Script_Modules_WpScriptModules::test_default_script_module_files_exist, looking forsrc/wp-includes/js/dist/script-modules/a11y/index.js. That path is gitignored build output that is unbuilt in my checkout, and the test fails identically on unmodifiedtrunk, so it is unrelated to this change.Documentation-only change; no runtime behaviour is affected.
Related: #13057 annotates
WP_HTML_Tag_Processor::parse_next_tag()for the same reason, and #13071 clears a different cluster of the same boolean baselines. All three are independent and touch disjoint files.Trac ticket: https://core.trac.wordpress.org/ticket/65817
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: grouping the remaining boolean PHPStan baselines by root cause, identifying this cluster, testing which annotation placements were load-bearing, measuring the before/after error diff across the whole codebase, regenerating the baselines, and drafting this description. The change itself and the verification runs were reviewed and confirmed by me in a local development environment.
Count of removed errors: 7