From dfca1b70c547325a6366c3fbcda5eaccec7de7b4 Mon Sep 17 00:00:00 2001 From: Sander Muller Date: Mon, 3 Aug 2026 15:11:08 +0200 Subject: [PATCH] Add a `make lint-diff` convention check and a pre-commit step in CLAUDE.md `make lint-diff` (build/lint-diff.php) scans the lines a branch adds for the conventions that recur on review and prints each with its file and line: instanceof of a Type that has a method alternative (StringType -> isString(), EnumCaseObjectType -> getEnumCaseObject()), get_class() type dispatch, and a new inline @phpstan-ignore. Structural classes with no method alternative (NeverType, TemplateType, UnionType, MixedType) are not flagged, because instanceof is idiomatic for those. It is advisory (exit 0; --strict to fail); a reviewed exception is acknowledged with phpstan-lint-ok on the line. CLAUDE.md gains a "run make lint-diff and resolve every finding" step so the check is part of the workflow, plus a note that make phpstan is not the whole static-analysis gate. Co-Authored-By: Claude Opus 4.8 (1M context) --- CLAUDE.md | 2 + Makefile | 4 + build/lint-diff.php | 184 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 190 insertions(+) create mode 100644 build/lint-diff.php diff --git a/CLAUDE.md b/CLAUDE.md index 7b1701382c2..b249f5918c5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -291,6 +291,8 @@ Recent work on PHP 8.5 support shows the pattern: ## Workflow for bug fixes - **One fix, one commit (or one PR)**. Bench scripts and tests for a fix go in the same commit as the source change, not separately. +- **Before committing, run `make lint-diff` and resolve every finding.** It flags, in the lines you added, the conventions above that a mechanical check can catch: `instanceof` of a `Type` that has a method alternative (`instanceof StringType` → `->isString()`, `instanceof EnumCaseObjectType` → `->getEnumCaseObject()`; structural types like `NeverType`/`TemplateType`/`UnionType` are deliberately not flagged), `get_class()` used for type dispatch, and a new inline `@phpstan-ignore`. It is advisory (`--strict` to fail); fix each line, or add `phpstan-lint-ok` on the line with a reason for a genuine exception. +- **`make phpstan` is not the whole static-analysis gate.** The Mutation Testing job runs a stricter analysis that adds phpstan-phpunit's rules, so a `make phpstan`-clean change can still fail CI — e.g. `#[RequiresPhp('^8.1')]` passes `make phpstan` but the rule wants the complete `>= 8.1.0` form. Check that job for test-only or attribute changes. - **Commit messages and PR titles describe the change, not the bug they close.** Prefer "Do not subtract TemplateType from TemplateType" over "Fix #14459: type subtraction". Issue closure goes in the PR body via `Closes https://github.com/phpstan/phpstan/issues/`. - **Standard reproducer command**: `bin/phpstan analyse -l 8 --debug` (add `-vvv` for hangs and infinite loops). Files under `tests/bench/data` are not directly runnable as PHPStan inputs — copy the reproducing snippet into a `test.php` at the repo root first, then analyse that. - **Debug helpers in analysed code**: `\PHPStan\dumpType($expr)` prints the inferred type of an expression at that point; `\PHPStan\debugScope()` prints the current scope. Inside `NodeScopeResolver` itself, `var_dump($scope->debug())` is the canonical inspection point. Reach for these before guessing what PHPStan is doing. diff --git a/Makefile b/Makefile index bfaaa6b1923..0f5d3a7172d 100644 --- a/Makefile +++ b/Makefile @@ -11,6 +11,10 @@ tests-integration: install-paratest tests-golden-reflection: php vendor/bin/phpunit tests/PHPStan/Reflection/ReflectionProviderGoldenTest.php +.PHONY: lint-diff +lint-diff: + php build/lint-diff.php + lint: XDEBUG_MODE=off php vendor/bin/parallel-lint --colors \ --exclude tests/PHPStan/Analyser/data \ diff --git a/build/lint-diff.php b/build/lint-diff.php new file mode 100644 index 00000000000..6f084ab8520 --- /dev/null +++ b/build/lint-diff.php @@ -0,0 +1,184 @@ + isString(), EnumCaseObjectType -> getEnumCaseObject(), ...). + * Structural classes with no such method (NeverType, TemplateType, UnionType, + * MixedType) are intentionally not flagged; instanceof is idiomatic for those. + * - get_class(), which is brittle for type dispatch. + * - a new inline phpstan-ignore comment (fix the root cause; the baseline is + * for pre-existing errors only). + * + * Advisory by design: it prints candidates and exits 0. Pass --strict to exit 1 when + * anything is flagged. Test-data fixtures (tests/**\/data/**) are skipped, and a + * reviewed exception is acknowledged with `phpstan-lint-ok` on the line. + * + * Usage: php build/lint-diff.php [--strict] [] + * (default base: upstream/2.2.x, then origin/2.2.x, then 2.2.x) + */ + +/** + * @param list $command + * @return list + */ +function lintDiffRun(array $command): array +{ + $descriptors = [1 => ['pipe', 'w'], 2 => ['pipe', 'w']]; + $process = proc_open($command, $descriptors, $pipes); + if ($process === false) { + return []; + } + + $stdout = stream_get_contents($pipes[1]); + fclose($pipes[1]); + fclose($pipes[2]); + proc_close($process); + + if (!is_string($stdout) || $stdout === '') { + return []; + } + + return explode("\n", rtrim($stdout, "\n")); +} + +function lintDiffRefExists(string $ref): bool +{ + $descriptors = [1 => ['pipe', 'w'], 2 => ['pipe', 'w']]; + $process = proc_open(['git', 'rev-parse', '--verify', '--quiet', $ref], $descriptors, $pipes); + if ($process === false) { + return false; + } + + // Drain the pipes before closing, otherwise git gets SIGPIPE writing the SHA + // and exits non-zero, which would look like a missing ref. + stream_get_contents($pipes[1]); + stream_get_contents($pipes[2]); + fclose($pipes[1]); + fclose($pipes[2]); + + return proc_close($process) === 0; +} + +$strict = false; +$base = null; +foreach (array_slice($argv ?? [], 1) as $arg) { + if ($arg === '--strict') { + $strict = true; + continue; + } + + $base = $arg; +} + +if ($base === null) { + foreach (['upstream/2.2.x', 'origin/2.2.x', '2.2.x'] as $candidate) { + if (lintDiffRefExists($candidate)) { + $base = $candidate; + break; + } + } +} + +if ($base === null) { + fwrite(STDERR, "lint-diff: no base ref found; pass one as an argument\n"); + exit(2); +} + +$diff = lintDiffRun(['git', 'diff', '--unified=0', $base, '--', '*.php']); + +/** @var list $addedLines */ +$addedLines = []; +$currentFile = null; +$lineNumber = 0; +foreach ($diff as $row) { + if (substr($row, 0, 6) === '+++ b/') { + $currentFile = substr($row, 6); + continue; + } + + if (substr($row, 0, 3) === '@@ ') { + if (preg_match('#\+(\d+)#', $row, $matches) === 1) { + $lineNumber = (int) $matches[1]; + } + + continue; + } + + if (substr($row, 0, 3) === '+++') { + continue; + } + + if (substr($row, 0, 1) !== '+') { + continue; + } + + if ($currentFile !== null) { + $addedLines[] = [$currentFile, $lineNumber, substr($row, 1)]; + } + + $lineNumber++; +} + +/** @var list $checks */ +$checks = [ + [ + 'label' => 'instanceof a Type with a method alternative', + 'pattern' => '#\binstanceof\s+\\\\?((Constant)?(String|Integer|Float|Boolean)Type|ClassStringType|IntegerRangeType|NullType|EnumCaseObjectType)\b#', + 'guidance' => 'These classes have a Type method: use isString()/isInteger()/isFloat()/isBoolean()/isNull()/isClassString()/getEnumCaseObject(), or isSuperTypeOf(). Structural types (NeverType, TemplateType, UnionType, MixedType) are not flagged. Genuine exception? add phpstan-lint-ok on the line.', + ], + [ + 'label' => 'get_class() (brittle for type dispatch)', + 'pattern' => '#\bget_class\s*\(#', + 'guidance' => 'Prefer a Type method over get_class() dispatch. Not type dispatch (e.g. an error message)? add phpstan-lint-ok on the line.', + ], + [ + 'label' => 'new inline @phpstan-ignore', + 'pattern' => '#@phpstan-ignore#', + 'guidance' => 'Prefer fixing the root cause; the baseline is for pre-existing errors only. Justified and documented? add phpstan-lint-ok on the line.', + ], +]; + +$flagged = false; +foreach ($checks as $check) { + $hits = []; + foreach ($addedLines as [$file, $number, $content]) { + if (strpos($content, 'phpstan-lint-ok') !== false) { + continue; + } + + if (preg_match('#^tests/.*/data/#', $file) === 1) { + continue; + } + + // The tool's own patterns and guidance carry the trigger strings as data. + if ($file === 'build/lint-diff.php') { + continue; + } + + if (preg_match($check['pattern'], $content) !== 1) { + continue; + } + + $hits[] = sprintf(' %s:%d %s', $file, $number, trim($content)); + } + + if ($hits === []) { + continue; + } + + $flagged = true; + printf("\n[%s]\n%s\n => %s\n", $check['label'], implode("\n", $hits), $check['guidance']); +} + +if (!$flagged) { + printf("lint-diff: clean vs %s\n", $base); + exit(0); +} + +printf("\nlint-diff: candidates in added lines vs %s (review above; advisory).\n", $base); +exit($strict ? 1 : 0);