From 6c3b2fb5219d3a7f348ec0c53726752ea66aff0c Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 31 Aug 2026 20:23:29 -0700 Subject: [PATCH 1/3] =?UTF-8?q?Retarget=20step-24=20to=20the=20structural?= =?UTF-8?q?=20M=C3=97N=20only?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/architecture/build-manifest.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/build-manifest.md b/docs/architecture/build-manifest.md index 9925c1e5..b76e8f5f 100644 --- a/docs/architecture/build-manifest.md +++ b/docs/architecture/build-manifest.md @@ -41,7 +41,7 @@ One signal: both baseline files are deleted, and the issues named below are clos - [x] **step-21** — Route the remaining `new ClassName`, `new PrimitiveType`, and `new UnionType` sites through `TypeFactory`. Delete `phpstan-baseline.neon` and `deptrac.baseline.yaml`; `bin/check-baseline-shrink` fails if either file exists (this edit to `bin/` is authorised here). Done: no baseline file exists; CI is green; #181 closed. - [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. -- [ ] **step-24** — Member lookup in `ExpressionResolver` is one function taking the receiver expression, the member name, and the member kind; `resolveMethodCall`, `resolveStaticCall`, `resolvePropertyFetch`, `resolveStaticPropertyFetch`, and `resolveClassConstFetch` call it. Late-binding type resolution runs for properties and class constants as it does for methods. Done: the five methods share one member-lookup helper; a `PropertyFetch` and a `ClassConstFetch` whose declared type is `static` in the parent class each resolve to the child class on the child receiver; a parity test asserts method-return, property-type, and class-constant-type agreement on late-binding across the five member-access node kinds. +- [ ] **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. From 8ffd800eb302001f94f463cb2d874becd34db584 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 31 Aug 2026 20:28:52 -0700 Subject: [PATCH 2/3] Collapse ExpressionResolver member lookup to one helper Co-Authored-By: Claude Opus 4.7 --- docs/architecture/build-manifest.md | 2 +- src/Resolution/ExpressionResolver.php | 115 ++++++++++++++------------ 2 files changed, 64 insertions(+), 53 deletions(-) diff --git a/docs/architecture/build-manifest.md b/docs/architecture/build-manifest.md index b76e8f5f..e6ca7ab0 100644 --- a/docs/architecture/build-manifest.md +++ b/docs/architecture/build-manifest.md @@ -41,7 +41,7 @@ One signal: both baseline files are deleted, and the issues named below are clos - [x] **step-21** — Route the remaining `new ClassName`, `new PrimitiveType`, and `new UnionType` sites through `TypeFactory`. Delete `phpstan-baseline.neon` and `deptrac.baseline.yaml`; `bin/check-baseline-shrink` fails if either file exists (this edit to `bin/` is authorised here). Done: no baseline file exists; CI is green; #181 closed. - [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. -- [ ] **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. +- [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. diff --git a/src/Resolution/ExpressionResolver.php b/src/Resolution/ExpressionResolver.php index 4aa14b46..73d2d54c 100644 --- a/src/Resolution/ExpressionResolver.php +++ b/src/Resolution/ExpressionResolver.php @@ -14,6 +14,7 @@ use Firehed\PhpLsp\Domain\FunctionName; use Firehed\PhpLsp\Domain\GlobalConstantName; use Firehed\PhpLsp\Domain\Location; +use Firehed\PhpLsp\Domain\MemberInfo; use Firehed\PhpLsp\Domain\MethodInfo; use Firehed\PhpLsp\Domain\MethodName; use Firehed\PhpLsp\Domain\NameKind; @@ -102,7 +103,7 @@ public function resolve(Expr $expr, array $ast): ?ResolvedSymbol } if ($expr instanceof StaticCall) { - return $this->resolveStaticCall($expr); + return $this->resolveStaticCall($expr, $ast); } if ($expr instanceof FuncCall) { @@ -114,7 +115,7 @@ public function resolve(Expr $expr, array $ast): ?ResolvedSymbol } if ($expr instanceof StaticPropertyFetch) { - return $this->resolveStaticPropertyFetch($expr); + return $this->resolveStaticPropertyFetch($expr, $ast); } if ($expr instanceof ClassConstFetch) { @@ -383,42 +384,18 @@ private function resolveMethodCall(MethodCall|NullsafeMethodCall $expr, array $a if (!$expr->name instanceof Identifier) { return null; } - $receiverType = $this->resolve($expr->var, $ast)?->getType(); - $classNames = $receiverType?->getResolvableClassNames() ?? []; - if ($classNames === []) { - return null; - } - $className = $classNames[0]; - $methodInfo = $this->memberResolver->findMethod( - $className, - new MethodName($expr->name->toString()), - Visibility::Private, - ); - if ($methodInfo === null) { - return null; - } - return $this->resolveLateBoundReturn($methodInfo, $className); + return $this->resolveMember($expr->var, $expr, $expr->name->toString(), $this->findMethod(...), $ast); } - private function resolveStaticCall(StaticCall $expr): ?MethodInfo + /** + * @param array $ast + */ + private function resolveStaticCall(StaticCall $expr, array $ast): ?MethodInfo { if (!$expr->name instanceof Identifier || !$expr->class instanceof Name) { return null; } - $classNameStr = ScopeFinder::resolveClassNameInContext($expr->class, $expr); - if ($classNameStr === null) { - return null; - } - $className = TypeFactory::className($classNameStr); - $methodInfo = $this->memberResolver->findMethod( - $className, - new MethodName($expr->name->toString()), - Visibility::Private, - ); - if ($methodInfo === null) { - return null; - } - return $this->resolveLateBoundReturn($methodInfo, $className); + return $this->resolveMember($expr->class, $expr, $expr->name->toString(), $this->findMethod(...), $ast); } /** @@ -450,34 +427,68 @@ private function resolvePropertyFetch(PropertyFetch|NullsafePropertyFetch $expr, if (!$expr->name instanceof Identifier) { return null; } - $receiverType = $this->resolve($expr->var, $ast)?->getType(); - $classNames = $receiverType?->getResolvableClassNames() ?? []; - if ($classNames === []) { - return null; - } - $info = $this->memberResolver->findProperty( - $classNames[0], - new PropertyName($expr->name->toString()), - Visibility::Private, - ); - return $info; + return $this->resolveMember($expr->var, $expr, $expr->name->toString(), $this->findProperty(...), $ast); } - private function resolveStaticPropertyFetch(StaticPropertyFetch $expr): ?PropertyInfo + /** + * @param array $ast + */ + private function resolveStaticPropertyFetch(StaticPropertyFetch $expr, array $ast): ?PropertyInfo { if (!$expr->name instanceof VarLikeIdentifier || !$expr->class instanceof Name) { return null; } - $classNameStr = ScopeFinder::resolveClassNameInContext($expr->class, $expr); - if ($classNameStr === null) { + return $this->resolveMember($expr->class, $expr, $expr->name->toString(), $this->findProperty(...), $ast); + } + + /** + * Access a member on the receiver, delegating the kind-specific lookup to + * `$find`. The receiver-to-ClassName dance — an instance expression that + * resolves through `resolve()`, versus a class-name `Name` that resolves + * through `ScopeFinder` — is here, so a new member-access node kind adds + * one call site rather than another copy of this dance. + * + * @template T of MemberInfo + * @param callable(ClassName, string): ?T $find + * @param array $ast + * @return ?T + */ + private function resolveMember( + Expr|Name $receiver, + Node $context, + string $memberName, + callable $find, + array $ast, + ): ?MemberInfo { + if ($receiver instanceof Name) { + $classNameStr = ScopeFinder::resolveClassNameInContext($receiver, $context); + if ($classNameStr === null) { + return null; + } + $className = TypeFactory::className($classNameStr); + } else { + $receiverType = $this->resolve($receiver, $ast)?->getType(); + $classNames = $receiverType?->getResolvableClassNames() ?? []; + if ($classNames === []) { + return null; + } + $className = $classNames[0]; + } + return $find($className, $memberName); + } + + private function findMethod(ClassName $className, string $name): ?MethodInfo + { + $info = $this->memberResolver->findMethod($className, new MethodName($name), Visibility::Private); + if ($info === null) { return null; } - $info = $this->memberResolver->findProperty( - TypeFactory::className($classNameStr), - new PropertyName($expr->name->toString()), - Visibility::Private, - ); - return $info; + return $this->resolveLateBoundReturn($info, $className); + } + + private function findProperty(ClassName $className, string $name): ?PropertyInfo + { + return $this->memberResolver->findProperty($className, new PropertyName($name), Visibility::Private); } private function resolveClassConstFetch(ClassConstFetch $expr): ?ResolvedSymbol From 179800f6b7365a86ef464028d531a3ccd7947d8f Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 31 Aug 2026 20:37:58 -0700 Subject: [PATCH 3/3] Confine MemberResolver::find* to ExpressionResolver's finders Route the docblock and constructor lookups through resolveMember so the two private find* finders in ExpressionResolver are the sole direct callers of MemberResolver::findMethod/findProperty, then add a disallowedMethodCalls entry that locks that boundary in. Co-Authored-By: Claude Opus 4.7 --- phpstan.neon | 9 ++++++++ src/Resolution/ExpressionResolver.php | 30 +++++---------------------- 2 files changed, 14 insertions(+), 25 deletions(-) diff --git a/phpstan.neon b/phpstan.neon index 12064035..40c9b528 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -327,6 +327,15 @@ parameters: allowIn: - src/Index/DocumentIndexer.php - tests/* + - + method: + - 'Firehed\PhpLsp\Repository\MemberResolver::findMethod()' + - 'Firehed\PhpLsp\Repository\MemberResolver::findProperty()' + message: 'member lookup on an expression goes through ExpressionResolver::resolveMember; the file''s two private find* finders are the only direct callers' + allowIn: + - src/Resolution/ExpressionResolver.php + - src/Repository/MemberResolver.php # internal self-calls in trait-alias resolution + - tests/* level: max paths: diff --git a/src/Resolution/ExpressionResolver.php b/src/Resolution/ExpressionResolver.php index 73d2d54c..8d240664 100644 --- a/src/Resolution/ExpressionResolver.php +++ b/src/Resolution/ExpressionResolver.php @@ -256,29 +256,17 @@ private function foreachElementType(Stmt\Foreach_ $foreach, Variable $bindingVar private function docblockForExpression(Expr $expr, array $ast): ?string { if ($expr instanceof MethodCall || $expr instanceof NullsafeMethodCall) { - $receiverType = $this->resolve($expr->var, $ast)?->getType(); - $classNames = $receiverType?->getResolvableClassNames() ?? []; - if ($classNames === [] || !$expr->name instanceof Identifier) { + if (!$expr->name instanceof Identifier) { return null; } - $info = $this->memberResolver->findMethod( - $classNames[0], - new MethodName($expr->name->toString()), - Visibility::Private, - ); + $info = $this->resolveMember($expr->var, $expr, $expr->name->toString(), $this->findMethod(...), $ast); return $info?->docblock; } if ($expr instanceof PropertyFetch || $expr instanceof NullsafePropertyFetch) { - $receiverType = $this->resolve($expr->var, $ast)?->getType(); - $classNames = $receiverType?->getResolvableClassNames() ?? []; - if ($classNames === [] || !$expr->name instanceof Identifier) { + if (!$expr->name instanceof Identifier) { return null; } - $info = $this->memberResolver->findProperty( - $classNames[0], - new PropertyName($expr->name->toString()), - Visibility::Private, - ); + $info = $this->resolveMember($expr->var, $expr, $expr->name->toString(), $this->findProperty(...), $ast); return $info?->docblock; } if ($expr instanceof FuncCall && $expr->name instanceof Name) { @@ -365,15 +353,7 @@ public function resolveConstructor(New_|Attribute $call): ?MethodInfo if (!$classNameNode instanceof Name) { return null; } - $classNameStr = ScopeFinder::resolveClassNameInContext($classNameNode, $call); - if ($classNameStr === null) { - return null; - } - return $this->memberResolver->findMethod( - TypeFactory::className($classNameStr), - new MethodName('__construct'), - Visibility::Private, - ); + return $this->resolveMember($classNameNode, $call, '__construct', $this->findMethod(...), []); } /**