From 115b3d022720d4e832c9013f70fbc93169b9b544 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Fri, 12 Apr 2024 09:40:10 -0400 Subject: [PATCH] phan: Allow deleting baseline files (#36857) In many cases it's pointless to keep around the empty baseline files, ideally they should never be repopulated. Update the tooling to allow for these to be deleted, in which case they won't be re-created by `--update-baseline`. But in case for some reason one does need to be re-created, add a `--force-update-baseline` option to allow for that. Also update the GitHub Action checking for baseline changes to handle these, and also to handle changes in pseudo-project baselines that wasn't done in #36467. For the moment I'm only actually removing the empty baselines for stuff Garage maintains. We can look at the rest in a followup. --- .../composer-plugin/.phan/baseline.php | 17 ------- .github/files/lint-project-structure.sh | 4 -- .github/workflows/tests.yml | 35 +++++++++++---- .phan/baseline.php | 17 ------- docs/monorepo.md | 2 +- .../packages/changelogger/.phan/baseline.php | 17 ------- .../add-phan-allow-removing-baselines | 5 +++ .../packages/codesniffer/.phan/baseline.php | 17 ------- .../add-phan-allow-removing-baselines | 5 +++ .../packages/ignorefile/.phan/baseline.php | 17 ------- .../add-phan-allow-removing-baselines | 5 +++ .../packages/phpcs-filter/.phan/baseline.php | 17 ------- .../add-phan-allow-removing-baselines | 5 +++ .../stub-generator/.phan/baseline.php | 17 ------- .../add-phan-allow-removing-baselines | 5 +++ tools/cli/commands/phan.js | 45 ++++++++++++++++--- .../cli/helpers/doc-parser/.phan/baseline.php | 17 ------- tools/e2e-commons/.phan/baseline.php | 17 ------- 18 files changed, 90 insertions(+), 174 deletions(-) delete mode 100644 .github/actions/tool-setup/composer-plugin/.phan/baseline.php delete mode 100644 .phan/baseline.php delete mode 100644 projects/packages/changelogger/.phan/baseline.php create mode 100644 projects/packages/changelogger/changelog/add-phan-allow-removing-baselines delete mode 100644 projects/packages/codesniffer/.phan/baseline.php create mode 100644 projects/packages/codesniffer/changelog/add-phan-allow-removing-baselines delete mode 100644 projects/packages/ignorefile/.phan/baseline.php create mode 100644 projects/packages/ignorefile/changelog/add-phan-allow-removing-baselines delete mode 100644 projects/packages/phpcs-filter/.phan/baseline.php create mode 100644 projects/packages/phpcs-filter/changelog/add-phan-allow-removing-baselines delete mode 100644 projects/packages/stub-generator/.phan/baseline.php create mode 100644 projects/packages/stub-generator/changelog/add-phan-allow-removing-baselines delete mode 100644 tools/cli/helpers/doc-parser/.phan/baseline.php delete mode 100644 tools/e2e-commons/.phan/baseline.php diff --git a/.github/actions/tool-setup/composer-plugin/.phan/baseline.php b/.github/actions/tool-setup/composer-plugin/.phan/baseline.php deleted file mode 100644 index 3df50068147ad..0000000000000 --- a/.github/actions/tool-setup/composer-plugin/.phan/baseline.php +++ /dev/null @@ -1,17 +0,0 @@ - [ - ], - // 'directory_suppressions' => ['src/directory_name' => ['PhanIssueName1', 'PhanIssueName2']] can be manually added if needed. - // (directory_suppressions will currently be ignored by subsequent calls to --save-baseline, but may be preserved in future Phan releases) -]; diff --git a/.github/files/lint-project-structure.sh b/.github/files/lint-project-structure.sh index b4ec73b621ce2..5aee4b5f5fea3 100755 --- a/.github/files/lint-project-structure.sh +++ b/.github/files/lint-project-structure.sh @@ -221,10 +221,6 @@ for PROJECT in projects/*/*; do EXIT=1 echo "::error file=$PROJECT/.phan/config.php::Project $SLUG has PHP files but does not contain .phan/config.php. Refer to Static Analysis in docs/monorepo.md." fi - if [[ ! -e "$PROJECT/.phan/baseline.php" ]]; then - EXIT=1 - echo "::error file=$PROJECT/.phan/baseline.php::Project $SLUG has PHP files but does not contain .phan/baseline.php. Refer to Static Analysis in docs/monorepo.md." - fi fi # - composer.json must exist. diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 26e6fb8343245..2772beda960c6 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -328,19 +328,36 @@ jobs: - name: Check baselines run: | # Anything changed (with a side of printing the diff) - if git diff --exit-code --ignore-matching-lines='^ // ' .phan/baseline.php 'projects/*/*/.phan/baseline.php'; then + if git diff --exit-code --ignore-matching-lines='^ // ' -- .phan/baseline.php '*/.phan/baseline.php'; then exit 0 fi # Collect which projects changed to suggest the right command. PROJECTS=() - if ! git diff --exit-code --name-only .phan/baseline.php &>/dev/null; then - PROJECTS+=( 'monorepo' ) - fi - for f in $( git -c core.quotepath=off diff --name-only 'projects/*/*/.phan/baseline.php' ); do - SLUG=${f%/.phan/baseline.php} - SLUG=${SLUG#projects/} - PROJECTS+=( "$SLUG" ) + for f in $( git -c core.quotepath=off diff --name-only -- .phan/baseline.php '*/.phan/baseline.php' ); do + if [[ "$f" == ".phan/baseline.php" ]]; then + SLUG=monorepo + elif [[ "$f" == projects/*/*/.phan/baseline.php ]]; then + SLUG=${f%/.phan/baseline.php} + SLUG=${SLUG#projects/} + elif SLUG=$( grep -v '^[ \t]*\/\/' .phan/monorepo-pseudo-projects.jsonc | jq -re --arg f "${f%.phan/baseline.php}" 'to_entries[] | select( .value == $f ) | .key' ); then + : # Ok + else + SLUG= + fi + if grep -q 'This baseline has no suppressions' "$f"; then + if [[ -n "$SLUG" ]]; then + echo "::error file=$f::This Phan baseline is now empty (good job!). You may remove it, or if you want to keep it (e.g. if you expect new unfixed issues to be added in the future) you can run \`jetpack phan --update-baseline $SLUG\` to update it." + else + echo "::error file=$f::This Phan baseline is now empty (good job!). You may remove it." + fi + elif [[ -n "$SLUG" ]]; then + PROJECTS+=( "$SLUG" ) + else + echo "::error file=$f::This Phan baseline has changed and should be updated. This Action was unable to determine the command needed to update it; please report this to the Garage team." + fi done - echo "::error::Phan baselines have changed (good job!). Run \`jetpack phan --update-baseline ${PROJECTS[*]}\` to update them." + if [[ ${#PROJECTS[@]} -gt 0 ]]; then + echo "::error::Phan baselines have changed (good job!). Run \`jetpack phan --update-baseline ${PROJECTS[*]}\` to update them." + fi exit 1 diff --git a/.phan/baseline.php b/.phan/baseline.php deleted file mode 100644 index 3df50068147ad..0000000000000 --- a/.phan/baseline.php +++ /dev/null @@ -1,17 +0,0 @@ - [ - ], - // 'directory_suppressions' => ['src/directory_name' => ['PhanIssueName1', 'PhanIssueName2']] can be manually added if needed. - // (directory_suppressions will currently be ignored by subsequent calls to --save-baseline, but may be preserved in future Phan releases) -]; diff --git a/docs/monorepo.md b/docs/monorepo.md index 3b071069241f1..c49a94cd1615e 100644 --- a/docs/monorepo.md +++ b/docs/monorepo.md @@ -205,7 +205,7 @@ We use eslint and phpcs to lint JavaScript and PHP code. Projects should comply ### Static Analysis -We use Phan for PHP static analysis.[^1] Configuration for a project resides in the `.phan/config.php` within the project, which should generally build on top of the `.phan/config.base.php` from the monorepo root. A baseline file also resides at `.phan/baseline.php` to allow for incremental fixing of errors. +We use Phan for PHP static analysis.[^1] Configuration for a project resides in the `.phan/config.php` within the project, which should generally build on top of the `.phan/config.base.php` from the monorepo root. A baseline file may also reside at `.phan/baseline.php` to allow for incremental fixing of errors. Phan in the monorepo should be run locally via [Jetpack's CLI tool](#first-time) as `jetpack phan`. Note that Phan soft-requires the [PHP ast extension](https://pecl.php.net/package/ast); while on Linux installing this is likely as easy as `sudo apt-get install php8.2-ast`, Mac users have reported having trouble. diff --git a/projects/packages/changelogger/.phan/baseline.php b/projects/packages/changelogger/.phan/baseline.php deleted file mode 100644 index 3df50068147ad..0000000000000 --- a/projects/packages/changelogger/.phan/baseline.php +++ /dev/null @@ -1,17 +0,0 @@ - [ - ], - // 'directory_suppressions' => ['src/directory_name' => ['PhanIssueName1', 'PhanIssueName2']] can be manually added if needed. - // (directory_suppressions will currently be ignored by subsequent calls to --save-baseline, but may be preserved in future Phan releases) -]; diff --git a/projects/packages/changelogger/changelog/add-phan-allow-removing-baselines b/projects/packages/changelogger/changelog/add-phan-allow-removing-baselines new file mode 100644 index 0000000000000..b3b9f6b080a87 --- /dev/null +++ b/projects/packages/changelogger/changelog/add-phan-allow-removing-baselines @@ -0,0 +1,5 @@ +Significance: patch +Type: removed +Comment: Remove empty Phan baseline. No change to functionality. + + diff --git a/projects/packages/codesniffer/.phan/baseline.php b/projects/packages/codesniffer/.phan/baseline.php deleted file mode 100644 index 3df50068147ad..0000000000000 --- a/projects/packages/codesniffer/.phan/baseline.php +++ /dev/null @@ -1,17 +0,0 @@ - [ - ], - // 'directory_suppressions' => ['src/directory_name' => ['PhanIssueName1', 'PhanIssueName2']] can be manually added if needed. - // (directory_suppressions will currently be ignored by subsequent calls to --save-baseline, but may be preserved in future Phan releases) -]; diff --git a/projects/packages/codesniffer/changelog/add-phan-allow-removing-baselines b/projects/packages/codesniffer/changelog/add-phan-allow-removing-baselines new file mode 100644 index 0000000000000..b3b9f6b080a87 --- /dev/null +++ b/projects/packages/codesniffer/changelog/add-phan-allow-removing-baselines @@ -0,0 +1,5 @@ +Significance: patch +Type: removed +Comment: Remove empty Phan baseline. No change to functionality. + + diff --git a/projects/packages/ignorefile/.phan/baseline.php b/projects/packages/ignorefile/.phan/baseline.php deleted file mode 100644 index 3df50068147ad..0000000000000 --- a/projects/packages/ignorefile/.phan/baseline.php +++ /dev/null @@ -1,17 +0,0 @@ - [ - ], - // 'directory_suppressions' => ['src/directory_name' => ['PhanIssueName1', 'PhanIssueName2']] can be manually added if needed. - // (directory_suppressions will currently be ignored by subsequent calls to --save-baseline, but may be preserved in future Phan releases) -]; diff --git a/projects/packages/ignorefile/changelog/add-phan-allow-removing-baselines b/projects/packages/ignorefile/changelog/add-phan-allow-removing-baselines new file mode 100644 index 0000000000000..b3b9f6b080a87 --- /dev/null +++ b/projects/packages/ignorefile/changelog/add-phan-allow-removing-baselines @@ -0,0 +1,5 @@ +Significance: patch +Type: removed +Comment: Remove empty Phan baseline. No change to functionality. + + diff --git a/projects/packages/phpcs-filter/.phan/baseline.php b/projects/packages/phpcs-filter/.phan/baseline.php deleted file mode 100644 index 3df50068147ad..0000000000000 --- a/projects/packages/phpcs-filter/.phan/baseline.php +++ /dev/null @@ -1,17 +0,0 @@ - [ - ], - // 'directory_suppressions' => ['src/directory_name' => ['PhanIssueName1', 'PhanIssueName2']] can be manually added if needed. - // (directory_suppressions will currently be ignored by subsequent calls to --save-baseline, but may be preserved in future Phan releases) -]; diff --git a/projects/packages/phpcs-filter/changelog/add-phan-allow-removing-baselines b/projects/packages/phpcs-filter/changelog/add-phan-allow-removing-baselines new file mode 100644 index 0000000000000..b3b9f6b080a87 --- /dev/null +++ b/projects/packages/phpcs-filter/changelog/add-phan-allow-removing-baselines @@ -0,0 +1,5 @@ +Significance: patch +Type: removed +Comment: Remove empty Phan baseline. No change to functionality. + + diff --git a/projects/packages/stub-generator/.phan/baseline.php b/projects/packages/stub-generator/.phan/baseline.php deleted file mode 100644 index 3df50068147ad..0000000000000 --- a/projects/packages/stub-generator/.phan/baseline.php +++ /dev/null @@ -1,17 +0,0 @@ - [ - ], - // 'directory_suppressions' => ['src/directory_name' => ['PhanIssueName1', 'PhanIssueName2']] can be manually added if needed. - // (directory_suppressions will currently be ignored by subsequent calls to --save-baseline, but may be preserved in future Phan releases) -]; diff --git a/projects/packages/stub-generator/changelog/add-phan-allow-removing-baselines b/projects/packages/stub-generator/changelog/add-phan-allow-removing-baselines new file mode 100644 index 0000000000000..b3b9f6b080a87 --- /dev/null +++ b/projects/packages/stub-generator/changelog/add-phan-allow-removing-baselines @@ -0,0 +1,5 @@ +Significance: patch +Type: removed +Comment: Remove empty Phan baseline. No change to functionality. + + diff --git a/tools/cli/commands/phan.js b/tools/cli/commands/phan.js index 382963daf0604..52233481a269e 100644 --- a/tools/cli/commands/phan.js +++ b/tools/cli/commands/phan.js @@ -55,6 +55,11 @@ export async function builder( yargs ) { type: 'boolean', description: 'Update the Phan baselines.', } ) + .option( 'force-update-baseline', { + type: 'boolean', + description: + 'Update the Phan baselines, even if no baseline currently exists. But please try not to use this, fix the issues instead.', + } ) .option( 'no-use-uncommitted-composer-lock', { type: 'boolean', description: "Don't use uncommitted composer.lock files.", @@ -106,6 +111,9 @@ export async function builder( yargs ) { description: 'Write the report to this file instead of to standard output.', } ) .check( argv => { + if ( argv.forceUpdateBaseline ) { + argv.updateBaseline = true; + } if ( argv.updateBaseline && ( argv[ 'allow-polyfill-parser' ] || argv[ 'force-polyfill-parser' ] ) @@ -276,12 +284,6 @@ export async function handler( argv ) { } else { phanArgs.push( '--progress-bar' ); } - if ( argv.baseline !== false ) { - phanArgs.push( '--load-baseline=.phan/baseline.php' ); - } - if ( argv.updateBaseline ) { - phanArgs.push( '--save-baseline=.phan/baseline.php' ); - } for ( const arg of [ 'automatic-fix', 'allow-polyfill-parser', @@ -327,7 +329,17 @@ export async function handler( argv ) { const projectPhanArgs = checkFilesByProject[ project ] ? [ ...phanArgs, '--include-analysis-file-list', checkFilesByProject[ project ].join( ',' ) ] - : phanArgs; + : [ ...phanArgs ]; + + // Baseline handling depends on whether the baseline exists. + const hasBaseline = + ( await fs.access( path.join( cwd, '.phan/baseline.php' ) ).catch( () => false ) ) !== false; + if ( hasBaseline && argv.baseline !== false ) { + projectPhanArgs.push( '--load-baseline=.phan/baseline.php' ); + } + if ( ( hasBaseline && argv.updateBaseline ) || argv.forceUpdateBaseline ) { + projectPhanArgs.push( '--save-baseline=.phan/baseline.php' ); + } let sstdout = process.stdout, sstderr = process.stderr; @@ -416,6 +428,25 @@ export async function handler( argv ) { } ); } await proc; + + // Don't re-create unneeded baselines with --force-update-baseline. + if ( ! hasBaseline && argv.forceUpdateBaseline ) { + if ( argv.v ) { + sstderr.write( + 'Removing that baseline, no issues were reported so it should be empty.\n' + ); + } + try { + await fs.unlink( path.join( cwd, '.phan/baseline.php' ) ); + } catch ( e ) { + if ( argv.v ) { + sstderr.write( + // prettier-ignore + `Failed to unlink ${ path.join( cwd, '.phan/baseline.php' ) }: ${ e.message }\n` + ); + } + } + } } catch ( e ) { let json; try { diff --git a/tools/cli/helpers/doc-parser/.phan/baseline.php b/tools/cli/helpers/doc-parser/.phan/baseline.php deleted file mode 100644 index 3df50068147ad..0000000000000 --- a/tools/cli/helpers/doc-parser/.phan/baseline.php +++ /dev/null @@ -1,17 +0,0 @@ - [ - ], - // 'directory_suppressions' => ['src/directory_name' => ['PhanIssueName1', 'PhanIssueName2']] can be manually added if needed. - // (directory_suppressions will currently be ignored by subsequent calls to --save-baseline, but may be preserved in future Phan releases) -]; diff --git a/tools/e2e-commons/.phan/baseline.php b/tools/e2e-commons/.phan/baseline.php deleted file mode 100644 index 3df50068147ad..0000000000000 --- a/tools/e2e-commons/.phan/baseline.php +++ /dev/null @@ -1,17 +0,0 @@ - [ - ], - // 'directory_suppressions' => ['src/directory_name' => ['PhanIssueName1', 'PhanIssueName2']] can be manually added if needed. - // (directory_suppressions will currently be ignored by subsequent calls to --save-baseline, but may be preserved in future Phan releases) -];