From c7f8939a6620f0922671754c6cb2054ad65a3fef Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Fri, 14 Aug 2026 16:30:41 -0700 Subject: [PATCH 1/2] Take SymbolExtractor's declarations from the scanner --- phpstan-baseline.neon | 18 ---- src/Index/SymbolExtractor.php | 163 +++++++++++----------------------- 2 files changed, 52 insertions(+), 129 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 0a83afec..e494a7d2 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -36,24 +36,6 @@ parameters: count: 2 path: src/Handler/CompletionHandler.php - - - message: '#^Class PhpParser\\NodeTraverser is forbidden, AST traversal is confined\: parser pipeline \(ParserService\), positional finders \(NodeAtPosition, Scope, ScopeFinder\), and DeclarationScanner\.$#' - identifier: disallowed.class - count: 1 - path: src/Index/SymbolExtractor.php - - - - message: '#^Namespace PhpParser\\NodeTraverser is forbidden, AST traversal is confined\: parser pipeline \(ParserService\), positional finders \(NodeAtPosition, Scope, ScopeFinder\), and DeclarationScanner\.$#' - identifier: disallowed.namespace - count: 1 - path: src/Index/SymbolExtractor.php - - - - message: '#^Namespace PhpParser\\NodeVisitorAbstract is forbidden, AST traversal is confined\: parser pipeline \(ParserService\), positional finders \(NodeAtPosition, Scope, ScopeFinder\), and DeclarationScanner\. \[PhpParser\\NodeVisitorAbstract matches PhpParser\\NodeVisitor\*\]$#' - identifier: disallowed.namespace - count: 2 - path: src/Index/SymbolExtractor.php - - message: '#^Calling strtolower\(\) is forbidden, case rules for symbol names live in NameKind; non\-symbol uses may be allowlisted here consciously\.$#' identifier: disallowed.function diff --git a/src/Index/SymbolExtractor.php b/src/Index/SymbolExtractor.php index 5885f018..1d6b05d3 100644 --- a/src/Index/SymbolExtractor.php +++ b/src/Index/SymbolExtractor.php @@ -7,17 +7,13 @@ use Firehed\PhpLsp\Document\TextDocument; use PhpParser\Node; use PhpParser\Node\Stmt; -use PhpParser\NodeTraverser; -use PhpParser\NodeVisitorAbstract; -final class SymbolExtractor extends NodeVisitorAbstract +final class SymbolExtractor { - /** @var list */ - private array $symbols = []; - private string $uri = ''; - private string $namespace = ''; - private ?string $currentClass = null; - private TextDocument $document; + public function __construct( + private readonly DeclarationScanner $declarations = new DeclarationScanner(), + ) { + } /** * @param array $ast @@ -25,126 +21,71 @@ final class SymbolExtractor extends NodeVisitorAbstract */ public function extract(TextDocument $document, array $ast): array { - $this->symbols = []; - $this->uri = $document->uri; - $this->namespace = ''; - $this->currentClass = null; - $this->document = $document; - - $traverser = new NodeTraverser(); - $traverser->addVisitor($this); - $traverser->traverse($ast); - - return $this->symbols; - } - - public function enterNode(Node $node): null - { - if ($node instanceof Stmt\Namespace_) { - $this->namespace = $node->name?->toString() ?? ''; - return null; - } + $declarations = $this->declarations->scan($ast); + $symbols = []; - if ($node instanceof Stmt\Class_) { - $this->addClassLikeSymbol($node, SymbolKind::Class_); - $this->currentClass = $node->name?->toString(); - return null; - } - - if ($node instanceof Stmt\Interface_) { - $this->addClassLikeSymbol($node, SymbolKind::Interface_); - $this->currentClass = $node->name?->toString(); - return null; - } - - if ($node instanceof Stmt\Trait_) { - $this->addClassLikeSymbol($node, SymbolKind::Trait_); - $this->currentClass = $node->name?->toString(); - return null; - } + foreach ($declarations->classLikes as $declaration) { + $fqn = $declaration->name->fullyQualifiedName(); + $symbols[] = new Symbol( + name: $declaration->name->shortName, + fullyQualifiedName: $fqn, + kind: self::kindOf($declaration->node), + location: self::locate($document, $declaration->node), + ); - if ($node instanceof Stmt\Enum_) { - $this->addClassLikeSymbol($node, SymbolKind::Enum_); - $this->currentClass = $node->name?->toString(); - return null; + foreach ($declaration->node->getMethods() as $method) { + $name = $method->name->toString(); + $symbols[] = new Symbol( + name: $name, + fullyQualifiedName: $fqn . '::' . $name, + kind: SymbolKind::Method, + location: self::locate($document, $method), + containerName: $declaration->name->shortName, + ); + } } - if ($node instanceof Stmt\Function_) { - $name = $node->name->toString(); - $fqn = $this->namespace !== '' ? $this->namespace . '\\' . $name : $name; - $this->symbols[] = new Symbol( - name: $name, - fullyQualifiedName: $fqn, + foreach ($declarations->functions as $declaration) { + $symbols[] = new Symbol( + name: $declaration->name->shortName, + fullyQualifiedName: $declaration->name->fullyQualifiedName(), kind: SymbolKind::Function_, - location: $this->createLocation($node), + location: self::locate($document, $declaration->node), ); - return null; } - if ($node instanceof Stmt\ClassMethod && $this->currentClass !== null) { - $name = $node->name->toString(); - $fqn = ($this->namespace !== '' ? $this->namespace . '\\' : '') - . $this->currentClass . '::' . $name; - $this->symbols[] = new Symbol( - name: $name, - fullyQualifiedName: $fqn, - kind: SymbolKind::Method, - location: $this->createLocation($node), - containerName: $this->currentClass, - ); - return null; - } + // Callers index `$symbols[0]` as the file's first declaration, which the two + // separate scanner lists would otherwise interleave by kind rather than by + // where each is written. + usort($symbols, self::byPosition(...)); - return null; + return $symbols; } - public function leaveNode(Node $node): null + private static function byPosition(Symbol $a, Symbol $b): int { - if ( - $node instanceof Stmt\Class_ - || $node instanceof Stmt\Interface_ - || $node instanceof Stmt\Trait_ - || $node instanceof Stmt\Enum_ - ) { - $this->currentClass = null; - } - - return null; + return [$a->location->startLine, $a->location->startCharacter] + <=> [$b->location->startLine, $b->location->startCharacter]; } - private function addClassLikeSymbol( - Stmt\Class_|Stmt\Interface_|Stmt\Trait_|Stmt\Enum_ $node, - SymbolKind $kind, - ): void { - $name = $node->name?->toString(); - if ($name === null) { - return; // Anonymous class - } - - $fqn = $this->namespace !== '' ? $this->namespace . '\\' . $name : $name; - $this->symbols[] = new Symbol( - name: $name, - fullyQualifiedName: $fqn, - kind: $kind, - location: $this->createLocation($node), - ); + private static function kindOf(Stmt\ClassLike $node): SymbolKind + { + return match (true) { + $node instanceof Stmt\Enum_ => SymbolKind::Enum_, + $node instanceof Stmt\Interface_ => SymbolKind::Interface_, + $node instanceof Stmt\Trait_ => SymbolKind::Trait_, + default => SymbolKind::Class_, + }; } - private function createLocation(Node $node): Location + private static function locate(TextDocument $document, Node $node): Location { - $startLine = $node->getStartLine() - 1; // LSP is 0-indexed - $endLine = $node->getEndLine() - 1; - - // Get character positions from the document - $startPos = $this->document->positionAt($node->getStartFilePos()); - $endPos = $this->document->positionAt($node->getEndFilePos() + 1); - return new Location( - uri: $this->uri, - startLine: $startLine, - startCharacter: $startPos['character'], - endLine: $endLine, - endCharacter: $endPos['character'], + uri: $document->uri, + startLine: $node->getStartLine() - 1, // LSP is 0-indexed + startCharacter: $document->positionAt($node->getStartFilePos())['character'], + endLine: $node->getEndLine() - 1, + endCharacter: $document->positionAt($node->getEndFilePos() + 1)['character'], ); } } From 2aea286cea7c2c668f05b94b1569cbc16c0785b4 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Fri, 14 Aug 2026 16:46:26 -0700 Subject: [PATCH 2/2] Correct the SymbolExtractor and traversal-rule notes --- docs/architecture/build-manifest.md | 40 +++++++++++++++++------------ 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/docs/architecture/build-manifest.md b/docs/architecture/build-manifest.md index c8b3f8ec..a1acd1eb 100644 --- a/docs/architecture/build-manifest.md +++ b/docs/architecture/build-manifest.md @@ -98,11 +98,10 @@ re-runs repo-wide as its completion gate. S4.6 4 SymbolResolver -> glue; CodeResolver positional S4.2,S4.3,S4.4,S4.5,S4.8 — S4.7 4 Step 4 duplication audit all Step 4 — SC.1 — Delete the dead WorkspaceIndexer — — - SC.3 — Namespace tracking -> the parser's namespacedName — — + SC.3 — SymbolExtractor reads DeclarationScanner — — SC.4 — Dedupe the hand-rolled file:// conversion — — SC.7 — Six member-hierarchy walks -> one — — SC.8 — Prefix matching: SymbolIndex -> PrefixMatcher — — - SC.10 — Enforce the one-declaration-scanner rule SC.3,SC.9 — SC.12 — Move MemberFilter out of Resolution — — SC.13 — Settle Domain->Utility type placement — — SC.14 — Filter BuiltinBackend class-like lookup to internal — — @@ -110,6 +109,7 @@ re-runs repo-wide as its completion gate. SC.16 — Index an open document's global constants — — SC.17 — Collapse the hand-routed invalidation fan-out — — SC.18 — One home for the kind-qualified symbol key SC.13 — + SC.19 — Own the four unowned baseline entries — — SZ.1 Z Definition of Done gate + repo-wide dup audit all prior — Notes: @@ -225,12 +225,21 @@ Notes: map, so a class and a function sharing a short name collide — and the constant table it also folds in is exactly what constant resolution will read. `NameContext::importsFor()` already keeps the three tables apart, so no caller needs Step 4 to move. - - **SC.3** — `SymbolExtractor` hand-tracks - `Stmt\Namespace_` to build FQNs that `NameResolver` already computed into - `namespacedName` (which `DefaultClassInfoFactory`, `DefaultFunctionRepository`, - `ScopeFinder`, and `DeclarationScanner` all read). Behavior-preserving, so the Step P - write-path and class-like-lookup goldens prove it. `SymbolExtractor`'s `Class::method` - FQNs are its own and stay. + - **SC.3** — `SymbolExtractor` is the last hand-written declaration traversal in `src/`: + it walks the AST itself and hand-tracks `Stmt\Namespace_` to rebuild FQNs that + `NameResolver` already computed into `namespacedName`. It reads `DeclarationScanner` + instead, so no consumer can disagree about what a file declares. Its `Class::method` + symbols are **not** an obstacle to that, as this row long claimed: methods come off + each class-like's own node via `ClassLike::getMethods()`, which the scanner already + hands back, so no visitor is needed for them either. Behavior-preserving, so the Step P + write-path and prefix-search goldens prove it. + + The confinement this satisfies is **already enforced** — `phpstan.neon` restricts + `NodeTraverser` / `NodeFinder` / `NodeVisitor*` to `ParserService`, the positional + finders, and `DeclarationScanner`. `SymbolExtractor`'s violations were merely frozen in + `phpstan-baseline.neon`, which is why nothing failed while they stood. Do not file a + slice to build that rule; the remaining frozen violations belong to S4.2 + (`SymbolResolver`) and S4.8 (`BasicTypeResolver`). - **SC.4** — `file://` URI and path conversion is hand-rolled in four live places (`DefaultClassInfoFactory`, `FilesystemBackend` ×2, `Location`, and the dead `WorkspaceIndexer`), each differing in how it handles the scheme and percent- @@ -324,15 +333,12 @@ Notes: more copies. Outside Step 4's scope — that step decomposes `src/Resolution/`, this is `src/Repository/`. The member-name case rule rides along: the collect keys (`strtolower`), `MethodName::equals` (`strcasecmp`), and the fallback's raw merge key disagree today, and the walks' seen-sets key raw FQNs where `ClassName::equals` is case-insensitive. - - **SC.10** — SC.5 states a hard invariant ("Do NOT write a new one; a rule about what - counts as a declaration is a change to the scanner") with no mechanism, which §8.1 - forbids where a static rule or test is feasible. One is: a PHPStan rule confining - `NodeVisitorAbstract` / `NodeFinder` / `NodeTraverser` to the parser, the positional - finders, and `DeclarationScanner`, in the shape of the existing - `SymbolDiscoveryAuthorityExtension`. Gated on its two known violators (SC.3's - `SymbolExtractor`, SC.9's `classesIn`) because a rule that must ship with two - exemptions enforces nothing. The analogue to follow is `TypeGraphParityTest`, which is - how the sibling single-traversal invariant on `supertypes()` is held. + - **SC.19** — four `phpstan-baseline.neon` entries that no row above drains: + `ReferenceResolver`'s `strcasecmp`, and the `preg_match` calls in `CompletionHandler` + and `NamedArgumentCandidates`. The case fold belongs to `NameKind`; the two regexes + belong in `CompletionClassifier`, which is where the allowlist already puts + text-pattern analysis. Filed because the baseline must reach zero and an entry with no + owning slice is how it stalls — found by auditing the baseline against this table. - **SC.8** — `Completion\PrefixMatcher::matches` and `SymbolIndex::findByPrefix` both hand-roll `str_starts_with(strtolower(...))`. SC.6 owns the `strtolower` half (it is the same per-kind case rule); what is left here is the duplicated *matching* helper, so