diff --git a/news/2 Fixes/16723.md b/news/2 Fixes/16723.md new file mode 100644 index 000000000000..75b29c7e8244 --- /dev/null +++ b/news/2 Fixes/16723.md @@ -0,0 +1 @@ +Ensure we block on autoselection when no interpreter is explictly set by user. diff --git a/src/client/common/interpreterPathProxyService.ts b/src/client/common/interpreterPathProxyService.ts new file mode 100644 index 000000000000..f1044747a1d0 --- /dev/null +++ b/src/client/common/interpreterPathProxyService.ts @@ -0,0 +1,33 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +'use strict'; + +import { inject, injectable } from 'inversify'; +import { IWorkspaceService } from './application/types'; +import { DeprecatePythonPath } from './experiments/groups'; +import { IExperimentService, IInterpreterPathProxyService, IInterpreterPathService, Resource } from './types'; +import { SystemVariables } from './variables/systemVariables'; + +@injectable() +export class InterpreterPathProxyService implements IInterpreterPathProxyService { + constructor( + @inject(IInterpreterPathService) private readonly interpreterPathService: IInterpreterPathService, + @inject(IExperimentService) private readonly experiment: IExperimentService, + @inject(IWorkspaceService) private readonly workspace: IWorkspaceService, + ) {} + + public get(resource: Resource): string { + const systemVariables = new SystemVariables( + undefined, + this.workspace.getWorkspaceFolder(resource)?.uri.fsPath, + this.workspace, + ); + const pythonSettings = this.workspace.getConfiguration('python', resource); + return systemVariables.resolveAny( + this.experiment.inExperimentSync(DeprecatePythonPath.experiment) + ? this.interpreterPathService.get(resource) + : pythonSettings.get('pythonPath'), + )!; + } +} diff --git a/src/client/common/serviceRegistry.ts b/src/client/common/serviceRegistry.ts index 66b37072cff5..16a4d99fd49c 100644 --- a/src/client/common/serviceRegistry.ts +++ b/src/client/common/serviceRegistry.ts @@ -20,6 +20,7 @@ import { IToolExecutionPath, IsWindows, ToolExecutionPath, + IInterpreterPathProxyService, } from './types'; import { IServiceManager } from '../ioc/types'; import { JupyterExtensionDependencyManager } from '../jupyter/jupyterExtensionDependencyManager'; @@ -118,11 +119,16 @@ import { IMultiStepInputFactory, MultiStepInputFactory } from './utils/multiStep import { Random } from './utils/random'; import { JupyterNotInstalledNotificationHelper } from '../jupyter/jupyterNotInstalledNotificationHelper'; import { IJupyterNotInstalledNotificationHelper } from '../jupyter/types'; +import { InterpreterPathProxyService } from './interpreterPathProxyService'; export function registerTypes(serviceManager: IServiceManager): void { serviceManager.addSingletonInstance(IsWindows, IS_WINDOWS); serviceManager.addSingleton(IActiveResourceService, ActiveResourceService); + serviceManager.addSingleton( + IInterpreterPathProxyService, + InterpreterPathProxyService, + ); serviceManager.addSingleton(IInterpreterPathService, InterpreterPathService); serviceManager.addSingleton(IExtensions, Extensions); serviceManager.addSingleton(IRandom, Random); diff --git a/src/client/common/types.ts b/src/client/common/types.ts index 8b9e423dea73..bf437ff3d493 100644 --- a/src/client/common/types.ts +++ b/src/client/common/types.ts @@ -557,6 +557,14 @@ export interface IInterpreterPathService { copyOldInterpreterStorageValuesToNew(resource: Uri | undefined): Promise; } +/** + * Interface used to access current Interpreter Path + */ +export const IInterpreterPathProxyService = Symbol('IInterpreterPathProxyService'); +export interface IInterpreterPathProxyService { + get(resource: Resource): string; +} + export type DefaultLSType = LanguageServerType.Jedi | LanguageServerType.JediLSP | LanguageServerType.Node; /** diff --git a/src/client/interpreter/display/index.ts b/src/client/interpreter/display/index.ts index 4e381031fad8..2e1f76e3538b 100644 --- a/src/client/interpreter/display/index.ts +++ b/src/client/interpreter/display/index.ts @@ -3,7 +3,13 @@ import { Disposable, OutputChannel, StatusBarAlignment, StatusBarItem, Uri } fro import { IApplicationShell, IWorkspaceService } from '../../common/application/types'; import { STANDARD_OUTPUT_CHANNEL } from '../../common/constants'; import '../../common/extensions'; -import { IConfigurationService, IDisposableRegistry, IOutputChannel, IPathUtils, Resource } from '../../common/types'; +import { + IDisposableRegistry, + IInterpreterPathProxyService, + IOutputChannel, + IPathUtils, + Resource, +} from '../../common/types'; import { Interpreters } from '../../common/utils/localize'; import { IServiceContainer } from '../../ioc/types'; import { PythonEnvironment } from '../../pythonEnvironments/info'; @@ -22,7 +28,7 @@ export class InterpreterDisplay implements IInterpreterDisplay { private readonly workspaceService: IWorkspaceService; private readonly pathUtils: IPathUtils; private readonly interpreterService: IInterpreterService; - private readonly configService: IConfigurationService; + private readonly interpreterPathExpHelper: IInterpreterPathProxyService; private currentlySelectedInterpreterPath?: string; private currentlySelectedWorkspaceFolder: Resource; private readonly autoSelection: IInterpreterAutoSelectionService; @@ -39,7 +45,9 @@ export class InterpreterDisplay implements IInterpreterDisplay { const application = serviceContainer.get(IApplicationShell); const disposableRegistry = serviceContainer.get(IDisposableRegistry); - this.configService = serviceContainer.get(IConfigurationService); + this.interpreterPathExpHelper = serviceContainer.get( + IInterpreterPathProxyService, + ); this.statusBar = application.createStatusBarItem(StatusBarAlignment.Left, 100); this.statusBar.command = 'python.setInterpreter'; @@ -75,7 +83,7 @@ export class InterpreterDisplay implements IInterpreterDisplay { } } private async updateDisplay(workspaceFolder?: Uri) { - const interpreterPath = this.configService.getSettings(workspaceFolder)?.pythonPath; + const interpreterPath = this.interpreterPathExpHelper.get(workspaceFolder); if (!interpreterPath || interpreterPath === 'python') { await this.autoSelection.autoSelectInterpreter(workspaceFolder); // Block on this only if no interpreter selected. } diff --git a/src/test/common/interpreterPathProxyService.unit.test.ts b/src/test/common/interpreterPathProxyService.unit.test.ts new file mode 100644 index 000000000000..1d7116335a66 --- /dev/null +++ b/src/test/common/interpreterPathProxyService.unit.test.ts @@ -0,0 +1,54 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +'use strict'; + +import { Uri, WorkspaceConfiguration } from 'vscode'; +import * as TypeMoq from 'typemoq'; +import { expect } from 'chai'; +import { InterpreterPathProxyService } from '../../client/common/interpreterPathProxyService'; +import { IExperimentService, IInterpreterPathProxyService, IInterpreterPathService } from '../../client/common/types'; +import { IWorkspaceService } from '../../client/common/application/types'; +import { DeprecatePythonPath } from '../../client/common/experiments/groups'; + +suite('Interpreter Path Proxy Service', async () => { + let interpreterPathProxyService: IInterpreterPathProxyService; + let workspaceService: TypeMoq.IMock; + let experiments: TypeMoq.IMock; + let interpreterPathService: TypeMoq.IMock; + const resource = Uri.parse('a'); + const interpreterPath = 'path/to/interpreter'; + setup(() => { + workspaceService = TypeMoq.Mock.ofType(); + experiments = TypeMoq.Mock.ofType(); + interpreterPathService = TypeMoq.Mock.ofType(); + workspaceService + .setup((w) => w.getWorkspaceFolder(resource)) + .returns(() => ({ + uri: resource, + name: 'Workspacefolder', + index: 0, + })); + interpreterPathProxyService = new InterpreterPathProxyService( + interpreterPathService.object, + experiments.object, + workspaceService.object, + ); + }); + + test('When in experiment, use interpreter path service to get setting value', () => { + experiments.setup((e) => e.inExperimentSync(DeprecatePythonPath.experiment)).returns(() => true); + interpreterPathService.setup((i) => i.get(resource)).returns(() => interpreterPath); + const value = interpreterPathProxyService.get(resource); + expect(value).to.equal(interpreterPath); + }); + + test('When not in experiment, use workspace service to get setting value', () => { + experiments.setup((e) => e.inExperimentSync(DeprecatePythonPath.experiment)).returns(() => false); + const workspaceConfig = TypeMoq.Mock.ofType(); + workspaceService.setup((i) => i.getConfiguration('python', resource)).returns(() => workspaceConfig.object); + workspaceConfig.setup((w) => w.get('pythonPath')).returns(() => interpreterPath); + const value = interpreterPathProxyService.get(resource); + expect(value).to.equal(interpreterPath); + }); +}); diff --git a/src/test/interpreters/display.unit.test.ts b/src/test/interpreters/display.unit.test.ts index d081758b384e..30b6587ed3fa 100644 --- a/src/test/interpreters/display.unit.test.ts +++ b/src/test/interpreters/display.unit.test.ts @@ -16,11 +16,10 @@ import { IApplicationShell, IWorkspaceService } from '../../client/common/applic import { STANDARD_OUTPUT_CHANNEL } from '../../client/common/constants'; import { IFileSystem } from '../../client/common/platform/types'; import { - IConfigurationService, + IInterpreterPathProxyService, IDisposableRegistry, IOutputChannel, IPathUtils, - IPythonSettings, ReadWrite, } from '../../client/common/types'; import { Interpreters } from '../../client/common/utils/localize'; @@ -57,8 +56,7 @@ suite('Interpreters Display', () => { let fileSystem: TypeMoq.IMock; let disposableRegistry: Disposable[]; let statusBar: TypeMoq.IMock; - let pythonSettings: TypeMoq.IMock; - let configurationService: TypeMoq.IMock; + let interpreterPathProxyService: TypeMoq.IMock; let interpreterDisplay: IInterpreterDisplay; let interpreterHelper: TypeMoq.IMock; let pathUtils: TypeMoq.IMock; @@ -73,8 +71,7 @@ suite('Interpreters Display', () => { interpreterHelper = TypeMoq.Mock.ofType(); disposableRegistry = []; statusBar = TypeMoq.Mock.ofType(); - pythonSettings = TypeMoq.Mock.ofType(); - configurationService = TypeMoq.Mock.ofType(); + interpreterPathProxyService = TypeMoq.Mock.ofType(); pathUtils = TypeMoq.Mock.ofType(); output = TypeMoq.Mock.ofType(); autoSelection = mock(InterpreterAutoSelectionService); @@ -94,8 +91,8 @@ suite('Interpreters Display', () => { serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(IFileSystem))).returns(() => fileSystem.object); serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(IDisposableRegistry))).returns(() => disposableRegistry); serviceContainer - .setup((c) => c.get(TypeMoq.It.isValue(IConfigurationService))) - .returns(() => configurationService.object); + .setup((c) => c.get(TypeMoq.It.isValue(IInterpreterPathProxyService))) + .returns(() => interpreterPathProxyService.object); serviceContainer .setup((c) => c.get(TypeMoq.It.isValue(IInterpreterHelper))) .returns(() => interpreterHelper.object); @@ -215,8 +212,7 @@ suite('Interpreters Display', () => { interpreterService .setup((i) => i.getActiveInterpreter(TypeMoq.It.isValue(workspaceFolder))) .returns(() => Promise.resolve(undefined)); - configurationService.setup((c) => c.getSettings(TypeMoq.It.isAny())).returns(() => pythonSettings.object); - pythonSettings.setup((p) => p.pythonPath).returns(() => pythonPath); + interpreterPathProxyService.setup((c) => c.get(TypeMoq.It.isAny())).returns(() => pythonPath); fileSystem.setup((f) => f.fileExists(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve(false)); interpreterHelper .setup((v) => v.getInterpreterInformation(TypeMoq.It.isValue(pythonPath)))