chore: Simplify extension#25
Merged
Merged
Conversation
7738cd7 to
7322e83
Compare
There was a problem hiding this comment.
Pull request overview
Simplifies the PHPX VS Code extension by removing dead code, deduplicating provider boilerplate, and tightening up compiler overflow handling and position/diagnostic remapping behaviors.
Changes:
- Refactors definition/type-definition/implementation providers to share a single delegation helper and simplifies
LocationLinkremapping. - Improves compiler process overflow handling and removes unused compilation error tracking.
- Cleans up diagnostics forwarding and strengthens/remodels several extension-host tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
extension/src/providers.ts |
Deduplicates location-style providers via a shared delegate helper; simplifies LocationLink remapping logic; removes unused workspace symbol provider. |
extension/src/compiler.ts |
Fixes stderr overflow handling in close path; removes unused compilation error map; improves error logging/message selection. |
extension/src/diagnostics.ts |
Removes no-op diagnostic filter and forwards diagnostics directly with remapped ranges. |
extension/src/positionMapper.ts |
Removes /g from shared word regex constant; adjusts cache miss file read/stat ordering; updates matchAll usage to use fresh regex instances. |
extension/src/languageClient.ts |
Removes redundant ?? undefined from cwd derivation. |
extension/src/extension.ts |
Deduplicates vendor watcher cache-invalidation handlers. |
extension/src/test/range-remap.test.ts |
Strengthens range/position remap assertions and validates forwarded URI/position for rename/prepareRename. |
extension/src/test/grammar.test.ts |
Replaces placeholder “runner” logic with a focused Mocha structure-validation test and clearer documentation. |
extension/src/test/extension.test.ts |
Simplifies activation test and adds a file-extension-based language auto-detection test. |
Comments suppressed due to low confidence (2)
extension/src/compiler.ts:190
- The output-limit checks compare
stdout.length(string code units) tomaxOutputBytes(bytes). This can under/over-count when output contains non-ASCII and makes the truncation threshold inaccurate. Track byte counts separately (e.g.,stdoutBytes += data.length) and compare that tomaxOutputBytes, or accumulate buffers and use total Buffer length.
proc.stdout!.on('data', (data: Buffer) => {
try {
if (stdout.length < maxOutputBytes) {
stdout += data.toString();
} else if (!truncated) {
truncated = true;
this.outputChannel.appendLine(`[PHPX] stdout truncated — exceeded ${maxOutputBytes} bytes (buffer: ${stdout.length}, chunk: ${data.length})`);
proc.kill();
}
extension/src/compiler.ts:206
- Same byte-vs-string-length issue for stderr:
stderr.lengthis not bytes, so the overflow guard may not enforce the intendedmaxOutputByteslimit. Consider maintaining astderrBytescounter based ondata.lengthand using that for comparisons/logging.
proc.stderr!.on('data', (data: Buffer) => {
try {
if (stderr.length < maxOutputBytes) {
stderr += data.toString();
} else if (!stderrOverflow) {
stderrOverflow = true;
this.outputChannel.appendLine(`[PHPX] stderr truncated — exceeded ${maxOutputBytes} bytes (buffer: ${stderr.length}, chunk: ${data.length})`);
proc.kill();
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Simplification (
chore: Simplify extension)providers.ts: ExtracteddelegateLocationProvider()helper shared byPHPXDefinitionProvider,PHPXTypeDefinitionProvider, andPHPXImplementationProvider— eliminated ~60 lines of triplicated boilerplate. SimplifiedmapLocationLinkToSourceusing a singleremapUrivariable instead of three mutable lets. Removed deadPHPXWorkspaceSymbolProviderclass (exported but never registered inextension.ts).compiler.ts: MovedstderrOverflowdeclaration above its handler (was declared one line after use). AddedstderrOverflowto theclosehandler's overflow branch — previously onlytruncatedwas checked, leaving stderr-overflow kills to fall through tocode !== 0and attemptJSON.parseon a truncated buffer. Removed deadcompilationErrorsmap andgetCompilationError()method (written but never read externally). Fixederr.message || err.stackorder incompileAndWritecatch — stack now used for logging, message for user-facing error.diagnostics.ts: RemovedshouldForwardDiagnostic()stub (always returnedtrue) and its no-op.filter()call site.positionMapper.ts: Removed/gflag from module-levelPHP_WORD_PATTERN—document.getWordRangeAtPosition()throws if passed a global regex (this was silently swallowing the error in all three location providers). Fixed double-statSyncon cache miss: stat first, then read.languageClient.ts: Removed redundant?? undefinedfrom optional-chain expression.extension.ts: Deduplicated the three identical vendorWatcher event handlers into a sharedclearCompilerCachecallback.Bug fix (
fix(positionMapper): use fresh regex in getWordAt for matchAll compatibility)getWordAtusesString.matchAll()which requires a global regex, butPHP_WORD_PATTERNno longer has/g. Fixed by creating a freshnew RegExp(PHP_WORD_PATTERN.source, 'g')at the call site — consistent with the pattern already used ingetNthOccurrenceandfindNthOccurrence.Tests (
test(extension): fix false positives and strengthen assertions)extension.test.ts: Replaced circularlanguageIdtest (was forcibly settinglanguage: 'phpx'then assertinglanguageId === 'phpx') with a real auto-detection test that opens a temp.phpxfile by URI.grammar.test.ts: RemovedrunGrammarTests()— it looped over 12 test cases withexpectedScopesarrays that were never read; every case unconditionally incrementedpassed. Actual tokenization is covered bypnpm test:grammarviavscode-tmgrammar-test. StrengthenedvalidateGrammarStructureto assert non-emptypatternsandrepository.range-remap.test.ts: FixedexecuteCommandmocks to capture and assert the forwarded PHP URI and PHPX→PHP mapped column — the forward position mapping was previously completely untested. Added end-position assertions to all four tests (only start was previously checked). AddedphpxTitleStart !== phpTitleStartguard to guarantee the fixture requires non-trivial column remapping.Test plan
pnpm compile— no errors)pnpm test)pnpm test:grammar).phpxfiles.phpxfiles🤖 Generated with Claude Code