Remove python.pythonPath experiment (microsoft/vscode-python#18213)

* Remove DeprecatePythonPath experiment

* Fix unit tests

* Fix tests for configsettings

* Fix mac interpreter tests

* Fix activation manager tests

* Fix config service tests

* Fix startup telemetry tests

* Remove all traces of experiment

* Fix python path updater tests

* Update setting description for `python.defaultInterpreterPath`

* Fix single workspace tests attempt#1

* Do not import from client folder in smoke tests

* new enttry

Author: Kartik Raj

extensions/positron-python/data/.vscode/settings.json

@@ -1,3 +1,3 @@
 {
-    "python.pythonPath": "/usr/bin/python3"
+    "python.defaultInterpreterPath": "/usr/bin/python3"
 }

extensions/positron-python/news/3 Code Health/17977.md

@@ -0,0 +1 @@
+Remove `python.pythonPath` setting and `pythonDeprecatePythonPath` experiment.

extensions/positron-python/package.json

@@ -455,7 +455,7 @@
                 },
                 "python.defaultInterpreterPath": {
                     "default": "python",
-                    "description": "Path to Python, you can use a custom version of Python by modifying this setting to include the full path. This default setting is used as a fallback if no interpreter is selected for the workspace. The extension will also not set nor change the value of this setting, it will only read from it.",
+                    "description": "Path to default Python to use when extension loads up for the first time, no longer used once an interpreter is selected for the workspace. See https://aka.ms/AAfekmf to understand when this is used.",
                     "scope": "machine-overridable",
                     "type": "string"
                 },
@@ -982,12 +982,6 @@
                     "scope": "machine-overridable",
                     "type": "string"
                 },
-                "python.pythonPath": {
-                    "default": "python",
-                    "description": "(DEPRECATED: Note this setting is not used when in pythonDeprecatePythonPath experiment) Path to Python, you can use a custom version of Python by modifying this setting to include the full path.",
-                    "scope": "machine-overridable",
-                    "type": "string"
-                },
                 "python.sortImports.args": {
                     "default": [],
                     "description": "Arguments passed in. Each argument is a separate item in the array.",

extensions/positron-python/src/client/activation/activationManager.ts

@@ -8,9 +8,8 @@ import { TextDocument } from 'vscode';
 import { IApplicationDiagnostics } from '../application/types';
 import { IActiveResourceService, IDocumentManager, IWorkspaceService } from '../common/application/types';
 import { PYTHON_LANGUAGE } from '../common/constants';
-import { DeprecatePythonPath } from '../common/experiments/groups';
 import { IFileSystem } from '../common/platform/types';
-import { IDisposable, IExperimentService, IInterpreterPathService, Resource } from '../common/types';
+import { IDisposable, Resource } from '../common/types';
 import { Deferred } from '../common/utils/async';
 import { IInterpreterAutoSelectionService } from '../interpreter/autoSelection/types';
 import { traceDecoratorError } from '../logging';
@@ -37,8 +36,6 @@ export class ExtensionActivationManager implements IExtensionActivationManager {
         @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService,
         @inject(IFileSystem) private readonly fileSystem: IFileSystem,
         @inject(IActiveResourceService) private readonly activeResourceService: IActiveResourceService,
-        @inject(IExperimentService) private readonly experiments: IExperimentService,
-        @inject(IInterpreterPathService) private readonly interpreterPathService: IInterpreterPathService,
     ) {
         if (!this.workspaceService.isTrusted) {
             this.activationServices = this.activationServices.filter(
@@ -90,9 +87,6 @@ export class ExtensionActivationManager implements IExtensionActivationManager {
 
         if (this.workspaceService.isTrusted) {
             // Do not interact with interpreters in a untrusted workspace.
-            if (this.experiments.inExperimentSync(DeprecatePythonPath.experiment)) {
-                await this.interpreterPathService.copyOldInterpreterStorageValuesToNew(resource);
-            }
             await this.autoSelection.autoSelectInterpreter(resource);
         }
         await sendActivationTelemetry(this.fileSystem, this.workspaceService, resource);

extensions/positron-python/src/client/activation/jedi/languageServerProxy.ts

@@ -11,8 +11,7 @@ import {
 } from 'vscode-languageclient/node';
 
 import { ChildProcess } from 'child_process';
-import { DeprecatePythonPath } from '../../common/experiments/groups';
-import { IExperimentService, IInterpreterPathService, Resource } from '../../common/types';
+import { IInterpreterPathService, Resource } from '../../common/types';
 import { PythonEnvironment } from '../../pythonEnvironments/info';
 import { captureTelemetry } from '../../telemetry';
 import { EventName } from '../../telemetry/constants';
@@ -37,7 +36,6 @@ export class JediLanguageServerProxy implements ILanguageServerProxy {
 
     constructor(
         @inject(ILanguageClientFactory) private readonly factory: ILanguageClientFactory,
-        @inject(IExperimentService) private readonly experiments: IExperimentService,
         @inject(IInterpreterPathService) private readonly interpreterPathService: IInterpreterPathService,
     ) {}
 
@@ -148,18 +146,16 @@ export class JediLanguageServerProxy implements ILanguageServerProxy {
         const progressReporting = new ProgressReporting(this.languageClient!);
         this.disposables.push(progressReporting);
 
-        if (this.experiments.inExperimentSync(DeprecatePythonPath.experiment)) {
-            this.disposables.push(
-                this.interpreterPathService.onDidChange(() => {
-                    // Manually send didChangeConfiguration in order to get the server to re-query
-                    // the workspace configurations (to then pick up pythonPath set in the middleware).
-                    // This is needed as interpreter changes via the interpreter path service happen
-                    // outside of VS Code's settings (which would mean VS Code sends the config updates itself).
-                    this.languageClient!.sendNotification(DidChangeConfigurationNotification.type, {
-                        settings: null,
-                    });
-                }),
-            );
-        }
+        this.disposables.push(
+            this.interpreterPathService.onDidChange(() => {
+                // Manually send didChangeConfiguration in order to get the server to re-query
+                // the workspace configurations (to then pick up pythonPath set in the middleware).
+                // This is needed as interpreter changes via the interpreter path service happen
+                // outside of VS Code's settings (which would mean VS Code sends the config updates itself).
+                this.languageClient!.sendNotification(DidChangeConfigurationNotification.type, {
+                    settings: null,
+                });
+            }),
+        );
     }
 }

extensions/positron-python/src/client/activation/node/languageServerProxy.ts

@@ -11,7 +11,6 @@ import {
     State,
 } from 'vscode-languageclient/node';
 
-import { DeprecatePythonPath } from '../../common/experiments/groups';
 import { IConfigurationService, IExperimentService, IInterpreterPathService, Resource } from '../../common/types';
 import { createDeferred, Deferred, sleep } from '../../common/utils/async';
 import { noop } from '../../common/utils/misc';
@@ -177,20 +176,17 @@ export class NodeLanguageServerProxy implements ILanguageServerProxy {
         const progressReporting = new ProgressReporting(this.languageClient!);
         this.disposables.push(progressReporting);
 
-        if (this.experimentService.inExperimentSync(DeprecatePythonPath.experiment)) {
-            this.disposables.push(
-                this.interpreterPathService.onDidChange(() => {
-                    // Manually send didChangeConfiguration in order to get the server to requery
-                    // the workspace configurations (to then pick up pythonPath set in the middleware).
-                    // This is needed as interpreter changes via the interpreter path service happen
-                    // outside of VS Code's settings (which would mean VS Code sends the config updates itself).
-                    this.languageClient!.sendNotification(DidChangeConfigurationNotification.type, {
-                        settings: null,
-                    });
-                }),
-            );
-        }
-
+        this.disposables.push(
+            this.interpreterPathService.onDidChange(() => {
+                // Manually send didChangeConfiguration in order to get the server to requery
+                // the workspace configurations (to then pick up pythonPath set in the middleware).
+                // This is needed as interpreter changes via the interpreter path service happen
+                // outside of VS Code's settings (which would mean VS Code sends the config updates itself).
+                this.languageClient!.sendNotification(DidChangeConfigurationNotification.type, {
+                    settings: null,
+                });
+            }),
+        );
         this.disposables.push(
             this.environmentService.onDidEnvironmentVariablesChange(() => {
                 this.languageClient!.sendNotification(DidChangeConfigurationNotification.type, {

extensions/positron-python/src/client/application/diagnostics/checks/macPythonInterpreter.ts

@@ -3,15 +3,12 @@
 
 // eslint-disable-next-line max-classes-per-file
 import { inject, injectable } from 'inversify';
-import { ConfigurationChangeEvent, DiagnosticSeverity, Uri } from 'vscode';
-import { IWorkspaceService } from '../../../common/application/types';
-import { DeprecatePythonPath } from '../../../common/experiments/groups';
+import { DiagnosticSeverity } from 'vscode';
 import '../../../common/extensions';
 import { IPlatformService } from '../../../common/platform/types';
 import {
     IConfigurationService,
     IDisposableRegistry,
-    IExperimentService,
     IInterpreterPathService,
     InterpreterConfigurationScope,
     Resource,
@@ -148,40 +145,15 @@ export class InvalidMacPythonInterpreterService extends BaseDiagnosticsService {
     }
 
     protected addPythonPathChangedHandler(): void {
-        const workspaceService = this.serviceContainer.get<IWorkspaceService>(IWorkspaceService);
         const disposables = this.serviceContainer.get<IDisposableRegistry>(IDisposableRegistry);
         const interpreterPathService = this.serviceContainer.get<IInterpreterPathService>(IInterpreterPathService);
-        const experiments = this.serviceContainer.get<IExperimentService>(IExperimentService);
-        if (experiments.inExperimentSync(DeprecatePythonPath.experiment)) {
-            disposables.push(interpreterPathService.onDidChange((i) => this.onDidChangeConfiguration(undefined, i)));
-        }
-        disposables.push(workspaceService.onDidChangeConfiguration(this.onDidChangeConfiguration.bind(this)));
+        disposables.push(interpreterPathService.onDidChange((i) => this.onDidChangeConfiguration(i)));
     }
 
     protected async onDidChangeConfiguration(
-        event?: ConfigurationChangeEvent,
-        interpreterConfigurationScope?: InterpreterConfigurationScope,
+        interpreterConfigurationScope: InterpreterConfigurationScope,
     ): Promise<void> {
-        let workspaceUri: Resource;
-        if (event) {
-            const workspaceService = this.serviceContainer.get<IWorkspaceService>(IWorkspaceService);
-            const workspacesUris: (Uri | undefined)[] = workspaceService.hasWorkspaceFolders
-                ? workspaceService.workspaceFolders!.map((workspace) => workspace.uri)
-                : [undefined];
-            const workspaceUriIndex = workspacesUris.findIndex((uri) =>
-                event.affectsConfiguration('python.pythonPath', uri),
-            );
-            if (workspaceUriIndex === -1) {
-                return;
-            }
-            workspaceUri = workspacesUris[workspaceUriIndex];
-        } else if (interpreterConfigurationScope) {
-            workspaceUri = interpreterConfigurationScope.uri;
-        } else {
-            throw new Error(
-                'One of `interpreterConfigurationScope` or `event` should be defined when calling `onDidChangeConfiguration`.',
-            );
-        }
+        const workspaceUri = interpreterConfigurationScope.uri;
         // Lets wait, for more changes, dirty simple throttling.
         if (this.timeOut && typeof this.timeOut !== 'number') {
             clearTimeout(this.timeOut);

extensions/positron-python/src/client/application/diagnostics/checks/pythonPathDeprecated.ts

@@ -5,8 +5,7 @@
 import { inject, named } from 'inversify';
 import { DiagnosticSeverity } from 'vscode';
 import { IWorkspaceService } from '../../../common/application/types';
-import { DeprecatePythonPath } from '../../../common/experiments/groups';
-import { IDisposableRegistry, IExperimentService, Resource } from '../../../common/types';
+import { IDisposableRegistry, Resource } from '../../../common/types';
 import { Common, Diagnostics } from '../../../common/utils/localize';
 import { IServiceContainer } from '../../../ioc/types';
 import { BaseDiagnostic, BaseDiagnosticsService } from '../base';
@@ -44,10 +43,6 @@ export class PythonPathDeprecatedDiagnosticService extends BaseDiagnosticsServic
     }
 
     public async diagnose(resource: Resource): Promise<IDiagnostic[]> {
-        const experiments = this.serviceContainer.get<IExperimentService>(IExperimentService);
-        if (!experiments.inExperimentSync(DeprecatePythonPath.experiment)) {
-            return [];
-        }
         const setting = this.workspaceService.getConfiguration('python', resource).inspect<string>('pythonPath');
         if (!setting) {
             return [];

extensions/positron-python/src/client/application/diagnostics/checks/upgradeCodeRunner.ts

@@ -6,8 +6,7 @@ import { inject, named } from 'inversify';
 import { DiagnosticSeverity } from 'vscode';
 import { IWorkspaceService } from '../../../common/application/types';
 import { CODE_RUNNER_EXTENSION_ID } from '../../../common/constants';
-import { DeprecatePythonPath } from '../../../common/experiments/groups';
-import { IDisposableRegistry, IExperimentService, IExtensions, Resource } from '../../../common/types';
+import { IDisposableRegistry, IExtensions, Resource } from '../../../common/types';
 import { Common, Diagnostics } from '../../../common/utils/localize';
 import { IServiceContainer } from '../../../ioc/types';
 import { BaseDiagnostic, BaseDiagnosticsService } from '../base';
@@ -51,10 +50,6 @@ export class UpgradeCodeRunnerDiagnosticService extends BaseDiagnosticsService {
         if (this._diagnosticReturned) {
             return [];
         }
-        const experiments = this.serviceContainer.get<IExperimentService>(IExperimentService);
-        if (!experiments.inExperimentSync(DeprecatePythonPath.experiment)) {
-            return [];
-        }
         const extension = this.extensions.getExtension(CODE_RUNNER_EXTENSION_ID);
         if (!extension) {
             return [];

extensions/positron-python/src/client/common/configSettings.ts

@@ -23,14 +23,12 @@ import { ITestingSettings } from '../testing/configuration/types';
 import { IWorkspaceService } from './application/types';
 import { WorkspaceService } from './application/workspace';
 import { DEFAULT_INTERPRETER_SETTING, isTestExecution } from './constants';
-import { DeprecatePythonPath } from './experiments/groups';
 import { ExtensionChannels } from './insidersBuild/types';
 import { IS_WINDOWS } from './platform/constants';
 import {
     IAutoCompleteSettings,
     IDefaultLanguageServer,
     IExperiments,
-    IExperimentService,
     IFormattingSettings,
     IInterpreterPathService,
     ILintingSettings,
@@ -146,10 +144,9 @@ export class PythonSettings implements IPythonSettings {
     constructor(
         workspaceFolder: Resource,
         private readonly interpreterAutoSelectionService: IInterpreterAutoSelectionProxyService,
-        workspace?: IWorkspaceService,
-        private readonly experimentsManager?: IExperimentService,
-        private readonly interpreterPathService?: IInterpreterPathService,
-        private readonly defaultLS?: IDefaultLanguageServer,
+        workspace: IWorkspaceService,
+        private readonly interpreterPathService: IInterpreterPathService,
+        private readonly defaultLS: IDefaultLanguageServer | undefined,
     ) {
         this.workspace = workspace || new WorkspaceService();
         this.workspaceRoot = workspaceFolder;
@@ -159,10 +156,9 @@ export class PythonSettings implements IPythonSettings {
     public static getInstance(
         resource: Uri | undefined,
         interpreterAutoSelectionService: IInterpreterAutoSelectionProxyService,
-        workspace?: IWorkspaceService,
-        experimentsManager?: IExperimentService,
-        interpreterPathService?: IInterpreterPathService,
-        defaultLS?: IDefaultLanguageServer,
+        workspace: IWorkspaceService,
+        interpreterPathService: IInterpreterPathService,
+        defaultLS: IDefaultLanguageServer | undefined,
     ): PythonSettings {
         workspace = workspace || new WorkspaceService();
         const workspaceFolderUri = PythonSettings.getSettingsUriAndTarget(resource, workspace).uri;
@@ -173,7 +169,6 @@ export class PythonSettings implements IPythonSettings {
                 workspaceFolderUri,
                 interpreterAutoSelectionService,
                 workspace,
-                experimentsManager,
                 interpreterPathService,
                 defaultLS,
             );
@@ -237,7 +232,7 @@ export class PythonSettings implements IPythonSettings {
         const workspaceRoot = this.workspaceRoot?.fsPath;
         const systemVariables: SystemVariables = new SystemVariables(undefined, workspaceRoot, this.workspace);
 
-        this.pythonPath = this.getPythonPath(pythonSettings, systemVariables, workspaceRoot);
+        this.pythonPath = this.getPythonPath(systemVariables, workspaceRoot);
 
         const defaultInterpreterPath = systemVariables.resolveAny(pythonSettings.get<string>('defaultInterpreterPath'));
         this.defaultInterpreterPath = defaultInterpreterPath || DEFAULT_INTERPRETER_SETTING;
@@ -561,24 +556,8 @@ export class PythonSettings implements IPythonSettings {
         this.changed.fire();
     }
 
-    private getPythonPath(
-        pythonSettings: WorkspaceConfiguration,
-        systemVariables: SystemVariables,
-        workspaceRoot: string | undefined,
-    ) {
-        /**
-         * Note that while calling `IExperimentsManager.inExperiment()`, we assume `IExperimentsManager.activate()` is already called.
-         * That's not true here, as this method is often called in the constructor,which runs before `.activate()` methods.
-         * But we can still use it here for this particular experiment. Reason being that this experiment only changes
-         * `pythonPath` setting, and I've checked that `pythonPath` setting is not accessed anywhere in the constructor.
-         */
-        const inExperiment = this.experimentsManager?.inExperimentSync(DeprecatePythonPath.experiment);
-        // Use the interpreter path service if in the experiment otherwise use the normal settings
-        this.pythonPath = systemVariables.resolveAny(
-            inExperiment && this.interpreterPathService
-                ? this.interpreterPathService.get(this.workspaceRoot)
-                : pythonSettings.get<string>('pythonPath'),
-        )!;
+    private getPythonPath(systemVariables: SystemVariables, workspaceRoot: string | undefined) {
+        this.pythonPath = systemVariables.resolveAny(this.interpreterPathService.get(this.workspaceRoot))!;
         if (
             !process.env.CI_DISABLE_AUTO_SELECTION &&
             (this.pythonPath.length === 0 || this.pythonPath === 'python') &&