From 46e2e6db92edcec163784a28e259d97d20da4873 Mon Sep 17 00:00:00 2001 From: Matheus Martins Date: Sun, 14 Jun 2026 00:40:10 +0000 Subject: [PATCH 1/4] fix(diagnostics): clamp diagnostic ranges to document bounds An EOF-anchored syntax error (unterminated comment, "unexpected EOF") made the server emit a diagnostic whose range end was one character past end-of-document: nikic reports a 1-based BYTE `endColumn` of `lineLength + 1` at EOF, which `buildParseErrorDiagnostic` used verbatim. A strict LSP annotator (PhpStorm) throws `Range must be inside element being annotated` when asked to render that range mid-edit, poisoning the diagnostics provider for the rest of the session. Clamp every emitted range into the analyzed buffer: - PositionMap gains `clampPosition` / `clampRange` primitives (UTF-16 aware, reusing the extracted `lineUtf16Length`). - `Analyzer::buildParseErrorDiagnostic` clamps at the source so the internal Diagnostic is never out of bounds. - `XphpDiagnosticsProvider` clamps at the wire boundary via a cached PositionMap (`toLspClamped`) as defense-in-depth across parse, undefined-name, and workspace diagnostics. `DiagnosticTranslator` stays pure. Why it escaped: nothing asserted that emitted ranges stay within document bounds, and the Behat "underlines" step routed through `positionToOffset`, which silently clamps past-EOL. Adds the `AssertsRangeWithinDocument` invariant (proven to fail pre-fix on the EOF cases), PositionMap clamp + characterization tests, a provider-level wire test, and a Behat bounds step + unterminated-comment scenario. Co-Authored-By: Claude Opus 4.8 (1M context) --- features/validate/diagnostics.feature | 14 ++++ src/Analyzer/Analyzer.php | 7 ++ src/Diagnostics/XphpDiagnosticsProvider.php | 59 +++++++++++-- src/PositionMap.php | 56 ++++++++++++- test/Analyzer/AnalyzerTest.php | 41 +++++++++ test/Behat/ValidateContext.php | 20 +++++ test/Behat/World.php | 21 +++++ .../XphpDiagnosticsProviderTest.php | 29 +++++++ test/PositionMapTest.php | 83 +++++++++++++++++++ test/Support/AssertsRangeWithinDocument.php | 54 ++++++++++++ 10 files changed, 377 insertions(+), 7 deletions(-) create mode 100644 test/Support/AssertsRangeWithinDocument.php diff --git a/features/validate/diagnostics.feature b/features/validate/diagnostics.feature index 1e86281..5fcc700 100644 --- a/features/validate/diagnostics.feature +++ b/features/validate/diagnostics.feature @@ -25,6 +25,20 @@ Feature: Diagnostics When I analyze "/Broken.xphp" for diagnostics Then a "xphp.parse" diagnostic is reported saying "Syntax error" + Scenario: An EOF-anchored syntax error stays within document bounds + # Regression for the PhpStorm "Range must be inside element being annotated" + # crash: an unterminated block comment yields an EOF-anchored error whose raw + # end column lands one past EOL. The emitted range must be clamped. + Given the file at "/Unterminated.xphp" contains the following lines: + """ + getStartLine()); $endLine = PositionMap::lspLineFromNikic($e->getEndLine()); + // nikic columns are 1-based BYTE columns and, for an EOF-anchored error, + // `endColumn` is `lineLength + 1` (one past EOL). Clamp the whole range + // into the buffer so we never hand the client a range outside the + // document it can render -- a strict LSP annotator (PhpStorm) throws on + // an out-of-bounds range mid-edit. + [$startLine, $startCharacter, $endLine, $endCharacter] = + $positionMap->clampRange($startLine, $startCharacter, $endLine, $endCharacter); return new Diagnostic( startLine: $startLine, startCharacter: $startCharacter, diff --git a/src/Diagnostics/XphpDiagnosticsProvider.php b/src/Diagnostics/XphpDiagnosticsProvider.php index 7491661..2a891a4 100644 --- a/src/Diagnostics/XphpDiagnosticsProvider.php +++ b/src/Diagnostics/XphpDiagnosticsProvider.php @@ -11,7 +11,10 @@ use Phpactor\LanguageServer\Core\Server\ClientApi; use Phpactor\LanguageServer\Core\Workspace\Workspace as PhpactorWorkspace; use Phpactor\LanguageServerProtocol\Diagnostic as LspDiagnostic; +use Phpactor\LanguageServerProtocol\Position; +use Phpactor\LanguageServerProtocol\Range; use Phpactor\LanguageServerProtocol\TextDocumentItem; +use XPHP\Lsp\Analyzer\Diagnostic; use XPHP\Lsp\Analyzer\ParsedDocumentCache; use XPHP\Lsp\Analyzer\WorkspaceAnalyzer; use XPHP\Lsp\Reflection\FqnIndex; @@ -115,10 +118,16 @@ private function computeAllOpenDiagnostics(TextDocumentItem $textDocument): arra // workspace pass, covering the current document plus every OTHER open // document. Documents that fail to parse contribute their syntax // diagnostics but are kept out of the workspace pass (unusable AST). + // Per-URI (version, source) so the workspace-diagnostics pass below can + // clamp its ranges against the same buffer the diagnostics were computed + // from (reusing the cached PositionMap). + $docMeta = [$currentUri => ['version' => $textDocument->version, 'source' => $textDocument->text]]; $perFileByUri = [ - $currentUri => array_map( - static fn ($d): LspDiagnostic => DiagnosticTranslator::toLsp($d), + $currentUri => $this->toLspClamped( $currentResult->diagnostics, + $currentUri, + $textDocument->version, + $textDocument->text, ), ]; $parsedFiles = []; @@ -130,9 +139,12 @@ private function computeAllOpenDiagnostics(TextDocumentItem $textDocument): arra continue; } $otherResult = $this->cache->getOrParse($uri, $item->version, $item->text); - $perFileByUri[$uri] = array_map( - static fn ($d): LspDiagnostic => DiagnosticTranslator::toLsp($d), + $docMeta[$uri] = ['version' => $item->version, 'source' => $item->text]; + $perFileByUri[$uri] = $this->toLspClamped( $otherResult->diagnostics, + $uri, + $item->version, + $item->text, ); if ($otherResult->ast !== null) { $parsedFiles[$uri] = ['ast' => $otherResult->ast, 'source' => $item->text]; @@ -176,9 +188,12 @@ private function computeAllOpenDiagnostics(TextDocumentItem $textDocument): arra $result = []; foreach ($perFileByUri as $uri => $perFile) { - $workspaceDiagnostics = array_map( - static fn ($d): LspDiagnostic => DiagnosticTranslator::toLsp($d), + $meta = $docMeta[$uri] ?? ['version' => 0, 'source' => '']; + $workspaceDiagnostics = $this->toLspClamped( $workspaceByUri[$uri] ?? [], + $uri, + $meta['version'], + $meta['source'], ); $result[$uri] = array_merge($perFile, $workspaceDiagnostics); } @@ -186,6 +201,38 @@ private function computeAllOpenDiagnostics(TextDocumentItem $textDocument): arra return $result; } + /** + * Translate internal diagnostics to LSP-wire diagnostics, clamping every + * range into the document's bounds first. The server must never emit a range + * outside the buffer it analyzed: a strict LSP annotator (PhpStorm) throws + * `Range must be inside element being annotated` when an EOF-anchored + * diagnostic lands one character past end-of-document mid-edit. Uses the + * cached PositionMap so we don't re-scan the source. + * + * @param list $diags + * @return list + */ + private function toLspClamped(array $diags, string $uri, int $version, string $source): array + { + if ($diags === []) { + return []; + } + $positionMap = $this->cache->positionMap($uri, $version, $source); + $out = []; + foreach ($diags as $d) { + $lsp = DiagnosticTranslator::toLsp($d); + [$sl, $sc, $el, $ec] = $positionMap->clampRange( + $lsp->range->start->line, + $lsp->range->start->character, + $lsp->range->end->line, + $lsp->range->end->character, + ); + $lsp->range = new Range(new Position($sl, $sc), new Position($el, $ec)); + $out[] = $lsp; + } + return $out; + } + /** * Push `textDocument/publishDiagnostics` for every open document OTHER than * the one being linted whose diagnostics changed since we last published diff --git a/src/PositionMap.php b/src/PositionMap.php index 1be36a2..edb84be 100644 --- a/src/PositionMap.php +++ b/src/PositionMap.php @@ -163,10 +163,64 @@ public function fullLineRangeFromNikic(int $nikicLine): array if ($line >= count($this->lineOffsets)) { return [$line, 0, $line, 0]; } + return [$line, 0, $line, $this->lineUtf16Length($line)]; + } + + /** + * UTF-16 code-unit length of the visible content of a 0-based line (the + * terminating `\n` is excluded). `$line` MUST be a valid line index + * (`0 .. count($lineOffsets) - 1`); callers that may exceed the range + * should go through {@see clampPosition} first. + */ + private function lineUtf16Length(int $line): int + { $lineStart = $this->lineOffsets[$line]; + // For the last line there is no following entry; +1 here is consumed + // by the `- 1` below so the slice runs to end-of-source. For any other + // line the next entry sits just past that line's `\n`, so `- 1` drops + // the terminator and we measure visible content only. $nextLineStart = $this->lineOffsets[$line + 1] ?? strlen($this->source) + 1; $lineText = substr($this->source, $lineStart, $nextLineStart - $lineStart - 1); - return [$line, 0, $line, self::toLspCharacter($lineText)]; + return self::toLspCharacter($lineText); + } + + /** + * Clamp an LSP {line, character} into this document's bounds: line into + * `[0, lastLine]` and character into `[0, lineLength]` (lengths in UTF-16 + * code units, the LSP unit). An already-in-bounds position is returned + * unchanged. + * + * Diagnostic ranges built from parser column info can land one past EOL at + * EOF (nikic reports `endColumn == lineLength + 1` for an EOF-anchored + * error); some strict clients (PhpStorm's LSP annotator) throw when asked + * to render a range outside the document. Clamping here guarantees the + * server never emits an out-of-buffer range. + * + * @return array{0: int, 1: int} [line, character] + */ + public function clampPosition(int $line, int $character): array + { + $lastLine = count($this->lineOffsets) - 1; + $line = max(0, min($line, $lastLine)); + $character = max(0, min($character, $this->lineUtf16Length($line))); + return [$line, $character]; + } + + /** + * Clamp both endpoints of an LSP range into this document's bounds via + * {@see clampPosition}, then normalise so the end never precedes the start + * (an inverted range collapses to a zero-width range at the start). + * + * @return array{0: int, 1: int, 2: int, 3: int} [startLine, startChar, endLine, endChar] + */ + public function clampRange(int $startLine, int $startCharacter, int $endLine, int $endCharacter): array + { + [$sl, $sc] = $this->clampPosition($startLine, $startCharacter); + [$el, $ec] = $this->clampPosition($endLine, $endCharacter); + if ($el < $sl || ($el === $sl && $ec < $sc)) { + [$el, $ec] = [$sl, $sc]; + } + return [$sl, $sc, $el, $ec]; } /** diff --git a/test/Analyzer/AnalyzerTest.php b/test/Analyzer/AnalyzerTest.php index 0dcc2a0..ed6a03a 100644 --- a/test/Analyzer/AnalyzerTest.php +++ b/test/Analyzer/AnalyzerTest.php @@ -9,10 +9,13 @@ use XPHP\Lsp\Analyzer\Analyzer; use XPHP\Lsp\Analyzer\DiagnosticCode; use XPHP\Lsp\Analyzer\DiagnosticSeverity; +use XPHP\Lsp\Test\Support\AssertsRangeWithinDocument; use XPHP\Transpiler\Monomorphize\XphpSourceParser; final class AnalyzerTest extends TestCase { + use AssertsRangeWithinDocument; + public function testCleanFileReturnsAstAndNoDiagnostics(): void { $analyzer = self::buildAnalyzer(); @@ -266,6 +269,44 @@ public function testFlagsEachOccurrenceSeparately(): void self::assertCount(2, $undef); } + /** + * Invariant: NO diagnostic may carry a range outside the document it was + * computed from. Regression lock for the PhpStorm crash where an + * EOF-anchored "unexpected EOF" / unterminated-comment error produced an + * endCharacter one past end-of-line (nikic reports `endColumn == + * lineLength + 1` at EOF). Before the clamp in `buildParseErrorDiagnostic`, + * the unterminated-comment and bare-`analyzeFile($source); + // Must actually exercise the diagnostics path, else the invariant is vacuous. + self::assertNotSame([], $result->diagnostics, 'expected at least one diagnostic for malformed source'); + foreach ($result->diagnostics as $i => $d) { + self::assertRangeWithinDocument( + $source, + $d->startLine, + $d->startCharacter, + $d->endLine, + $d->endCharacter, + sprintf('diagnostic #%d (%s)', $i, $d->message), + ); + } + } + + /** @return iterable */ + public static function malformedSourceProvider(): iterable + { + yield 'unterminated string' => [' [' [""]; + yield 'incomplete turbofish' => [' ["createForHostVersion())); diff --git a/test/Behat/ValidateContext.php b/test/Behat/ValidateContext.php index a4f8435..bcd7930 100644 --- a/test/Behat/ValidateContext.php +++ b/test/Behat/ValidateContext.php @@ -135,6 +135,26 @@ public function aDiagnosticIsReportedSaying(string $code, string $text): void )); } + /** + * @Then every reported diagnostic range is within document bounds + */ + public function everyDiagnosticRangeIsWithinDocumentBounds(): void + { + foreach ($this->diagnosticItems() as $diagnostic) { + $this->world->assert( + $this->world->rangeWithinDocument($this->analyzedPath, $diagnostic->range), + sprintf( + 'diagnostic range out of document bounds (%d:%d-%d:%d): %s', + $diagnostic->range->start->line, + $diagnostic->range->start->character, + $diagnostic->range->end->line, + $diagnostic->range->end->character, + $diagnostic->message ?? '', + ), + ); + } + } + /** * @Then no diagnostics are reported */ diff --git a/test/Behat/World.php b/test/Behat/World.php index 72d173b..d269dff 100644 --- a/test/Behat/World.php +++ b/test/Behat/World.php @@ -235,6 +235,27 @@ public function textInRange(Location $location): string return $this->textForRange($location->uri, $location->range); } + /** + * True iff the LSP range sits inside the document `$uri`. Detects overshoot + * via {@see PositionMap::clampRange}: a range that clamping leaves unchanged + * is in-bounds. (Note: {@see textForRange} cannot be used for this — its + * `positionToOffset` silently clamps past-EOL, masking out-of-bounds ranges.) + */ + public function rangeWithinDocument(string $uri, object $range): bool + { + $map = new PositionMap($this->sourceFor($uri)); + [$sl, $sc, $el, $ec] = $map->clampRange( + $range->start->line, + $range->start->character, + $range->end->line, + $range->end->character, + ); + return $sl === $range->start->line + && $sc === $range->start->character + && $el === $range->end->line + && $ec === $range->end->character; + } + /** * Decode the delta-encoded semantic-token stream into absolute tokens, * slicing the source for each token's text and mapping its type index. diff --git a/test/Diagnostics/XphpDiagnosticsProviderTest.php b/test/Diagnostics/XphpDiagnosticsProviderTest.php index 1402848..db7614a 100644 --- a/test/Diagnostics/XphpDiagnosticsProviderTest.php +++ b/test/Diagnostics/XphpDiagnosticsProviderTest.php @@ -20,6 +20,7 @@ use XPHP\Lsp\Analyzer\WorkspaceAnalyzer; use XPHP\Lsp\Diagnostics\XphpDiagnosticsProvider; use XPHP\Lsp\Reflection\FqnIndex; +use XPHP\Lsp\Test\Support\AssertsRangeWithinDocument; use XPHP\Transpiler\Monomorphize\XphpSourceParser; use function Amp\Promise\wait; @@ -30,6 +31,8 @@ */ final class XphpDiagnosticsProviderTest extends TestCase { + use AssertsRangeWithinDocument; + public function testCleanSingleDocumentReturnsNoDiagnostics(): void { $workspace = new PhpactorWorkspace(); @@ -139,6 +142,32 @@ class Box { public T $item; } } } + public function testOutOfBoundsRangeIsClampedInEmittedLspDiagnostic(): void + { + // End-to-end wire proof for the PhpStorm "Range must be inside element" + // crash: an unterminated block comment yields an EOF-anchored syntax + // error whose raw end column lands one past EOL. The emitted LSP range + // must be clamped into the document. + $workspace = new PhpactorWorkspace(); + $source = "openDoc($workspace, '/Broken.xphp', $source); + + $diagnostics = $this->lint($workspace, $doc); + + self::assertNotEmpty($diagnostics); + foreach ($diagnostics as $d) { + self::assertInstanceOf(LspDiagnostic::class, $d); + self::assertRangeWithinDocument( + $source, + $d->range->start->line, + $d->range->start->character, + $d->range->end->line, + $d->range->end->character, + $d->message, + ); + } + } + public function testWorkspaceDiagnosticsTranslateToLspWireFormatRanges(): void { // Locks the array_map translation on line 96. Without it, the diff --git a/test/PositionMapTest.php b/test/PositionMapTest.php index 205308c..0c07254 100644 --- a/test/PositionMapTest.php +++ b/test/PositionMapTest.php @@ -445,4 +445,87 @@ public function testConsecutiveNewlinesProduceEmptyLines(): void self::assertSame([1, 0], $map->offsetToPosition(1)); self::assertSame([2, 0], $map->offsetToPosition(2)); } + + // ===================================================================== + // clampPosition / clampRange — keep emitted ranges inside the document + // ===================================================================== + + public function testClampPositionInBoundsIsUnchanged(): void + { + $map = new PositionMap("hello\nworld"); + self::assertSame([1, 2], $map->clampPosition(1, 2)); + } + + public function testClampPositionLinePastLastLineClampsToLastLine(): void + { + $map = new PositionMap("a\nbc"); + // Line 99 doesn't exist; clamp to last line (1), char clamps to its length (2). + self::assertSame([1, 2], $map->clampPosition(99, 99)); + } + + public function testClampPositionCharacterPastEolClampsToLineLength(): void + { + $map = new PositionMap("hello\nworld"); + self::assertSame([0, 5], $map->clampPosition(0, 99)); + } + + public function testClampPositionNegativesClampToZero(): void + { + $map = new PositionMap("hello"); + self::assertSame([0, 0], $map->clampPosition(-3, -7)); + } + + public function testClampPositionMultibyteLineUsesUtf16Length(): void + { + // "héllo" is 5 UTF-16 code units but 6 bytes (é is 2 bytes in UTF-8). + // The max valid character is the UTF-16 length, 5 — not the byte length. + $map = new PositionMap("h\u{00e9}llo"); + self::assertSame([0, 5], $map->clampPosition(0, 99)); + } + + public function testClampPositionEmojiLineCountsSurrogatePair(): void + { + // "😀" is one supplementary-plane scalar = 2 UTF-16 code units. + $map = new PositionMap("\u{1F600}"); + self::assertSame([0, 2], $map->clampPosition(0, 99)); + } + + public function testClampRangeEofOvershootClampsToLineEnd(): void + { + // Miniature of the reported PhpStorm bug: a single 5-char line whose + // diagnostic end lands one past EOL (char 6) clamps back to 5. + $map = new PositionMap("hello"); + self::assertSame([0, 0, 0, 5], $map->clampRange(0, 0, 0, 6)); + } + + public function testClampRangeNormalizesInvertedRange(): void + { + // End before start collapses to a zero-width range at the start. + $map = new PositionMap("hello\nworld"); + self::assertSame([0, 4, 0, 4], $map->clampRange(0, 4, 0, 2)); + } + + public function testClampRangeInBoundsIsUnchanged(): void + { + $map = new PositionMap("hello\nworld"); + self::assertSame([0, 1, 1, 3], $map->clampRange(0, 1, 1, 3)); + } + + // ===================================================================== + // characterization: offsetToPosition / rangeFromOffsets self-clamp past EOF + // (locks the substr-truncation behavior a refactor must not regress) + // ===================================================================== + + public function testOffsetToPositionPastStrlenClampsToDocumentEnd(): void + { + $map = new PositionMap("a\nbc"); // strlen 4, last line "bc" (len 2) + // An offset well past EOF resolves to the last line at its end, no throw. + self::assertSame([1, 2], $map->offsetToPosition(50)); + } + + public function testRangeFromOffsetsEndBytePastStrlenClampsToDocumentEnd(): void + { + $map = new PositionMap("a\nbc"); + self::assertSame([0, 0, 1, 2], $map->rangeFromOffsets(0, 50)); + } } diff --git a/test/Support/AssertsRangeWithinDocument.php b/test/Support/AssertsRangeWithinDocument.php new file mode 100644 index 0000000..e0fd9aa --- /dev/null +++ b/test/Support/AssertsRangeWithinDocument.php @@ -0,0 +1,54 @@ += 0'); + self::assertGreaterThanOrEqual(0, $startChar, $prefix . 'startChar >= 0'); + self::assertLessThanOrEqual($lastLine, $endLine, $prefix . 'endLine within document'); + self::assertGreaterThanOrEqual($startLine, $endLine, $prefix . 'endLine >= startLine'); + + // The `\n` terminator is not part of the line's renderable content, so + // each line's max valid character is its UTF-16 content length. + $startLineLen = PositionMap::lengthInUtf16($lines[$startLine] ?? ''); + $endLineLen = PositionMap::lengthInUtf16($lines[$endLine] ?? ''); + self::assertLessThanOrEqual($startLineLen, $startChar, $prefix . 'startChar within its line'); + self::assertLessThanOrEqual($endLineLen, $endChar, $prefix . 'endChar within its line'); + + if ($startLine === $endLine) { + self::assertGreaterThanOrEqual($startChar, $endChar, $prefix . 'endChar >= startChar on same line'); + } + } +} From 7621af00f5d52371583324dd8fef00e53be375ce Mon Sep 17 00:00:00 2001 From: Matheus Martins Date: Sun, 14 Jun 2026 00:40:10 +0000 Subject: [PATCH 2/4] ci(release): bump version in server info sent to clients Release 0.2.4. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/LspDispatcherFactory.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/LspDispatcherFactory.php b/src/LspDispatcherFactory.php index 9c2aa58..71801ea 100644 --- a/src/LspDispatcherFactory.php +++ b/src/LspDispatcherFactory.php @@ -376,7 +376,7 @@ public function create(MessageTransmitter $transmitter, InitializeParams $initia new ErrorHandlingMiddleware($this->logger), new InitializeMiddleware($handlers, $eventDispatcher, [ 'name' => 'xphp-lsp', - 'version' => '0.2.3', + 'version' => '0.2.4', ]), new ShutdownMiddleware($eventDispatcher), new ResponseHandlingMiddleware($responseWatcher), From c204e76394cd13cb113c70fbf9fd696452a15aa2 Mon Sep 17 00:00:00 2001 From: Matheus Martins Date: Sun, 14 Jun 2026 01:05:11 +0000 Subject: [PATCH 3/4] feat(hover): substitute generic type params in property hovers Hovering a property accessed through a generic receiver showed the raw declared type parameter (`public A $first`) instead of the concrete type, even though the variable-inference path already computed the substituted type (the assigned `$x = $pair->first` hover showed `Pair`). Wire the existing substitution into the member-hover path: - GenericResolver gains `resolvePropertyTypeAt`, which finds the PropertyFetch at the cursor and renders its substituted type via the existing `resolvePropertyFetch` (same machinery the assignment-tracking path uses -- no new substitution logic). - PhpHoverResolver passes that type into `renderProperty`, which prefers it over `prettify(inferredType)` and falls back cleanly when the receiver isn't a tracked generic instantiation. Now `$nested->swap()->first` hovers as `public App\...\Pair $first`, matching the LHS-variable hover. Covers the direct-receiver and chained-method-call cases with tests. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/Resolver/GenericResolver.php | 44 +++++++++++++++++ src/Resolver/PhpHoverResolver.php | 17 ++++++- test/Resolver/PhpHoverResolverTest.php | 68 ++++++++++++++++++++++++++ 3 files changed, 127 insertions(+), 2 deletions(-) diff --git a/src/Resolver/GenericResolver.php b/src/Resolver/GenericResolver.php index 4c531c1..8293c77 100644 --- a/src/Resolver/GenericResolver.php +++ b/src/Resolver/GenericResolver.php @@ -593,6 +593,50 @@ public function resolvePropertyReceiverClassAt(string $uri, int $byteOffset): ?s return $fqn; } + /** + * Resolve the substituted concrete type of the property access at + * `$byteOffset`, rendered (e.g. `App\Containers\Pair`). This is the member-hover parallel of the + * assignment-tracking path: where `resolvePropertyReceiverClassAt` answers + * "which class hosts this property", this answers "what concrete type does + * the property have once the receiver's type-args are substituted in" -- + * so hovering `$pair->first` shows the substituted type instead of the + * declared type-param `A`. + * + * Returns null when the cursor isn't on a property-name token, the + * receiver isn't a tracked generic instantiation, or the property type + * isn't a shape we can model (union / intersection -- caller falls back + * to the unsubstituted render). + */ + public function resolvePropertyTypeAt(string $uri, int $byteOffset): ?string + { + if (!$this->workspace->has($uri)) { + return null; + } + $item = $this->workspace->get($uri); + $scopes = $this->scopesFor($uri, $item->version, $item->text); + $bindings = self::bindingsAt($scopes, $byteOffset); + + $result = $this->documents->getOrParse($uri, $item->version, $item->text); + if ($result->ast === null) { + return null; + } + $fetch = self::findEnclosingPropertyFetchNameAt($result->ast, $byteOffset); + if ($fetch === null) { + return null; + } + [$useMap, $namespace] = self::useMapAndNamespaceFor($result->ast); + $resolved = self::resolvePropertyFetch( + $fetch, + $bindings, + $this->classes, + $this->fqnIndex, + $useMap, + $namespace, + ); + return $resolved?->render(); + } + /** * Walk the AST looking for a `PropertyFetch` or `NullsafePropertyFetch` * whose property-name identifier covers `$byteOffset`. The cursor diff --git a/src/Resolver/PhpHoverResolver.php b/src/Resolver/PhpHoverResolver.php index 170d553..9c4aaff 100644 --- a/src/Resolver/PhpHoverResolver.php +++ b/src/Resolver/PhpHoverResolver.php @@ -171,6 +171,12 @@ private function resolveInner(string $uri, int $line, int $character, ?Cancellat $propertyReceiver = $this->genericResolver->resolvePropertyReceiverClassAt($uri, $offset) ?? self::containerOrNull($context) ?? ''; + // Substituted concrete type of the property at the cursor, e.g. + // hovering `$pair->first` on a `Pair` receiver yields + // `Pair` instead of the declared type-param `A`. Null + // when the receiver isn't a tracked generic instantiation -- the + // renderer falls back to the declared (prettified) type. + $propertyType = $this->genericResolver->resolvePropertyTypeAt($uri, $offset); $markdown = match ($symbol->symbolType()) { Symbol::CLASS_ => $this->fanOutRender( self::preferType($context, $symbol->name()), @@ -195,6 +201,7 @@ private function resolveInner(string $uri, int $line, int $character, ?Cancellat fn (string $fqn): ?string => $this->renderProperty( $fqn, $symbol->name(), + $propertyType, ), ), Symbol::CONSTANT, @@ -347,7 +354,7 @@ private function renderMethod(string $classFqn, string $methodName, ?MethodCallS return self::format($signature, $docblock); } - private function renderProperty(?string $classFqn, string $propertyName): ?string + private function renderProperty(?string $classFqn, string $propertyName, ?string $substitutedType = null): ?string { if ($classFqn === null) { return null; @@ -364,7 +371,13 @@ private function renderProperty(?string $classFqn, string $propertyName): ?strin } $visibility = (string) $property->visibility(); $static = $property->isStatic() ? 'static ' : ''; - $type = $this->genericParams->prettify((string) $property->inferredType()); + // When the cursor is on a property access through a tracked generic + // receiver (`$pair->first` with `$pair: Pair`), + // GenericResolver has already substituted the type-params, so the + // declared `A` renders as the concrete `Pair`. Fall + // back to prettify(inferredType) when there's no substitution (plain + // property, untracked receiver, union/intersection type). + $type = $substitutedType ?? $this->genericParams->prettify((string) $property->inferredType()); $signature = sprintf( "// %s\n%s %s%s\$%s", $classFqn, diff --git a/test/Resolver/PhpHoverResolverTest.php b/test/Resolver/PhpHoverResolverTest.php index 5befcd9..98068cb 100644 --- a/test/Resolver/PhpHoverResolverTest.php +++ b/test/Resolver/PhpHoverResolverTest.php @@ -114,6 +114,74 @@ class User { ); } + public function testPropertyHoverSubstitutesTypeParamThroughDirectReceiver(): void + { + // Hovering a property whose declared type is a type-param + // (`public A $first`) on a tracked generic receiver shows the + // SUBSTITUTED concrete type, not the raw `A`. For + // `$pair: Pair`, `$pair->first` is `Plastic`. + $workspace = $this->workspace(); + $this->open($workspace, '/Plastic.xphp', "open($workspace, '/User.xphp', "open($workspace, '/Pair.xphp', <<<'XPHP' + { + public function __construct( + public A $first, + public B $second, + ) {} + public function swap(): Pair { + return new Pair::($this->second, $this->first); + } + } + XPHP); + $useSource = "(new Plastic(), new User());\necho \$pair->first;\n"; + $this->open($workspace, '/Use.xphp', $useSource); + + $hover = $this->hoverAt($workspace, '/Use.xphp', $useSource, '->first', strlen('->')); + $markdown = $this->markdown($hover); + + self::assertStringContainsString('public App\\Plastic $first', $markdown); + self::assertStringNotContainsString('public A $first', $markdown); + } + + public function testPropertyHoverSubstitutesTypeParamThroughChainedMethodCall(): void + { + // Headline: `$nested->swap()->first` where + // `$nested: Pair, Pair>`. swap() returns + // `Pair`, so the result's `A` (-> `$first`) is `Pair`. + // Previously the member hover showed the raw `public A $first`. + $workspace = $this->workspace(); + $this->open($workspace, '/Plastic.xphp', "open($workspace, '/User.xphp', "open($workspace, '/Map.xphp', " {}\n"); + $this->open($workspace, '/Pair.xphp', <<<'XPHP' + { + public function __construct( + public A $first, + public B $second, + ) {} + public function swap(): Pair { + return new Pair::($this->second, $this->first); + } + } + XPHP); + $useSource = ", Pair>(new Map(), new Pair::(new Plastic(), new User()));\n" + . "echo \$nested->swap()->first;\n"; + $this->open($workspace, '/Use.xphp', $useSource); + + $hover = $this->hoverAt($workspace, '/Use.xphp', $useSource, '->first', strlen('->')); + $markdown = $this->markdown($hover); + + self::assertStringContainsString('Pair $first', $markdown); + self::assertStringNotContainsString('public A $first', $markdown); + } + public function testHoversStaticPropertyWithStaticModifier(): void { // Pins the `$static = $property->isStatic() ? 'static ' : ''` From 70bf07127f7c0a1661b8c02e0a5528e2a59e259d Mon Sep 17 00:00:00 2001 From: Matheus Martins Date: Sun, 14 Jun 2026 01:08:58 +0000 Subject: [PATCH 4/4] test(hover): behat coverage for generic property type substitution End-to-end scenarios through the real hover handler: a property accessed through a direct generic receiver (`$pair->first` on `Pair`) and through a chained generic method call (`$nested->swap()->first`) both hover with the substituted concrete type instead of the raw type-param `A`. Co-Authored-By: Claude Opus 4.8 (1M context) --- features/understand/hover.feature | 79 +++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/features/understand/hover.feature b/features/understand/hover.feature index 1e6e141..53c292f 100644 --- a/features/understand/hover.feature +++ b/features/understand/hover.feature @@ -103,3 +103,82 @@ Feature: Hover And the FQN index has been warmed on initialize When I request "textDocument/hover" on "fresh" at line 3 of "/Use.xphp" Then the hover contents contain "fresh(int $v): App\Builder" + + Scenario: Hover over a generic property substitutes the type parameter + Given the file at "/Plastic.xphp" contains the following lines: + """ + + { + public function __construct(public A $first, public B $second) {} + public function swap(): Pair { return new Pair::($this->second, $this->first); } + } + """ + And the file at "/Use.xphp" contains the following lines: + """ + (new Plastic(), new User()); + echo $pair->first; + """ + And the FQN index has been warmed on initialize + When I request "textDocument/hover" on "first" at line 5 of "/Use.xphp" + Then the hover contents contain "App\Plastic $first" + + Scenario: Hover over a property through a chained generic method call substitutes the type parameter + Given the file at "/Plastic.xphp" contains the following lines: + """ + {} + """ + And the file at "/Pair.xphp" contains the following lines: + """ + + { + public function __construct(public A $first, public B $second) {} + public function swap(): Pair { return new Pair::($this->second, $this->first); } + } + """ + And the file at "/Use.xphp" contains the following lines: + """ + , Pair>(new Map(), new Pair::(new Plastic(), new User())); + echo $nested->swap()->first; + """ + And the FQN index has been warmed on initialize + When I request "textDocument/hover" on "first" at line 6 of "/Use.xphp" + Then the hover contents contain "Pair $first"