diff --git a/.github/workflows/static-analysis.yml b/.github/workflows/static-analysis.yml index 281e8170..0824a4d3 100644 --- a/.github/workflows/static-analysis.yml +++ b/.github/workflows/static-analysis.yml @@ -25,11 +25,13 @@ jobs: - name: PHPStan run: composer phpstan -- --error-format=github - baseline-retired: + baseline-shrink: runs-on: ubuntu-latest - name: Baselines stay retired + name: Baselines shrink or hold steps: - uses: actions/checkout@v7 + with: + fetch-depth: 0 # bin/check-baseline-shrink reads origin/main via `git show` - name: Setup PHP uses: shivammathur/setup-php@v2 diff --git a/bin/check-baseline-shrink b/bin/check-baseline-shrink index e59ce3f4..1d8fce9e 100755 --- a/bin/check-baseline-shrink +++ b/bin/check-baseline-shrink @@ -3,24 +3,50 @@ declare(strict_types=1); -// Fails when either guardrail baseline file exists. Both baselines were -// drained to zero by build-manifest step 21 and are gone for good; the -// checker now enforces that they stay gone. A new violation must be routed -// through its authority, never absorbed into a baseline. +// Fails when a guardrail baseline grows relative to a base ref (default +// origin/main). RFC 1 §8.1: baseline totals only shrink; relocating a frozen +// violation is allowed, absorbing a new one is not. +// +// A baseline file may be absent (treated as zero) or present. The hard lock — +// baselines must not exist at all — is deferred to build-manifest step-32 +// (retire); until then, tighten rules that surface pre-existing residuals need +// somewhere to record them. + +$baseRef = $argv[1] ?? 'origin/main'; + +$phpstanTotal = function (string $contents): int { + preg_match_all('/^\s*count:\s*(\d+)/m', $contents, $matches); + return (int) array_sum($matches[1]); +}; + +$deptracTotal = function (string $contents): int { + preg_match_all('/^\s+-\s+\S/m', $contents, $matches); + return count($matches[0]); +}; + +$contentsAt = function (string $ref, string $file): string { + $out = shell_exec('git show ' . escapeshellarg("$ref:$file") . ' 2>/dev/null'); + return is_string($out) ? $out : ''; +}; $files = [ - 'phpstan-baseline.neon', - 'deptrac.baseline.yaml', + 'phpstan-baseline.neon' => $phpstanTotal, + 'deptrac.baseline.yaml' => $deptracTotal, ]; $failed = false; -foreach ($files as $file) { - if (is_file($file)) { +foreach ($files as $file => $total) { + $base = $total($contentsAt($baseRef, $file)); + $head = $total(is_file($file) ? (string) file_get_contents($file) : ''); + printf("%s: %d -> %d\n", $file, $base, $head); + if ($head > $base) { fwrite(STDERR, sprintf( - "%s exists: baselines are permanently retired. Route the new" . - " violation through its authority; see" . - " docs/architecture/enforcement-edits.md.\n", + "%s grew (%d -> %d): baselines only shrink. Route the new" . + " violation through its authority. Growth is permitted only" . + " for a newly added check; see docs/architecture/enforcement-edits.md.\n", $file, + $base, + $head, )); $failed = true; } diff --git a/docs/architecture/build-manifest.md b/docs/architecture/build-manifest.md index e6ca7ab0..09d4d869 100644 --- a/docs/architecture/build-manifest.md +++ b/docs/architecture/build-manifest.md @@ -13,7 +13,7 @@ One signal: both baseline files are deleted, and the issues named below are clos - A step's `Done` clause is its whole acceptance. A reviewer checks that clause and nothing else. - Background reading is RFC 1 §4 (the invariants). Plan 0002 is history, not work. - Regex stays in the files `phpstan.neon` already allows it in. `TextFallbackHelper` is the text branch every positional question calls; it holds regex and nothing else. -- When step-27 lands, this file and its two driver skills are gone, and work returns to plain issue flow: pick an issue, "do #xxx". +- When step-32 lands, this file and its two driver skills are gone, and work returns to plain issue flow: pick an issue, "do #xxx". ## Steps @@ -42,6 +42,11 @@ One signal: both baseline files are deleted, and the issues named below are clos - [x] **step-22** — `SymbolCandidates` absorbs `NamespaceCandidates`'s namespace-tree navigation, so every position that offers symbols calls one source. When the prefix is namespace-qualified or `\`-rooted, `SymbolCandidates` navigates the tree internally; otherwise it does flat lookup. `CompletionHandler` has one symbol-completion call per position instead of parallel `SymbolCandidates` + `NamespaceCandidates` calls. Done: `NamespaceCandidates` is deleted; `getClassCompletions` and `getExpressionCompletions` each call `SymbolCandidates` once; `\`-prefixed function and constant completion works at expression start; filtered positions (`implements`, `extends`, trait `use`, `catch`, `#[…]`) no longer offer functions or constants via namespace navigation. - [x] **step-23** — `SymbolResolver::resolveCallable` answers every callable-shaped node (`FuncCall`, `MethodCall`, `NullsafeMethodCall`, `StaticCall`, `New_`, `Attribute`) by delegating to `ExpressionResolver`: `FuncCall`, `MethodCall`, `NullsafeMethodCall`, and `StaticCall` go through one `ExpressionResolver::resolve` call, and `New_` and `Attribute` go through one `ExpressionResolver::resolveConstructor` call (the constructor question is separate from the type question `resolve(New_)` answers). The method-call path applies late-bound return-type resolution the same way the static-call path does. Done: `resolveCallable` has no `match`/`switch`/`instanceof` on the call-node kind and no direct `MemberResolver::findMethod` call; hover on `$obj->foo()` where `foo(): static` reports the receiver's class the same way hover on `Foo::bar()` does; a parity test asserts hover-signature agreement across all callable node kinds for `self`/`static`/`parent` return types. - [x] **step-24** — Member lookup in `ExpressionResolver` is one function taking the receiver expression, the member name, and a kind-specific finder; `resolveMethodCall`, `resolveStaticCall`, `resolvePropertyFetch`, and `resolveStaticPropertyFetch` call it. Done: the four methods share one member-lookup helper; adding a fifth member-access node kind is one call site, not four; the existing hover, definition, completion, and signature-help suites remain green. -- [ ] **step-25** — `ExpressionResolver::docblockForExpression` is one line reading the resolved symbol's docblock — no per-kind branch. Done: the method has no `match`/`instanceof` on the expression node; foreach element-type inference from `@return list` (and equivalent `@var` docblocks on properties and constants) works on `Foo::items()`, `Foo::$items`, and `Foo::ITEMS` the same way it works on `$this->items()`; a test covers each callable and member-access kind. -- [ ] **step-26** — The three late-binding keywords (`self`, `static`, `parent`) resolve in one place. `MemberAccessDetector`'s text and AST paths, `ScopeFinder`, and any other reader route through one function (extending `ScopeFinder::resolveClassName` or `LateBindingKeyword`, whichever is the natural home); the `parent`-of-non-`Class_` guard exists there once. Done: no `src/` file outside that home compares against the three keyword literals in a class-name-resolution context; a text-path and an AST-path test exercise the same behavior through one code path. -- [ ] **step-27** — Retire the rebuild. Delete this manifest and the `do-next` and `review-slice` skills (this row authorises the `.claude/` and policy deletions), and drop the manifest read from `SymbolCoverageGridTest` so a blocker must name an issue or an RFC section. Done: this file and both skills are gone; `composer test` is green; work continues as plain issues. +- [ ] **step-25** — `ExpressionResolver::docblockForExpression` reads the resolved symbol's docblock through one `resolve(...)?->getDocumentation()` call — no per-kind branch. If the wrapper carries no logic once the branch is gone, delete it and inline the call at every caller. Done: the method either does not exist or is one line with no `match`/`instanceof` on the expression node; `@return list` and `@var` docblock inference works on `FuncCall`, `MethodCall`, `NullsafeMethodCall`, `StaticCall`, `PropertyFetch`, `NullsafePropertyFetch`, `StaticPropertyFetch`, `ClassConstFetch`, and `ConstFetch` the same way it works on `$this->items()`; a test covers each of those node kinds. +- [ ] **step-26** — The three late-binding keywords (`self`, `static`, `parent`) resolve in one place: `Domain\LateBindingKeyword`. Every reader (`ScopeFinder`, `MemberAccessDetector`'s text and AST paths, any other) identifies a keyword through `LateBindingKeyword::tryFrom(strtolower($name))` and resolves it through one function on the enum; the `parent`-of-non-`Class_` guard exists there once. A test under `tests/Architecture/` fails if a string comparison against `'self'`, `'static'`, or `'parent'` appears in `src/` outside `src/Domain/LateBindingKeyword.php` — the tighten that pins the seam. Done: no `src/` file outside the enum compares against the three keyword literals in a class-name-resolution context; a text-path and an AST-path test exercise the same behavior through one code path; the architecture test above is green. +- [ ] **step-27** — `ExpressionResolver::resolveMember` (introduced in step-24) iterates every class it gets from the receiver's `Type::getResolvableClassNames()` instead of indexing `[0]`, the same way `SymbolResolver::getAccessibleMembers` iterates. `MemberAccessDetector`'s three instance-receiver sites route through the same helper (or apply the same iteration). Tighten: `disallowedMethodCalls` restricts `Type::getResolvableClassNames()` to the shared helper and to `SymbolResolver::getAccessibleMembers`, so a future direct caller fails PHPStan. Done: no callsite in `src/Resolution/` indexes `[0]` on `getResolvableClassNames()`; hover, definition, and signature-help on `$x->onlyB()` where `$x: A|B` and only `B` declares `onlyB` answer the same way completion offers it; a parity test asserts the four positional handlers and completion agree on union and intersection receivers; the phpstan baseline for the rule reaches zero. +- [ ] **step-28** — `resolveConstFetch` iterates `NameContext::candidates(short, NameKind::Constant)` the way `resolveFuncCall` iterates `NameKind::Function_`, so PHP name-resolution rules 5-7 (namespaced-first, global fallback) apply to constants as they do to functions. Tighten: `disallowedMethodCalls` restricts `SymbolSource::lookupConstant` to `src/Resolution/ExpressionResolver.php` (mirroring the #478 pattern for `findMethod`/`findProperty`), so a future direct `lookupConstant` outside the candidate loop fails PHPStan. Done: hover and definition on `X` in `namespace App; const X = 1; echo X;` answer; hover and definition on `PHP_INT_MAX` in a namespaced file with no `use const` answer; `resolveConstFetch` has no direct `lookupConstant` call that bypasses the candidate loop; a test covers both the namespaced-constant and global-fallback paths. +- [ ] **step-29** — `SymbolCandidates` reads a symbol's documentation through the `ResolvedSymbol::getDocumentation()` interface method, not by direct `->docblock` field access plus `DocblockParser::extractDescription`. Tighten: `disallowedMethodCalls` restricts `DocblockParser::extractDescription` to `src/Domain/HasSymbolLocation.php`, so a second bypass of the interface fails PHPStan. Done: `SymbolCandidates` does not name `->docblock` or `DocblockParser` directly; a future change to `getDocumentation()` (e.g. tag stripping) reaches completion detail the same way it reaches hover. +- [ ] **step-30** — Signature-plus-documentation assembly lives in one place. A `ResolvedSymbolPresenter` in `src/Resolution/` returns the shape (signature, documentation, deprecation, tags) that `HoverHandler`, `SignatureHelpHandler`, and `CompletionItemFactory` all consume. Each handler and factory maps the presenter output into its LSP shape but does not compose signature-and-documentation itself. Tighten: `disallowedMethodCalls` restricts `ResolvedSymbol::format()` and `ResolvedSymbol::getDocumentation()` to `ResolvedSymbolPresenter`, so a future handler that recomposes signature-plus-doc directly fails PHPStan. Done: no handler or factory calls both `->format()` and `->getDocumentation()` on a resolved symbol; adding a new user-facing field to `ResolvedSymbol` (e.g. `getDeprecation()`) is one edit that all three surfaces read; a parity test asserts hover, signature-help, and completion-detail surface the added attribute the same way; the disallow above is in place. +- [ ] **step-31** — `Variable('this')` types through one code path. `ExpressionResolver::resolve(Variable('this'))` reads the enclosing class the same way `MemberAccessDetector` reads it — a shared helper that consults the text fallback when the parent chain is detached — and the `resolvedType` attribute side-channel `MemberAccessDetector` sets on the `$this` node is deleted. Tighten: `disallowedMethodCalls` restricts `TextFallbackHelper::resolveEnclosingClassName` to the shared helper, so a future ad-hoc text-fallback caller fails PHPStan. Done: hover on `$this` and completion on `$this->` answer for the same set of broken-code inputs; a fixture whose enclosing class-like parent chain is detached drives both features through one path; `MemberAccessDetector` does not write `resolvedType` on any AST node; the disallow above is in place. +- [ ] **step-32** — Retire the rebuild. Delete this manifest, the `do-next` and `review-slice` skills (this row authorises the `.claude/` and policy deletions), and the manifest read from `SymbolCoverageGridTest` so a blocker must name an issue or an RFC section. Restore the hard lock — `bin/check-baseline-shrink` fails if either baseline file exists (this reverses the shrink-only-when-present relaxation carried since PR #479). Add a `tests/Architecture/HandlerDependenciesRule.php` (or an equivalent architecture test) that fails if any handler in `src/Handler/` names `ParserService`, `SymbolIndex`, `MemberResolver`, or `SymbolSource` directly — the "handlers are formatters, not resolvers" invariant becomes code-enforced. Update `CLAUDE.md` Guardrails and Architecture Invariants to match final state: baselines permanently gone, the freeze paragraph deleted, the running list of active `disallowedMethodCalls` tightens (from #478 and steps 27-31) named as the seams they pin. Done: this file and both skills are gone; both baseline files are absent; `bin/check-baseline-shrink` fails with them present; the handler-dependencies rule is in place and green; `CLAUDE.md` matches reality; `composer test` is green; work continues as plain issues. diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon new file mode 100644 index 00000000..3af362d0 --- /dev/null +++ b/phpstan-baseline.neon @@ -0,0 +1,49 @@ +parameters: + ignoreErrors: + - + message: '#^Calling Firehed\\PhpLsp\\Domain\\ResolvedSymbol\:\:getDocumentation\(\) \(as Firehed\\PhpLsp\\Domain\\ResolvedMember\:\:getDocumentation\(\)\) is forbidden, signature\-plus\-documentation assembly goes through ResolvedSymbolPresenter, so hover/signature\-help/completion\-detail surface new attributes uniformly \(build\-manifest step\-30 adds the presenter and lists it here\)\.$#' + identifier: disallowed.method + count: 1 + path: src/Completion/CompletionItemFactory.php + + - + message: '#^Calling Firehed\\PhpLsp\\Domain\\DocblockParser\:\:extractDescription\(\) is forbidden, docblock description extraction goes through HasSymbolLocation\:\:getDocumentation\(\), so a future normalisation \(e\.g\. tag stripping\) reaches every surface \(build\-manifest step\-29\)\.$#' + identifier: disallowed.method + count: 1 + path: src/Completion/SymbolCandidates.php + + - + message: '#^Calling Firehed\\PhpLsp\\Domain\\ResolvedSymbol\:\:getDocumentation\(\) is forbidden, signature\-plus\-documentation assembly goes through ResolvedSymbolPresenter, so hover/signature\-help/completion\-detail surface new attributes uniformly \(build\-manifest step\-30 adds the presenter and lists it here\)\.$#' + identifier: disallowed.method + count: 1 + path: src/Handler/HoverHandler.php + + - + message: '#^Calling Firehed\\PhpLsp\\Domain\\ResolvedSymbol\:\:getDocumentation\(\) \(as Firehed\\PhpLsp\\Domain\\ResolvedCallable\:\:getDocumentation\(\)\) is forbidden, signature\-plus\-documentation assembly goes through ResolvedSymbolPresenter, so hover/signature\-help/completion\-detail surface new attributes uniformly \(build\-manifest step\-30 adds the presenter and lists it here\)\.$#' + identifier: disallowed.method + count: 1 + path: src/Handler/SignatureHelpHandler.php + + - + message: '#^Calling Firehed\\PhpLsp\\Domain\\Type\:\:getResolvableClassNames\(\) is forbidden, union/intersection receivers\: member lookup must iterate every class, not index \[0\]\. Route through the shared member\-on\-any\-type helper \(build\-manifest step\-27\)\.$#' + identifier: disallowed.method + count: 1 + path: src/Resolution/ExpressionResolver.php + + - + message: '#^Calling Firehed\\PhpLsp\\Knowledge\\SymbolSource\:\:lookupConstant\(\) is forbidden, constant lookup iterates NameContext\:\:candidates\(NameKind\:\:Constant\) inside its own helper \(build\-manifest step\-28 adds it and lists it here\); mirrors \#478 for findMethod/findProperty\.$#' + identifier: disallowed.method + count: 1 + path: src/Resolution/ExpressionResolver.php + + - + message: '#^Calling Firehed\\PhpLsp\\Domain\\Type\:\:getResolvableClassNames\(\) is forbidden, union/intersection receivers\: member lookup must iterate every class, not index \[0\]\. Route through the shared member\-on\-any\-type helper \(build\-manifest step\-27\)\.$#' + identifier: disallowed.method + count: 3 + path: src/Resolution/MemberAccessDetector.php + + - + message: '#^Calling Firehed\\PhpLsp\\Resolution\\TextFallbackHelper\:\:resolveEnclosingClassName\(\) is forbidden, enclosing\-class resolution goes through one helper that consults the text fallback when the parent chain is detached \(build\-manifest step\-31 adds the helper and lists it here\)\.$#' + identifier: disallowed.method + count: 1 + path: src/Resolution/MemberAccessDetector.php diff --git a/phpstan.neon b/phpstan.neon index 40c9b528..7773851e 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -1,6 +1,7 @@ includes: - vendor/phpstan/phpstan-strict-rules/rules.neon - vendor/spaze/phpstan-disallowed-calls/extension.neon + - phpstan-baseline.neon parameters: # Default-deny capability confinement: each mechanism below is usable only in @@ -270,6 +271,12 @@ parameters: message: 'lexing outside the parser is the token rim: ContextDetector' allowIn: - tests/* + - + method: 'Firehed\PhpLsp\Domain\DocblockParser::extractDescription()' + message: 'docblock description extraction goes through HasSymbolLocation::getDocumentation(), so a future normalisation (e.g. tag stripping) reaches every surface (build-manifest step-29)' + allowIn: + - src/Domain/HasSymbolLocation.php + - tests/* disallowedClasses: - @@ -336,6 +343,31 @@ parameters: - src/Resolution/ExpressionResolver.php - src/Repository/MemberResolver.php # internal self-calls in trait-alias resolution - tests/* + - + method: 'Firehed\PhpLsp\Domain\Type::getResolvableClassNames()' + message: 'union/intersection receivers: member lookup must iterate every class, not index [0]. Route through the shared member-on-any-type helper (build-manifest step-27)' + allowIn: + - src/Resolution/SymbolResolver.php # getAccessibleMembers: the legitimate every-class iterator + - src/Domain/UnionType.php # recursive composition + - src/Domain/IntersectionType.php # recursive composition + - tests/* + - + method: 'Firehed\PhpLsp\Knowledge\SymbolSource::lookupConstant()' + message: 'constant lookup iterates NameContext::candidates(NameKind::Constant) inside its own helper (build-manifest step-28 adds it and lists it here); mirrors #478 for findMethod/findProperty' + allowIn: + - src/Knowledge/CompositeSymbolSource.php # composite delegates to backends + - tests/* + - + method: 'Firehed\PhpLsp\Domain\ResolvedSymbol::getDocumentation()' + message: 'signature-plus-documentation assembly goes through ResolvedSymbolPresenter, so hover/signature-help/completion-detail surface new attributes uniformly (build-manifest step-30 adds the presenter and lists it here)' + allowIn: + - tests/* + - + method: 'Firehed\PhpLsp\Resolution\TextFallbackHelper::resolveEnclosingClassName()' + message: 'enclosing-class resolution goes through one helper that consults the text fallback when the parent chain is detached (build-manifest step-31 adds the helper and lists it here)' + allowIn: + - src/Resolution/TextFallbackHelper.php # self-call inside the fallback + - tests/* level: max paths: