Do not display environments in the list for which running python binary fails (#15449)

* Do not display environments in the list for which running python binary fails

* Code reviews

Author: Kartik Raj

src/client/pythonEnvironments/base/locator.ts

@@ -22,8 +22,9 @@ export type PythonEnvUpdatedEvent = {
     old?: PythonEnvInfo;
     /**
      * The env info that replaces the old info.
+     * Update is sent as `undefined` if we find out that the environment is no longer valid.
      */
-    update: PythonEnvInfo;
+    update: PythonEnvInfo | undefined;
 };
 
 /**

src/client/pythonEnvironments/base/locatorUtils.ts

@@ -2,6 +2,7 @@
 // Licensed under the MIT License.
 
 import { Uri } from 'vscode';
+import { traceVerbose } from '../../common/logger';
 import { createDeferred } from '../../common/utils/async';
 import { getURIFilter } from '../../common/utils/misc';
 import { IDisposable } from '../../common/utils/resourceLifecycle';
@@ -75,7 +76,7 @@ function getSearchLocationFilters(query: PythonLocatorQuery): ((u: Uri) => boole
  * This includes applying any received updates.
  */
 export async function getEnvs(iterator: IPythonEnvsIterator): Promise<PythonEnvInfo[]> {
-    const envs: PythonEnvInfo[] = [];
+    const envs: (PythonEnvInfo | undefined)[] = [];
 
     const updatesDone = createDeferred<void>();
     if (iterator.onUpdated === undefined) {
@@ -87,6 +88,12 @@ export async function getEnvs(iterator: IPythonEnvsIterator): Promise<PythonEnvI
                 listener.dispose();
             } else {
                 const { index, update } = event;
+                if (envs[index] === undefined) {
+                    const json = JSON.stringify(update);
+                    traceVerbose(
+                        `Updates sent for an env which was classified as invalid earlier, currently not expected, ${json}`,
+                    );
+                }
                 // We don't worry about if envs[index] is set already.
                 envs[index] = update;
             }
@@ -103,7 +110,8 @@ export async function getEnvs(iterator: IPythonEnvsIterator): Promise<PythonEnvI
     }
     await updatesDone.promise;
 
-    return envs;
+    // Do not return invalid environments
+    return envs.filter((e) => e !== undefined).map((e) => e!);
 }
 
 /**
@@ -175,7 +183,7 @@ export async function resolveEnvFromIterator(
         listener = iterator.onUpdated((event: PythonEnvUpdatedEvent | null) => {
             if (event === null) {
                 done.resolve();
-            } else if (matchEnv(event.update)) {
+            } else if (!event.update || matchEnv(event.update)) {
                 resolved = event.update;
             }
         });

src/client/pythonEnvironments/base/locators/composite/environmentsReducer.ts

@@ -56,6 +56,10 @@ async function* iterEnvsIterator(
                 state.done = true;
                 checkIfFinishedAndNotify(state, didUpdate);
                 listener.dispose();
+            } else if (event.update === undefined) {
+                throw new Error(
+                    'Unsupported behavior: `undefined` environment updates are not supported from downstream locators in reducer',
+                );
             } else if (seen[event.index] !== undefined) {
                 state.pending += 1;
                 resolveDifferencesInBackground(event.index, event.update, state, didUpdate, seen).ignoreErrors();

src/client/pythonEnvironments/base/locators/composite/environmentsResolver.ts

@@ -30,11 +30,11 @@ export class PythonEnvsResolver implements ILocator {
         if (!environment) {
             return undefined;
         }
-        const interpreterInfo = await this.environmentInfoService.getEnvironmentInfo(environment.executable.filename);
-        if (!interpreterInfo) {
+        const info = await this.environmentInfoService.getEnvironmentInfo(environment.executable.filename);
+        if (!info) {
             return undefined;
         }
-        return getResolvedEnv(interpreterInfo, environment);
+        return getResolvedEnv(info, environment);
     }
 
     public iterEnvs(query?: PythonLocatorQuery): IPythonEnvsIterator {
@@ -61,6 +61,10 @@ export class PythonEnvsResolver implements ILocator {
                     state.done = true;
                     checkIfFinishedAndNotify(state, didUpdate);
                     listener.dispose();
+                } else if (event.update === undefined) {
+                    throw new Error(
+                        'Unsupported behavior: `undefined` environment updates are not supported from downstream locators in resolver',
+                    );
                 } else if (seen[event.index] !== undefined) {
                     seen[event.index] = event.update;
                     this.resolveInBackground(event.index, state, didUpdate, seen).ignoreErrors();
@@ -97,14 +101,15 @@ export class PythonEnvsResolver implements ILocator {
         state.pending += 1;
         // It's essential we increment the pending call count before any asynchronus calls in this method.
         // We want this to be run even when `resolveInBackground` is called in background.
-        const interpreterInfo = await this.environmentInfoService.getEnvironmentInfo(
-            seen[envIndex].executable.filename,
-        );
-        if (interpreterInfo) {
-            const resolvedEnv = getResolvedEnv(interpreterInfo, seen[envIndex]);
-            const old = seen[envIndex];
+        const info = await this.environmentInfoService.getEnvironmentInfo(seen[envIndex].executable.filename);
+        const old = seen[envIndex];
+        if (info) {
+            const resolvedEnv = getResolvedEnv(info, seen[envIndex]);
             seen[envIndex] = resolvedEnv;
             didUpdate.fire({ old, index: envIndex, update: resolvedEnv });
+        } else {
+            // Send update that the environment is not valid.
+            didUpdate.fire({ old, index: envIndex, update: undefined });
         }
         state.pending -= 1;
         checkIfFinishedAndNotify(state, didUpdate);

src/test/pythonEnvironments/base/locators/composite/environmentsResolver.unit.test.ts

@@ -14,6 +14,7 @@ import { PythonEnvInfo, PythonEnvKind } from '../../../../../client/pythonEnviro
 import { parseVersion } from '../../../../../client/pythonEnvironments/base/info/pythonVersion';
 import { PythonEnvUpdatedEvent } from '../../../../../client/pythonEnvironments/base/locator';
 import { PythonEnvsResolver } from '../../../../../client/pythonEnvironments/base/locators/composite/environmentsResolver';
+import { getEnvs as getEnvsWithUpdates } from '../../../../../client/pythonEnvironments/base/locatorUtils';
 import { PythonEnvsChangedEvent } from '../../../../../client/pythonEnvironments/base/watcher';
 import * as ExternalDep from '../../../../../client/pythonEnvironments/common/externalDependencies';
 import { EnvironmentInfoService } from '../../../../../client/pythonEnvironments/info/environmentInfoService';
@@ -113,6 +114,30 @@ suite('Python envs locator - Environments Resolver', () => {
             assert.deepEqual(onUpdatedEvents, expectedUpdates);
         });
 
+        test('If fetching interpreter info fails, it is not reported in the final list of envs', async () => {
+            // Arrange
+            stubShellExec.returns(
+                new Promise<ExecutionResult<string>>((resolve) => {
+                    resolve({
+                        stderr: 'Kaboom',
+                        stdout: '',
+                    });
+                }),
+            );
+            const env1 = createNamedEnv('env1', '3.5.12b1', PythonEnvKind.Unknown, path.join('path', 'to', 'exec1'));
+            const env2 = createNamedEnv('env2', '3.8.1', PythonEnvKind.Unknown, path.join('path', 'to', 'exec2'));
+            const environmentsToBeIterated = [env1, env2];
+            const parentLocator = new SimpleLocator(environmentsToBeIterated);
+            const resolver = new PythonEnvsResolver(parentLocator, envInfoService, () => true);
+
+            // Act
+            const iterator = resolver.iterEnvs();
+            const envs = await getEnvsWithUpdates(iterator);
+
+            // Assert
+            assert.deepEqual(envs, []);
+        });
+
         test('Updates to environments from the incoming iterator are sent correctly followed by the null event', async () => {
             // Arrange
             const env = createNamedEnv('env1', '3.8', PythonEnvKind.Unknown, path.join('path', 'to', 'exec'));
@@ -254,7 +279,7 @@ suite('Python envs locator - Environments Resolver', () => {
             assert.deepEqual(expected, createExpectedEnvInfo(resolvedEnvReturnedByReducer));
         });
 
-        test('If the parent locator resolves environment, but fetching interpreter info returns undefined, return undefined', async () => {
+        test('If the parent locator resolves environment, but running interpreter info throws error, return undefined', async () => {
             stubShellExec.returns(
                 new Promise<ExecutionResult<string>>((_resolve, reject) => {
                     reject();
@@ -282,6 +307,37 @@ suite('Python envs locator - Environments Resolver', () => {
             assert.deepEqual(expected, undefined);
         });
 
+        test('If fetching interpreter info fails with stderr, return undefined', async () => {
+            stubShellExec.returns(
+                new Promise<ExecutionResult<string>>((resolve) => {
+                    resolve({
+                        stderr: 'Kaboom',
+                        stdout: '',
+                    });
+                }),
+            );
+            const env = createNamedEnv('env1', '3.8', PythonEnvKind.Unknown, path.join('path', 'to', 'exec'));
+            const resolvedEnvReturnedByReducer = createNamedEnv(
+                'env1',
+                '3.8.1',
+                PythonEnvKind.Conda,
+                'resolved/path/to/exec',
+            );
+            const parentLocator = new SimpleLocator([], {
+                resolve: async (e: PythonEnvInfo) => {
+                    if (e === env) {
+                        return resolvedEnvReturnedByReducer;
+                    }
+                    throw new Error('Incorrect environment sent to the resolver');
+                },
+            });
+            const resolver = new PythonEnvsResolver(parentLocator, envInfoService, () => true);
+
+            const expected = await resolver.resolveEnv(env);
+
+            assert.deepEqual(expected, undefined);
+        });
+
         test("If the parent locator isn't able to resolve environment, return undefined", async () => {
             const env = createNamedEnv('env', '3.8', PythonEnvKind.Unknown, path.join('path', 'to', 'exec'));
             const parentLocator = new SimpleLocator([], {