Ensure default version and platform only once (#8802)

* Ensure default version and platform only once * Refactor execEnv creation to after everything else * Fix tests to do the same thing as service * Comment missing . * Default python version to behave as it did before
microsoft · Aug 21, 2024 · 38b1bd2 · 38b1bd2
1 parent a9bd905
commit 38b1bd2
Show file tree

Hide file tree

Showing 5 changed files with 67 additions and 84 deletions.
diff --git a/packages/pyright-internal/src/analyzer/service.ts b/packages/pyright-internal/src/analyzer/service.ts
@@ -295,13 +295,6 @@ export class AnalyzerService {
         const host = this._hostFactory();
         const configOptions = this._getConfigOptions(host, commandLineOptions);
 
-        if (configOptions.pythonPath) {
-            // Make sure we have default python environment set.
-            configOptions.ensureDefaultPythonVersion(host, this._console);
-        }
-
-        configOptions.ensureDefaultPythonPlatform(host, this._console);
-
         this._backgroundAnalysisProgram.setConfigOptions(configOptions);
 
         this._executionRootUri = configOptions.projectRoot;
@@ -652,6 +645,18 @@ export class AnalyzerService {
         // Ensure that if no command line or config options were applied, we have some defaults.
         this._ensureDefaultOptions(host, configOptions, projectRoot, executionRoot, commandLineOptions);
 
+        // Once we have defaults, we can then setup the execution environments. Execution enviroments
+        // inherit from the defaults.
+        if (configs) {
+            for (const config of configs) {
+                configOptions.setupExecutionEnvironments(
+                    config.configFileJsonObj,
+                    config.configFileDirUri,
+                    this.serviceProvider.console()
+                );
+            }
+        }
+
         return configOptions;
     }
 
@@ -794,6 +799,13 @@ export class AnalyzerService {
         if (commandLineOptions.configSettings.verboseOutput !== undefined) {
             configOptions.verboseOutput = commandLineOptions.configSettings.verboseOutput;
         }
+
+        // Ensure default python version and platform. A default should only be picked if
+        // there is a python path however.
+        if (configOptions.pythonPath) {
+            configOptions.ensureDefaultPythonVersion(host, this._console);
+        }
+        configOptions.ensureDefaultPythonPlatform(host, this._console);
     }
 
     private _applyLanguageServerOptions(
@@ -840,31 +852,17 @@ export class AnalyzerService {
         }
 
         if (commandLineOptions.extraPaths) {
-            const oldExtraPaths = configOptions.defaultExtraPaths ? [...configOptions.defaultExtraPaths] : [];
             configOptions.ensureDefaultExtraPaths(
                 this.fs,
                 commandLineOptions.autoSearchPaths ?? false,
                 commandLineOptions.extraPaths
             );
-
-            // Execution environments inherit the default extra paths, so we need to update them as well.
-            configOptions.executionEnvironments.forEach((env) => {
-                env.extraPaths = env.extraPaths.filter(
-                    (path) => !oldExtraPaths.some((oldPath) => oldPath.equals(path))
-                );
-                env.extraPaths.push(...configOptions.defaultExtraPaths!);
-            });
         }
 
         if (commandLineOptions.pythonVersion || commandLineOptions.pythonPlatform) {
             configOptions.defaultPythonVersion = commandLineOptions.pythonVersion ?? configOptions.defaultPythonVersion;
             configOptions.defaultPythonPlatform =
                 commandLineOptions.pythonPlatform ?? configOptions.defaultPythonPlatform;
-            // This should also override any of the execution environment settings.
-            configOptions.executionEnvironments.forEach((env) => {
-                env.pythonVersion = commandLineOptions.pythonVersion ?? env.pythonVersion;
-                env.pythonPlatform = commandLineOptions.pythonPlatform ?? env.pythonPlatform;
-            });
         }
 
         if (commandLineOptions.pythonPath) {

diff --git a/packages/pyright-internal/src/common/configOptions.ts b/packages/pyright-internal/src/common/configOptions.ts
@@ -1282,12 +1282,10 @@ export class ConfigOptions {
         }
 
         // Read the default "pythonVersion".
-        let configPythonVersion: PythonVersion | undefined = undefined;
         if (configObj.pythonVersion !== undefined) {
             if (typeof configObj.pythonVersion === 'string') {
                 const version = PythonVersion.fromString(configObj.pythonVersion);
                 if (version) {
-                    configPythonVersion = version;
                     this.defaultPythonVersion = version;
                 } else {
                     console.error(`Config "pythonVersion" field contains unsupported version.`);
@@ -1297,21 +1295,15 @@ export class ConfigOptions {
             }
         }
 
-        this.ensureDefaultPythonVersion(host, console);
-
         // Read the default "pythonPlatform".
-        let configPythonPlatform: string | undefined = undefined;
         if (configObj.pythonPlatform !== undefined) {
             if (typeof configObj.pythonPlatform !== 'string') {
                 console.error(`Config "pythonPlatform" field must contain a string.`);
             } else {
                 this.defaultPythonPlatform = configObj.pythonPlatform;
-                configPythonPlatform = configObj.pythonPlatform;
             }
         }
 
-        this.ensureDefaultPythonPlatform(host, console);
-
         // Read the "typeshedPath" setting.
         if (configObj.typeshedPath !== undefined) {
             if (typeof configObj.typeshedPath !== 'string') {
@@ -1381,35 +1373,6 @@ export class ConfigOptions {
             }
         }
 
-        // Read the "executionEnvironments" array. This should be done at the end
-        // after we've established default values.
-        if (configObj.executionEnvironments !== undefined) {
-            if (!Array.isArray(configObj.executionEnvironments)) {
-                console.error(`Config "executionEnvironments" field must contain an array.`);
-            } else {
-                this.executionEnvironments = [];
-
-                const execEnvironments = configObj.executionEnvironments as ExecutionEnvironment[];
-
-                execEnvironments.forEach((env, index) => {
-                    const execEnv = this._initExecutionEnvironmentFromJson(
-                        env,
-                        configDirUri,
-                        index,
-                        console,
-                        configRuleSet,
-                        configPythonVersion,
-                        configPythonPlatform,
-                        configExtraPaths
-                    );
-
-                    if (execEnv) {
-                        this.executionEnvironments.push(execEnv);
-                    }
-                });
-            }
-        }
-
         // Read the "autoImportCompletions" setting.
         if (configObj.autoImportCompletions !== undefined) {
             if (typeof configObj.autoImportCompletions !== 'boolean') {
@@ -1543,6 +1506,37 @@ export class ConfigOptions {
         }
     }
 
+    setupExecutionEnvironments(configObj: any, configDirUri: Uri, console: ConsoleInterface) {
+        // Read the "executionEnvironments" array. This should be done at the end
+        // after we've established default values.
+        if (configObj.executionEnvironments !== undefined) {
+            if (!Array.isArray(configObj.executionEnvironments)) {
+                console.error(`Config "executionEnvironments" field must contain an array.`);
+            } else {
+                this.executionEnvironments = [];
+
+                const execEnvironments = configObj.executionEnvironments as ExecutionEnvironment[];
+
+                execEnvironments.forEach((env, index) => {
+                    const execEnv = this._initExecutionEnvironmentFromJson(
+                        env,
+                        configDirUri,
+                        index,
+                        console,
+                        this.diagnosticRuleSet,
+                        this.defaultPythonVersion,
+                        this.defaultPythonPlatform,
+                        this.defaultExtraPaths || []
+                    );
+
+                    if (execEnv) {
+                        this.executionEnvironments.push(execEnv);
+                    }
+                });
+            }
+        }
+    }
+
     private _getEnvironmentName(): string {
         return this.pythonEnvironmentName || this.pythonPath?.toString() || 'python';
     }

diff --git a/packages/pyright-internal/src/tests/config.test.ts b/packages/pyright-internal/src/tests/config.test.ts
@@ -15,7 +15,6 @@ import { CommandLineOptions, DiagnosticSeverityOverrides } from '../common/comma
 import { ConfigOptions, ExecutionEnvironment, getStandardDiagnosticRuleSet } from '../common/configOptions';
 import { ConsoleInterface, NullConsole } from '../common/console';
 import { TaskListPriority } from '../common/diagnostic';
-import { NoAccessHost } from '../common/host';
 import { combinePaths, normalizePath, normalizeSlashes } from '../common/pathUtils';
 import { pythonVersion3_9 } from '../common/pythonVersion';
 import { RealTempFile, createFromRealFileSystem } from '../common/realFileSystem';
@@ -202,27 +201,18 @@ test('FindExecEnv1', () => {
 });
 
 test('PythonPlatform', () => {
-    const cwd = UriEx.file(normalizePath(process.cwd()));
-
-    const configOptions = new ConfigOptions(cwd);
-
-    const json = JSON.parse(`{
-        "executionEnvironments" : [
-        {
-            "root": ".",
-            "pythonVersion" : "3.7",
-            "pythonPlatform" : "platform",
-            "extraPaths" : []
-    }]}`);
-
-    const fs = new TestFileSystem(/* ignoreCase */ false);
     const nullConsole = new NullConsole();
+    const service = createAnalyzer(nullConsole);
+    const cwd = Uri.file(
+        normalizePath(combinePaths(process.cwd(), 'src/tests/samples/project_with_pyproject_toml_platform')),
+        service.serviceProvider
+    );
+    const commandLineOptions = new CommandLineOptions(cwd.getFilePath(), /* fromLanguageServer */ false);
+    service.setOptions(commandLineOptions);
 
-    const sp = createServiceProvider(fs, nullConsole);
-    configOptions.initializeFromJson(json, cwd, sp, new NoAccessHost());
-
-    const env = configOptions.executionEnvironments[0];
-    assert.strictEqual(env.pythonPlatform, 'platform');
+    const configOptions = service.test_getConfigOptions(commandLineOptions);
+    assert.ok(configOptions.executionEnvironments[0]);
+    assert.equal(configOptions.executionEnvironments[0].pythonPlatform, 'platform');
 });
 
 test('AutoSearchPathsOn', () => {

diff --git a/packages/pyright-internal/src/tests/harness/fourslash/testState.ts b/packages/pyright-internal/src/tests/harness/fourslash/testState.ts
@@ -181,13 +181,10 @@ export class TestState {
         const configOptions = this._convertGlobalOptionsToConfigOptions(vfsInfo.projectRoot, mountPaths);
 
         if (this.rawConfigJson) {
+            const configDirUri = Uri.file(projectRoot, this.serviceProvider);
             configOptions.initializeTypeCheckingMode('standard');
-            configOptions.initializeFromJson(
-                this.rawConfigJson,
-                Uri.file(projectRoot, this.serviceProvider),
-                this.serviceProvider,
-                testAccessHost
-            );
+            configOptions.initializeFromJson(this.rawConfigJson, configDirUri, this.serviceProvider, testAccessHost);
+            configOptions.setupExecutionEnvironments(this.rawConfigJson, configDirUri, this.serviceProvider.console());
             this._applyTestConfigOptions(configOptions);
         }
 

diff --git a/...es/pyright-internal/src/tests/samples/project_with_pyproject_toml_platform/pyproject.toml b/...es/pyright-internal/src/tests/samples/project_with_pyproject_toml_platform/pyproject.toml
@@ -0,0 +1,4 @@
+[tool.pyright]
+executionEnvironments = [
+    { python = "3.7", pythonPlatform = "platform" },
+]