From aa04fc4789892709d5866be5108618c88e07c81e Mon Sep 17 00:00:00 2001 From: Varun Gandhi Date: Wed, 22 Nov 2023 19:20:33 +0800 Subject: [PATCH] client: Minor cleanup for search-based code intel --- .../src/enterprise/codeintel/searchBased.ts | 33 ++- .../codeintel/useSearchBasedCodeIntel.ts | 262 +++++++++--------- 2 files changed, 148 insertions(+), 147 deletions(-) diff --git a/client/web/src/enterprise/codeintel/searchBased.ts b/client/web/src/enterprise/codeintel/searchBased.ts index ee2d3c62272d..070411ac3da4 100644 --- a/client/web/src/enterprise/codeintel/searchBased.ts +++ b/client/web/src/enterprise/codeintel/searchBased.ts @@ -110,11 +110,11 @@ function makeRepositoryPattern(repo: string): string { /** The time in ms to delay between unindexed search request and the fallback indexed search request. */ const DEFAULT_UNINDEXED_SEARCH_TIMEOUT_MS = 5000 +export type RepoFilter = 'current-repo' | 'all-other-repos' + /** * Invoke the given search function by modifying the query with a term that will - * only look in the current git tree by appending a repo filter with the repo name - * and the current commit or, if `negateRepoFilter` is set, outside of current git - * tree. + * look in certain git trees (controlled by `repoFilter`). * * This is likely to timeout on large repos or organizations with monorepos if the * current commit is not an indexed commit. Instead of waiting for a timeout, we @@ -125,7 +125,6 @@ const DEFAULT_UNINDEXED_SEARCH_TIMEOUT_MS = 5000 * * @param search The search function. * @param args The arguments to the search function. - * @param negateRepoFilter Whether to look only inside or outside the given repo. */ export function searchWithFallback< P extends { @@ -136,14 +135,14 @@ export function searchWithFallback< queryTerms: string[] }, R ->(search: (args: P) => Promise, args: P, negateRepoFilter = false, getSetting: SettingsGetter): Promise { +>(search: (args: P) => Promise, args: P, repoFilter: RepoFilter, getSetting: SettingsGetter): Promise { if (getSetting('basicCodeIntel.indexOnly', false)) { - return searchIndexed(search, args, negateRepoFilter, getSetting) + return searchIndexed(search, args, repoFilter, getSetting) } return raceWithDelayOffset( - searchUnindexed(search, args, negateRepoFilter, getSetting), - () => searchIndexed(search, args, negateRepoFilter, getSetting), + searchUnindexed(search, args, repoFilter, getSetting), + () => searchIndexed(search, args, repoFilter, getSetting), getSetting('basicCodeIntel.unindexedSearchTimeout', DEFAULT_UNINDEXED_SEARCH_TIMEOUT_MS) ) } @@ -153,18 +152,16 @@ export function searchWithFallback< * * @param search The search function. * @param args The arguments to the search function. - * @param negateRepoFilter Whether to look only inside or outside the given repo. */ function searchIndexed< P extends { repo: string isFork: boolean isArchived: boolean - commit: string queryTerms: string[] }, R ->(search: (args: P) => Promise, args: P, negateRepoFilter = false, getSetting: SettingsGetter): Promise { +>(search: (args: P) => Promise, args: P, repoFilter: RepoFilter, getSetting: SettingsGetter): Promise { const { repo, isFork, isArchived, queryTerms } = args // Create a copy of the args so that concurrent calls to other @@ -175,14 +172,16 @@ function searchIndexed< // Unlike unindexed search, we can't supply a commit as that particular // commit may not be indexed. We force index and look inside/outside // the repo at _whatever_ commit happens to be indexed at the time. - queryTermsCopy.push((negateRepoFilter ? '-' : '') + `repo:${makeRepositoryPattern(repo)}`) + const isCurrentRepoSearch = repoFilter === 'current-repo' + const prefix = isCurrentRepoSearch ? '' : '-' + queryTermsCopy.push(prefix + `repo:${makeRepositoryPattern(repo)}`) queryTermsCopy.push('index:only') // If we're a fork, search in forks _for the same repo_. Otherwise, // search in forks only if it's set in the settings. This is also // symmetric for archived repositories. queryTermsCopy.push( - ...repositoryKindTerms(isFork && !negateRepoFilter, isArchived && !negateRepoFilter, getSetting) + ...repositoryKindTerms(isFork && isCurrentRepoSearch, isArchived && isCurrentRepoSearch, getSetting) ) return search({ ...args, queryTerms: queryTermsCopy }) @@ -193,7 +192,6 @@ function searchIndexed< * * @param search The search function. * @param args The arguments to the search function. - * @param negateRepoFilter Whether to look only inside or outside the given repo. */ function searchUnindexed< P extends { @@ -204,7 +202,7 @@ function searchUnindexed< queryTerms: string[] }, R ->(search: (args: P) => Promise, args: P, negateRepoFilter = false, getSetting: SettingsGetter): Promise { +>(search: (args: P) => Promise, args: P, repoFilter: RepoFilter, getSetting: SettingsGetter): Promise { const { repo, isFork, isArchived, commit, queryTerms } = args // Create a copy of the args so that concurrent calls to other @@ -212,7 +210,8 @@ function searchUnindexed< // modified. const queryTermsCopy = [...queryTerms] - if (!negateRepoFilter) { + const isCurrentRepoSearch = repoFilter === 'current-repo' + if (isCurrentRepoSearch) { // Look in this commit only queryTermsCopy.push(`repo:${makeRepositoryPattern(repo)}@${commit}`) } else { @@ -224,7 +223,7 @@ function searchUnindexed< // search in forks only if it's set in the settings. This is also // symmetric for archived repositories. queryTermsCopy.push( - ...repositoryKindTerms(isFork && !negateRepoFilter, isArchived && !negateRepoFilter, getSetting) + ...repositoryKindTerms(isFork && isCurrentRepoSearch, isArchived && isCurrentRepoSearch, getSetting) ) return search({ ...args, queryTerms: queryTermsCopy }) diff --git a/client/web/src/enterprise/codeintel/useSearchBasedCodeIntel.ts b/client/web/src/enterprise/codeintel/useSearchBasedCodeIntel.ts index 7c45907c51b1..25ac58046499 100644 --- a/client/web/src/enterprise/codeintel/useSearchBasedCodeIntel.ts +++ b/client/web/src/enterprise/codeintel/useSearchBasedCodeIntel.ts @@ -28,6 +28,7 @@ import { type SearchResult, searchResultToResults, searchWithFallback, + type RepoFilter, } from './searchBased' import { sortByProximity } from './sort' @@ -119,149 +120,139 @@ export const useSearchBasedCodeIntel = (options: UseSearchBasedCodeIntelOptions) } } -// searchBasedReferences is 90% copy&paste from code-intel-extension's -export async function searchBasedReferences({ - repo, - isFork, - isArchived, - commit, - searchToken, - path, - position, - fileContent, - spec, - getSetting, - filter, -}: UseSearchBasedCodeIntelOptions): Promise { +function searchBasedReferencesViaSCIPLocals(options: UseSearchBasedCodeIntelOptions): Location[] | undefined { const view = getBlobEditView() - if (view !== null) { - const occurrences = view.state.facet(syntaxHighlight).occurrences - for (const occurrence of occurrences) { - if ( - occurrence.symbol?.startsWith('local ') && - occurrence.range.contains(new ScipPosition(position.line, position.character)) - ) { - return occurrences - .filter(reference => reference.symbol === occurrence.symbol) - .map(reference => ({ - repo, - file: path, - content: fileContent, - commitID: commit, - range: reference.range, - url: toPrettyBlobURL({ - filePath: path, - revision: commit, - repoName: repo, - commitID: commit, - position: { - line: reference.range.start.line + 1, - character: reference.range.start.character + 1, - }, - }), - lines: split(fileContent), - precise: false, - })) - } + if (view === null) { + return + } + const occurrences = view.state.facet(syntaxHighlight).occurrences + const { path, repo, position, fileContent: content, commit: commitID } = options + const lines = split(content) + const scipPosition = new ScipPosition(position.line, position.character) + for (const occurrence of occurrences) { + const symbol = occurrence.symbol + if (!(symbol?.startsWith('local ') && occurrence.range.contains(scipPosition))) { + continue } + return occurrences + .filter(reference => reference.symbol === symbol) + .map(reference => ({ + repo, + file: path, + content, + commitID, + range: reference.range, + url: toPrettyBlobURL({ + filePath: path, + revision: commitID, + repoName: repo, + commitID, + position: { + line: reference.range.start.line + 1, + character: reference.range.start.character + 1, + }, + }), + lines, + precise: false, + })) } - const filterReferences = (results: Location[]): Location[] => - filter ? results.filter(location => location.file.includes(filter)) : results + return +} +async function searchBasedReferencesViaSquirrel( + options: UseSearchBasedCodeIntelOptions +): Promise { + const { repo, position, path, commit, fileContent } = options + const symbol = await findSymbol({ repository: repo, path, commit, row: position.line, column: position.character }) + if (!symbol?.refs) { + return + } + // HISTORICAL NOTE: Squirrel only support find refs for locals + // (the code below uses the same 'path' value for all references, + // and is based as-is on the original code written by Chris), + // so we can delete this code once we have SCIP locals support + // for all the same languages that Squirrel does. + const lines = split(fileContent) + return symbol.refs.map(reference => ({ + repo, + file: path, + content: fileContent, + commitID: commit, + range: rangeToExtensionRange(reference), + url: toPrettyBlobURL({ + filePath: path, + revision: commit, + repoName: repo, + commitID: commit, + position: { + line: reference.row + 1, + character: reference.column + 1, + }, + }), + lines, + precise: false, + })) +} + +async function searchBasedDefinitionsViaSquirrel( + options: UseSearchBasedCodeIntelOptions +): Promise { + const { repo, commit, path, position, fileContent } = options const symbol = await findSymbol({ repository: repo, commit, path, row: position.line, column: position.character }) - if (symbol?.refs) { - return symbol.refs.map(reference => ({ + if (!symbol?.def) { + return + } + return [ + { repo, file: path, content: fileContent, commitID: commit, - range: rangeToExtensionRange(reference), + range: rangeToExtensionRange(symbol.def), url: toPrettyBlobURL({ filePath: path, revision: commit, repoName: repo, commitID: commit, position: { - line: reference.row + 1, - character: reference.column + 1, + line: symbol.def.row + 1, + character: symbol.def.column + 1, }, }), lines: split(fileContent), precise: false, - })) - } + }, + ] +} +async function searchBasedReferencesViaSearchQueries(options: UseSearchBasedCodeIntelOptions): Promise { + const { searchToken, path, repo, isFork, isArchived, commit, spec, filter } = options const queryTerms = referencesQuery({ searchToken, path, fileExts: spec.fileExts }) - const queryArguments = { - repo, - isFork, - isArchived, - commit, - queryTerms, - filterReferences, - } + const filterReferences = (results: Location[]): Location[] => + filter ? results.filter(location => location.file.includes(filter)) : results - const doSearch = (negateRepoFilter: boolean): Promise => + const doSearch = (repoFilter: RepoFilter): Promise => searchWithFallback( args => searchAndFilterReferences({ queryTerms: args.queryTerms, filterReferences }), - queryArguments, - negateRepoFilter, - getSetting + { repo, isFork, isArchived, commit, queryTerms, filterReferences }, + repoFilter, + options.getSetting ) - // Perform a search in the current git tree - const sameRepoReferences = doSearch(false) + const sameRepoReferences = doSearch('current-repo') // Perform an indexed search over all _other_ repositories. This // query is ineffective on DotCom as we do not keep repositories // in the index permanently. - const remoteRepoReferences = isSourcegraphDotCom() ? Promise.resolve([]) : doSearch(true) + const remoteRepoReferences = isSourcegraphDotCom() ? Promise.resolve([]) : doSearch('all-other-repos') - // Resolve then merge all references and sort them by proximity - // to the current text document path. - const referenceChunk = [sameRepoReferences, remoteRepoReferences] - const mergedReferences = flatten(await Promise.all(referenceChunk)) - return sortByProximity(mergedReferences, location.pathname) + return flatten(await Promise.all([sameRepoReferences, remoteRepoReferences])) } -export async function searchBasedDefinitions({ - repo, - isFork, - isArchived, - commit, - searchToken, - fileContent, - path, - position, - spec, - getSetting, - filter, -}: UseSearchBasedCodeIntelOptions): Promise { - const symbol = await findSymbol({ repository: repo, commit, path, row: position.line, column: position.character }) - if (symbol?.def) { - return [ - { - repo, - file: path, - content: fileContent, - commitID: commit, - range: rangeToExtensionRange(symbol.def), - url: toPrettyBlobURL({ - filePath: path, - revision: commit, - repoName: repo, - commitID: commit, - position: { - line: symbol.def.row + 1, - character: symbol.def.column + 1, - }, - }), - lines: split(fileContent), - precise: false, - }, - ] - } - +async function searchBasedDefinitionsViaSearchQueries(options: UseSearchBasedCodeIntelOptions): Promise { + const { searchToken, path, repo, isFork, fileContent, isArchived, commit, spec, filter } = options + // Construct base definition query without scoping terms + const queryTerms = definitionQuery({ searchToken, path, fileExts: spec.fileExts }) const filterDefinitions = (results: Location[]): Location[] => { const filteredByName = filter ? results.filter(location => location.file.includes(filter)) : results return spec?.filterDefinitions @@ -273,29 +264,15 @@ export async function searchBasedDefinitions({ : filteredByName } - // Construct base definition query without scoping terms - const queryTerms = definitionQuery({ searchToken, path, fileExts: spec.fileExts }) - const queryArguments = { - repo, - isFork, - isArchived, - commit, - path, - fileContent, - filterDefinitions, - queryTerms, - } - - const doSearch = (negateRepoFilter: boolean): Promise => + const doSearch = (repoFilter: RepoFilter): Promise => searchWithFallback( args => searchAndFilterDefinitions({ spec, path, filterDefinitions, queryTerms: args.queryTerms }), - queryArguments, - negateRepoFilter, - getSetting + { repo, isFork, isArchived, commit, path, fileContent, filterDefinitions, queryTerms }, + repoFilter, + options.getSetting ) - // Perform a search in the current git tree - const sameRepoDefinitions = doSearch(false) + const sameRepoDefinitions = doSearch('current-repo') // Return any local location definitions first const results = await sameRepoDefinitions @@ -307,7 +284,32 @@ export async function searchBasedDefinitions({ // an indexed search over all repositories. Do not do this on the DotCom // instance as we are unlikely to have indexed the relevant definition // and we'd end up jumping to what would seem like a random line of code. - return isSourcegraphDotCom() ? Promise.resolve([]) : doSearch(true) + return isSourcegraphDotCom() ? Promise.resolve([]) : doSearch('all-other-repos') +} + +// Originally based on the code from code-intel-extension +export async function searchBasedReferences(options: UseSearchBasedCodeIntelOptions): Promise { + const refsViaSCIPLocals = searchBasedReferencesViaSCIPLocals(options) + if (refsViaSCIPLocals) { + return refsViaSCIPLocals + } + + const refsViaSquirrel = await searchBasedReferencesViaSquirrel(options) + if (refsViaSquirrel) { + return refsViaSquirrel + } + + const refsViaSearchQueries = await searchBasedReferencesViaSearchQueries(options) + return sortByProximity(refsViaSearchQueries, options.path) +} + +export async function searchBasedDefinitions(options: UseSearchBasedCodeIntelOptions): Promise { + const defsViaSquirrel = await searchBasedDefinitionsViaSquirrel(options) + if (defsViaSquirrel) { + return defsViaSquirrel + } + + return searchBasedDefinitionsViaSearchQueries(options) } /**