diff --git a/extension/src/extension.ts b/extension/src/extension.ts index 761510f537..518337986c 100644 --- a/extension/src/extension.ts +++ b/extension/src/extension.ts @@ -50,6 +50,7 @@ import { Setup } from './setup' import { definedAndNonEmpty } from './util/array' import { stopProcesses } from './processExecution' import { Flag } from './cli/dvc/constants' +import { LanguageClientWrapper } from './lspClient/languageClient' export class Extension extends Disposable { protected readonly internalCommands: InternalCommands @@ -255,6 +256,8 @@ export class Extension extends Disposable { void showWalkthroughOnFirstUse(env.isNewAppInstall) this.dispose.track(recommendRedHatExtensionOnce()) + + this.dispose.track(new LanguageClientWrapper()) } public async initialize() { diff --git a/extension/src/fileSystem/index.ts b/extension/src/fileSystem/index.ts index f6a149eb27..fd611afdc1 100644 --- a/extension/src/fileSystem/index.ts +++ b/extension/src/fileSystem/index.ts @@ -204,3 +204,15 @@ export const getBinDisplayText = ( ? '.' + sep + relative(workspaceRoot, path) : path } + +export const readFileContents = ( + uriString: string +): { contents: string } | null => { + try { + const uri = Uri.parse(uriString) + if (!isDirectory(uri.fsPath)) { + return { contents: readFileSync(uri.fsPath, 'utf8') } + } + } catch {} + return null +} diff --git a/extension/src/lspClient/languageClient.ts b/extension/src/lspClient/languageClient.ts index 9c00e379a3..7efc6900c3 100644 --- a/extension/src/lspClient/languageClient.ts +++ b/extension/src/lspClient/languageClient.ts @@ -1,4 +1,4 @@ -import { Uri, workspace } from 'vscode' +import { workspace } from 'vscode' import { LanguageClient, LanguageClientOptions, @@ -6,9 +6,8 @@ import { TransportKind } from 'vscode-languageclient/node' import { documentSelector, serverModule } from 'dvc-vscode-lsp' -import { readFileSync } from 'fs-extra' import { Disposable } from '../class/dispose' -import { findFiles } from '../fileSystem/workspace' +import { readFileContents } from '../fileSystem' export class LanguageClientWrapper extends Disposable { private client: LanguageClient @@ -20,9 +19,7 @@ export class LanguageClientWrapper extends Disposable { documentSelector, synchronize: { - fileEvents: workspace.createFileSystemWatcher( - '**/*.{yaml,dvc,dvc.lock,json,toml}' - ) + fileEvents: workspace.createFileSystemWatcher('**/dvc.yaml') } } @@ -35,37 +32,11 @@ export class LanguageClientWrapper extends Disposable { ) ) - // Start the client. This will also launch the server - void this.start() - } - - async start() { - await this.client.start() - - const files = await findFiles('**/*.{yaml,json,py,toml}', '.??*') - - const textDocuments = files.map(filePath => { - const uri = Uri.file(filePath).toString() - const languageId = filePath.endsWith('yaml') ? 'yaml' : 'json' - const text = readFileSync(filePath, 'utf8') - - return { - languageId, - text, - uri, - version: 0 - } - }) - - await this.client.sendRequest('initialTextDocuments', { - textDocuments - }) - - return this - } + this.dispose.track( + this.client.onRequest('readFileContents', readFileContents) + ) - stop() { - void this.client.stop() + void this.client.start() } private getServerOptions(): ServerOptions { diff --git a/extension/src/test/suite/fileSystem/index.test.ts b/extension/src/test/suite/fileSystem/index.test.ts index 6dc9bfd0f9..5243d9a040 100644 --- a/extension/src/test/suite/fileSystem/index.test.ts +++ b/extension/src/test/suite/fileSystem/index.test.ts @@ -1,12 +1,18 @@ import { join, resolve } from 'path' import process from 'process' +import { Uri } from 'vscode' import { afterEach, beforeEach, describe, it, suite } from 'mocha' import { expect } from 'chai' import { Disposable } from '@hediet/std/disposable' import { restore } from 'sinon' import { ensureFileSync, removeSync, writeFileSync } from 'fs-extra' import { dvcDemoPath } from '../../util' -import { checkSignalFile, exists, getGitPath } from '../../../fileSystem' +import { + checkSignalFile, + exists, + getGitPath, + readFileContents +} from '../../../fileSystem' import { gitPath } from '../../../cli/git/constants' import { GitReader } from '../../../cli/git/reader' import { standardizePath } from '../../../fileSystem/path' @@ -93,4 +99,20 @@ suite('File System Test Suite', () => { expect(exists(mockSignalFilePath)).to.be.false }) }) + + describe('readFileContents', () => { + it('should read the contents of a file when it exists', () => { + const uriString = Uri.file(join(dvcDemoPath, 'train.py')).toString() + + const file = readFileContents(uriString) + expect(file?.contents).to.contain('main()') + }) + + it('should return null when the file cannot be found', () => { + const uriString = 'file:///some/fun/file.txt' + + const contents = readFileContents(uriString) + expect(contents).to.be.null + }) + }) }) diff --git a/languageServer/src/LanguageServer.ts b/languageServer/src/LanguageServer.ts index b30b1afad7..d6f3e279ba 100644 --- a/languageServer/src/LanguageServer.ts +++ b/languageServer/src/LanguageServer.ts @@ -1,3 +1,4 @@ +import { dirname } from 'path' import { TextDocuments, InitializeResult, @@ -7,11 +8,9 @@ import { CodeActionParams, DefinitionParams, SymbolKind, - Location, - Position, - Range, DocumentSymbol, - TextDocumentItem + Connection, + Location } from 'vscode-languageserver/node' import { TextDocument } from 'vscode-languageserver-textdocument' import { URI } from 'vscode-uri' @@ -19,7 +18,6 @@ import { TextDocumentWrapper } from './TextDocumentWrapper' export class LanguageServer { private documentsKnownToEditor!: TextDocuments - private documentsFromDvcClient: TextDocumentWrapper[] = [] public listen(connection: _Connection) { this.documentsKnownToEditor = new TextDocuments(TextDocument) @@ -31,27 +29,9 @@ export class LanguageServer { return null } - return this.onDefinition(params) + return this.onDefinition(params, connection) }) - connection.onRequest( - 'initialTextDocuments', - (params: { textDocuments: TextDocumentItem[] }) => { - this.documentsFromDvcClient = params.textDocuments.map( - ({ uri, languageId, version, text: content }) => { - const textDocument = TextDocument.create( - uri, - languageId, - version, - content - ) - - return this.wrap(textDocument) - } - ) - } - ) - this.documentsKnownToEditor.listen(connection) connection.listen() @@ -59,32 +39,16 @@ export class LanguageServer { private getAllDocuments() { const openDocuments = this.documentsKnownToEditor.all() - const acc: TextDocumentWrapper[] = openDocuments.map(doc => this.wrap(doc)) - - for (const textDocument of this.documentsFromDvcClient) { - const userAlreadyOpenedIt = this.documentsKnownToEditor.get( - textDocument.uri - ) - - if (!userAlreadyOpenedIt) { - acc.push(textDocument) - } - } - - return acc + return openDocuments.map(doc => this.wrap(doc)) } - private getDvcTextDocument( - params: TextDocumentPositionParams | CodeActionParams - ) { + private getDocument(params: TextDocumentPositionParams | CodeActionParams) { const uri = params.textDocument.uri + const doc = this.documentsKnownToEditor.get(uri) if (!doc) { - const alternative = this.documentsFromDvcClient.find( - txtDoc => txtDoc.uri === uri - ) - return alternative ?? null + return null } return this.wrap(doc) @@ -94,78 +58,70 @@ export class LanguageServer { return new TextDocumentWrapper(doc) } - private getFilePathLocations( - symbolUnderCursor: DocumentSymbol, - allDocs: TextDocumentWrapper[] - ) { + private getKnownDocumentLocations(symbolUnderCursor: DocumentSymbol) { if (symbolUnderCursor.kind !== SymbolKind.File) { return [] } const filePath = symbolUnderCursor.name - const matchingFiles = allDocs.filter(doc => - URI.file(doc.uri).fsPath.endsWith(filePath) + const matchingFiles = this.getAllDocuments().filter(doc => + URI.parse(doc.uri).fsPath.endsWith(filePath) ) - return matchingFiles.map(doc => { - const uri = doc.uri - const start = Position.create(0, 0) - const end = doc.positionAt(doc.getText().length - 1) - const range = Range.create(start, end) - - return Location.create(uri, range) - }) + return matchingFiles.map(doc => doc.getLocation()) } - private getLocationsFromOtherDocuments( - symbolUnderCursor: DocumentSymbol, - allDocs: TextDocumentWrapper[] - ) { - const locationsAccumulator = [] - - for (const txtDoc of allDocs) { - const locations = txtDoc.findLocationsFor(symbolUnderCursor) - locationsAccumulator.push(...locations) - } + private async onDefinition(params: DefinitionParams, connection: Connection) { + const document = this.getDocument(params) - return locationsAccumulator - } - - private onDefinition(params: DefinitionParams) { - const document = this.getDvcTextDocument(params) const symbolUnderCursor = document?.symbolAt(params.position) - if (document && symbolUnderCursor) { - const allDocs = this.getAllDocuments() - const locationsAccumulator = [] - - const fileLocations = this.getFilePathLocations( - symbolUnderCursor, - allDocs - ) + if (!(document && symbolUnderCursor)) { + return null + } - locationsAccumulator.push(...fileLocations) + const fileLocations = this.getKnownDocumentLocations(symbolUnderCursor) - const locationsFromOtherDocuments = this.getLocationsFromOtherDocuments( + if (fileLocations.length === 0) { + await this.checkIfSymbolsAreFiles( + connection, + document, symbolUnderCursor, - allDocs + fileLocations ) + } - locationsAccumulator.push(...locationsFromOtherDocuments) + if (fileLocations.length > 0) { + return this.arrayOrSingleResponse(fileLocations) + } - const externalLocations = locationsAccumulator.filter( - location => location.uri !== document.uri - ) + return null + } - if (externalLocations.length > 0) { - return this.arrayOrSingleResponse(externalLocations) + private async checkIfSymbolsAreFiles( + connection: _Connection, + document: TextDocumentWrapper, + symbolUnderCursor: DocumentSymbol, + fileLocations: Location[] + ) { + for (const possibleFile of symbolUnderCursor.name.split(' ')) { + const possiblePath = URI.parse( + [dirname(document.uri), possibleFile].join('/') + ).toString() + const file = await connection.sendRequest<{ + contents: string + } | null>('readFileContents', possiblePath) + if (file) { + const location = this.getLocation(possiblePath, file.contents) + fileLocations.push(location) } - - return this.arrayOrSingleResponse(locationsAccumulator) } + } - return null + private getLocation(path: string, contents: string) { + const doc = this.wrap(TextDocument.create(path, 'plain/text', 0, contents)) + return doc.getLocation() } private arrayOrSingleResponse(elements: T[]) { diff --git a/languageServer/src/TextDocumentWrapper.ts b/languageServer/src/TextDocumentWrapper.ts index 8e8b5b207b..d88ac5f3d5 100644 --- a/languageServer/src/TextDocumentWrapper.ts +++ b/languageServer/src/TextDocumentWrapper.ts @@ -16,7 +16,6 @@ import { Pair, isPair } from 'yaml' -import { alphadecimalWords, variableTemplates } from './regexes' import { ITextDocumentWrapper } from './ITextDocumentWrapper' export class TextDocumentWrapper implements ITextDocumentWrapper { @@ -29,12 +28,17 @@ export class TextDocumentWrapper implements ITextDocumentWrapper { this.uri = this.textDocument.uri } - public offsetAt(position: Position) { - return this.textDocument.offsetAt(position) + public getLocation() { + const uri = this.uri + const start = Position.create(0, 0) + const end = this.positionAt(this.getText().length - 1) + const range = Range.create(start, end) + + return Location.create(uri, range) } - public getText() { - return this.textDocument.getText() + public offsetAt(position: Position) { + return this.textDocument.offsetAt(position) } public positionAt(offset: number) { @@ -45,62 +49,12 @@ export class TextDocumentWrapper implements ITextDocumentWrapper { return parseDocument(this.getText()) } - public findLocationsFor(aSymbol: DocumentSymbol) { - const parts = aSymbol.name.split(/\s/g) - const txt = this.getText() - - const acc: Location[] = [] - for (const str of parts) { - const index = txt.indexOf(str) - if (index <= 0) { - continue - } - const pos = this.positionAt(index) - const range = this.symbolAt(pos)?.range - if (!range) { - continue - } - acc.push(Location.create(this.uri, range)) - } - return acc - } - public symbolAt(position: Position): DocumentSymbol | undefined { return this.symbolScopeAt(position).pop() } - private getTemplateExpressionSymbolsInsideScalar( - scalarValue: string, - nodeOffset: number - ) { - const templateSymbols: DocumentSymbol[] = [] - - const templates = scalarValue.matchAll(variableTemplates) - for (const template of templates) { - const expression = template[1] - const expressionOffset: number = nodeOffset + (template.index ?? 0) + 2 // To account for the '${' - const symbols = expression.matchAll(alphadecimalWords) // It works well for now. We can always add more sophistication when needed. - - for (const templateSymbol of symbols) { - const symbolStart = (templateSymbol.index ?? 0) + expressionOffset - const symbolEnd = symbolStart + templateSymbol[0].length - const symbolRange = Range.create( - this.positionAt(symbolStart), - this.positionAt(symbolEnd) - ) - templateSymbols.push( - DocumentSymbol.create( - templateSymbol[0], - undefined, - SymbolKind.Variable, - symbolRange, - symbolRange - ) - ) - } - } - - return templateSymbols + private getText() { + return this.textDocument.getText() } private yamlScalarNodeToDocumentSymbols( @@ -111,22 +65,19 @@ export class TextDocumentWrapper implements ITextDocumentWrapper { let symbolKind: SymbolKind = SymbolKind.String - if (/\.[A-Za-z]+$/.test(nodeValue)) { + if (/\.[A-Za-z]+$/.test(nodeValue) && !nodeValue.includes(' ')) { symbolKind = SymbolKind.File } - const symbolsSoFar: DocumentSymbol[] = [ + return [ DocumentSymbol.create( nodeValue, undefined, symbolKind, Range.create(this.positionAt(nodeStart), this.positionAt(nodeEnd)), Range.create(this.positionAt(nodeStart), this.positionAt(valueEnd)) - ), - ...this.getTemplateExpressionSymbolsInsideScalar(nodeValue, nodeStart) + ) ] - - return symbolsSoFar } private yamlNodeToDocumentSymbols( diff --git a/languageServer/src/regexes.ts b/languageServer/src/regexes.ts deleted file mode 100644 index 3c63bc1d2f..0000000000 --- a/languageServer/src/regexes.ts +++ /dev/null @@ -1,3 +0,0 @@ -export const variableTemplates = /\${([^}]+)}/g -export const filePaths = /[\d/A-Za-z]+\.[A-Za-z]+/g -export const alphadecimalWords = /[\dA-Za-z]+/g diff --git a/languageServer/src/test/definitions.test.ts b/languageServer/src/test/definitions.test.ts index f6021183e7..8cb29797a0 100644 --- a/languageServer/src/test/definitions.test.ts +++ b/languageServer/src/test/definitions.test.ts @@ -1,21 +1,26 @@ +import { join } from 'path' import { Position, Range } from 'vscode-languageserver/node' +import { URI } from 'vscode-uri' import { - foreach_dvc_yaml, params_dvc_yaml, - vars_dvc_yaml + plots_dvc_yaml, + train_dvc_yaml } from './fixtures/examples/valid' import { params } from './fixtures/params' +import { train } from './fixtures/python' import { requestDefinitions } from './utils/requestDefinitions' import { openTheseFilesAndNotifyServer } from './utils/openTheseFilesAndNotifyServer' import { disposeTestConnections, setupTestConnections } from './utils/setup-test-connections' -import { sendTheseFilesToServer } from './utils/sendTheseFilesToServer' + +const mockedReadFileContents = jest.fn() describe('textDocument/definitions', () => { beforeEach(() => { - setupTestConnections() + jest.resetAllMocks() + setupTestConnections(mockedReadFileContents) }) afterEach(() => { @@ -49,51 +54,60 @@ describe('textDocument/definitions', () => { mockPath: 'params.yaml' } ]) - const response = await requestDefinitions(dvcYaml, 'auc') + const response = await requestDefinitions(dvcYaml, '- params.yaml', 2) expect(response).toBeTruthy() expect(response).toStrictEqual({ - range: Range.create(Position.create(4, 0), Position.create(4, 3)), + range: Range.create(Position.create(0, 0), Position.create(5, 9)), uri: 'file:///params.yaml' }) }) - it('should be able to read a complicated command from a stage', async () => { + it('should try to read the file system when a python file is unknown', async () => { + const trainUri = URI.file(join(__dirname, 'train.py')).toString() + + mockedReadFileContents.mockImplementation(path => { + if (path === trainUri) { + return { contents: train } + } + return null + }) + const [dvcYaml] = await openTheseFilesAndNotifyServer([ { languageId: 'yaml', - mockContents: foreach_dvc_yaml, - mockPath: 'dvc.yaml' + mockContents: train_dvc_yaml, + mockPath: join(__dirname, 'dvc.yaml') } ]) - const response = await requestDefinitions(dvcYaml, '{item.os}') + + const response = await requestDefinitions(dvcYaml, 'train.py') + + expect(mockedReadFileContents).toHaveBeenCalledWith(trainUri, { + _isCancelled: false + }) expect(response).toBeTruthy() expect(response).toStrictEqual({ - range: Range.create(Position.create(15, 8), Position.create(15, 10)), - uri: 'file:///dvc.yaml' + range: Range.create(Position.create(0, 0), Position.create(7, 13)), + uri: trainUri }) }) - it('should provide a single location that points to the top of the file path symbol', async () => { - const [dvcYaml] = await sendTheseFilesToServer([ + it('should return null if the provided symbol is not in the known documents and cannot be found on the file system', async () => { + mockedReadFileContents.mockResolvedValue(null) + + const [dvcYaml] = await openTheseFilesAndNotifyServer([ { languageId: 'yaml', - mockContents: vars_dvc_yaml, - mockPath: 'dvc.yaml' - }, - { - languageId: 'json', - mockContents: '', - mockPath: 'params.json' + mockContents: plots_dvc_yaml, + mockPath: join(__dirname, 'dvc.yaml') } ]) - const response = await requestDefinitions(dvcYaml, 'params.json') - expect(response).toBeTruthy() - expect(response).toStrictEqual({ - range: Range.create(Position.create(0, 0), Position.create(0, 0)), - uri: 'file:///params.json' - }) + const response = await requestDefinitions(dvcYaml, 'plot5') + + expect(mockedReadFileContents).toHaveBeenCalledTimes(1) + expect(response).toBeNull() }) }) diff --git a/languageServer/src/test/fixtures/examples/valid/index.ts b/languageServer/src/test/fixtures/examples/valid/index.ts index 5c5d6601d4..93a35e798e 100644 --- a/languageServer/src/test/fixtures/examples/valid/index.ts +++ b/languageServer/src/test/fixtures/examples/valid/index.ts @@ -97,8 +97,9 @@ stages: use-params_stage: cmd: cat params.yaml > params2.yaml params: - - auc - - loss + - params.yaml: + - auc + - loss outs: - params2.yaml use-custom-params_file: @@ -116,6 +117,21 @@ stages: outs: - params2.yaml ` +export const train_dvc_yaml = `stages: +train: + cmd: python train.py + deps: + - data + - train.py + params: + - params.yaml: + outs: + - model.pt: + checkpoint: true + - training/plots: + persist: true + - hist.csv +` export const plots_dvc_yaml = ` stages: stage_one: @@ -162,7 +178,6 @@ vars: - myvar: 'value' - config/myapp.yaml - params.json:clean,feats - stages: test_vars: vars: @@ -178,5 +193,4 @@ stages: - model: filename: 'model-us.hdf5' cmd: echo \${item} \${model.filename} - -` + ` diff --git a/languageServer/src/test/fixtures/python/index.ts b/languageServer/src/test/fixtures/python/index.ts new file mode 100644 index 0000000000..c813ba3c32 --- /dev/null +++ b/languageServer/src/test/fixtures/python/index.ts @@ -0,0 +1,9 @@ +export const train = `from secret_notebook import model + +def main(): + """Don't tell the boss that the model in production is actually a notebook on my laptop.""" + model.train() + +if __name__ == "__main__": + main() +` diff --git a/languageServer/src/test/utils/openTheseFilesAndNotifyServer.ts b/languageServer/src/test/utils/openTheseFilesAndNotifyServer.ts index 1134ca0d12..7ae49968bb 100644 --- a/languageServer/src/test/utils/openTheseFilesAndNotifyServer.ts +++ b/languageServer/src/test/utils/openTheseFilesAndNotifyServer.ts @@ -4,7 +4,7 @@ import { URI } from 'vscode-uri' import { client } from './setup-test-connections' export const openTheseFilesAndNotifyServer = async ( - files: Array<{ mockPath: string; mockContents: string; languageId: string }> + files: { mockPath: string; mockContents: string; languageId: string }[] ) => { const filesToReturn: TextDocument[] = [] diff --git a/languageServer/src/test/utils/requestDefinitions.ts b/languageServer/src/test/utils/requestDefinitions.ts index 89ae70d642..8373e50b14 100644 --- a/languageServer/src/test/utils/requestDefinitions.ts +++ b/languageServer/src/test/utils/requestDefinitions.ts @@ -4,16 +4,18 @@ import { client } from './setup-test-connections' export const requestDefinitions = async ( textDocument: TextDocument, - substring: string + substring: string, + offset = 0 ) => { const text = textDocument.getText() const uri = textDocument.uri const symbolOffset = text.indexOf(substring) - const position = textDocument.positionAt(symbolOffset) + + const { character, line } = textDocument.positionAt(symbolOffset) const params: DefinitionParams = { - position, + position: { character: character + offset, line }, textDocument: { uri } diff --git a/languageServer/src/test/utils/sendTheseFilesToServer.ts b/languageServer/src/test/utils/sendTheseFilesToServer.ts deleted file mode 100644 index 8ff75626f3..0000000000 --- a/languageServer/src/test/utils/sendTheseFilesToServer.ts +++ /dev/null @@ -1,27 +0,0 @@ -import { TextDocument } from 'vscode-languageserver-textdocument' -import { URI } from 'vscode-uri' -import { client } from './setup-test-connections' - -export const sendTheseFilesToServer = async ( - files: Array<{ mockPath: string; mockContents: string; languageId: string }> -) => { - const filesToReturn: TextDocument[] = [] - - for (const { mockPath, mockContents, languageId } of files) { - const uri = URI.file(mockPath).toString() - const textDocument = TextDocument.create(uri, languageId, 1, mockContents) - filesToReturn.push(textDocument) - } - await client.sendRequest('initialTextDocuments', { - textDocuments: filesToReturn.map(textDocument => { - return { - languageId: 'yaml', - text: textDocument.getText(), - uri: textDocument.uri, - version: textDocument.version - } - }) - }) - - return filesToReturn -} diff --git a/languageServer/src/test/utils/setup-test-connections.ts b/languageServer/src/test/utils/setup-test-connections.ts index d32ded5177..73b98fa038 100644 --- a/languageServer/src/test/utils/setup-test-connections.ts +++ b/languageServer/src/test/utils/setup-test-connections.ts @@ -15,13 +15,16 @@ class TestStream extends Duplex { export let server: Connection export let client: Connection -export const setupTestConnections = () => { +export const setupTestConnections = ( + mockedReadFileContents: typeof jest.fn +) => { const dvcLanguageService = new LanguageServer() const up = new TestStream() const down = new TestStream() server = createConnection(up, down) client = createConnection(down, up) + client.onRequest('readFileContents', mockedReadFileContents) dvcLanguageService.listen(server) client.listen()