Skip to content

Code Quality: Annotate three impure functions for PHPStan - #13074

Open
tstokes8040 wants to merge 2 commits into
WordPress:trunkfrom
tstokes8040:phpstan/impure-annotations
Open

Code Quality: Annotate three impure functions for PHPStan#13074
tstokes8040 wants to merge 2 commits into
WordPress:trunkfrom
tstokes8040:phpstan/impure-annotations

Conversation

@tstokes8040

@tstokes8040 tstokes8040 commented Aug 16, 2026

Copy link
Copy Markdown

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:

Function Why a second call can differ
current_user_can() Reads the current user and applies the user_has_cap filter, so a plugin hooked in between can change the answer
WP_Filesystem_Base::chmod() Changes the permissions that a following is_writable() reports
WP_HTML_Processor::next_token() Advances the parser, so the token accessors then describe a different token

The clearest case is wp-admin/link-manager.php. Line 11 bails out unless the user has manage_links, and line 91 checks the same capability again after admin-header.php has 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() and set_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: in WP_Upgrader::install_package() it is the chmod() between two is_writable() calls, and in WP_Block it is the next_token() loop between two is_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 the copy_dir() case. WP_User::has_cap(), where the user_has_cap filter is actually applied, was also tried and cleared nothing, so the annotation sits on current_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:

Baseline Before After
booleanNot.alwaysFalse 7 2
booleanNot.alwaysTrue 8 4
booleanAnd.leftAlwaysTrue 5 4
booleanOr.alwaysTrue 2 1

The other five fall outside those baselines and come along for free: wp-admin/my-sites.php and wp-admin/upload.php (×2) in if.alwaysTrue, wp-admin/theme-install.php in ternary.alwaysTrue, and a deadCode.unreachable in class-wp-block.php that 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.dist is unchanged.

Testing instructions

  1. npm run typecheck:php on trunk reports [OK] No errors, because the occurrences are baselined.
  2. With this branch applied, npm run typecheck:php reports [OK] No errors across 1289 files with the seven baselines regenerated.
  3. To confirm the change is surgical, run the analysis with all baselines suppressed before and after and diff the results, comparing on file plus message rather than on line number: adding two lines to class-wp-html-processor.php shifts 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.
  4. 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 for src/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 unmodified trunk, 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

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.
@github-actions

Copy link
Copy Markdown

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props tstokes8040.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions

Copy link
Copy Markdown

Test using WordPress Playground

The 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

  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant