diff --git a/.github/workflows/phpstan-pull-request-comments.yml b/.github/workflows/phpstan-pull-request-comments.yml index aabe26def6414..0c94761f6a9ed 100644 --- a/.github/workflows/phpstan-pull-request-comments.yml +++ b/.github/workflows/phpstan-pull-request-comments.yml @@ -261,21 +261,21 @@ jobs: '> [!IMPORTANT]', '> **The PHPStan baselines are out of date, so the PHPStan check fails.**', '>', - '> An error recorded in a baseline no longer occurs, or occurs a different number of times. If this pull request fixed it, that is expected: nothing needs to change in the code, but each baseline entry records an exact count for a specific file, and the baselines need to catch up. Regenerate them and commit the result:', + '> An error recorded in a baseline no longer occurs, or occurs a different number of times. If this pull request fixed it, that is expected: nothing needs to change in the code, but each baseline entry records an exact count for a specific file, and the baselines need to catch up. Remove the stale entries, and commit the result:', '>', '> ```bash', - '> npm run typecheck:php:baselines', + '> npm run typecheck:php:fix', '> ```', '>', '> or, outside the Docker environment:', '>', '> ```bash', - '> composer phpstan:baselines', + '> composer phpstan:fix', '> ```', '>', - '> Where an error below names an `@phpstan-ignore` comment in the code rather than a baseline entry, remove that comment instead; regenerating will not clear it.', + '> This also deletes any baseline left with no entries, and removes any `@phpstan-ignore` comment in the code that no longer ignores anything.', '>', - `> See [Regenerating the baselines](${ readmeUrl }#regenerating-the-baselines) for details.`, + `> See [Removing stale entries with \`--fix\`](${ readmeUrl }#removing-stale-entries-with---fix) for details, and for regenerating the baselines instead.`, ].join( '\n' ) ); } else { sections.push( [ diff --git a/.github/workflows/reusable-phpstan-static-analysis-v1.yml b/.github/workflows/reusable-phpstan-static-analysis-v1.yml index 321f98189cdf9..5853618014855 100644 --- a/.github/workflows/reusable-phpstan-static-analysis-v1.yml +++ b/.github/workflows/reusable-phpstan-static-analysis-v1.yml @@ -150,16 +150,20 @@ jobs: identifier: message.identifier ?? null, tip: message.tip ?? null, ignorable: message.ignorable, + fixable: message.fixable ?? false, } ); } } - // Errors that are not specific to a file, such as an `ignoreErrors` entry without a - // path that no longer matches. These cannot be ignored either. + // Errors that are not specific to a file, such as an `ignoreErrors` entry from a config + // file PHPStan cannot locate it in. These cannot be ignored either. for ( const message of report.errors ) { - errors.push( { file: null, line: null, message, identifier: null, tip: null, ignorable: false } ); + errors.push( { file: null, line: null, message, identifier: null, tip: null, ignorable: false, fixable: false } ); } + // PHPStan marks an error it can fix itself, such as a stale baseline entry, as fixable. + const fixHint = 'This can be fixed automatically by running `composer phpstan:fix` (or `npm run typecheck:php:fix`).'; + for ( const error of errors ) { const properties = { title: error.identifier ? `PHPStan: ${ error.identifier }` : 'PHPStan' }; if ( error.file ) { @@ -168,7 +172,7 @@ jobs: if ( error.line ) { properties.startLine = error.line; } - const message = error.tip ? `${ error.message }\n${ error.tip }` : error.message; + const message = [ error.message, error.tip, error.fixable ? fixHint : null ].filter( Boolean ).join( '\n' ); if ( error.ignorable ) { core.warning( message, properties ); } else { @@ -176,7 +180,8 @@ jobs: } } - // Tips are left out of the results, since they are only shown in the annotations, next to the code. + // Tips and whether an error is fixable are left out of the results, since they are only shown in the + // annotations, next to the code. // // The commit analyzed is the pull request merged into its base branch, whose second parent is the head // commit. The files with errors that differ between the two are those the base branch has changed, in @@ -196,7 +201,7 @@ jobs: fs.writeFileSync( `${ process.env.RUNNER_TEMP }/phpstan-results/results.json`, JSON.stringify( { pull_request: context.payload.pull_request.number, changed_in_base: changedInBase, - errors: errors.map( ( { tip, ...error } ) => error ), + errors: errors.map( ( { tip, fixable, ...error } ) => error ), } ) ); } @@ -266,21 +271,21 @@ jobs: **If you fixed the error, this is expected.** Each baseline entry records an exact count for a specific file, so that a new occurrence of an already baselined error is reported rather than absorbed. That same exactness means fixing one leaves the baseline describing a state that no longer exists. There is nothing to fix in the code; the baselines just need to catch up. - Regenerate them and commit the result: + Remove the stale entries, and commit the result: ```bash - npm run typecheck:php:baselines + npm run typecheck:php:fix ``` or, outside the Docker environment: ```bash - composer phpstan:baselines + composer phpstan:fix ``` - That rewrites the files under [`tests/phpstan/baselines`][baselines], deletes any whose errors are now all fixed, and updates the list of them in `phpstan.neon.dist`. + That runs PHPStan with `--fix`, which lowers the counts in the files under [`tests/phpstan/baselines`][baselines], removes the entries that no longer match, and removes any `@phpstan-ignore` annotation in the code that no longer ignores anything. It then deletes any baseline left with no entries, and updates the list of them in `phpstan.neon.dist`. - Where the report names an `@phpstan-ignore` annotation in the code rather than a baseline entry, remove that annotation instead; regenerating will not clear it. + Alternatively, regenerate the baselines with `npm run typecheck:php:baselines` or `composer phpstan:baselines`. That also deletes any baseline whose errors are now all fixed, and updates the list of them in `phpstan.neon.dist`, but it does not clear an `@phpstan-ignore` annotation. See [`tests/phpstan/README.md`][readme] for details. @@ -294,7 +299,7 @@ jobs: echo "[readme]: ${README_URL}" } >> "${GITHUB_STEP_SUMMARY}" - echo "::error title=PHPStan baselines are out of date::An ignored error no longer occurs, or occurs a different number of times. If you fixed it, that is expected: run \`npm run typecheck:php:baselines\` or \`composer phpstan:baselines\` and commit the updated baselines. See ${README_URL}" + echo "::error title=PHPStan baselines are out of date::An ignored error no longer occurs, or occurs a different number of times. If you fixed it, that is expected: run \`npm run typecheck:php:fix\` or \`composer phpstan:fix\` and commit the updated baselines. See ${README_URL}" else echo "::error title=PHPStan reported errors that cannot be ignored::PHPStan reported ${unignorable} error(s) that cannot be ignored or reported as warnings. See the error annotations for details, and ${README_URL} for how to run the analysis locally." fi diff --git a/composer.json b/composer.json index e20f4c7806b6e..fe6d3fa924bc9 100644 --- a/composer.json +++ b/composer.json @@ -77,6 +77,7 @@ "scripts": { "phpstan": "@php ./vendor/bin/phpstan analyse --memory-limit=2G", "phpstan:baselines": [ "Composer\\Config::disableProcessTimeout", "@php ./tests/phpstan/generate-baselines.php" ], + "phpstan:fix": [ "Composer\\Config::disableProcessTimeout", "@phpstan --configuration=phpstan.neon.dist --fix", "@phpstan:baselines --prune", "@phpstan --configuration=phpstan.neon.dist" ], "compat": "@php ./vendor/squizlabs/php_codesniffer/bin/phpcs --standard=phpcompat.xml.dist --report=summary,source", "format": "@php ./vendor/squizlabs/php_codesniffer/bin/phpcbf --report=summary,source", "lint": "@php ./vendor/squizlabs/php_codesniffer/bin/phpcs --report=summary,source", diff --git a/package.json b/package.json index d8e955f0d9809..a967f47655347 100644 --- a/package.json +++ b/package.json @@ -154,6 +154,7 @@ "test:visual": "wp-scripts test-playwright --config tests/visual-regression/playwright.config.js", "typecheck:php": "node ./tools/local-env/scripts/docker.js run --rm php composer phpstan --", "typecheck:php:baselines": "node ./tools/local-env/scripts/docker.js run --rm php composer phpstan:baselines --", + "typecheck:php:fix": "node ./tools/local-env/scripts/docker.js run --rm php composer phpstan:fix --", "gutenberg:copy": "node tools/gutenberg/copy.js", "gutenberg:verify": "node tools/gutenberg/utils.js", "gutenberg:download": "node tools/gutenberg/download.js && npm run build:gutenberg" diff --git a/tests/phpstan/README.md b/tests/phpstan/README.md index e263cd409036d..62003ca812d6a 100644 --- a/tests/phpstan/README.md +++ b/tests/phpstan/README.md @@ -50,7 +50,7 @@ The analysis also runs on every pull request that changes PHP files or the PHPSt - As annotations on the changed files. Most errors are annotated as warnings, which do not fail the check. - In a comment on the pull request, which lists every error in a table along with the commit it was first reported for. The comment is updated as new commits are pushed, moving each error that a commit resolves into a collapsed list of resolved errors, which also names that commit. Once all of the errors are resolved, the comment is marked as resolved and collapsed. -Some errors cannot be ignored, and so cannot be reported as warnings either. These fail the check. The most common is an [out-of-date baseline](#regenerating-the-baselines), which is reported when an error recorded in a baseline was fixed without the baseline being regenerated. Such an error is reported against a file rather than a line in it, so its annotation does not appear alongside the changes, and it would easily go unnoticed if the check passed. +Some errors cannot be ignored, and so cannot be reported as warnings either. These fail the check. The most common is an [out-of-date baseline](#removing-stale-entries-with---fix), which is reported when an error recorded in a baseline was fixed without the baseline being updated. Such an error is reported against a file rather than a line in it, so its annotation does not appear alongside the changes, and it would easily go unnoticed if the check passed. ## The PHPStan configuration @@ -216,11 +216,40 @@ Every entry is scoped to the file the error occurs in and carries an exact occur Both the path and the count matter. A new occurrence of an already baselined error does not match the entry, even in a file that is already listed, and is reported as a new error. That is the point of recording them this way: the baselines describe exactly what exists today, so nothing new slips in behind them. -The consequence is that **fixing a baselined error means regenerating its baseline as part of the same change**, because the count no longer matches. A count that no longer matches is reported as an `ignore.count` error, which PHPStan does not allow to be ignored or baselined. +The consequence is that **fixing a baselined error means updating its baseline as part of the same change**, because the count no longer matches. A count that no longer matches is reported as an `ignore.count` error, and an entry that matches nothing at all as an `ignore.unmatched` error, neither of which PHPStan allows to be ignored or baselined. Either [remove the stale entries with `--fix`](#removing-stale-entries-with---fix), or [regenerate the baselines](#regenerating-the-baselines). + +### Removing stale entries with `--fix` + +PHPStan marks an error it can fix automatically with 🔧, and ends its report by saying how many there are. Stale ignores are among them: a baseline entry whose count is now too high, or that matches nothing at all, and an `@phpstan-ignore` comment that no longer ignores anything. Fix them in place with: + +```bash +npm run typecheck:php:fix +``` + +or, outside the Docker environment: + +```bash +composer phpstan:fix +``` + +This runs three steps, each only when the one before it succeeds: + +1. PHPStan with its `--fix` option, which lowers the count of the entry, or removes the entry, leaving the rest of the baseline as it was. It is usually much faster than regenerating, since it reuses the result cache of the run that reported the errors. Unlike regenerating, it also removes an `@phpstan-ignore` comment that no longer matches. +2. `composer phpstan:baselines -- --prune`, which deletes any baseline left with no entries, along with its `includes` entry in `phpstan.neon.dist`. Removing the last entry of a baseline leaves the file behind, as `ignoreErrors: []`. Pruning analyzes nothing, so it can also be run on its own. +3. The analysis once more, which reuses the result cache and so is quick. PHPStan's `--fix` reports only what it fixed, and succeeds even when other errors remain, so this is what shows the errors that are left. + +Keep in mind: + +- **It fails when nothing is fixable.** PHPStan then says `No fixable errors found` and stops the script there, so run `composer phpstan` to see whether any errors remain. +- **It always analyzes the whole tree with `phpstan.neon.dist`**, whatever a local `phpstan.neon` says. PHPStan only reports stale ignores when it analyzes every path, and a configuration at another rule level can report a baselined error with a different message, in which case `--fix` would remove an entry that the default configuration still needs. +- **It fixes every fixable error the run reports**, not only stale ignores. A few other errors can also be fixed automatically, such as a [useless cast](https://phpstan.org/error-identifiers/cast.useless), so check the diff before committing it. +- PHPStan's `--fix-ignored`, run as `composer phpstan -- --fix-ignored`, goes further and also fixes the fixable errors which a baseline currently ignores. That is a way to work through a baseline, but expect larger diffs, and follow it with `composer phpstan:baselines -- --prune`. + +The feature is marked experimental in PHPStan. See [the PHPStan documentation](https://phpstan.org/user-guide/command-line-usage) for details. ### Regenerating the baselines -The baselines are generated, and should not be edited by hand. Regenerate them with: +The baselines are generated, and should not be edited by hand, other than by `--fix` as described [above](#removing-stale-entries-with---fix). Regenerate them with: ```bash npm run typecheck:php:baselines @@ -241,6 +270,9 @@ npm run typecheck:php:baselines -- --identifier=isset.variable --identifier=empt # print every error as one baseline, writing nothing: npm run typecheck:php:baselines -- --combined +# without analyzing anything, delete the baselines that list no entries: +npm run typecheck:php:baselines -- --prune + # the remaining options: npm run typecheck:php:baselines -- --help ``` @@ -252,6 +284,7 @@ composer phpstan:baselines composer phpstan:baselines -- --identifier=variable.undefined composer phpstan:baselines -- --combined +composer phpstan:baselines -- --prune composer phpstan:baselines -- --help ``` diff --git a/tests/phpstan/generate-baselines.php b/tests/phpstan/generate-baselines.php index 427140fb82b99..7f28af6bcdc47 100644 --- a/tests/phpstan/generate-baselines.php +++ b/tests/phpstan/generate-baselines.php @@ -18,6 +18,12 @@ * baseline, and retiring one that has reached zero, therefore need no manual * edit of the configuration. * + * With `--prune`, nothing is analyzed: the baselines that no longer list any + * entries are deleted, and the list in the configuration is updated to match. That + * completes running PHPStan with `--fix`, which removes the entries of fixed + * errors in place but leaves a baseline it has emptied behind, and so + * `composer phpstan:fix` runs it with `--prune` after `--fix`. + * * The intermediate baseline is generated in PHPStan's PHP format and read back * with `require`, so the entries arrive as an array. Nothing has to parse, or * re-escape, the message patterns. @@ -29,6 +35,7 @@ * composer phpstan:baselines -- --identifier=variable.undefined,isset.variable * composer phpstan:baselines -- --identifier=isset.variable --identifier=empty.variable * composer phpstan:baselines -- --combined > all-errors.neon + * composer phpstan:baselines -- --prune * * @package WordPress */ @@ -76,6 +83,7 @@ $memory_limit = '2G'; $only_identifiers = array(); $combined = false; +$prune = false; foreach ( $args as $arg ) { if ( '--help' === $arg || '-h' === $arg ) { @@ -88,6 +96,11 @@ continue; } + if ( '--prune' === $arg ) { + $prune = true; + continue; + } + if ( 1 === preg_match( '/^--identifier=(.+)$/', $arg, $matches ) ) { foreach ( explode( ',', $matches[1] ) as $identifier ) { $identifier = trim( $identifier ); @@ -125,6 +138,57 @@ exit( 1 ); } +/* + * Pruning needs no analysis. Running PHPStan with `--fix` lowers the counts of the + * entries whose errors were fixed and removes those that no longer match, but when + * it removes the last entry of a baseline it leaves the file behind, as + * `ignoreErrors: []`. That file suppresses nothing, so all that remains is to + * delete it and drop it from the configuration's `includes`, which is known from + * the file alone. + */ +if ( $prune ) { + if ( $combined ) { + fwrite( STDERR, "--prune and --combined cannot be used together.\n" ); + exit( 1 ); + } + + $pruned = 0; + + foreach ( find_baselines( $output_dir ) as $file ) { + $identifier = basename( $file, '.neon' ); + + if ( $only_identifiers && ! in_array( $identifier, $only_identifiers, true ) ) { + continue; + } + + $is_empty = is_empty_baseline( read_file( $file ) ); + + if ( null === $is_empty ) { + fwrite( STDERR, sprintf( "%s: no `ignoreErrors` list found, left untouched.\n", $output_option . '/' . basename( $file ) ) ); + continue; + } + + if ( ! $is_empty ) { + continue; + } + + if ( ! unlink( $file ) ) { + fwrite( STDERR, sprintf( "%s: could not be deleted.\n", $output_option . '/' . basename( $file ) ) ); + exit( 1 ); + } + + printf( "%s: no entries remain, file deleted.\n", $output_option . '/' . basename( $file ) ); + ++$pruned; + } + + if ( 0 === $pruned ) { + echo "No empty baselines found.\n"; + } + + update_config_includes( $config_path, $config_option, $output_dir ); + exit( 0 ); +} + /* * All three temporary files sit beside the configuration, and so inside the * repository, for two separate reasons. @@ -346,6 +410,11 @@ function get_usage(): string { Default: tests/phpstan/baselines --combined Print one combined baseline to stdout instead of writing per-identifier files. Nothing is written to disk. + --prune Without analyzing anything, delete the baselines that no + longer list any entries, such as one emptied by running + PHPStan with --fix, and update the configuration's + `includes` to match. With --identifier, only those + baselines are considered. --memory-limit= Passed through to PHPStan. Default: 2G -h, --help Show this message. @@ -363,13 +432,19 @@ function get_usage(): string { Inspect everything as one baseline without writing any files: composer phpstan:baselines -- --combined + Remove the entries of fixed errors, and any baseline left empty: + composer phpstan:fix + + Remove only the baselines that list no entries: + composer phpstan:baselines -- --prune + TEXT; } /** * Reads a file, failing loudly rather than continuing with false. * - * @param non-falsy-string $path Absolute path to the file. + * @param non-empty-string $path Absolute path to the file. * @return string File contents. */ function read_file( string $path ): string { @@ -532,6 +607,53 @@ function find_baselines( string $output_dir ): array { return $files; } +/** + * Determines whether a baseline ignores no errors. + * + * That is the case when its `ignoreErrors` is an empty inline list, as PHPStan's + * `--fix` writes after removing the last entry, or a key with nothing nested under + * it. Only whole-line comments are skipped, since a message pattern such as + * `'#^…$#'` contains `#` and so a trailing comment cannot be told apart from it + * without parsing the NEON. + * + * @param string $contents Baseline file contents. + * @return bool|null Whether the baseline is empty, or null when it has no `ignoreErrors` key. + */ +function is_empty_baseline( string $contents ): ?bool { + $lines = explode( "\n", $contents ); + + foreach ( $lines as $i => $line ) { + if ( 1 !== preg_match( '/^(\s*)ignoreErrors:\s*(.*)$/', $line, $matches ) ) { + continue; + } + + $value = trim( $matches[2] ); + + if ( '' !== $value && '#' !== $value[0] ) { + return 1 === preg_match( '/^\[\s*\](\s*#.*)?$/', $value ); + } + + // Nothing after the key, so its value is whatever is nested beneath it. + $indent = strlen( $matches[1] ); + + foreach ( array_slice( $lines, $i + 1 ) as $next ) { + $trimmed = trim( $next ); + + if ( '' === $trimmed || '#' === $trimmed[0] ) { + continue; + } + + $next_indent = strlen( $next ) - strlen( ltrim( $next ) ); + + return $next_indent <= $indent; + } + + return true; + } + + return null; +} + /** * Rewrites the managed region of the configuration's `includes` list. *