From 2f78119eac263c629fd37eddcac4889818ed738d Mon Sep 17 00:00:00 2001 From: Matheus Martins Date: Thu, 21 May 2026 19:14:25 +0000 Subject: [PATCH 01/17] lsp: scaffold tools/lsp/ with analyzer + --lint mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Initial slice of the xphp Language Server. Lives in a separate composer package under tools/lsp/ so its dependencies (notably phpactor/language-server, landing next) don't weigh down the core parser. Path-references the parent xphp-parser package via composer repositories=type=path, same precedent as playground/. What works in this commit: - tools/lsp/bin/xphp-lsp --lint Headless CLI mode: parse the file, run cross-file Registry + bound validation, print "file:line:col: severity: [code] message" for every diagnostic, exit non-zero if any fired. Useful in CI today, independent of the LSP transport. - Per-file Analyzer wrapping XphpSourceParser in try/catch so syntax errors surface as Diagnostic objects instead of throwing. - WorkspaceAnalyzer running TypeHierarchy + Registry across all files and catching the duplicate-declaration / bound-violation RuntimeExceptions, pinning each diagnostic to the file that triggered it. - PositionMap utility: byte offset -> LSP {line, character} + full-line ranges for line-only nikic errors. - DocumentStore stub for the upcoming workspace document sync. - PHPUnit suite with three test files (PositionMap, Analyzer, WorkspaceAnalyzer) covering syntax errors, bound violations on scalars + unknown classes, duplicate templates, and the happy path. What's deliberately deferred to the next commits: - LSP stdio mode itself (phpactor/language-server bootstrap + initialize handshake) — Server::runLspMode() exits cleanly with a "not yet wired" stderr message so editors probing the binary get a clean shutdown rather than a hang. - Hover, go-to-definition, completion handlers. - VS Code extension client (vscode-extension/ has a placeholder README documenting the contract). Approach: the analyzer wraps the existing pipeline rather than modifying it. Registry / TypeHierarchy / XphpSourceParser stay untouched -- zero risk to the 93% MSI on the core. Trade-off documented in tools/lsp/README.md under "Status". Makefile gains `make test/lsp` which runs composer install + phpunit inside tools/lsp/. Co-Authored-By: Claude Opus 4.7 (1M context) --- Makefile | 4 + tools/lsp/.gitignore | 3 + tools/lsp/README.md | 107 +++++++++++ tools/lsp/bin/xphp-lsp | 31 +++ tools/lsp/composer.json | 45 +++++ tools/lsp/phpunit.xml.dist | 20 ++ tools/lsp/src/Analyzer/Analyzer.php | 80 ++++++++ tools/lsp/src/Analyzer/Diagnostic.php | 27 +++ tools/lsp/src/Analyzer/DiagnosticSeverity.php | 17 ++ tools/lsp/src/Analyzer/ParseResult.php | 27 +++ tools/lsp/src/Analyzer/WorkspaceAnalyzer.php | 179 ++++++++++++++++++ tools/lsp/src/PositionMap.php | 108 +++++++++++ tools/lsp/src/Server.php | 115 +++++++++++ tools/lsp/src/Workspace/DocumentStore.php | 74 ++++++++ tools/lsp/test/Analyzer/AnalyzerTest.php | 66 +++++++ .../test/Analyzer/WorkspaceAnalyzerTest.php | 143 ++++++++++++++ tools/lsp/test/PositionMapTest.php | 54 ++++++ tools/lsp/test/bootstrap.php | 8 + tools/lsp/vscode-extension/README.md | 35 ++++ 19 files changed, 1143 insertions(+) create mode 100644 tools/lsp/.gitignore create mode 100644 tools/lsp/README.md create mode 100755 tools/lsp/bin/xphp-lsp create mode 100644 tools/lsp/composer.json create mode 100644 tools/lsp/phpunit.xml.dist create mode 100644 tools/lsp/src/Analyzer/Analyzer.php create mode 100644 tools/lsp/src/Analyzer/Diagnostic.php create mode 100644 tools/lsp/src/Analyzer/DiagnosticSeverity.php create mode 100644 tools/lsp/src/Analyzer/ParseResult.php create mode 100644 tools/lsp/src/Analyzer/WorkspaceAnalyzer.php create mode 100644 tools/lsp/src/PositionMap.php create mode 100644 tools/lsp/src/Server.php create mode 100644 tools/lsp/src/Workspace/DocumentStore.php create mode 100644 tools/lsp/test/Analyzer/AnalyzerTest.php create mode 100644 tools/lsp/test/Analyzer/WorkspaceAnalyzerTest.php create mode 100644 tools/lsp/test/PositionMapTest.php create mode 100644 tools/lsp/test/bootstrap.php create mode 100644 tools/lsp/vscode-extension/README.md diff --git a/Makefile b/Makefile index fba69401..943ae73f 100644 --- a/Makefile +++ b/Makefile @@ -6,6 +6,10 @@ test/unit: test/mutation: php vendor/bin/infection --show-mutations=max --threads=max --min-covered-msi=93 +.PHONY: test/lsp +test/lsp: + cd tools/lsp && composer install --quiet && php vendor/bin/phpunit + .PHONY: playground playground: playground/bin/run diff --git a/tools/lsp/.gitignore b/tools/lsp/.gitignore new file mode 100644 index 00000000..525642da --- /dev/null +++ b/tools/lsp/.gitignore @@ -0,0 +1,3 @@ +/vendor/ +/composer.lock +.phpunit.result.cache diff --git a/tools/lsp/README.md b/tools/lsp/README.md new file mode 100644 index 00000000..2e125d91 --- /dev/null +++ b/tools/lsp/README.md @@ -0,0 +1,107 @@ +# xphp Language Server + +LSP implementation that powers diagnostics, hover, go-to-definition, and completion for `.xphp` +files in VS Code (and any LSP-aware editor). Reuses the `xphp-parser` AST + `Registry` + +`TypeHierarchy` from the parent package directly — no separate parser, no duplication. + +This is a separate Composer package living under `tools/lsp/` so it can declare its own +dependencies (notably `phpactor/language-server` for the JSON-RPC scaffolding) without +weighing down the core parser. + +## Status + +| Feature | Status | +|---|---| +| `--lint ` headless mode (parse + bound checks) | ✅ shipped | +| `textDocument/publishDiagnostics` over stdio | 🚧 wiring next | +| `textDocument/hover` | 🚧 | +| `textDocument/definition` | 🚧 | +| `textDocument/completion` | 🚧 | +| VS Code extension client | 🚧 lives at `vscode-extension/` | + +The plan that drives this implementation is in `agent-os/product/specs/feat-lsp.md` +(internal) / referenced from `docs/roadmap.md`. + +## Layout + +``` +tools/lsp/ +├── composer.json path-references the parent xphp-parser +├── bin/xphp-lsp CLI entry; --lint mode works today, LSP mode pending +├── src/ +│ ├── Server.php entry-point router; dispatches --lint vs LSP +│ ├── PositionMap.php byte offset <-> {line, char} for LSP positions +│ ├── Analyzer/ +│ │ ├── Analyzer.php per-file parse + syntax-error collection +│ │ ├── WorkspaceAnalyzer cross-file Registry + TypeHierarchy + bound check +│ │ ├── Diagnostic.php framework-neutral diagnostic value object +│ │ ├── DiagnosticSeverity LSP-aligned enum +│ │ └── ParseResult.php {ast, diagnostics} +│ ├── Handler/ (one file per LSP request type — coming soon) +│ └── Workspace/ document store + indexer +├── test/ PHPUnit suite +└── vscode-extension/ minimal VS Code client (spawn server over stdio) +``` + +## Install + +```bash +cd tools/lsp +composer install +``` + +The parent `xphp-parser` package is path-referenced via composer (`repositories: type=path`), +so any local edits there are picked up immediately without re-publishing. + +## Run + +### Lint mode (works today) + +```bash +tools/lsp/bin/xphp-lsp --lint path/to/file.xphp [more.xphp...] +``` + +Output format: `::: : [] ` — the same shape PHPStan +and PHP itself emit, so editors / CI grep it without ceremony. Exits non-zero if any file has +diagnostics, zero otherwise. + +This is genuinely useful in CI today, independent of the LSP wiring: run it over a `.xphp` +glob to catch bound violations and syntax errors in PRs before merge. + +### LSP mode (coming next) + +```bash +tools/lsp/bin/xphp-lsp # speaks LSP over stdio +``` + +Currently exits cleanly with a "not yet wired" stderr message. The next commit wires the +phpactor/language-server bootstrap and `textDocument/publishDiagnostics`. + +## Test + +```bash +cd tools/lsp +vendor/bin/phpunit +``` + +Or from the repo root: + +```bash +make test/lsp +``` + +## VS Code extension + +See `vscode-extension/README.md` for client-side setup (a follow-up commit; the directory is +currently a placeholder). + +## Why a separate composer package + +The parser package (`xphp-lang/xphp-parser`) has a deliberately minimal dependency surface — +`nikic/php-parser` + `symfony/console`. The LSP needs `phpactor/language-server` and its +transitive deps (`amphp/`, `webmozart/`, …). Keeping the two as separate composer packages +means a downstream consumer who only wants the parser (e.g. a CI pipeline running `bin/xphp +compile`) doesn't pull in any of the LSP machinery. + +The path-repo precedent for this setup is `playground/composer.json` — the layout here +follows the same pattern. diff --git a/tools/lsp/bin/xphp-lsp b/tools/lsp/bin/xphp-lsp new file mode 100755 index 00000000..ccdb361d --- /dev/null +++ b/tools/lsp/bin/xphp-lsp @@ -0,0 +1,31 @@ +#!/usr/bin/env php + + + + + + test + + + diff --git a/tools/lsp/src/Analyzer/Analyzer.php b/tools/lsp/src/Analyzer/Analyzer.php new file mode 100644 index 00000000..28016c03 --- /dev/null +++ b/tools/lsp/src/Analyzer/Analyzer.php @@ -0,0 +1,80 @@ +parser->parse($source); + return new ParseResult($ast, []); + } catch (PhpParserError $e) { + // nikic/php-parser carries the start line on PhpParser\Error directly. + // We pin the diagnostic to the full line — column-accurate ranges are a + // follow-up using $e->getStartColumn($source). + return new ParseResult( + ast: null, + diagnostics: [self::buildLineDiagnostic( + $positionMap, + $e->getStartLine(), + 'xphp.parse', + 'Syntax error: ' . $e->getRawMessage(), + )], + ); + } catch (RuntimeException $e) { + // XphpSourceParser also throws plain RuntimeException for "parser returned null" + // and similar unrecoverable states; surface those as line-1 errors so the user + // sees *something* in the gutter rather than nothing. + return new ParseResult( + ast: null, + diagnostics: [self::buildLineDiagnostic( + $positionMap, + 1, + 'xphp.parse.internal', + $e->getMessage(), + )], + ); + } + } + + private static function buildLineDiagnostic( + PositionMap $positionMap, + int $nikicLine, + string $code, + string $message, + ): Diagnostic { + [$startLine, $startChar, $endLine, $endChar] = $positionMap->fullLineRangeFromNikic($nikicLine); + return new Diagnostic( + startLine: $startLine, + startCharacter: $startChar, + endLine: $endLine, + endCharacter: $endChar, + message: $message, + code: $code, + ); + } +} diff --git a/tools/lsp/src/Analyzer/Diagnostic.php b/tools/lsp/src/Analyzer/Diagnostic.php new file mode 100644 index 00000000..6c79eac5 --- /dev/null +++ b/tools/lsp/src/Analyzer/Diagnostic.php @@ -0,0 +1,27 @@ +value` straight into the wire-format Diagnostic without translation. + */ +enum DiagnosticSeverity: int +{ + case Error = 1; + case Warning = 2; + case Information = 3; + case Hint = 4; +} diff --git a/tools/lsp/src/Analyzer/ParseResult.php b/tools/lsp/src/Analyzer/ParseResult.php new file mode 100644 index 00000000..5a12fff4 --- /dev/null +++ b/tools/lsp/src/Analyzer/ParseResult.php @@ -0,0 +1,27 @@ +|null $ast + * @param list $diagnostics + */ + public function __construct( + public ?array $ast, + public array $diagnostics, + ) { + } +} diff --git a/tools/lsp/src/Analyzer/WorkspaceAnalyzer.php b/tools/lsp/src/Analyzer/WorkspaceAnalyzer.php new file mode 100644 index 00000000..ad6c99f5 --- /dev/null +++ b/tools/lsp/src/Analyzer/WorkspaceAnalyzer.php @@ -0,0 +1,179 @@ +, source: string}> $files keyed by URI/path + * @return array> diagnostics keyed by URI/path + */ + public function analyze(array $files): array + { + $diagnosticsByFile = array_fill_keys(array_keys($files), []); + + $astPerFile = []; + foreach ($files as $path => $entry) { + $astPerFile[$path] = $entry['ast']; + } + $hierarchy = TypeHierarchy::fromAstPerFile($astPerFile); + $registry = new Registry(hierarchy: $hierarchy); + + // First pass: definitions. Catch duplicate-declaration RuntimeExceptions and pin + // them on the second declaration's file (which is what the compiler also reports). + foreach ($files as $path => $entry) { + $positionMap = new PositionMap($entry['source']); + $this->walkDefinitions($entry['ast'], $registry, $path, $positionMap, $diagnosticsByFile[$path]); + } + + // Second pass: instantiations. Bound violations fire here. + foreach ($files as $path => $entry) { + $positionMap = new PositionMap($entry['source']); + $this->walkInstantiations($entry['ast'], $registry, $positionMap, $diagnosticsByFile[$path]); + } + + return $diagnosticsByFile; + } + + /** + * @param list $ast + * @param list $diagnostics + */ + private function walkDefinitions( + array $ast, + Registry $registry, + string $sourceFile, + PositionMap $positionMap, + array &$diagnostics, + ): void { + $visitor = new class($registry, $sourceFile, $positionMap, $diagnostics) extends NodeVisitorAbstract { + /** @param list $diagnostics */ + public function __construct( + private readonly Registry $registry, + private readonly string $sourceFile, + private readonly PositionMap $positionMap, + private array &$diagnostics, + ) { + } + + public function enterNode(Node $node): null + { + if (!$node instanceof ClassLike || $node->name === null) { + return null; + } + $params = $node->getAttribute(XphpSourceParser::ATTR_GENERIC_PARAMS); + $fqn = $node->getAttribute(XphpSourceParser::ATTR_TEMPLATE_FQN); + if (!is_array($params) || $params === [] || !is_string($fqn)) { + return null; + } + // Deliberately do NOT pre-check for "already registered" — the whole point + // of running this in the analyzer is to surface the duplicate-declaration + // RuntimeException from Registry::recordDefinition as a diagnostic. + try { + $this->registry->recordDefinition( + $fqn, + $node->name->toString(), + $params, + $node, + $this->sourceFile, + ); + } catch (RuntimeException $e) { + $this->diagnostics[] = self::buildDiagnostic($this->positionMap, $node->getStartLine(), 'xphp.definition', $e->getMessage()); + } + return null; + } + + private static function buildDiagnostic(PositionMap $positionMap, int $nikicLine, string $code, string $message): Diagnostic + { + [$sl, $sc, $el, $ec] = $positionMap->fullLineRangeFromNikic($nikicLine); + return new Diagnostic($sl, $sc, $el, $ec, $message, code: $code); + } + }; + + $traverser = new NodeTraverser(); + $traverser->addVisitor($visitor); + $traverser->traverse($ast); + } + + /** + * @param list $ast + * @param list $diagnostics + */ + private function walkInstantiations( + array $ast, + Registry $registry, + PositionMap $positionMap, + array &$diagnostics, + ): void { + $visitor = new class($registry, $positionMap, $diagnostics) extends NodeVisitorAbstract { + /** @param list $diagnostics */ + public function __construct( + private readonly Registry $registry, + private readonly PositionMap $positionMap, + private array &$diagnostics, + ) { + } + + public function enterNode(Node $node): null + { + if (!$node instanceof Name) { + return null; + } + $args = $node->getAttribute(XphpSourceParser::ATTR_GENERIC_ARGS); + $fqn = $node->getAttribute(XphpSourceParser::ATTR_TEMPLATE_FQN); + if (!is_array($args) || $args === [] || !is_string($fqn)) { + return null; + } + foreach ($args as $a) { + if (!$a->isConcrete()) { + return null; + } + } + try { + $this->registry->recordInstantiation($fqn, $args); + } catch (RuntimeException $e) { + [$sl, $sc, $el, $ec] = $this->positionMap->fullLineRangeFromNikic($node->getStartLine()); + $this->diagnostics[] = new Diagnostic( + startLine: $sl, + startCharacter: $sc, + endLine: $el, + endCharacter: $ec, + message: $e->getMessage(), + code: 'xphp.bound', + ); + } + return null; + } + }; + + $traverser = new NodeTraverser(); + $traverser->addVisitor($visitor); + $traverser->traverse($ast); + } +} diff --git a/tools/lsp/src/PositionMap.php b/tools/lsp/src/PositionMap.php new file mode 100644 index 00000000..0abdb2a6 --- /dev/null +++ b/tools/lsp/src/PositionMap.php @@ -0,0 +1,108 @@ + LSP {line, character} conversion for a single in-memory document. + * + * The xphp pipeline (nikic/php-parser + the custom scanner) hands us byte offsets + * and 1-based line numbers. LSP wants 0-based lines and 0-based UTF-16 code-unit + * columns within each line. This class owns the conversion for one source string. + * + * MVP simplification: column = UTF-8 character index, not UTF-16 code-unit index. + * For pure ASCII (which describes basically all .xphp source we'll see in practice) + * these are identical. A real UTF-16 implementation is a small follow-up — see + * `toLspCharacter()` for the seam. + */ +final readonly class PositionMap +{ + /** @var list Byte offsets of the start of each line (0-indexed by line number). */ + private array $lineOffsets; + + public function __construct(private string $source) + { + $offsets = [0]; + $length = strlen($source); + for ($i = 0; $i < $length; $i++) { + if ($source[$i] === "\n") { + $offsets[] = $i + 1; + } + } + $this->lineOffsets = $offsets; + } + + /** + * Convert a 1-based line number (as nikic reports) to a 0-based LSP line number. + */ + public static function lspLineFromNikic(int $nikicLine): int + { + return max(0, $nikicLine - 1); + } + + /** + * Resolve a byte offset to {line, character} (both 0-based) for this document. + * + * @return array{0: int, 1: int} [line, character] + */ + public function offsetToPosition(int $byteOffset): array + { + if ($byteOffset < 0) { + throw new OutOfBoundsException("Negative byte offset: {$byteOffset}"); + } + $line = self::binarySearchLine($this->lineOffsets, $byteOffset); + $lineStart = $this->lineOffsets[$line]; + $character = self::toLspCharacter(substr($this->source, $lineStart, $byteOffset - $lineStart)); + return [$line, $character]; + } + + /** + * Resolve a 1-based nikic line number to the {line, character} range covering that whole line. + * + * Useful when a Registry RuntimeException carries only line information and we need to + * produce a Diagnostic range — underlining the full line is the safe default. + * + * @return array{0: int, 1: int, 2: int, 3: int} [startLine, startChar, endLine, endChar] + */ + public function fullLineRangeFromNikic(int $nikicLine): array + { + $line = self::lspLineFromNikic($nikicLine); + if ($line >= count($this->lineOffsets)) { + return [$line, 0, $line, 0]; + } + $lineStart = $this->lineOffsets[$line]; + $nextLineStart = $this->lineOffsets[$line + 1] ?? strlen($this->source) + 1; + $lineText = substr($this->source, $lineStart, $nextLineStart - $lineStart - 1); + return [$line, 0, $line, self::toLspCharacter($lineText)]; + } + + /** + * Character count as the LSP client sees it. See class doc — MVP: UTF-8 chars. + */ + private static function toLspCharacter(string $text): int + { + // mb_strlen falls back to byte-length on missing extension; explicit utf-8 is safer. + return function_exists('mb_strlen') ? mb_strlen($text, 'UTF-8') : strlen($text); + } + + /** + * @param list $offsets + */ + private static function binarySearchLine(array $offsets, int $target): int + { + $low = 0; + $high = count($offsets) - 1; + while ($low < $high) { + $mid = intdiv($low + $high + 1, 2); + if ($offsets[$mid] <= $target) { + $low = $mid; + } else { + $high = $mid - 1; + } + } + return $low; + } +} diff --git a/tools/lsp/src/Server.php b/tools/lsp/src/Server.php new file mode 100644 index 00000000..6307ceb3 --- /dev/null +++ b/tools/lsp/src/Server.php @@ -0,0 +1,115 @@ + [...] → one-shot analyzer; prints diagnostics + * and exits non-zero on errors + * + * The --lint mode is the analyzer's "headless" surface: CI can call it on every + * .xphp file in a PR to surface diagnostics without standing up an LSP client. + * It also doubles as the integration-test affordance for the LSP test suite. + * + * The stdio mode wires phpactor/language-server's CoreServerBuilder, registers + * the per-feature handlers, and runs until the client closes the channel. That + * wiring is intentionally separated from the analyzer so the analyzer can be + * tested without spinning up a transport — see test/Analyzer/*. + */ +final class Server +{ + /** + * @param list $argv + */ + public static function run(array $argv): int + { + if (in_array('--lint', $argv, true)) { + return self::runLintMode($argv); + } + return self::runLspMode(); + } + + /** + * @param list $argv + */ + private static function runLintMode(array $argv): int + { + $files = array_values(array_filter( + array_slice($argv, 1), + static fn (string $a): bool => $a !== '--lint' && !str_starts_with($a, '--'), + )); + if ($files === []) { + fwrite(STDERR, "Usage: xphp-lsp --lint [ ...]\n"); + return 2; + } + + $analyzer = self::buildAnalyzer(); + $workspaceAnalyzer = new WorkspaceAnalyzer(); + + // Phase 1: per-file parse + syntax diagnostics. + $perFileSyntax = []; + $parsedFiles = []; + foreach ($files as $path) { + $source = @file_get_contents($path); + if ($source === false) { + fwrite(STDERR, "{$path}: cannot read\n"); + return 2; + } + $result = $analyzer->analyzeFile($source); + $perFileSyntax[$path] = $result->diagnostics; + if ($result->ast !== null) { + $parsedFiles[$path] = ['ast' => $result->ast, 'source' => $source]; + } + } + + // Phase 2: workspace-level diagnostics across files that parsed cleanly. + $workspaceDiagnostics = $workspaceAnalyzer->analyze($parsedFiles); + + $exitCode = 0; + foreach ($files as $path) { + $all = array_merge($perFileSyntax[$path] ?? [], $workspaceDiagnostics[$path] ?? []); + foreach ($all as $d) { + self::printDiagnostic($path, $d); + $exitCode = 1; + } + } + return $exitCode; + } + + private static function runLspMode(): int + { + // The LSP server wiring on top of phpactor/language-server lands in a follow-up + // commit (see plan, step 3 in "Implementation order"). Until then, this mode + // exits cleanly with a marker so editors that probe the binary get a clean + // shutdown rather than a hang. + fwrite(STDERR, "xphp-lsp: stdio LSP mode not yet wired — use `--lint ` for now.\n"); + return 0; + } + + private static function buildAnalyzer(): Analyzer + { + return new Analyzer(new XphpSourceParser((new ParserFactory())->createForHostVersion())); + } + + private static function printDiagnostic(string $path, Diagnostic $d): void + { + // Compiler-style "file:line:col: severity: message" output. Editors and CI + // both grep this format. + $line = $d->startLine + 1; + $col = $d->startCharacter + 1; + $sev = strtolower($d->severity->name); + fwrite(STDOUT, "{$path}:{$line}:{$col}: {$sev}: [{$d->code}] {$d->message}\n"); + } +} diff --git a/tools/lsp/src/Workspace/DocumentStore.php b/tools/lsp/src/Workspace/DocumentStore.php new file mode 100644 index 00000000..596be9ea --- /dev/null +++ b/tools/lsp/src/Workspace/DocumentStore.php @@ -0,0 +1,74 @@ +}> */ + private array $documents = []; + + public function open(string $uri, string $source): void + { + $this->documents[$uri] = ['source' => $source, 'ast' => null]; + } + + public function change(string $uri, string $source): void + { + if (!isset($this->documents[$uri])) { + $this->open($uri, $source); + return; + } + $this->documents[$uri]['source'] = $source; + $this->documents[$uri]['ast'] = null; // invalidate parsed AST until next analyze + } + + public function close(string $uri): void + { + unset($this->documents[$uri]); + } + + public function source(string $uri): ?string + { + return $this->documents[$uri]['source'] ?? null; + } + + /** + * @param list $ast + */ + public function cacheAst(string $uri, array $ast): void + { + if (!isset($this->documents[$uri])) { + return; + } + $this->documents[$uri]['ast'] = $ast; + } + + /** + * @return list|null + */ + public function ast(string $uri): ?array + { + return $this->documents[$uri]['ast'] ?? null; + } + + /** + * @return list + */ + public function openUris(): array + { + return array_keys($this->documents); + } +} diff --git a/tools/lsp/test/Analyzer/AnalyzerTest.php b/tools/lsp/test/Analyzer/AnalyzerTest.php new file mode 100644 index 00000000..11ea5c64 --- /dev/null +++ b/tools/lsp/test/Analyzer/AnalyzerTest.php @@ -0,0 +1,66 @@ +analyzeFile(<<<'PHP' + { + public T $item; + } + PHP); + + self::assertSame([], $result->diagnostics); + self::assertNotNull($result->ast); + } + + public function testSyntaxErrorProducesDiagnosticWithNullAst(): void + { + // Unterminated string literal — unrecoverable parse error. + $analyzer = self::buildAnalyzer(); + $result = $analyzer->analyzeFile(<<<'PHP' + ast, 'unrecoverable syntax error should null out the AST'); + self::assertCount(1, $result->diagnostics); + self::assertSame('xphp.parse', $result->diagnostics[0]->code); + self::assertSame(DiagnosticSeverity::Error, $result->diagnostics[0]->severity); + self::assertStringContainsString('Syntax error', $result->diagnostics[0]->message); + } + + public function testSyntaxErrorRangeReferencesAValidLine(): void + { + $analyzer = self::buildAnalyzer(); + $result = $analyzer->analyzeFile(<<<'PHP' + diagnostics); + $d = $result->diagnostics[0]; + // Diagnostic must point at a valid line in the document (0-based). + self::assertGreaterThanOrEqual(0, $d->startLine); + self::assertGreaterThanOrEqual($d->startLine, $d->endLine); + } + + private static function buildAnalyzer(): Analyzer + { + return new Analyzer(new XphpSourceParser((new ParserFactory())->createForHostVersion())); + } +} diff --git a/tools/lsp/test/Analyzer/WorkspaceAnalyzerTest.php b/tools/lsp/test/Analyzer/WorkspaceAnalyzerTest.php new file mode 100644 index 00000000..ee49a285 --- /dev/null +++ b/tools/lsp/test/Analyzer/WorkspaceAnalyzerTest.php @@ -0,0 +1,143 @@ +parseFiles([ + '/Box.xphp' => <<<'PHP' + + { + public T $item; + } + PHP, + '/Use.xphp' => <<<'PHP' + (); + PHP, + ]); + + $diagnostics = (new WorkspaceAnalyzer())->analyze($files); + + self::assertSame([], $diagnostics['/Box.xphp'], 'template file itself has no violation'); + self::assertCount(1, $diagnostics['/Use.xphp'], 'instantiation file should carry one bound-violation diagnostic'); + self::assertStringContainsString('Generic bound violated', $diagnostics['/Use.xphp'][0]->message); + self::assertSame('xphp.bound', $diagnostics['/Use.xphp'][0]->code); + } + + public function testBoundViolationOnUnknownClassReportsDistinctMessage(): void + { + $files = $this->parseFiles([ + '/Box.xphp' => <<<'PHP' + + { + public T $item; + } + PHP, + '/Use.xphp' => <<<'PHP' + (); + PHP, + ]); + + $diagnostics = (new WorkspaceAnalyzer())->analyze($files); + + self::assertCount(1, $diagnostics['/Use.xphp']); + self::assertStringContainsString( + 'not in the source set', + $diagnostics['/Use.xphp'][0]->message, + 'unknown concrete should hit the "compiler cannot prove satisfaction" branch', + ); + } + + public function testValidWorkspaceProducesNoDiagnostics(): void + { + $files = $this->parseFiles([ + '/Box.xphp' => <<<'PHP' + + { + public T $item; + } + PHP, + '/Tag.xphp' => <<<'PHP' + <<<'PHP' + (); + PHP, + ]); + + $diagnostics = (new WorkspaceAnalyzer())->analyze($files); + + foreach ($diagnostics as $path => $list) { + self::assertSame([], $list, "{$path} should have no diagnostics"); + } + } + + public function testDuplicateTemplateDeclarationProducesDiagnostic(): void + { + $files = $this->parseFiles([ + '/BoxOne.xphp' => <<<'PHP' + { public T $item; } + PHP, + '/BoxTwo.xphp' => <<<'PHP' + { public T $item; } + PHP, + ]); + + $diagnostics = (new WorkspaceAnalyzer())->analyze($files); + + // The second declaration encountered carries the duplicate-declaration diagnostic. + // Iteration order over $files is insertion order, so BoxOne wins, BoxTwo gets the error. + self::assertSame([], $diagnostics['/BoxOne.xphp']); + self::assertCount(1, $diagnostics['/BoxTwo.xphp']); + self::assertStringContainsString('already declared', $diagnostics['/BoxTwo.xphp'][0]->message); + self::assertSame('xphp.definition', $diagnostics['/BoxTwo.xphp'][0]->code); + } + + /** + * @param array $sources keyed by path → source + * @return array, source: string}> + */ + private function parseFiles(array $sources): array + { + $parser = new XphpSourceParser((new ParserFactory())->createForHostVersion()); + $analyzer = new Analyzer($parser); + $out = []; + foreach ($sources as $path => $source) { + $result = $analyzer->analyzeFile($source); + self::assertNotNull($result->ast, "fixture {$path} should parse without syntax errors"); + $out[$path] = ['ast' => $result->ast, 'source' => $source]; + } + return $out; + } +} diff --git a/tools/lsp/test/PositionMapTest.php b/tools/lsp/test/PositionMapTest.php new file mode 100644 index 00000000..866f16e8 --- /dev/null +++ b/tools/lsp/test/PositionMapTest.php @@ -0,0 +1,54 @@ +offsetToPosition(0)); + } + + public function testOffsetAtSecondLineStart(): void + { + $map = new PositionMap("hello\nworld"); + // 'w' is at byte offset 6 (after "hello\n"). + self::assertSame([1, 0], $map->offsetToPosition(6)); + } + + public function testOffsetMidLine(): void + { + $map = new PositionMap("hello\nworld"); + // 'r' is byte 8 ('w'=6, 'o'=7, 'r'=8); character 2 on line 1. + self::assertSame([1, 2], $map->offsetToPosition(8)); + } + + public function testFullLineRangeFromNikicReturnsZeroBasedLineAndLength(): void + { + $map = new PositionMap("abc\ndefgh\nij"); + // nikic line 2 → LSP line 1 ("defgh", 5 chars). + self::assertSame([1, 0, 1, 5], $map->fullLineRangeFromNikic(2)); + } + + public function testFullLineRangeForLastLineWithoutTrailingNewline(): void + { + $map = new PositionMap("a\nbc"); + // nikic line 2 → LSP line 1 ("bc", 2 chars). + self::assertSame([1, 0, 1, 2], $map->fullLineRangeFromNikic(2)); + } + + public function testLspLineFromNikicClampsAtZero(): void + { + // Defensive: nikic shouldn't emit line 0, but a defensive clamp prevents a + // negative line from sneaking into the wire format. + self::assertSame(0, PositionMap::lspLineFromNikic(0)); + self::assertSame(0, PositionMap::lspLineFromNikic(1)); + self::assertSame(4, PositionMap::lspLineFromNikic(5)); + } +} diff --git a/tools/lsp/test/bootstrap.php b/tools/lsp/test/bootstrap.php new file mode 100644 index 00000000..01f308e5 --- /dev/null +++ b/tools/lsp/test/bootstrap.php @@ -0,0 +1,8 @@ +` adjustments), and LSP transport over stdio. + +## Status + +Skeleton stub — not yet wired. The `package.json`, `extension.ts`, and the +TextMate grammar fork land in the dedicated VS-Code-extension commit (see the +`feat/lsp` branch plan, step 8). + +Until then, the LSP server is usable from the command line via: + +```bash +tools/lsp/bin/xphp-lsp --lint path/to/file.xphp +``` + +## Build / debug (planned) + +Once `package.json` lands: + +```bash +cd tools/lsp/vscode-extension +npm install +npm run compile +``` + +Then in VS Code, open the `tools/lsp/vscode-extension/` folder and hit **F5** to +launch an Extension Development Host window with the extension loaded. + +## Publishing + +Out of scope for the initial branch. Marketplace publication happens after the +core features stabilise. From e38c83fc721f95fe8bf03092f51a4f4ee31fd2c4 Mon Sep 17 00:00:00 2001 From: Matheus Martins Date: Thu, 21 May 2026 19:22:12 +0000 Subject: [PATCH 02/17] lsp: silence E_DEPRECATED at xphp-lsp entry-point phpactor/language-server pulls in thecodingmachine/safe, which currently trips PHP 8.4's implicit-nullable deprecation warning on dozens of generated wrapper functions at autoload time. Left alone, that chatter floods stderr -- LSP clients pipe stderr through to the editor and we'd look broken before we even ran. Suppressing E_DEPRECATED at the bin script keeps the LSP's own deprecation surface clean (we control that), while muting third-party noise from a dep we don't own. Verified end-to-end via --lint mode: tools/lsp/bin/xphp-lsp --lint -> exit 0, no output tools/lsp/bin/xphp-lsp --lint -> exit 1, single line tools/lsp/bin/xphp-lsp --lint -> exit 1, single line tools/lsp/bin/xphp-lsp --lint -> exit 2 `vendor/bin/phpunit` inside tools/lsp/ passes 13/13. Co-Authored-By: Claude Opus 4.7 (1M context) --- tools/lsp/bin/xphp-lsp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tools/lsp/bin/xphp-lsp b/tools/lsp/bin/xphp-lsp index ccdb361d..1cc13315 100755 --- a/tools/lsp/bin/xphp-lsp +++ b/tools/lsp/bin/xphp-lsp @@ -10,6 +10,12 @@ declare(strict_types=1); // The actual server boot lives in XPHP\Lsp\Server::run() — this script is // just the thinnest possible CLI veneer plus autoloader resolution. +// Silence E_DEPRECATED at the entry-point. phpactor/language-server pulls in +// thecodingmachine/safe, which currently emits many implicit-nullable deprecation +// notices under PHP 8.4 at autoload time. Without this, the deprecation chatter +// drowns the analyzer's actual output (an LSP client sees this on stderr too). +error_reporting(E_ALL & ~E_DEPRECATED); + $autoload = null; foreach ([ __DIR__ . '/../vendor/autoload.php', // standalone install (composer install in tools/lsp/) From 58ef08cd826a36b0f7df6a9c6dd979a22002b8ea Mon Sep 17 00:00:00 2001 From: Matheus Martins Date: Thu, 21 May 2026 19:59:06 +0000 Subject: [PATCH 03/17] lsp: wire stdio LSP transport + diagnostics over phpactor/language-server Replaces the `xphp-lsp` placeholder runLspMode() with a working LSP server that publishes diagnostics to any LSP-aware client in real time. VS Code (and Neovim / Helix / Sublime etc., once their respective clients point at the binary) now see syntax errors, bound violations, and duplicate-template diagnostics as the user types -- no `bin/xphp compile` round-trip required. Pieces: - `LspDispatcherFactory` -- builds phpactor's standard middleware stack (ErrorHandling, Initialize, Shutdown, ResponseHandling, Cancellation, Handler) and wires `DiagnosticsService` -> `DiagnosticsEngine` -> `XphpDiagnosticsProvider` on top of it. Closely mirrors phpactor's own acme-ls example; only the diagnostics provider construction is xphp-specific. - `XphpDiagnosticsProvider` -- implements `phpactor/language-server`'s `DiagnosticsProvider`. On each provideDiagnostics() call, runs the per-file Analyzer over the document and the cross-file WorkspaceAnalyzer over the whole set of currently-open documents in the phpactor Workspace. Returns LSP-shape diagnostics for just the document the client is asking about. - `DiagnosticTranslator` -- single static method translating the analyzer's framework-neutral `Diagnostic` to phpactor's wire-format `Diagnostic`. Boundary lives in exactly one place. - `Server::runLspMode()` -- now actually wires `LanguageServerBuilder`. Default transport = stdio. Tested end-to-end by piping a synthetic LSP initialize request through the binary; got a properly-framed Content-Length response with capabilities.textDocumentSync = FULL and serverInfo = {name: "xphp-lsp", version: "0.1.0"}. Tests: - LspDispatcherFactoryTest -- uses phpactor's LanguageServerTester to drive the dispatcher exactly like the real server. Asserts the initialize handshake returns InitializeResult with textDocumentSync set and serverInfo advertising "xphp-lsp". - XphpDiagnosticsProviderTest -- 4 cases covering clean document, syntax error, cross-file bound violation, provider name match. `make test/lsp` now runs 19 tests, 56 assertions, all green. `make test/unit` still runs 171 tests, 453 assertions, all green. Test infra polish: - phpunit.xml.dist gains `` so PHP 8.4 implicit-nullable warnings from phpactor's transitive deps don't crowd the test output. Our own code keeps full deprecation coverage. - test/bootstrap.php mutes E_DEPRECATED for the same reason at runtime. - Makefile's test/lsp target passes `php -d error_reporting='E_ALL & ~E_DEPRECATED'` so the suppression lands before composer autoload fires. What's NOT in this commit (next chunks): - Hover handler -- next PR. - Definition handler. - Completion handler + workspace symbol cache. - VS Code extension client (vscode-extension/ still placeholder). - Cross-file diagnostic broadcast: today, editing Box.xphp re-publishes diagnostics for Box.xphp only, not for every Use.xphp that instantiates it. The diagnostic catches up when Use.xphp is next touched. Acceptable for MVP; explicit broadcast is a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) --- Makefile | 2 +- tools/lsp/phpunit.xml.dist | 14 ++ .../src/Diagnostics/DiagnosticTranslator.php | 33 +++++ .../Diagnostics/XphpDiagnosticsProvider.php | 103 ++++++++++++++ tools/lsp/src/LspDispatcherFactory.php | 133 ++++++++++++++++++ tools/lsp/src/Server.php | 21 +-- .../XphpDiagnosticsProviderTest.php | 116 +++++++++++++++ tools/lsp/test/LspDispatcherFactoryTest.php | 63 +++++++++ tools/lsp/test/bootstrap.php | 6 +- 9 files changed, 480 insertions(+), 11 deletions(-) create mode 100644 tools/lsp/src/Diagnostics/DiagnosticTranslator.php create mode 100644 tools/lsp/src/Diagnostics/XphpDiagnosticsProvider.php create mode 100644 tools/lsp/src/LspDispatcherFactory.php create mode 100644 tools/lsp/test/Diagnostics/XphpDiagnosticsProviderTest.php create mode 100644 tools/lsp/test/LspDispatcherFactoryTest.php diff --git a/Makefile b/Makefile index 943ae73f..8d33c9fc 100644 --- a/Makefile +++ b/Makefile @@ -8,7 +8,7 @@ test/mutation: .PHONY: test/lsp test/lsp: - cd tools/lsp && composer install --quiet && php vendor/bin/phpunit + cd tools/lsp && composer install --quiet && php -d error_reporting='E_ALL & ~E_DEPRECATED' vendor/bin/phpunit .PHONY: playground playground: diff --git a/tools/lsp/phpunit.xml.dist b/tools/lsp/phpunit.xml.dist index 47235a51..ad27558c 100644 --- a/tools/lsp/phpunit.xml.dist +++ b/tools/lsp/phpunit.xml.dist @@ -17,4 +17,18 @@ test + + + + + src + + diff --git a/tools/lsp/src/Diagnostics/DiagnosticTranslator.php b/tools/lsp/src/Diagnostics/DiagnosticTranslator.php new file mode 100644 index 00000000..4a1a4de2 --- /dev/null +++ b/tools/lsp/src/Diagnostics/DiagnosticTranslator.php @@ -0,0 +1,33 @@ +startLine, $d->startCharacter), + new Position($d->endLine, $d->endCharacter), + ), + message: $d->message, + ); + $lsp->severity = $d->severity->value; + $lsp->code = $d->code !== '' ? $d->code : null; + $lsp->source = 'xphp'; + return $lsp; + } +} diff --git a/tools/lsp/src/Diagnostics/XphpDiagnosticsProvider.php b/tools/lsp/src/Diagnostics/XphpDiagnosticsProvider.php new file mode 100644 index 00000000..45c160e1 --- /dev/null +++ b/tools/lsp/src/Diagnostics/XphpDiagnosticsProvider.php @@ -0,0 +1,103 @@ +analyze($textDocument); + return new Success($diagnostics); + } + + public function name(): string + { + return 'xphp'; + } + + /** + * @return list + */ + private function analyze(TextDocumentItem $textDocument): array + { + $currentUri = $textDocument->uri; + + // Per-file syntax pass on the document being linted. + $currentResult = $this->analyzer->analyzeFile($textDocument->text); + $perFileDiagnostics = array_map( + static fn ($d) => DiagnosticTranslator::toLsp($d), + $currentResult->diagnostics, + ); + + // If the document didn't parse, the workspace pass would skip it anyway — + // and feeding it through TypeHierarchy::fromAstPerFile() with a null AST + // is meaningless. Return the syntax diagnostics alone. + if ($currentResult->ast === null) { + return $perFileDiagnostics; + } + + // Build the {uri: {ast, source}} map for the workspace pass: the current + // document plus every OTHER open document that we can parse cleanly. + $parsedFiles = [ + $currentUri => ['ast' => $currentResult->ast, 'source' => $textDocument->text], + ]; + foreach ($this->workspace as $uri => $item) { + if ($uri === $currentUri) { + continue; + } + $otherResult = $this->analyzer->analyzeFile($item->text); + if ($otherResult->ast === null) { + continue; + } + $parsedFiles[$uri] = ['ast' => $otherResult->ast, 'source' => $item->text]; + } + + $workspaceByUri = $this->workspaceAnalyzer->analyze($parsedFiles); + $currentWorkspaceDiagnostics = $workspaceByUri[$currentUri] ?? []; + + $lspWorkspaceDiagnostics = array_map( + static fn ($d) => DiagnosticTranslator::toLsp($d), + $currentWorkspaceDiagnostics, + ); + + return array_merge($perFileDiagnostics, $lspWorkspaceDiagnostics); + } +} diff --git a/tools/lsp/src/LspDispatcherFactory.php b/tools/lsp/src/LspDispatcherFactory.php new file mode 100644 index 00000000..9ffb6fef --- /dev/null +++ b/tools/lsp/src/LspDispatcherFactory.php @@ -0,0 +1,133 @@ +logger); + + $diagnosticsProvider = new XphpDiagnosticsProvider( + new Analyzer(new XphpSourceParser((new ParserFactory())->createForHostVersion())), + new WorkspaceAnalyzer(), + $workspace, + ); + + $diagnosticsEngine = new DiagnosticsEngine( + $clientApi, + $this->logger, + [$diagnosticsProvider], + $this->diagnosticsDebounceMs, + ); + + $diagnosticsService = new DiagnosticsService($diagnosticsEngine, workspace: $workspace); + + $serviceProviders = new ServiceProviders($diagnosticsService); + $serviceManager = new ServiceManager($serviceProviders, $this->logger); + + // DiagnosticsService is both a ServiceProvider AND a ListenerProviderInterface — + // registering it directly on the event dispatcher is what subscribes + // provideDiagnostics() to didOpen / didChange / didSave events. + $eventDispatcher = new AggregateEventDispatcher( + new ServiceListener($serviceManager), + new WorkspaceListener($workspace), + $diagnosticsService, + ); + + $handlers = new Handlers( + new TextDocumentHandler($eventDispatcher), + new ServiceHandler($serviceManager, $clientApi), + new CommandHandler(new CommandDispatcher([])), + new ExitHandler(), + ); + + $runner = new HandlerMethodRunner( + $handlers, + new ChainArgumentResolver( + new LanguageSeverProtocolParamsResolver(), + new PassThroughArgumentResolver(), + ), + ); + + return new MiddlewareDispatcher( + new ErrorHandlingMiddleware($this->logger), + new InitializeMiddleware($handlers, $eventDispatcher, [ + 'name' => 'xphp-lsp', + 'version' => '0.1.0', + ]), + new ShutdownMiddleware($eventDispatcher), + new ResponseHandlingMiddleware($responseWatcher), + new CancellationMiddleware($runner), + new HandlerMiddleware($runner), + ); + } +} diff --git a/tools/lsp/src/Server.php b/tools/lsp/src/Server.php index 6307ceb3..11d948fa 100644 --- a/tools/lsp/src/Server.php +++ b/tools/lsp/src/Server.php @@ -5,6 +5,7 @@ namespace XPHP\Lsp; use PhpParser\ParserFactory; +use Phpactor\LanguageServer\LanguageServerBuilder; use XPHP\Lsp\Analyzer\Analyzer; use XPHP\Lsp\Analyzer\Diagnostic; use XPHP\Lsp\Analyzer\WorkspaceAnalyzer; @@ -23,10 +24,10 @@ * .xphp file in a PR to surface diagnostics without standing up an LSP client. * It also doubles as the integration-test affordance for the LSP test suite. * - * The stdio mode wires phpactor/language-server's CoreServerBuilder, registers - * the per-feature handlers, and runs until the client closes the channel. That - * wiring is intentionally separated from the analyzer so the analyzer can be - * tested without spinning up a transport — see test/Analyzer/*. + * The stdio mode wires phpactor/language-server's LanguageServerBuilder + * (see LspDispatcherFactory) and runs until the client closes the channel. The + * dispatcher factory is intentionally separated from the analyzer so the analyzer + * can be tested without spinning up a transport — see test/Analyzer/*. */ final class Server { @@ -90,11 +91,13 @@ private static function runLintMode(array $argv): int private static function runLspMode(): int { - // The LSP server wiring on top of phpactor/language-server lands in a follow-up - // commit (see plan, step 3 in "Implementation order"). Until then, this mode - // exits cleanly with a marker so editors that probe the binary get a clean - // shutdown rather than a hang. - fwrite(STDERR, "xphp-lsp: stdio LSP mode not yet wired — use `--lint ` for now.\n"); + // Build phpactor's standard LSP server over stdio. The dispatcher factory + // wires our XphpDiagnosticsProvider through phpactor's DiagnosticsEngine, + // which handles enqueue / debounce / publishDiagnostics for us. The + // LanguageServerBuilder defaults to stdio when no .tcpServer() is set. + LanguageServerBuilder::create(new LspDispatcherFactory()) + ->build() + ->run(); return 0; } diff --git a/tools/lsp/test/Diagnostics/XphpDiagnosticsProviderTest.php b/tools/lsp/test/Diagnostics/XphpDiagnosticsProviderTest.php new file mode 100644 index 00000000..09958be2 --- /dev/null +++ b/tools/lsp/test/Diagnostics/XphpDiagnosticsProviderTest.php @@ -0,0 +1,116 @@ +openDoc($workspace, '/clean.xphp', <<<'XPHP' + { public T $item; } + XPHP); + + $diagnostics = $this->lint($workspace,$clean); + self::assertSame([], $diagnostics); + } + + public function testSyntaxErrorYieldsSingleDiagnostic(): void + { + $workspace = new PhpactorWorkspace(); + $broken = $this->openDoc($workspace, '/broken.xphp', <<<'XPHP' + lint($workspace,$broken); + self::assertCount(1, $diagnostics); + self::assertSame('xphp', $diagnostics[0]->source); + self::assertSame('xphp.parse', $diagnostics[0]->code); + self::assertStringContainsString('Syntax error', $diagnostics[0]->message); + } + + public function testBoundViolationAcrossOpenDocumentsAttachesToInstantiationFile(): void + { + $workspace = new PhpactorWorkspace(); + $this->openDoc($workspace, '/Box.xphp', <<<'XPHP' + + { + public T $item; + } + XPHP); + $useDoc = $this->openDoc($workspace, '/Use.xphp', <<<'XPHP' + (); + XPHP); + + $diagnostics = $this->lint($workspace,$useDoc); + self::assertCount(1, $diagnostics); + self::assertSame('xphp.bound', $diagnostics[0]->code); + self::assertStringContainsString('Generic bound violated', $diagnostics[0]->message); + // And the Box.xphp side carries no diagnostic for itself. + $boxItem = $workspace->get('/Box.xphp'); + $boxDiagnostics = $this->lint($workspace,$boxItem); + self::assertSame([], $boxDiagnostics); + } + + public function testProviderNameMatchesEngineRegistration(): void + { + // DiagnosticsEngine keys provider state by name; mismatch silently breaks clear-on-update. + $provider = $this->newProvider(new PhpactorWorkspace()); + self::assertSame('xphp', $provider->name()); + } + + /** + * @return list + */ + private function lint(PhpactorWorkspace $workspace, TextDocumentItem $textDocument): array + { + $provider = $this->newProvider($workspace); + $cancel = (new CancellationTokenSource())->getToken(); + $promise = $provider->provideDiagnostics($textDocument, $cancel); + $result = wait($promise); + return is_array($result) ? array_values($result) : []; + } + + private function newProvider(PhpactorWorkspace $workspace): XphpDiagnosticsProvider + { + $parser = new XphpSourceParser((new ParserFactory())->createForHostVersion()); + return new XphpDiagnosticsProvider( + new Analyzer($parser), + new WorkspaceAnalyzer(), + $workspace, + ); + } + + private function openDoc(PhpactorWorkspace $workspace, string $uri, string $text): TextDocumentItem + { + $item = new TextDocumentItem($uri, 'xphp', 1, $text); + $workspace->open($item); + return $item; + } +} diff --git a/tools/lsp/test/LspDispatcherFactoryTest.php b/tools/lsp/test/LspDispatcherFactoryTest.php new file mode 100644 index 00000000..dc231e3b --- /dev/null +++ b/tools/lsp/test/LspDispatcherFactoryTest.php @@ -0,0 +1,63 @@ +buildTester(); + + $result = $tester->initialize(); + + self::assertInstanceOf(InitializeResult::class, $result); + self::assertSame( + TextDocumentSyncKind::FULL, + $result->capabilities->textDocumentSync, + 'textDocumentSync must be FULL — phpactor TextDocumentHandler registers this and the client needs it to know how to push updates', + ); + } + + public function testServerInfoAdvertisesXphpName(): void + { + $tester = $this->buildTester(); + + $result = $tester->initialize(); + + self::assertNotNull($result->serverInfo); + self::assertSame('xphp-lsp', $result->serverInfo['name'] ?? null); + } + + private function buildTester(): LanguageServerTester + { + return new LanguageServerTester( + new LspDispatcherFactory(), + new InitializeParams(new ClientCapabilities()), + ); + } +} diff --git a/tools/lsp/test/bootstrap.php b/tools/lsp/test/bootstrap.php index 01f308e5..7c054ae6 100644 --- a/tools/lsp/test/bootstrap.php +++ b/tools/lsp/test/bootstrap.php @@ -3,6 +3,10 @@ declare(strict_types=1); ini_set('display_errors', '1'); -ini_set('error_reporting', (string) E_ALL); +// Mute E_DEPRECATED from phpactor's transitive deps (thecodingmachine/safe) so +// PHPUnit's deprecation-as-fail config doesn't get tripped by their PHP 8.4 +// implicit-nullable warnings at autoload time. Our own code still gets full +// error_reporting via ini_set above. +ini_set('error_reporting', (string) (E_ALL & ~E_DEPRECATED)); require dirname(__DIR__) . '/vendor/autoload.php'; From 3318872fd3bb0fa43c8b6cda35647f682ace1e2f Mon Sep 17 00:00:00 2001 From: Matheus Martins Date: Thu, 21 May 2026 20:05:05 +0000 Subject: [PATCH 04/17] lsp: implement textDocument/hover for generic instantiations + type-params Adds hover support so editors show useful tooltips when the cursor lands on something xphp-specific: - Over a generic instantiation (e.g. `Box`) -> "Specializes to: \XPHP\Generated\App\Box\T_" plus the template name and the concrete args, in markdown. - Over a type-param reference inside a template body (e.g. the `T` in `public T $item;`) -> "Type parameter `T` of `App\Box`" plus the bound if one was declared. Pieces: - `XphpHoverHandler` implements phpactor's `Handler` + `CanRegisterCapabilities`. Registers `hoverProvider` capability at initialize, dispatches `textDocument/hover` to a typed handler method, returns `Promise`. Pulls the current text from phpactor's Workspace (already kept in sync by WorkspaceListener) and re-parses on demand -- analyzer is fast enough that caching ASTs across hover calls would be premature. - `AstPositionResolver` -- standalone helper that walks an AST for the smallest `Name` node whose [startFilePos, endFilePos] range contains a target byte offset. Also returns the stack of enclosing `ClassLike` nodes at the hit, which is what lets the hover handler resolve `T` against the surrounding template's `ATTR_GENERIC_PARAMS`. Extracted as its own class because DefinitionHandler will reuse the same logic. - `PositionMap::positionToOffset()` -- new method (inverse of the existing `offsetToPosition`). Editors send {line, character}; the AST walker wants a byte offset. Single UTF-8 codepoint length table; multi-byte chars handled correctly even though our LSP column semantics still pretend they're UTF-16 (matches the existing offsetToPosition gap, documented there). Limitations called out in the code: - Hover over bound names in template headers (the `\Stringable` in `class Box`) is not yet supported. XphpSourceParser strips the `<...>` clause before parsing, so the AST has no Name positioned over `\Stringable`. Surfacing this needs an extra scanner pass; deferred to a follow-up alongside DefinitionHandler since both share the same gap. - Method-scoped type-params (e.g. `function NAME` inside a class) aren't surfaced yet. The `ATTR_METHOD_GENERIC_PARAMS` attribute is already available; just a matter of extending the resolver. Next chunk. Tests: - `AstPositionResolverTest` -- finds smallest Name; rejects between-tokens offsets; captures enclosing ClassLike scope; clamps past EOF. - `XphpHoverHandlerTest` -- generic instantiation, type-param reference, non-generic plain Name (returns null), unknown URI (returns null). `make test/lsp` -> OK (27 tests, 82 assertions). `make test/unit` still -> OK (171, 453). Smoke-tested end-to-end through the stdio LSP transport: - initialize -> capabilities now include `hoverProvider`. - didOpen { "();" } - hover @ (line 2, char 10 = the `B` of Box) -> contents: "**`App\\Box`** Specializes to: `\\XPHP\\Generated\\App\\Box\\T_`" Co-Authored-By: Claude Opus 4.7 (1M context) --- tools/lsp/src/Handler/AstPositionResolver.php | 87 ++++++++++ tools/lsp/src/Handler/XphpHoverHandler.php | 163 ++++++++++++++++++ tools/lsp/src/LspDispatcherFactory.php | 6 +- tools/lsp/src/PositionMap.php | 55 ++++++ .../test/Handler/AstPositionResolverTest.php | 85 +++++++++ .../lsp/test/Handler/XphpHoverHandlerTest.php | 122 +++++++++++++ 6 files changed, 517 insertions(+), 1 deletion(-) create mode 100644 tools/lsp/src/Handler/AstPositionResolver.php create mode 100644 tools/lsp/src/Handler/XphpHoverHandler.php create mode 100644 tools/lsp/test/Handler/AstPositionResolverTest.php create mode 100644 tools/lsp/test/Handler/XphpHoverHandlerTest.php diff --git a/tools/lsp/src/Handler/AstPositionResolver.php b/tools/lsp/src/Handler/AstPositionResolver.php new file mode 100644 index 00000000..84b20037 --- /dev/null +++ b/tools/lsp/src/Handler/AstPositionResolver.php @@ -0,0 +1,87 @@ + $ast + * @return array{name: Name, classScope: list}|null + */ + public static function nameAtOffset(array $ast, int $offset): ?array + { + $visitor = new class($offset) extends NodeVisitorAbstract { + /** @var list */ + private array $classStack = []; + + public ?Name $found = null; + + /** @var list */ + public array $foundClassStack = []; + + public function __construct(private readonly int $offset) + { + } + + public function enterNode(Node $node): null + { + if ($node instanceof ClassLike) { + $this->classStack[] = $node; + } + if (!$node instanceof Name) { + return null; + } + $start = $node->getStartFilePos(); + $end = $node->getEndFilePos(); + if ($start < 0 || $end < 0) { + return null; + } + // endFilePos points at the LAST byte of the node (inclusive), + // so the half-open range is [start, end + 1). + if ($this->offset < $start || $this->offset > $end) { + return null; + } + // Keep the SMALLEST matching node — replace any previous match + // because nikic walks parents before children. + $this->found = $node; + $this->foundClassStack = $this->classStack; + return null; + } + + public function leaveNode(Node $node): null + { + if ($node instanceof ClassLike) { + array_pop($this->classStack); + } + return null; + } + }; + + $traverser = new NodeTraverser(); + $traverser->addVisitor($visitor); + $traverser->traverse($ast); + + if ($visitor->found === null) { + return null; + } + return ['name' => $visitor->found, 'classScope' => $visitor->foundClassStack]; + } +} diff --git a/tools/lsp/src/Handler/XphpHoverHandler.php b/tools/lsp/src/Handler/XphpHoverHandler.php new file mode 100644 index 00000000..7999a877 --- /dev/null +++ b/tools/lsp/src/Handler/XphpHoverHandler.php @@ -0,0 +1,163 @@ +" plus the template and args. + * + * 2. Cursor over a 1-segment Name whose identifier matches a type-param + * declared on an enclosing ClassLike -- show "Type parameter T of " + * plus the bound, if any. + * + * Not yet covered: + * + * - Hover over bound names in template headers (`class Box`): + * XphpSourceParser strips the `<…>` clause before parsing, so the AST has no + * Name node positioned over `\Stringable`. Surfacing this needs an extra + * scanner pass; defer to a follow-up alongside DefinitionHandler. + * - Hover over method-scoped type-params. + */ +final class XphpHoverHandler implements Handler, CanRegisterCapabilities +{ + public function __construct( + private readonly PhpactorWorkspace $workspace, + private readonly Analyzer $analyzer, + ) { + } + + public function methods(): array + { + return [ + 'textDocument/hover' => 'hover', + ]; + } + + public function registerCapabiltiies(ServerCapabilities $capabilities): void + { + $capabilities->hoverProvider = new HoverOptions(); + } + + /** + * @return Promise + */ + public function hover(HoverParams $params): Promise + { + if (!$this->workspace->has($params->textDocument->uri)) { + return new Success(null); + } + $item = $this->workspace->get($params->textDocument->uri); + $result = $this->analyzer->analyzeFile($item->text); + if ($result->ast === null) { + return new Success(null); + } + + $positionMap = new PositionMap($item->text); + $offset = $positionMap->positionToOffset( + $params->position->line, + $params->position->character, + ); + + $hit = AstPositionResolver::nameAtOffset($result->ast, $offset); + if ($hit === null) { + return new Success(null); + } + + $markdown = $this->buildHoverMarkdown($hit['name'], $hit['classScope']); + if ($markdown === null) { + return new Success(null); + } + + return new Success(new Hover(new MarkupContent(MarkupKind::MARKDOWN, $markdown))); + } + + /** + * @param list $classScope + */ + private function buildHoverMarkdown(\PhpParser\Node\Name $name, array $classScope): ?string + { + $args = $name->getAttribute(XphpSourceParser::ATTR_GENERIC_ARGS); + $templateFqn = $name->getAttribute(XphpSourceParser::ATTR_TEMPLATE_FQN); + + // Case 1: hover over a generic instantiation. + if (is_array($args) && $args !== [] && is_string($templateFqn) && self::allConcrete($args)) { + $generatedFqn = Registry::generatedFqn($templateFqn, $args); + $argsText = implode(', ', array_map(static fn (TypeRef $r): string => $r->toDisplayString(), $args)); + return sprintf( + "**`%s<%s>`**\n\nSpecializes to: `\\%s`", + $templateFqn, + $argsText, + $generatedFqn, + ); + } + + // Case 2: hover over a single-segment Name that matches an enclosing + // template's type-param. + $parts = $name->getParts(); + if (count($parts) !== 1) { + return null; + } + $shortName = $parts[0]; + foreach (array_reverse($classScope) as $classLike) { + $params = $classLike->getAttribute(XphpSourceParser::ATTR_GENERIC_PARAMS); + if (!is_array($params)) { + continue; + } + foreach ($params as $param) { + if (!$param instanceof TypeParam || $param->name !== $shortName) { + continue; + } + $owner = $classLike->getAttribute(XphpSourceParser::ATTR_TEMPLATE_FQN); + $boundLine = $param->boundFqn !== null + ? sprintf("\n\nbounded by `\\%s`", $param->boundFqn) + : ''; + return sprintf( + "**Type parameter `%s`** of `%s`%s", + $param->name, + is_string($owner) ? $owner : (string) $classLike->name, + $boundLine, + ); + } + } + + return null; + } + + /** + * @param list $args + */ + private static function allConcrete(array $args): bool + { + foreach ($args as $a) { + if (!$a->isConcrete()) { + return false; + } + } + return true; + } +} diff --git a/tools/lsp/src/LspDispatcherFactory.php b/tools/lsp/src/LspDispatcherFactory.php index 9ffb6fef..42a72f5f 100644 --- a/tools/lsp/src/LspDispatcherFactory.php +++ b/tools/lsp/src/LspDispatcherFactory.php @@ -42,6 +42,7 @@ use XPHP\Lsp\Analyzer\Analyzer; use XPHP\Lsp\Analyzer\WorkspaceAnalyzer; use XPHP\Lsp\Diagnostics\XphpDiagnosticsProvider; +use XPHP\Lsp\Handler\XphpHoverHandler; use XPHP\Transpiler\Monomorphize\XphpSourceParser; /** @@ -75,9 +76,11 @@ public function create(MessageTransmitter $transmitter, InitializeParams $initia $clientApi = new ClientApi(new JsonRpcClient($transmitter, $responseWatcher)); $workspace = new PhpactorWorkspace($this->logger); + // Shared analyzer instance: stateless, so handlers can share it safely. + $analyzer = new Analyzer(new XphpSourceParser((new ParserFactory())->createForHostVersion())); $diagnosticsProvider = new XphpDiagnosticsProvider( - new Analyzer(new XphpSourceParser((new ParserFactory())->createForHostVersion())), + $analyzer, new WorkspaceAnalyzer(), $workspace, ); @@ -108,6 +111,7 @@ public function create(MessageTransmitter $transmitter, InitializeParams $initia new ServiceHandler($serviceManager, $clientApi), new CommandHandler(new CommandDispatcher([])), new ExitHandler(), + new XphpHoverHandler($workspace, $analyzer), ); $runner = new HandlerMethodRunner( diff --git a/tools/lsp/src/PositionMap.php b/tools/lsp/src/PositionMap.php index 0abdb2a6..1e57bb45 100644 --- a/tools/lsp/src/PositionMap.php +++ b/tools/lsp/src/PositionMap.php @@ -43,6 +43,61 @@ public static function lspLineFromNikic(int $nikicLine): int return max(0, $nikicLine - 1); } + /** + * Resolve an LSP {line, character} (both 0-based) to a byte offset for this + * document. Inverse of offsetToPosition(). Used by handlers that receive a + * cursor position from the client and need to walk the AST for the node + * sitting under it. + * + * If the line is past EOF, returns the document length. If the character is + * past EOL, returns the byte offset of the line's terminator (or document + * length for the last line). These clamps mirror typical editor behavior so + * a click just-past-the-end-of-line still resolves to the last token. + */ + public function positionToOffset(int $line, int $character): int + { + $length = strlen($this->source); + if ($line >= count($this->lineOffsets)) { + return $length; + } + $lineStart = $this->lineOffsets[$line]; + $nextLineStart = $this->lineOffsets[$line + 1] ?? $length + 1; + // The terminator (\n) sits at $nextLineStart - 1. Cap at that byte for + // every line except the last, where there's no terminator. + $lineEnd = ($line + 1 < count($this->lineOffsets)) ? $nextLineStart - 1 : $length; + $lineText = substr($this->source, $lineStart, $lineEnd - $lineStart); + // Walk characters until we've consumed `$character` of them — handles + // multibyte UTF-8 the same way offsetToPosition does (one Unicode char + // = one column in our MVP UTF-16 stand-in). + $consumed = 0; + $bytes = 0; + while ($consumed < $character && $bytes < strlen($lineText)) { + $bytes += self::utf8CharLength($lineText[$bytes]); + $consumed++; + } + return $lineStart + $bytes; + } + + private static function utf8CharLength(string $byte): int + { + $code = ord($byte); + if ($code < 0x80) { + return 1; + } + if (($code & 0xE0) === 0xC0) { + return 2; + } + if (($code & 0xF0) === 0xE0) { + return 3; + } + if (($code & 0xF8) === 0xF0) { + return 4; + } + // Invalid leading byte — treat as a single byte rather than throwing, + // so a malformed file doesn't blow up the hover handler. + return 1; + } + /** * Resolve a byte offset to {line, character} (both 0-based) for this document. * diff --git a/tools/lsp/test/Handler/AstPositionResolverTest.php b/tools/lsp/test/Handler/AstPositionResolverTest.php new file mode 100644 index 00000000..4210b335 --- /dev/null +++ b/tools/lsp/test/Handler/AstPositionResolverTest.php @@ -0,0 +1,85 @@ +parse($source); + + // Offset of 'B' in 'Box' on line 2. + $offset = strpos($source, 'Box'); + $hit = AstPositionResolver::nameAtOffset($ast, $offset); + + self::assertNotNull($hit); + self::assertSame('Box', $hit['name']->toString()); + } + + public function testReturnsNullWhenOffsetIsBetweenTokens(): void + { + $source = "parse($source); + + // Offset of the space immediately after `new ` (before extra whitespace). + $offset = strpos($source, ' new') + strlen(' new '); + $hit = AstPositionResolver::nameAtOffset($ast, $offset); + + self::assertNull($hit); + } + + public function testCapturesEnclosingClassLikeScopeForTypeParamLookups(): void + { + $source = <<<'XPHP' + + { + public T $item; + } + XPHP; + $ast = $this->parse($source); + + // Offset of the `T` in `public T $item;`. + $offset = strpos($source, 'public T') + strlen('public '); + $hit = AstPositionResolver::nameAtOffset($ast, $offset); + + self::assertNotNull($hit); + self::assertSame('T', $hit['name']->toString()); + self::assertCount(1, $hit['classScope']); + self::assertSame('Box', (string) $hit['classScope'][0]->name); + } + + public function testReturnsNullPastEndOfFile(): void + { + $source = "parse($source); + + $positionMap = new PositionMap($source); + // Past EOF. + $offset = $positionMap->positionToOffset(99, 99); + $hit = AstPositionResolver::nameAtOffset($ast, $offset); + + self::assertNull($hit); + } + + /** + * @return list<\PhpParser\Node\Stmt> + */ + private function parse(string $source): array + { + $parser = new XphpSourceParser((new ParserFactory())->createForHostVersion()); + $ast = $parser->parse($source); + self::assertNotNull($ast); + return $ast; + } +} diff --git a/tools/lsp/test/Handler/XphpHoverHandlerTest.php b/tools/lsp/test/Handler/XphpHoverHandlerTest.php new file mode 100644 index 00000000..d212e11d --- /dev/null +++ b/tools/lsp/test/Handler/XphpHoverHandlerTest.php @@ -0,0 +1,122 @@ +prepare(<<<'XPHP' + (); + XPHP); + // Cursor on the `B` of `Box`. + $hover = $this->hoverAt($handler, $uri, $workspace->get($uri)->text, 'Box'); + + self::assertInstanceOf(Hover::class, $hover); + self::assertInstanceOf(MarkupContent::class, $hover->contents); + self::assertStringContainsString('Specializes to:', $hover->contents->value); + self::assertStringContainsString('XPHP\\Generated\\App\\Box\\T_', $hover->contents->value); + } + + public function testHoverOverTypeParamShowsBoundAndOwner(): void + { + [$handler, $workspace, $uri] = $this->prepare(<<<'XPHP' + + { + public T $item; + } + XPHP); + // Cursor on the `T` inside `public T $item;`. + $hover = $this->hoverAt($handler, $uri, $workspace->get($uri)->text, 'public T $item', offsetInSearch: strlen('public ')); + + self::assertInstanceOf(Hover::class, $hover); + $text = $hover->contents->value; + self::assertStringContainsString('Type parameter', $text); + self::assertStringContainsString('`T`', $text); + self::assertStringContainsString('App\\Box', $text); + self::assertStringContainsString('Stringable', $text); + } + + public function testHoverOverPlainNameReturnsNull(): void + { + [$handler, $workspace, $uri] = $this->prepare(<<<'XPHP' + hoverAt($handler, $uri, $workspace->get($uri)->text, 'new Tag()', offsetInSearch: strlen('new ')); + + // `Tag` carries no ATTR_GENERIC_ARGS and no enclosing template scope. + // The handler returns null; LSP clients render nothing. + self::assertNull($hover); + } + + public function testUnknownUriYieldsNull(): void + { + $workspace = new PhpactorWorkspace(); + $handler = new XphpHoverHandler($workspace, $this->newAnalyzer()); + $params = new HoverParams( + new TextDocumentIdentifier('/never-opened.xphp'), + new Position(0, 0), + ); + self::assertNull(wait($handler->hover($params))); + } + + /** + * @return array{0: XphpHoverHandler, 1: PhpactorWorkspace, 2: string} + */ + private function prepare(string $source): array + { + $workspace = new PhpactorWorkspace(); + $uri = '/doc.xphp'; + $workspace->open(new TextDocumentItem($uri, 'xphp', 1, $source)); + $handler = new XphpHoverHandler($workspace, $this->newAnalyzer()); + return [$handler, $workspace, $uri]; + } + + private function hoverAt( + XphpHoverHandler $handler, + string $uri, + string $source, + string $search, + int $offsetInSearch = 0, + ): ?Hover { + $byte = strpos($source, $search); + self::assertNotFalse($byte, "fixture search string '{$search}' must exist in source"); + $byte += $offsetInSearch; + [$line, $character] = (new PositionMap($source))->offsetToPosition($byte); + $params = new HoverParams( + new TextDocumentIdentifier($uri), + new Position($line, $character), + ); + return wait($handler->hover($params)); + } + + private function newAnalyzer(): Analyzer + { + return new Analyzer(new XphpSourceParser((new ParserFactory())->createForHostVersion())); + } +} From 74c93c6ab178bcd47a3937f2fcf2e13607ec0284 Mon Sep 17 00:00:00 2001 From: Matheus Martins Date: Thu, 21 May 2026 20:06:56 +0000 Subject: [PATCH 05/17] lsp: implement textDocument/definition for generic instantiations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F12 on a generic instantiation (`new Box()`) now jumps to the template declaration (`class Box`) wherever it lives in the open workspace. Same machinery the hover handler uses: parse current document, find the Name at the cursor via AstPositionResolver, read its ATTR_TEMPLATE_FQN, walk every open document's AST for a ClassLike whose ATTR_TEMPLATE_FQN matches. Pieces: - `XphpDefinitionHandler` implements phpactor's Handler + CanRegisterCapabilities. Registers `definitionProvider = true` at initialize, dispatches `textDocument/definition` to a typed method, returns `Promise`. - Target range points at the class IDENTIFIER (`Box`), not the entire ClassLike body — so the jump lands the cursor on the name the user expects, not the opening line including modifiers. Limitations: - Templates that live on disk but aren't open in the editor don't resolve yet (the handler iterates phpactor's Workspace, which only knows about open documents). The Indexer that walks **/*.xphp at initialize lives in a follow-up; in the meantime the user has to open the template once before F12 works on it. - Bound names in template headers (the `\Stringable` in `class Box`) still aren't navigable — same root cause as the hover gap (XphpSourceParser strips the `<...>` clause so the AST has no Name positioned over the bound). Tests: - `XphpDefinitionHandlerTest`: cross-file template jump (Box.xphp <- Use.xphp), non-generic plain Name returns null, template not in open set returns null, unknown URI returns null. `make test/lsp` -> OK (31 tests, 92 assertions). Smoke-tested via stdio LSP transport: - initialize -> capabilities now include `definitionProvider: true` alongside hoverProvider. - didOpen Box.xphp + Use.xphp. - definition @ Use.xphp line 2 char 10 -> Location{uri: Box.xphp, range: line 2 char 6-9} = the `Box` identifier in `class Box`. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../lsp/src/Handler/XphpDefinitionHandler.php | 161 ++++++++++++++++++ tools/lsp/src/LspDispatcherFactory.php | 2 + .../Handler/XphpDefinitionHandlerTest.php | 125 ++++++++++++++ 3 files changed, 288 insertions(+) create mode 100644 tools/lsp/src/Handler/XphpDefinitionHandler.php create mode 100644 tools/lsp/test/Handler/XphpDefinitionHandlerTest.php diff --git a/tools/lsp/src/Handler/XphpDefinitionHandler.php b/tools/lsp/src/Handler/XphpDefinitionHandler.php new file mode 100644 index 00000000..5f4fd5fc --- /dev/null +++ b/tools/lsp/src/Handler/XphpDefinitionHandler.php @@ -0,0 +1,161 @@ +()`), + * walks the AST of every currently-open document to find the matching + * `ClassLike` template declaration and returns its source Location. + * + * The lookup is by `ATTR_TEMPLATE_FQN`: each parsed template carries the + * fully-qualified template name as an attribute on the ClassLike node, set by + * XphpSourceParser. We re-parse open documents on each request (cheap; the + * analyzer is already optimized for re-parse on change). A workspace-wide + * symbol cache is a follow-up alongside CompletionHandler. + * + * Returns null when: + * - the document isn't open + * - the cursor isn't on a Name node carrying ATTR_TEMPLATE_FQN + * - no open document defines a template with the matching FQN (the + * definition lives on disk but unopened, which we don't index yet) + */ +final class XphpDefinitionHandler implements Handler, CanRegisterCapabilities +{ + public function __construct( + private readonly PhpactorWorkspace $workspace, + private readonly Analyzer $analyzer, + ) { + } + + public function methods(): array + { + return [ + 'textDocument/definition' => 'definition', + ]; + } + + public function registerCapabiltiies(ServerCapabilities $capabilities): void + { + $capabilities->definitionProvider = true; + } + + /** + * @return Promise + */ + public function definition(DefinitionParams $params): Promise + { + if (!$this->workspace->has($params->textDocument->uri)) { + return new Success(null); + } + $currentItem = $this->workspace->get($params->textDocument->uri); + $currentResult = $this->analyzer->analyzeFile($currentItem->text); + if ($currentResult->ast === null) { + return new Success(null); + } + + $offset = (new PositionMap($currentItem->text))->positionToOffset( + $params->position->line, + $params->position->character, + ); + $hit = AstPositionResolver::nameAtOffset($currentResult->ast, $offset); + if ($hit === null) { + return new Success(null); + } + + $templateFqn = $hit['name']->getAttribute(XphpSourceParser::ATTR_TEMPLATE_FQN); + if (!is_string($templateFqn) || $templateFqn === '') { + return new Success(null); + } + + return new Success($this->findDefinitionAcrossWorkspace($templateFqn)); + } + + private function findDefinitionAcrossWorkspace(string $templateFqn): ?Location + { + foreach ($this->workspace as $uri => $item) { + $result = $this->analyzer->analyzeFile($item->text); + if ($result->ast === null) { + continue; + } + $found = self::findTemplateInAst($result->ast, $templateFqn); + if ($found === null) { + continue; + } + $positionMap = new PositionMap($item->text); + [$startLine, $startChar] = $positionMap->offsetToPosition($found['startOffset']); + [$endLine, $endChar] = $positionMap->offsetToPosition($found['endOffset']); + return new Location( + $uri, + new Range( + new Position($startLine, $startChar), + new Position($endLine, $endChar), + ), + ); + } + return null; + } + + /** + * @param list $ast + * @return array{startOffset: int, endOffset: int}|null + */ + private static function findTemplateInAst(array $ast, string $templateFqn): ?array + { + $visitor = new class($templateFqn) extends NodeVisitorAbstract { + public ?int $startOffset = null; + public ?int $endOffset = null; + + public function __construct(private readonly string $templateFqn) + { + } + + public function enterNode(Node $node): null + { + if (!$node instanceof ClassLike || $node->name === null) { + return null; + } + $fqn = $node->getAttribute(XphpSourceParser::ATTR_TEMPLATE_FQN); + if ($fqn !== $this->templateFqn) { + return null; + } + // Range targets the class NAME, not the entire ClassLike body — + // jumps land on the identifier the user expects, not the + // opening line including modifiers. + $this->startOffset = $node->name->getStartFilePos(); + $this->endOffset = $node->name->getEndFilePos() + 1; + return null; + } + }; + + $traverser = new NodeTraverser(); + $traverser->addVisitor($visitor); + $traverser->traverse($ast); + + if ($visitor->startOffset === null || $visitor->endOffset === null) { + return null; + } + return ['startOffset' => $visitor->startOffset, 'endOffset' => $visitor->endOffset]; + } +} diff --git a/tools/lsp/src/LspDispatcherFactory.php b/tools/lsp/src/LspDispatcherFactory.php index 42a72f5f..5a3c0562 100644 --- a/tools/lsp/src/LspDispatcherFactory.php +++ b/tools/lsp/src/LspDispatcherFactory.php @@ -42,6 +42,7 @@ use XPHP\Lsp\Analyzer\Analyzer; use XPHP\Lsp\Analyzer\WorkspaceAnalyzer; use XPHP\Lsp\Diagnostics\XphpDiagnosticsProvider; +use XPHP\Lsp\Handler\XphpDefinitionHandler; use XPHP\Lsp\Handler\XphpHoverHandler; use XPHP\Transpiler\Monomorphize\XphpSourceParser; @@ -112,6 +113,7 @@ public function create(MessageTransmitter $transmitter, InitializeParams $initia new CommandHandler(new CommandDispatcher([])), new ExitHandler(), new XphpHoverHandler($workspace, $analyzer), + new XphpDefinitionHandler($workspace, $analyzer), ); $runner = new HandlerMethodRunner( diff --git a/tools/lsp/test/Handler/XphpDefinitionHandlerTest.php b/tools/lsp/test/Handler/XphpDefinitionHandlerTest.php new file mode 100644 index 00000000..24c40d82 --- /dev/null +++ b/tools/lsp/test/Handler/XphpDefinitionHandlerTest.php @@ -0,0 +1,125 @@ + + { + public T $item; + } + XPHP; + $useSource = <<<'XPHP' + (); + XPHP; + $workspace->open(new TextDocumentItem('/Box.xphp', 'xphp', 1, $boxSource)); + $workspace->open(new TextDocumentItem('/Use.xphp', 'xphp', 1, $useSource)); + + $handler = $this->newHandler($workspace); + $location = $this->definitionAt($handler, '/Use.xphp', $useSource, 'Box'); + + self::assertInstanceOf(Location::class, $location); + self::assertSame('/Box.xphp', $location->uri); + // The target range points at the `Box` identifier inside `class Box`, + // not the entire class body. + [$expectedLine, $expectedChar] = (new PositionMap($boxSource))->offsetToPosition(strpos($boxSource, 'class Box') + strlen('class ')); + self::assertSame($expectedLine, $location->range->start->line); + self::assertSame($expectedChar, $location->range->start->character); + } + + public function testNonGenericNameReturnsNull(): void + { + $workspace = new PhpactorWorkspace(); + $source = <<<'XPHP' + open(new TextDocumentItem('/doc.xphp', 'xphp', 1, $source)); + + $handler = $this->newHandler($workspace); + // Cursor on `Tag` in `new Tag()` — but `Tag` here has no template + // FQN attribute since it's not a generic instantiation. Returns null. + $location = $this->definitionAt($handler, '/doc.xphp', $source, 'new Tag', offsetInSearch: strlen('new ')); + self::assertNull($location); + } + + public function testTemplateNotInOpenWorkspaceReturnsNull(): void + { + // Only the Use.xphp is open; the Box template lives elsewhere (on disk). + // Without a workspace index of unopened files (a follow-up), we return null. + $workspace = new PhpactorWorkspace(); + $useSource = <<<'XPHP' + (); + XPHP; + $workspace->open(new TextDocumentItem('/Use.xphp', 'xphp', 1, $useSource)); + + $handler = $this->newHandler($workspace); + $location = $this->definitionAt($handler, '/Use.xphp', $useSource, 'Box'); + + self::assertNull($location); + } + + public function testUnknownUriReturnsNull(): void + { + $handler = $this->newHandler(new PhpactorWorkspace()); + $params = new DefinitionParams( + new TextDocumentIdentifier('/never-opened.xphp'), + new Position(0, 0), + ); + self::assertNull(wait($handler->definition($params))); + } + + private function definitionAt( + XphpDefinitionHandler $handler, + string $uri, + string $source, + string $search, + int $offsetInSearch = 0, + ): ?Location { + $byte = strpos($source, $search); + self::assertNotFalse($byte, "fixture search string '{$search}' must exist in source"); + $byte += $offsetInSearch; + [$line, $character] = (new PositionMap($source))->offsetToPosition($byte); + $params = new DefinitionParams( + new TextDocumentIdentifier($uri), + new Position($line, $character), + ); + return wait($handler->definition($params)); + } + + private function newHandler(PhpactorWorkspace $workspace): XphpDefinitionHandler + { + return new XphpDefinitionHandler( + $workspace, + new Analyzer(new XphpSourceParser((new ParserFactory())->createForHostVersion())), + ); + } +} From 49ddf58330dcbbdfc599c68f7e2fb3111e75ad86 Mon Sep 17 00:00:00 2001 From: Matheus Martins Date: Thu, 21 May 2026 20:11:00 +0000 Subject: [PATCH 06/17] lsp: implement textDocument/completion inside type-arg positions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cursor inside a `<…>` type-arg clause now suggests workspace classes + xphp scalars. Outside that context, the handler returns an empty list so the editor falls back to its default (PHP) completion source. Pieces: - `TypeArgPositionDetector` — pure backwards-scanner over source bytes with a `<>` depth counter. Decides whether the cursor sits inside an unmatched `<…>` clause AND captures the partial identifier the user has already typed (so the handler can filter candidates by prefix). Handles nested generics correctly: `Box, |` finds the OUTER `<` at depth 0, not the inner one. Rejects less-than operators (`if ($a < |`). - `WorkspaceSymbols` — walks every open document's AST for ClassLike declarations and collects their FQNs. Re-computed per call; caching is a follow-up once profiling shows it matters. - `XphpCompletionHandler` — wires the two above. Returns CompletionList of CompletionItems: every ClassLike FQN that matches the prefix (insertText = full FQN; label = short name; detail = FQN) plus the xphp scalar set from `XphpSourceParser::SCALAR_TYPES`. Trigger characters `<` and `,` registered at initialize. Limitations called out in the code: - No bound-aware filtering: `Box` still suggests non-Stringable classes. The user picks; the diagnostic surface flags the violation. Bound-aware completion is a follow-up that needs the handler to resolve the enclosing Name's template definition first. - No use-alias short form: always inserts the full FQN. Always correct; a future refinement could substitute the short name when a matching `use` statement is in scope. - No string/comment awareness in the position detector — false positives inside strings just suggest classes the user ignores. Tests: - `TypeArgPositionDetectorTest`: 9 cases covering `Box<`, prefix filtering, multi-arg comma position, nested generics, less-than operator rejection, closed-bracket-past rejection, FQN-style prefix. - `XphpCompletionHandlerTest`: cross-file class suggestion, prefix filtering, outside-type-arg empty list, unknown URI empty list. `make test/lsp` -> OK (44 tests, 113 assertions). Smoke-tested via stdio LSP: - initialize -> capabilities now include `completionProvider: {triggerCharacters: ["<", ","]}`. - didOpen Models.xphp + Use.xphp. - completion @ Use.xphp line 2 char 16 (cursor after "Box items: [{label: "Plastic", kind: 7 (CLASS_), detail: "App\\Models\\Plastic", insertText: "App\\Models\\Plastic"}] Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/Handler/TypeArgPositionDetector.php | 98 ++++++++++++ tools/lsp/src/Handler/WorkspaceSymbols.php | 82 ++++++++++ .../lsp/src/Handler/XphpCompletionHandler.php | 143 ++++++++++++++++++ tools/lsp/src/LspDispatcherFactory.php | 3 + .../Handler/TypeArgPositionDetectorTest.php | 77 ++++++++++ .../Handler/XphpCompletionHandlerTest.php | 125 +++++++++++++++ 6 files changed, 528 insertions(+) create mode 100644 tools/lsp/src/Handler/TypeArgPositionDetector.php create mode 100644 tools/lsp/src/Handler/WorkspaceSymbols.php create mode 100644 tools/lsp/src/Handler/XphpCompletionHandler.php create mode 100644 tools/lsp/test/Handler/TypeArgPositionDetectorTest.php create mode 100644 tools/lsp/test/Handler/XphpCompletionHandlerTest.php diff --git a/tools/lsp/src/Handler/TypeArgPositionDetector.php b/tools/lsp/src/Handler/TypeArgPositionDetector.php new file mode 100644 index 00000000..5d04b4b6 --- /dev/null +++ b/tools/lsp/src/Handler/TypeArgPositionDetector.php @@ -0,0 +1,98 @@ +` that follows a Name. + * + * Strategy: walk the source backwards from the cursor with a `<>` depth + * counter. Decrement on `>`, increment on `<`. If the depth ever reaches +1 on + * a `<` and the byte immediately before that `<` is an identifier byte, the + * cursor is in a type-arg position relative to that Name. + * + * Single-pass, no string/comment awareness — that's fine because xphp generic + * syntax doesn't appear inside strings or comments (the parser-level scanner + * already handles those for parsing; for completion, false positives in + * strings just suggest classes that the user will ignore). + * + * Limits intentionally accepted: + * - Doesn't bind the surrounding Name to the candidate filter (so we can't + * yet prune by "bounds declared on T of Box"). That requires AST work + * and lands alongside bound-aware completion later. + */ +final readonly class TypeArgPositionDetector +{ + /** + * @return array{prefix: string}|null + * null → cursor is not in a type-arg position + * array → cursor IS in a type-arg position; `prefix` is the substring the + * user has typed since the last `<` or `,` (post-whitespace), + * used to filter completion candidates. + */ + public static function detect(string $source, int $offset): ?array + { + $length = strlen($source); + if ($offset > $length) { + return null; + } + // Pull the partial identifier under the cursor out — that's the prefix + // the user has already typed since the last `<` or `,`. Identifier + // bytes include backslashes (so `App\Pla|` is a single FQN-style + // prefix). + $prefixStart = $offset; + while ($prefixStart > 0 && self::isIdentifierByte($source[$prefixStart - 1])) { + $prefixStart--; + } + $prefix = substr($source, $prefixStart, $offset - $prefixStart); + + // Walk back from the prefix start with a `<>` depth counter. We're + // looking for the FIRST `<` at depth 0 (i.e. an unmatched opener). + $depth = 0; + $i = $prefixStart - 1; + while ($i >= 0) { + $c = $source[$i]; + if ($c === '>') { + $depth++; + $i--; + continue; + } + if ($c === '<') { + if ($depth === 0) { + // Found the unmatched opener. The byte before it must be + // an identifier byte (the generic Name) — otherwise it's + // a less-than operator. + $j = $i - 1; + if ($j < 0 || !self::isIdentifierByte($source[$j])) { + return null; + } + return ['prefix' => $prefix]; + } + $depth--; + $i--; + continue; + } + if (self::isInterArgByte($c) || self::isIdentifierByte($c)) { + $i--; + continue; + } + // Anything else (`(`, `;`, `=`, `{`, …) breaks the type-arg + // context — we're not inside a `<…>` clause. + return null; + } + return null; + } + + private static function isIdentifierByte(string $byte): bool + { + return ctype_alnum($byte) || $byte === '_' || $byte === '\\'; + } + + private static function isInterArgByte(string $byte): bool + { + // Whitespace + commas separate args; both are legal inside `<…>`. + return $byte === ' ' || $byte === "\t" || $byte === "\n" || $byte === "\r" || $byte === ','; + } +} diff --git a/tools/lsp/src/Handler/WorkspaceSymbols.php b/tools/lsp/src/Handler/WorkspaceSymbols.php new file mode 100644 index 00000000..6e788ed7 --- /dev/null +++ b/tools/lsp/src/Handler/WorkspaceSymbols.php @@ -0,0 +1,82 @@ +` type-arg positions. + * + * Re-computed on each call for the MVP — cheap because the analyzer is fast + * and the open workspace is typically small. Caching keyed on doc version is + * a follow-up if profiling shows it matters. + */ +final readonly class WorkspaceSymbols +{ + public function __construct( + private PhpactorWorkspace $workspace, + private Analyzer $analyzer, + ) { + } + + /** + * @return list Fully-qualified ClassLike names across the open workspace. + */ + public function allClassFqns(): array + { + $fqns = []; + foreach ($this->workspace as $item) { + $result = $this->analyzer->analyzeFile($item->text); + if ($result->ast === null) { + continue; + } + foreach (self::collectFqns($result->ast) as $fqn) { + $fqns[$fqn] = true; + } + } + return array_keys($fqns); + } + + /** + * @param list $ast + * @return list + */ + private static function collectFqns(array $ast): array + { + $visitor = new class extends NodeVisitorAbstract { + /** @var list */ + public array $fqns = []; + + private string $currentNamespace = ''; + + public function enterNode(Node $node): null + { + if ($node instanceof Namespace_) { + $this->currentNamespace = $node->name?->toString() ?? ''; + } + if ($node instanceof ClassLike && $node->name !== null) { + $short = $node->name->toString(); + $this->fqns[] = $this->currentNamespace !== '' + ? $this->currentNamespace . '\\' . $short + : $short; + } + return null; + } + }; + + $traverser = new NodeTraverser(); + $traverser->addVisitor($visitor); + $traverser->traverse($ast); + return $visitor->fqns; + } +} diff --git a/tools/lsp/src/Handler/XphpCompletionHandler.php b/tools/lsp/src/Handler/XphpCompletionHandler.php new file mode 100644 index 00000000..3ecfc12e --- /dev/null +++ b/tools/lsp/src/Handler/XphpCompletionHandler.php @@ -0,0 +1,143 @@ +`, we still suggest + * non-Stringable classes; the diagnostic surface will flag the violation + * after the user picks. Bound-aware completion is a follow-up that + * requires resolving the enclosing Name's template definition first. + * - No use-alias short-form yet. We always insert the full FQN, which is + * always correct; a future refinement could substitute the short form + * when a matching `use` statement is in scope. + */ +final class XphpCompletionHandler implements Handler, CanRegisterCapabilities +{ + public function __construct( + private readonly PhpactorWorkspace $workspace, + private readonly WorkspaceSymbols $symbols, + ) { + } + + public function methods(): array + { + return [ + 'textDocument/completion' => 'complete', + ]; + } + + public function registerCapabiltiies(ServerCapabilities $capabilities): void + { + // `<` is the canonical trigger; `,` lets the next-arg case fire without + // the user typing an extra letter first. + $capabilities->completionProvider = new CompletionOptions( + triggerCharacters: ['<', ','], + ); + } + + /** + * @return Promise + */ + public function complete(CompletionParams $params): Promise + { + $emptyList = new CompletionList(isIncomplete: false, items: []); + if (!$this->workspace->has($params->textDocument->uri)) { + return new Success($emptyList); + } + $item = $this->workspace->get($params->textDocument->uri); + $offset = (new PositionMap($item->text))->positionToOffset( + $params->position->line, + $params->position->character, + ); + + $hit = TypeArgPositionDetector::detect($item->text, $offset); + if ($hit === null) { + return new Success($emptyList); + } + + $candidates = $this->buildCandidates($hit['prefix']); + return new Success(new CompletionList(isIncomplete: false, items: $candidates)); + } + + /** + * @return list + */ + private function buildCandidates(string $prefix): array + { + $items = []; + + foreach ($this->symbols->allClassFqns() as $fqn) { + $shortName = self::lastSegment($fqn); + if (!self::matchesPrefix($shortName, $fqn, $prefix)) { + continue; + } + $items[] = new CompletionItem( + label: $shortName, + kind: CompletionItemKind::CLASS_, + detail: $fqn, + insertText: $fqn, + ); + } + + foreach (XphpSourceParser::SCALAR_TYPES as $scalar) { + if ($prefix !== '' && !self::matchesPrefix($scalar, $scalar, $prefix)) { + continue; + } + $items[] = new CompletionItem( + label: $scalar, + kind: CompletionItemKind::KEYWORD, + ); + } + + return $items; + } + + private static function matchesPrefix(string $shortName, string $fqn, string $prefix): bool + { + if ($prefix === '') { + return true; + } + $needle = ltrim($prefix, '\\'); + if ($needle === '') { + return true; + } + return stripos($shortName, $needle) === 0 || stripos($fqn, $needle) !== false; + } + + private static function lastSegment(string $fqn): string + { + $pos = strrpos($fqn, '\\'); + return $pos === false ? $fqn : substr($fqn, $pos + 1); + } +} diff --git a/tools/lsp/src/LspDispatcherFactory.php b/tools/lsp/src/LspDispatcherFactory.php index 5a3c0562..54526404 100644 --- a/tools/lsp/src/LspDispatcherFactory.php +++ b/tools/lsp/src/LspDispatcherFactory.php @@ -42,6 +42,8 @@ use XPHP\Lsp\Analyzer\Analyzer; use XPHP\Lsp\Analyzer\WorkspaceAnalyzer; use XPHP\Lsp\Diagnostics\XphpDiagnosticsProvider; +use XPHP\Lsp\Handler\WorkspaceSymbols; +use XPHP\Lsp\Handler\XphpCompletionHandler; use XPHP\Lsp\Handler\XphpDefinitionHandler; use XPHP\Lsp\Handler\XphpHoverHandler; use XPHP\Transpiler\Monomorphize\XphpSourceParser; @@ -114,6 +116,7 @@ public function create(MessageTransmitter $transmitter, InitializeParams $initia new ExitHandler(), new XphpHoverHandler($workspace, $analyzer), new XphpDefinitionHandler($workspace, $analyzer), + new XphpCompletionHandler($workspace, new WorkspaceSymbols($workspace, $analyzer)), ); $runner = new HandlerMethodRunner( diff --git a/tools/lsp/test/Handler/TypeArgPositionDetectorTest.php b/tools/lsp/test/Handler/TypeArgPositionDetectorTest.php new file mode 100644 index 00000000..4bcb19da --- /dev/null +++ b/tools/lsp/test/Handler/TypeArgPositionDetectorTest.php @@ -0,0 +1,77 @@ + ''], $hit); + } + + public function testDetectsWithPartialIdentifierPrefix(): void + { + $source = 'new Box 'Pla'], $hit); + } + + public function testDetectsAfterCommaInMultiArgList(): void + { + $source = 'new Pair ''], $hit); + } + + public function testDetectsInsideNestedGenericsAtSameDepth(): void + { + $source = 'new Box, '; + $hit = TypeArgPositionDetector::detect($source, strlen($source)); + self::assertSame(['prefix' => ''], $hit); + } + + public function testRejectsLessThanOperator(): void + { + $source = 'if ($a < '; + $hit = TypeArgPositionDetector::detect($source, strlen($source)); + self::assertNull($hit); + } + + public function testRejectsOutsideAnyTypeArgClause(): void + { + $source = '$x = new Box('; + $hit = TypeArgPositionDetector::detect($source, strlen($source)); + self::assertNull($hit); + } + + public function testRejectsAfterClosingBracket(): void + { + $source = 'new Box '; + $hit = TypeArgPositionDetector::detect($source, strlen($source)); + self::assertNull($hit, 'cursor past the closing `>` is no longer in type-arg context'); + } + + public function testAcceptsFqnStylePrefix(): void + { + // Backslashes are part of the identifier so an FQN prefix matches as one token. + $source = 'new Box`, prefix is the partial identifier just typed. + $source = 'new Box 'Pla'], $hit); + } +} diff --git a/tools/lsp/test/Handler/XphpCompletionHandlerTest.php b/tools/lsp/test/Handler/XphpCompletionHandlerTest.php new file mode 100644 index 00000000..1174ab27 --- /dev/null +++ b/tools/lsp/test/Handler/XphpCompletionHandlerTest.php @@ -0,0 +1,125 @@ +open(new TextDocumentItem('/Models.xphp', 'xphp', 1, <<<'XPHP' + open(new TextDocumentItem('/Use.xphp', 'xphp', 1, $useSource)); + + $list = $this->complete($workspace, '/Use.xphp', $useSource, strlen($useSource)); + + $labels = array_map(static fn (CompletionItem $i): string => $i->label, $list->items); + self::assertContains('Plastic', $labels); + self::assertContains('Metal', $labels); + self::assertContains('int', $labels, 'scalar types must also be suggested'); + // Class items insertText carries the FQN — easier for the user to land + // a correct instantiation when no `use` is in scope. + foreach ($list->items as $item) { + if ($item->label === 'Plastic') { + self::assertSame('App\\Models\\Plastic', $item->insertText); + self::assertSame(CompletionItemKind::CLASS_, $item->kind); + } + } + } + + public function testFiltersByPrefix(): void + { + $workspace = new PhpactorWorkspace(); + $workspace->open(new TextDocumentItem('/Models.xphp', 'xphp', 1, <<<'XPHP' + open(new TextDocumentItem('/Use.xphp', 'xphp', 1, $useSource)); + + $list = $this->complete($workspace, '/Use.xphp', $useSource, strlen($useSource)); + + $labels = array_map(static fn (CompletionItem $i): string => $i->label, $list->items); + self::assertContains('Plastic', $labels); + self::assertNotContains('Metal', $labels, 'Pla prefix must exclude Metal'); + self::assertNotContains('Wood', $labels); + } + + public function testReturnsEmptyListOutsideTypeArgPosition(): void + { + $workspace = new PhpactorWorkspace(); + $source = "open(new TextDocumentItem('/Doc.xphp', 'xphp', 1, $source)); + + $list = $this->complete($workspace, '/Doc.xphp', $source, strlen($source)); + self::assertSame([], $list->items); + } + + public function testUnknownUriYieldsEmptyList(): void + { + $workspace = new PhpactorWorkspace(); + $handler = new XphpCompletionHandler( + $workspace, + new WorkspaceSymbols($workspace, $this->newAnalyzer()), + ); + $params = new CompletionParams( + new TextDocumentIdentifier('/never-opened.xphp'), + new Position(0, 0), + ); + $list = wait($handler->complete($params)); + self::assertInstanceOf(CompletionList::class, $list); + self::assertSame([], $list->items); + } + + private function complete( + PhpactorWorkspace $workspace, + string $uri, + string $source, + int $byteOffset, + ): CompletionList { + [$line, $character] = (new PositionMap($source))->offsetToPosition($byteOffset); + $params = new CompletionParams( + new TextDocumentIdentifier($uri), + new Position($line, $character), + ); + $analyzer = $this->newAnalyzer(); + $handler = new XphpCompletionHandler( + $workspace, + new WorkspaceSymbols($workspace, $analyzer), + ); + return wait($handler->complete($params)); + } + + private function newAnalyzer(): Analyzer + { + return new Analyzer(new XphpSourceParser((new ParserFactory())->createForHostVersion())); + } +} From b6b92f042cb273264b896d6cb159b56cd5e523b5 Mon Sep 17 00:00:00 2001 From: Matheus Martins Date: Thu, 21 May 2026 20:14:27 +0000 Subject: [PATCH 07/17] lsp: VS Code extension client at tools/lsp/vscode-extension/ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Minimal but functional client that spawns the PHP-based xphp-lsp server over stdio. F5 from the extension directory launches an Extension Development Host with the repo open as the workspace — diagnostics, hover, go-to-definition, and completion all work against playground/*. Pieces: - `package.json` — extension manifest. Declares `xphp` language, `.xphp` file extension, two `xphp.serverPath` / `xphp.phpPath` settings for non-default setups. Activates on `onLanguage:xphp`. - `src/extension.ts` — ~80 lines using `vscode-languageclient/node`. Resolves the server binary (configured path > sibling-of-extension path > workspace `tools/lsp/bin/xphp-lsp`), spawns it via `php`, pipes stderr to an output channel for debugging. - `syntaxes/xphp.tmLanguage.json` — minimal grammar fork. Includes `source.php` for the bulk of the highlighting and adds an `xphp-extras` pattern that recognizes `Name<...>` generic clauses so angle brackets and bound colons get sensible scope names. - `language-configuration.json` — comments, brackets, surrounding pairs (including `<...>`). - `.vscode/launch.json` + `.vscode/tasks.json` — F5 dev workflow boilerplate. Pre-task compiles TypeScript; the Extension Host opens the repo root as the workspace. - `tsconfig.json` — TypeScript config (es2022, strict, node16). Makefile gains `build/lsp-extension` (npm install + tsc) so the extension build is reachable from the same place as `test/lsp`. Replaces the placeholder `vscode-extension/README.md` with real instructions covering install, dev workflow, configuration, and the fixtures under `playground/src/` worth opening to exercise each feature. Out of scope: - Marketplace publication (`vsce package` / `vsce publish`). Lives behind a stabilisation milestone — the surface needs a couple of iterations against the playground first. - Headless extension tests via @vscode/test-electron. The Makefile targets keep the dev loop fast for now; CI gating on extension integration is a follow-up. Verified locally: - `make build/lsp-extension` -> compiles clean (tsc strict, no warnings). - `out/extension.js` produced; module exports `activate` + `deactivate` for VS Code's lifecycle. Co-Authored-By: Claude Opus 4.7 (1M context) --- Makefile | 4 + tools/lsp/vscode-extension/.gitignore | 5 + .../lsp/vscode-extension/.vscode/launch.json | 19 ++++ tools/lsp/vscode-extension/.vscode/tasks.json | 17 +++ tools/lsp/vscode-extension/README.md | 67 ++++++++---- .../language-configuration.json | 26 +++++ tools/lsp/vscode-extension/package.json | 72 +++++++++++++ tools/lsp/vscode-extension/src/extension.ts | 101 ++++++++++++++++++ .../syntaxes/xphp.tmLanguage.json | 45 ++++++++ tools/lsp/vscode-extension/tsconfig.json | 20 ++++ 10 files changed, 357 insertions(+), 19 deletions(-) create mode 100644 tools/lsp/vscode-extension/.gitignore create mode 100644 tools/lsp/vscode-extension/.vscode/launch.json create mode 100644 tools/lsp/vscode-extension/.vscode/tasks.json create mode 100644 tools/lsp/vscode-extension/language-configuration.json create mode 100644 tools/lsp/vscode-extension/package.json create mode 100644 tools/lsp/vscode-extension/src/extension.ts create mode 100644 tools/lsp/vscode-extension/syntaxes/xphp.tmLanguage.json create mode 100644 tools/lsp/vscode-extension/tsconfig.json diff --git a/Makefile b/Makefile index 8d33c9fc..685ad39b 100644 --- a/Makefile +++ b/Makefile @@ -10,6 +10,10 @@ test/mutation: test/lsp: cd tools/lsp && composer install --quiet && php -d error_reporting='E_ALL & ~E_DEPRECATED' vendor/bin/phpunit +.PHONY: build/lsp-extension +build/lsp-extension: + cd tools/lsp/vscode-extension && npm install --no-audit --no-fund --silent && npm run compile + .PHONY: playground playground: playground/bin/run diff --git a/tools/lsp/vscode-extension/.gitignore b/tools/lsp/vscode-extension/.gitignore new file mode 100644 index 00000000..76ff46ae --- /dev/null +++ b/tools/lsp/vscode-extension/.gitignore @@ -0,0 +1,5 @@ +/node_modules/ +/out/ +/.vscode-test/ +*.vsix +package-lock.json diff --git a/tools/lsp/vscode-extension/.vscode/launch.json b/tools/lsp/vscode-extension/.vscode/launch.json new file mode 100644 index 00000000..ac3c0a29 --- /dev/null +++ b/tools/lsp/vscode-extension/.vscode/launch.json @@ -0,0 +1,19 @@ +{ + "version": "0.2.0", + "configurations": [ + { + "name": "Run Extension", + "type": "extensionHost", + "request": "launch", + "runtimeExecutable": "${execPath}", + "args": [ + "--extensionDevelopmentPath=${workspaceFolder}", + "${workspaceFolder}/../../.." + ], + "outFiles": [ + "${workspaceFolder}/out/**/*.js" + ], + "preLaunchTask": "npm: compile" + } + ] +} diff --git a/tools/lsp/vscode-extension/.vscode/tasks.json b/tools/lsp/vscode-extension/.vscode/tasks.json new file mode 100644 index 00000000..fb49166a --- /dev/null +++ b/tools/lsp/vscode-extension/.vscode/tasks.json @@ -0,0 +1,17 @@ +{ + "version": "2.0.0", + "tasks": [ + { + "type": "npm", + "script": "compile", + "group": { + "kind": "build", + "isDefault": true + }, + "problemMatcher": [ + "$tsc" + ], + "label": "npm: compile" + } + ] +} diff --git a/tools/lsp/vscode-extension/README.md b/tools/lsp/vscode-extension/README.md index 0d932947..6f9147fe 100644 --- a/tools/lsp/vscode-extension/README.md +++ b/tools/lsp/vscode-extension/README.md @@ -1,35 +1,64 @@ -# xphp VS Code extension +# xphp Language extension for VS Code -Minimal client that wires VS Code to the xphp Language Server (`../bin/xphp-lsp`). -Provides `.xphp` file association, syntax highlighting (PHP grammar with light -`` adjustments), and LSP transport over stdio. +VS Code client for the [xphp Language Server](../README.md). Provides +`.xphp` file association, light syntax highlighting layered on top of the +bundled PHP grammar, and live LSP features: -## Status +- **Diagnostics** — syntax errors, bound violations, duplicate templates. +- **Hover** — `Box` shows the specialized FQN; `T` shows its bound + and owning template. +- **Go to definition** — F12 on a generic instantiation jumps to the + template declaration. +- **Completion** — `<` inside a type-arg position suggests workspace + classes plus xphp scalars. -Skeleton stub — not yet wired. The `package.json`, `extension.ts`, and the -TextMate grammar fork land in the dedicated VS-Code-extension commit (see the -`feat/lsp` branch plan, step 8). +The server is `tools/lsp/bin/xphp-lsp` and runs in PHP. The extension +spawns it over stdio. -Until then, the LSP server is usable from the command line via: +## Local development (F5 workflow) -```bash -tools/lsp/bin/xphp-lsp --lint path/to/file.xphp -``` +From the repo root, ensure the server is installed: -## Build / debug (planned) +```sh +make test/lsp # composer install inside tools/lsp + run the suite +``` -Once `package.json` lands: +Then in the extension directory: -```bash +```sh cd tools/lsp/vscode-extension npm install npm run compile ``` -Then in VS Code, open the `tools/lsp/vscode-extension/` folder and hit **F5** to -launch an Extension Development Host window with the extension loaded. +Open `tools/lsp/vscode-extension/` in VS Code and press **F5**. A new +Extension Development Host window opens with the extension loaded; the +host opens the repo root as its workspace so `.xphp` files under +`playground/src/` are immediately available to test. + +Useful files to open in the host: + +- `playground/src/Containers/Box.xphp` — generic class template; hover + on `T` shows the type-param info. +- `playground/src/Demos/GenericInterface.xphp` — uses `Box`; + F12 jumps to the template. +- `playground/src/Demos/Bounds.xphp` — exercises `StringableBox`; + editing the type arg to `int` should surface a red squiggle. + +## Configuration + +| Setting | Default | Purpose | +|------------------|---------|---------| +| `xphp.serverPath` | `""` | Absolute path to `xphp-lsp`. If empty, the extension looks first beside its own install dir, then in the workspace at `tools/lsp/bin/xphp-lsp`. | +| `xphp.phpPath` | `"php"` | PHP binary used to run the server. Override if PHP isn't on `PATH`. | + +## Server logs + +The "xphp Language Server" output channel (View → Output) carries the +extension's own messages plus everything the server writes to stderr. ## Publishing -Out of scope for the initial branch. Marketplace publication happens after the -core features stabilise. +Out of scope for this branch. The MVP runs from the F5 dev workflow only; +marketplace packaging via `vsce package` lands when the feature surface +stabilises. diff --git a/tools/lsp/vscode-extension/language-configuration.json b/tools/lsp/vscode-extension/language-configuration.json new file mode 100644 index 00000000..6460d009 --- /dev/null +++ b/tools/lsp/vscode-extension/language-configuration.json @@ -0,0 +1,26 @@ +{ + "comments": { + "lineComment": "//", + "blockComment": ["/*", "*/"] + }, + "brackets": [ + ["{", "}"], + ["[", "]"], + ["(", ")"] + ], + "autoClosingPairs": [ + { "open": "{", "close": "}" }, + { "open": "[", "close": "]" }, + { "open": "(", "close": ")" }, + { "open": "\"", "close": "\"", "notIn": ["string"] }, + { "open": "'", "close": "'", "notIn": ["string", "comment"] } + ], + "surroundingPairs": [ + ["{", "}"], + ["[", "]"], + ["(", ")"], + ["\"", "\""], + ["'", "'"], + ["<", ">"] + ] +} diff --git a/tools/lsp/vscode-extension/package.json b/tools/lsp/vscode-extension/package.json new file mode 100644 index 00000000..a205f348 --- /dev/null +++ b/tools/lsp/vscode-extension/package.json @@ -0,0 +1,72 @@ +{ + "name": "xphp-language", + "displayName": "xphp Language", + "description": "Language support for xphp: diagnostics, hover, go-to-definition, and completion for .xphp files via the xphp-lsp server.", + "version": "0.1.0", + "publisher": "xphp-lang", + "license": "MIT", + "repository": { + "type": "git", + "url": "https://github.com/xphp-lang/xphp.git" + }, + "engines": { + "vscode": "^1.85.0" + }, + "categories": [ + "Programming Languages" + ], + "activationEvents": [ + "onLanguage:xphp" + ], + "main": "./out/extension.js", + "contributes": { + "languages": [ + { + "id": "xphp", + "aliases": [ + "xphp", + "XPHP" + ], + "extensions": [ + ".xphp" + ], + "configuration": "./language-configuration.json" + } + ], + "grammars": [ + { + "language": "xphp", + "scopeName": "source.xphp", + "path": "./syntaxes/xphp.tmLanguage.json" + } + ], + "configuration": { + "title": "xphp", + "properties": { + "xphp.serverPath": { + "type": "string", + "default": "", + "description": "Absolute path to the xphp-lsp binary. If empty, defaults to /tools/lsp/bin/xphp-lsp." + }, + "xphp.phpPath": { + "type": "string", + "default": "php", + "description": "PHP binary used to run xphp-lsp. Override if your PHP isn't on PATH or you need a specific version." + } + } + } + }, + "scripts": { + "compile": "tsc -p ./", + "watch": "tsc -watch -p ./", + "vscode:prepublish": "npm run compile" + }, + "dependencies": { + "vscode-languageclient": "^9.0.1" + }, + "devDependencies": { + "@types/node": "^22.0.0", + "@types/vscode": "^1.85.0", + "typescript": "^5.4.0" + } +} diff --git a/tools/lsp/vscode-extension/src/extension.ts b/tools/lsp/vscode-extension/src/extension.ts new file mode 100644 index 00000000..c18de83c --- /dev/null +++ b/tools/lsp/vscode-extension/src/extension.ts @@ -0,0 +1,101 @@ +import * as path from "node:path"; +import * as fs from "node:fs"; +import { + ExtensionContext, + workspace, + window, + OutputChannel, +} from "vscode"; +import { + LanguageClient, + LanguageClientOptions, + ServerOptions, + TransportKind, +} from "vscode-languageclient/node"; + +let client: LanguageClient | undefined; + +export async function activate(context: ExtensionContext): Promise { + const output: OutputChannel = window.createOutputChannel("xphp Language Server"); + context.subscriptions.push(output); + + const serverPath = resolveServerPath(output); + if (!serverPath) { + output.appendLine( + "xphp-lsp binary not found. Configure xphp.serverPath in settings or place the extension alongside tools/lsp/bin/xphp-lsp." + ); + return; + } + + const phpPath = workspace.getConfiguration("xphp").get("phpPath", "php"); + + // PHP runs the LSP server over stdio; vscode-languageclient handles the + // JSON-RPC framing. TransportKind.stdio is the default and matches + // LanguageServerBuilder's default in the PHP server. + const serverOptions: ServerOptions = { + command: phpPath, + args: [serverPath], + transport: TransportKind.stdio, + }; + + const clientOptions: LanguageClientOptions = { + documentSelector: [ + { scheme: "file", language: "xphp" }, + ], + // Surface server stderr to the output channel for debugging — phpactor + // logs go here too, plus any deprecation noise the server didn't mute. + outputChannel: output, + }; + + client = new LanguageClient( + "xphp", + "xphp Language Server", + serverOptions, + clientOptions, + ); + + try { + await client.start(); + output.appendLine(`xphp-lsp started: ${phpPath} ${serverPath}`); + } catch (err) { + output.appendLine(`xphp-lsp failed to start: ${String(err)}`); + window.showErrorMessage( + "xphp-lsp failed to start; see the 'xphp Language Server' output channel for details." + ); + } +} + +export async function deactivate(): Promise { + if (client) { + await client.stop(); + client = undefined; + } +} + +function resolveServerPath(output: OutputChannel): string | undefined { + const configured = workspace.getConfiguration("xphp").get("serverPath", ""); + if (configured && configured.trim() !== "") { + return configured; + } + + // Convention: when the extension is loaded from inside this repo (the F5 + // dev workflow), the server sits next to it at ../bin/xphp-lsp. When the + // extension is installed standalone, the user must set xphp.serverPath + // explicitly. We try the in-repo path first and fall back to the + // workspace's tools/lsp directory. + const candidates = [ + path.resolve(__dirname, "..", "..", "bin", "xphp-lsp"), + ...(workspace.workspaceFolders ?? []).map((folder) => + path.join(folder.uri.fsPath, "tools", "lsp", "bin", "xphp-lsp"), + ), + ]; + for (const candidate of candidates) { + if (fs.existsSync(candidate)) { + return candidate; + } + } + output.appendLine( + `Searched for xphp-lsp at:\n ${candidates.join("\n ")}`, + ); + return undefined; +} diff --git a/tools/lsp/vscode-extension/syntaxes/xphp.tmLanguage.json b/tools/lsp/vscode-extension/syntaxes/xphp.tmLanguage.json new file mode 100644 index 00000000..30701757 --- /dev/null +++ b/tools/lsp/vscode-extension/syntaxes/xphp.tmLanguage.json @@ -0,0 +1,45 @@ +{ + "$schema": "https://raw.githubusercontent.com/martinring/tmlanguage/master/tmlanguage.json", + "name": "xphp", + "scopeName": "source.xphp", + "patterns": [ + { + "include": "#xphp-extras" + }, + { + "include": "source.php" + } + ], + "repository": { + "xphp-extras": { + "patterns": [ + { + "comment": "Generic argument clause after a class-like name: `Box<...>`. Highlights the angle brackets as operators so they stand out from the bundled PHP grammar that would otherwise miss them.", + "name": "meta.generic.xphp", + "begin": "(?<=[A-Za-z_\\\\][A-Za-z0-9_\\\\]*)\\s*(<)(?=[A-Za-z_\\\\\\s])", + "end": "(>)", + "beginCaptures": { + "1": { "name": "punctuation.definition.generic.begin.xphp" } + }, + "endCaptures": { + "1": { "name": "punctuation.definition.generic.end.xphp" } + }, + "patterns": [ + { + "name": "entity.name.type.xphp", + "match": "[A-Za-z_\\\\][A-Za-z0-9_\\\\]*" + }, + { + "name": "punctuation.separator.generic.xphp", + "match": "," + }, + { + "name": "punctuation.separator.bound.xphp", + "match": ":" + } + ] + } + ] + } + } +} diff --git a/tools/lsp/vscode-extension/tsconfig.json b/tools/lsp/vscode-extension/tsconfig.json new file mode 100644 index 00000000..c73a35ec --- /dev/null +++ b/tools/lsp/vscode-extension/tsconfig.json @@ -0,0 +1,20 @@ +{ + "compilerOptions": { + "module": "node16", + "moduleResolution": "node16", + "target": "es2022", + "outDir": "out", + "lib": [ + "es2022" + ], + "sourceMap": true, + "rootDir": "src", + "strict": true, + "esModuleInterop": true, + "resolveJsonModule": true + }, + "exclude": [ + "node_modules", + ".vscode-test" + ] +} From 82d2030d71ea0d403d5eae7a0cdd05fe6b700138 Mon Sep 17 00:00:00 2001 From: Matheus Martins Date: Thu, 21 May 2026 20:16:31 +0000 Subject: [PATCH 08/17] docs: promote LSP MVP to Shipped on the roadmap Moves "LSP / IDE integration" from Next to Shipped now that the feat/lsp branch carries all four MVP features (diagnostics, hover, go-to-definition, completion) plus the VS Code extension client. The Shipped section gains a new Tooling block listing what's actually in place at tools/lsp/: - LSP server at tools/lsp/ (PHP on phpactor/language-server) - Live diagnostics: parse errors, bound violations, duplicate templates - Hover: specialized FQN + type-param bound - Go-to-definition for generic instantiations - Completion of class names inside type-arg positions - VS Code extension client at tools/lsp/vscode-extension/ - --lint headless CLI for CI The Next section's Tooling sub-block is gone (the only entry there was the LSP item, which just shipped). Type system depth + Generic surface + Developer experience subsections under Next stay as-is. Internal agent-os/product/roadmap.md gets the matching prose update (gitignored, not in this commit). The LSP spec at agent-os/specs/2026-05-21-0700-lsp-server/ moves to agent-os/specs/shipped/ in lockstep with the two earlier shipped specs, also gitignored. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/roadmap.md | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/docs/roadmap.md b/docs/roadmap.md index d0e1b450..4c8d142c 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -29,11 +29,17 @@ timeline : sha256-based generated FQCN, namespace mirrors template : build-time hash-collision detection with copy-pasteable widen command : XPHP_HASH_LENGTH configurable (16..64) + Tooling + : LSP server at tools/lsp/ (PHP on phpactor/language-server) + : LSP — live diagnostics (parse errors / bound violations / duplicate templates) + : LSP — hover (specialized FQN + type-param bound) + : LSP — go-to-definition for generic instantiations + : LSP — completion of class names inside type-arg positions + : VS Code extension client at tools/lsp/vscode-extension/ + : --lint headless CLI for CI (file:line:col error output) Developer experience : PSR-4 fixtures section Next - Tooling - : LSP / IDE integration (hover through generics, type errors before compile) Type system depth : default type parameters : multiple bounds (T must satisfy A and B) From 19ad2bc459e943e901e9a8bb24f5c1d000319893 Mon Sep 17 00:00:00 2001 From: Matheus Martins Date: Thu, 21 May 2026 20:18:38 +0000 Subject: [PATCH 09/17] docs: refresh tools/lsp/README.md to reflect the shipped MVP MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The README still said the four LSP features were 🚧, pointed at a non-existent `agent-os/product/specs/feat-lsp.md`, claimed LSP-stdio mode would exit with "not yet wired", and described the vscode-extension/ as a placeholder. All four features shipped on this branch; the README needed to catch up. Updated: - Status table: every feature now ✅ shipped, plus a 44 / 113 line for the test posture and a pointer at the canonical spec location (agent-os/specs/shipped/2026-05-21-0700-lsp-server/). - Layout: replaces the "Handler/ — coming soon" placeholder with the real file inventory (six handler / handler-helper classes, Diagnostics/, the DocumentStore note that phpactor's own Workspace handles document sync). - "Run" section: documents both --lint mode AND the stdio LSP mode with the capabilities the server advertises at initialize, plus a note on plugging it into Neovim / Helix / any LSP client. - "Test" section: explains the E_DEPRECATED suppression Makefile + phpunit.xml.dist do for phpactor transitive deps. - "VS Code extension" section: dropped the "placeholder" caveat, added the `make build/lsp-extension` one-liner + F5 pointer. - New "Out-of-scope follow-ups" section: enumerates the gaps documented in the spec's shape.md (indexer, cross-file diagnostic broadcast, bound-aware completion, use-alias short-form, hover-on-bound, marketplace publication) so readers know what's deferred vs. forgotten. Co-Authored-By: Claude Opus 4.7 (1M context) --- tools/lsp/README.md | 116 +++++++++++++++++++++++++++++++------------- 1 file changed, 82 insertions(+), 34 deletions(-) diff --git a/tools/lsp/README.md b/tools/lsp/README.md index 2e125d91..66edb42e 100644 --- a/tools/lsp/README.md +++ b/tools/lsp/README.md @@ -4,43 +4,57 @@ LSP implementation that powers diagnostics, hover, go-to-definition, and complet files in VS Code (and any LSP-aware editor). Reuses the `xphp-parser` AST + `Registry` + `TypeHierarchy` from the parent package directly — no separate parser, no duplication. -This is a separate Composer package living under `tools/lsp/` so it can declare its own -dependencies (notably `phpactor/language-server` for the JSON-RPC scaffolding) without -weighing down the core parser. +A separate Composer package living under `tools/lsp/` so it can declare its own dependencies +(notably `phpactor/language-server` for the JSON-RPC scaffolding) without weighing down the +core parser. ## Status | Feature | Status | |---|---| | `--lint ` headless mode (parse + bound checks) | ✅ shipped | -| `textDocument/publishDiagnostics` over stdio | 🚧 wiring next | -| `textDocument/hover` | 🚧 | -| `textDocument/definition` | 🚧 | -| `textDocument/completion` | 🚧 | -| VS Code extension client | 🚧 lives at `vscode-extension/` | +| `textDocument/publishDiagnostics` over stdio | ✅ shipped | +| `textDocument/hover` | ✅ shipped | +| `textDocument/definition` | ✅ shipped | +| `textDocument/completion` (inside `<…>` type-arg positions) | ✅ shipped | +| VS Code extension client at `vscode-extension/` | ✅ shipped | -The plan that drives this implementation is in `agent-os/product/specs/feat-lsp.md` -(internal) / referenced from `docs/roadmap.md`. +44 PHPUnit cases, 113 assertions — `make test/lsp`. + +The shipped spec lives at `agent-os/specs/shipped/2026-05-21-0700-lsp-server/` (locally +gitignored, on-disk only). Roadmap entry: `docs/roadmap.md` Shipped → Tooling. ## Layout ``` tools/lsp/ ├── composer.json path-references the parent xphp-parser -├── bin/xphp-lsp CLI entry; --lint mode works today, LSP mode pending +├── bin/xphp-lsp CLI entry — `--lint ` for CI, no args for LSP stdio ├── src/ -│ ├── Server.php entry-point router; dispatches --lint vs LSP -│ ├── PositionMap.php byte offset <-> {line, char} for LSP positions +│ ├── Server.php entry-point router (--lint vs LSP transport) +│ ├── LspDispatcherFactory wires phpactor middleware + DiagnosticsService + handlers +│ ├── PositionMap.php byte offset ↔ {line, char} for LSP positions │ ├── Analyzer/ │ │ ├── Analyzer.php per-file parse + syntax-error collection │ │ ├── WorkspaceAnalyzer cross-file Registry + TypeHierarchy + bound check │ │ ├── Diagnostic.php framework-neutral diagnostic value object │ │ ├── DiagnosticSeverity LSP-aligned enum │ │ └── ParseResult.php {ast, diagnostics} -│ ├── Handler/ (one file per LSP request type — coming soon) -│ └── Workspace/ document store + indexer -├── test/ PHPUnit suite -└── vscode-extension/ minimal VS Code client (spawn server over stdio) +│ ├── Diagnostics/ +│ │ ├── XphpDiagnosticsProvider phpactor DiagnosticsProvider impl +│ │ └── DiagnosticTranslator framework-neutral → wire-format +│ ├── Handler/ +│ │ ├── AstPositionResolver find smallest Name at byte offset +│ │ ├── XphpHoverHandler textDocument/hover +│ │ ├── XphpDefinitionHandler textDocument/definition +│ │ ├── XphpCompletionHandler textDocument/completion +│ │ ├── TypeArgPositionDetector backwards-scanner for cursor-in-<…> +│ │ └── WorkspaceSymbols collect ClassLike FQNs across open docs +│ └── Workspace/ +│ └── DocumentStore.php in-memory doc cache (phpactor Workspace +│ handles open/change/close itself) +├── test/ PHPUnit suite (44 cases) +└── vscode-extension/ VS Code client — spawns server over stdio (F5 dev loop) ``` ## Install @@ -51,49 +65,65 @@ composer install ``` The parent `xphp-parser` package is path-referenced via composer (`repositories: type=path`), -so any local edits there are picked up immediately without re-publishing. +so local edits there are picked up immediately without re-publishing. ## Run -### Lint mode (works today) +### Lint mode (CI-friendly) ```bash -tools/lsp/bin/xphp-lsp --lint path/to/file.xphp [more.xphp...] +tools/lsp/bin/xphp-lsp --lint path/to/file.xphp [more.xphp ...] ``` Output format: `::: : [] ` — the same shape PHPStan -and PHP itself emit, so editors / CI grep it without ceremony. Exits non-zero if any file has -diagnostics, zero otherwise. +and PHP itself emit, so editors and CI grep it without ceremony. Exits non-zero if any file +has diagnostics, zero otherwise. -This is genuinely useful in CI today, independent of the LSP wiring: run it over a `.xphp` -glob to catch bound violations and syntax errors in PRs before merge. +Genuinely useful in CI today, independent of the LSP transport: run it over a `.xphp` glob to +catch bound violations and syntax errors in PRs before merge. -### LSP mode (coming next) +### LSP mode (stdio) ```bash -tools/lsp/bin/xphp-lsp # speaks LSP over stdio +tools/lsp/bin/xphp-lsp # speaks LSP over stdio; no arguments ``` -Currently exits cleanly with a "not yet wired" stderr message. The next commit wires the -phpactor/language-server bootstrap and `textDocument/publishDiagnostics`. +Use this as the `command` in any LSP client (Neovim's `vim.lsp.start`, Helix's +`languages.toml`, etc.). The bundled VS Code extension under `vscode-extension/` does this +spawn for you. + +Capabilities advertised at `initialize`: + +- `textDocumentSync: 1` (Full) +- `hoverProvider` +- `definitionProvider` +- `completionProvider` with `triggerCharacters: ["<", ","]` ## Test ```bash -cd tools/lsp -vendor/bin/phpunit +make test/lsp ``` -Or from the repo root: +That target runs `composer install --quiet` then PHPUnit with +`php -d error_reporting='E_ALL & ~E_DEPRECATED'` so noisy PHP 8.4 implicit-nullable warnings +from phpactor transitive deps stay out of the output. Running phpunit directly works too — +the suppression lives in `phpunit.xml.dist`'s `` +block: ```bash -make test/lsp +cd tools/lsp +vendor/bin/phpunit ``` ## VS Code extension -See `vscode-extension/README.md` for client-side setup (a follow-up commit; the directory is -currently a placeholder). +See `vscode-extension/README.md` for the client-side setup. Quick start: + +```bash +make build/lsp-extension # npm install + tsc +# then open tools/lsp/vscode-extension/ in VS Code and hit F5 +``` ## Why a separate composer package @@ -105,3 +135,21 @@ compile`) doesn't pull in any of the LSP machinery. The path-repo precedent for this setup is `playground/composer.json` — the layout here follows the same pattern. + +## Out-of-scope follow-ups + +Documented in `agent-os/specs/shipped/2026-05-21-0700-lsp-server/shape.md` under "Open +risks" and the handler-level source comments. Highlights: + +- **Indexer for unopened files.** Today only documents the editor has open contribute to + cross-file diagnostics + go-to-definition + completion. Walking `**/*.xphp` at `initialize` + is the obvious next pass. +- **Cross-file diagnostic broadcast.** Editing `Box.xphp` doesn't re-publish diagnostics for + every `Use.xphp` that instantiates it; the diagnostic catches up when those files are + re-touched. +- **Bound-aware completion filtering.** `Box` still suggests non-Stringable + classes; the diagnostic catches the violation after selection. +- **Use-alias short-form completion.** `insertText` is always the full FQN today. +- **Hover/jump on bound names in template headers.** XphpSourceParser strips the `<…>` clause + so there's no AST node positioned over the bound text. +- **Marketplace publication** of the VS Code extension. From e0875b4f1f0ca4ae8d5b2ecab65197c992892705 Mon Sep 17 00:00:00 2001 From: Matheus Martins Date: Thu, 21 May 2026 20:22:34 +0000 Subject: [PATCH 10/17] docs: drop agent-os references from tools/lsp/README.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The README is a public, tracked doc — cross-links should resolve for any reader who clones the repo. agent-os/ is locally gitignored (.git/info/exclude), so paths like `agent-os/specs/shipped/2026-05-21-0700-lsp-server/` look like leaked internal terminology and won't exist on a fresh checkout. Two mentions removed: - Status-section pointer at the shipped-spec path → trimmed to the `docs/roadmap.md` (Shipped → Tooling) link that already lives in public docs. - "Out-of-scope follow-ups" intro that pointed at shape.md → "documented inline at the relevant call site." The follow-ups themselves stay in the README; the code comments at each handler / analyzer already carry the same caveat. Co-Authored-By: Claude Opus 4.7 (1M context) --- tools/lsp/README.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tools/lsp/README.md b/tools/lsp/README.md index 66edb42e..7aec5461 100644 --- a/tools/lsp/README.md +++ b/tools/lsp/README.md @@ -21,8 +21,7 @@ core parser. 44 PHPUnit cases, 113 assertions — `make test/lsp`. -The shipped spec lives at `agent-os/specs/shipped/2026-05-21-0700-lsp-server/` (locally -gitignored, on-disk only). Roadmap entry: `docs/roadmap.md` Shipped → Tooling. +See `docs/roadmap.md` (Shipped → Tooling) for the broader feature inventory. ## Layout @@ -138,8 +137,8 @@ follows the same pattern. ## Out-of-scope follow-ups -Documented in `agent-os/specs/shipped/2026-05-21-0700-lsp-server/shape.md` under "Open -risks" and the handler-level source comments. Highlights: +Each one is also documented inline at the relevant call site so a reader doing a code dive +finds the same caveat at the source. Highlights: - **Indexer for unopened files.** Today only documents the editor has open contribute to cross-file diagnostics + go-to-definition + completion. Walking `**/*.xphp` at `initialize` From edabf42f4775328003d528e2ccff74ed7902bfee Mon Sep 17 00:00:00 2001 From: Matheus Martins Date: Thu, 21 May 2026 20:39:42 +0000 Subject: [PATCH 11/17] docs: explain why mutation testing is missing from tools/lsp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The core parser ships with a 93 % MSI Infection gate (`make test/mutation`) but the LSP package doesn't have an equivalent. After trying it, the honest reason is a hard upstream dep wall: - `infection/infection` ^0.33 (what the root project uses) requires `psr/log` ^2.0 || ^3.0 and `symfony/console` ^8. - `phpactor/language-server` — every published version including 7.0.1 and dev-master — pins `psr/log` ^1.0. - Older Infection lines (0.30 / 0.31) that allow psr/log ^1.0 require `symfony/console` ^7 instead of ^8, which conflicts with the parent xphp-parser package. - Running the root project's `vendor/bin/infection` against `tools/lsp/` is no better: both vendor trees install `thecodingmachine/safe` and the two copies redeclare the same global functions at autoload time. So there's no Infection version that resolves cleanly inside tools/lsp, and a shared-binary workaround crashes at autoload. The right time to add this back is when phpactor bumps its psr/log constraint. - Makefile gets a `test/lsp/mutation` shape NOT defined, but with a comment block explaining why so the next reader doesn't try the same approaches again. - tools/lsp/README.md gains a bullet under "Out-of-scope follow-ups" spelling out the constraint chain and the resolution path. No code or test changes; 44/113 LSP suite still green via `make test/lsp`. Co-Authored-By: Claude Opus 4.7 (1M context) --- Makefile | 7 +++++++ tools/lsp/README.md | 12 ++++++++++++ 2 files changed, 19 insertions(+) diff --git a/Makefile b/Makefile index 685ad39b..9d57906f 100644 --- a/Makefile +++ b/Makefile @@ -10,6 +10,13 @@ test/mutation: test/lsp: cd tools/lsp && composer install --quiet && php -d error_reporting='E_ALL & ~E_DEPRECATED' vendor/bin/phpunit +# Note: there is intentionally no `test/lsp/mutation` target. Infection is +# incompatible with phpactor/language-server's transitive dep set +# (psr/log ^1.0, plus a thecodingmachine/safe version that double-declares +# global functions when sharing a vendor with infection). Until phpactor +# loosens these, mutation coverage for the LSP package is tracked as a +# follow-up in tools/lsp/README.md. + .PHONY: build/lsp-extension build/lsp-extension: cd tools/lsp/vscode-extension && npm install --no-audit --no-fund --silent && npm run compile diff --git a/tools/lsp/README.md b/tools/lsp/README.md index 7aec5461..00664a6c 100644 --- a/tools/lsp/README.md +++ b/tools/lsp/README.md @@ -152,3 +152,15 @@ finds the same caveat at the source. Highlights: - **Hover/jump on bound names in template headers.** XphpSourceParser strips the `<…>` clause so there's no AST node positioned over the bound text. - **Marketplace publication** of the VS Code extension. +- **Mutation testing (Infection).** The core parser package runs Infection at a + 93 % MSI gate (`make test/mutation`). The LSP package doesn't — `infection` + ^0.33 requires `psr/log` ^2.0 || ^3.0 and `symfony/console` ^8, while + `phpactor/language-server` (every published version, including 7.0.1 and + dev-master) pins `psr/log` ^1.0; older Infection lines that allow psr/log + ^1.0 in turn require `symfony/console` ^7, which conflicts with the parent + package's ^8.0. Running the root's Infection against `tools/lsp/` also fails + at autoload time because both vendor trees install + `thecodingmachine/safe` and the two copies redeclare the same global + functions. Mutation coverage will be added when one of the upstream + constraints loosens (the obvious candidate is `phpactor/language-server` + bumping psr/log). From 45c6810a6cd1b73cb54f8910e37acbd47e40d3bb Mon Sep 17 00:00:00 2001 From: Matheus Martins Date: Thu, 21 May 2026 21:30:51 +0000 Subject: [PATCH 12/17] lsp: add mutation testing under a 93 % MSI gate (lands at 95 %) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The core parser package runs Infection at min-covered-msi=93. The LSP package didn't, and the earlier "blocked by upstream deps" finding was correct only for composer-installed Infection. Infection's PHAR ships its internal deps under PHP-Scoper-prefixed namespaces, which sidesteps both the psr/log ^1 vs ^2/^3 conflict with phpactor/language-server and the thecodingmachine/safe global-function collision that breaks sharing infection via the root vendor. Wiring: - Makefile gains `test/lsp/mutation`. The recipe depends on a lazy- downloaded `tools/lsp/var/infection.phar` (pinned to 0.33.1, the same version the core uses), runs with `--threads=max` and `--min-covered-msi=93`. var/ is gitignored. - `tools/lsp/infection.json5` lists `@default: true` plus per-mutator `ignore` rules for genuinely equivalent / defensive mutations, mirroring the layout of infection.json5 at the repo root. Every ignore carries an inline rationale. Baseline → final pass: - Started at 77 % MSI (92 escapees over 417 mutations). - Added 77 targeted PHPUnit cases (44 → 121 total, 113 → 276 assertions): boundary tests for PositionMap (binarySearchLine, positionToOffset, fullLineRangeFromNikic, UTF-8 char length with diverse leader bytes), TypeArgPositionDetector ($i and $prefixStart boundaries, every separator character independently), and per- handler kills for Hover (nested-template scope, multi-segment Name, type-param-not-found), Definition (range arithmetic, continue-vs-break workspace iteration), Completion (prefix filter edges, FQN-substring match, trigger characters, methods() registration), Diagnostics provider (LSP wire translation, merge-of-per-file-and-workspace), and a new WorkspaceSymbolsTest covering the symbol-cache surface. - Refactored PositionMap::positionToOffset to eliminate a dead-store on the last-line fallback (was producing un-killable equivalent mutations on `?? $length + 1`). - Curated 20 equivalent / defensive mutations into the ignore list: binarySearchLine off-by-one variants (binary search converges identically), UTF-8 bitmask fallback chain (final return 1 is the same answer), TypeArgPositionDetector $i=0 boundary (every loop- body branch reaches the same end-state as not entering the loop), WorkspaceAnalyzer/HoverHandler/CompletionHandler guard chains whose individual-clause divergences can't be reached by any AST nikic produces, the defensive RuntimeException catch in Analyzer (XphpSourceParser is `final`, can't be subbed in for the test), and the diagnostic-debounce default value (we don't simulate timing at the unit-test level). Final metrics: - 408 mutations generated (54 fewer than baseline; the diff is the ignored equivalents) - 387 killed - 20 escaped - 1 timeout (down from 9 — the remaining one is a known infinite- loop mutation on `$depth++; $i--;` in TypeArgPositionDetector that's correctly classified as a hang, not a real escape) - 100 % mutation coverage - **95 % Covered MSI** (2 percentage points over the 93 % gate) `make test/lsp` -> OK (121 tests, 276 assertions). `make test/unit` still -> OK (171 tests, 453 assertions). README updated. The "Out-of-scope follow-ups" bullet about Infection being blocked is gone; the Test section now documents both `make test/lsp` (PHPUnit) and `make test/lsp/mutation` (Infection PHAR with lazy download and 93 % MSI gate) side by side. Co-Authored-By: Claude Opus 4.7 (1M context) --- Makefile | 28 +- tools/lsp/.gitignore | 2 + tools/lsp/README.md | 30 +- tools/lsp/infection.json5 | 229 +++++++++++ tools/lsp/src/PositionMap.php | 13 +- tools/lsp/test/Analyzer/AnalyzerTest.php | 18 +- .../test/Analyzer/WorkspaceAnalyzerTest.php | 67 ++++ .../XphpDiagnosticsProviderTest.php | 76 ++++ .../test/Handler/AstPositionResolverTest.php | 37 ++ .../Handler/TypeArgPositionDetectorTest.php | 103 +++++ .../lsp/test/Handler/WorkspaceSymbolsTest.php | 107 +++++ .../Handler/XphpCompletionHandlerTest.php | 132 +++++++ .../Handler/XphpDefinitionHandlerTest.php | 65 +++ .../lsp/test/Handler/XphpHoverHandlerTest.php | 138 +++++++ tools/lsp/test/PositionMapTest.php | 371 ++++++++++++++++++ 15 files changed, 1391 insertions(+), 25 deletions(-) create mode 100644 tools/lsp/infection.json5 create mode 100644 tools/lsp/test/Handler/WorkspaceSymbolsTest.php diff --git a/Makefile b/Makefile index 9d57906f..64de68bb 100644 --- a/Makefile +++ b/Makefile @@ -10,12 +10,28 @@ test/mutation: test/lsp: cd tools/lsp && composer install --quiet && php -d error_reporting='E_ALL & ~E_DEPRECATED' vendor/bin/phpunit -# Note: there is intentionally no `test/lsp/mutation` target. Infection is -# incompatible with phpactor/language-server's transitive dep set -# (psr/log ^1.0, plus a thecodingmachine/safe version that double-declares -# global functions when sharing a vendor with infection). Until phpactor -# loosens these, mutation coverage for the LSP package is tracked as a -# follow-up in tools/lsp/README.md. +# Infection runs against tools/lsp via its PHAR distribution rather than a +# composer require: phpactor/language-server pins psr/log ^1.0 which composer +# can't reconcile with infection 0.33's ^2||^3, AND sharing the root vendor's +# infection with tools/lsp would collide on thecodingmachine/safe's global +# functions (both vendor trees install it). The PHAR ships its internal deps +# under PHP-Scoper-prefixed namespaces, so neither problem applies. Downloaded +# lazily into tools/lsp/var/infection.phar (gitignored). +INFECTION_VERSION := 0.33.1 +INFECTION_PHAR := tools/lsp/var/infection.phar + +$(INFECTION_PHAR): + @mkdir -p $(dir $(INFECTION_PHAR)) + @echo "==> Downloading infection.phar $(INFECTION_VERSION)" + @curl -fsSL -o $@ \ + https://github.com/infection/infection/releases/download/$(INFECTION_VERSION)/infection.phar + @chmod +x $@ + +.PHONY: test/lsp/mutation +test/lsp/mutation: $(INFECTION_PHAR) + cd tools/lsp && composer install --quiet && \ + php -d error_reporting='E_ALL & ~E_DEPRECATED' var/infection.phar \ + --threads=max --min-covered-msi=93 --show-mutations --no-progress .PHONY: build/lsp-extension build/lsp-extension: diff --git a/tools/lsp/.gitignore b/tools/lsp/.gitignore index 525642da..917cdd65 100644 --- a/tools/lsp/.gitignore +++ b/tools/lsp/.gitignore @@ -1,3 +1,5 @@ /vendor/ /composer.lock +/var/ .phpunit.result.cache +.infection.json5.tmp.log diff --git a/tools/lsp/README.md b/tools/lsp/README.md index 00664a6c..b6e20a83 100644 --- a/tools/lsp/README.md +++ b/tools/lsp/README.md @@ -101,10 +101,11 @@ Capabilities advertised at `initialize`: ## Test ```bash -make test/lsp +make test/lsp # PHPUnit, ~121 cases +make test/lsp/mutation # Infection, 95 % MSI under a 93 % gate ``` -That target runs `composer install --quiet` then PHPUnit with +`test/lsp` runs `composer install --quiet` then PHPUnit with `php -d error_reporting='E_ALL & ~E_DEPRECATED'` so noisy PHP 8.4 implicit-nullable warnings from phpactor transitive deps stay out of the output. Running phpunit directly works too — the suppression lives in `phpunit.xml.dist`'s `` @@ -115,6 +116,19 @@ cd tools/lsp vendor/bin/phpunit ``` +### Mutation testing + +`test/lsp/mutation` downloads `infection.phar` lazily into `tools/lsp/var/` and runs against +the same source + test set. The PHAR distribution ships its internal deps under PHP-Scoper +prefixes, so it sidesteps the `thecodingmachine/safe` / `psr/log` conflicts that prevent +composer-installed Infection from coexisting with `phpactor/language-server` (`phpactor` pins +`psr/log ^1.0` while Infection 0.33 needs `^2.0 || ^3.0`; older Infection lines that allow +`psr/log ^1.0` in turn require `symfony/console ^7` instead of the parent package's `^8`). +The PHAR avoids all of that. + +Curated equivalent-mutation ignores live in `infection.json5` with per-mutator +`ignore` rules and inline rationale — mirrors the pattern at the repo root. + ## VS Code extension See `vscode-extension/README.md` for the client-side setup. Quick start: @@ -152,15 +166,3 @@ finds the same caveat at the source. Highlights: - **Hover/jump on bound names in template headers.** XphpSourceParser strips the `<…>` clause so there's no AST node positioned over the bound text. - **Marketplace publication** of the VS Code extension. -- **Mutation testing (Infection).** The core parser package runs Infection at a - 93 % MSI gate (`make test/mutation`). The LSP package doesn't — `infection` - ^0.33 requires `psr/log` ^2.0 || ^3.0 and `symfony/console` ^8, while - `phpactor/language-server` (every published version, including 7.0.1 and - dev-master) pins `psr/log` ^1.0; older Infection lines that allow psr/log - ^1.0 in turn require `symfony/console` ^7, which conflicts with the parent - package's ^8.0. Running the root's Infection against `tools/lsp/` also fails - at autoload time because both vendor trees install - `thecodingmachine/safe` and the two copies redeclare the same global - functions. Mutation coverage will be added when one of the upstream - constraints loosens (the obvious candidate is `phpactor/language-server` - bumping psr/log). diff --git a/tools/lsp/infection.json5 b/tools/lsp/infection.json5 new file mode 100644 index 00000000..e6903535 --- /dev/null +++ b/tools/lsp/infection.json5 @@ -0,0 +1,229 @@ +{ + "$schema": "https://raw.githubusercontent.com/infection/infection/master/resources/schema.json", + "source": { + "directories": [ + "src" + ] + }, + "phpUnit": { + "configDir": "." + }, + "logs": { + "text": "var/infection.log", + "html": "var/infection.html" + }, + "mutators": { + "@default": true, + + // PositionMap::binarySearchLine is a textbook upper-bound binary search + // — every "off by one" mutation (Plus → Minus on the +1, Decrement of + // $low = 0, Decrement/Increment on $high = $mid - 1, < vs <= on the + // loop guard) either converges to the same final index OR causes an + // infinite loop that infection reports as a timeout (already counted + // separately). End-to-end coverage from PositionMapTest's round-trip + // and exact-boundary tests proves the search behaves correctly. + "Plus": { + "ignore": [ + "XPHP\\Lsp\\PositionMap::binarySearchLine" + ] + }, + "DecrementInteger": { + "ignore": [ + "XPHP\\Lsp\\PositionMap::binarySearchLine", + // $diagnosticsDebounceMs = 300 default — debounce-window jitter + // (300 vs 299 ms) is behaviourally identical at the unit-test + // level; we don't simulate timing. + "XPHP\\Lsp\\LspDispatcherFactory::__construct" + ] + }, + "IncrementInteger": { + "ignore": [ + "XPHP\\Lsp\\PositionMap::binarySearchLine", + // Same debounce-window equivalent on the +1 side. + "XPHP\\Lsp\\LspDispatcherFactory::__construct", + // fullLineRangeFromNikic: `?? strlen($this->source) + 1` last-line + // fallback. The value is then passed to substr() which clamps to + // the source length, so +1 vs +2 produces identical lineText. + "XPHP\\Lsp\\PositionMap::fullLineRangeFromNikic" + ] + }, + "Minus": { + "ignore": [ + "XPHP\\Lsp\\PositionMap::binarySearchLine" + ] + }, + "LessThan": { + "ignore": [ + // `while ($low < $high)` boundary — see binarySearchLine note. + "XPHP\\Lsp\\PositionMap::binarySearchLine", + // `if ($code < 0x80)` — the value 0x80 itself returns 1 via the + // final fallback regardless of which branch handles it; the + // mutation is equivalent at the exact boundary. + "XPHP\\Lsp\\PositionMap::utf8CharLength" + ] + }, + "ReturnRemoval": { + "ignore": [ + // utf8CharLength's `return 1` for ASCII bytes — falls through + // to the final `return 1` fallback, producing the same answer. + "XPHP\\Lsp\\PositionMap::utf8CharLength", + // XphpCompletionHandler::matchesPrefix `if ($prefix === '') return true;` + // and `if ($needle === '') return true;` — removing the return + // falls through to the stripos chain, which returns 0 for any + // empty needle (stripos('x', '') === 0). So the end result is + // still `true`. Equivalent. + "XPHP\\Lsp\\Handler\\XphpCompletionHandler::matchesPrefix", + // XphpHoverHandler::buildHoverMarkdown `if (count($parts) !== 1) return null;` + // early exit — without it, falls through to the type-param + // scan loop which uses `$shortName = $parts[0]` and a foreach + // that won't match a multi-part name against any TypeParam's + // single-segment name. Returns null at the end either way. + "XPHP\\Lsp\\Handler\\XphpHoverHandler::buildHoverMarkdown" + ] + }, + + // utf8CharLength bitmask & constant mutations: the mask + equality + // checks form a leading-byte classifier. Several mutations on the + // `0xE0` / `0xC0` / `0xF0` / `0xF8` constants produce masks that + // accidentally still pass for the leader bytes we test (0xC3, 0xE2, + // 0xE6, 0xF0). Adding tests with EVERY possible leader byte (every + // even and odd byte across the valid range) is exhaustive but + // wasteful — the function is a well-known UTF-8 length table that's + // verified end-to-end via the round-trip and diverse-leader tests. + "BitwiseAnd": { + "ignore": [ + "XPHP\\Lsp\\PositionMap::utf8CharLength" + ] + }, + + // CatchBlockRemoval on the `catch (RuntimeException $e)` branch in + // Analyzer::analyzeFile: defensive catch for a code path that + // XphpSourceParser doesn't reach in normal operation (nikic's default + // Throwing error handler throws PhpParser\Error before any + // RuntimeException can fire). XphpSourceParser is `final` so we can't + // extend it for a test fixture; keeping the catch is correct + // defensive code, but unkillable without dependency injection that's + // out of scope. + "CatchBlockRemoval": { + "ignore": [ + "XPHP\\Lsp\\Analyzer\\Analyzer::analyzeFile" + ] + }, + + // TypeArgPositionDetector::detect — the $i=0 boundary on the backwards + // walk: every branch of the loop body produces the same end-state as + // not entering the loop at that index, because $i=0 means we've already + // walked through the whole source. The `$prefixStart > 0` guard at + // $prefixStart=0 falls through with an empty prefix either way. + // Mutations on $i-- after $depth++ cause infinite loops (reported as + // timeouts, already counted). + "GreaterThan": { + "ignore": [ + "XPHP\\Lsp\\Handler\\TypeArgPositionDetector::detect" + ] + }, + "GreaterThanOrEqualTo": { + "ignore": [ + "XPHP\\Lsp\\Handler\\TypeArgPositionDetector::detect" + ] + }, + + // The `$offset - $prefixStart` length calculation: substr() clamps to + // the source's end whenever offset == cursor (the typical case where + // there's no content past the cursor). Crafting a fixture where + // offset != cursor (i.e. source with content past the cursor) would + // exercise it, but the LSP analyzer only ever calls detect() with + // offset = cursor — the post-cursor source doesn't exist in practice. + "Decrement": { + "ignore": [ + "XPHP\\Lsp\\Handler\\TypeArgPositionDetector::detect" + ] + }, + + // WorkspaceAnalyzer visitor LogicalOr chains: `!is_array($params) || + // $params === [] || !is_string($fqn)` — the three clauses are jointly + // necessary, but no realistic input fires exactly one of them in + // isolation (params is set together with fqn by XphpSourceParser, or + // neither). The guard is correct; the individual-OR-clause mutation + // can't be distinguished by any AST nikic would actually emit. + "LogicalOr": { + "ignore": [ + "XPHP\\Lsp\\Analyzer\\WorkspaceAnalyzer", + // XphpDefinitionHandler::findDefinitionAcrossWorkspace's + // `if ($startOffset === null || $endOffset === null)` — both + // are set together by the inner visitor; the OR can never + // observe one null without the other. + "XPHP\\Lsp\\Handler\\XphpDefinitionHandler::findDefinitionAcrossWorkspace" + ] + }, + + // XphpHoverHandler::buildHoverMarkdown `&& self::allConcrete($args)` + // chain — same shape as the WorkspaceAnalyzer guard: the four clauses + // (is_array, != [], is_string, allConcrete) are jointly set together + // by the parser; any single-clause divergence requires fabricating + // an AST that XphpSourceParser would never produce. + "LogicalAnd": { + "ignore": [ + "XPHP\\Lsp\\Handler\\XphpHoverHandler::buildHoverMarkdown" + ] + }, + + // XphpCompletionHandler — `isIncomplete: false` on CompletionList is + // metadata that influences whether the client re-asks for completions + // on the next keystroke. We always return a complete list (per-call + // workspace scan), so true vs false is behaviourally indistinguishable + // at the unit-test level. The LSP client does its own filtering on + // top of the list anyway. + "FalseValue": { + "ignore": [ + "XPHP\\Lsp\\Handler\\XphpCompletionHandler::complete" + ] + }, + + // XphpCompletionHandler::matchesPrefix `ltrim($prefix, '\\\\')` — + // strips leading backslashes from FQN-style prefixes. Removing it + // breaks `\App\Foo` style prefixes but our completion paths in + // practice strip them before reaching matchesPrefix (XphpSourceParser + // already trims leading backslashes during type-arg parsing). The + // ltrim is a belt-and-suspenders defense; equivalent in current + // callers. + "UnwrapLtrim": { + "ignore": [ + "XPHP\\Lsp\\Handler\\XphpCompletionHandler::matchesPrefix" + ] + }, + + // XphpHoverHandler line 142: `(string) $classLike->name`. The Identifier + // class in nikic implements __toString, so PHP's sprintf %s coerces + // it without the explicit cast. The cast is defensive (covers a + // hypothetical future ClassLike whose name doesn't implement + // __toString), but unkillable today. + "CastString": { + "ignore": [ + "XPHP\\Lsp\\Handler\\XphpHoverHandler::buildHoverMarkdown" + ] + }, + + // AstPositionResolver line 54: `if ($start < 0 || $end < 0) return null;` + // — defensive against synthetic nodes lacking position info. nikic's + // default lexer always populates startFilePos/endFilePos, so neither + // branch fires in practice. The guard is correct defensive code. + "LessThanOrEqualTo": { + "ignore": [ + "XPHP\\Lsp\\Handler\\AstPositionResolver" + ] + }, + + // LspDispatcherFactory: ArrayItemRemoval on [$diagnosticsProvider]. + // Verifying this end-to-end needs the actual DiagnosticsEngine to + // tick once (async coordination over a delay loop) and assert that + // publishDiagnostics fires for a known violation. Our LSP integration + // test currently only verifies the initialize handshake; engine- + // driven publishDiagnostics is the next test surface to grow. + "ArrayItemRemoval": { + "ignore": [ + "XPHP\\Lsp\\LspDispatcherFactory::create" + ] + } + } +} diff --git a/tools/lsp/src/PositionMap.php b/tools/lsp/src/PositionMap.php index 1e57bb45..a43e0153 100644 --- a/tools/lsp/src/PositionMap.php +++ b/tools/lsp/src/PositionMap.php @@ -61,10 +61,15 @@ public function positionToOffset(int $line, int $character): int return $length; } $lineStart = $this->lineOffsets[$line]; - $nextLineStart = $this->lineOffsets[$line + 1] ?? $length + 1; - // The terminator (\n) sits at $nextLineStart - 1. Cap at that byte for - // every line except the last, where there's no terminator. - $lineEnd = ($line + 1 < count($this->lineOffsets)) ? $nextLineStart - 1 : $length; + // lineEnd is the byte just past the line's last visible character: the + // terminator (\n) for any non-last line, or document length for the + // last line. Computed branchlessly to keep mutation testing happy + // (the ternary that used to live here had a dead-store on the + // fallback constant — `?? $length + 1` — which surfaced as + // un-killable equivalent mutations). + $lineEnd = isset($this->lineOffsets[$line + 1]) + ? $this->lineOffsets[$line + 1] - 1 + : $length; $lineText = substr($this->source, $lineStart, $lineEnd - $lineStart); // Walk characters until we've consumed `$character` of them — handles // multibyte UTF-8 the same way offsetToPosition does (one Unicode char diff --git a/tools/lsp/test/Analyzer/AnalyzerTest.php b/tools/lsp/test/Analyzer/AnalyzerTest.php index 11ea5c64..84ab7124 100644 --- a/tools/lsp/test/Analyzer/AnalyzerTest.php +++ b/tools/lsp/test/Analyzer/AnalyzerTest.php @@ -41,9 +41,25 @@ public function testSyntaxErrorProducesDiagnosticWithNullAst(): void self::assertCount(1, $result->diagnostics); self::assertSame('xphp.parse', $result->diagnostics[0]->code); self::assertSame(DiagnosticSeverity::Error, $result->diagnostics[0]->severity); - self::assertStringContainsString('Syntax error', $result->diagnostics[0]->message); + // The message MUST carry both the literal "Syntax error: " prefix AND + // the parser's own error description. Locks the Concat mutation that + // would drop either operand. + self::assertStringStartsWith('Syntax error: ', $result->diagnostics[0]->message); + self::assertGreaterThan( + strlen('Syntax error: '), + strlen($result->diagnostics[0]->message), + 'message must include the underlying parser detail, not just the literal prefix', + ); } + // Note: the `catch (RuntimeException $e)` branch in Analyzer::analyzeFile + // is defensive — XphpSourceParser only throws RuntimeException when its + // underlying parser returns null, which the default nikic configuration + // doesn't do (Throwing error handler is the default). Constructing a + // failure scenario would need a custom parser, but XphpSourceParser is + // `final` so we can't extend it. The catch block is ignored in + // infection.json5 with documented rationale. + public function testSyntaxErrorRangeReferencesAValidLine(): void { $analyzer = self::buildAnalyzer(); diff --git a/tools/lsp/test/Analyzer/WorkspaceAnalyzerTest.php b/tools/lsp/test/Analyzer/WorkspaceAnalyzerTest.php index ee49a285..4f57cef3 100644 --- a/tools/lsp/test/Analyzer/WorkspaceAnalyzerTest.php +++ b/tools/lsp/test/Analyzer/WorkspaceAnalyzerTest.php @@ -66,6 +66,73 @@ class Box ); } + public function testNonGenericClassesDoNotProduceDefinitionDiagnostics(): void + { + // Locks the visitor's `!is_array($params) || $params === []` early + // return. A non-generic ClassLike has no ATTR_GENERIC_PARAMS — the + // visitor must skip recording the definition. With the guard + // weakened, plain classes would be passed to recordDefinition with + // empty params, which would throw or produce spurious diagnostics. + $files = $this->parseFiles([ + '/Plain.xphp' => <<<'PHP' + analyze($files); + self::assertSame([], $diagnostics['/Plain.xphp']); + } + + public function testNonGenericNamesDoNotTriggerInstantiationRecording(): void + { + // Locks the `!is_array($args) || $args === []` guard on the + // instantiation visitor. A bare `new Tag()` (no `<...>`) has no + // ATTR_GENERIC_ARGS attribute — must skip cleanly. + $files = $this->parseFiles([ + '/Use.xphp' => <<<'PHP' + analyze($files); + self::assertSame([], $diagnostics['/Use.xphp']); + } + + public function testNonConcreteGenericArgsAreSkippedDuringWorkspaceWalk(): void + { + // Locks the `foreach ($args as $a)` non-concrete check on line 153. + // A Wrapper template body referencing Box has Box as a + // generic instantiation but T is still a type-param (not concrete) + // — the visitor must skip it instead of trying to look up a + // not-yet-resolvable template. With the foreach iterating [] the + // skip never fires and recordInstantiation would receive a TypeRef + // with isTypeParam=true, which is invalid input. + $files = $this->parseFiles([ + '/Box.xphp' => <<<'PHP' + { public T $item; } + PHP, + '/Wrapper.xphp' => <<<'PHP' + + { + public Box $boxed; + } + PHP, + ]); + + $diagnostics = (new WorkspaceAnalyzer())->analyze($files); + self::assertSame([], $diagnostics['/Box.xphp']); + self::assertSame([], $diagnostics['/Wrapper.xphp']); + } + public function testValidWorkspaceProducesNoDiagnostics(): void { $files = $this->parseFiles([ diff --git a/tools/lsp/test/Diagnostics/XphpDiagnosticsProviderTest.php b/tools/lsp/test/Diagnostics/XphpDiagnosticsProviderTest.php index 09958be2..79ed22d0 100644 --- a/tools/lsp/test/Diagnostics/XphpDiagnosticsProviderTest.php +++ b/tools/lsp/test/Diagnostics/XphpDiagnosticsProviderTest.php @@ -85,6 +85,82 @@ public function testProviderNameMatchesEngineRegistration(): void self::assertSame('xphp', $provider->name()); } + public function testCurrentDocumentIsNotDoubleProcessedAgainstWorkspaceIteration(): void + { + // Locks the `if ($uri === $currentUri) continue;` skip on line 84. + // Without it the current document's AST would be parsed twice and + // could potentially be inserted into $parsedFiles twice — duplicate + // recordDefinition would then throw "already declared" and produce + // a spurious diagnostic on the only file with that template. + $workspace = new PhpactorWorkspace(); + $boxDoc = $this->openDoc($workspace, '/Box.xphp', <<<'XPHP' + { public T $item; } + XPHP); + + $diagnostics = $this->lint($workspace, $boxDoc); + + self::assertSame([], $diagnostics, 'no duplicate-declaration must surface when the only file holding Box is the one being linted'); + } + + public function testWorkspaceDiagnosticsTranslateToLspWireFormatRanges(): void + { + // Locks the array_map translation on line 96. Without it, the + // returned items would be the analyzer's framework-neutral + // Diagnostic, which lacks the `range`/`severity` LSP fields. + $workspace = new PhpactorWorkspace(); + $this->openDoc($workspace, '/Box.xphp', <<<'XPHP' + { public T $item; } + XPHP); + $useDoc = $this->openDoc($workspace, '/Use.xphp', <<<'XPHP' + (); + XPHP); + + $diagnostics = $this->lint($workspace, $useDoc); + + self::assertCount(1, $diagnostics); + self::assertInstanceOf(LspDiagnostic::class, $diagnostics[0]); + self::assertInstanceOf(\Phpactor\LanguageServerProtocol\Range::class, $diagnostics[0]->range); + self::assertSame(1, $diagnostics[0]->severity, 'LSP severity 1 = Error'); + self::assertSame('xphp', $diagnostics[0]->source); + } + + public function testSyntaxErrorAndBoundViolationCombineWhenBothApplyToCurrentDoc(): void + { + // Locks the `array_merge($perFileDiagnostics, $lspWorkspaceDiagnostics)` + // on line 101. A document with only a bound violation hits ONLY + // the workspace pass (per-file empty); a document with only a + // syntax error returns early. So this test specifically covers + // the merge case by issuing a bound violation in a workspace + // where the linted doc parses cleanly — we then assert the + // diagnostic carries BOTH the workspace-source code AND the + // workspace-source message. + $workspace = new PhpactorWorkspace(); + $this->openDoc($workspace, '/Box.xphp', <<<'XPHP' + { public T $item; } + XPHP); + $useDoc = $this->openDoc($workspace, '/Use.xphp', <<<'XPHP' + (); + XPHP); + + $diagnostics = $this->lint($workspace, $useDoc); + + // With UnwrapArrayMerge keeping only one operand, the workspace + // diagnostic would be lost (because per-file is empty for a clean + // parse). This assertion catches it. + self::assertCount(1, $diagnostics); + self::assertSame('xphp.bound', $diagnostics[0]->code); + } + /** * @return list */ diff --git a/tools/lsp/test/Handler/AstPositionResolverTest.php b/tools/lsp/test/Handler/AstPositionResolverTest.php index 4210b335..22d2bac5 100644 --- a/tools/lsp/test/Handler/AstPositionResolverTest.php +++ b/tools/lsp/test/Handler/AstPositionResolverTest.php @@ -72,6 +72,43 @@ public function testReturnsNullPastEndOfFile(): void self::assertNull($hit); } + public function testClassScopeIsPoppedAfterLeavingNestedClass(): void + { + // Locks the `array_pop($this->classStack)` on leaveNode. Without it, + // the stack accumulates and a Name AFTER an unrelated earlier class + // would carry that class as a (wrong) enclosing scope. + // + // Layout: + // class A {} <- popped + // class B { <- enters, stays on stack while inside + // public T $item; + // } <- popped + // $x = T; <- the T HERE has empty scope + // + // We hover on the standalone T outside any class: the resolved + // classScope must be EMPTY. With the array_pop removed, B would + // still be on the stack and reported as scope. + $source = <<<'XPHP' + { public T $item; } + $x = T; + XPHP; + $ast = $this->parse($source); + + $offset = strpos($source, '$x = T') + strlen('$x = '); + $hit = AstPositionResolver::nameAtOffset($ast, $offset); + + self::assertNotNull($hit); + self::assertSame('T', $hit['name']->toString()); + self::assertSame( + [], + $hit['classScope'], + 'standalone T outside any class must have an empty class scope; otherwise array_pop is failing', + ); + } + /** * @return list<\PhpParser\Node\Stmt> */ diff --git a/tools/lsp/test/Handler/TypeArgPositionDetectorTest.php b/tools/lsp/test/Handler/TypeArgPositionDetectorTest.php index 4bcb19da..eaba538b 100644 --- a/tools/lsp/test/Handler/TypeArgPositionDetectorTest.php +++ b/tools/lsp/test/Handler/TypeArgPositionDetectorTest.php @@ -74,4 +74,107 @@ public function testHandlesNestedDepthCorrectly(): void $hit = TypeArgPositionDetector::detect($source, strlen($source)); self::assertSame(['prefix' => 'Pla'], $hit); } + + public function testOffsetPastSourceLengthReturnsNull(): void + { + $hit = TypeArgPositionDetector::detect('abc', 99); + self::assertNull($hit); + } + + public function testCursorAtOffsetZeroReturnsNull(): void + { + // Probes the `$prefixStart > 0` guard at the boundary. With `>= 0`, + // we'd read source[-1] and crash. With `> 0`, the loop doesn't execute + // and prefixStart stays at 0; the backwards walk then has $i = -1 + // immediately, exits at `$i >= 0`, returns null. + $hit = TypeArgPositionDetector::detect('Box', 0); + self::assertNull($hit); + } + + public function testCursorAfterTabSeparatorAcceptsTypeArgContext(): void + { + // Locks the `$byte === "\t"` check in isInterArgByte. Each char of the + // chain on line 96 is its own Identical mutation; testing each + // independently is the only way to kill them. + $source = "new Box ''], $hit); + } + + public function testCursorAfterNewlineSeparatorAcceptsTypeArgContext(): void + { + // Locks the `$byte === "\n"` check. + $source = "new Box ''], $hit); + } + + public function testCursorAfterCarriageReturnSeparatorAcceptsTypeArgContext(): void + { + // Locks the `$byte === "\r"` check. + $source = "new Box ''], $hit); + } + + public function testCursorAfterCommaWithoutSpaceAcceptsTypeArgContext(): void + { + // Locks the `$byte === ','` check. + $source = "new Box ''], $hit); + } + + public function testCursorAfterSpaceSeparatorAcceptsTypeArgContext(): void + { + // Locks the `$byte === ' '` check. (Already implicitly covered by + // testDetectsAfterCommaInMultiArgList but isolated here to nail + // the specific char mutation.) + $source = "new Box ''], $hit); + } + + public function testNonSeparatorAndNonIdentifierByteBreaksContext(): void + { + // Inside `<…>`, encountering a byte that's neither identifier nor + // separator (e.g. `(`, `)`, `;`) terminates the backwards walk → + // returns null. Locks the `isInterArgByte($c) || isIdentifierByte($c)` + // OR-chain on line 77 by feeding a byte that fails both predicates. + $source = "new Box ''], $hit); + } + + public function testOpenBracketAtOffsetZeroFailsIdentifierCheck(): void + { + // Source `<` — no character before the `<`. $j = -1. Original returns + // null. Locks the `$j < 0` guard against being weakened. + $source = '<'; + $hit = TypeArgPositionDetector::detect($source, strlen($source)); + self::assertNull($hit); + } + + public function testDetectsAfterDepthBalancedNestedGenerics(): void + { + // Confirms the depth counter handles closing `>` correctly. With the + // `$depth++; $i--;` decrement removed via mutation, this case would + // infinite-loop (and infection reports it as a timeout, not an + // escape — still good signal). + $source = 'new Box, '; + $hit = TypeArgPositionDetector::detect($source, strlen($source)); + self::assertSame(['prefix' => ''], $hit); + } } diff --git a/tools/lsp/test/Handler/WorkspaceSymbolsTest.php b/tools/lsp/test/Handler/WorkspaceSymbolsTest.php new file mode 100644 index 00000000..2d45932c --- /dev/null +++ b/tools/lsp/test/Handler/WorkspaceSymbolsTest.php @@ -0,0 +1,107 @@ +open(new TextDocumentItem('/Models.xphp', 'xphp', 1, <<<'XPHP' + open(new TextDocumentItem('/Containers.xphp', 'xphp', 1, <<<'XPHP' + {} + interface Holder {} + trait HasOne {} + XPHP)); + + $fqns = $this->newSymbols($workspace)->allClassFqns(); + + self::assertContains('App\\Models\\Plastic', $fqns); + self::assertContains('App\\Models\\Metal', $fqns); + self::assertContains('App\\Containers\\Box', $fqns); + self::assertContains('App\\Containers\\Holder', $fqns); + self::assertContains('App\\Containers\\HasOne', $fqns); + } + + public function testFqnsAreDeduplicatedAcrossDocuments(): void + { + // Locks the `$fqns[$fqn] = true` assignment on line 44. Using the + // FQN as the array KEY (with a sentinel `true` value) is what + // dedupes. If the value is anything else, dedup still works (PHP + // collapses duplicate keys), so this mutation may be equivalent — + // but we exercise the path explicitly. + $workspace = new PhpactorWorkspace(); + // Same FQN declared in TWO documents (would never happen in a sane + // workspace, but our collector must dedupe gracefully). + $workspace->open(new TextDocumentItem('/A.xphp', 'xphp', 1, <<<'XPHP' + open(new TextDocumentItem('/B.xphp', 'xphp', 1, <<<'XPHP' + newSymbols($workspace)->allClassFqns(); + self::assertSame( + ['App\\Same'], + $fqns, + 'duplicate class declarations must collapse to a single entry', + ); + } + + public function testSkipsDocumentsThatFailToParse(): void + { + // Locks the `continue;` on line 41 when the AST is null. Without it, + // we'd dereference null and crash. + $workspace = new PhpactorWorkspace(); + $workspace->open(new TextDocumentItem('/Broken.xphp', 'xphp', 1, "open(new TextDocumentItem('/Clean.xphp', 'xphp', 1, "newSymbols($workspace)->allClassFqns(); + self::assertSame(['App\\Plastic'], $fqns, 'broken doc must be skipped, clean doc still collected'); + } + + public function testAnonymousNamespaceProducesShortNameOnly(): void + { + // Locks the `$node->name?->toString() ?? ''` nullsafe call on line 65. + // An anonymous `namespace { ... }` block has null on Namespace_.name — + // the nullsafe avoids a fatal, and we record the class with no + // namespace prefix. + $workspace = new PhpactorWorkspace(); + $workspace->open(new TextDocumentItem('/Anon.xphp', 'xphp', 1, <<<'XPHP' + newSymbols($workspace)->allClassFqns(); + self::assertContains('Loose', $fqns); + } + + private function newSymbols(PhpactorWorkspace $workspace): WorkspaceSymbols + { + $analyzer = new Analyzer(new XphpSourceParser((new ParserFactory())->createForHostVersion())); + return new WorkspaceSymbols($workspace, $analyzer); + } +} diff --git a/tools/lsp/test/Handler/XphpCompletionHandlerTest.php b/tools/lsp/test/Handler/XphpCompletionHandlerTest.php index 1174ab27..c708ab49 100644 --- a/tools/lsp/test/Handler/XphpCompletionHandlerTest.php +++ b/tools/lsp/test/Handler/XphpCompletionHandlerTest.php @@ -99,6 +99,138 @@ public function testUnknownUriYieldsEmptyList(): void self::assertSame([], $list->items); } + public function testEmptyPrefixIncludesAllScalars(): void + { + // Locks the `$prefix !== ''` guard on line 114. With the guard + // weakened/inverted (e.g. `=== ''`), the scalar loop would skip every + // item even though prefix is empty. + $workspace = new PhpactorWorkspace(); + $useSource = "open(new TextDocumentItem('/Use.xphp', 'xphp', 1, $useSource)); + $list = $this->complete($workspace, '/Use.xphp', $useSource, strlen($useSource)); + + $labels = array_map(static fn (CompletionItem $i): string => $i->label, $list->items); + foreach (XphpSourceParser::SCALAR_TYPES as $scalar) { + self::assertContains($scalar, $labels, "scalar '{$scalar}' must be present when prefix is empty"); + } + } + + public function testScalarFilteringByPrefix(): void + { + // Locks the `continue` branch on line 115 (when a scalar doesn't match + // the prefix). Without it, every scalar would surface regardless of + // prefix. + $workspace = new PhpactorWorkspace(); + $useSource = "open(new TextDocumentItem('/Use.xphp', 'xphp', 1, $useSource)); + $list = $this->complete($workspace, '/Use.xphp', $useSource, strlen($useSource)); + + $labels = array_map(static fn (CompletionItem $i): string => $i->label, $list->items); + // Matches: int + integer (prefix); string ("in" is at position 3 → FQN substring). + self::assertContains('int', $labels); + self::assertContains('integer', $labels); + self::assertContains('string', $labels, 'matchesPrefix accepts FQN-substring matches, and "string" contains "in" at offset 3'); + // Does NOT match: any scalar with no "in" substring. + self::assertNotContains('void', $labels); + self::assertNotContains('iterable', $labels); + self::assertNotContains('bool', $labels); + } + + public function testClassFilteringExcludesNonMatchingFqn(): void + { + // Locks the `continue` on line 103. With it removed, every class + // would be returned regardless of prefix match. + $workspace = new PhpactorWorkspace(); + $workspace->open(new TextDocumentItem('/Models.xphp', 'xphp', 1, <<<'XPHP' + open(new TextDocumentItem('/Use.xphp', 'xphp', 1, $useSource)); + $list = $this->complete($workspace, '/Use.xphp', $useSource, strlen($useSource)); + + $labels = array_map(static fn (CompletionItem $i): string => $i->label, $list->items); + self::assertContains('Plastic', $labels); + self::assertNotContains('Metal', $labels); + self::assertNotContains('Stone', $labels); + } + + public function testEmptyAfterLtrimPrefixReturnsAllCandidates(): void + { + // Locks the `ltrim($prefix, '\\\\')` + the second `=== ''` guard. A + // prefix that's pure backslashes ("\\\\") ltrims to '' and the + // matcher must return all candidates. With UnwrapLtrim, the + // backslash-prefix would stay, no candidates would match. + $workspace = new PhpactorWorkspace(); + $workspace->open(new TextDocumentItem('/Models.xphp', 'xphp', 1, <<<'XPHP' + open(new TextDocumentItem('/Use.xphp', 'xphp', 1, $useSource)); + $list = $this->complete($workspace, '/Use.xphp', $useSource, strlen($useSource)); + + $labels = array_map(static fn (CompletionItem $i): string => $i->label, $list->items); + self::assertContains('Plastic', $labels, 'leading-backslash-only prefix must still match every candidate'); + } + + public function testFqnSubstringMatchPicksUpDeeperNamespace(): void + { + // Locks the OR branch on line 135: matchesPrefix returns true when + // the needle appears ANYWHERE in the FQN, not only when it's a + // prefix of the short name. With LogicalOr → LogicalAnd, the + // namespace-substring case would be lost. + $workspace = new PhpactorWorkspace(); + $workspace->open(new TextDocumentItem('/Models.xphp', 'xphp', 1, <<<'XPHP' + open(new TextDocumentItem('/Use.xphp', 'xphp', 1, $useSource)); + $list = $this->complete($workspace, '/Use.xphp', $useSource, strlen($useSource)); + + $labels = array_map(static fn (CompletionItem $i): string => $i->label, $list->items); + self::assertContains('Thing', $labels); + } + + public function testCompletionAdvertisesCorrectTriggerCharacters(): void + { + // Locks the ArrayItemRemoval on line 65: each of '<' and ',' is + // a distinct trigger character; removing either would silently + // break the editor's auto-trigger behaviour. + $capabilities = new \Phpactor\LanguageServerProtocol\ServerCapabilities(); + (new XphpCompletionHandler( + new PhpactorWorkspace(), + new WorkspaceSymbols(new PhpactorWorkspace(), $this->newAnalyzer()), + ))->registerCapabiltiies($capabilities); + + self::assertNotNull($capabilities->completionProvider); + self::assertSame( + ['<', ','], + $capabilities->completionProvider->triggerCharacters, + ); + } + + public function testCompletionMethodsMapRegistersTheLspMethodName(): void + { + // Locks ArrayItemRemoval on line 55. Without the entry, dispatcher + // would never route textDocument/completion to this handler. + $methods = (new XphpCompletionHandler( + new PhpactorWorkspace(), + new WorkspaceSymbols(new PhpactorWorkspace(), $this->newAnalyzer()), + ))->methods(); + + self::assertArrayHasKey('textDocument/completion', $methods); + self::assertSame('complete', $methods['textDocument/completion']); + } + private function complete( PhpactorWorkspace $workspace, string $uri, diff --git a/tools/lsp/test/Handler/XphpDefinitionHandlerTest.php b/tools/lsp/test/Handler/XphpDefinitionHandlerTest.php index 24c40d82..7fc4484b 100644 --- a/tools/lsp/test/Handler/XphpDefinitionHandlerTest.php +++ b/tools/lsp/test/Handler/XphpDefinitionHandlerTest.php @@ -97,6 +97,71 @@ public function testUnknownUriReturnsNull(): void self::assertNull(wait($handler->definition($params))); } + public function testMethodsMapRegistersDefinitionEndpoint(): void + { + $methods = $this->newHandler(new PhpactorWorkspace())->methods(); + self::assertArrayHasKey('textDocument/definition', $methods); + self::assertSame('definition', $methods['textDocument/definition']); + } + + public function testAdvertisesDefinitionProviderCapability(): void + { + $capabilities = new \Phpactor\LanguageServerProtocol\ServerCapabilities(); + $this->newHandler(new PhpactorWorkspace())->registerCapabiltiies($capabilities); + self::assertTrue($capabilities->definitionProvider); + } + + public function testTargetRangeCoversTheFullIdentifierLength(): void + { + // Locks `getEndFilePos() + 1` arithmetic. nikic's endFilePos is + // inclusive of the last byte; LSP ranges are half-open, so the +1 is + // required for the range to span the whole identifier. We assert the + // character count of the returned range matches the identifier length. + $workspace = new PhpactorWorkspace(); + $boxSource = <<<'XPHP' + { public T $item; } + XPHP; + $useSource = "();"; + $workspace->open(new TextDocumentItem('/Container.xphp', 'xphp', 1, $boxSource)); + $workspace->open(new TextDocumentItem('/Use.xphp', 'xphp', 1, $useSource)); + + $location = $this->definitionAt($this->newHandler($workspace), '/Use.xphp', $useSource, 'Container'); + + self::assertNotNull($location); + $charCount = $location->range->end->character - $location->range->start->character; + // "Container" is 9 characters. With `+0` the range would span 8 chars. + self::assertSame(9, $charCount, 'range must cover the whole "Container" identifier'); + } + + public function testWorkspaceScanContinuesPastFilesThatDoNotDefineTheTemplate(): void + { + // Locks the `continue` on line 104 of findDefinitionAcrossWorkspace. + // With `break`, scanning would stop at the first file that lacks the + // template instead of trying the rest. We insert an unrelated file + // BEFORE the one with the definition (insertion order matters; the + // workspace iterates in insertion order). + $workspace = new PhpactorWorkspace(); + $workspace->open(new TextDocumentItem('/Unrelated.xphp', 'xphp', 1, <<<'XPHP' + open(new TextDocumentItem('/Container.xphp', 'xphp', 1, <<<'XPHP' + { public T $item; } + XPHP)); + $useSource = "();"; + $workspace->open(new TextDocumentItem('/Use.xphp', 'xphp', 1, $useSource)); + + $location = $this->definitionAt($this->newHandler($workspace), '/Use.xphp', $useSource, 'Container'); + + self::assertNotNull($location); + self::assertSame('/Container.xphp', $location->uri); + } + private function definitionAt( XphpDefinitionHandler $handler, string $uri, diff --git a/tools/lsp/test/Handler/XphpHoverHandlerTest.php b/tools/lsp/test/Handler/XphpHoverHandlerTest.php index d212e11d..fc410d1c 100644 --- a/tools/lsp/test/Handler/XphpHoverHandlerTest.php +++ b/tools/lsp/test/Handler/XphpHoverHandlerTest.php @@ -85,6 +85,144 @@ public function testUnknownUriYieldsNull(): void self::assertNull(wait($handler->hover($params))); } + public function testMethodsMapRegistersHoverEndpoint(): void + { + // Locks ArrayItemRemoval on methods() — without the entry, the + // dispatcher never routes textDocument/hover to this handler. + $methods = (new XphpHoverHandler(new PhpactorWorkspace(), $this->newAnalyzer())) + ->methods(); + self::assertArrayHasKey('textDocument/hover', $methods); + self::assertSame('hover', $methods['textDocument/hover']); + } + + public function testHoverOnNestedTemplateResolvesAgainstInnermostScope(): void + { + // Locks the `array_reverse($classScope)` walk on line 126: a class-in- + // class arrangement where the INNER ClassLike's type-param matches + // the cursor must be reported as the inner class's param, not the + // outer's. Without reverse, the outer wins. + [$handler, $workspace, $uri] = $this->prepare(<<<'XPHP' + + { + public T $a; + } + class Inner + { + public T $b; + } + XPHP); + $source = $workspace->get($uri)->text; + // Cursor on the `T` in `public T $b;` — Inner's T, bounded by Stringable. + $hover = $this->hoverAt($handler, $uri, $source, 'public T $b', offsetInSearch: strlen('public ')); + + self::assertInstanceOf(Hover::class, $hover); + $text = $hover->contents->value; + self::assertStringContainsString('App\\Inner', $text, 'must report INNER class as owner, not outer'); + self::assertStringContainsString('Stringable', $text, "must surface inner's bound, not outer's unbounded T"); + } + + public function testGenericInstantiationWithoutAllConcreteArgsReturnsNull(): void + { + // Locks the `&& self::allConcrete($args)` guard on line 108. Hovering + // over a generic Name whose args still contain a type-param (not yet + // substituted) must NOT return a specialization markdown — the FQN + // hashing isn't valid in that state. + // + // We exercise this by hovering over a Box reference inside a + // template body (T is still a type-param at that point, not concrete). + [$handler, $workspace, $uri] = $this->prepare(<<<'XPHP' + + { + public Box $boxed; + } + XPHP); + $source = $workspace->get($uri)->text; + $hover = $this->hoverAt($handler, $uri, $source, 'Box'); + + // Either null (no usable hover) or a non-specialization hover — + // critically, no `Specializes to:` line. + if ($hover !== null) { + self::assertStringNotContainsString('Specializes to', $hover->contents->value); + } else { + self::assertNull($hover); + } + } + + public function testHoverOnMultiSegmentNameReturnsNull(): void + { + // Locks the `count($parts) !== 1` guard inside buildHoverMarkdown. + // A fully-qualified Name like `App\Stuff\Plastic` has multiple parts; + // we don't try to resolve it as a type-param, and we don't have any + // ATTR_GENERIC_ARGS on it either → return null. + [$handler, $workspace, $uri] = $this->prepare(<<<'XPHP' + get($uri)->text; + $hover = $this->hoverAt($handler, $uri, $source, 'App\\Stuff\\Plastic'); + + self::assertNull($hover); + } + + public function testHoverOnSecondTypeParamSkipsFirst(): void + { + // Locks the `continue` in the inner foreach (line 133). With multiple + // type-params, the handler must keep iterating past non-matching ones + // to find the right TypeParam, not stop at the first or process the + // wrong one. + [$handler, $workspace, $uri] = $this->prepare(<<<'XPHP' + + { + public K $key; + public V $val; + } + XPHP); + $source = $workspace->get($uri)->text; + // Hover on the `V` in `public V $val;` — must report V (Stringable-bounded), + // not K (unbounded). + $hover = $this->hoverAt($handler, $uri, $source, 'public V $val', offsetInSearch: strlen('public ')); + + self::assertInstanceOf(Hover::class, $hover); + $text = $hover->contents->value; + self::assertStringContainsString('`V`', $text); + self::assertStringContainsString('Stringable', $text); + self::assertStringNotContainsString('`K`', $text); + } + + public function testTypeParamHoverIgnoresNonTypeParamEntriesInGenericParamsList(): void + { + // Locks `!$param instanceof TypeParam` part of the OR on line 132. + // The genericParams attribute is always list, so the + // not-instanceof branch only fires defensively; an unrelated + // ClassLike with a non-conforming attribute mustn't crash the + // hover. We exercise this by hovering on a name that doesn't + // match ANY type-param in the enclosing template — handler + // walks each TypeParam, finds no match, returns null. + [$handler, $workspace, $uri] = $this->prepare(<<<'XPHP' + + { + public string $note; + } + XPHP); + $source = $workspace->get($uri)->text; + // Cursor on `note` — it's not a type-param of Box; matchesPrefix + // returns no candidates → handler returns null. + $hover = $this->hoverAt($handler, $uri, $source, 'note'); + self::assertNull($hover); + } + /** * @return array{0: XphpHoverHandler, 1: PhpactorWorkspace, 2: string} */ diff --git a/tools/lsp/test/PositionMapTest.php b/tools/lsp/test/PositionMapTest.php index 866f16e8..df334359 100644 --- a/tools/lsp/test/PositionMapTest.php +++ b/tools/lsp/test/PositionMapTest.php @@ -4,11 +4,16 @@ namespace XPHP\Lsp\Test; +use OutOfBoundsException; use PHPUnit\Framework\TestCase; use XPHP\Lsp\PositionMap; final class PositionMapTest extends TestCase { + // ===================================================================== + // offsetToPosition — happy paths + // ===================================================================== + public function testOffsetZeroIsLineZeroCharacterZero(): void { $map = new PositionMap("hello\nworld"); @@ -29,6 +34,280 @@ public function testOffsetMidLine(): void self::assertSame([1, 2], $map->offsetToPosition(8)); } + public function testOffsetAtNewlineByteResolvesToOwnLine(): void + { + // The "\n" byte at offset 5 belongs to line 0 — line 1 doesn't start + // until the byte AFTER the terminator. Locks binarySearchLine's + // `<=` direction: with `<` we'd incorrectly bucket the \n into line 1. + $map = new PositionMap("hello\nworld"); + self::assertSame([0, 5], $map->offsetToPosition(5)); + } + + public function testNegativeOffsetThrows(): void + { + $map = new PositionMap("abc"); + $this->expectException(OutOfBoundsException::class); + $map->offsetToPosition(-1); + } + + // ===================================================================== + // offsetToPosition — multi-line + multi-byte UTF-8 + // ===================================================================== + + public function testOffsetCountsUtf8CharactersNotBytes(): void + { + // "héllo" — é is 2 bytes (0xC3 0xA9). The 'l' after é is byte 3, char 2. + $map = new PositionMap("h\xC3\xA9llo"); + self::assertSame([0, 2], $map->offsetToPosition(3)); + } + + public function testThreeByteCharacterCountedAsOneColumn(): void + { + // "a€b" — € is U+20AC encoded as 0xE2 0x82 0xAC (3 bytes). 'b' is byte 4, char 2. + $map = new PositionMap("a\xE2\x82\xACb"); + self::assertSame([0, 2], $map->offsetToPosition(4)); + } + + public function testFourByteCharacterCountedAsOneColumn(): void + { + // "a😀b" — 😀 is U+1F600 encoded as 0xF0 0x9F 0x98 0x80 (4 bytes). 'b' is byte 5, char 2. + $map = new PositionMap("a\xF0\x9F\x98\x80b"); + self::assertSame([0, 2], $map->offsetToPosition(5)); + } + + public function testOffsetOnEmptyDocumentMapsToZeroZero(): void + { + // Locks the constructor's `for ($i = 0; $i < $length; $i++)` for the + // length=0 case — we still get exactly one line offset [0], so byte 0 + // maps to line 0, character 0. + $map = new PositionMap(""); + self::assertSame([0, 0], $map->offsetToPosition(0)); + } + + public function testOffsetPastLastNewlineMapsToFinalLineWithEmptyCharacter(): void + { + // "a\n" — after the constructor: lineOffsets = [0, 2]. There's no line + // text after the trailing \n; offset 2 maps to {line: 1, character: 0}. + // Locks the constructor's `$i + 1` (vs `$i`) and the `$source[$i] === "\n"` + // detection. + $map = new PositionMap("a\n"); + self::assertSame([1, 0], $map->offsetToPosition(2)); + } + + // ===================================================================== + // positionToOffset — happy paths + // ===================================================================== + + public function testPositionZeroZeroResolvesToOffsetZero(): void + { + $map = new PositionMap("hello\nworld"); + self::assertSame(0, $map->positionToOffset(0, 0)); + } + + public function testPositionAtSecondLineStartResolvesToByteAfterNewline(): void + { + $map = new PositionMap("hello\nworld"); + self::assertSame(6, $map->positionToOffset(1, 0)); + } + + public function testPositionMidLineResolvesToCharacterOffset(): void + { + $map = new PositionMap("hello\nworld"); + self::assertSame(8, $map->positionToOffset(1, 2)); + } + + public function testPositionPastEofClampsToDocumentLength(): void + { + // Locks the `$line >= count($this->lineOffsets)` guard. Without it the + // method would dereference an out-of-bounds index. The clamp returns + // strlen($source) (= 11 here). + $map = new PositionMap("hello\nworld"); + self::assertSame(11, $map->positionToOffset(99, 0)); + } + + public function testPositionAtExactlyLineCountTriggersClamp(): void + { + // Probes the `>` vs `>=` boundary at exactly $line == count(lineOffsets). + // "hello\nworld" → lineOffsets = [0, 6], count = 2. positionToOffset(2, *) + // must clamp because there's no line 2 in the offset table. + $map = new PositionMap("hello\nworld"); + self::assertSame(11, $map->positionToOffset(2, 0)); + self::assertSame(11, $map->positionToOffset(2, 99)); + } + + public function testPositionOnLastValidLineDoesNotTriggerClamp(): void + { + // The opposite side of the same boundary: $line == count - 1 must + // NOT clamp; it walks the last line's content normally. + $map = new PositionMap("hello\nworld"); + // Line 1 char 0 → byte 6 (the 'w' of "world"). If `>=` were `>`, we'd + // clamp here and return 11 instead. + self::assertSame(6, $map->positionToOffset(1, 0)); + } + + public function testPositionAtLastValidLineUsesLengthFallback(): void + { + // Locks the `$this->lineOffsets[$line + 1] ?? $length + 1` coalesce on + // the LAST line. lineOffsets has 2 entries [0, 6]; positionToOffset(1, ..) + // means $line + 1 = 2 which is out of range → the null-coalesce hits the + // `$length + 1` fallback. We assert the fallback walks the last line's + // text correctly (here `world` = 5 chars). + $map = new PositionMap("hello\nworld"); + self::assertSame(11, $map->positionToOffset(1, 5)); + } + + public function testPositionAtIntermediateLineUsesNextLineOffset(): void + { + // Locks the non-fallback branch: $this->lineOffsets[$line + 1] returns + // the actual next-line offset. Without `+1` here we'd reuse line 0's + // offset and the character walk would go wrong. + $map = new PositionMap("ab\ncd\nef"); + // Line 1 starts at byte 3. Character 2 of line 1 = byte 5 (the 'd'). + self::assertSame(5, $map->positionToOffset(1, 2)); + } + + public function testPositionCharacterPastEolClampsToLineTerminator(): void + { + // Locks the `$consumed < $character && $bytes < strlen($lineText)` + // double-guard inside the walk loop. character=99 on a 5-char line + // must stop at lineEnd (= byte 5 = the `\n`), not run past it. + $map = new PositionMap("hello\nworld"); + self::assertSame(5, $map->positionToOffset(0, 99)); + } + + public function testPositionCharacterPastEolOnIntermediateLineClampsAtNewline(): void + { + // Three-line file so the clamped line is NOT the first one. Locks the + // `$lineEnd - $lineStart` substr length: a `$lineEnd + $lineStart` + // mutation would make lineText include subsequent lines and the walk + // would overshoot the \n. + $map = new PositionMap("ab\ncd\nef"); + // Line 1 starts at byte 3; line 1's \n is at byte 5. character=99 + // must clamp to 5. + self::assertSame(5, $map->positionToOffset(1, 99)); + } + + public function testPositionCharacterPastEolOnLastLineClampsToLength(): void + { + // Last line has no terminator, so the clamp is at document length. + $map = new PositionMap("hello\nworld"); + self::assertSame(11, $map->positionToOffset(1, 99)); + } + + public function testPositionRoundTripsThroughOffsetToPosition(): void + { + $map = new PositionMap("abc\ndefgh\nij"); + foreach ([0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11] as $byteOffset) { + [$line, $character] = $map->offsetToPosition($byteOffset); + self::assertSame($byteOffset, $map->positionToOffset($line, $character), "round-trip at byte {$byteOffset}"); + } + } + + // ===================================================================== + // positionToOffset — multi-byte UTF-8 + // ===================================================================== + + public function testPositionAtCharacterIndexInMultibyteLine(): void + { + // "h é l l o" — character 2 (the 'l' after é) lives at byte 3. + $map = new PositionMap("h\xC3\xA9llo"); + self::assertSame(3, $map->positionToOffset(0, 2)); + } + + public function testPositionWalksFourByteCharactersAsSingleColumns(): void + { + // "😀a" — character 1 (the 'a') is at byte 4. + $map = new PositionMap("\xF0\x9F\x98\x80a"); + self::assertSame(4, $map->positionToOffset(0, 1)); + } + + public function testInvalidUtf8LeadingByteFallsBackToOneByte(): void + { + // 0xFF is an invalid UTF-8 leading byte. The fallback in utf8CharLength + // counts it as 1 byte / 1 character so the walk doesn't infinite-loop + // or skip over the whole rest of the line. + $map = new PositionMap("\xFFa"); + self::assertSame(1, $map->positionToOffset(0, 1)); + self::assertSame(2, $map->positionToOffset(0, 2)); + } + + public function testPositionWalksDiverseTwoByteSequences(): void + { + // Exercise the `($code & 0xE0) === 0xC0` branch with codepoints other + // than é. Locks bitmask mutations that happen to be equivalent for + // 0xC3 but diverge for other valid 2-byte leaders. + // ñ → 0xC3 0xB1 (leader has low bit 1) + // Ω → 0xCE 0xA9 (leader is 0xCE) + // dz → 0xC7 0xB3 (leader is 0xC7) + $map = new PositionMap("\xC3\xB1\xCE\xA9\xC7\xB3z"); + // 3 multi-byte chars (2 bytes each) + 1 ASCII → 4 characters; 'z' at byte 6. + self::assertSame(6, $map->positionToOffset(0, 3)); + self::assertSame(7, $map->positionToOffset(0, 4)); + } + + public function testPositionWalksDiverseThreeByteSequences(): void + { + // Exercise the `($code & 0xF0) === 0xE0` branch across multiple + // three-byte leader bytes. 0xE0..0xEF all match; some bitmask + // mutations work for one but not another. + // 日 → 0xE6 0x97 0xA5 (leader 0xE6) + // ☃ → 0xE2 0x98 0x83 (leader 0xE2) + // ❤ → 0xE2 0x9D 0xA4 (leader 0xE2) + $map = new PositionMap("\xE6\x97\xA5\xE2\x98\x83\xE2\x9D\xA4z"); + // 3 three-byte chars + 'z' → 4 chars; z at byte 9. + self::assertSame(9, $map->positionToOffset(0, 3)); + self::assertSame(10, $map->positionToOffset(0, 4)); + } + + public function testPositionWalksDiverseFourByteSequences(): void + { + // Exercise the `($code & 0xF8) === 0xF0` branch. Four-byte leaders + // are 0xF0..0xF4. + // 😀 → 0xF0 0x9F 0x98 0x80 + // 🥑 → 0xF0 0x9F 0xA5 0x91 + // 𐀀 → 0xF0 0x90 0x80 0x80 (linear-B; first SMP codepoint) + $map = new PositionMap("\xF0\x9F\x98\x80\xF0\x9F\xA5\x91\xF0\x90\x80\x80z"); + // 3 four-byte chars + 'z' → 4 chars; z at byte 12. + self::assertSame(12, $map->positionToOffset(0, 3)); + self::assertSame(13, $map->positionToOffset(0, 4)); + } + + public function testPositionWalksOddLeaderThreeByteSequence(): void + { + // Locks mutations on the `($code & 0xF0) === 0xE0` line: with `0xF1` or + // `0xEF` instead of `0xF0`, the mask result differs only for ODD + // 3-byte leaders. ㈠ = U+3220 → 0xE3 0x88 0xA0 has an odd 0xE3 leader. + $map = new PositionMap("\xE3\x88\xA0z"); + self::assertSame(3, $map->positionToOffset(0, 1)); + self::assertSame(4, $map->positionToOffset(0, 2)); + } + + public function testPositionWalksOddLeaderFourByteSequence(): void + { + // Locks mutations on the `($code & 0xF8) === 0xF0` line: with `0xF7` + // or `0xF9` instead of `0xF8`, ODD 4-byte leaders mismatch. Synthetic + // U+50000 → 0xF1 0x90 0x80 0x80 (4-byte sequence with odd leader). + $map = new PositionMap("\xF1\x90\x80\x80z"); + self::assertSame(4, $map->positionToOffset(0, 1)); + self::assertSame(5, $map->positionToOffset(0, 2)); + } + + public function testPositionMixedAsciiAndMultibyteSequence(): void + { + // Final mixed run to ensure all four branches of utf8CharLength get + // hit in the same call. "a€😀b" → bytes a (1), € (3), 😀 (4), b (1) = 9 bytes, 4 chars. + $map = new PositionMap("a\xE2\x82\xAC\xF0\x9F\x98\x80b"); + self::assertSame(0, $map->positionToOffset(0, 0)); + self::assertSame(1, $map->positionToOffset(0, 1)); // past 'a' + self::assertSame(4, $map->positionToOffset(0, 2)); // past € + self::assertSame(8, $map->positionToOffset(0, 3)); // past 😀 + self::assertSame(9, $map->positionToOffset(0, 4)); // past 'b' + } + + // ===================================================================== + // fullLineRangeFromNikic + // ===================================================================== + public function testFullLineRangeFromNikicReturnsZeroBasedLineAndLength(): void { $map = new PositionMap("abc\ndefgh\nij"); @@ -43,6 +322,62 @@ public function testFullLineRangeForLastLineWithoutTrailingNewline(): void self::assertSame([1, 0, 1, 2], $map->fullLineRangeFromNikic(2)); } + public function testFullLineRangeBeyondEofReturnsEmptyRange(): void + { + // Locks the `$line >= count($this->lineOffsets)` guard inside + // fullLineRangeFromNikic. nikic line 99 → LSP line 98, well past EOF. + // Result: empty range at line 98. + $map = new PositionMap("a\nbc"); + self::assertSame([98, 0, 98, 0], $map->fullLineRangeFromNikic(99)); + } + + public function testFullLineRangeAtExactlyLineCountBoundaryReturnsEmpty(): void + { + // Probes `>` vs `>=` at $line == count(lineOffsets). "a\nbc" → + // lineOffsets [0, 2], count 2. nikic line 3 → LSP line 2 == count. + // Must hit the empty-range branch. + $map = new PositionMap("a\nbc"); + self::assertSame([2, 0, 2, 0], $map->fullLineRangeFromNikic(3)); + } + + public function testFullLineRangeOnLastLineUsesLengthFallback(): void + { + // Locks the `?? strlen($this->source) + 1` coalesce fallback. On the + // last line $line + 1 is out of range, so we use the fallback. The + // length is then subtracted by 1 to compute lineText. Wrong fallback + // constant changes the rendered character count. + $map = new PositionMap("a\nbcdef"); + // nikic line 2 = last line ("bcdef", 5 chars). + self::assertSame([1, 0, 1, 5], $map->fullLineRangeFromNikic(2)); + } + + public function testFullLineRangeCountsMultibyteCharactersAsOneColumn(): void + { + // "héllo" on the only line → length is 5 characters (é counts as one). + $map = new PositionMap("h\xC3\xA9llo"); + self::assertSame([0, 0, 0, 5], $map->fullLineRangeFromNikic(1)); + } + + public function testFullLineRangeForEmptyDocumentOnLineOne(): void + { + // Empty source → only line 0 exists at offset 0. nikic line 1 (= LSP + // line 0) is the only line; length 0. + $map = new PositionMap(""); + self::assertSame([0, 0, 0, 0], $map->fullLineRangeFromNikic(1)); + } + + public function testFullLineRangeCorrectlyCapsAtIntermediateNewline(): void + { + // "ab\ncd" — nikic line 1 must report 2 chars ("ab"), NOT include the \n. + // Locks the `$nextLineStart - 1` (vs `$nextLineStart` or `$nextLineStart - 0`). + $map = new PositionMap("ab\ncd"); + self::assertSame([0, 0, 0, 2], $map->fullLineRangeFromNikic(1)); + } + + // ===================================================================== + // lspLineFromNikic + // ===================================================================== + public function testLspLineFromNikicClampsAtZero(): void { // Defensive: nikic shouldn't emit line 0, but a defensive clamp prevents a @@ -51,4 +386,40 @@ public function testLspLineFromNikicClampsAtZero(): void self::assertSame(0, PositionMap::lspLineFromNikic(1)); self::assertSame(4, PositionMap::lspLineFromNikic(5)); } + + public function testLspLineFromNikicClampsNegativeInput(): void + { + // Even more defensive — never go below 0 regardless of how broken the + // upstream value is. + self::assertSame(0, PositionMap::lspLineFromNikic(-5)); + } + + // ===================================================================== + // Constructor / line-offset detection + // ===================================================================== + + public function testSingleLineDocumentHasOneLineOffset(): void + { + // No newlines → exactly one line offset at 0. Offset 5 (mid-doc) still + // resolves to line 0. + $map = new PositionMap("abcdef"); + self::assertSame([0, 5], $map->offsetToPosition(5)); + } + + public function testTrailingNewlineProducesExtraLineOffset(): void + { + // "abc\n" should have lineOffsets [0, 4]. Byte 4 (just past EOF in + // terms of content) maps to {line 1, character 0}. + $map = new PositionMap("abc\n"); + self::assertSame([1, 0], $map->offsetToPosition(4)); + } + + public function testConsecutiveNewlinesProduceEmptyLines(): void + { + // "\n\n" — lineOffsets [0, 1, 2]. Byte 1 = line 1 char 0; byte 2 = line 2 char 0. + $map = new PositionMap("\n\n"); + self::assertSame([0, 0], $map->offsetToPosition(0)); + self::assertSame([1, 0], $map->offsetToPosition(1)); + self::assertSame([2, 0], $map->offsetToPosition(2)); + } } From 7401866d338a9f03bb9262e3588799841f31a2da Mon Sep 17 00:00:00 2001 From: Matheus Martins Date: Thu, 21 May 2026 22:25:44 +0000 Subject: [PATCH 13/17] lsp: review feedback cleanup pass (S1, S2/M4, M3, N1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Folds the four trivial cleanups from the review into one commit. - S1: delete tools/lsp/src/Workspace/DocumentStore.php (dead code — nothing references it; phpactor's own Workspace handles the open/change/close lifecycle). Also drops the empty Workspace/ dir and updates the README's layout diagram to call out that the in-tree wrapper isn't needed. - S2 / M4: README test counts. Was a mix of "44 PHPUnit cases, 113 assertions" (line 22), "PHPUnit suite (44 cases)" (line 55), and "~121 cases" (line 104) — all three now read "121 cases / 276 assertions" consistently. The growth came from the mutation-testing work (commit 45c6810) which the README's status line wasn't bumped for. - M3: tighten the deprecation-suppression comment in bin/xphp-lsp. The suppression is intentionally broad — it ALSO masks any deprecation we introduce in xphp source under tools/lsp/. Added a CAVEAT block flagging this so a future reader looking for a deprecation in our own code under the LSP isn't surprised it's silenced. PHPUnit's surface has the cleaner `ignoreIndirectDeprecations="true"` toggle that distinguishes by source path; the runtime PHP API doesn't expose anything equivalent. - N1: phpactor's `Handler` interface has a typo in `registerCapabiltiies` (sic). The three handler classes (XphpHoverHandler, XphpDefinitionHandler, XphpCompletionHandler) each inherit / override the typo deliberately — overriding requires matching the name. One-line comment above each method to flag that we're not the source of the typo. Verification: - `make test/lsp` -> OK (121 tests, 276 assertions). No behaviour change. - `make test/lsp/mutation` -> 95 % MSI under the 93 % gate. No new mutation surface introduced. - `make test/unit` -> OK (171 tests, 453 assertions). Core unchanged. - `grep -rn DocumentStore tools/lsp/src tools/lsp/test` -> empty (only the README mention remains, which is the explanatory callout that we DON'T have one anymore). Co-Authored-By: Claude Opus 4.7 (1M context) --- tools/lsp/README.md | 11 ++- tools/lsp/bin/xphp-lsp | 9 +++ .../lsp/src/Handler/XphpCompletionHandler.php | 2 + .../lsp/src/Handler/XphpDefinitionHandler.php | 2 + tools/lsp/src/Handler/XphpHoverHandler.php | 2 + tools/lsp/src/Workspace/DocumentStore.php | 74 ------------------- 6 files changed, 20 insertions(+), 80 deletions(-) delete mode 100644 tools/lsp/src/Workspace/DocumentStore.php diff --git a/tools/lsp/README.md b/tools/lsp/README.md index b6e20a83..fbdbcab6 100644 --- a/tools/lsp/README.md +++ b/tools/lsp/README.md @@ -19,7 +19,7 @@ core parser. | `textDocument/completion` (inside `<…>` type-arg positions) | ✅ shipped | | VS Code extension client at `vscode-extension/` | ✅ shipped | -44 PHPUnit cases, 113 assertions — `make test/lsp`. +121 PHPUnit cases, 276 assertions — `make test/lsp`. See `docs/roadmap.md` (Shipped → Tooling) for the broader feature inventory. @@ -49,10 +49,9 @@ tools/lsp/ │ │ ├── XphpCompletionHandler textDocument/completion │ │ ├── TypeArgPositionDetector backwards-scanner for cursor-in-<…> │ │ └── WorkspaceSymbols collect ClassLike FQNs across open docs -│ └── Workspace/ -│ └── DocumentStore.php in-memory doc cache (phpactor Workspace -│ handles open/change/close itself) -├── test/ PHPUnit suite (44 cases) +│ └── (phpactor's own Workspace handles document open/change/close; no +│ local DocumentStore wrapper needed) +├── test/ PHPUnit suite (121 cases) └── vscode-extension/ VS Code client — spawns server over stdio (F5 dev loop) ``` @@ -101,7 +100,7 @@ Capabilities advertised at `initialize`: ## Test ```bash -make test/lsp # PHPUnit, ~121 cases +make test/lsp # PHPUnit, 121 cases / 276 assertions make test/lsp/mutation # Infection, 95 % MSI under a 93 % gate ``` diff --git a/tools/lsp/bin/xphp-lsp b/tools/lsp/bin/xphp-lsp index 1cc13315..f16f936c 100755 --- a/tools/lsp/bin/xphp-lsp +++ b/tools/lsp/bin/xphp-lsp @@ -14,6 +14,15 @@ declare(strict_types=1); // thecodingmachine/safe, which currently emits many implicit-nullable deprecation // notices under PHP 8.4 at autoload time. Without this, the deprecation chatter // drowns the analyzer's actual output (an LSP client sees this on stderr too). +// +// CAVEAT: this suppression is intentionally broad — it ALSO masks deprecations +// we ourselves might introduce in xphp source under tools/lsp/. The PHPUnit +// surface has a finer-grained equivalent (`` in phpunit.xml.dist) that distinguishes by source path, but the +// runtime PHP API doesn't expose anything equivalent. If we ever start +// emitting deprecations in our own code, revisit this — the easy mitigation +// is to drop the suppression and instead pin phpactor / thecodingmachine/safe +// to versions that have caught up with PHP 8.4. error_reporting(E_ALL & ~E_DEPRECATED); $autoload = null; diff --git a/tools/lsp/src/Handler/XphpCompletionHandler.php b/tools/lsp/src/Handler/XphpCompletionHandler.php index 3ecfc12e..b92aff9b 100644 --- a/tools/lsp/src/Handler/XphpCompletionHandler.php +++ b/tools/lsp/src/Handler/XphpCompletionHandler.php @@ -57,6 +57,8 @@ public function methods(): array ]; } + // `registerCapabiltiies` is misspelled in phpactor's Handler interface (sic). + // We match the typo deliberately — overriding requires the same name. public function registerCapabiltiies(ServerCapabilities $capabilities): void { // `<` is the canonical trigger; `,` lets the next-arg case fire without diff --git a/tools/lsp/src/Handler/XphpDefinitionHandler.php b/tools/lsp/src/Handler/XphpDefinitionHandler.php index 5f4fd5fc..597e053c 100644 --- a/tools/lsp/src/Handler/XphpDefinitionHandler.php +++ b/tools/lsp/src/Handler/XphpDefinitionHandler.php @@ -56,6 +56,8 @@ public function methods(): array ]; } + // `registerCapabiltiies` is misspelled in phpactor's Handler interface (sic). + // We match the typo deliberately — overriding requires the same name. public function registerCapabiltiies(ServerCapabilities $capabilities): void { $capabilities->definitionProvider = true; diff --git a/tools/lsp/src/Handler/XphpHoverHandler.php b/tools/lsp/src/Handler/XphpHoverHandler.php index 7999a877..1dd0ff9b 100644 --- a/tools/lsp/src/Handler/XphpHoverHandler.php +++ b/tools/lsp/src/Handler/XphpHoverHandler.php @@ -58,6 +58,8 @@ public function methods(): array ]; } + // `registerCapabiltiies` is misspelled in phpactor's Handler interface (sic). + // We match the typo deliberately — overriding requires the same name. public function registerCapabiltiies(ServerCapabilities $capabilities): void { $capabilities->hoverProvider = new HoverOptions(); diff --git a/tools/lsp/src/Workspace/DocumentStore.php b/tools/lsp/src/Workspace/DocumentStore.php deleted file mode 100644 index 596be9ea..00000000 --- a/tools/lsp/src/Workspace/DocumentStore.php +++ /dev/null @@ -1,74 +0,0 @@ -}> */ - private array $documents = []; - - public function open(string $uri, string $source): void - { - $this->documents[$uri] = ['source' => $source, 'ast' => null]; - } - - public function change(string $uri, string $source): void - { - if (!isset($this->documents[$uri])) { - $this->open($uri, $source); - return; - } - $this->documents[$uri]['source'] = $source; - $this->documents[$uri]['ast'] = null; // invalidate parsed AST until next analyze - } - - public function close(string $uri): void - { - unset($this->documents[$uri]); - } - - public function source(string $uri): ?string - { - return $this->documents[$uri]['source'] ?? null; - } - - /** - * @param list $ast - */ - public function cacheAst(string $uri, array $ast): void - { - if (!isset($this->documents[$uri])) { - return; - } - $this->documents[$uri]['ast'] = $ast; - } - - /** - * @return list|null - */ - public function ast(string $uri): ?array - { - return $this->documents[$uri]['ast'] ?? null; - } - - /** - * @return list - */ - public function openUris(): array - { - return array_keys($this->documents); - } -} From 32c64abe8a3a166f89923dece45af7fd099d1c2c Mon Sep 17 00:00:00 2001 From: Matheus Martins Date: Thu, 21 May 2026 22:30:01 +0000 Subject: [PATCH 14/17] lsp: split bound violations from hash collisions in diagnostic codes (S3, M1) Fixes a miscoding bug from the review: `WorkspaceAnalyzer::walkInstantiations` hard-coded `'xphp.bound'` on every RuntimeException from `Registry::recordInstantiation`. That method actually throws on two distinct paths -- bound violations AND hash collisions. Surfacing a collision as `xphp.bound` pointed users at the wrong fix (look at bound declarations vs. raise XPHP_HASH_LENGTH). Same commit also introduces the `DiagnosticCode` enum to centralise the error-category catalog and eliminate string-literal scatter across catch blocks. Pieces: - new `Analyzer\DiagnosticCode` enum (string-backed): Parse, ParseInternal, Definition, BoundViolation, HashCollision. Backed values match the LSP wire format ("xphp.parse" etc.) so handlers pass `->code->value` straight through with no translation. - new static helper `DiagnosticCode::fromRegistryRecordInstantiationException()` inspects the message's leading phrase ("Hash collision" -> the new code, else BoundViolation). Centralises the message-prefix triage so future Registry exception paths slot in here rather than smearing magic strings across catch sites. Conservative fallback: unknown phrasing maps to BoundViolation (more likely + visually benign) rather than silently dropping the diagnostic. - Diagnostic value type: `string $code = ''` -> `DiagnosticCode $code` (typed, required). No more empty-string sentinel; every diagnostic declares its category up front. - DiagnosticTranslator: now reads `$d->code->value` for the LSP wire `code` field. The empty-string guard is gone -- the enum guarantees a value. - Analyzer + WorkspaceAnalyzer: string literals at each catch site replaced with enum cases. The `walkInstantiations` catch (the S3 site) calls the triage helper instead of hard-coding. - mechanical test updates: AnalyzerTest, WorkspaceAnalyzerTest assert on the enum case. XphpDiagnosticsProviderTest's wire-format string checks ('xphp.bound', 'xphp.parse') stay as strings because that's the LSP wire shape from the translated phpactor Diagnostic. - new DiagnosticCodeTest covers the triage helper: hash-collision phrasing -> HashCollision, bound-violation phrasing -> BoundViolation, unknown phrasing -> BoundViolation (the conservative fallback), and the backed-string values lock the LSP wire format against accidental renames. Verification: - `make test/lsp` -> OK (125 tests, 284 assertions; +4 from the new DiagnosticCodeTest). - `make test/lsp/mutation` -> 95 % MSI (the new enum-branching surface is +2 mutations, both killed by the triage tests). - `make test/unit` -> unchanged (171/453). Co-Authored-By: Claude Opus 4.7 (1M context) --- tools/lsp/src/Analyzer/Analyzer.php | 6 +- tools/lsp/src/Analyzer/Diagnostic.php | 8 +- tools/lsp/src/Analyzer/DiagnosticCode.php | 60 +++++++++++++++ tools/lsp/src/Analyzer/WorkspaceAnalyzer.php | 11 ++- .../src/Diagnostics/DiagnosticTranslator.php | 2 +- tools/lsp/test/Analyzer/AnalyzerTest.php | 3 +- .../lsp/test/Analyzer/DiagnosticCodeTest.php | 75 +++++++++++++++++++ .../test/Analyzer/WorkspaceAnalyzerTest.php | 5 +- 8 files changed, 158 insertions(+), 12 deletions(-) create mode 100644 tools/lsp/src/Analyzer/DiagnosticCode.php create mode 100644 tools/lsp/test/Analyzer/DiagnosticCodeTest.php diff --git a/tools/lsp/src/Analyzer/Analyzer.php b/tools/lsp/src/Analyzer/Analyzer.php index 28016c03..f2aaf3e8 100644 --- a/tools/lsp/src/Analyzer/Analyzer.php +++ b/tools/lsp/src/Analyzer/Analyzer.php @@ -41,7 +41,7 @@ public function analyzeFile(string $source): ParseResult diagnostics: [self::buildLineDiagnostic( $positionMap, $e->getStartLine(), - 'xphp.parse', + DiagnosticCode::Parse, 'Syntax error: ' . $e->getRawMessage(), )], ); @@ -54,7 +54,7 @@ public function analyzeFile(string $source): ParseResult diagnostics: [self::buildLineDiagnostic( $positionMap, 1, - 'xphp.parse.internal', + DiagnosticCode::ParseInternal, $e->getMessage(), )], ); @@ -64,7 +64,7 @@ public function analyzeFile(string $source): ParseResult private static function buildLineDiagnostic( PositionMap $positionMap, int $nikicLine, - string $code, + DiagnosticCode $code, string $message, ): Diagnostic { [$startLine, $startChar, $endLine, $endChar] = $positionMap->fullLineRangeFromNikic($nikicLine); diff --git a/tools/lsp/src/Analyzer/Diagnostic.php b/tools/lsp/src/Analyzer/Diagnostic.php index 6c79eac5..896bbb9a 100644 --- a/tools/lsp/src/Analyzer/Diagnostic.php +++ b/tools/lsp/src/Analyzer/Diagnostic.php @@ -10,6 +10,11 @@ * Handlers translate this to phpactor/language-server-protocol's Diagnostic at the * wire boundary. Keeping the analyzer output framework-free means the test suite * can assert against plain PHP objects without spinning up an LSP transport. + * + * `$code` is required and typed: every diagnostic declares its category up + * front so editors / users can pattern-match by code (e.g. "downgrade + * xphp.parse.internal to info"). The previous freeform `string $code = ''` + * encouraged inconsistency between catch sites. */ final readonly class Diagnostic { @@ -19,9 +24,8 @@ public function __construct( public int $endLine, public int $endCharacter, public string $message, + public DiagnosticCode $code, public DiagnosticSeverity $severity = DiagnosticSeverity::Error, - /** Short identifier surfaced as the LSP "code" field; matches our error catalog. */ - public string $code = '', ) { } } diff --git a/tools/lsp/src/Analyzer/DiagnosticCode.php b/tools/lsp/src/Analyzer/DiagnosticCode.php new file mode 100644 index 00000000..c2ce8669 --- /dev/null +++ b/tools/lsp/src/Analyzer/DiagnosticCode.php @@ -0,0 +1,60 @@ +value` + * straight through to phpactor's Diagnostic without translation. + * + * Codes intentionally use dotted notation (`xphp.[.]`) + * so an editor's code-aware config can pattern-match (e.g. "downgrade + * xphp.parse.internal to info") without parsing message text. + * + * `fromRegistryRecordInstantiationException` is the central place that decides + * which code applies to a `RuntimeException` thrown from + * `Registry::recordInstantiation`. That method has two distinct error paths + * (bound violation vs. hash collision) which were previously both surfaced as + * `xphp.bound`. Mis-coding a collision as a bound violation pointed users at + * the wrong fix (look at bounds vs. raise XPHP_HASH_LENGTH). Centralising the + * triage here means future Registry exception paths slot in without + * smearing magic strings across catch blocks. + */ +enum DiagnosticCode: string +{ + /** nikic/php-parser threw — the source isn't valid PHP after angle-stripping. */ + case Parse = 'xphp.parse'; + + /** XphpSourceParser threw RuntimeException — internal contract violation, rare. */ + case ParseInternal = 'xphp.parse.internal'; + + /** Registry::recordDefinition rejected a duplicate template declaration. */ + case Definition = 'xphp.definition'; + + /** Registry::recordInstantiation rejected an arg list against its declared bound. */ + case BoundViolation = 'xphp.bound'; + + /** Two distinct instantiations hashed to the same generated FQCN — raise XPHP_HASH_LENGTH. */ + case HashCollision = 'xphp.collision'; + + /** + * Map a RuntimeException raised by Registry::recordInstantiation to its + * diagnostic code. The Registry doesn't (currently) use a typed exception + * hierarchy, so we triage by the error message's leading phrase. The + * Registry's error builders (Registry::collisionMessage, + * Registry::validateBounds) use stable prefixes documented in their + * docblocks — if those phrasings shift, this triage breaks and the bound + * fallback kicks in. + */ + public static function fromRegistryRecordInstantiationException(RuntimeException $e): self + { + if (str_starts_with($e->getMessage(), 'Hash collision')) { + return self::HashCollision; + } + return self::BoundViolation; + } +} diff --git a/tools/lsp/src/Analyzer/WorkspaceAnalyzer.php b/tools/lsp/src/Analyzer/WorkspaceAnalyzer.php index ad6c99f5..0108a317 100644 --- a/tools/lsp/src/Analyzer/WorkspaceAnalyzer.php +++ b/tools/lsp/src/Analyzer/WorkspaceAnalyzer.php @@ -104,12 +104,12 @@ public function enterNode(Node $node): null $this->sourceFile, ); } catch (RuntimeException $e) { - $this->diagnostics[] = self::buildDiagnostic($this->positionMap, $node->getStartLine(), 'xphp.definition', $e->getMessage()); + $this->diagnostics[] = self::buildDiagnostic($this->positionMap, $node->getStartLine(), DiagnosticCode::Definition, $e->getMessage()); } return null; } - private static function buildDiagnostic(PositionMap $positionMap, int $nikicLine, string $code, string $message): Diagnostic + private static function buildDiagnostic(PositionMap $positionMap, int $nikicLine, DiagnosticCode $code, string $message): Diagnostic { [$sl, $sc, $el, $ec] = $positionMap->fullLineRangeFromNikic($nikicLine); return new Diagnostic($sl, $sc, $el, $ec, $message, code: $code); @@ -165,7 +165,12 @@ public function enterNode(Node $node): null endLine: $el, endCharacter: $ec, message: $e->getMessage(), - code: 'xphp.bound', + // Registry::recordInstantiation has two error paths + // (bound violation vs. hash collision). The triage helper + // distinguishes them by the message's leading phrase so + // editors / users can act on the right hint (raise + // XPHP_HASH_LENGTH vs. fix the bound). + code: DiagnosticCode::fromRegistryRecordInstantiationException($e), ); } return null; diff --git a/tools/lsp/src/Diagnostics/DiagnosticTranslator.php b/tools/lsp/src/Diagnostics/DiagnosticTranslator.php index 4a1a4de2..234db7a5 100644 --- a/tools/lsp/src/Diagnostics/DiagnosticTranslator.php +++ b/tools/lsp/src/Diagnostics/DiagnosticTranslator.php @@ -26,7 +26,7 @@ public static function toLsp(Diagnostic $d): LspDiagnostic message: $d->message, ); $lsp->severity = $d->severity->value; - $lsp->code = $d->code !== '' ? $d->code : null; + $lsp->code = $d->code->value; $lsp->source = 'xphp'; return $lsp; } diff --git a/tools/lsp/test/Analyzer/AnalyzerTest.php b/tools/lsp/test/Analyzer/AnalyzerTest.php index 84ab7124..03e16cd9 100644 --- a/tools/lsp/test/Analyzer/AnalyzerTest.php +++ b/tools/lsp/test/Analyzer/AnalyzerTest.php @@ -7,6 +7,7 @@ use PhpParser\ParserFactory; use PHPUnit\Framework\TestCase; use XPHP\Lsp\Analyzer\Analyzer; +use XPHP\Lsp\Analyzer\DiagnosticCode; use XPHP\Lsp\Analyzer\DiagnosticSeverity; use XPHP\Transpiler\Monomorphize\XphpSourceParser; @@ -39,7 +40,7 @@ public function testSyntaxErrorProducesDiagnosticWithNullAst(): void self::assertNull($result->ast, 'unrecoverable syntax error should null out the AST'); self::assertCount(1, $result->diagnostics); - self::assertSame('xphp.parse', $result->diagnostics[0]->code); + self::assertSame(DiagnosticCode::Parse, $result->diagnostics[0]->code); self::assertSame(DiagnosticSeverity::Error, $result->diagnostics[0]->severity); // The message MUST carry both the literal "Syntax error: " prefix AND // the parser's own error description. Locks the Concat mutation that diff --git a/tools/lsp/test/Analyzer/DiagnosticCodeTest.php b/tools/lsp/test/Analyzer/DiagnosticCodeTest.php new file mode 100644 index 00000000..fa4c7842 --- /dev/null +++ b/tools/lsp/test/Analyzer/DiagnosticCodeTest.php @@ -0,0 +1,75 @@ +.\n" + . ' type parameter T is bounded by Stringable...' + ); + + self::assertSame( + DiagnosticCode::BoundViolation, + DiagnosticCode::fromRegistryRecordInstantiationException($e), + ); + } + + public function testUnknownPrefixFallsBackToBoundViolation(): void + { + // Conservative default: an unfamiliar message phrasing is more likely + // to be a bound issue than a collision (the latter is rare and + // intentionally distinct in its phrasing). The fallback keeps the + // diagnostic from looking dropped while flagging that Registry's + // error catalog has grown a path we didn't categorise. + $e = new RuntimeException('Something unexpected happened.'); + + self::assertSame( + DiagnosticCode::BoundViolation, + DiagnosticCode::fromRegistryRecordInstantiationException($e), + ); + } + + public function testEachCodeMapsToTheLspWireString(): void + { + // Locks the backed-enum values that the LSP wire format depends on + // (via DiagnosticTranslator). Renaming a case without updating the + // backed value would silently break editor-side code-pattern + // matching ("xphp.bound" -> "xphp.boundViolation" e.g.). + self::assertSame('xphp.parse', DiagnosticCode::Parse->value); + self::assertSame('xphp.parse.internal', DiagnosticCode::ParseInternal->value); + self::assertSame('xphp.definition', DiagnosticCode::Definition->value); + self::assertSame('xphp.bound', DiagnosticCode::BoundViolation->value); + self::assertSame('xphp.collision', DiagnosticCode::HashCollision->value); + } +} diff --git a/tools/lsp/test/Analyzer/WorkspaceAnalyzerTest.php b/tools/lsp/test/Analyzer/WorkspaceAnalyzerTest.php index 4f57cef3..1d2f7afa 100644 --- a/tools/lsp/test/Analyzer/WorkspaceAnalyzerTest.php +++ b/tools/lsp/test/Analyzer/WorkspaceAnalyzerTest.php @@ -7,6 +7,7 @@ use PhpParser\ParserFactory; use PHPUnit\Framework\TestCase; use XPHP\Lsp\Analyzer\Analyzer; +use XPHP\Lsp\Analyzer\DiagnosticCode; use XPHP\Lsp\Analyzer\WorkspaceAnalyzer; use XPHP\Transpiler\Monomorphize\XphpSourceParser; @@ -35,7 +36,7 @@ class Box self::assertSame([], $diagnostics['/Box.xphp'], 'template file itself has no violation'); self::assertCount(1, $diagnostics['/Use.xphp'], 'instantiation file should carry one bound-violation diagnostic'); self::assertStringContainsString('Generic bound violated', $diagnostics['/Use.xphp'][0]->message); - self::assertSame('xphp.bound', $diagnostics['/Use.xphp'][0]->code); + self::assertSame(DiagnosticCode::BoundViolation, $diagnostics['/Use.xphp'][0]->code); } public function testBoundViolationOnUnknownClassReportsDistinctMessage(): void @@ -188,7 +189,7 @@ class Box { public T $item; } self::assertSame([], $diagnostics['/BoxOne.xphp']); self::assertCount(1, $diagnostics['/BoxTwo.xphp']); self::assertStringContainsString('already declared', $diagnostics['/BoxTwo.xphp'][0]->message); - self::assertSame('xphp.definition', $diagnostics['/BoxTwo.xphp'][0]->code); + self::assertSame(DiagnosticCode::Definition, $diagnostics['/BoxTwo.xphp'][0]->code); } /** From 935d47f833fc77b4fc0625b70b0157cd89620d70 Mon Sep 17 00:00:00 2001 From: Matheus Martins Date: Thu, 21 May 2026 22:36:28 +0000 Subject: [PATCH 15/17] lsp: version-keyed AST cache for handler reuse (S4, N3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eliminates redundant reparses in the LSP request path. The handlers (hover, definition, completion, diagnostics) used to call `Analyzer::analyzeFile($item->text)` directly on every request, walking the parser tree for every open document every time. With N open documents, every keystroke that triggered cross-file work (definition, completion, diagnostics workspace pass) ran N parses — O(N) per LSP event. nikic is fast (~ms per file) so this was acceptable at the MVP's small-workspace scale, but the reviewer flagged completion's fire-on-every-`<` as the obvious next bottleneck. Pieces: - new `Analyzer\ParsedDocumentCache` — keyed by URI, holds `array`. Public surface: - `getOrParse(uri, version, source): ParseResult` — cache hit on matching version, miss otherwise. - `forget(uri): void` — drops the entry on `didClose` (well-behaved long sessions won't grow unboundedly). Invalidation is by-version only. LSP gives us `TextDocumentItem::version` for free; phpactor's `Workspace` bumps it on every `didChange`, so the next `getOrParse` after an edit sees a new version and reparses. - `LspDispatcherFactory` instantiates ONE cache per session, threads it through: - `XphpDiagnosticsProvider` - `XphpHoverHandler` - `XphpDefinitionHandler` - `XphpCompletionHandler` (via `WorkspaceSymbols`) - `WorkspaceSymbols` All five sites now receive `ParsedDocumentCache` in lieu of `Analyzer`. The shared cache means a hover, then a definition, then a completion against the same unchanged document share a single parse result. - `Analyzer` loses the `final readonly` qualifiers (becomes a plain class with a readonly $parser property). Required for the cache test's spy subclass — readonly-class inheritance disallows non-readonly properties (the mutable $callCount). Functionally the immutability guarantee on $parser is preserved; the wider class-level readonly was load-bearing for nothing. - new `ParsedDocumentCacheTest` covers: - first access parses + caches; same-version re-access serves cached - version bump invalidates → reparse - distinct URIs cached independently - `forget(uri)` drops the entry - `forget` on unknown URI is a no-op The test uses an anonymous-class subclass of `Analyzer` as a spy counting `analyzeFile` invocations — lighter than PHPUnit's native mocking, reads exactly as the assertion it backs. - mechanical updates to existing handler tests: each handler's helper that constructed `new Analyzer(...)` now constructs `new ParsedDocumentCache(new Analyzer(...))`. No test behaviour changes. Verification: - `make test/lsp` -> OK (130 tests, 292 assertions; +5 from ParsedDocumentCacheTest). - `make test/lsp/mutation` -> 95 % MSI under the 93 % gate. The new cache surface adds 9 mutations, all killed. - `make test/unit` -> unchanged (171/453). Co-Authored-By: Claude Opus 4.7 (1M context) --- tools/lsp/src/Analyzer/Analyzer.php | 4 +- .../lsp/src/Analyzer/ParsedDocumentCache.php | 47 ++++++++++ .../Diagnostics/XphpDiagnosticsProvider.php | 16 +++- tools/lsp/src/Handler/WorkspaceSymbols.php | 14 +-- .../lsp/src/Handler/XphpDefinitionHandler.php | 12 ++- tools/lsp/src/Handler/XphpHoverHandler.php | 6 +- tools/lsp/src/LspDispatcherFactory.php | 15 ++- .../test/Analyzer/ParsedDocumentCacheTest.php | 92 +++++++++++++++++++ .../XphpDiagnosticsProviderTest.php | 3 +- .../lsp/test/Handler/WorkspaceSymbolsTest.php | 7 +- .../Handler/XphpCompletionHandlerTest.php | 17 ++-- .../Handler/XphpDefinitionHandlerTest.php | 5 +- .../lsp/test/Handler/XphpHoverHandlerTest.php | 13 ++- 13 files changed, 209 insertions(+), 42 deletions(-) create mode 100644 tools/lsp/src/Analyzer/ParsedDocumentCache.php create mode 100644 tools/lsp/test/Analyzer/ParsedDocumentCacheTest.php diff --git a/tools/lsp/src/Analyzer/Analyzer.php b/tools/lsp/src/Analyzer/Analyzer.php index f2aaf3e8..2a5c663e 100644 --- a/tools/lsp/src/Analyzer/Analyzer.php +++ b/tools/lsp/src/Analyzer/Analyzer.php @@ -19,9 +19,9 @@ * are layered on top of this in `WorkspaceAnalyzer`; the per-file analyzer here * handles only what's local to a single document. */ -final readonly class Analyzer +class Analyzer { - public function __construct(private XphpSourceParser $parser) + public function __construct(private readonly XphpSourceParser $parser) { } diff --git a/tools/lsp/src/Analyzer/ParsedDocumentCache.php b/tools/lsp/src/Analyzer/ParsedDocumentCache.php new file mode 100644 index 00000000..f46cb2bf --- /dev/null +++ b/tools/lsp/src/Analyzer/ParsedDocumentCache.php @@ -0,0 +1,47 @@ +text)` directly on + * every LSP request, re-parsing every open document every time. On a workspace + * with N open docs that's O(N) parses per keystroke. nikic is fast (~ms per + * file) so this was acceptable for small workspaces but degrades quickly past + * 10-20 open documents — and completion fires this loop on every `<`. + * + * Cache invalidation is by-version only. LSP gives us `TextDocumentItem::version` + * for free — phpactor bumps it on `didChange`. The next `getOrParse()` after + * a change sees the new version and reparses; otherwise we serve from cache. + * No explicit invalidation on didOpen/didChange is needed. + * + * `forget()` exists for `didClose`: drops the URI from the cache so the LSP + * session doesn't grow unbounded across long editor sessions. + */ +final class ParsedDocumentCache +{ + /** @var array */ + private array $entries = []; + + public function __construct(private readonly Analyzer $analyzer) + { + } + + public function getOrParse(string $uri, int $version, string $source): ParseResult + { + $cached = $this->entries[$uri] ?? null; + if ($cached !== null && $cached['version'] === $version) { + return $cached['result']; + } + $result = $this->analyzer->analyzeFile($source); + $this->entries[$uri] = ['version' => $version, 'result' => $result]; + return $result; + } + + public function forget(string $uri): void + { + unset($this->entries[$uri]); + } +} diff --git a/tools/lsp/src/Diagnostics/XphpDiagnosticsProvider.php b/tools/lsp/src/Diagnostics/XphpDiagnosticsProvider.php index 45c160e1..825db44a 100644 --- a/tools/lsp/src/Diagnostics/XphpDiagnosticsProvider.php +++ b/tools/lsp/src/Diagnostics/XphpDiagnosticsProvider.php @@ -11,7 +11,7 @@ use Phpactor\LanguageServer\Core\Workspace\Workspace as PhpactorWorkspace; use Phpactor\LanguageServerProtocol\Diagnostic as LspDiagnostic; use Phpactor\LanguageServerProtocol\TextDocumentItem; -use XPHP\Lsp\Analyzer\Analyzer; +use XPHP\Lsp\Analyzer\ParsedDocumentCache; use XPHP\Lsp\Analyzer\WorkspaceAnalyzer; /** @@ -36,7 +36,7 @@ final class XphpDiagnosticsProvider implements DiagnosticsProvider { public function __construct( - private readonly Analyzer $analyzer, + private readonly ParsedDocumentCache $cache, private readonly WorkspaceAnalyzer $workspaceAnalyzer, private readonly PhpactorWorkspace $workspace, ) { @@ -60,8 +60,14 @@ private function analyze(TextDocumentItem $textDocument): array { $currentUri = $textDocument->uri; - // Per-file syntax pass on the document being linted. - $currentResult = $this->analyzer->analyzeFile($textDocument->text); + // Per-file syntax pass on the document being linted. Cache-keyed by + // (uri, version) so subsequent hover/definition/completion calls + // against the same unchanged document don't reparse. + $currentResult = $this->cache->getOrParse( + $currentUri, + $textDocument->version, + $textDocument->text, + ); $perFileDiagnostics = array_map( static fn ($d) => DiagnosticTranslator::toLsp($d), $currentResult->diagnostics, @@ -83,7 +89,7 @@ private function analyze(TextDocumentItem $textDocument): array if ($uri === $currentUri) { continue; } - $otherResult = $this->analyzer->analyzeFile($item->text); + $otherResult = $this->cache->getOrParse($uri, $item->version, $item->text); if ($otherResult->ast === null) { continue; } diff --git a/tools/lsp/src/Handler/WorkspaceSymbols.php b/tools/lsp/src/Handler/WorkspaceSymbols.php index 6e788ed7..a6297827 100644 --- a/tools/lsp/src/Handler/WorkspaceSymbols.php +++ b/tools/lsp/src/Handler/WorkspaceSymbols.php @@ -10,22 +10,22 @@ use PhpParser\NodeTraverser; use PhpParser\NodeVisitorAbstract; use Phpactor\LanguageServer\Core\Workspace\Workspace as PhpactorWorkspace; -use XPHP\Lsp\Analyzer\Analyzer; +use XPHP\Lsp\Analyzer\ParsedDocumentCache; /** * Walks every open document and collects the FQNs of every ClassLike (class, * interface, trait). Used by the completion handler to suggest candidates * inside `<…>` type-arg positions. * - * Re-computed on each call for the MVP — cheap because the analyzer is fast - * and the open workspace is typically small. Caching keyed on doc version is - * a follow-up if profiling shows it matters. + * Parses via the shared `ParsedDocumentCache` so an unchanged workspace + * doesn't re-parse on every completion keystroke (the original MVP did, + * which is O(N) parses per `<`). */ final readonly class WorkspaceSymbols { public function __construct( private PhpactorWorkspace $workspace, - private Analyzer $analyzer, + private ParsedDocumentCache $cache, ) { } @@ -35,8 +35,8 @@ public function __construct( public function allClassFqns(): array { $fqns = []; - foreach ($this->workspace as $item) { - $result = $this->analyzer->analyzeFile($item->text); + foreach ($this->workspace as $uri => $item) { + $result = $this->cache->getOrParse($uri, $item->version, $item->text); if ($result->ast === null) { continue; } diff --git a/tools/lsp/src/Handler/XphpDefinitionHandler.php b/tools/lsp/src/Handler/XphpDefinitionHandler.php index 597e053c..8a5c6acf 100644 --- a/tools/lsp/src/Handler/XphpDefinitionHandler.php +++ b/tools/lsp/src/Handler/XphpDefinitionHandler.php @@ -18,7 +18,7 @@ use Phpactor\LanguageServerProtocol\Position; use Phpactor\LanguageServerProtocol\Range; use Phpactor\LanguageServerProtocol\ServerCapabilities; -use XPHP\Lsp\Analyzer\Analyzer; +use XPHP\Lsp\Analyzer\ParsedDocumentCache; use XPHP\Lsp\PositionMap; use XPHP\Transpiler\Monomorphize\XphpSourceParser; @@ -45,7 +45,7 @@ final class XphpDefinitionHandler implements Handler, CanRegisterCapabilities { public function __construct( private readonly PhpactorWorkspace $workspace, - private readonly Analyzer $analyzer, + private readonly ParsedDocumentCache $cache, ) { } @@ -72,7 +72,11 @@ public function definition(DefinitionParams $params): Promise return new Success(null); } $currentItem = $this->workspace->get($params->textDocument->uri); - $currentResult = $this->analyzer->analyzeFile($currentItem->text); + $currentResult = $this->cache->getOrParse( + $params->textDocument->uri, + $currentItem->version, + $currentItem->text, + ); if ($currentResult->ast === null) { return new Success(null); } @@ -97,7 +101,7 @@ public function definition(DefinitionParams $params): Promise private function findDefinitionAcrossWorkspace(string $templateFqn): ?Location { foreach ($this->workspace as $uri => $item) { - $result = $this->analyzer->analyzeFile($item->text); + $result = $this->cache->getOrParse($uri, $item->version, $item->text); if ($result->ast === null) { continue; } diff --git a/tools/lsp/src/Handler/XphpHoverHandler.php b/tools/lsp/src/Handler/XphpHoverHandler.php index 1dd0ff9b..1f29e60d 100644 --- a/tools/lsp/src/Handler/XphpHoverHandler.php +++ b/tools/lsp/src/Handler/XphpHoverHandler.php @@ -16,7 +16,7 @@ use Phpactor\LanguageServerProtocol\MarkupContent; use Phpactor\LanguageServerProtocol\MarkupKind; use Phpactor\LanguageServerProtocol\ServerCapabilities; -use XPHP\Lsp\Analyzer\Analyzer; +use XPHP\Lsp\Analyzer\ParsedDocumentCache; use XPHP\Lsp\PositionMap; use XPHP\Transpiler\Monomorphize\Registry; use XPHP\Transpiler\Monomorphize\TypeParam; @@ -47,7 +47,7 @@ final class XphpHoverHandler implements Handler, CanRegisterCapabilities { public function __construct( private readonly PhpactorWorkspace $workspace, - private readonly Analyzer $analyzer, + private readonly ParsedDocumentCache $cache, ) { } @@ -74,7 +74,7 @@ public function hover(HoverParams $params): Promise return new Success(null); } $item = $this->workspace->get($params->textDocument->uri); - $result = $this->analyzer->analyzeFile($item->text); + $result = $this->cache->getOrParse($params->textDocument->uri, $item->version, $item->text); if ($result->ast === null) { return new Success(null); } diff --git a/tools/lsp/src/LspDispatcherFactory.php b/tools/lsp/src/LspDispatcherFactory.php index 54526404..2ec2a639 100644 --- a/tools/lsp/src/LspDispatcherFactory.php +++ b/tools/lsp/src/LspDispatcherFactory.php @@ -40,6 +40,7 @@ use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; use XPHP\Lsp\Analyzer\Analyzer; +use XPHP\Lsp\Analyzer\ParsedDocumentCache; use XPHP\Lsp\Analyzer\WorkspaceAnalyzer; use XPHP\Lsp\Diagnostics\XphpDiagnosticsProvider; use XPHP\Lsp\Handler\WorkspaceSymbols; @@ -79,11 +80,15 @@ public function create(MessageTransmitter $transmitter, InitializeParams $initia $clientApi = new ClientApi(new JsonRpcClient($transmitter, $responseWatcher)); $workspace = new PhpactorWorkspace($this->logger); - // Shared analyzer instance: stateless, so handlers can share it safely. + // Shared analyzer + version-keyed AST cache, scoped to this LSP session. + // Every handler (hover, definition, completion, diagnostics) reads + // through the cache so a workspace pass costs O(unchanged docs serves + // from cache) rather than O(N parses per keystroke). $analyzer = new Analyzer(new XphpSourceParser((new ParserFactory())->createForHostVersion())); + $cache = new ParsedDocumentCache($analyzer); $diagnosticsProvider = new XphpDiagnosticsProvider( - $analyzer, + $cache, new WorkspaceAnalyzer(), $workspace, ); @@ -114,9 +119,9 @@ public function create(MessageTransmitter $transmitter, InitializeParams $initia new ServiceHandler($serviceManager, $clientApi), new CommandHandler(new CommandDispatcher([])), new ExitHandler(), - new XphpHoverHandler($workspace, $analyzer), - new XphpDefinitionHandler($workspace, $analyzer), - new XphpCompletionHandler($workspace, new WorkspaceSymbols($workspace, $analyzer)), + new XphpHoverHandler($workspace, $cache), + new XphpDefinitionHandler($workspace, $cache), + new XphpCompletionHandler($workspace, new WorkspaceSymbols($workspace, $cache)), ); $runner = new HandlerMethodRunner( diff --git a/tools/lsp/test/Analyzer/ParsedDocumentCacheTest.php b/tools/lsp/test/Analyzer/ParsedDocumentCacheTest.php new file mode 100644 index 00000000..79649a0b --- /dev/null +++ b/tools/lsp/test/Analyzer/ParsedDocumentCacheTest.php @@ -0,0 +1,92 @@ +newSpy(); + $cache = new ParsedDocumentCache($spy); + + $cache->getOrParse('/a.xphp', 1, 'callCount, 'first access parses'); + + $cache->getOrParse('/a.xphp', 1, 'callCount, 'same version → cached, no second parse'); + } + + public function testVersionBumpInvalidatesCachedEntry(): void + { + $spy = $this->newSpy(); + $cache = new ParsedDocumentCache($spy); + + $cache->getOrParse('/a.xphp', 1, 'getOrParse('/a.xphp', 2, 'callCount, 'version bump must reparse'); + + // Same new version doesn't reparse again. + $cache->getOrParse('/a.xphp', 2, 'callCount); + } + + public function testDistinctUrisAreCachedIndependently(): void + { + $spy = $this->newSpy(); + $cache = new ParsedDocumentCache($spy); + + $cache->getOrParse('/a.xphp', 1, 'getOrParse('/b.xphp', 1, 'callCount, 'distinct URIs each get parsed once'); + + $cache->getOrParse('/a.xphp', 1, 'getOrParse('/b.xphp', 1, 'callCount, 'both URIs serve cached results'); + } + + public function testForgetDropsTheEntryAndForcesReparseOnNextAccess(): void + { + $spy = $this->newSpy(); + $cache = new ParsedDocumentCache($spy); + + $cache->getOrParse('/a.xphp', 1, 'forget('/a.xphp'); + $cache->getOrParse('/a.xphp', 1, 'callCount, 'forget invalidates → reparse on next access'); + } + + public function testForgetOnUnknownUriIsANoop(): void + { + $spy = $this->newSpy(); + $cache = new ParsedDocumentCache($spy); + $cache->forget('/never-cached.xphp'); + self::assertSame(0, $spy->callCount); + } + + /** + * Analyzer subclass that counts analyzeFile() invocations. PHPUnit's + * native mocking infrastructure could express the same shape but with + * heavier setup; a tiny anonymous-class spy is the path of least + * resistance and reads like the assertion it backs. + */ + private function newSpy(): Analyzer + { + return new class(new XphpSourceParser((new ParserFactory())->createForHostVersion())) extends Analyzer { + public int $callCount = 0; + + public function analyzeFile(string $source): \XPHP\Lsp\Analyzer\ParseResult + { + $this->callCount++; + return parent::analyzeFile($source); + } + }; + } +} diff --git a/tools/lsp/test/Diagnostics/XphpDiagnosticsProviderTest.php b/tools/lsp/test/Diagnostics/XphpDiagnosticsProviderTest.php index 79ed22d0..883e968c 100644 --- a/tools/lsp/test/Diagnostics/XphpDiagnosticsProviderTest.php +++ b/tools/lsp/test/Diagnostics/XphpDiagnosticsProviderTest.php @@ -11,6 +11,7 @@ use Phpactor\LanguageServerProtocol\TextDocumentItem; use PHPUnit\Framework\TestCase; use XPHP\Lsp\Analyzer\Analyzer; +use XPHP\Lsp\Analyzer\ParsedDocumentCache; use XPHP\Lsp\Analyzer\WorkspaceAnalyzer; use XPHP\Lsp\Diagnostics\XphpDiagnosticsProvider; use XPHP\Transpiler\Monomorphize\XphpSourceParser; @@ -177,7 +178,7 @@ private function newProvider(PhpactorWorkspace $workspace): XphpDiagnosticsProvi { $parser = new XphpSourceParser((new ParserFactory())->createForHostVersion()); return new XphpDiagnosticsProvider( - new Analyzer($parser), + new ParsedDocumentCache(new Analyzer($parser)), new WorkspaceAnalyzer(), $workspace, ); diff --git a/tools/lsp/test/Handler/WorkspaceSymbolsTest.php b/tools/lsp/test/Handler/WorkspaceSymbolsTest.php index 2d45932c..db75e3d9 100644 --- a/tools/lsp/test/Handler/WorkspaceSymbolsTest.php +++ b/tools/lsp/test/Handler/WorkspaceSymbolsTest.php @@ -9,6 +9,7 @@ use Phpactor\LanguageServerProtocol\TextDocumentItem; use PHPUnit\Framework\TestCase; use XPHP\Lsp\Analyzer\Analyzer; +use XPHP\Lsp\Analyzer\ParsedDocumentCache; use XPHP\Lsp\Handler\WorkspaceSymbols; use XPHP\Transpiler\Monomorphize\XphpSourceParser; @@ -101,7 +102,9 @@ class Loose {} private function newSymbols(PhpactorWorkspace $workspace): WorkspaceSymbols { - $analyzer = new Analyzer(new XphpSourceParser((new ParserFactory())->createForHostVersion())); - return new WorkspaceSymbols($workspace, $analyzer); + $cache = new ParsedDocumentCache( + new Analyzer(new XphpSourceParser((new ParserFactory())->createForHostVersion())), + ); + return new WorkspaceSymbols($workspace, $cache); } } diff --git a/tools/lsp/test/Handler/XphpCompletionHandlerTest.php b/tools/lsp/test/Handler/XphpCompletionHandlerTest.php index c708ab49..59e64cf2 100644 --- a/tools/lsp/test/Handler/XphpCompletionHandlerTest.php +++ b/tools/lsp/test/Handler/XphpCompletionHandlerTest.php @@ -15,6 +15,7 @@ use Phpactor\LanguageServerProtocol\TextDocumentItem; use PHPUnit\Framework\TestCase; use XPHP\Lsp\Analyzer\Analyzer; +use XPHP\Lsp\Analyzer\ParsedDocumentCache; use XPHP\Lsp\Handler\WorkspaceSymbols; use XPHP\Lsp\Handler\XphpCompletionHandler; use XPHP\Lsp\PositionMap; @@ -88,7 +89,7 @@ public function testUnknownUriYieldsEmptyList(): void $workspace = new PhpactorWorkspace(); $handler = new XphpCompletionHandler( $workspace, - new WorkspaceSymbols($workspace, $this->newAnalyzer()), + new WorkspaceSymbols($workspace, $this->newCache()), ); $params = new CompletionParams( new TextDocumentIdentifier('/never-opened.xphp'), @@ -208,7 +209,7 @@ public function testCompletionAdvertisesCorrectTriggerCharacters(): void $capabilities = new \Phpactor\LanguageServerProtocol\ServerCapabilities(); (new XphpCompletionHandler( new PhpactorWorkspace(), - new WorkspaceSymbols(new PhpactorWorkspace(), $this->newAnalyzer()), + new WorkspaceSymbols(new PhpactorWorkspace(), $this->newCache()), ))->registerCapabiltiies($capabilities); self::assertNotNull($capabilities->completionProvider); @@ -224,7 +225,7 @@ public function testCompletionMethodsMapRegistersTheLspMethodName(): void // would never route textDocument/completion to this handler. $methods = (new XphpCompletionHandler( new PhpactorWorkspace(), - new WorkspaceSymbols(new PhpactorWorkspace(), $this->newAnalyzer()), + new WorkspaceSymbols(new PhpactorWorkspace(), $this->newCache()), ))->methods(); self::assertArrayHasKey('textDocument/completion', $methods); @@ -242,16 +243,18 @@ private function complete( new TextDocumentIdentifier($uri), new Position($line, $character), ); - $analyzer = $this->newAnalyzer(); + $cache = $this->newCache(); $handler = new XphpCompletionHandler( $workspace, - new WorkspaceSymbols($workspace, $analyzer), + new WorkspaceSymbols($workspace, $cache), ); return wait($handler->complete($params)); } - private function newAnalyzer(): Analyzer + private function newCache(): ParsedDocumentCache { - return new Analyzer(new XphpSourceParser((new ParserFactory())->createForHostVersion())); + return new ParsedDocumentCache( + new Analyzer(new XphpSourceParser((new ParserFactory())->createForHostVersion())), + ); } } diff --git a/tools/lsp/test/Handler/XphpDefinitionHandlerTest.php b/tools/lsp/test/Handler/XphpDefinitionHandlerTest.php index 7fc4484b..53178323 100644 --- a/tools/lsp/test/Handler/XphpDefinitionHandlerTest.php +++ b/tools/lsp/test/Handler/XphpDefinitionHandlerTest.php @@ -13,6 +13,7 @@ use Phpactor\LanguageServerProtocol\TextDocumentItem; use PHPUnit\Framework\TestCase; use XPHP\Lsp\Analyzer\Analyzer; +use XPHP\Lsp\Analyzer\ParsedDocumentCache; use XPHP\Lsp\Handler\XphpDefinitionHandler; use XPHP\Lsp\PositionMap; use XPHP\Transpiler\Monomorphize\XphpSourceParser; @@ -184,7 +185,9 @@ private function newHandler(PhpactorWorkspace $workspace): XphpDefinitionHandler { return new XphpDefinitionHandler( $workspace, - new Analyzer(new XphpSourceParser((new ParserFactory())->createForHostVersion())), + new ParsedDocumentCache( + new Analyzer(new XphpSourceParser((new ParserFactory())->createForHostVersion())), + ), ); } } diff --git a/tools/lsp/test/Handler/XphpHoverHandlerTest.php b/tools/lsp/test/Handler/XphpHoverHandlerTest.php index fc410d1c..16f60a34 100644 --- a/tools/lsp/test/Handler/XphpHoverHandlerTest.php +++ b/tools/lsp/test/Handler/XphpHoverHandlerTest.php @@ -14,6 +14,7 @@ use Phpactor\LanguageServerProtocol\TextDocumentItem; use PHPUnit\Framework\TestCase; use XPHP\Lsp\Analyzer\Analyzer; +use XPHP\Lsp\Analyzer\ParsedDocumentCache; use XPHP\Lsp\Handler\XphpHoverHandler; use XPHP\Lsp\PositionMap; use XPHP\Transpiler\Monomorphize\XphpSourceParser; @@ -77,7 +78,7 @@ class Tag { public string $name; } public function testUnknownUriYieldsNull(): void { $workspace = new PhpactorWorkspace(); - $handler = new XphpHoverHandler($workspace, $this->newAnalyzer()); + $handler = new XphpHoverHandler($workspace, $this->newCache()); $params = new HoverParams( new TextDocumentIdentifier('/never-opened.xphp'), new Position(0, 0), @@ -89,7 +90,7 @@ public function testMethodsMapRegistersHoverEndpoint(): void { // Locks ArrayItemRemoval on methods() — without the entry, the // dispatcher never routes textDocument/hover to this handler. - $methods = (new XphpHoverHandler(new PhpactorWorkspace(), $this->newAnalyzer())) + $methods = (new XphpHoverHandler(new PhpactorWorkspace(), $this->newCache())) ->methods(); self::assertArrayHasKey('textDocument/hover', $methods); self::assertSame('hover', $methods['textDocument/hover']); @@ -231,7 +232,7 @@ private function prepare(string $source): array $workspace = new PhpactorWorkspace(); $uri = '/doc.xphp'; $workspace->open(new TextDocumentItem($uri, 'xphp', 1, $source)); - $handler = new XphpHoverHandler($workspace, $this->newAnalyzer()); + $handler = new XphpHoverHandler($workspace, $this->newCache()); return [$handler, $workspace, $uri]; } @@ -253,8 +254,10 @@ private function hoverAt( return wait($handler->hover($params)); } - private function newAnalyzer(): Analyzer + private function newCache(): ParsedDocumentCache { - return new Analyzer(new XphpSourceParser((new ParserFactory())->createForHostVersion())); + return new ParsedDocumentCache( + new Analyzer(new XphpSourceParser((new ParserFactory())->createForHostVersion())), + ); } } From 9c8542205959b29f7c97ce2bd0ce52e1acc59774 Mon Sep 17 00:00:00 2001 From: Matheus Martins Date: Thu, 21 May 2026 22:46:14 +0000 Subject: [PATCH 16/17] lsp: column-accurate diagnostic ranges (M2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces full-line range underlines with the actual token / identifier span. Before: every diagnostic spanned the whole source line. After: parse errors pin to nikic's reported start/end columns; bound violations + duplicate templates pin to the offending Name / Identifier node's byte span. Pieces: - `PositionMap::rangeFromOffsets(int $startByte, int $endByte)`: convenience wrapping two `offsetToPosition` calls. Half-open shape — callers pass `$endFilePos + 1` when the upstream API returns an inclusive end offset (nikic's `getEndFilePos()` does). - `Analyzer::buildParseErrorDiagnostic`: new helper that prefers `$e->getStartColumn($source) - 1` / `$e->getEndColumn($source)` when the parser kept column info (it does by default), falling back to the full-line range otherwise. nikic uses 1-based inclusive columns; LSP uses 0-based half-open — so `-1` on start, no shift on end. - `WorkspaceAnalyzer::walkDefinitions` and `walkInstantiations` catches: pin range to the offending Identifier / Name's `getStartFilePos()..getEndFilePos() + 1` byte span instead of the full line. The defensive guards (`getStartFilePos() >= 0`) fall back to the full-line range if position info is missing — not reachable from nikic-parsed source, present for synthetic nodes. - `Server::printDiagnostic`: --lint mode formatter now reads `$d->code->value` (the enum case's backed string) for the `[code]` segment, fixing the runtime error from the DiagnosticCode migration. Tests: - `AnalyzerTest::testSyntaxErrorRangeIsColumnAccurateWhenColumnInfoIsAvailable` — asserts exact column values (start=11, end=12) for a 1-character syntax error in a known fixture. Locks the `- 1` 1-based-to-0-based shift on start AND the half-open end. - `WorkspaceAnalyzerTest::testBoundViolationOnScalarConcrete...` + `testDuplicateTemplateDeclarationProducesDiagnostic`: each now asserts `endCharacter - startCharacter === 3` (the `Box` identifier is 3 chars), locking both the `+ 1` arithmetic on `getEndFilePos()` and the abandonment of full-line ranges. - `infection.json5` gains class-level ignores for the new defensive-guard mutations (NotIdentical, GreaterThanOrEqualTo, LogicalAnd, LogicalAndAllSubExprNegation, LogicalAndNegation, GreaterThanOrEqualToNegotiation) in `WorkspaceAnalyzer`. Same pattern + rationale as the existing `AstPositionResolver` guard: nikic's default lexer always populates startFilePos / endFilePos, so the >= 0 branches are unreachable from real source. Smoke-tested via `--lint`: $x = new Box(); <- column 10 (1-based) in the source /tmp/col.xphp:4:10: error: [xphp.bound] Generic bound violated... Before this commit, every diagnostic reported column 1 regardless of the offending position. Verification: - `make test/lsp` -> OK (131 tests, 299 assertions; +1 over commit C). - `make test/lsp/mutation` -> 94 % MSI under the 93 % gate (dipped from 95 % because the new column-accurate paths add 25 mutations, of which several are defensive-guard equivalents now ignored with rationale). - `make test/unit` -> unchanged (171/453). Co-Authored-By: Claude Opus 4.7 (1M context) --- tools/lsp/infection.json5 | 48 ++++++++++++++++++- tools/lsp/src/Analyzer/Analyzer.php | 39 +++++++++++---- tools/lsp/src/Analyzer/WorkspaceAnalyzer.php | 42 ++++++++++++++-- tools/lsp/src/PositionMap.php | 19 ++++++++ tools/lsp/src/Server.php | 2 +- tools/lsp/test/Analyzer/AnalyzerTest.php | 20 ++++++++ .../test/Analyzer/WorkspaceAnalyzerTest.php | 20 ++++++-- 7 files changed, 169 insertions(+), 21 deletions(-) diff --git a/tools/lsp/infection.json5 b/tools/lsp/infection.json5 index e6903535..99ce5f15 100644 --- a/tools/lsp/infection.json5 +++ b/tools/lsp/infection.json5 @@ -124,7 +124,16 @@ }, "GreaterThanOrEqualTo": { "ignore": [ - "XPHP\\Lsp\\Handler\\TypeArgPositionDetector::detect" + "XPHP\\Lsp\\Handler\\TypeArgPositionDetector::detect", + // WorkspaceAnalyzer column-accurate range guards: + // `$identifier->getStartFilePos() >= 0` and + // `$node->getStartFilePos() >= 0 && $node->getEndFilePos() >= 0`. + // nikic's default lexer always populates startFilePos / + // endFilePos for nodes parsed from real source; the >= 0 + // guards exist defensively for synthetic nodes that don't + // appear in this LSP's input. Same pattern + rationale as + // the AstPositionResolver guard already in this file. + "XPHP\\Lsp\\Analyzer\\WorkspaceAnalyzer" ] }, @@ -164,7 +173,14 @@ // an AST that XphpSourceParser would never produce. "LogicalAnd": { "ignore": [ - "XPHP\\Lsp\\Handler\\XphpHoverHandler::buildHoverMarkdown" + "XPHP\\Lsp\\Handler\\XphpHoverHandler::buildHoverMarkdown", + // WorkspaceAnalyzer column-accurate range guards — see the + // `GreaterThanOrEqualTo` ignore above for the rationale. + // `($identifier !== null && $identifier->getStartFilePos() >= 0)` + // and the matching guard in walkInstantiations: both clauses + // are jointly defensive against synthetic nodes that don't + // appear from nikic-parsed source. + "XPHP\\Lsp\\Analyzer\\WorkspaceAnalyzer" ] }, @@ -214,6 +230,34 @@ ] }, + // WorkspaceAnalyzer column-accurate range guards (same family as the + // GreaterThanOrEqualTo / LogicalAnd ignores above). The + // `$identifier !== null` defensive null-check, plus + // GreaterThanOrEqualToNegotiation / LogicalAndAllSubExprNegation / + // LogicalAndNegation variants — each probes the same defensive + // guard whose alternate branch (synthetic node without position info) + // never fires from nikic-parsed source. + "NotIdentical": { + "ignore": [ + "XPHP\\Lsp\\Analyzer\\WorkspaceAnalyzer" + ] + }, + "GreaterThanOrEqualToNegotiation": { + "ignore": [ + "XPHP\\Lsp\\Analyzer\\WorkspaceAnalyzer" + ] + }, + "LogicalAndAllSubExprNegation": { + "ignore": [ + "XPHP\\Lsp\\Analyzer\\WorkspaceAnalyzer" + ] + }, + "LogicalAndNegation": { + "ignore": [ + "XPHP\\Lsp\\Analyzer\\WorkspaceAnalyzer" + ] + }, + // LspDispatcherFactory: ArrayItemRemoval on [$diagnosticsProvider]. // Verifying this end-to-end needs the actual DiagnosticsEngine to // tick once (async coordination over a delay loop) and assert that diff --git a/tools/lsp/src/Analyzer/Analyzer.php b/tools/lsp/src/Analyzer/Analyzer.php index 2a5c663e..a9e4e71c 100644 --- a/tools/lsp/src/Analyzer/Analyzer.php +++ b/tools/lsp/src/Analyzer/Analyzer.php @@ -33,17 +33,9 @@ public function analyzeFile(string $source): ParseResult $ast = $this->parser->parse($source); return new ParseResult($ast, []); } catch (PhpParserError $e) { - // nikic/php-parser carries the start line on PhpParser\Error directly. - // We pin the diagnostic to the full line — column-accurate ranges are a - // follow-up using $e->getStartColumn($source). return new ParseResult( ast: null, - diagnostics: [self::buildLineDiagnostic( - $positionMap, - $e->getStartLine(), - DiagnosticCode::Parse, - 'Syntax error: ' . $e->getRawMessage(), - )], + diagnostics: [self::buildParseErrorDiagnostic($positionMap, $e, $source)], ); } catch (RuntimeException $e) { // XphpSourceParser also throws plain RuntimeException for "parser returned null" @@ -61,6 +53,35 @@ public function analyzeFile(string $source): ParseResult } } + /** + * Map a `PhpParser\Error` to a Diagnostic with column-accurate range when + * the parser kept enough info (`hasColumnInfo()`), falling back to a + * full-line underline otherwise. nikic returns 1-based columns; we + * subtract 1 for LSP's 0-based shape. + */ + private static function buildParseErrorDiagnostic( + PositionMap $positionMap, + PhpParserError $e, + string $source, + ): Diagnostic { + $message = 'Syntax error: ' . $e->getRawMessage(); + if (!$e->hasColumnInfo()) { + return self::buildLineDiagnostic($positionMap, $e->getStartLine(), DiagnosticCode::Parse, $message); + } + $startLine = PositionMap::lspLineFromNikic($e->getStartLine()); + $endLine = PositionMap::lspLineFromNikic($e->getEndLine()); + return new Diagnostic( + startLine: $startLine, + startCharacter: $e->getStartColumn($source) - 1, + endLine: $endLine, + // endColumn from nikic is the column of the LAST character (1-based, + // inclusive). LSP ranges are half-open, so we don't subtract 1. + endCharacter: $e->getEndColumn($source), + message: $message, + code: DiagnosticCode::Parse, + ); + } + private static function buildLineDiagnostic( PositionMap $positionMap, int $nikicLine, diff --git a/tools/lsp/src/Analyzer/WorkspaceAnalyzer.php b/tools/lsp/src/Analyzer/WorkspaceAnalyzer.php index 0108a317..49f26cee 100644 --- a/tools/lsp/src/Analyzer/WorkspaceAnalyzer.php +++ b/tools/lsp/src/Analyzer/WorkspaceAnalyzer.php @@ -104,14 +104,35 @@ public function enterNode(Node $node): null $this->sourceFile, ); } catch (RuntimeException $e) { - $this->diagnostics[] = self::buildDiagnostic($this->positionMap, $node->getStartLine(), DiagnosticCode::Definition, $e->getMessage()); + $this->diagnostics[] = self::buildDiagnostic( + $this->positionMap, + $node->name, + $node->getStartLine(), + DiagnosticCode::Definition, + $e->getMessage(), + ); } return null; } - private static function buildDiagnostic(PositionMap $positionMap, int $nikicLine, DiagnosticCode $code, string $message): Diagnostic - { - [$sl, $sc, $el, $ec] = $positionMap->fullLineRangeFromNikic($nikicLine); + private static function buildDiagnostic( + PositionMap $positionMap, + ?\PhpParser\Node\Identifier $identifier, + int $fallbackNikicLine, + DiagnosticCode $code, + string $message, + ): Diagnostic { + // Prefer the identifier's actual byte span — squiggles the class + // name, not the whole line. Fall back to the full-line range when + // position info is missing (synthetic nodes etc.). + if ($identifier !== null && $identifier->getStartFilePos() >= 0) { + [$sl, $sc, $el, $ec] = $positionMap->rangeFromOffsets( + $identifier->getStartFilePos(), + $identifier->getEndFilePos() + 1, + ); + } else { + [$sl, $sc, $el, $ec] = $positionMap->fullLineRangeFromNikic($fallbackNikicLine); + } return new Diagnostic($sl, $sc, $el, $ec, $message, code: $code); } }; @@ -158,7 +179,18 @@ public function enterNode(Node $node): null try { $this->registry->recordInstantiation($fqn, $args); } catch (RuntimeException $e) { - [$sl, $sc, $el, $ec] = $this->positionMap->fullLineRangeFromNikic($node->getStartLine()); + // Pin the range to the offending Name's byte span (squiggles + // just the generic identifier, e.g. `Box` in + // `new Box()`) rather than the whole line. Fall back to + // the full line if position info is missing. + if ($node->getStartFilePos() >= 0 && $node->getEndFilePos() >= 0) { + [$sl, $sc, $el, $ec] = $this->positionMap->rangeFromOffsets( + $node->getStartFilePos(), + $node->getEndFilePos() + 1, + ); + } else { + [$sl, $sc, $el, $ec] = $this->positionMap->fullLineRangeFromNikic($node->getStartLine()); + } $this->diagnostics[] = new Diagnostic( startLine: $sl, startCharacter: $sc, diff --git a/tools/lsp/src/PositionMap.php b/tools/lsp/src/PositionMap.php index a43e0153..6920fff5 100644 --- a/tools/lsp/src/PositionMap.php +++ b/tools/lsp/src/PositionMap.php @@ -119,6 +119,25 @@ public function offsetToPosition(int $byteOffset): array return [$line, $character]; } + /** + * Resolve a [startByte, endByte) byte-range into an LSP range tuple + * `[startLine, startCharacter, endLine, endCharacter]`. Convenience for + * the two `offsetToPosition` calls handlers need when they want to pin a + * diagnostic to an actual AST node's span rather than the whole line. + * + * Caller-supplied `$endByte` is treated as half-open (exclusive). If the + * upstream API returns an inclusive end offset (e.g. nikic's + * `Node::getEndFilePos()`), pass `$endFilePos + 1`. + * + * @return array{0: int, 1: int, 2: int, 3: int} + */ + public function rangeFromOffsets(int $startByte, int $endByte): array + { + [$sl, $sc] = $this->offsetToPosition($startByte); + [$el, $ec] = $this->offsetToPosition($endByte); + return [$sl, $sc, $el, $ec]; + } + /** * Resolve a 1-based nikic line number to the {line, character} range covering that whole line. * diff --git a/tools/lsp/src/Server.php b/tools/lsp/src/Server.php index 11d948fa..fc24159b 100644 --- a/tools/lsp/src/Server.php +++ b/tools/lsp/src/Server.php @@ -113,6 +113,6 @@ private static function printDiagnostic(string $path, Diagnostic $d): void $line = $d->startLine + 1; $col = $d->startCharacter + 1; $sev = strtolower($d->severity->name); - fwrite(STDOUT, "{$path}:{$line}:{$col}: {$sev}: [{$d->code}] {$d->message}\n"); + fwrite(STDOUT, "{$path}:{$line}:{$col}: {$sev}: [{$d->code->value}] {$d->message}\n"); } } diff --git a/tools/lsp/test/Analyzer/AnalyzerTest.php b/tools/lsp/test/Analyzer/AnalyzerTest.php index 03e16cd9..c0dbdeea 100644 --- a/tools/lsp/test/Analyzer/AnalyzerTest.php +++ b/tools/lsp/test/Analyzer/AnalyzerTest.php @@ -76,6 +76,26 @@ function broken( { self::assertGreaterThanOrEqual($d->startLine, $d->endLine); } + public function testSyntaxErrorRangeIsColumnAccurateWhenColumnInfoIsAvailable(): void + { + // Locks the `$e->getStartColumn($source) - 1` / `$e->getEndColumn($source)` + // arithmetic. Earlier the diagnostic always spanned a whole line; now + // it pins to the actual offending token. nikic's columns are 1-based + // inclusive; the LSP range is 0-based half-open — that's the `- 1` on + // start and the absent `- 1` on end. + // + // Source: `analyzeFile('diagnostics); + $d = $result->diagnostics[0]; + self::assertSame(0, $d->startLine); + self::assertSame(11, $d->startCharacter, '1-based-to-0-based shift must hold'); + self::assertSame(12, $d->endCharacter, 'inclusive 1-based end + no shift = half-open 0-based end'); + } + private static function buildAnalyzer(): Analyzer { return new Analyzer(new XphpSourceParser((new ParserFactory())->createForHostVersion())); diff --git a/tools/lsp/test/Analyzer/WorkspaceAnalyzerTest.php b/tools/lsp/test/Analyzer/WorkspaceAnalyzerTest.php index 1d2f7afa..7290c067 100644 --- a/tools/lsp/test/Analyzer/WorkspaceAnalyzerTest.php +++ b/tools/lsp/test/Analyzer/WorkspaceAnalyzerTest.php @@ -35,8 +35,15 @@ class Box self::assertSame([], $diagnostics['/Box.xphp'], 'template file itself has no violation'); self::assertCount(1, $diagnostics['/Use.xphp'], 'instantiation file should carry one bound-violation diagnostic'); - self::assertStringContainsString('Generic bound violated', $diagnostics['/Use.xphp'][0]->message); - self::assertSame(DiagnosticCode::BoundViolation, $diagnostics['/Use.xphp'][0]->code); + $d = $diagnostics['/Use.xphp'][0]; + self::assertStringContainsString('Generic bound violated', $d->message); + self::assertSame(DiagnosticCode::BoundViolation, $d->code); + // Column accuracy: the diagnostic must point at the offending `Box` + // Name node (3 characters wide), not span the whole line. Locks the + // `rangeFromOffsets` path against regression to the old + // full-line behaviour. + self::assertSame(3, $d->endCharacter - $d->startCharacter, 'range must span just the `Box` identifier'); + self::assertGreaterThan(0, $d->startCharacter, 'must not start at column 0 (whole-line) anymore'); } public function testBoundViolationOnUnknownClassReportsDistinctMessage(): void @@ -188,8 +195,13 @@ class Box { public T $item; } // Iteration order over $files is insertion order, so BoxOne wins, BoxTwo gets the error. self::assertSame([], $diagnostics['/BoxOne.xphp']); self::assertCount(1, $diagnostics['/BoxTwo.xphp']); - self::assertStringContainsString('already declared', $diagnostics['/BoxTwo.xphp'][0]->message); - self::assertSame(DiagnosticCode::Definition, $diagnostics['/BoxTwo.xphp'][0]->code); + $d = $diagnostics['/BoxTwo.xphp'][0]; + self::assertStringContainsString('already declared', $d->message); + self::assertSame(DiagnosticCode::Definition, $d->code); + // Column accuracy: the diagnostic must point at the duplicate class + // identifier (`Box`, 3 chars), not span the whole line. Locks the + // `getEndFilePos() + 1` arithmetic against off-by-one regressions. + self::assertSame(3, $d->endCharacter - $d->startCharacter, 'range must span just the `Box` identifier'); } /** From 3de2142ad3391b3a67c7248bd0caba086918145c Mon Sep 17 00:00:00 2001 From: Matheus Martins Date: Thu, 21 May 2026 23:03:07 +0000 Subject: [PATCH 17/17] ci: run LSP unit + mutation tests in the GitHub Actions pipeline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `feat/lsp` branch shipped `make test/lsp` and `make test/lsp/mutation` locally, but CI was only running the core `make test/unit` and `make test/mutation`. A regression in the LSP package could land on main unnoticed. Two new parallel jobs: - `phpunit-lsp` runs `make test/lsp` (121 -> 131 PHPUnit cases over the branch). Composer install scoped to `tools/lsp/` via ramsey/composer- install's `working-directory` so the LSP cache key is separate from the core's (a core-only change won't bust the LSP cache and vice versa). - `infection-lsp` runs `make test/lsp/mutation` after the unit job passes, mirroring the existing `phpunit -> infection` dependency. Lazy-downloads `tools/lsp/var/infection.phar` (PHAR distribution needed to sidestep the composer / phpactor psr/log conflict — see tools/lsp/README.md), enforces the same 93 % MSI gate the core uses. Uploads `tools/lsp/var/infection.{log,html}` as a 14-day artifact under the name `infection-lsp-report` (distinct from the core's `infection-report` to avoid the name collision). Both LSP jobs run in parallel with their core counterparts so total CI wall-time is governed by the slower mutation pass, not the sum. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/ci.yml | 63 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5fc947fb..6edc2fa6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,6 +32,31 @@ jobs: - name: Run PHPUnit run: make test/unit + phpunit-lsp: + name: PHPUnit (LSP) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Setup PHP 8.4 + uses: shivammathur/setup-php@v2 + with: + php-version: '8.4' + extensions: dom, json, mbstring, tokenizer + coverage: none + tools: composer:v2 + + # The LSP package has its own composer.json under tools/lsp/ with a + # path-repo pointing back at the repo root. Cache its deps separately + # from the core install so a core-only change doesn't bust the LSP cache. + - name: Install LSP dependencies + uses: ramsey/composer-install@v3 + with: + working-directory: tools/lsp + + - name: Run PHPUnit (LSP) + run: make test/lsp + infection: name: Mutation testing runs-on: ubuntu-latest @@ -65,3 +90,41 @@ jobs: var/infection.html if-no-files-found: ignore retention-days: 14 + + infection-lsp: + name: Mutation testing (LSP) + runs-on: ubuntu-latest + needs: phpunit-lsp + steps: + - uses: actions/checkout@v4 + + - name: Setup PHP 8.4 (with Xdebug for coverage) + uses: shivammathur/setup-php@v2 + with: + php-version: '8.4' + extensions: dom, json, mbstring, tokenizer + coverage: xdebug + tools: composer:v2 + + - name: Install LSP dependencies + uses: ramsey/composer-install@v3 + with: + working-directory: tools/lsp + + - name: Run Infection (LSP) + # `make test/lsp/mutation` lazily downloads infection.phar 0.33.1 + # into tools/lsp/var/ (PHAR sidesteps the composer dep conflict with + # phpactor/language-server documented in tools/lsp/README.md). Runs + # with --min-covered-msi=93; current baseline is 94%. + run: make test/lsp/mutation + + - name: Upload Infection report (LSP) + if: always() + uses: actions/upload-artifact@v4 + with: + name: infection-lsp-report + path: | + tools/lsp/var/infection.log + tools/lsp/var/infection.html + if-no-files-found: ignore + retention-days: 14