Skip to content

feat(Compiler,LSP): Implement Extensibility#28

Draft
attitude wants to merge 2 commits into
mainfrom
feat/extend-compiler
Draft

feat(Compiler,LSP): Implement Extensibility#28
attitude wants to merge 2 commits into
mainfrom
feat/extend-compiler

Conversation

@attitude

@attitude attitude commented Apr 13, 2026

Copy link
Copy Markdown
Owner

Summary

Adds plugin/extension points to the Compiler and Language Server — no parser changes.

Compiler — CompilerPlugin visitor interface

  • CompilerPlugin interface with three visitor methods: visitElement, visitFragment, visitAttribute
  • AbstractCompilerPlugin base class with null no-ops so plugins only override what they need
  • Plugins are injected via new Compiler(plugins: [...]) — first non-null return wins, then falls back to the active FormatterInterface
  • Covers all attribute compilation paths including prop punning ({$var} shorthand)
  • Whitespace-only '[]' values are normalised to null before reaching plugins, matching the Formatter's own empty-signal convention

Language Server — three extension interfaces

Interface Behaviour
CompletionExtension Items merged with built-in list; getCapabilities() trigger chars deduplicated into initialize response
HoverExtension Tried before built-in provider; first non-null wins
DiagnosticsExtension Results merged with built-in diagnostics

All injected via Server constructor: completionExtensions, hoverExtensions, diagnosticsExtensions.

Every extension call is wrapped in try/catch(\Throwable) with logging — a crashing extension is skipped and remaining extensions + built-in results proceed normally.

What this does NOT touch

  • Parser — no new token types, no AST changes
  • Formatter / Renderer — unchanged (FormatterInterface was already an extension point)
  • Existing public API — all new parameters have defaults; positional argument order preserved (plugins comes after logger)

Test plan

  • 14 new compiler plugin tests (CompilerPlugin.spec.php): element/fragment/attribute interception, plugin ordering, no-op passthrough, prop-punning interception
  • 13 new LSP extension tests (LSPExtensions.spec.php): completion merging, trigger char dedup, hover priority + fallback, diagnostics merging across didOpen/didChange, error isolation for all three extension types
  • Full suite: 472 tests, 0 failures
  • Moved readOutputMessages to shared helpers.php — no implicit cross-file test dependency

🤖 Generated with Claude Code

…Butler and `feat/extend-compiler` branch for the changes.

- Add `CompilerPlugin` interface
  (visitElement/visitFragment/visitAttribute) and
  `AbstractCompilerPlugin` base class with null no-ops
- Wire plugin array into `Compiler` constructor; compilePHPXElement,
  compilePHPXFragmentElement, and compilePHPXAttribute now try plugins
  (first non-null wins) before delegating to the formatter
- Normalise `'[]'` compiled values to null before passing to plugins so
  plugins see the same "empty" signal the formatter uses
- Add `CompletionExtension`, `HoverExtension`, and
  `DiagnosticsExtension` interfaces in src/language-server/
- Wire all three extension arrays into `Server` constructor: completion
  results are merged, hover extensions have priority over built-in
  (first non-null wins), diagnostics are merged, and completion trigger
  chars from extensions are deduplicated into capabilities
- Update src/compiler/index.php to require the two new compiler files
- Add tests: CompilerPlugin.spec.php (13 tests) and
  LSPExtensions.spec.php covering all extension types; all 468 tests
  pass

This comment was marked as resolved.

- Fix positional argument BC break: moved `$plugins` parameter after `$logger` in Compiler constructor so `new Compiler($parser, $formatter, $logger)` remains valid
- Add try/catch(\Throwable) isolation around all LSP extension calls (completion, hover, diagnostics, and capability merging) — a throwing extension is logged and skipped, remaining extensions + built-in results proceed normally
- Added 3 error isolation tests verifying a throwing extension does not crash the server for each extension type

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated 8 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread extension/src/tsxQuery.ts
Comment on lines +15 to +16
* • phpx-intrinsics.d.ts — copied from the extension's types/ folder
* • .query.tsx — tiny JSX snippet used as the completion/hover target

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The header comment says phpx-intrinsics.d.ts is copied into the query directory, but the implementation references the file in-place via intrinsicsPath in tsconfig.files. Please either update the comment to match the current approach or actually copy the file into queryDir to keep the on-disk project self-contained.

Suggested change
* phpx-intrinsics.d.ts copied from the extension's types/ folder
* .query.tsx tiny JSX snippet used as the completion/hover target
* .query.tsx tiny JSX snippet used as the completion/hover target
* The tsconfig also references phpx-intrinsics.d.ts in-place from the
* extension's types/ folder rather than copying it into queryDir.

Copilot uses AI. Check for mistakes.
Comment thread extension/src/tsxQuery.ts
Comment on lines +72 to +80
fs.writeFileSync(queryTsxPath, '/* @jsx h */\nconst _warmup = <div />;\n', 'utf-8');

// Open the document so VS Code's TypeScript language server picks it up
const doc = await vscode.workspace.openTextDocument(vscode.Uri.file(queryTsxPath));
// Request completions once to warm up the TypeScript server project
await vscode.commands.executeCommand(
'vscode.executeCompletionItemProvider',
doc.uri,
new vscode.Position(1, 18), // inside `<div ` on line 1

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The warm-up completion request uses a hard-coded new vscode.Position(1, 18) that doesn't appear to correspond to the <div position in the warmup line (const _warmup = <div />;). If the position is inside the tag name (or otherwise wrong), the warm-up may not actually prime attribute completions. Consider computing the column dynamically (e.g., based on line.indexOf('<div ') + '<div '.length) to avoid off-by-one errors.

Suggested change
fs.writeFileSync(queryTsxPath, '/* @jsx h */\nconst _warmup = <div />;\n', 'utf-8');
// Open the document so VS Code's TypeScript language server picks it up
const doc = await vscode.workspace.openTextDocument(vscode.Uri.file(queryTsxPath));
// Request completions once to warm up the TypeScript server project
await vscode.commands.executeCommand(
'vscode.executeCompletionItemProvider',
doc.uri,
new vscode.Position(1, 18), // inside `<div ` on line 1
const warmupSource = '/* @jsx h */\nconst _warmup = <div />;\n';
fs.writeFileSync(queryTsxPath, warmupSource, 'utf-8');
// Open the document so VS Code's TypeScript language server picks it up
const doc = await vscode.workspace.openTextDocument(vscode.Uri.file(queryTsxPath));
const warmupLine = warmupSource.split('\n')[1] ?? '';
const warmupColumn = (() => {
const divStart = warmupLine.indexOf('<div ');
return divStart >= 0 ? divStart + '<div '.length : 0;
})();
// Request completions once to warm up the TypeScript server project
await vscode.commands.executeCommand(
'vscode.executeCompletionItemProvider',
doc.uri,
new vscode.Position(1, warmupColumn), // just after `<div ` on line 1

Copilot uses AI. Check for mistakes.
Comment thread extension/src/tsxQuery.ts

// Cursor is at the end of line 1, after `<tagName `
// Column = length of `const _el = <` (13) + tagName.length + 1 (space)
const position = new vscode.Position(1, 13 + tagName.length);

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cursor position for attribute completions is off by one: the snippet ends with a trailing space (<${tagName} ) but the position uses 13 + tagName.length, which places the cursor before that space (on/adjacent to the last tag-name character). This can change the completion context returned by TypeScript. Adjust the position to be after the space so completions are requested inside the attribute list.

Suggested change
const position = new vscode.Position(1, 13 + tagName.length);
const position = new vscode.Position(1, 13 + tagName.length + 1);

Copilot uses AI. Check for mistakes.
Comment thread extension/src/tsxQuery.ts
Comment on lines +115 to +116
fs.writeFileSync(queryTsxPath, content, 'utf-8');
const uri = vscode.Uri.file(queryTsxPath);

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

writeAndOpen() uses fs.writeFileSync on the extension host thread. Since this is called from completion/hover providers, synchronous disk I/O can block the UI and degrade editor responsiveness. Prefer vscode.workspace.fs.writeFile (async) or keep a single opened TextDocument and update its contents via WorkspaceEdit to avoid repeated sync writes + reopens.

Suggested change
fs.writeFileSync(queryTsxPath, content, 'utf-8');
const uri = vscode.Uri.file(queryTsxPath);
const uri = vscode.Uri.file(queryTsxPath);
await vscode.workspace.fs.writeFile(uri, new TextEncoder().encode(content));

Copilot uses AI. Check for mistakes.
Comment thread extension/src/tsxQuery.ts
Comment on lines +158 to +160
// Small delay to let TypeScript pick up the file change
await new Promise<void>((resolve) => setTimeout(resolve, 150));

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every completion request waits a fixed 150ms (setTimeout(resolve, 150)) before querying TypeScript. This adds noticeable latency to completions and is also brittle across machines (sometimes too short, sometimes unnecessarily long). Consider waiting on a deterministic signal (e.g., updating the file via VS Code APIs and awaiting the edit/save) or reusing the already-opened document so the language service sees changes without an arbitrary delay.

Copilot uses AI. Check for mistakes.
Comment thread extension/src/tsxQuery.ts
Comment on lines +283 to +284
// Only lowercase-first tags (HTML intrinsic elements).
const match = prefix.match(/<([a-z][a-zA-Z0-9-]*)(?:\s[^>]*)?\s\w*$/);

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

detectTagAtCursor() uses \w* at the end of the regex, which does not include - or :. This means tag detection can fail while typing hyphenated attribute names like data-test / aria-label (cursor after data-), so TypeScript-backed attribute completions won't trigger in those common cases. Consider expanding the allowed attribute prefix characters (e.g., include - and :) so detection keeps working during typing.

Suggested change
// Only lowercase-first tags (HTML intrinsic elements).
const match = prefix.match(/<([a-z][a-zA-Z0-9-]*)(?:\s[^>]*)?\s\w*$/);
// Only lowercase-first tags (HTML intrinsic elements), allowing partial
// attribute names that include common HTML/JSX characters like `-` and `:`.
const match = prefix.match(/<([a-z][a-zA-Z0-9-]*)(?:\s[^>]*)?\s[\w:-]*$/);

Copilot uses AI. Check for mistakes.
Comment thread extension/src/tsxQuery.ts
if (!ch) {
return false;
}
return /[a-zA-Z0-9_]/.test(ch);

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isWordChar() only treats [a-zA-Z0-9_] as part of an attribute name. JSX/HTML attributes commonly include hyphens (e.g., data-testid, aria-label), so hover detection will stop at - and extract a partial attribute name (or return undefined). Expanding the accepted character set here will make detectTagAndAttributeAtCursor() work for real-world attribute names.

Suggested change
return /[a-zA-Z0-9_]/.test(ch);
return /[a-zA-Z0-9_-]/.test(ch);

Copilot uses AI. Check for mistakes.
Comment on lines +59 to +67
// ─── TypeScript JSX attribute query ──────────────────────────────────────
//
// Initialises a small virtual TypeScript project using phpx-intrinsics.d.ts
// (the PHPX JSX type declarations, no React) in the extension's global
// storage directory. This lets us query VS Code's built-in TypeScript
// language service for per-element HTML attribute completions and types.
//
// The query is purely additive — if the TypeScript server is unavailable
// the PHPX LSP's own completions continue to work unchanged.

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR description focuses on compiler/LSP plugin extension points, but this change also introduces a new TypeScript-based completion/hover pipeline in the VS Code extension (tsxQuery + shipped DOM intrinsics types). Please update the PR description to include this user-facing editor feature (or split it into a separate PR) so reviewers know to evaluate packaging, performance, and behavior changes in the extension.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants