From 409041b7ff707baf57076c6a2faa842122ca1cbc Mon Sep 17 00:00:00 2001 From: Johannes Rieken Date: Tue, 16 Nov 2021 14:07:03 +0100 Subject: [PATCH] dedupe workspace symbols results --- .../api/browser/mainThreadLanguageFeatures.ts | 37 ++-- .../workbench/api/common/extHost.protocol.ts | 2 +- .../api/common/extHostApiCommands.ts | 10 +- .../api/common/extHostLanguageFeatures.ts | 2 +- .../search/browser/search.contribution.ts | 7 +- .../search/browser/symbolsQuickAccess.ts | 179 +++++++++--------- .../workbench/contrib/search/common/search.ts | 63 +++++- .../browser/api/extHostApiCommands.test.ts | 2 +- .../api/extHostLanguageFeatures.test.ts | 38 +++- 9 files changed, 205 insertions(+), 135 deletions(-) diff --git a/src/vs/workbench/api/browser/mainThreadLanguageFeatures.ts b/src/vs/workbench/api/browser/mainThreadLanguageFeatures.ts index 5e6151d2875..3229f48b2aa 100644 --- a/src/vs/workbench/api/browser/mainThreadLanguageFeatures.ts +++ b/src/vs/workbench/api/browser/mainThreadLanguageFeatures.ts @@ -384,27 +384,26 @@ export class MainThreadLanguageFeatures implements MainThreadLanguageFeaturesSha // --- navigate type - $registerNavigateTypeSupport(handle: number): void { + $registerNavigateTypeSupport(handle: number, supportsResolve: boolean): void { let lastResultId: number | undefined; - this._registrations.set(handle, search.WorkspaceSymbolProviderRegistry.register({ - provideWorkspaceSymbols: (search: string, token: CancellationToken): Promise => { - return this._proxy.$provideWorkspaceSymbols(handle, search, token).then(result => { - if (lastResultId !== undefined) { - this._proxy.$releaseWorkspaceSymbols(handle, lastResultId); - } - lastResultId = result._id; - return MainThreadLanguageFeatures._reviveWorkspaceSymbolDto(result.symbols); - }); - }, - resolveWorkspaceSymbol: (item: search.IWorkspaceSymbol, token: CancellationToken): Promise => { - return this._proxy.$resolveWorkspaceSymbol(handle, item, token).then(i => { - if (i) { - return MainThreadLanguageFeatures._reviveWorkspaceSymbolDto(i); - } - return undefined; - }); + + const provider: search.IWorkspaceSymbolProvider = { + provideWorkspaceSymbols: async (search: string, token: CancellationToken): Promise => { + const result = await this._proxy.$provideWorkspaceSymbols(handle, search, token); + if (lastResultId !== undefined) { + this._proxy.$releaseWorkspaceSymbols(handle, lastResultId); + } + lastResultId = result._id; + return MainThreadLanguageFeatures._reviveWorkspaceSymbolDto(result.symbols); } - })); + }; + if (supportsResolve) { + provider.resolveWorkspaceSymbol = async (item: search.IWorkspaceSymbol, token: CancellationToken): Promise => { + const resolvedItem = await this._proxy.$resolveWorkspaceSymbol(handle, item, token); + return resolvedItem && MainThreadLanguageFeatures._reviveWorkspaceSymbolDto(resolvedItem); + }; + } + this._registrations.set(handle, search.WorkspaceSymbolProviderRegistry.register(provider)); } // --- rename diff --git a/src/vs/workbench/api/common/extHost.protocol.ts b/src/vs/workbench/api/common/extHost.protocol.ts index 2144f6df3e6..b8565df3724 100644 --- a/src/vs/workbench/api/common/extHost.protocol.ts +++ b/src/vs/workbench/api/common/extHost.protocol.ts @@ -405,7 +405,7 @@ export interface MainThreadLanguageFeaturesShape extends IDisposable { $registerDocumentFormattingSupport(handle: number, selector: IDocumentFilterDto[], extensionId: ExtensionIdentifier, displayName: string): void; $registerRangeFormattingSupport(handle: number, selector: IDocumentFilterDto[], extensionId: ExtensionIdentifier, displayName: string): void; $registerOnTypeFormattingSupport(handle: number, selector: IDocumentFilterDto[], autoFormatTriggerCharacters: string[], extensionId: ExtensionIdentifier): void; - $registerNavigateTypeSupport(handle: number): void; + $registerNavigateTypeSupport(handle: number, supportsResolve: boolean): void; $registerRenameSupport(handle: number, selector: IDocumentFilterDto[], supportsResolveInitialValues: boolean): void; $registerDocumentSemanticTokensProvider(handle: number, selector: IDocumentFilterDto[], legend: modes.SemanticTokensLegend, eventHandle: number | undefined): void; $emitDocumentSemanticTokensEvent(eventHandle: number): void; diff --git a/src/vs/workbench/api/common/extHostApiCommands.ts b/src/vs/workbench/api/common/extHostApiCommands.ts index 1a46b7661de..eeb38e33bf8 100644 --- a/src/vs/workbench/api/common/extHostApiCommands.ts +++ b/src/vs/workbench/api/common/extHostApiCommands.ts @@ -129,14 +129,8 @@ const newCommands: ApiCommand[] = [ new ApiCommand( 'vscode.executeWorkspaceSymbolProvider', '_executeWorkspaceSymbolProvider', 'Execute all workspace symbol providers.', [ApiCommandArgument.String.with('query', 'Search string')], - new ApiCommandResult<[search.IWorkspaceSymbolProvider, search.IWorkspaceSymbol[]][], types.SymbolInformation[]>('A promise that resolves to an array of SymbolInformation-instances.', value => { - const result: types.SymbolInformation[] = []; - if (Array.isArray(value)) { - for (let tuple of value) { - result.push(...tuple[1].map(typeConverters.WorkspaceSymbol.to)); - } - } - return result; + new ApiCommandResult('A promise that resolves to an array of SymbolInformation-instances.', value => { + return value.map(typeConverters.WorkspaceSymbol.to); }) ), // --- call hierarchy diff --git a/src/vs/workbench/api/common/extHostLanguageFeatures.ts b/src/vs/workbench/api/common/extHostLanguageFeatures.ts index 667289e83ad..7809c705277 100644 --- a/src/vs/workbench/api/common/extHostLanguageFeatures.ts +++ b/src/vs/workbench/api/common/extHostLanguageFeatures.ts @@ -1845,7 +1845,7 @@ export class ExtHostLanguageFeatures implements extHostProtocol.ExtHostLanguageF registerWorkspaceSymbolProvider(extension: IExtensionDescription, provider: vscode.WorkspaceSymbolProvider): vscode.Disposable { const handle = this._addNewAdapter(new NavigateTypeAdapter(provider, this._logService), extension); - this._proxy.$registerNavigateTypeSupport(handle); + this._proxy.$registerNavigateTypeSupport(handle, typeof provider.resolveWorkspaceSymbol === 'function'); return this._createDisposable(handle); } diff --git a/src/vs/workbench/contrib/search/browser/search.contribution.ts b/src/vs/workbench/contrib/search/browser/search.contribution.ts index 9e368be9a08..cdc625287a5 100644 --- a/src/vs/workbench/contrib/search/browser/search.contribution.ts +++ b/src/vs/workbench/contrib/search/browser/search.contribution.ts @@ -46,7 +46,7 @@ import { registerContributions as searchWidgetContributions } from 'vs/workbench import { SymbolsQuickAccessProvider } from 'vs/workbench/contrib/search/browser/symbolsQuickAccess'; import * as Constants from 'vs/workbench/contrib/search/common/constants'; import { resolveResourcesForSearchIncludes } from 'vs/workbench/contrib/search/common/queryBuilder'; -import { getWorkspaceSymbols, SearchStateKey, SearchUIState } from 'vs/workbench/contrib/search/common/search'; +import { getWorkspaceSymbols, IWorkspaceSymbol, SearchStateKey, SearchUIState } from 'vs/workbench/contrib/search/common/search'; import { ISearchHistoryService, SearchHistoryService } from 'vs/workbench/contrib/search/common/searchHistoryService'; import { FileMatch, FileMatchOrMatch, FolderMatch, ISearchWorkbenchService, Match, RenderableMatch, SearchWorkbenchService } from 'vs/workbench/contrib/search/common/searchModel'; import * as SearchEditorConstants from 'vs/workbench/contrib/searchEditor/browser/constants'; @@ -1025,10 +1025,11 @@ configurationRegistry.registerConfiguration({ } }); -CommandsRegistry.registerCommand('_executeWorkspaceSymbolProvider', function (accessor, ...args) { +CommandsRegistry.registerCommand('_executeWorkspaceSymbolProvider', async function (accessor, ...args): Promise { const [query] = args; assertType(typeof query === 'string'); - return getWorkspaceSymbols(query); + const result = await getWorkspaceSymbols(query); + return result.map(item => item.symbol); }); // Go to menu diff --git a/src/vs/workbench/contrib/search/browser/symbolsQuickAccess.ts b/src/vs/workbench/contrib/search/browser/symbolsQuickAccess.ts index eaa56c70ef2..478712d763e 100644 --- a/src/vs/workbench/contrib/search/browser/symbolsQuickAccess.ts +++ b/src/vs/workbench/contrib/search/browser/symbolsQuickAccess.ts @@ -119,102 +119,101 @@ export class SymbolsQuickAccessProvider extends PickerQuickAccessProvider 0) { + + // First: try to score on the entire query, it is possible that + // the symbol matches perfectly (e.g. searching for "change log" + // can be a match on a markdown symbol "change log"). In that + // case we want to skip the container query altogether. + if (symbolQuery !== query) { + [symbolScore, symbolMatches] = scoreFuzzy2(symbolLabelWithIcon, { ...query, values: undefined /* disable multi-query support */ }, 0, symbolLabelIconOffset); + if (typeof symbolScore === 'number') { + skipContainerQuery = true; // since we consumed the query, skip any container matching + } + } + + // Otherwise: score on the symbol query and match on the container later + if (typeof symbolScore !== 'number') { + [symbolScore, symbolMatches] = scoreFuzzy2(symbolLabelWithIcon, symbolQuery, 0, symbolLabelIconOffset); + if (typeof symbolScore !== 'number') { + continue; + } + } + } + + const symbolUri = symbol.location.uri; + let containerLabel: string | undefined = undefined; + if (symbolUri) { + const containerPath = this.labelService.getUriLabel(symbolUri, { relative: true }); + if (symbol.containerName) { + containerLabel = `${symbol.containerName} • ${containerPath}`; + } else { + containerLabel = containerPath; + } + } + + // Score by container if specified and searching + let containerScore: number | undefined = undefined; + let containerMatches: IMatch[] | undefined = undefined; + if (!skipContainerQuery && containerQuery && containerQuery.original.length > 0) { + if (containerLabel) { + [containerScore, containerMatches] = scoreFuzzy2(containerLabel, containerQuery); + } + + if (typeof containerScore !== 'number') { continue; } - const symbolLabel = symbol.name; - const symbolLabelWithIcon = `$(symbol-${SymbolKinds.toString(symbol.kind) || 'property'}) ${symbolLabel}`; - const symbolLabelIconOffset = symbolLabelWithIcon.length - symbolLabel.length; - - // Score by symbol label if searching - let symbolScore: number | undefined = undefined; - let symbolMatches: IMatch[] | undefined = undefined; - let skipContainerQuery = false; - if (symbolQuery.original.length > 0) { - - // First: try to score on the entire query, it is possible that - // the symbol matches perfectly (e.g. searching for "change log" - // can be a match on a markdown symbol "change log"). In that - // case we want to skip the container query altogether. - if (symbolQuery !== query) { - [symbolScore, symbolMatches] = scoreFuzzy2(symbolLabelWithIcon, { ...query, values: undefined /* disable multi-query support */ }, 0, symbolLabelIconOffset); - if (typeof symbolScore === 'number') { - skipContainerQuery = true; // since we consumed the query, skip any container matching - } - } - - // Otherwise: score on the symbol query and match on the container later - if (typeof symbolScore !== 'number') { - [symbolScore, symbolMatches] = scoreFuzzy2(symbolLabelWithIcon, symbolQuery, 0, symbolLabelIconOffset); - if (typeof symbolScore !== 'number') { - continue; - } - } + if (typeof symbolScore === 'number') { + symbolScore += containerScore; // boost symbolScore by containerScore } - - const symbolUri = symbol.location.uri; - let containerLabel: string | undefined = undefined; - if (symbolUri) { - const containerPath = this.labelService.getUriLabel(symbolUri, { relative: true }); - if (symbol.containerName) { - containerLabel = `${symbol.containerName} • ${containerPath}`; - } else { - containerLabel = containerPath; - } - } - - // Score by container if specified and searching - let containerScore: number | undefined = undefined; - let containerMatches: IMatch[] | undefined = undefined; - if (!skipContainerQuery && containerQuery && containerQuery.original.length > 0) { - if (containerLabel) { - [containerScore, containerMatches] = scoreFuzzy2(containerLabel, containerQuery); - } - - if (typeof containerScore !== 'number') { - continue; - } - - if (typeof symbolScore === 'number') { - symbolScore += containerScore; // boost symbolScore by containerScore - } - } - - const deprecated = symbol.tags ? symbol.tags.indexOf(SymbolTag.Deprecated) >= 0 : false; - - symbolPicks.push({ - symbol, - resource: symbolUri, - score: symbolScore, - label: symbolLabelWithIcon, - ariaLabel: symbolLabel, - highlights: deprecated ? undefined : { - label: symbolMatches, - description: containerMatches - }, - description: containerLabel, - strikethrough: deprecated, - buttons: [ - { - iconClass: openSideBySideDirection === 'right' ? Codicon.splitHorizontal.classNames : Codicon.splitVertical.classNames, - tooltip: openSideBySideDirection === 'right' ? localize('openToSide', "Open to the Side") : localize('openToBottom', "Open to the Bottom") - } - ], - trigger: (buttonIndex, keyMods) => { - this.openSymbol(provider, symbol, token, { keyMods, forceOpenSideBySide: true }); - - return TriggerAction.CLOSE_PICKER; - }, - accept: async (keyMods, event) => this.openSymbol(provider, symbol, token, { keyMods, preserveFocus: event.inBackground, forcePinned: event.inBackground }), - }); } + + const deprecated = symbol.tags ? symbol.tags.indexOf(SymbolTag.Deprecated) >= 0 : false; + + symbolPicks.push({ + symbol, + resource: symbolUri, + score: symbolScore, + label: symbolLabelWithIcon, + ariaLabel: symbolLabel, + highlights: deprecated ? undefined : { + label: symbolMatches, + description: containerMatches + }, + description: containerLabel, + strikethrough: deprecated, + buttons: [ + { + iconClass: openSideBySideDirection === 'right' ? Codicon.splitHorizontal.classNames : Codicon.splitVertical.classNames, + tooltip: openSideBySideDirection === 'right' ? localize('openToSide', "Open to the Side") : localize('openToBottom', "Open to the Bottom") + } + ], + trigger: (buttonIndex, keyMods) => { + this.openSymbol(provider, symbol, token, { keyMods, forceOpenSideBySide: true }); + + return TriggerAction.CLOSE_PICKER; + }, + accept: async (keyMods, event) => this.openSymbol(provider, symbol, token, { keyMods, preserveFocus: event.inBackground, forcePinned: event.inBackground }), + }); + } // Sort picks (unless disabled) diff --git a/src/vs/workbench/contrib/search/common/search.ts b/src/vs/workbench/contrib/search/common/search.ts index 69581fc213f..03eaa7595de 100644 --- a/src/vs/workbench/contrib/search/common/search.ts +++ b/src/vs/workbench/contrib/search/common/search.ts @@ -14,9 +14,11 @@ import { IEditorService } from 'vs/workbench/services/editor/common/editorServic import { CancellationToken } from 'vs/base/common/cancellation'; import { ServicesAccessor } from 'vs/platform/instantiation/common/instantiation'; import { IFileService } from 'vs/platform/files/common/files'; -import { IRange } from 'vs/editor/common/core/range'; +import { IRange, Range } from 'vs/editor/common/core/range'; import { isNumber } from 'vs/base/common/types'; import { RawContextKey } from 'vs/platform/contextkey/common/contextkey'; +import { compare } from 'vs/base/common/strings'; +import { groupBy } from 'vs/base/common/arrays'; export interface IWorkspaceSymbol { name: string; @@ -59,19 +61,62 @@ export namespace WorkspaceSymbolProviderRegistry { } } -export function getWorkspaceSymbols(query: string, token: CancellationToken = CancellationToken.None): Promise<[IWorkspaceSymbolProvider, IWorkspaceSymbol[]][]> { +export class WorkspaceSymbolItem { + constructor(readonly symbol: IWorkspaceSymbol, readonly provider: IWorkspaceSymbolProvider) { } +} - const result: [IWorkspaceSymbolProvider, IWorkspaceSymbol[]][] = []; +export async function getWorkspaceSymbols(query: string, token: CancellationToken = CancellationToken.None): Promise { - const promises = WorkspaceSymbolProviderRegistry.all().map(support => { - return Promise.resolve(support.provideWorkspaceSymbols(query, token)).then(value => { - if (Array.isArray(value)) { - result.push([support, value]); + const all: WorkspaceSymbolItem[] = []; + + const promises = WorkspaceSymbolProviderRegistry.all().map(async provider => { + try { + const value = await provider.provideWorkspaceSymbols(query, token); + if (!value) { + return; } - }, onUnexpectedError); + for (let symbol of value) { + all.push(new WorkspaceSymbolItem(symbol, provider)); + } + } catch (err) { + onUnexpectedError(err); + } }); - return Promise.all(promises).then(_ => result); + await Promise.all(promises); + + if (token.isCancellationRequested) { + return []; + } + + // de-duplicate entries + + function compareItems(a: WorkspaceSymbolItem, b: WorkspaceSymbolItem): number { + let res = compare(a.symbol.name, b.symbol.name); + if (res === 0) { + res = a.symbol.kind - b.symbol.kind; + } + if (res === 0) { + res = compare(a.symbol.location.uri.toString(), b.symbol.location.uri.toString()); + } + if (res === 0) { + if (a.symbol.location.range && b.symbol.location.range) { + if (!Range.areIntersecting(a.symbol.location.range, b.symbol.location.range)) { + res = Range.compareRangesUsingStarts(a.symbol.location.range, b.symbol.location.range); + } + } else if (a.provider.resolveWorkspaceSymbol && !b.provider.resolveWorkspaceSymbol) { + res = -1; + } else if (!a.provider.resolveWorkspaceSymbol && b.provider.resolveWorkspaceSymbol) { + res = 1; + } + } + if (res === 0) { + res = compare(a.symbol.containerName ?? '', b.symbol.containerName ?? ''); + } + return res; + } + + return groupBy(all, compareItems).map(group => group[0]).flat(); } export interface IWorkbenchSearchConfigurationProperties extends ISearchConfigurationProperties { diff --git a/src/vs/workbench/test/browser/api/extHostApiCommands.test.ts b/src/vs/workbench/test/browser/api/extHostApiCommands.test.ts index 141d2420fa2..334d6dbf96c 100644 --- a/src/vs/workbench/test/browser/api/extHostApiCommands.test.ts +++ b/src/vs/workbench/test/browser/api/extHostApiCommands.test.ts @@ -201,12 +201,12 @@ suite('ExtHostLanguageFeatureCommands', function () { return rpcProtocol.sync().then(() => { return commands.executeCommand('vscode.executeWorkspaceSymbolProvider', 'testing').then(value => { + assert.strictEqual(value.length, 2); // de-duped for (let info of value) { assert.strictEqual(info instanceof types.SymbolInformation, true); assert.strictEqual(info.name, 'testing'); assert.strictEqual(info.kind, types.SymbolKind.Array); } - assert.strictEqual(value.length, 3); }); }); }); diff --git a/src/vs/workbench/test/browser/api/extHostLanguageFeatures.test.ts b/src/vs/workbench/test/browser/api/extHostLanguageFeatures.test.ts index 0871f46573c..52f13c584d7 100644 --- a/src/vs/workbench/test/browser/api/extHostLanguageFeatures.test.ts +++ b/src/vs/workbench/test/browser/api/extHostLanguageFeatures.test.ts @@ -681,9 +681,41 @@ suite('ExtHostLanguageFeatures', function () { let value = await getWorkspaceSymbols(''); assert.strictEqual(value.length, 1); const [first] = value; - const [, symbols] = first; - assert.strictEqual(symbols.length, 1); - assert.strictEqual(symbols[0].name, 'testing'); + assert.strictEqual(first.symbol.name, 'testing'); + }); + + test('Navigate types, de-duplicate results', async () => { + const uri = URI.from({ scheme: 'foo', path: '/some/path' }); + disposables.push(extHost.registerWorkspaceSymbolProvider(defaultExtension, new class implements vscode.WorkspaceSymbolProvider { + provideWorkspaceSymbols(): any { + return [new types.SymbolInformation('ONE', types.SymbolKind.Array, undefined, new types.Location(uri, new types.Range(0, 0, 1, 1)))]; + } + })); + + disposables.push(extHost.registerWorkspaceSymbolProvider(defaultExtension, new class implements vscode.WorkspaceSymbolProvider { + provideWorkspaceSymbols(): any { + return [new types.SymbolInformation('ONE', types.SymbolKind.Array, undefined, new types.Location(uri, new types.Range(0, 0, 1, 1)))]; // get de-duped + } + })); + + disposables.push(extHost.registerWorkspaceSymbolProvider(defaultExtension, new class implements vscode.WorkspaceSymbolProvider { + provideWorkspaceSymbols(): any { + return [new types.SymbolInformation('ONE', types.SymbolKind.Array, undefined, new types.Location(uri, undefined!))]; // NO dedupe because of resolve + } + resolveWorkspaceSymbol(a: vscode.SymbolInformation) { + return a; + } + })); + + disposables.push(extHost.registerWorkspaceSymbolProvider(defaultExtension, new class implements vscode.WorkspaceSymbolProvider { + provideWorkspaceSymbols(): any { + return [new types.SymbolInformation('ONE', types.SymbolKind.Struct, undefined, new types.Location(uri, new types.Range(0, 0, 1, 1)))]; // NO dedupe because of kind + } + })); + + await rpcProtocol.sync(); + let value = await getWorkspaceSymbols(''); + assert.strictEqual(value.length, 3); }); // --- rename