diff --git a/docs/architecture/build-manifest.md b/docs/architecture/build-manifest.md index 31d89732..0144296e 100644 --- a/docs/architecture/build-manifest.md +++ b/docs/architecture/build-manifest.md @@ -44,7 +44,7 @@ One signal: both baseline files are deleted, and the issues named below are clos - [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. - [x] **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. - [x] **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. +- [x] **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. diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 3af362d0..2b80d8c9 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -24,24 +24,12 @@ parameters: 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 diff --git a/phpstan.neon b/phpstan.neon index 336bad1e..b9d53ebe 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -345,9 +345,10 @@ parameters: - 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)' + message: 'union/intersection receivers: member lookup must iterate every class, not index [0]. Route through ExpressionResolver::receiverClassNames or SymbolResolver::getAccessibleMembers (build-manifest step-27)' allowIn: - src/Resolution/SymbolResolver.php # getAccessibleMembers: the legitimate every-class iterator + - src/Resolution/ExpressionResolver.php # receiverClassNames: the shared helper for member-access receivers - src/Domain/UnionType.php # recursive composition - src/Domain/IntersectionType.php # recursive composition - tests/* diff --git a/src/Resolution/ExpressionResolver.php b/src/Resolution/ExpressionResolver.php index 4fe4c7d7..8c07124e 100644 --- a/src/Resolution/ExpressionResolver.php +++ b/src/Resolution/ExpressionResolver.php @@ -410,16 +410,24 @@ private function resolveMember( if ($classNameStr === null) { return null; } - $className = TypeFactory::className($classNameStr); - } else { - $receiverType = $this->resolve($receiver, $ast)?->getType(); - $classNames = $receiverType?->getResolvableClassNames() ?? []; - if ($classNames === []) { - return null; + return $find(TypeFactory::className($classNameStr), $memberName); + } + $receiverType = $this->resolve($receiver, $ast)?->getType(); + foreach (self::receiverClassNames($receiverType) as $className) { + $member = $find($className, $memberName); + if ($member !== null) { + return $member; } - $className = $classNames[0]; } - return $find($className, $memberName); + return null; + } + + /** + * @return list + */ + public static function receiverClassNames(?Type $type): array + { + return $type?->getResolvableClassNames() ?? []; } private function findMethod(ClassName $className, string $name): ?MethodInfo diff --git a/src/Resolution/MemberAccessDetector.php b/src/Resolution/MemberAccessDetector.php index f1d2ced0..3988bfe3 100644 --- a/src/Resolution/MemberAccessDetector.php +++ b/src/Resolution/MemberAccessDetector.php @@ -105,15 +105,11 @@ public function detect( $prefix = $node->name instanceof Identifier ? $node->name->toString() : ''; $type = $this->resolveInstanceAccessType($node, $ast, $document, $line); - $target = $type?->getResolvableClassNames()[0] ?? null; - if ($type !== null && $target !== null) { - $enclosingName = ScopeFinder::findEnclosingClassName($node); - $vantage = $enclosingName !== null ? TypeFactory::className($enclosingName) : null; - return MemberAccessContext::forInstance( - $type, - $this->visibilityBetween($vantage, $target), - $prefix, - ); + $enclosingName = ScopeFinder::findEnclosingClassName($node); + $vantage = $enclosingName !== null ? TypeFactory::className($enclosingName) : null; + $visibility = $this->visibilityForReceiver($vantage, $type); + if ($type !== null && $visibility !== null) { + return MemberAccessContext::forInstance($type, $visibility, $prefix); } return $this->fromText($document, $ast, $line, $character); } @@ -182,6 +178,30 @@ public function resolveInstanceAccessType( return $exprResolver->resolve($node->var, $ast)?->getType(); } + /** + * Null when the receiver resolves to no classes; otherwise the most + * restrictive visibility across the constituents — a member must be + * visible on every possible runtime class to be safe to offer. + */ + private function visibilityForReceiver(?ClassName $vantage, ?Type $type): ?Visibility + { + $classes = ExpressionResolver::receiverClassNames($type); + if ($classes === []) { + return null; + } + $visibility = Visibility::Private; + foreach ($classes as $target) { + $per = $this->visibilityBetween($vantage, $target); + if ($per->value > $visibility->value) { + $visibility = $per; + } + if ($visibility === Visibility::Public) { + break; + } + } + return $visibility; + } + /** * The one function that decides how visible a target class is to a vantage * class. Same class: private. Subclass (any depth): protected. Otherwise @@ -249,15 +269,11 @@ private function resolveTextMatch( return null; } $type = $this->resolveChainReceiverType($match['chain'], $enclosingClass, $document, $ast); - $target = $type?->getResolvableClassNames()[0] ?? null; - if ($type === null || $target === null) { + $visibility = $this->visibilityForReceiver(TypeFactory::className($enclosingClass), $type); + if ($type === null || $visibility === null) { return null; } - return MemberAccessContext::forInstance( - $type, - $this->visibilityBetween(TypeFactory::className($enclosingClass), $target), - $match['prefix'], - ); + return MemberAccessContext::forInstance($type, $visibility, $match['prefix']); } if ($match['kind'] === 'instance') { @@ -360,18 +376,13 @@ private function resolveVariableAccessWithAst( return null; } - $target = $type->getResolvableClassNames()[0] ?? null; - if ($target === null) { - return null; - } $enclosingClassName = $scope->getSelfContext(); $vantage = $enclosingClassName !== null ? TypeFactory::className($enclosingClassName) : null; - - return MemberAccessContext::forInstance( - $type, - $this->visibilityBetween($vantage, $target), - $match['prefix'], - ); + $visibility = $this->visibilityForReceiver($vantage, $type); + if ($visibility === null) { + return null; + } + return MemberAccessContext::forInstance($type, $visibility, $match['prefix']); } /** diff --git a/tests/Fixtures/src/Intersection/IntersectionReceiver.php b/tests/Fixtures/src/Intersection/IntersectionReceiver.php new file mode 100644 index 00000000..ed8d7038 --- /dev/null +++ b/tests/Fixtures/src/Intersection/IntersectionReceiver.php @@ -0,0 +1,36 @@ +getName(); //hover:intersection_person_member + } + + public function triggerEntityMember(Entity&Person $value): void + { + $value->getId(); //hover:intersection_entity_member + } + + public function completePersonMember(Entity&Person $value): void + { + $value->/*|intersection_completion*/ + } + + public function signaturePersonMember(Entity&Person $value): void + { + $value->getName(/*|intersection_signature*/); + } +} diff --git a/tests/Fixtures/src/Union/UnionReceiver.php b/tests/Fixtures/src/Union/UnionReceiver.php new file mode 100644 index 00000000..e2f267ce --- /dev/null +++ b/tests/Fixtures/src/Union/UnionReceiver.php @@ -0,0 +1,36 @@ +getName(); //hover:union_person_member + } + + public function triggerEntityMember(Entity|Person $value): void + { + $value->getId(); //hover:union_entity_member + } + + public function completePersonMember(Entity|Person $value): void + { + $value->/*|union_completion*/ + } + + public function signaturePersonMember(Entity|Person $value): void + { + $value->getName(/*|union_signature*/); + } +} diff --git a/tests/Handler/CompositeReceiverParityTest.php b/tests/Handler/CompositeReceiverParityTest.php new file mode 100644 index 00000000..83d31c91 --- /dev/null +++ b/tests/Handler/CompositeReceiverParityTest.php @@ -0,0 +1,208 @@ +documents = new DocumentManager(); + $parser = new ParserService(); + + $fixturesRoot = __DIR__ . '/../Fixtures'; + $knowledge = KnowledgeStack::forProject( + ComposerAutoloadMap::fromProjectRoot($fixturesRoot), + $fixturesRoot . '/vendor', + $parser, + ); + $memberResolver = new MemberResolver($knowledge->source); + $symbolResolver = new SymbolResolver( + $parser, + $knowledge->source, + $memberResolver, + ); + + $capabilities = self::createStub(SessionCapabilitiesProvider::class); + $capabilities->method('getSessionCapabilities') + ->willReturn(new SessionCapabilities(hoverMarkupKind: MarkupKind::PlainText)); + + $this->hover = new HoverHandler($this->documents, $symbolResolver, $capabilities); + $this->definition = new DefinitionHandler($this->documents, $symbolResolver); + $this->signatureHelp = new SignatureHelpHandler($this->documents, $symbolResolver); + $this->completion = new CompletionHandler( + $this->documents, + $symbolResolver, + new SymbolCandidates($knowledge->source, $symbolResolver, $capabilities), + new KeywordCandidates(), + new VariableCandidates($symbolResolver), + new MemberCandidates($symbolResolver, $capabilities), + new NamedArgumentCandidates(), + new BuiltinTypeCandidates(), + ); + $this->syncHandler = new TextDocumentSyncHandler($this->documents, $knowledge->sink); + + $this->entityUri = $this->openFixture('src/Domain/Entity.php'); + $this->personUri = $this->openFixture('src/Domain/Person.php'); + } + + /** + * @return iterable + */ + public static function compositeReceiverCases(): iterable + { + yield 'union: member on second constituent' => [ + 'src/Union/UnionReceiver.php', + 'union_person_member', + 'getName', + 'Fixtures\\Domain\\Person', + ]; + yield 'union: member on first constituent' => [ + 'src/Union/UnionReceiver.php', + 'union_entity_member', + 'getId', + 'Fixtures\\Domain\\Entity', + ]; + yield 'intersection: member on second constituent' => [ + 'src/Intersection/IntersectionReceiver.php', + 'intersection_person_member', + 'getName', + 'Fixtures\\Domain\\Person', + ]; + yield 'intersection: member on first constituent' => [ + 'src/Intersection/IntersectionReceiver.php', + 'intersection_entity_member', + 'getId', + 'Fixtures\\Domain\\Entity', + ]; + } + + #[DataProvider('compositeReceiverCases')] + public function testHoverResolvesMemberOnEitherConstituent( + string $fixture, + string $marker, + string $memberName, + string $declaringClass, + ): void { + $cursor = $this->openFixtureAtHoverMarker($fixture, $marker); + + $result = $this->hover->handle($this->hoverRequestAt($cursor)); + + self::assertIsArray($result, "hover must answer for {$memberName}() on the composite receiver"); + self::assertStringContainsString( + $memberName, + $result['contents']['value'], + "hover signature must name {$memberName}", + ); + } + + #[DataProvider('compositeReceiverCases')] + public function testDefinitionResolvesMemberOnEitherConstituent( + string $fixture, + string $marker, + string $memberName, + string $declaringClass, + ): void { + $cursor = $this->openFixtureAtHoverMarker($fixture, $marker); + + $result = $this->definition->handle($this->definitionRequestAt($cursor)); + + self::assertIsArray($result, "definition must answer for {$memberName}() on the composite receiver"); + $expected = $declaringClass === 'Fixtures\\Domain\\Person' ? $this->personUri : $this->entityUri; + self::assertSame( + $expected, + $result['uri'], + "definition must land in the class that declares {$memberName}", + ); + } + + /** + * @return iterable + */ + public static function signatureHelpCases(): iterable + { + yield 'union receiver' => ['src/Union/UnionReceiver.php', 'union_signature']; + yield 'intersection receiver' => ['src/Intersection/IntersectionReceiver.php', 'intersection_signature']; + } + + #[DataProvider('signatureHelpCases')] + public function testSignatureHelpResolvesMemberDeclaredOnlySecondConstituent( + string $fixture, + string $marker, + ): void { + $cursor = $this->openFixtureAtCursor($fixture, $marker); + + $result = $this->signatureHelp->handle($this->signatureHelpRequestAt($cursor)); + + self::assertIsArray($result, 'signature help must answer for a composite-receiver method call'); + self::assertNotEmpty($result['signatures'], 'signature help must return at least one signature'); + self::assertStringContainsString( + 'getName', + $result['signatures'][0]['label'], + 'the resolved signature must be Person::getName', + ); + } + + /** + * @return iterable + */ + public static function completionCases(): iterable + { + yield 'union receiver' => ['src/Union/UnionReceiver.php', 'union_completion']; + yield 'intersection receiver' => ['src/Intersection/IntersectionReceiver.php', 'intersection_completion']; + } + + #[DataProvider('completionCases')] + public function testCompletionOffersMembersFromEveryConstituent( + string $fixture, + string $marker, + ): void { + $cursor = $this->openFixtureAtCursor($fixture, $marker); + + $result = $this->completion->handle($this->completionRequestAt($cursor)); + + self::assertIsArray($result); + $labels = array_column($result['items'], 'label'); + self::assertContains('getId', $labels, 'Entity::getId must be offered on the composite receiver'); + self::assertContains('getName', $labels, 'Person::getName must be offered on the composite receiver'); + } +}