Point release with fix for Issue 10250 (#10254)

DonJayamanne · web-flow · commit 9bcce5e270c7 · 2020-02-21T10:35:33.000-08:00
* Fix to return env variables of interpreter that is not current interpreter (#10251) For #10250 * Bug fix * Update change log * Fix date * Oops * Fix linter
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,65 @@
 # Changelog
 
+## 2020.2.3 (21 February 2020)
+
+### Fixes
+
+1. Ensure to correctly return env variables of the activated interpreter, when dealing with non-workspace interpreters.
+   ([#10250](https://github.com/Microsoft/vscode-python/issues/10250))
+
+### Thanks
+
+Thanks to the following projects which we fully rely on to provide some of
+our features:
+
+-   [isort](https://pypi.org/project/isort/)
+-   [jedi](https://pypi.org/project/jedi/)
+    and [parso](https://pypi.org/project/parso/)
+-   [Microsoft Python Language Server](https://github.com/microsoft/python-language-server)
+-   [ptvsd](https://pypi.org/project/ptvsd/)
+-   [exuberant ctags](http://ctags.sourceforge.net/) (user-installed)
+-   [rope](https://pypi.org/project/rope/) (user-installed)
+
+Also thanks to the various projects we provide integrations with which help
+make this extension useful:
+
+-   Debugging support:
+    [Django](https://pypi.org/project/Django/),
+    [Flask](https://pypi.org/project/Flask/),
+    [gevent](https://pypi.org/project/gevent/),
+    [Jinja](https://pypi.org/project/Jinja/),
+    [Pyramid](https://pypi.org/project/pyramid/),
+    [PySpark](https://pypi.org/project/pyspark/),
+    [Scrapy](https://pypi.org/project/Scrapy/),
+    [Watson](https://pypi.org/project/Watson/)
+-   Formatting:
+    [autopep8](https://pypi.org/project/autopep8/),
+    [black](https://pypi.org/project/black/),
+    [yapf](https://pypi.org/project/yapf/)
+-   Interpreter support:
+    [conda](https://conda.io/),
+    [direnv](https://direnv.net/),
+    [pipenv](https://pypi.org/project/pipenv/),
+    [pyenv](https://github.com/pyenv/pyenv),
+    [venv](https://docs.python.org/3/library/venv.html#module-venv),
+    [virtualenv](https://pypi.org/project/virtualenv/)
+-   Linting:
+    [bandit](https://pypi.org/project/bandit/),
+    [flake8](https://pypi.org/project/flake8/),
+    [mypy](https://pypi.org/project/mypy/),
+    [prospector](https://pypi.org/project/prospector/),
+    [pylint](https://pypi.org/project/pylint/),
+    [pydocstyle](https://pypi.org/project/pydocstyle/),
+    [pylama](https://pypi.org/project/pylama/)
+-   Testing:
+    [nose](https://pypi.org/project/nose/),
+    [pytest](https://pypi.org/project/pytest/),
+    [unittest](https://docs.python.org/3/library/unittest.html#module-unittest)
+
+And finally thanks to the [Python](https://www.python.org/) development team and
+community for creating a fantastic programming language and community to be a
+part of!
+
 ## 2020.2.2 (19 February 2020)
 
 ### Fixes
diff --git a/package.json b/package.json
@@ -2,7 +2,7 @@
     "name": "python",
     "displayName": "Python",
     "description": "Linting, Debugging (multi-threaded, remote), Intellisense, Jupyter Notebooks, code formatting, refactoring, unit tests, snippets, and more.",
-    "version": "2020.2.2",
+    "version": "2020.2.3",
     "languageServerVersion": "0.5.30",
     "publisher": "ms-python",
     "author": {
diff --git a/src/client/common/logger.ts b/src/client/common/logger.ts
@@ -285,7 +285,7 @@ function trace(message: string, options: LogOptions = LogOptions.None, logLevel?
             // tslint:disable-next-line:no-any
             function writeToLog(elapsedTime: number, returnValue?: any, ex?: Error) {
                 const messagesToLog = [message];
-                messagesToLog.push(`Class name = ${className}, completed in ${elapsedTime}ms`);
+                messagesToLog.push(`Class name = ${className}, completed in ${elapsedTime}ms, has a ${returnValue ? 'truthy' : 'falsy'} return value`);
                 if ((options && LogOptions.Arguments) === LogOptions.Arguments) {
                     messagesToLog.push(argsToLogString(args));
                 }
diff --git a/src/client/interpreter/activation/service.ts b/src/client/interpreter/activation/service.ts
@@ -6,19 +6,20 @@ import '../../common/extensions';
 import { inject, injectable } from 'inversify';
 import * as path from 'path';
 
+import { IWorkspaceService } from '../../common/application/types';
 import { PYTHON_WARNINGS } from '../../common/constants';
 import { LogOptions, traceDecorators, traceError, traceVerbose } from '../../common/logger';
 import { IPlatformService } from '../../common/platform/types';
 import { IProcessServiceFactory } from '../../common/process/types';
 import { ITerminalHelper, TerminalShellType } from '../../common/terminal/types';
 import { ICurrentProcess, IDisposable, Resource } from '../../common/types';
-import { cacheResourceSpecificInterpreterData, clearCachedResourceSpecificIngterpreterData } from '../../common/utils/decorators';
+import { InMemoryCache } from '../../common/utils/cacheUtils';
 import { OSType } from '../../common/utils/platform';
 import { IEnvironmentVariablesProvider } from '../../common/variables/types';
 import { EXTENSION_ROOT_DIR } from '../../constants';
 import { captureTelemetry, sendTelemetryEvent } from '../../telemetry';
 import { EventName } from '../../telemetry/constants';
-import { PythonInterpreter } from '../contracts';
+import { IInterpreterService, PythonInterpreter } from '../contracts';
 import { IEnvironmentActivationService } from './types';
 
 const getEnvironmentPrefix = 'e8b39361-0157-4923-80e1-22d70d46dee6';
@@ -36,23 +37,46 @@ const defaultShells = {
 @injectable()
 export class EnvironmentActivationService implements IEnvironmentActivationService, IDisposable {
     private readonly disposables: IDisposable[] = [];
+    private readonly activatedEnvVariablesCache = new Map<string, InMemoryCache<NodeJS.ProcessEnv | undefined>>();
     constructor(
         @inject(ITerminalHelper) private readonly helper: ITerminalHelper,
         @inject(IPlatformService) private readonly platform: IPlatformService,
         @inject(IProcessServiceFactory) private processServiceFactory: IProcessServiceFactory,
         @inject(ICurrentProcess) private currentProcess: ICurrentProcess,
+        @inject(IWorkspaceService) private workspace: IWorkspaceService,
+        @inject(IInterpreterService) private interpreterService: IInterpreterService,
         @inject(IEnvironmentVariablesProvider) private readonly envVarsService: IEnvironmentVariablesProvider
     ) {
-        this.envVarsService.onDidEnvironmentVariablesChange(this.onDidEnvironmentVariablesChange, this, this.disposables);
+        this.envVarsService.onDidEnvironmentVariablesChange(() => this.activatedEnvVariablesCache.clear(), this, this.disposables);
+
+        this.interpreterService.onDidChangeInterpreter(() => this.activatedEnvVariablesCache.clear(), this, this.disposables);
     }
 
     public dispose(): void {
         this.disposables.forEach(d => d.dispose());
     }
     @traceDecorators.verbose('getActivatedEnvironmentVariables', LogOptions.Arguments)
     @captureTelemetry(EventName.PYTHON_INTERPRETER_ACTIVATION_ENVIRONMENT_VARIABLES, { failed: false }, true)
-    @cacheResourceSpecificInterpreterData('ActivatedEnvironmentVariables', cacheDuration)
     public async getActivatedEnvironmentVariables(resource: Resource, interpreter?: PythonInterpreter, allowExceptions?: boolean): Promise<NodeJS.ProcessEnv | undefined> {
+        // Cache key = resource + interpreter.
+        const workspaceKey = this.workspace.getWorkspaceFolderIdentifier(resource);
+        const interpreterPath = this.platform.isWindows ? interpreter?.path.toLowerCase() : interpreter?.path;
+        const cacheKey = `${workspaceKey}_${interpreterPath}`;
+
+        if (this.activatedEnvVariablesCache.get(cacheKey)?.hasData) {
+            return this.activatedEnvVariablesCache.get(cacheKey)!.data;
+        }
+
+        // Cache only if successful, else keep trying & failing if necessary.
+        const cache = new InMemoryCache<NodeJS.ProcessEnv | undefined>(cacheDuration, '');
+        return this.getActivatedEnvironmentVariablesImpl(resource, interpreter, allowExceptions).then(vars => {
+            cache.data = vars;
+            this.activatedEnvVariablesCache.set(cacheKey, cache);
+            return vars;
+        });
+    }
+
+    public async getActivatedEnvironmentVariablesImpl(resource: Resource, interpreter?: PythonInterpreter, allowExceptions?: boolean): Promise<NodeJS.ProcessEnv | undefined> {
         const shellInfo = defaultShells[this.platform.osType];
         if (!shellInfo) {
             return;
@@ -115,9 +139,7 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi
             }
         }
     }
-    protected onDidEnvironmentVariablesChange(affectedResource: Resource) {
-        clearCachedResourceSpecificIngterpreterData('ActivatedEnvironmentVariables', affectedResource);
-    }
+
     protected fixActivationCommands(commands: string[]): string[] {
         // Replace 'source ' with '. ' as that works in shell exec
         return commands.map(cmd => cmd.replace(/^source\s+/, '. '));
diff --git a/src/test/interpreters/activation/service.unit.test.ts b/src/test/interpreters/activation/service.unit.test.ts
@@ -7,8 +7,9 @@ import { EOL } from 'os';
 import * as path from 'path';
 import { SemVer } from 'semver';
 import { anything, capture, instance, mock, verify, when } from 'ts-mockito';
-import * as typemoq from 'typemoq';
-import { EventEmitter, Uri, workspace as workspaceType, WorkspaceConfiguration } from 'vscode';
+import { EventEmitter, Uri } from 'vscode';
+import { IWorkspaceService } from '../../../client/common/application/types';
+import { WorkspaceService } from '../../../client/common/application/workspace';
 import { PlatformService } from '../../../client/common/platform/platformService';
 import { IPlatformService } from '../../../client/common/platform/types';
 import { CurrentProcess } from '../../../client/common/process/currentProcess';
@@ -18,15 +19,14 @@ import { IProcessService, IProcessServiceFactory } from '../../../client/common/
 import { TerminalHelper } from '../../../client/common/terminal/helper';
 import { ITerminalHelper } from '../../../client/common/terminal/types';
 import { ICurrentProcess } from '../../../client/common/types';
-import { clearCache } from '../../../client/common/utils/cacheUtils';
 import { getNamesAndValues } from '../../../client/common/utils/enum';
 import { Architecture, OSType } from '../../../client/common/utils/platform';
 import { EnvironmentVariablesProvider } from '../../../client/common/variables/environmentVariablesProvider';
 import { IEnvironmentVariablesProvider } from '../../../client/common/variables/types';
 import { EXTENSION_ROOT_DIR } from '../../../client/constants';
 import { EnvironmentActivationService } from '../../../client/interpreter/activation/service';
-import { InterpreterType, PythonInterpreter } from '../../../client/interpreter/contracts';
-import { mockedVSCodeNamespaces } from '../../vscode-mock';
+import { IInterpreterService, InterpreterType, PythonInterpreter } from '../../../client/interpreter/contracts';
+import { InterpreterService } from '../../../client/interpreter/interpreterService';
 
 const getEnvironmentPrefix = 'e8b39361-0157-4923-80e1-22d70d46dee6';
 const defaultShells = {
@@ -45,8 +45,10 @@ suite('Interpreters Activation - Python Environment Variables', () => {
     let processService: IProcessService;
     let currentProcess: ICurrentProcess;
     let envVarsService: IEnvironmentVariablesProvider;
-    let workspace: typemoq.IMock<typeof workspaceType>;
-
+    let workspace: IWorkspaceService;
+    let interpreterService: IInterpreterService;
+    let onDidChangeEnvVariables: EventEmitter<Uri | undefined>;
+    let onDidChangeInterpreter: EventEmitter<void>;
     const pythonInterpreter: PythonInterpreter = {
         path: '/foo/bar/python.exe',
         version: new SemVer('3.6.6-final'),
@@ -63,21 +65,22 @@ suite('Interpreters Activation - Python Environment Variables', () => {
         processService = mock(ProcessService);
         currentProcess = mock(CurrentProcess);
         envVarsService = mock(EnvironmentVariablesProvider);
-        workspace = mockedVSCodeNamespaces.workspace!;
-        when(envVarsService.onDidEnvironmentVariablesChange).thenReturn(new EventEmitter<Uri | undefined>().event);
-        service = new EnvironmentActivationService(instance(helper), instance(platform), instance(processServiceFactory), instance(currentProcess), instance(envVarsService));
-
-        const cfg = typemoq.Mock.ofType<WorkspaceConfiguration>();
-        workspace.setup(w => w.getConfiguration(typemoq.It.isValue('python'), typemoq.It.isAny())).returns(() => cfg.object);
-        workspace.setup(w => w.workspaceFolders).returns(() => []);
-        cfg.setup(c => c.inspect(typemoq.It.isValue('pythonPath'))).returns(() => {
-            return { globalValue: 'GlobalValuepython' } as any;
-        });
-        clearCache();
+        interpreterService = mock(InterpreterService);
+        workspace = mock(WorkspaceService);
+        onDidChangeEnvVariables = new EventEmitter<Uri | undefined>();
+        onDidChangeInterpreter = new EventEmitter<void>();
+        when(envVarsService.onDidEnvironmentVariablesChange).thenReturn(onDidChangeEnvVariables.event);
+        when(interpreterService.onDidChangeInterpreter).thenReturn(onDidChangeInterpreter.event);
+        service = new EnvironmentActivationService(
+            instance(helper),
+            instance(platform),
+            instance(processServiceFactory),
+            instance(currentProcess),
+            instance(workspace),
+            instance(interpreterService),
+            instance(envVarsService)
+        );
     }
-    teardown(() => {
-        mockedVSCodeNamespaces.workspace!.reset();
-    });
 
     function title(resource?: Uri, interpreter?: PythonInterpreter) {
         return `${resource ? 'With a resource' : 'Without a resource'}${interpreter ? ' and an interpreter' : ''}`;
@@ -218,6 +221,64 @@ suite('Interpreters Activation - Python Environment Variables', () => {
                             verify(envVarsService.getEnvironmentVariables(resource)).once();
                             verify(processService.shellExec(anything(), anything())).once();
                         });
+                        test('Cache Variables', async () => {
+                            const cmd = ['1', '2'];
+                            const varsFromEnv = { one: '11', two: '22', HELLO: 'xxx' };
+                            const stdout = `${getEnvironmentPrefix}${EOL}${JSON.stringify(varsFromEnv)}`;
+                            when(platform.osType).thenReturn(osType.value);
+                            when(helper.getEnvironmentActivationShellCommands(resource, anything(), interpreter)).thenResolve(cmd);
+                            when(processServiceFactory.create(resource)).thenResolve(instance(processService));
+                            when(envVarsService.getEnvironmentVariables(resource)).thenResolve({});
+                            when(processService.shellExec(anything(), anything())).thenResolve({ stdout: stdout });
+
+                            const env = await service.getActivatedEnvironmentVariables(resource, interpreter);
+                            const env2 = await service.getActivatedEnvironmentVariables(resource, interpreter);
+                            const env3 = await service.getActivatedEnvironmentVariables(resource, interpreter);
+
+                            expect(env).to.deep.equal(varsFromEnv);
+                            // All same objects.
+                            expect(env)
+                                .to.equal(env2)
+                                .to.equal(env3);
+
+                            // All methods invoked only once.
+                            verify(helper.getEnvironmentActivationShellCommands(resource, anything(), interpreter)).once();
+                            verify(processServiceFactory.create(resource)).once();
+                            verify(envVarsService.getEnvironmentVariables(resource)).once();
+                            verify(processService.shellExec(anything(), anything())).once();
+                        });
+                        async function testClearingCache(bustCache: Function) {
+                            const cmd = ['1', '2'];
+                            const varsFromEnv = { one: '11', two: '22', HELLO: 'xxx' };
+                            const stdout = `${getEnvironmentPrefix}${EOL}${JSON.stringify(varsFromEnv)}`;
+                            when(platform.osType).thenReturn(osType.value);
+                            when(helper.getEnvironmentActivationShellCommands(resource, anything(), interpreter)).thenResolve(cmd);
+                            when(processServiceFactory.create(resource)).thenResolve(instance(processService));
+                            when(envVarsService.getEnvironmentVariables(resource)).thenResolve({});
+                            when(processService.shellExec(anything(), anything())).thenResolve({ stdout: stdout });
+
+                            const env = await service.getActivatedEnvironmentVariables(resource, interpreter);
+                            bustCache();
+                            const env2 = await service.getActivatedEnvironmentVariables(resource, interpreter);
+
+                            expect(env).to.deep.equal(varsFromEnv);
+                            // Objects are different (not same reference).
+                            expect(env).to.not.equal(env2);
+                            // However variables are the same.
+                            expect(env).to.deep.equal(env2);
+
+                            // All methods invoked twice as cache was blown.
+                            verify(helper.getEnvironmentActivationShellCommands(resource, anything(), interpreter)).twice();
+                            verify(processServiceFactory.create(resource)).twice();
+                            verify(envVarsService.getEnvironmentVariables(resource)).twice();
+                            verify(processService.shellExec(anything(), anything())).twice();
+                        }
+                        test('Cache Variables get cleared when changing interpreter', async () => {
+                            await testClearingCache(onDidChangeInterpreter.fire.bind(onDidChangeInterpreter));
+                        });
+                        test('Cache Variables get cleared when changing env variables file', async () => {
+                            await testClearingCache(onDidChangeEnvVariables.fire.bind(onDidChangeEnvVariables));
+                        });
                     });
                 });
             });

Original file line number	Diff line number	Diff line change
`@@ -285,7 +285,7 @@ function trace(message: string, options: LogOptions = LogOptions.None, logLevel?`
`285`	`285`	`// tslint:disable-next-line:no-any`
`286`	`286`	`function writeToLog(elapsedTime: number, returnValue?: any, ex?: Error) {`
`287`	`287`	`const messagesToLog = [message];`
`288`		- messagesToLog.push(`Class name = ${className}, completed in ${elapsedTime}ms`);
	`288`	+ messagesToLog.push(`Class name = ${className}, completed in ${elapsedTime}ms, has a ${returnValue ? 'truthy' : 'falsy'} return value`);
`289`	`289`	`if ((options && LogOptions.Arguments) === LogOptions.Arguments) {`
`290`	`290`	`messagesToLog.push(argsToLogString(args));`
`291`	`291`	`}`