From 011d0c4f0d49bc0956df14e5d9161b01cfba0d91 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Thu, 11 Feb 2021 14:03:53 -0800 Subject: [PATCH 1/4] Do not call activate the discovery component before registering all the classes (#15379) * Do not attempt to activate discovery component before registering all the classes * Add clarification comment * Code reviews --- src/client/extensionActivation.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/client/extensionActivation.ts b/src/client/extensionActivation.ts index 816a0d400333..0652602250d0 100644 --- a/src/client/extensionActivation.ts +++ b/src/client/extensionActivation.ts @@ -78,12 +78,19 @@ export async function activateComponents( // `Promise.all()`, etc.) will flatten nested promises. Thus // activation resolves `ActivationResult`, which can safely wrap // the "inner" promise. + + // TODO: As of now activateLegacy() registers various classes which might + // be required while activating components. Once registration from + // activateLegacy() are moved before we activate other components, we can + // activate them parallelly with the other components. + // https://github.com/microsoft/vscode-python/issues/15380 + // These will go away eventually once everything is refactored into components. + const legacyActivationResult = await activateLegacy(ext); const promises: Promise[] = [ + // More component activations will go here pythonEnvironments.activate(components.pythonEnvs), - // These will go away eventually. - activateLegacy(ext), ]; - return Promise.all(promises); + return Promise.all([legacyActivationResult, ...promises]); } /// ////////////////////////// From 540a16da8571c7a86f35efaaaddf7f573c0513d0 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Tue, 16 Feb 2021 10:59:54 -0800 Subject: [PATCH 2/4] Skip windows store and shims paths when using known path locators (#15388) * Skip windows store and shims paths when using known path locators * Clean up and comments * Tests * Handle cases where envs variables might not be set * Typo Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com> --- .../lowLevel/windowsKnownPathsLocator.ts | 13 +++ .../services/posixKnownPathsLocator.ts | 7 +- .../locators/services/pyenvLocator.ts | 13 ++- .../locators/services/windowsStoreLocator.ts | 88 +++++++++++++++---- .../common/environmentIdentifier.unit.test.ts | 17 +++- .../locators/pyenvLocator.unit.test.ts | 22 +++++ .../locators/windowsStoreLocator.unit.test.ts | 10 +++ 7 files changed, 147 insertions(+), 23 deletions(-) diff --git a/src/client/pythonEnvironments/base/locators/lowLevel/windowsKnownPathsLocator.ts b/src/client/pythonEnvironments/base/locators/lowLevel/windowsKnownPathsLocator.ts index c5f1bde4733e..d9e986db45de 100644 --- a/src/client/pythonEnvironments/base/locators/lowLevel/windowsKnownPathsLocator.ts +++ b/src/client/pythonEnvironments/base/locators/lowLevel/windowsKnownPathsLocator.ts @@ -9,6 +9,8 @@ import { Event } from 'vscode'; import { getSearchPathEntries } from '../../../../common/utils/exec'; import { Disposables, IDisposable } from '../../../../common/utils/resourceLifecycle'; import { isStandardPythonBinary } from '../../../common/commonUtils'; +import { isPyenvShimDir } from '../../../discovery/locators/services/pyenvLocator'; +import { isWindowsStoreDir } from '../../../discovery/locators/services/windowsStoreLocator'; import { PythonEnvInfo, PythonEnvKind, PythonEnvSource } from '../../info'; import { ILocator, IPythonEnvsIterator, PythonLocatorQuery } from '../../locator'; import { Locators } from '../../locators'; @@ -31,6 +33,17 @@ export class WindowsPathEnvVarLocator implements ILocator, IDisposable { constructor() { const dirLocators: (ILocator & IDisposable)[] = getSearchPathEntries() + .filter((dirname) => { + // Filter out following directories: + // 1. Windows Store app directories: We have a store app locator that handles this. The + // python.exe available in these directories might not be python. It can be a store + // install shortcut that takes you to windows store. + // + // 2. Filter out pyenv shims: They are not actual python binaries, they are used to launch + // the binaries specified in .python-version file in the cwd. We should not be reporting + // those binaries as environments. + return !isWindowsStoreDir(dirname) && !isPyenvShimDir(dirname); + }) // Build a locator for each directory. .map((dirname) => getDirFilesLocator(dirname, PythonEnvKind.Unknown)); this.disposables.push(...dirLocators); diff --git a/src/client/pythonEnvironments/discovery/locators/services/posixKnownPathsLocator.ts b/src/client/pythonEnvironments/discovery/locators/services/posixKnownPathsLocator.ts index 8dc0e1dc095b..e7017089280d 100644 --- a/src/client/pythonEnvironments/discovery/locators/services/posixKnownPathsLocator.ts +++ b/src/client/pythonEnvironments/discovery/locators/services/posixKnownPathsLocator.ts @@ -4,7 +4,6 @@ import * as fs from 'fs'; import * as path from 'path'; import { traceError, traceInfo } from '../../../../common/logger'; - import { Architecture } from '../../../../common/utils/platform'; import { PythonEnvInfo, PythonEnvKind, PythonEnvSource, PythonReleaseLevel, PythonVersion } from '../../../base/info'; import { buildEnvInfo } from '../../../base/info/env'; @@ -12,9 +11,13 @@ import { parseVersion } from '../../../base/info/pythonVersion'; import { IPythonEnvsIterator, Locator } from '../../../base/locator'; import { getFileInfo, resolveSymbolicLink } from '../../../common/externalDependencies'; import { commonPosixBinPaths, isPosixPythonBinPattern } from '../../../common/posixUtils'; +import { isPyenvShimDir } from './pyenvLocator'; async function getPythonBinFromKnownPaths(): Promise { - const knownDirs = await commonPosixBinPaths(); + // Filter out pyenv shims. They are not actual python binaries, they are used to launch + // the binaries specified in .python-version file in the cwd. We should not be reporting + // those binaries as environments. + const knownDirs = (await commonPosixBinPaths()).filter((dirname) => !isPyenvShimDir(dirname)); const pythonBins: Set = new Set(); for (const dirname of knownDirs) { const paths = (await fs.promises.readdir(dirname, { withFileTypes: true })) diff --git a/src/client/pythonEnvironments/discovery/locators/services/pyenvLocator.ts b/src/client/pythonEnvironments/discovery/locators/services/pyenvLocator.ts index 9fcbde3bd2dc..ddac1574932d 100644 --- a/src/client/pythonEnvironments/discovery/locators/services/pyenvLocator.ts +++ b/src/client/pythonEnvironments/discovery/locators/services/pyenvLocator.ts @@ -13,7 +13,7 @@ import { getInterpreterPathFromDir, getPythonVersionFromPath, } from '../../../common/commonUtils'; -import { getFileInfo, getSubDirs, pathExists } from '../../../common/externalDependencies'; +import { arePathsSame, getFileInfo, getSubDirs, pathExists } from '../../../common/externalDependencies'; function getPyenvDir(): string { // Check if the pyenv environment variables exist: PYENV on Windows, PYENV_ROOT on Unix. @@ -37,6 +37,17 @@ function getPyenvVersionsDir(): string { return path.join(getPyenvDir(), 'versions'); } +/** + * Checks if a given directory path is same as `pyenv` shims path. This checks + * `~/.pyenv/shims` on posix and `~/.pyenv/pyenv-win/shims` on windows. + * @param {string} dirPath: Absolute path to any directory + * @returns {boolean}: Returns true if the patch is same as `pyenv` shims directory. + */ +export function isPyenvShimDir(dirPath: string): boolean { + const shimPath = path.join(getPyenvDir(), 'shims'); + return arePathsSame(shimPath, dirPath) || arePathsSame(`${shimPath}${path.sep}`, dirPath); +} + /** * Checks if the given interpreter belongs to a pyenv based environment. * @param {string} interpreterPath: Absolute path to the python interpreter. diff --git a/src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts b/src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts index 571c415551a2..daa46800dc0d 100644 --- a/src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts +++ b/src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts @@ -11,7 +11,7 @@ import { buildEnvInfo } from '../../../base/info/env'; import { getPythonVersionFromPath } from '../../../base/info/pythonVersion'; import { IPythonEnvsIterator } from '../../../base/locator'; import { FSWatchingLocator } from '../../../base/locators/lowLevel/fsWatchingLocator'; -import { getFileInfo } from '../../../common/externalDependencies'; +import { getFileInfo, pathExists } from '../../../common/externalDependencies'; import { PythonEnvStructure } from '../../../common/pythonBinariesWatcher'; /** @@ -26,16 +26,59 @@ export function getWindowsStoreAppsRoot(): string { /** * Checks if a given path is under the forbidden windows store directory. - * @param {string} interpreterPath : Absolute path to the python interpreter. + * @param {string} absPath : Absolute path to a file or directory. * @returns {boolean} : Returns true if `interpreterPath` is under * `%ProgramFiles%/WindowsApps`. */ -export function isForbiddenStorePath(interpreterPath: string): boolean { +export function isForbiddenStorePath(absPath: string): boolean { const programFilesStorePath = path .join(getEnvironmentVariable('ProgramFiles') || 'Program Files', 'WindowsApps') .normalize() .toUpperCase(); - return path.normalize(interpreterPath).toUpperCase().includes(programFilesStorePath); + return path.normalize(absPath).toUpperCase().includes(programFilesStorePath); +} + +/** + * Checks if a given directory is any one of the possible windows store directories, or + * its sub-directory. + * @param {string} dirPath : Absolute path to a directory. + * + * Remarks: + * These locations are tested: + * 1. %LOCALAPPDATA%/Microsoft/WindowsApps + * 2. %ProgramFiles%/WindowsApps + */ +export function isWindowsStoreDir(dirPath: string): boolean { + const storeRootPath = path.normalize(getWindowsStoreAppsRoot()).toUpperCase(); + return path.normalize(dirPath).toUpperCase().includes(storeRootPath) || isForbiddenStorePath(dirPath); +} + +/** + * Checks if store python is installed. + * @param {string} interpreterPath : Absolute path to a interpreter. + * Remarks: + * If store python was never installed then the store apps directory will not + * have idle.exe or pip.exe. We can use this as a way to identify the python.exe + * found in the store apps directory is a real python or a store install shortcut. + */ +async function isStorePythonInstalled(interpreterPath?: string): Promise { + let results = await Promise.all([ + pathExists(path.join(getWindowsStoreAppsRoot(), 'idle.exe')), + pathExists(path.join(getWindowsStoreAppsRoot(), 'pip.exe')), + ]); + + if (results.includes(true)) { + return true; + } + + if (interpreterPath) { + results = await Promise.all([ + pathExists(path.join(path.dirname(interpreterPath), 'idle.exe')), + pathExists(path.join(path.dirname(interpreterPath), 'pip.exe')), + ]); + return results.includes(true); + } + return false; } /** @@ -71,17 +114,19 @@ export function isForbiddenStorePath(interpreterPath: string): boolean { * */ export async function isWindowsStoreEnvironment(interpreterPath: string): Promise { - const pythonPathToCompare = path.normalize(interpreterPath).toUpperCase(); - const localAppDataStorePath = path.normalize(getWindowsStoreAppsRoot()).toUpperCase(); - if (pythonPathToCompare.includes(localAppDataStorePath)) { - return true; - } + if (await isStorePythonInstalled(interpreterPath)) { + const pythonPathToCompare = path.normalize(interpreterPath).toUpperCase(); + const localAppDataStorePath = path.normalize(getWindowsStoreAppsRoot()).toUpperCase(); + if (pythonPathToCompare.includes(localAppDataStorePath)) { + return true; + } - // Program Files store path is a forbidden path. Only admins and system has access this path. - // We should never have to look at this path or even execute python from this path. - if (isForbiddenStorePath(pythonPathToCompare)) { - traceWarning('isWindowsStoreEnvironment called with Program Files store path.'); - return true; + // Program Files store path is a forbidden path. Only admins and system has access this path. + // We should never have to look at this path or even execute python from this path. + if (isForbiddenStorePath(pythonPathToCompare)) { + traceWarning('isWindowsStoreEnvironment called with Program Files store path.'); + return true; + } } return false; } @@ -107,7 +152,7 @@ const pythonExeGlob = 'python3.{[0-9],[0-9][0-9]}.exe'; * @param {string} interpreterPath : Path to python interpreter. * @returns {boolean} : Returns true if the path matches pattern for windows python executable. */ -export function isWindowsStorePythonExe(interpreterPath: string): boolean { +export function isWindowsStorePythonExePattern(interpreterPath: string): boolean { return minimatch(path.basename(interpreterPath), pythonExeGlob, { nocase: true }); } @@ -128,11 +173,16 @@ export function isWindowsStorePythonExe(interpreterPath: string): boolean { * that location. */ export async function getWindowsStorePythonExes(): Promise { - const windowsAppsRoot = getWindowsStoreAppsRoot(); + if (await isStorePythonInstalled()) { + const windowsAppsRoot = getWindowsStoreAppsRoot(); - // Collect python*.exe directly under %LOCALAPPDATA%/Microsoft/WindowsApps - const files = await fsapi.readdir(windowsAppsRoot); - return files.map((filename: string) => path.join(windowsAppsRoot, filename)).filter(isWindowsStorePythonExe); + // Collect python*.exe directly under %LOCALAPPDATA%/Microsoft/WindowsApps + const files = await fsapi.readdir(windowsAppsRoot); + return files + .map((filename: string) => path.join(windowsAppsRoot, filename)) + .filter(isWindowsStorePythonExePattern); + } + return []; } export class WindowsStoreLocator extends FSWatchingLocator { diff --git a/src/test/pythonEnvironments/common/environmentIdentifier.unit.test.ts b/src/test/pythonEnvironments/common/environmentIdentifier.unit.test.ts index c258b0d04481..0a87b1331e6c 100644 --- a/src/test/pythonEnvironments/common/environmentIdentifier.unit.test.ts +++ b/src/test/pythonEnvironments/common/environmentIdentifier.unit.test.ts @@ -81,6 +81,7 @@ suite('Environment Identifier', () => { suite('Windows Store', () => { let getEnvVar: sinon.SinonStub; + let pathExists: sinon.SinonStub; const fakeLocalAppDataPath = path.join(TEST_LAYOUT_ROOT, 'storeApps'); const fakeProgramFilesPath = 'X:\\Program Files'; const executable = ['python.exe', 'python3.exe', 'python3.8.exe']; @@ -88,17 +89,23 @@ suite('Environment Identifier', () => { getEnvVar = sinon.stub(platformApis, 'getEnvironmentVariable'); getEnvVar.withArgs('LOCALAPPDATA').returns(fakeLocalAppDataPath); getEnvVar.withArgs('ProgramFiles').returns(fakeProgramFilesPath); + + pathExists = sinon.stub(externalDependencies, 'pathExists'); + pathExists.withArgs(path.join(fakeLocalAppDataPath, 'Microsoft', 'WindowsApps', 'idle.exe')).resolves(true); }); suiteTeardown(() => { getEnvVar.restore(); + pathExists.restore(); }); executable.forEach((exe) => { test(`Path to local app data windows store interpreter (${exe})`, async () => { + getEnvVar.withArgs('LOCALAPPDATA').returns(fakeLocalAppDataPath); const interpreterPath = path.join(fakeLocalAppDataPath, 'Microsoft', 'WindowsApps', exe); const envType: EnvironmentType = await identifyEnvironment(interpreterPath); assert.deepEqual(envType, EnvironmentType.WindowsStore); }); test(`Path to local app data windows store interpreter app sub-directory (${exe})`, async () => { + getEnvVar.withArgs('LOCALAPPDATA').returns(fakeLocalAppDataPath); const interpreterPath = path.join( fakeLocalAppDataPath, 'Microsoft', @@ -126,13 +133,15 @@ suite('Environment Identifier', () => { assert.deepEqual(envType, EnvironmentType.WindowsStore); }); test(`Program files app data not set (${exe})`, async () => { - getEnvVar.withArgs('ProgramFiles').returns(undefined); const interpreterPath = path.join( fakeProgramFilesPath, 'WindowsApps', 'PythonSoftwareFoundation.Python.3.8_qbz5n2kfra8p0', exe, ); + getEnvVar.withArgs('ProgramFiles').returns(undefined); + pathExists.withArgs(path.join(path.dirname(interpreterPath), 'idle.exe')).resolves(true); + const envType: EnvironmentType = await identifyEnvironment(interpreterPath); assert.deepEqual(envType, EnvironmentType.WindowsStore); }); @@ -147,6 +156,12 @@ suite('Environment Identifier', () => { const interpreterPath = path .join(fakeLocalAppDataPath, 'Microsoft', 'WindowsApps', exe) .replace('\\', '/'); + pathExists.callsFake((p: string) => { + if (p.endsWith('idle.exe')) { + return Promise.resolve(true); + } + return Promise.resolve(false); + }); const envType: EnvironmentType = await identifyEnvironment(`\\\\?\\${interpreterPath}`); assert.deepEqual(envType, EnvironmentType.WindowsStore); }); diff --git a/src/test/pythonEnvironments/discovery/locators/pyenvLocator.unit.test.ts b/src/test/pythonEnvironments/discovery/locators/pyenvLocator.unit.test.ts index 74c5b912f20f..1e6819aa1a6f 100644 --- a/src/test/pythonEnvironments/discovery/locators/pyenvLocator.unit.test.ts +++ b/src/test/pythonEnvironments/discovery/locators/pyenvLocator.unit.test.ts @@ -9,6 +9,7 @@ import * as fileUtils from '../../../../client/pythonEnvironments/common/externa import { IPyenvVersionStrings, isPyenvEnvironment, + isPyenvShimDir, parsePyenvVersion, } from '../../../../client/pythonEnvironments/discovery/locators/services/pyenvLocator'; @@ -269,3 +270,24 @@ suite('Pyenv Versions Parser Test', () => { }); }); }); + +suite('Pyenv Shims Dir filter tests', () => { + let getEnvVariableStub: sinon.SinonStub; + const pyenvRoot = path.join('path', 'to', 'pyenv', 'root'); + + setup(() => { + getEnvVariableStub = sinon.stub(platformUtils, 'getEnvironmentVariable'); + getEnvVariableStub.withArgs('PYENV_ROOT').returns(pyenvRoot); + }); + + teardown(() => { + getEnvVariableStub.restore(); + }); + + test('isPyenvShimDir: valid case', () => { + assert.deepStrictEqual(isPyenvShimDir(path.join(pyenvRoot, 'shims')), true); + }); + test('isPyenvShimDir: invalid case', () => { + assert.deepStrictEqual(isPyenvShimDir(__dirname), false); + }); +}); diff --git a/src/test/pythonEnvironments/discovery/locators/windowsStoreLocator.unit.test.ts b/src/test/pythonEnvironments/discovery/locators/windowsStoreLocator.unit.test.ts index fc065aa6096b..995082bb4192 100644 --- a/src/test/pythonEnvironments/discovery/locators/windowsStoreLocator.unit.test.ts +++ b/src/test/pythonEnvironments/discovery/locators/windowsStoreLocator.unit.test.ts @@ -20,6 +20,7 @@ import { parseVersion } from '../../../../client/pythonEnvironments/base/info/py import * as externalDep from '../../../../client/pythonEnvironments/common/externalDependencies'; import { getWindowsStorePythonExes, + isWindowsStoreDir, WindowsStoreLocator, } from '../../../../client/pythonEnvironments/discovery/locators/services/windowsStoreLocator'; import { getEnvs } from '../../base/common'; @@ -50,6 +51,15 @@ suite('Windows Store', () => { const actual = await getWindowsStorePythonExes(); assert.deepEqual(actual, expected); }); + + test('isWindowsStoreDir: valid case', () => { + assert.deepStrictEqual(isWindowsStoreDir(testStoreAppRoot), true); + assert.deepStrictEqual(isWindowsStoreDir(testStoreAppRoot + path.sep), true); + }); + + test('isWindowsStoreDir: invalid case', () => { + assert.deepStrictEqual(isWindowsStoreDir(__dirname), false); + }); }); suite('Locator', () => { From c515153bdfc52faedc85956a3388295f09cde48f Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Tue, 16 Feb 2021 17:34:40 -0800 Subject: [PATCH 3/4] Change "Pylance not installed" prompt to allow reverting to Jedi (#15420) --- package.nls.json | 4 +- package.nls.ru.json | 1 - package.nls.zh-cn.json | 1 - src/client/activation/activationService.ts | 2 + src/client/activation/common/activatorBase.ts | 2 +- .../common/languageServerChangeHandler.ts | 36 +++-- src/client/activation/node/activator.ts | 7 +- src/client/common/utils/localize.ts | 11 +- .../activation/node/activator.unit.test.ts | 27 ++-- .../languageServerChangeHandler.unit.test.ts | 131 +++++++++++++++--- 10 files changed, 174 insertions(+), 48 deletions(-) diff --git a/package.nls.json b/package.nls.json index 3c903570d2b9..e86c191091a9 100644 --- a/package.nls.json +++ b/package.nls.json @@ -49,9 +49,11 @@ "Pylance.proposePylanceMessage": "Try out a new faster, feature-rich language server for Python by Microsoft, Pylance! Install the extension now.", "Pylance.tryItNow": "Try it now", "Pylance.remindMeLater": "Remind me later", - "Pylance.installPylanceMessage": "Pylance extension is not installed. Click Yes to open Pylance installation page.", "Pylance.pylanceNotInstalledMessage": "Pylance extension is not installed.", "Pylance.pylanceInstalledReloadPromptMessage": "Pylance extension is now installed. Reload window to activate?", + "Pylance.pylanceRevertToJediPrompt": "The Pylance extension is not installed but the python.languageServer value is set to \"Pylance\". Would you like to install the Pylance extension to use Pylance, or revert back to Jedi?", + "Pylance.pylanceInstallPylance": "Install Pylance", + "Pylance.pylanceRevertToJedi": "Revert to Jedi", "Experiments.inGroup": "User belongs to experiment group '{0}'", "Experiments.optedOutOf": "User opted out of experiment group '{0}'", "Interpreters.RefreshingInterpreters": "Refreshing Python Interpreters", diff --git a/package.nls.ru.json b/package.nls.ru.json index 9b6b0a5661da..0a80260e6644 100644 --- a/package.nls.ru.json +++ b/package.nls.ru.json @@ -34,7 +34,6 @@ "Pylance.proposePylanceMessage": "Попробуйте новый языковый сервер для Python от Microsoft: Pylance! Установите расширение Pylance.", "Pylance.tryItNow": "Да, хочу", "Pylance.remindMeLater": "Напомните позже", - "Pylance.installPylanceMessage": "Расширение Pylance не установлено. Нажмите Да чтобы открыть страницу установки Pylance.", "Pylance.pylanceNotInstalledMessage": "Расширение Pylance не установлено.", "Pylance.pylanceInstalledReloadPromptMessage": "Расширение Pylance установлено. Перезагрузить окно для его активации?", "LanguageService.reloadAfterLanguageServerChange": "Пожалуйста, перезагрузите окно после смены типа языкового сервера." diff --git a/package.nls.zh-cn.json b/package.nls.zh-cn.json index 823b1d6a9268..14b9701851f8 100644 --- a/package.nls.zh-cn.json +++ b/package.nls.zh-cn.json @@ -49,7 +49,6 @@ "Pylance.proposePylanceMessage": "试试微软新的更快的、功能丰富的语言服务器 Pylance! ", "Pylance.tryItNow": "立即安装", "Pylance.remindMeLater": "稍后提醒", - "Pylance.installPylanceMessage": "Pylance 扩展未安装。点击 \"是\" 打开 Pylance 安装页面。", "Pylance.pylanceNotInstalledMessage": "Pylance 扩展未安装。", "Pylance.pylanceInstalledReloadPromptMessage": "Pylance 扩展未安装。重新加载窗口以激活？", "Experiments.inGroup": "用户属于 '{0}' 实验组", diff --git a/src/client/activation/activationService.ts b/src/client/activation/activationService.ts index 82bc3eebc33b..b63721acd392 100644 --- a/src/client/activation/activationService.ts +++ b/src/client/activation/activationService.ts @@ -86,6 +86,8 @@ export class LanguageServerExtensionActivationService this.serviceContainer.get(IExtensions), this.serviceContainer.get(IApplicationShell), this.serviceContainer.get(ICommandManager), + this.serviceContainer.get(IWorkspaceService), + this.serviceContainer.get(IConfigurationService), ); disposables.push(this.languageServerChangeHandler); } diff --git a/src/client/activation/common/activatorBase.ts b/src/client/activation/common/activatorBase.ts index 7dcc80455291..3de6b41c2e98 100644 --- a/src/client/activation/common/activatorBase.ts +++ b/src/client/activation/common/activatorBase.ts @@ -42,7 +42,7 @@ export abstract class LanguageServerActivatorBase implements ILanguageServerActi protected resource?: Resource; constructor( protected readonly manager: ILanguageServerManager, - private readonly workspace: IWorkspaceService, + protected readonly workspace: IWorkspaceService, protected readonly fs: IFileSystem, protected readonly configurationService: IConfigurationService, ) {} diff --git a/src/client/activation/common/languageServerChangeHandler.ts b/src/client/activation/common/languageServerChangeHandler.ts index d81f2cfdc723..22c325b6d6e5 100644 --- a/src/client/activation/common/languageServerChangeHandler.ts +++ b/src/client/activation/common/languageServerChangeHandler.ts @@ -1,10 +1,10 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import { Disposable } from 'vscode'; -import { IApplicationShell, ICommandManager } from '../../common/application/types'; +import { ConfigurationTarget, Disposable } from 'vscode'; +import { IApplicationShell, ICommandManager, IWorkspaceService } from '../../common/application/types'; import { PYLANCE_EXTENSION_ID } from '../../common/constants'; -import { IExtensions } from '../../common/types'; +import { IConfigurationService, IExtensions } from '../../common/types'; import { createDeferred } from '../../common/utils/async'; import { Common, LanguageService, Pylance } from '../../common/utils/localize'; import { LanguageServerType } from '../types'; @@ -12,16 +12,32 @@ import { LanguageServerType } from '../types'; export async function promptForPylanceInstall( appShell: IApplicationShell, commandManager: ICommandManager, + workspace: IWorkspaceService, + configService: IConfigurationService, ): Promise { - // If not installed, point user to Pylance at the store. const response = await appShell.showWarningMessage( - Pylance.installPylanceMessage(), - Common.bannerLabelYes(), - Common.bannerLabelNo(), + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), ); - if (response === Common.bannerLabelYes()) { + if (response === Pylance.pylanceInstallPylance()) { commandManager.executeCommand('extension.open', PYLANCE_EXTENSION_ID); + } else if (response === Pylance.pylanceRevertToJedi()) { + const inspection = workspace.getConfiguration('python').inspect('languageServer'); + + let target: ConfigurationTarget | undefined; + if (inspection?.workspaceValue) { + target = ConfigurationTarget.Workspace; + } else if (inspection?.globalValue) { + target = ConfigurationTarget.Global; + } + + if (target) { + await configService.updateSetting('languageServer', LanguageServerType.Jedi, undefined, target); + commandManager.executeCommand('workbench.action.reloadWindow'); + } } } @@ -37,6 +53,8 @@ export class LanguageServerChangeHandler implements Disposable { private readonly extensions: IExtensions, private readonly appShell: IApplicationShell, private readonly commands: ICommandManager, + private readonly workspace: IWorkspaceService, + private readonly configService: IConfigurationService, ) { this.pylanceInstalled = this.isPylanceInstalled(); this.disposables.push( @@ -70,7 +88,7 @@ export class LanguageServerChangeHandler implements Disposable { let response: string | undefined; if (lsType === LanguageServerType.Node && !this.isPylanceInstalled()) { // If not installed, point user to Pylance at the store. - await promptForPylanceInstall(this.appShell, this.commands); + await promptForPylanceInstall(this.appShell, this.commands, this.workspace, this.configService); // At this point Pylance is not yet installed. Skip reload prompt // since we are going to show it when Pylance becomes available. } else { diff --git a/src/client/activation/node/activator.ts b/src/client/activation/node/activator.ts index 1d5766c1856e..fa3a99e2df01 100644 --- a/src/client/activation/node/activator.ts +++ b/src/client/activation/node/activator.ts @@ -47,7 +47,12 @@ export class NodeLanguageServerActivator extends LanguageServerActivatorBase { // Pylance is not yet installed. Throw will cause activator to use Jedi // temporarily. Language server installation tracker will prompt for window // reload when Pylance becomes available. - await promptForPylanceInstall(this.appShell, this.commandManager); + await promptForPylanceInstall( + this.appShell, + this.commandManager, + this.workspace, + this.configurationService, + ); throw new Error(Pylance.pylanceNotInstalledMessage()); } } diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index 65423b36f133..ca090b39afff 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -114,10 +114,6 @@ export namespace Pylance { export const tryItNow = localize('Pylance.tryItNow', 'Try it now'); export const remindMeLater = localize('Pylance.remindMeLater', 'Remind me later'); - export const installPylanceMessage = localize( - 'Pylance.installPylanceMessage', - 'Pylance extension is not installed. Click Yes to open Pylance installation page.', - ); export const pylanceNotInstalledMessage = localize( 'Pylance.pylanceNotInstalledMessage', 'Pylance extension is not installed.', @@ -126,6 +122,13 @@ export namespace Pylance { 'Pylance.pylanceInstalledReloadPromptMessage', 'Pylance extension is now installed. Reload window to activate?', ); + + export const pylanceRevertToJediPrompt = localize( + 'Pylance.pylanceRevertToJediPrompt', + 'The Pylance extension is not installed but the python.languageServer value is set to "Pylance". Would you like to install the Pylance extension to use Pylance, or revert back to Jedi?', + ); + export const pylanceInstallPylance = localize('Pylance.pylanceInstallPylance', 'Install Pylance'); + export const pylanceRevertToJedi = localize('Pylance.pylanceRevertToJedi', 'Revert to Jedi'); } export namespace Jupyter { diff --git a/src/test/activation/node/activator.unit.test.ts b/src/test/activation/node/activator.unit.test.ts index bfc3bfa3cb0f..1207526d1f09 100644 --- a/src/test/activation/node/activator.unit.test.ts +++ b/src/test/activation/node/activator.unit.test.ts @@ -17,7 +17,7 @@ import { PYLANCE_EXTENSION_ID } from '../../../client/common/constants'; import { FileSystem } from '../../../client/common/platform/fileSystem'; import { IFileSystem } from '../../../client/common/platform/types'; import { IConfigurationService, IExtensions, IPythonSettings } from '../../../client/common/types'; -import { Common, Pylance } from '../../../client/common/utils/localize'; +import { Pylance } from '../../../client/common/utils/localize'; suite('Pylance Language Server - Activator', () => { let activator: NodeLanguageServerActivator; @@ -92,20 +92,22 @@ suite('Pylance Language Server - Activator', () => { test('When Pylance is not installed activator should show install prompt ', async () => { when( appShell.showWarningMessage( - Pylance.installPylanceMessage(), - Common.bannerLabelYes(), - Common.bannerLabelNo(), + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), ), - ).thenReturn(Promise.resolve(Common.bannerLabelNo())); + ).thenReturn(Promise.resolve(Pylance.remindMeLater())); try { await activator.start(undefined); } catch {} verify( appShell.showWarningMessage( - Pylance.installPylanceMessage(), - Common.bannerLabelYes(), - Common.bannerLabelNo(), + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), ), ).once(); verify(commandManager.executeCommand('extension.open', PYLANCE_EXTENSION_ID)).never(); @@ -114,11 +116,12 @@ suite('Pylance Language Server - Activator', () => { test('When Pylance is not installed activator should open Pylance install page if users clicks Yes', async () => { when( appShell.showWarningMessage( - Pylance.installPylanceMessage(), - Common.bannerLabelYes(), - Common.bannerLabelNo(), + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), ), - ).thenReturn(Promise.resolve(Common.bannerLabelYes())); + ).thenReturn(Promise.resolve(Pylance.pylanceInstallPylance())); try { await activator.start(undefined); diff --git a/src/test/activation/node/languageServerChangeHandler.unit.test.ts b/src/test/activation/node/languageServerChangeHandler.unit.test.ts index 09c96206708c..d66b4e574f0f 100644 --- a/src/test/activation/node/languageServerChangeHandler.unit.test.ts +++ b/src/test/activation/node/languageServerChangeHandler.unit.test.ts @@ -3,13 +3,13 @@ 'use strict'; -import { anyString, instance, mock, verify, when } from 'ts-mockito'; -import { EventEmitter, Extension } from 'vscode'; +import { anyString, instance, mock, verify, when, anything } from 'ts-mockito'; +import { ConfigurationTarget, EventEmitter, Extension, WorkspaceConfiguration } from 'vscode'; import { LanguageServerChangeHandler } from '../../../client/activation/common/languageServerChangeHandler'; import { LanguageServerType } from '../../../client/activation/types'; -import { IApplicationShell, ICommandManager } from '../../../client/common/application/types'; +import { IApplicationShell, ICommandManager, IWorkspaceService } from '../../../client/common/application/types'; import { PYLANCE_EXTENSION_ID } from '../../../client/common/constants'; -import { IExtensions } from '../../../client/common/types'; +import { IConfigurationService, IExtensions } from '../../../client/common/types'; import { Common, LanguageService, Pylance } from '../../../client/common/utils/localize'; suite('Language Server - Change Handler', () => { @@ -19,11 +19,16 @@ suite('Language Server - Change Handler', () => { let extensionsChangedEvent: EventEmitter; let handler: LanguageServerChangeHandler; + let workspace: IWorkspaceService; + let configService: IConfigurationService; + let pylanceExtension: Extension; setup(() => { extensions = mock(); appShell = mock(); commands = mock(); + workspace = mock(); + configService = mock(); pylanceExtension = mock>(); @@ -84,9 +89,10 @@ suite('Language Server - Change Handler', () => { test('Handler should prompt for install when language server changes to Pylance and Pylance is not installed', async () => { when( appShell.showWarningMessage( - Pylance.installPylanceMessage(), - Common.bannerLabelYes(), - Common.bannerLabelNo(), + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), ), ).thenReturn(Promise.resolve(undefined)); @@ -98,9 +104,10 @@ suite('Language Server - Change Handler', () => { ).never(); verify( appShell.showWarningMessage( - Pylance.installPylanceMessage(), - Common.bannerLabelYes(), - Common.bannerLabelNo(), + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), ), ).once(); }); @@ -108,11 +115,12 @@ suite('Language Server - Change Handler', () => { test('Handler should open Pylance store page when language server changes to Pylance, Pylance is not installed and user clicks Yes', async () => { when( appShell.showWarningMessage( - Pylance.installPylanceMessage(), - Common.bannerLabelYes(), - Common.bannerLabelNo(), + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), ), - ).thenReturn(Promise.resolve(Common.bannerLabelYes())); + ).thenReturn(Promise.resolve(Pylance.pylanceInstallPylance())); handler = makeHandler(undefined); await handler.handleLanguageServerChange(LanguageServerType.Node); @@ -124,11 +132,12 @@ suite('Language Server - Change Handler', () => { test('Handler should not open Pylance store page when language server changes to Pylance, Pylance is not installed and user clicks No', async () => { when( appShell.showWarningMessage( - Pylance.installPylanceMessage(), - Common.bannerLabelYes(), - Common.bannerLabelNo(), + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), ), - ).thenReturn(Promise.resolve(Common.bannerLabelNo())); + ).thenReturn(Promise.resolve(Pylance.remindMeLater())); handler = makeHandler(undefined); await handler.handleLanguageServerChange(LanguageServerType.Node); @@ -171,12 +180,98 @@ suite('Language Server - Change Handler', () => { verify(commands.executeCommand('workbench.action.reloadWindow')).never(); }); + [ConfigurationTarget.Global, ConfigurationTarget.Workspace].forEach((target) => { + const targetName = target === ConfigurationTarget.Global ? 'global' : 'workspace'; + test(`Revert to Jedi with setting in ${targetName} config`, async () => { + const configuration = mock(); + + when( + appShell.showWarningMessage( + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), + ), + ).thenReturn(Promise.resolve(Pylance.pylanceRevertToJedi())); + + when(workspace.getConfiguration('python')).thenReturn(instance(configuration)); + + const inspection = { + key: 'python.languageServer', + workspaceValue: target === ConfigurationTarget.Workspace ? LanguageServerType.Node : undefined, + globalValue: target === ConfigurationTarget.Global ? LanguageServerType.Node : undefined, + }; + + when(configuration.inspect('languageServer')).thenReturn(inspection); + + handler = makeHandler(undefined); + await handler.handleLanguageServerChange(LanguageServerType.Node); + + verify( + appShell.showInformationMessage(LanguageService.reloadAfterLanguageServerChange(), Common.reload()), + ).never(); + verify( + appShell.showWarningMessage( + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), + ), + ).once(); + verify(configService.updateSetting('languageServer', LanguageServerType.Jedi, undefined, target)).once(); + }); + }); + + [ConfigurationTarget.WorkspaceFolder, undefined].forEach((target) => { + const targetName = target === ConfigurationTarget.WorkspaceFolder ? 'workspace folder' : 'missing'; + test(`Revert to Jedi with ${targetName} setting does nothing`, async () => { + const configuration = mock(); + + when( + appShell.showWarningMessage( + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), + ), + ).thenReturn(Promise.resolve(Pylance.pylanceRevertToJedi())); + + when(workspace.getConfiguration('python')).thenReturn(instance(configuration)); + + const inspection = { + key: 'python.languageServer', + workspaceFolderValue: + target === ConfigurationTarget.WorkspaceFolder ? LanguageServerType.Node : undefined, + }; + + when(configuration.inspect('languageServer')).thenReturn(inspection); + + handler = makeHandler(undefined); + await handler.handleLanguageServerChange(LanguageServerType.Node); + + verify( + appShell.showInformationMessage(LanguageService.reloadAfterLanguageServerChange(), Common.reload()), + ).never(); + verify( + appShell.showWarningMessage( + Pylance.pylanceRevertToJediPrompt(), + Pylance.pylanceInstallPylance(), + Pylance.pylanceRevertToJedi(), + Pylance.remindMeLater(), + ), + ).once(); + verify(configService.updateSetting(anything(), anything(), anything(), anything())).never(); + }); + }); + function makeHandler(initialLSType: LanguageServerType | undefined): LanguageServerChangeHandler { return new LanguageServerChangeHandler( initialLSType, instance(extensions), instance(appShell), instance(commands), + instance(workspace), + instance(configService), ); } }); From 13f613a8861f50e1513872a70837b66a8b483f55 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 17 Feb 2021 11:41:31 -0800 Subject: [PATCH 4/4] Allow on suggestion refresh by default (#15430) --- src/client/pythonEnvironments/legacyIOC.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/pythonEnvironments/legacyIOC.ts b/src/client/pythonEnvironments/legacyIOC.ts index e1f09632e33c..c8c2d1ad736d 100644 --- a/src/client/pythonEnvironments/legacyIOC.ts +++ b/src/client/pythonEnvironments/legacyIOC.ts @@ -142,7 +142,7 @@ class ComponentAdapter implements IComponentAdapter, IExtensionSingleActivationS private readonly refreshed = new vscode.EventEmitter(); - private allowOnSuggestionRefresh = false; + private allowOnSuggestionRefresh = true; constructor( // The adapter only wraps one thing: the component API.