Repository navigation
LSP MVP: diagnostics, hover, definition, completion - #7
Merged
Merged
Conversation
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 <file.xphp>
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) <noreply@anthropic.com>
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 <valid.xphp> -> exit 0, no output tools/lsp/bin/xphp-lsp --lint <bound-violation> -> exit 1, single line tools/lsp/bin/xphp-lsp --lint <syntax-error> -> exit 1, single line tools/lsp/bin/xphp-lsp --lint <missing-file> -> exit 2 `vendor/bin/phpunit` inside tools/lsp/ passes 13/13. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rver
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 `<source ignoreIndirectDeprecations="true">` 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) <noreply@anthropic.com>
…arams
Adds hover support so editors show useful tooltips when the cursor lands on
something xphp-specific:
- Over a generic instantiation (e.g. `Box<Plastic>`) -> "Specializes to:
\XPHP\Generated\App\Box\T_<sha256>" 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<Hover|null>`.
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<T: \Stringable>`) 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<T>` 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 { "<?php namespace App; $x = new Box<Plastic>();" }
- hover @ (line 2, char 10 = the `B` of Box) ->
contents: "**`App\\Box<App\\Plastic>`** Specializes to:
`\\XPHP\\Generated\\App\\Box\\T_<hash>`"
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
F12 on a generic instantiation (`new Box<Plastic>()`) now jumps to the
template declaration (`class Box<T>`) 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<Location|null>`.
- 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<T: \Stringable>`) 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<T>`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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<List<int>, |` 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<T: \Stringable>` 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<Pla") ->
items: [{label: "Plastic", kind: 7 (CLASS_),
detail: "App\\Models\\Plastic",
insertText: "App\\Models\\Plastic"}]
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Member
Author
ReviewHigh-levelA new The work is structured well: one feature per commit, clear separation of SignificantS1.
|
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) <noreply@anthropic.com>
…(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) <noreply@anthropic.com>
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<string, array{version: int, result: ParseResult}>`. 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) <noreply@anthropic.com>
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<int>(); <- 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) <noreply@anthropic.com>
Member
Author
Review feedback addressed in this branchA second-pass review (in
|
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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a language-server implementation at
tools/lsp/that surfaces xphp-awarefeedback live in any LSP client: real-time diagnostics (parse errors, bound
violations, duplicate-template declarations), hover (specialized FQN +
type-param bound), go-to-definition (instantiation -> template), and completion
of class names inside
<...>type-arg positions. Plus a minimal VS Code clientunder
tools/lsp/vscode-extension/.The server is in PHP, built on
phpactor/language-server. The analyzer reusesXphpSourceParser,Registry,TypeHierarchy, andRegistryCollectorfromthe core package verbatim -- zero edits to existing code, zero impact on
the 93 % MSI gate on
src/. Cross-language feedback is achieved by wrappingthe existing throw-on-first-error pipeline in per-file try/catch and
converting each
RuntimeException/PhpParser\Errorinto a framework-neutralDiagnostic, then translating to the wire-format LSPDiagnosticat exactlyone boundary (
DiagnosticTranslator).Layout
Design decisions
These are the trade-offs worth disagreeing with. Documented inline at the
relevant call sites; restated here so reviewers can argue them in one place.
PHP, not Node, for the server. Reusing the existing parser verbatim
eliminated a re-implementation that would have been months of work for
no functional gain. Node would have been the obvious choice if we
needed to ship a wider ecosystem of editor integrations independently
from the PHP toolchain, but the LSP wire format is the same regardless
of language and the parser IS the contract.
phpactor/language-server6.2 for JSON-RPC scaffolding. It's themost mature PHP-native LSP framework, handles framing /
document-sync / dispatch / capabilities / cancellation, and ships a
DiagnosticsProviderextension point we plug straight into. Trade-offspulled in:
this package -- see "Testing" below for the resolution).
thecodingmachine/safePHP 8.4 implicit-nullable deprecation noise,suppressed at the bin entry-point (
error_reporting(E_ALL & ~E_DEPRECATED))and via
<source ignoreIndirectDeprecations="true">in phpunit.xml.dist.Wrap, don't modify. The compiler throws on first error;
the LSP needs per-document batched errors. Picked option B: the
analyzer wraps each
Registrycall in try/catch and accumulatesdiagnostics. Option A (modifying
Registry::recordInstantiationtooptionally collect) would be cleaner long-term but touches well-tested
code paths. Re-evaluate if a future feature needs structured errors
from the compiler itself.
Framework-neutral
Diagnosticvalue type internally. The analyzeremits
XPHP\Lsp\Analyzer\Diagnostic(plain readonly class). Handlerstranslate to
Phpactor\LanguageServerProtocol\Diagnosticat the wireboundary only. Means the analyzer is testable without spinning up a
transport, and a future LSP-framework swap touches only the
DiagnosticTranslator.Re-parse per LSP request rather than cache ASTs. The analyzer is
microseconds-fast; an AST cache keyed on document version would shave
a small constant factor at the cost of cache-invalidation bugs.
Profile-driven follow-up if it ever matters.
Diagnostic publishing is per-changed-file, not workspace-broadcast.
Editing
Box.xphpre-publishes diagnostics forBox.xphponly. AUse.xphpthat instantiatesBox<int>won't re-render its boundviolation until the next time the user touches it. The fix needs
handler-level cross-file tracking; deferred to a follow-up alongside
the disk Indexer (item 7).
Open documents only. The handlers iterate
phpactor\Workspace,which only knows about documents the editor has open. A workspace-
wide
Indexerwalking**/*.xphpatinitializeis the obviousnext pass -- required for go-to-def + completion + cross-file
diagnostics to work on files the user hasn't opened yet.
PHAR-based Infection rather than composer-installed. Infection
0.33 requires
psr/log ^2 || ^3which composer cannot reconcile withphpactor's
^1.0pin; older Infection lines that allowpsr/log ^1require
symfony/console ^7against the parent package's^8.Running the root vendor's Infection against
tools/lsp/also failsat autoload time because both vendor trees install
thecodingmachine/safeand the two copies redeclare the same globalfunctions. Infection's PHAR ships its internal deps under
PHP-Scoper-prefixed namespaces and side-steps both problems. The
test/lsp/mutationMakefile target downloads the PHAR lazily intotools/lsp/var/(gitignored). When phpactor finally bumps psr/log,migrate to a normal composer dev-dep.
Testing
make test/lsp(PHPUnit)make test/lsp/mutation(Infection PHAR, 0.33.1,--threads=max --min-covered-msi=93)make test/unitLanguageServerTesteroverLspDispatcherFactorybin/xphp-lspover stdioinfection.json5carries per-mutatorignorerules for genuinelyequivalent / defensive mutations -- every entry has an inline rationale.
Class-level ignores are scoped to specific methods, not whole files.
Mirrors the pattern at the repo root (
/infection.json5).The deprecation-suppression chain (
error_reporting=E_ALL & ~E_DEPRECATEDat the bin entry-point,
php -d error_reporting=...in the Makefile,<source ignoreIndirectDeprecations="true">in phpunit.xml.dist) is alldocumented at each layer with the same cross-reference so future readers
don't re-discover the constraint chain.
How to try it locally
The F5 Extension Development Host opens the repo root as the workspace,
so the playground
*.xphpfiles underplayground/src/Demos/areimmediately usable as a test surface. Open
playground/src/Demos/Bounds.xphp, editStringableBox<Tag>-><int>,see the red squiggle.
Out of scope (deferred follow-ups)
Each one is documented inline at the relevant source location too --
this list is the index, not the source of truth.
**/*.xphpatinitializeso go-to-def andcompletion see unopened files.
document affected by an edit, not just the one being typed in.
Box<T: \Stringable>should notsuggest non-Stringable classes. Needs the handler to resolve the
enclosing Name's template definition first.
insertTextis always the full FQNtoday; should prefer the short name when a matching
useis in scope.XphpSourceParserstrips the
<...>clause before parsing, so there's no AST nodepositioned over the bound text. Needs a complementary scanner pass.
ATTR_METHOD_GENERIC_PARAMSisavailable; extension is mechanical.
Compatibility / impact on the core package
Zero code edits under
src/. Zero new deps in the rootcomposer.json.Zero changes to existing test fixtures.
make test/unitandmake test/mutationon the core run identically before and after thisPR.
The roadmap (
docs/roadmap.md) is updated to move "LSP / IDEintegration" from Next to Shipped with the four-feature inventory.