Leave pipenv in a corner until the user decides to select an interpre…

…ter (microsoft#11654)

* add onSuggestion option
* Swap onActivation with onSuggestion
* Update unit tests
* Remove registration of IPipenvService
* Move didTriggerInterpreterSuggestions logic inside pipenv locator
* Fix existing unit tests
* Add new unit tests
* Replace typemoq any param with object
* Shorten the tests
* Fix warning
* Duplicate teardown

src/client/activation/activationManager.ts

@@ -73,7 +73,7 @@ export class ExtensionActivationManager implements IExtensionActivationManager {
         this.experiments.sendTelemetryIfInExperiment(DeprecatePythonPath.control);
 
         // Get latest interpreter list in the background.
-        this.interpreterService.getInterpreters(resource, { onActivation: true }).ignoreErrors();
+        this.interpreterService.getInterpreters(resource).ignoreErrors();
 
         await sendActivationTelemetry(this.fileSystem, this.workspaceService, resource);
 

src/client/application/diagnostics/checks/macPythonInterpreter.ts

@@ -101,7 +101,7 @@ export class InvalidMacPythonInterpreterService extends BaseDiagnosticsService {
             return [];
         }
 
-        const interpreters = await this.interpreterService.getInterpreters(resource, { onActivation: true });
+        const interpreters = await this.interpreterService.getInterpreters(resource);
         if (interpreters.filter((i) => !this.helper.isMacDefaultPythonPath(i.path)).length === 0) {
             return [
                 new InvalidMacPythonInterpreterDiagnostic(

src/client/common/terminal/environmentActivationProviders/pipEnvActivationProvider.ts

@@ -3,10 +3,16 @@
 
 'use strict';
 
-import { inject, injectable } from 'inversify';
+import { inject, injectable, named } from 'inversify';
 import { Uri } from 'vscode';
 import '../../../common/extensions';
-import { IInterpreterService, InterpreterType, IPipEnvService } from '../../../interpreter/contracts';
+import {
+    IInterpreterLocatorService,
+    IInterpreterService,
+    InterpreterType,
+    IPipEnvService,
+    PIPENV_SERVICE
+} from '../../../interpreter/contracts';
 import { IWorkspaceService } from '../../application/types';
 import { IFileSystem } from '../../platform/types';
 import { ITerminalActivationCommandProvider, TerminalShellType } from '../types';
@@ -15,7 +21,9 @@ import { ITerminalActivationCommandProvider, TerminalShellType } from '../types'
 export class PipEnvActivationCommandProvider implements ITerminalActivationCommandProvider {
     constructor(
         @inject(IInterpreterService) private readonly interpreterService: IInterpreterService,
-        @inject(IPipEnvService) private readonly pipenvService: IPipEnvService,
+        @inject(IInterpreterLocatorService)
+        @named(PIPENV_SERVICE)
+        private readonly pipenvService: IPipEnvService,
         @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService,
         @inject(IFileSystem) private readonly fs: IFileSystem
     ) {}

src/client/interpreter/autoSelection/rules/system.ts

@@ -25,7 +25,7 @@ export class SystemWideInterpretersAutoSelectionRule extends BaseRuleService {
         resource: Resource,
         manager?: IInterpreterAutoSelectionService
     ): Promise<NextAction> {
-        const interpreters = await this.interpreterService.getInterpreters(resource, { onActivation: true });
+        const interpreters = await this.interpreterService.getInterpreters(resource);
         // Exclude non-local interpreters.
         const filteredInterpreters = interpreters.filter(
             (int) =>

src/client/interpreter/configuration/interpreterSelector/interpreterSelector.ts

@@ -27,7 +27,7 @@ export class InterpreterSelector implements IInterpreterSelector {
     }
 
     public async getSuggestions(resource: Resource) {
-        let interpreters = await this.interpreterManager.getInterpreters(resource);
+        let interpreters = await this.interpreterManager.getInterpreters(resource, { onSuggestion: true });
         if (this.experimentsManager.inExperiment(DeprecatePythonPath.experiment)) {
             interpreters = interpreters.filter((item) => this.interpreterSecurityService.isSafe(item) !== false);
         }

src/client/interpreter/contracts.ts

@@ -28,7 +28,7 @@ export interface IVirtualEnvironmentsSearchPathProvider {
 }
 
 export type GetInterpreterOptions = {
-    onActivation?: boolean;
+    onSuggestion?: boolean;
 };
 
 export type GetInterpreterLocatorOptions = GetInterpreterOptions & { ignoreCache?: boolean };
@@ -38,6 +38,7 @@ export const IInterpreterLocatorService = Symbol('IInterpreterLocatorService');
 export interface IInterpreterLocatorService extends Disposable {
     readonly onLocating: Event<Promise<PythonInterpreter[]>>;
     readonly hasInterpreters: Promise<boolean>;
+    didTriggerInterpreterSuggestions?: boolean;
     getInterpreters(resource?: Uri, options?: GetInterpreterLocatorOptions): Promise<PythonInterpreter[]>;
 }
 
@@ -126,7 +127,7 @@ export interface IInterpreterHelper {
 }
 
 export const IPipEnvService = Symbol('IPipEnvService');
-export interface IPipEnvService {
+export interface IPipEnvService extends IInterpreterLocatorService {
     executable: string;
     isRelatedPipEnvironment(dir: string, pythonPath: string): Promise<boolean>;
 }

src/client/interpreter/locators/index.ts

@@ -30,10 +30,13 @@ const flatten = require('lodash/flatten') as typeof import('lodash/flatten');
  */
 @injectable()
 export class PythonInterpreterLocatorService implements IInterpreterLocatorService {
+    public didTriggerInterpreterSuggestions: boolean;
+
     private readonly disposables: Disposable[] = [];
     private readonly platform: IPlatformService;
     private readonly interpreterLocatorHelper: IInterpreterLocatorHelper;
     private readonly _hasInterpreters: Deferred<boolean>;
+
     constructor(
         @inject(IServiceContainer) private serviceContainer: IServiceContainer,
         @inject(InterpreterFilter) private readonly interpreterFilter: IInterpreterFilter
@@ -42,6 +45,7 @@ export class PythonInterpreterLocatorService implements IInterpreterLocatorServi
         serviceContainer.get<Disposable[]>(IDisposableRegistry).push(this);
         this.platform = serviceContainer.get<IPlatformService>(IPlatformService);
         this.interpreterLocatorHelper = serviceContainer.get<IInterpreterLocatorHelper>(IInterpreterLocatorHelper);
+        this.didTriggerInterpreterSuggestions = false;
     }
     /**
      * This class should never emit events when we're locating.
@@ -106,18 +110,27 @@ export class PythonInterpreterLocatorService implements IInterpreterLocatorServi
         // The order is important because the data sources at the bottom of the list do not contain all,
         //  the information about the interpreters (e.g. type, environment name, etc).
         // This way, the items returned from the top of the list will win, when we combine the items returned.
-        const keys: [string | undefined, OSType | undefined][] = [
+        const keys: [string, OSType | undefined][] = [
             [WINDOWS_REGISTRY_SERVICE, OSType.Windows],
             [CONDA_ENV_SERVICE, undefined],
             [CONDA_ENV_FILE_SERVICE, undefined],
-            options?.onActivation ? [undefined, undefined] : [PIPENV_SERVICE, undefined],
+            [PIPENV_SERVICE, undefined],
             [GLOBAL_VIRTUAL_ENV_SERVICE, undefined],
             [WORKSPACE_VIRTUAL_ENV_SERVICE, undefined],
             [KNOWN_PATH_SERVICE, undefined],
             [CURRENT_PATH_SERVICE, undefined]
         ];
-        return keys
-            .filter((item) => item[0] !== undefined && (item[1] === undefined || item[1] === this.platform.osType))
+
+        const locators = keys
+            .filter((item) => item[1] === undefined || item[1] === this.platform.osType)
             .map((item) => this.serviceContainer.get<IInterpreterLocatorService>(IInterpreterLocatorService, item[0]));
+
+        // Set it to true the first time the user selects an interpreter
+        if (!this.didTriggerInterpreterSuggestions && options?.onSuggestion === true) {
+            this.didTriggerInterpreterSuggestions = true;
+            locators.forEach((locator) => (locator.didTriggerInterpreterSuggestions = true));
+        }
+
+        return locators;
     }
 }

src/client/interpreter/locators/services/cacheableLocatorService.ts

@@ -70,13 +70,24 @@ export abstract class CacheableLocatorService implements IInterpreterLocatorServ
     private readonly handlersAddedToResource = new Set<string>();
     private readonly cacheKeyPrefix: string;
     private readonly locating = new EventEmitter<Promise<PythonInterpreter[]>>();
+    private _didTriggerInterpreterSuggestions: boolean;
+
     constructor(
         @unmanaged() private readonly name: string,
         @unmanaged() protected readonly serviceContainer: IServiceContainer,
         @unmanaged() private cachePerWorkspace: boolean = false
     ) {
         this._hasInterpreters = createDeferred<boolean>();
         this.cacheKeyPrefix = `INTERPRETERS_CACHE_v3_${name}`;
+        this._didTriggerInterpreterSuggestions = false;
+    }
+
+    public get didTriggerInterpreterSuggestions(): boolean {
+        return this._didTriggerInterpreterSuggestions;
+    }
+
+    public set didTriggerInterpreterSuggestions(value: boolean) {
+        this._didTriggerInterpreterSuggestions = value;
     }
 
     public get onLocating(): Event<Promise<PythonInterpreter[]>> {

src/client/interpreter/locators/services/pipEnvService.ts

@@ -43,9 +43,15 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer
         this.configService = this.serviceContainer.get<IConfigurationService>(IConfigurationService);
         this.pipEnvServiceHelper = this.serviceContainer.get<IPipEnvServiceHelper>(IPipEnvServiceHelper);
     }
+
     // tslint:disable-next-line:no-empty
     public dispose() {}
+
     public async isRelatedPipEnvironment(dir: string, pythonPath: string): Promise<boolean> {
+        if (!this.didTriggerInterpreterSuggestions) {
+            return false;
+        }
+
         // In PipEnv, the name of the cwd is used as a prefix in the virtual env.
         if (pythonPath.indexOf(`${path.sep}${path.basename(dir)}-`) === -1) {
             return false;
@@ -55,10 +61,14 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer
     }
 
     public get executable(): string {
-        return this.configService.getSettings().pipenvPath;
+        return this.didTriggerInterpreterSuggestions ? this.configService.getSettings().pipenvPath : '';
     }
 
     public async getInterpreters(resource?: Uri, options?: GetInterpreterLocatorOptions): Promise<PythonInterpreter[]> {
+        if (!this.didTriggerInterpreterSuggestions) {
+            return [];
+        }
+
         const stopwatch = new StopWatch();
         const startDiscoveryTime = stopwatch.elapsedTime;
 
@@ -71,6 +81,10 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer
     }
 
     protected getInterpretersImplementation(resource?: Uri): Promise<PythonInterpreter[]> {
+        if (!this.didTriggerInterpreterSuggestions) {
+            return Promise.resolve([]);
+        }
+
         const pipenvCwd = this.getPipenvWorkingDirectory(resource);
         if (!pipenvCwd) {
             return Promise.resolve([]);
@@ -146,6 +160,7 @@ export class PipEnvService extends CacheableLocatorService implements IPipEnvSer
             }
         }
     }
+
     private async checkIfPipFileExists(cwd: string): Promise<boolean> {
         const currentProcess = this.serviceContainer.get<ICurrentProcess>(ICurrentProcess);
         const pipFileName = currentProcess.env[pipEnvFileNameVariable];

src/client/interpreter/serviceRegistry.ts

@@ -60,7 +60,6 @@ import {
     IInterpreterWatcherBuilder,
     IKnownSearchPathsForInterpreters,
     INTERPRETER_LOCATOR_SERVICE,
-    IPipEnvService,
     IShebangCodeLensProvider,
     IVirtualEnvironmentsSearchPathProvider,
     KNOWN_PATH_SERVICE,
@@ -200,7 +199,6 @@ export function registerInterpreterTypes(serviceManager: IServiceManager) {
         WORKSPACE_VIRTUAL_ENV_SERVICE
     );
     serviceManager.addSingleton<IInterpreterLocatorService>(IInterpreterLocatorService, PipEnvService, PIPENV_SERVICE);
-    serviceManager.addSingleton<IInterpreterLocatorService>(IPipEnvService, PipEnvService);
 
     serviceManager.addSingleton<IInterpreterLocatorService>(
         IInterpreterLocatorService,

src/client/interpreter/virtualEnvs/index.ts

@@ -12,7 +12,7 @@ import { ICurrentProcess, IPathUtils } from '../../common/types';
 import { getNamesAndValues } from '../../common/utils/enum';
 import { noop } from '../../common/utils/misc';
 import { IServiceContainer } from '../../ioc/types';
-import { InterpreterType, IPipEnvService } from '../contracts';
+import { IInterpreterLocatorService, InterpreterType, IPipEnvService, PIPENV_SERVICE } from '../contracts';
 import { IVirtualEnvironmentManager } from './types';
 
 const PYENVFILES = ['pyvenv.cfg', path.join('..', 'pyvenv.cfg')];
@@ -27,7 +27,10 @@ export class VirtualEnvironmentManager implements IVirtualEnvironmentManager {
     constructor(@inject(IServiceContainer) private readonly serviceContainer: IServiceContainer) {
         this.processServiceFactory = serviceContainer.get<IProcessServiceFactory>(IProcessServiceFactory);
         this.fs = serviceContainer.get<IFileSystem>(IFileSystem);
-        this.pipEnvService = serviceContainer.get<IPipEnvService>(IPipEnvService);
+        this.pipEnvService = serviceContainer.get<IInterpreterLocatorService>(
+            IInterpreterLocatorService,
+            PIPENV_SERVICE
+        ) as IPipEnvService;
         this.workspaceService = serviceContainer.get<IWorkspaceService>(IWorkspaceService);
     }
     public async getEnvironmentName(pythonPath: string, resource?: Uri): Promise<string> {
@@ -37,6 +40,7 @@ export class VirtualEnvironmentManager implements IVirtualEnvironmentManager {
         const workspaceFolder = resource ? this.workspaceService.getWorkspaceFolder(resource) : undefined;
         const workspaceUri = workspaceFolder ? workspaceFolder.uri : defaultWorkspaceUri;
         const grandParentDirName = path.basename(path.dirname(path.dirname(pythonPath)));
+
         if (workspaceUri && (await this.pipEnvService.isRelatedPipEnvironment(workspaceUri.fsPath, pythonPath))) {
             // In pipenv, return the folder name of the workspace.
             return path.basename(workspaceUri.fsPath);

src/client/startupTelemetry.ts

@@ -129,9 +129,7 @@ async function getActivationTelemetryProps(serviceContainer: IServiceContainer):
             .then((ver) => (ver ? ver.raw : ''))
             .catch<string>(() => ''),
         interpreterService.getActiveInterpreter().catch<PythonInterpreter | undefined>(() => undefined),
-        interpreterService
-            .getInterpreters(mainWorkspaceUri, { onActivation: true })
-            .catch<PythonInterpreter[]>(() => [])
+        interpreterService.getInterpreters(mainWorkspaceUri).catch<PythonInterpreter[]>(() => [])
     ]);
     const workspaceFolderCount = workspaceService.hasWorkspaceFolders ? workspaceService.workspaceFolders!.length : 0;
     const pythonVersion = interpreter && interpreter.version ? interpreter.version.raw : undefined;

src/test/activation/activationManager.unit.test.ts

@@ -198,7 +198,7 @@ suite('Activation Manager', () => {
             when(workspaceService.getWorkspaceFolder(resource)).thenReturn(folder2);
             when(activationService1.activate(resource)).thenResolve();
             when(activationService2.activate(resource)).thenResolve();
-            when(interpreterService.getInterpreters(anything(), anything())).thenResolve();
+            when(interpreterService.getInterpreters(anything())).thenResolve();
             autoSelection
                 .setup((a) => a.autoSelectInterpreter(resource))
                 .returns(() => Promise.resolve())
@@ -232,7 +232,7 @@ suite('Activation Manager', () => {
             const resource = Uri.parse('two');
             when(activationService1.activate(resource)).thenResolve();
             when(activationService2.activate(resource)).thenResolve();
-            when(interpreterService.getInterpreters(anything(), anything())).thenResolve();
+            when(interpreterService.getInterpreters(anything())).thenResolve();
             autoSelection
                 .setup((a) => a.autoSelectInterpreter(resource))
                 .returns(() => Promise.resolve())
@@ -252,7 +252,7 @@ suite('Activation Manager', () => {
             const resource = Uri.parse('two');
             when(activationService1.activate(resource)).thenResolve();
             when(activationService2.activate(resource)).thenResolve();
-            when(interpreterService.getInterpreters(anything(), anything())).thenResolve();
+            when(interpreterService.getInterpreters(anything())).thenResolve();
             when(experiments.inExperiment(DeprecatePythonPath.experiment)).thenReturn(true);
             interpreterPathService
                 .setup((i) => i.copyOldInterpreterStorageValuesToNew(resource))
@@ -278,7 +278,7 @@ suite('Activation Manager', () => {
             const resource = Uri.parse('two');
             when(activationService1.activate(resource)).thenResolve();
             when(activationService2.activate(resource)).thenResolve();
-            when(interpreterService.getInterpreters(anything(), anything())).thenResolve();
+            when(interpreterService.getInterpreters(anything())).thenResolve();
             autoSelection
                 .setup((a) => a.autoSelectInterpreter(resource))
                 .returns(() => Promise.resolve())

src/test/application/diagnostics/checks/macPythonInterpreter.unit.test.ts

@@ -174,7 +174,7 @@ suite('Application Diagnostics - Checks Mac Python Interpreter', () => {
                 .returns(() => Promise.resolve(true))
                 .verifiable(typemoq.Times.once());
             interpreterService
-                .setup((i) => i.getInterpreters(typemoq.It.isAny(), { onActivation: true }))
+                .setup((i) => i.getInterpreters(typemoq.It.isAny()))
                 .returns(() => Promise.resolve([{} as any]))
                 .verifiable(typemoq.Times.never());
             interpreterService
@@ -204,7 +204,7 @@ suite('Application Diagnostics - Checks Mac Python Interpreter', () => {
                 .returns(() => Promise.resolve(true))
                 .verifiable(typemoq.Times.once());
             interpreterService
-                .setup((i) => i.getInterpreters(typemoq.It.isAny(), { onActivation: true }))
+                .setup((i) => i.getInterpreters(typemoq.It.isAny()))
                 .returns(() => Promise.resolve([{} as any]))
                 .verifiable(typemoq.Times.never());
             interpreterService
@@ -235,7 +235,7 @@ suite('Application Diagnostics - Checks Mac Python Interpreter', () => {
                 .returns(() => false)
                 .verifiable(typemoq.Times.once());
             interpreterService
-                .setup((i) => i.getInterpreters(typemoq.It.isAny(), { onActivation: true }))
+                .setup((i) => i.getInterpreters(typemoq.It.isAny()))
                 .returns(() => Promise.resolve([{ path: pythonPath } as any, { path: pythonPath } as any]))
                 .verifiable(typemoq.Times.once());
             interpreterService
@@ -275,7 +275,7 @@ suite('Application Diagnostics - Checks Mac Python Interpreter', () => {
                 .returns(() => false)
                 .verifiable(typemoq.Times.once());
             interpreterService
-                .setup((i) => i.getInterpreters(typemoq.It.isAny(), { onActivation: true }))
+                .setup((i) => i.getInterpreters(typemoq.It.isAny()))
                 .returns(() =>
                     Promise.resolve([
                         { path: pythonPath } as any,

src/test/configuration/interpreterSelector/interpreterSelector.unit.test.ts

@@ -106,7 +106,7 @@ suite('Interpreters - selector', () => {
                 return { ...info, ...item };
             });
             interpreterService
-                .setup((x) => x.getInterpreters(TypeMoq.It.isAny()))
+                .setup((x) => x.getInterpreters(TypeMoq.It.isAny(), { onSuggestion: true }))
                 .returns(() => new Promise((resolve) => resolve(initial)));
 
             const actual = await selector.getSuggestions(undefined);
@@ -139,7 +139,9 @@ suite('Interpreters - selector', () => {
     test('When in Deprecate PythonPath experiment, remove unsafe interpreters from the suggested interpreters list', async () => {
         // tslint:disable-next-line: no-any
         const interpreterList = ['interpreter1', 'interpreter2', 'interpreter3'] as any;
-        interpreterService.setup((i) => i.getInterpreters(folder1.uri)).returns(() => interpreterList);
+        interpreterService
+            .setup((i) => i.getInterpreters(folder1.uri, { onSuggestion: true }))
+            .returns(() => interpreterList);
         // tslint:disable-next-line: no-any
         interpreterSecurityService.setup((i) => i.isSafe('interpreter1' as any)).returns(() => true);
         // tslint:disable-next-line: no-any