Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LSP: New settings option to Disable server and Better Goto Definition for property and file paths (1/2) #2504

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions extension/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -548,6 +548,12 @@
"type": "boolean",
"default": null
},
"dvc.disableLanguageServer": {
"title": "%config.disableLanguageServer.title%",
"description": "%config.disableLanguageServer.description%",
"type": "boolean",
"default": null
},
"dvc.dvcPath": {
"title": "%config.dvcPath.title%",
"description": "%config.dvcPath.description%",
Expand Down
2 changes: 2 additions & 0 deletions extension/package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@
"config.doNotShowWalkthroughAfterInstall.title": "Do not show the Get Started page.",
"config.doNotShowUnableToFilter.description": "Do not warn before disabling auto-apply filters when these would result in too many experiments being selected.",
"config.doNotShowUnableToFilter.title": "Do not warn before disabling auto-apply filters to experiment selection.",
"config.disableLanguageServer.description": "Disable the language server for dvc.yaml files.",
"config.disableLanguageServer.title": "Disable language server",
"config.dvcPath.description": "Path or shell command to the DVC binary. Required unless Microsoft's Python extension is installed and the `dvc` package found in its environment.",
"config.dvcPath.title": "DVC Path (or shell command)",
"config.expTableHeadMaxLayers.title": "Maximum depth of Experiment table head rows",
Expand Down
3 changes: 3 additions & 0 deletions extension/src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ import { collectWorkspaceScale } from './telemetry/collect'
import { createFileSystemWatcher } from './fileSystem/watcher'
import { GitExecutor } from './cli/git/executor'
import { GitReader } from './cli/git/reader'
import { LanguageClientWrapper } from './lspClient/languageClient'

export class Extension extends Disposable implements IExtension {
protected readonly internalCommands: InternalCommands
Expand Down Expand Up @@ -92,6 +93,8 @@ export class Extension extends Disposable implements IExtension {
this.setCommandsAvailability(false)
this.setProjectAvailability()

this.dispose.track(new LanguageClientWrapper())

this.resourceLocator = this.dispose.track(
new ResourceLocator(context.extensionUri)
)
Expand Down
54 changes: 24 additions & 30 deletions extension/src/lspClient/languageClient.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
import { Uri, workspace } from 'vscode'
import {
LanguageClient,
LanguageClientOptions,
ServerOptions,
TransportKind
} from 'vscode-languageclient/node'
import { documentSelector, serverModule } from 'dvc-vscode-lsp'
import { readFileSync } from 'fs-extra'
import { ConfigurationChangeEvent, workspace } from 'vscode'
import { Disposable } from '../class/dispose'
import { findFiles } from '../fileSystem/workspace'
import { ConfigKey, getConfigValue } from '../vscode/config'

export class LanguageClientWrapper extends Disposable {
private client: LanguageClient
Expand All @@ -17,13 +16,7 @@ export class LanguageClientWrapper extends Disposable {
super()

const clientOptions: LanguageClientOptions = {
documentSelector,

synchronize: {
fileEvents: workspace.createFileSystemWatcher(
'**/*.{yaml,dvc,dvc.lock,json,toml}'
)
}
documentSelector
}

this.client = this.dispose.track(
Expand All @@ -35,31 +28,24 @@ export class LanguageClientWrapper extends Disposable {
)
)

// Start the client. This will also launch the server
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')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was very confused as to how the "ring-fenced" language server managed to bring down the extension host.

This would be what caused the crashes. A synchronous call to read every file would definitely bring down the thread.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The watcher would not have caused this issue. We already have at least one watcher which watches the entire workspace (and more).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more I think about this the more I feel like the approach was backwards. We should be parsing each dvc.yaml file when it is opened and checking to see which symbols are files. From there we read those files so that we can jump into them if needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (!this.languageServerDisabled()) {
// This will also start the server process
await this.client.start()

return {
languageId,
text,
uri,
version: 0
}
})

await this.client.sendRequest('initialTextDocuments', {
textDocuments
})
workspace.onDidChangeConfiguration((event: ConfigurationChangeEvent) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the language server is on and I disable it in the settings, the server turns off without any need to reload. But if I try to turn it back on in settings, I need to reload the page to see the language server running again:

Screen.Recording.2022-10-05.at.4.13.12.PM.mov

Do we want it to be able to turn it back on without needing to reload?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[I] this is a disposable and should be tracked.

if (
event.affectsConfiguration(ConfigKey.DISABLE_LANGUAGE_SERVER) &&
this.isRunning() &&
this.languageServerDisabled()
) {
this.stop()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we dispose of the onDidChangeConfiguration here? We could get rid of the this.isRunning() && this.languageServerDisabled() as well as this would only run once (when disabling the server the first time).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

}
})
}

return this
}
Expand All @@ -68,6 +54,14 @@ export class LanguageClientWrapper extends Disposable {
this.client.stop()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Q] Are you sure that stopping the client stops the server?

}

private isRunning() {
return this.client.isRunning()
}

private languageServerDisabled() {
return getConfigValue<boolean>(ConfigKey.DISABLE_LANGUAGE_SERVER)
}

private getServerOptions(): ServerOptions {
const debugOptions = { execArgv: ['--nolazy', '--inspect=6009'] }

Expand Down
1 change: 1 addition & 0 deletions extension/src/vscode/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ export enum ConfigKey {
DO_NOT_SHOW_CLI_UNAVAILABLE = 'dvc.doNotShowCliUnavailable',
DO_NOT_SHOW_WALKTHROUGH_AFTER_INSTALL = 'dvc.doNotShowWalkthroughAfterInstall',
DO_NOT_SHOW_UNABLE_TO_FILTER = 'dvc.doNotShowUnableToFilter',
DISABLE_LANGUAGE_SERVER = 'dvc.disableLanguageServer',
EXP_TABLE_HEAD_MAX_LAYERS = 'dvc.expTableHeadMaxLayers',
DVC_PATH = 'dvc.dvcPath',
PYTHON_PATH = 'dvc.pythonPath'
Expand Down
58 changes: 20 additions & 38 deletions languageServer/src/LanguageServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,14 @@ import {
Location,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File definitions inside of cmds don't seem to work correctly on my end:

Screen.Recording.2022-10-05.at.5.35.47.PM.mov

Position,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should be shown if a file hasn't been opened yet? Currently, it appears to take us somewhere that doesn't seem to make sense:

Screen.Recording.2022-10-05.at.5.39.18.PM.mov

Range,
DocumentSymbol,
TextDocumentItem
DocumentSymbol
} from 'vscode-languageserver/node'
import { TextDocument } from 'vscode-languageserver-textdocument'
import { URI } from 'vscode-uri'
import { TextDocumentWrapper } from './TextDocumentWrapper'

export class LanguageServer {
private documentsKnownToEditor!: TextDocuments<TextDocument>
private documentsFromDvcClient: TextDocumentWrapper[] = []

public listen(connection: _Connection) {
this.documentsKnownToEditor = new TextDocuments(TextDocument)
Expand All @@ -34,24 +32,6 @@ export class LanguageServer {
return this.onDefinition(params)
})

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()
Expand All @@ -61,16 +41,6 @@ export class LanguageServer {
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
}

Expand All @@ -81,10 +51,7 @@ export class LanguageServer {
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)
Expand All @@ -94,18 +61,33 @@ export class LanguageServer {
return new TextDocumentWrapper(doc)
}

private isPossibleFilePath(symbol: DocumentSymbol) {
const isFile = symbol.kind === SymbolKind.File
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Q] any ideas how symbol.kind is set to SymbolKind.File?

const isProperty = symbol.kind === SymbolKind.Property

return isFile || (symbol.detail && isProperty)
}

private getFilePathFromSymbol(symbol: DocumentSymbol) {
if (symbol.kind === SymbolKind.File) {
return symbol.name
}

return symbol.detail ?? ''
}

private getFilePathLocations(
symbolUnderCursor: DocumentSymbol,
allDocs: TextDocumentWrapper[]
) {
if (symbolUnderCursor.kind !== SymbolKind.File) {
if (!this.isPossibleFilePath(symbolUnderCursor)) {
return []
}

const filePath = symbolUnderCursor.name
const filePath = this.getFilePathFromSymbol(symbolUnderCursor)

const matchingFiles = allDocs.filter(doc =>
URI.file(doc.uri).fsPath.endsWith(filePath)
URI.file(doc.uri).path.endsWith(filePath)
)

return matchingFiles.map(doc => {
Expand Down
Loading