Cherry-pick pipenv changes and pythonpath prompt changes to release (#11700)

* Show the prompt again if user clicks on more info (#11664)

* Show the prompt again if user clicks on more info

* Review feedback

* Use Learn more as text for the link.

* Leave pipenv in a corner until the user decides to select an interpreter (#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

Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com>

Author: karthiknadig

package.nls.json

@@ -240,9 +240,9 @@
     "DataScience.liveShareServiceFailure": "Failure starting '{0}' service during live share connection.",
     "DataScience.documentMismatch": "Cannot run cells, duplicate documents for {0} found.",
     "DataScience.pythonInteractiveCreateFailed": "Failure to create a 'Python Interactive' window. Try reinstalling the Python extension.",
-    "diagnostics.removePythonPathSettingsJson": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the \"python.pythonPath\" entry from your settings.json.",
-    "diagnostics.removePythonPathCodeWorkspace": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the \"python.pythonPath\" entry from your .code-workspace file.",
-    "diagnostics.removePythonPathCodeWorkspaceAndSettingsJson": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the \"python.pythonPath\" entry from your settings.json and .code-workspace file.",
+    "diagnostics.removePythonPathSettingsJson": "The setting \"python.pythonPath\" defined in your settings.json is now deprecated. Do you want us to delete it from your settings.json only? [Learn more](https://aka.ms/AA7jfor).",
+    "diagnostics.removePythonPathCodeWorkspace": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file only? [Learn more](https://aka.ms/AA7jfor).",
+    "diagnostics.removePythonPathCodeWorkspaceAndSettingsJson": "The setting \"python.pythonPath\" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file and settings.json? [Learn more](https://aka.ms/AA7jfor).",
     "diagnostics.warnSourceMaps": "Source map support is enabled in the Python Extension, this will adversely impact performance of the extension.",
     "diagnostics.disableSourceMaps": "Disable Source Map Support",
     "diagnostics.warnBeforeEnablingSourceMaps": "Enabling source map support in the Python Extension will adversely impact performance of the extension.",

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/application/diagnostics/checks/pythonPathDeprecated.ts

@@ -9,7 +9,6 @@ import { IWorkspaceService } from '../../../common/application/types';
 import { DeprecatePythonPath } from '../../../common/experimentGroups';
 import { IDisposableRegistry, IExperimentsManager, Resource } from '../../../common/types';
 import { Common, Diagnostics } from '../../../common/utils/localize';
-import { learnMoreOnInterpreterSecurityURI } from '../../../interpreter/autoSelection/constants';
 import { IServiceContainer } from '../../../ioc/types';
 import { BaseDiagnostic, BaseDiagnosticsService } from '../base';
 import { IDiagnosticsCommandFactory } from '../commands/types';
@@ -97,13 +96,6 @@ export class PythonPathDeprecatedDiagnosticService extends BaseDiagnosticsServic
             {
                 prompt: Common.noIWillDoItLater()
             },
-            {
-                prompt: Common.moreInfo(),
-                command: commandFactory.createCommand(diagnostic, {
-                    type: 'launch',
-                    options: learnMoreOnInterpreterSecurityURI
-                })
-            },
             {
                 prompt: Common.doNotShowAgain(),
                 command: commandFactory.createCommand(diagnostic, { type: 'ignore', options: DiagnosticScope.Global })

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/common/utils/localize.ts

@@ -32,15 +32,15 @@ export namespace Diagnostics {
     );
     export const removePythonPathSettingsJson = localize(
         'diagnostics.removePythonPathSettingsJson',
-        'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the "python.pythonPath" entry from your settings.json.'
+        'The setting "python.pythonPath" defined in your settings.json is now deprecated. Do you want us to delete it from your settings.json only? [Learn more](https://aka.ms/AA7jfor).'
     );
     export const removePythonPathCodeWorkspace = localize(
         'diagnostics.removePythonPathCodeWorkspace',
-        'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the "python.pythonPath" entry from your .code-workspace file.'
+        'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file only? [Learn more](https://aka.ms/AA7jfor).'
     );
     export const removePythonPathCodeWorkspaceAndSettingsJson = localize(
         'diagnostics.removePythonPathCodeWorkspaceAndSettingsJson',
-        'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the "python.pythonPath" entry from your settings.json and .code-workspace file.'
+        'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file and settings.json? [Learn more](https://aka.ms/AA7jfor).'
     );
     export const invalidPythonPathInDebuggerSettings = localize(
         'diagnostics.invalidPythonPathInDebuggerSettings',

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;