From 02c5127d92c7ebb98751865e228ce9ecf89f870b Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Tue, 4 May 2021 13:21:52 -0700 Subject: [PATCH 1/4] Add Default LS setting --- package.json | 3 +- .../common/defaultlanguageServer.ts | 57 ++++++++++ src/client/common/configSettings.ts | 27 +++-- src/client/common/configuration/service.ts | 4 +- src/client/common/experiments/helpers.ts | 46 +------- src/client/extensionActivation.ts | 6 +- .../defaultLanguageServer.unit.test.ts | 103 ++++++++++++++++++ .../common/experiments/helpers.unit.test.ts | 97 +---------------- 8 files changed, 191 insertions(+), 152 deletions(-) create mode 100644 src/client/activation/common/defaultlanguageServer.ts create mode 100644 src/test/activation/defaultLanguageServer.unit.test.ts diff --git a/package.json b/package.json index 1d3b03029875..6cdd20c5b485 100644 --- a/package.json +++ b/package.json @@ -1225,13 +1225,14 @@ "python.languageServer": { "type": "string", "enum": [ + "Default", "Jedi", "JediLSP", "Pylance", "Microsoft", "None" ], - "default": "Jedi", + "default": "Default", "description": "Defines type of the language server.", "scope": "window" }, diff --git a/src/client/activation/common/defaultlanguageServer.ts b/src/client/activation/common/defaultlanguageServer.ts new file mode 100644 index 000000000000..15a0ab5c28b6 --- /dev/null +++ b/src/client/activation/common/defaultlanguageServer.ts @@ -0,0 +1,57 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { injectable } from 'inversify'; +import { PYLANCE_EXTENSION_ID } from '../../common/constants'; +import { JediLSP } from '../../common/experiments/groups'; +import { IDefaultLanguageServer, IExperimentService, IExtensions } from '../../common/types'; +import { IServiceManager } from '../../ioc/types'; +import { ILSExtensionApi } from '../node/languageServerFolderService'; +import { LanguageServerType } from '../types'; + +export type PotentialDefault = LanguageServerType.Jedi | LanguageServerType.JediLSP | LanguageServerType.Node; + +@injectable() +class DefaultLanguageServer implements IDefaultLanguageServer { + public readonly defaultLSType: PotentialDefault; + + constructor(defaultServer: PotentialDefault) { + this.defaultLSType = defaultServer; + } +} + +export async function setDefaultLanguageServer( + experimentService: IExperimentService, + extensions: IExtensions, + serviceManager: IServiceManager, +): Promise { + const lsType = await getDefaultLanguageServer(experimentService, extensions); + console.log(`Default LS will be ${lsType}`); + serviceManager.addSingletonInstance( + IDefaultLanguageServer, + new DefaultLanguageServer(lsType), + ); +} + +async function getDefaultLanguageServer( + experimentService: IExperimentService, + extensions: IExtensions, +): Promise { + if (extensions.getExtension(PYLANCE_EXTENSION_ID)) { + return LanguageServerType.Node; + } + + // If Pylance is installed and functional, use it as the default. + // TODO: This causes a dependency cycle, as VS Code is trying to wait for python to even start pylance... + // console.log('Activating Pylance'); + // const pylance = await extensions.getExtension(PYLANCE_EXTENSION_ID)?.activate(); + // console.log('Activated Pylance'); + // if (pylance && Object.keys(pylance).length !== 0) { + // return LanguageServerType.Node; + // } + + // Otherwise, use jedi. + return (await experimentService.inExperiment(JediLSP.experiment)) + ? LanguageServerType.JediLSP + : LanguageServerType.Jedi; +} diff --git a/src/client/common/configSettings.ts b/src/client/common/configSettings.ts index 193817bc74b3..ae2caf6be20a 100644 --- a/src/client/common/configSettings.ts +++ b/src/client/common/configSettings.ts @@ -167,7 +167,7 @@ export class PythonSettings implements IPythonSettings { private readonly experimentsManager?: IExperimentsManager, private readonly interpreterPathService?: IInterpreterPathService, private readonly interpreterSecurityService?: IInterpreterSecurityService, - private readonly defaultJedi?: IDefaultLanguageServer, + private readonly defaultLS?: IDefaultLanguageServer, ) { this.workspace = workspace || new WorkspaceService(); this.workspaceRoot = workspaceFolder; @@ -181,7 +181,7 @@ export class PythonSettings implements IPythonSettings { experimentsManager?: IExperimentsManager, interpreterPathService?: IInterpreterPathService, interpreterSecurityService?: IInterpreterSecurityService, - defaultJedi?: IDefaultLanguageServer, + defaultLS?: IDefaultLanguageServer, ): PythonSettings { workspace = workspace || new WorkspaceService(); const workspaceFolderUri = PythonSettings.getSettingsUriAndTarget(resource, workspace).uri; @@ -195,7 +195,7 @@ export class PythonSettings implements IPythonSettings { experimentsManager, interpreterPathService, interpreterSecurityService, - defaultJedi, + defaultLS, ); PythonSettings.pythonSettings.set(workspaceFolderKey, settings); // Pass null to avoid VSC from complaining about not passing in a value. @@ -284,14 +284,21 @@ export class PythonSettings implements IPythonSettings { this.useIsolation = systemVariables.resolveAny(pythonSettings.get('useIsolation', true))!; - const defaultServer = this.defaultJedi - ? this.defaultJedi.defaultLSType - : pythonSettings.get('languageServer'); - let ls = defaultServer ?? LanguageServerType.Jedi; - ls = systemVariables.resolveAny(ls); - if (!Object.values(LanguageServerType).includes(ls)) { - ls = LanguageServerType.Jedi; + // Get as a string and verify; don't just accept. + let userLS = pythonSettings.get('languageServer'); + userLS = systemVariables.resolveAny(userLS); + + let ls: LanguageServerType; + if ( + !userLS || + userLS === 'Default' || + !Object.values(LanguageServerType).includes(userLS as LanguageServerType) + ) { + ls = this.defaultLS?.defaultLSType ?? LanguageServerType.Jedi; + } else { + ls = userLS as LanguageServerType; } + this.languageServer = ls; this.jediPath = systemVariables.resolveAny(pythonSettings.get('jediPath'))!; diff --git a/src/client/common/configuration/service.ts b/src/client/common/configuration/service.ts index 8fed2b50b266..ea5abd493b24 100644 --- a/src/client/common/configuration/service.ts +++ b/src/client/common/configuration/service.ts @@ -37,7 +37,7 @@ export class ConfigurationService implements IConfigurationService { const interpreterSecurityService = this.serviceContainer.get( IInterpreterSecurityService, ); - const defaultJedi = this.serviceContainer.tryGet(IDefaultLanguageServer); + const defaultLS = this.serviceContainer.get(IDefaultLanguageServer); return PythonSettings.getInstance( resource, InterpreterAutoSelectionService, @@ -45,7 +45,7 @@ export class ConfigurationService implements IConfigurationService { experiments, interpreterPathService, interpreterSecurityService, - defaultJedi, + defaultLS, ); } diff --git a/src/client/common/experiments/helpers.ts b/src/client/common/experiments/helpers.ts index ac4b162f0f82..a85a468f494f 100644 --- a/src/client/common/experiments/helpers.ts +++ b/src/client/common/experiments/helpers.ts @@ -3,12 +3,8 @@ 'use strict'; -import { injectable } from 'inversify'; -import { LanguageServerType } from '../../activation/types'; -import { IServiceManager } from '../../ioc/types'; -import { IWorkspaceService } from '../application/types'; -import { IDefaultLanguageServer, IExperimentService } from '../types'; -import { DiscoveryVariants, JediLSP } from './groups'; +import { IExperimentService } from '../types'; +import { DiscoveryVariants } from './groups'; export async function inDiscoveryExperiment(experimentService: IExperimentService): Promise { const results = await Promise.all([ @@ -17,41 +13,3 @@ export async function inDiscoveryExperiment(experimentService: IExperimentServic ]); return results.includes(true); } - -@injectable() -class DefaultLanguageServer implements IDefaultLanguageServer { - public readonly defaultLSType: LanguageServerType.Jedi | LanguageServerType.JediLSP; - - constructor(defaultServer: LanguageServerType.Jedi | LanguageServerType.JediLSP) { - this.defaultLSType = defaultServer; - } -} - -export async function setDefaultLanguageServerByExperiment( - experimentService: IExperimentService, - workspaceService: IWorkspaceService, - serviceManager: IServiceManager, -): Promise { - const settings = workspaceService.getConfiguration('python'); - const lsSetting = settings.inspect('languageServer'); - if (lsSetting) { - if ( - lsSetting.globalValue || - lsSetting.globalLanguageValue || - lsSetting.workspaceFolderValue || - lsSetting.workspaceFolderLanguageValue || - lsSetting.workspaceValue || - lsSetting.workspaceLanguageValue - ) { - return Promise.resolve(); - } - } - const lsType = (await experimentService.inExperiment(JediLSP.experiment)) - ? LanguageServerType.JediLSP - : LanguageServerType.Jedi; - serviceManager.addSingletonInstance( - IDefaultLanguageServer, - new DefaultLanguageServer(lsType), - ); - return Promise.resolve(); -} diff --git a/src/client/extensionActivation.ts b/src/client/extensionActivation.ts index d6da4616520d..a27263c79512 100644 --- a/src/client/extensionActivation.ts +++ b/src/client/extensionActivation.ts @@ -26,6 +26,7 @@ import { IDisposableRegistry, IExperimentService, IExperimentsManager, + IExtensions, IOutputChannel, } from './common/types'; import { noop } from './common/utils/misc'; @@ -61,7 +62,7 @@ import * as pythonEnvironments from './pythonEnvironments'; import { ActivationResult, ExtensionState } from './components'; import { Components } from './extensionInit'; -import { setDefaultLanguageServerByExperiment } from './common/experiments/helpers'; +import { setDefaultLanguageServer } from './activation/common/defaultlanguageServer'; export async function activateComponents( // `ext` is passed to any extra activation funcs. @@ -131,7 +132,8 @@ async function activateLegacy(ext: ExtensionState): Promise { await experimentService.activate(); const workspaceService = serviceContainer.get(IWorkspaceService); - await setDefaultLanguageServerByExperiment(experimentService, workspaceService, serviceManager); + const extensions = serviceContainer.get(IExtensions); + await setDefaultLanguageServer(experimentService, extensions, serviceManager); const configuration = serviceManager.get(IConfigurationService); // We should start logging using the log level as soon as possible, so set it as soon as we can access the level. diff --git a/src/test/activation/defaultLanguageServer.unit.test.ts b/src/test/activation/defaultLanguageServer.unit.test.ts new file mode 100644 index 000000000000..4dd8b4eaf88e --- /dev/null +++ b/src/test/activation/defaultLanguageServer.unit.test.ts @@ -0,0 +1,103 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +'use strict'; + +import { expect } from 'chai'; +import { anything, instance, mock, when, verify } from 'ts-mockito'; +import { Extension } from 'vscode'; +import { setDefaultLanguageServer } from '../../client/activation/common/defaultlanguageServer'; +import { LanguageServerType } from '../../client/activation/types'; +import { PYLANCE_EXTENSION_ID } from '../../client/common/constants'; +import { JediLSP } from '../../client/common/experiments/groups'; +import { ExperimentService } from '../../client/common/experiments/service'; +import { IDefaultLanguageServer, IExperimentService, IExtensions } from '../../client/common/types'; +import { ServiceManager } from '../../client/ioc/serviceManager'; +import { IServiceManager } from '../../client/ioc/types'; + +suite('Activation - setDefaultLanguageServer()', () => { + let experimentService: IExperimentService; + let extensions: IExtensions; + let extension: Extension; + let serviceManager: IServiceManager; + setup(() => { + experimentService = mock(ExperimentService); + extensions = mock(); + extension = mock(); + serviceManager = mock(ServiceManager); + }); + + test('Pylance not installed and NOT in experiment', async () => { + let defaultServerType; + + when(extensions.getExtension(PYLANCE_EXTENSION_ID)).thenReturn(undefined); + when(experimentService.inExperiment(JediLSP.experiment)).thenResolve(false); + when(serviceManager.addSingletonInstance(IDefaultLanguageServer, anything())).thenCall( + (_symbol, value: IDefaultLanguageServer) => { + defaultServerType = value.defaultLSType; + }, + ); + + await setDefaultLanguageServer(instance(experimentService), instance(extensions), instance(serviceManager)); + + verify(extensions.getExtension(PYLANCE_EXTENSION_ID)).once(); + verify(experimentService.inExperiment(JediLSP.experiment)).once(); + verify(serviceManager.addSingletonInstance(IDefaultLanguageServer, anything())).once(); + expect(defaultServerType).to.equal(LanguageServerType.Jedi); + }); + + test('Pylance not installed and in experiment', async () => { + let defaultServerType; + when(extensions.getExtension(PYLANCE_EXTENSION_ID)).thenReturn(undefined); + when(experimentService.inExperiment(JediLSP.experiment)).thenResolve(true); + when(serviceManager.addSingletonInstance(IDefaultLanguageServer, anything())).thenCall( + (_symbol, value: IDefaultLanguageServer) => { + defaultServerType = value.defaultLSType; + }, + ); + + await setDefaultLanguageServer(instance(experimentService), instance(extensions), instance(serviceManager)); + + verify(extensions.getExtension(PYLANCE_EXTENSION_ID)).once(); + verify(experimentService.inExperiment(JediLSP.experiment)).once(); + verify(serviceManager.addSingletonInstance(IDefaultLanguageServer, anything())).once(); + expect(defaultServerType).to.equal(LanguageServerType.JediLSP); + }); + + test('Pylance installed and NOT in experiment', async () => { + let defaultServerType; + + when(extensions.getExtension(PYLANCE_EXTENSION_ID)).thenReturn(instance(extension)); + when(experimentService.inExperiment(JediLSP.experiment)).thenResolve(false); + when(serviceManager.addSingletonInstance(IDefaultLanguageServer, anything())).thenCall( + (_symbol, value: IDefaultLanguageServer) => { + defaultServerType = value.defaultLSType; + }, + ); + + await setDefaultLanguageServer(instance(experimentService), instance(extensions), instance(serviceManager)); + + verify(extensions.getExtension(PYLANCE_EXTENSION_ID)).once(); + verify(experimentService.inExperiment(JediLSP.experiment)).never(); + verify(serviceManager.addSingletonInstance(IDefaultLanguageServer, anything())).once(); + expect(defaultServerType).to.equal(LanguageServerType.Node); + }); + + test('Pylance installed and in experiment', async () => { + let defaultServerType; + when(extensions.getExtension(PYLANCE_EXTENSION_ID)).thenReturn(instance(extension)); + when(experimentService.inExperiment(JediLSP.experiment)).thenResolve(true); + when(serviceManager.addSingletonInstance(IDefaultLanguageServer, anything())).thenCall( + (_symbol, value: IDefaultLanguageServer) => { + defaultServerType = value.defaultLSType; + }, + ); + + await setDefaultLanguageServer(instance(experimentService), instance(extensions), instance(serviceManager)); + + verify(extensions.getExtension(PYLANCE_EXTENSION_ID)).once(); + verify(experimentService.inExperiment(JediLSP.experiment)).never(); + verify(serviceManager.addSingletonInstance(IDefaultLanguageServer, anything())).once(); + expect(defaultServerType).to.equal(LanguageServerType.Node); + }); +}); diff --git a/src/test/common/experiments/helpers.unit.test.ts b/src/test/common/experiments/helpers.unit.test.ts index a9bc306e8ece..0b9f8a19bc75 100644 --- a/src/test/common/experiments/helpers.unit.test.ts +++ b/src/test/common/experiments/helpers.unit.test.ts @@ -4,20 +4,11 @@ 'use strict'; import { expect } from 'chai'; -import { anything, instance, mock, when, verify } from 'ts-mockito'; -import { LanguageServerType } from '../../../client/activation/types'; -import { IWorkspaceService } from '../../../client/common/application/types'; -import { WorkspaceService } from '../../../client/common/application/workspace'; -import { DiscoveryVariants, JediLSP } from '../../../client/common/experiments/groups'; -import { - inDiscoveryExperiment, - setDefaultLanguageServerByExperiment, -} from '../../../client/common/experiments/helpers'; +import { anything, instance, mock, when } from 'ts-mockito'; +import { DiscoveryVariants } from '../../../client/common/experiments/groups'; +import { inDiscoveryExperiment } from '../../../client/common/experiments/helpers'; import { ExperimentService } from '../../../client/common/experiments/service'; -import { IDefaultLanguageServer, IExperimentService } from '../../../client/common/types'; -import { ServiceManager } from '../../../client/ioc/serviceManager'; -import { IServiceManager } from '../../../client/ioc/types'; -import { MockWorkspaceConfiguration } from '../../startPage/mockWorkspaceConfig'; +import { IExperimentService } from '../../../client/common/types'; suite('Experiments - inDiscoveryExperiment()', () => { let experimentService: IExperimentService; @@ -43,83 +34,3 @@ suite('Experiments - inDiscoveryExperiment()', () => { expect(result).to.equal(false); }); }); - -suite('Experiments - setDefaultLanguageServerByExperiment()', () => { - let experimentService: IExperimentService; - let workspaceService: IWorkspaceService; - let serviceManager: IServiceManager; - setup(() => { - experimentService = mock(ExperimentService); - workspaceService = mock(WorkspaceService); - serviceManager = mock(ServiceManager); - }); - - test('languageServer set by user', async () => { - when(workspaceService.getConfiguration('python')).thenReturn( - new MockWorkspaceConfiguration({ - languageServer: { globalValue: LanguageServerType.Node }, - }), - ); - await setDefaultLanguageServerByExperiment( - instance(experimentService), - instance(workspaceService), - instance(serviceManager), - ); - - verify(workspaceService.getConfiguration('python')).once(); - verify(experimentService.inExperiment(JediLSP.experiment)).never(); - verify(serviceManager.addSingletonInstance(IDefaultLanguageServer, anything())).never(); - }); - - test('languageServer NOT set by user and NOT in experiment', async () => { - let defaultServerType; - when(workspaceService.getConfiguration('python')).thenReturn( - new MockWorkspaceConfiguration({ - languageServer: { defaultValue: LanguageServerType.Jedi }, - }), - ); - when(experimentService.inExperiment(JediLSP.experiment)).thenResolve(false); - when(serviceManager.addSingletonInstance(IDefaultLanguageServer, anything())).thenCall( - (_symbol, value: IDefaultLanguageServer) => { - defaultServerType = value.defaultLSType; - }, - ); - - await setDefaultLanguageServerByExperiment( - instance(experimentService), - instance(workspaceService), - instance(serviceManager), - ); - - verify(workspaceService.getConfiguration('python')).once(); - verify(experimentService.inExperiment(JediLSP.experiment)).once(); - verify(serviceManager.addSingletonInstance(IDefaultLanguageServer, anything())).once(); - expect(defaultServerType).to.equal(LanguageServerType.Jedi); - }); - - test('languageServer NOT set by user and in experiment', async () => { - let defaultServerType; - when(workspaceService.getConfiguration('python')).thenReturn( - new MockWorkspaceConfiguration({ - languageServer: { defaultValue: LanguageServerType.Jedi }, - }), - ); - when(experimentService.inExperiment(JediLSP.experiment)).thenResolve(true); - when(serviceManager.addSingletonInstance(IDefaultLanguageServer, anything())).thenCall( - (_symbol, value: IDefaultLanguageServer) => { - defaultServerType = value.defaultLSType; - }, - ); - - await setDefaultLanguageServerByExperiment( - instance(experimentService), - instance(workspaceService), - instance(serviceManager), - ); - - verify(workspaceService.getConfiguration('python')).once(); - verify(experimentService.inExperiment(JediLSP.experiment)).once(); - verify(serviceManager.addSingletonInstance(IDefaultLanguageServer, anything())).once(); - expect(defaultServerType).to.equal(LanguageServerType.JediLSP); - }); -}); From 45b6420f67b08e1b4845752728ac0746fde129f7 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Tue, 4 May 2021 13:30:12 -0700 Subject: [PATCH 2/4] Remove unused code --- src/client/activation/common/defaultlanguageServer.ts | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/client/activation/common/defaultlanguageServer.ts b/src/client/activation/common/defaultlanguageServer.ts index 15a0ab5c28b6..e9f97e8d337f 100644 --- a/src/client/activation/common/defaultlanguageServer.ts +++ b/src/client/activation/common/defaultlanguageServer.ts @@ -41,16 +41,6 @@ async function getDefaultLanguageServer( return LanguageServerType.Node; } - // If Pylance is installed and functional, use it as the default. - // TODO: This causes a dependency cycle, as VS Code is trying to wait for python to even start pylance... - // console.log('Activating Pylance'); - // const pylance = await extensions.getExtension(PYLANCE_EXTENSION_ID)?.activate(); - // console.log('Activated Pylance'); - // if (pylance && Object.keys(pylance).length !== 0) { - // return LanguageServerType.Node; - // } - - // Otherwise, use jedi. return (await experimentService.inExperiment(JediLSP.experiment)) ? LanguageServerType.JediLSP : LanguageServerType.Jedi; From b4c90b47fa7f5d145fbfbf89ed7bbc1057faa36d Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Tue, 4 May 2021 13:30:26 -0700 Subject: [PATCH 3/4] Remove debug log --- src/client/activation/common/defaultlanguageServer.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/client/activation/common/defaultlanguageServer.ts b/src/client/activation/common/defaultlanguageServer.ts index e9f97e8d337f..6212a0943c81 100644 --- a/src/client/activation/common/defaultlanguageServer.ts +++ b/src/client/activation/common/defaultlanguageServer.ts @@ -26,7 +26,6 @@ export async function setDefaultLanguageServer( serviceManager: IServiceManager, ): Promise { const lsType = await getDefaultLanguageServer(experimentService, extensions); - console.log(`Default LS will be ${lsType}`); serviceManager.addSingletonInstance( IDefaultLanguageServer, new DefaultLanguageServer(lsType), From c3d461da5f42e5e102e521bc136c5859ffce0bc2 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Tue, 4 May 2021 14:43:01 -0700 Subject: [PATCH 4/4] Restore tryGet --- src/client/common/configuration/service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/common/configuration/service.ts b/src/client/common/configuration/service.ts index ea5abd493b24..b8c30a6bef64 100644 --- a/src/client/common/configuration/service.ts +++ b/src/client/common/configuration/service.ts @@ -37,7 +37,7 @@ export class ConfigurationService implements IConfigurationService { const interpreterSecurityService = this.serviceContainer.get( IInterpreterSecurityService, ); - const defaultLS = this.serviceContainer.get(IDefaultLanguageServer); + const defaultLS = this.serviceContainer.tryGet(IDefaultLanguageServer); return PythonSettings.getInstance( resource, InterpreterAutoSelectionService,