Improve linked symbol anchors with tree-sitter fallback#314864
Improve linked symbol anchors with tree-sitter fallback#314864dmitrivMS merged 8 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an eager Tree-sitter-based fallback when linkifying linked-backtick symbol references so that symbol links can open near the referenced declaration even before vscode.executeDocumentSymbolProvider (LSP) results are available, while still allowing LSP-based kind/location upgrades later.
Changes:
- Inject
IParserServiceintoSymbolLinkifierand resolve an initial best-effort symbol location via Tree-sitter (with per-linkify caching). - Add
findSymbolLocationInFileto parse/query per-file symbols, preferring declaration-like matches and falling back to generic symbol/qualified-name matches. - Export
extractSymbolNamesInCodeand add tests covering Tree-sitter fallback behavior, caching, and LSP upgrade behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| extensions/copilot/src/extension/linkify/vscode-node/symbolLinkifier.ts | Uses Tree-sitter-derived initial symbol locations (cached per run) and preserves the existing LSP upgrade path. |
| extensions/copilot/src/extension/linkify/vscode-node/findWord.ts | Implements Tree-sitter symbol location lookup with declaration preference, qualified-name fallback, and a per-URI symbol cache. |
| extensions/copilot/src/extension/linkify/vscode-node/findSymbol.ts | Exposes extractSymbolNamesInCode for reuse by Tree-sitter fallback logic. |
| extensions/copilot/src/extension/linkify/test/vscode-node/symbolLinkifier.test.ts | Adds coverage for initial Tree-sitter locations, cache reuse, and LSP upgrade behavior. |
| extensions/copilot/src/extension/linkify/test/vscode-node/findWord.test.ts | Adds unit tests for findSymbolLocationInFile, including declaration preference, qualified fallback, and cache behavior. |
| function ensureTestWorkspace(): MutableTestWorkspace { | ||
| const testVscode = vscode as unknown as { workspace?: PartialMutableTestWorkspace }; | ||
| testVscode.workspace ??= {}; | ||
| testVscode.workspace.textDocuments ??= []; | ||
| testVscode.workspace.fs ??= {}; | ||
| testVscode.workspace.fs.readFile ??= (async () => { throw new Error('workspace.fs.readFile not mocked in test'); }) as typeof vscode.workspace.fs.readFile; | ||
| return testVscode.workspace as MutableTestWorkspace; | ||
| } | ||
|
|
||
| function ensureTestCommands(): MutableTestCommands { | ||
| const testVscode = vscode as unknown as { commands?: Partial<MutableTestCommands> }; | ||
| testVscode.commands ??= {}; | ||
| testVscode.commands.executeCommand ??= (async () => undefined) as typeof vscode.commands.executeCommand; | ||
| return testVscode.commands as MutableTestCommands; | ||
| } | ||
|
|
||
| const testWorkspace = ensureTestWorkspace(); | ||
| const testCommands = ensureTestCommands(); | ||
| const originalWorkspaceReadFile = vscode.workspace.fs.readFile; | ||
| const originalWorkspaceTextDocuments = vscode.workspace.textDocuments; | ||
| const originalExecuteCommand = vscode.commands.executeCommand; | ||
|
|
||
| afterEach(() => { | ||
| testWorkspace.textDocuments = originalWorkspaceTextDocuments; | ||
| testWorkspace.fs.readFile = originalWorkspaceReadFile; | ||
| testCommands.executeCommand = originalExecuteCommand; | ||
| }); |
| class TestParserService implements Partial<IParserService> { | ||
| public parseCount = 0; | ||
| public genericSymbolQueryCount = 0; | ||
|
|
||
| constructor( | ||
| private readonly symbols: readonly TreeSitterExpressionInfo[] = [], | ||
| private readonly classDeclarations: readonly TreeSitterExpressionInfo[] = [], | ||
| private readonly functionDefinitions: readonly TreeSitterExpressionInfo[] = [], | ||
| private readonly typeDeclarations: readonly TreeSitterExpressionInfo[] = [], | ||
| ) { } | ||
|
|
||
| getTreeSitterASTForWASMLanguage(_language: WASMLanguage, _source: string): TreeSitterAST { | ||
| this.parseCount++; | ||
| const symbols = this.symbols; | ||
| const classDeclarations = this.classDeclarations; | ||
| const functionDefinitions = this.functionDefinitions; | ||
| const typeDeclarations = this.typeDeclarations; | ||
| return { | ||
| getClassDeclarations: async () => classDeclarations, | ||
| getFunctionDefinitions: async () => functionDefinitions, | ||
| getTypeDeclarations: async () => typeDeclarations, | ||
| getSymbols: async () => { | ||
| this.genericSymbolQueryCount++; | ||
| return symbols; | ||
| }, | ||
| } as unknown as TreeSitterAST; | ||
| } | ||
| } | ||
|
|
||
| interface MutableTestWorkspace { | ||
| textDocuments: typeof vscode.workspace.textDocuments; | ||
| fs: { | ||
| readFile: typeof vscode.workspace.fs.readFile; | ||
| }; | ||
| } | ||
|
|
||
| interface PartialMutableTestWorkspace { | ||
| textDocuments?: MutableTestWorkspace['textDocuments']; | ||
| fs?: Partial<MutableTestWorkspace['fs']>; | ||
| } | ||
|
|
||
| function ensureTestWorkspace(): MutableTestWorkspace { | ||
| const testVscode = vscode as unknown as { workspace?: PartialMutableTestWorkspace }; | ||
| testVscode.workspace ??= {}; | ||
| testVscode.workspace.textDocuments ??= []; | ||
| testVscode.workspace.fs ??= {}; | ||
| testVscode.workspace.fs.readFile ??= (async () => { throw new Error('workspace.fs.readFile not mocked in test'); }) as typeof vscode.workspace.fs.readFile; | ||
| return testVscode.workspace as MutableTestWorkspace; | ||
| } | ||
|
|
||
| const testWorkspace = ensureTestWorkspace(); | ||
| const originalWorkspaceReadFile = vscode.workspace.fs.readFile; | ||
| const originalWorkspaceTextDocuments = vscode.workspace.textDocuments; | ||
|
|
||
| afterEach(() => { | ||
| testWorkspace.textDocuments = originalWorkspaceTextDocuments; | ||
| testWorkspace.fs.readFile = originalWorkspaceReadFile; | ||
| }); | ||
|
|
||
| function setWorkspaceFileContents(contentsByUri: ReadonlyMap<string, string>): void { | ||
| testWorkspace.textDocuments = []; | ||
| testWorkspace.fs.readFile = async (uri: vscode.Uri) => { | ||
| const contents = contentsByUri.get(uri.toString()); | ||
| if (contents === undefined) { | ||
| throw new Error(`File not found: ${uri.toString()}`); | ||
| } | ||
| return new TextEncoder().encode(contents); | ||
| }; | ||
| } |
There was a problem hiding this comment.
this may be worth addressing as I saw this as well
| function ensureTestWorkspace(): MutableTestWorkspace { | ||
| const testVscode = vscode as unknown as { workspace?: PartialMutableTestWorkspace }; | ||
| testVscode.workspace ??= {}; | ||
| testVscode.workspace.textDocuments ??= []; | ||
| testVscode.workspace.fs ??= {}; | ||
| testVscode.workspace.fs.readFile ??= (async () => { throw new Error('workspace.fs.readFile not mocked in test'); }) as typeof vscode.workspace.fs.readFile; | ||
| return testVscode.workspace as MutableTestWorkspace; | ||
| } | ||
|
|
||
| const testWorkspace = ensureTestWorkspace(); | ||
| const originalWorkspaceReadFile = vscode.workspace.fs.readFile; | ||
| const originalWorkspaceTextDocuments = vscode.workspace.textDocuments; | ||
|
|
||
| afterEach(() => { | ||
| testWorkspace.textDocuments = originalWorkspaceTextDocuments; | ||
| testWorkspace.fs.readFile = originalWorkspaceReadFile; | ||
| }); | ||
|
|
||
| function setWorkspaceFileContents(contentsByUri: ReadonlyMap<string, string>): void { | ||
| testWorkspace.textDocuments = []; | ||
| testWorkspace.fs.readFile = async (uri: vscode.Uri) => { | ||
| const contents = contentsByUri.get(uri.toString()); | ||
| if (contents === undefined) { | ||
| throw new Error(`File not found: ${uri.toString()}`); | ||
| } | ||
| return new TextEncoder().encode(contents); | ||
| }; | ||
| } |
@microsoft-github-policy-service agree |
|
@pranavvaid-ac Unit-tests are failing - please see the logs. |
|
@pranavvaid-ac Please take a look at the new Copilot comments. Also on perf from AI - what are your thoughts on below? The PR adds non-trivial work to a code path that previously was constant time (
AI Suggestion |
Addressed points 1, 2, and 5. Points 1 and 5 were related to the other review comments, so addressing those also fixed that. Point 2 makes sense as well - I was relying on the performance of the On point 3, the three declaration queries let us prefer the declaration-kind matches over generic symbol references which is a better user experience. Performance wise I'm not too worried since the declaration queries are per file and cached for the response. On point 4, this is consistent with the behavior of the |
Adds an eager tree-sitter fallback for linked-backtick symbol references so links open near the referenced symbol even before an LSP DocumentSymbolProvider result is available. Before this PR, links to referenced symbols would just take you to the top of the file if an LSP was not available.
Changes: