Refactor parsing of environment variables (after 439) (#466)

Refactor how environment variables are parsed and used in tools, language server and debugger

Fixes #316 (PYTHONPATH not used as extra path for auto complete)
Fixes #183 (changes to .env file requires restarts of vscode)
Fixes #436 (Path variables not inherited when debugging)
Fixes #435 (Virtual environment name is same as the default environment file)

Author: DonJayamanne

gulpfile.js

@@ -309,7 +309,7 @@ function getFilesToProcess(options) {
     // If we need only modified files, then filter the glob.
     if (options && options.mode === 'changes') {
         return gulp.src(all, gulpSrcOptions)
-            .pipe(gitmodified('M', 'A', 'D', 'R', 'C', 'U', '??'));
+            .pipe(gitmodified(['M', 'A', 'D', 'R', 'C', 'U', '??']));
     }
 
     if (options && options.mode === 'staged') {

package-lock.json

@@ -1559,6 +1559,7 @@
 		"@types/mocha": "^2.2.43",
 		"@types/node": "^6.0.40",
 		"@types/semver": "^5.4.0",
+		"@types/shortid": "0.0.29",
 		"@types/sinon": "^2.3.2",
 		"@types/uuid": "^3.3.27",
 		"@types/winreg": "^1.2.30",
@@ -1578,6 +1579,7 @@
 		"mocha": "^2.3.3",
 		"relative": "^3.0.2",
 		"retyped-diff-match-patch-tsd-ambient": "^1.0.0-0",
+		"shortid": "^2.2.8",
 		"sinon": "^2.3.6",
 		"tslint": "^5.7.0",
 		"tslint-eslint-rules": "^4.1.1",

package.json

@@ -0,0 +1,47 @@
+import * as _ from 'lodash';
+
+type AsyncVoidAction = (...params: {}[]) => Promise<void>;
+type VoidAction = (...params: {}[]) => void;
+
+/**
+ * Debounces a function execution. Function must return either a void or a promise that resolves to a void.
+ * @export
+ * @param {number} [wait] Wait time.
+ * @returns void
+ */
+export function debounce(wait?: number) {
+    // tslint:disable-next-line:no-any no-function-expression
+    return function (_target: any, _propertyName: string, descriptor: TypedPropertyDescriptor<VoidAction> | TypedPropertyDescriptor<AsyncVoidAction>) {
+        const originalMethod = descriptor.value!;
+        // tslint:disable-next-line:no-invalid-this no-any
+        (descriptor as any).value = _.debounce(function () { return originalMethod.apply(this, arguments); }, wait);
+    };
+}
+
+/**
+ * Swallows exceptions thrown by a function. Function must return either a void or a promise that resolves to a void.
+ * @export
+ * @param {string} [scopeName] Scope for the error message to be logged along with the error.
+ * @returns void
+ */
+export function swallowExceptions(scopeName: string) {
+    // tslint:disable-next-line:no-any no-function-expression
+    return function (_target: any, propertyName: string, descriptor: TypedPropertyDescriptor<VoidAction> | TypedPropertyDescriptor<AsyncVoidAction>) {
+        const originalMethod = descriptor.value!;
+        const errorMessage = `Python Extension (Error in ${scopeName}, method:${propertyName}):`;
+        // tslint:disable-next-line:no-any no-function-expression
+        descriptor.value = function (...args: any[]) {
+            try {
+                // tslint:disable-next-line:no-invalid-this no-use-before-declare no-unsafe-any
+                const result = originalMethod.apply(this, args);
+
+                // If method being wrapped returns a promise then wait and swallow errors.
+                if (result && typeof result.then === 'function' && typeof result.catch === 'function') {
+                    return (result as Promise<void>).catch(error => console.error(errorMessage, error));
+                }
+            } catch (error) {
+                console.error(errorMessage, error);
+            }
+        };
+    };
+}

src/client/common/decorators.ts

@@ -0,0 +1,10 @@
+import { injectable } from 'inversify';
+import { ICurrentProcess } from '../types';
+import { EnvironmentVariables } from '../variables/types';
+
+@injectable()
+export class CurrentProcess implements ICurrentProcess {
+    public get env(): EnvironmentVariables {
+        return process.env;
+    }
+}

src/client/common/process/currentProcess.ts

@@ -14,7 +14,7 @@ export class PythonExecutionFactory implements IPythonExecutionFactory {
         @inject(IEnvironmentVariablesProvider) private envVarsService: IEnvironmentVariablesProvider) { }
     public async create(resource?: Uri): Promise<IPythonExecutionService> {
         const settings = PythonSettings.getInstance(resource);
-        return this.envVarsService.getEnvironmentVariables(true, resource)
+        return this.envVarsService.getEnvironmentVariables(resource)
             .then(customEnvVars => {
                 return new PythonExecutionService(this.procService, settings.pythonPath, customEnvVars);
             });

src/client/common/process/pythonExecutionFactory.ts

@@ -12,9 +12,10 @@ import { IS_64_BIT, IS_WINDOWS } from './platform/constants';
 import { PathUtils } from './platform/pathUtils';
 import { RegistryImplementation } from './platform/registry';
 import { IRegistry } from './platform/types';
+import { CurrentProcess } from './process/currentProcess';
 import { TerminalService } from './terminal/service';
 import { ITerminalService } from './terminal/types';
-import { IInstaller, ILogger, IPathUtils, IPersistentStateFactory, Is64Bit, IsWindows } from './types';
+import { ICurrentProcess, IInstaller, ILogger, IPathUtils, IPersistentStateFactory, Is64Bit, IsWindows } from './types';
 
 export function registerTypes(serviceManager: IServiceManager) {
     serviceManager.addSingletonInstance<boolean>(IsWindows, IS_WINDOWS);
@@ -27,6 +28,7 @@ export function registerTypes(serviceManager: IServiceManager) {
     serviceManager.addSingleton<ILogger>(ILogger, Logger);
     serviceManager.addSingleton<ITerminalService>(ITerminalService, TerminalService);
     serviceManager.addSingleton<IPathUtils>(IPathUtils, PathUtils);
+    serviceManager.addSingleton<ICurrentProcess>(ICurrentProcess, CurrentProcess);
 
     if (IS_WINDOWS) {
         serviceManager.addSingleton<IRegistry>(IRegistry, RegistryImplementation);

src/client/common/serviceRegistry.ts

@@ -3,6 +3,7 @@
 // Licensed under the MIT License.
 
 import { Uri } from 'vscode';
+import { EnvironmentVariables } from './variables/types';
 export const IOutputChannel = Symbol('IOutputChannel');
 export const IDocumentSymbolProvider = Symbol('IDocumentSymbolProvider');
 export const IsWindows = Symbol('IS_WINDOWS');
@@ -80,3 +81,8 @@ export const IPathUtils = Symbol('IPathUtils');
 export interface IPathUtils {
     getPathVariableName(): 'Path' | 'PATH';
 }
+
+export const ICurrentProcess = Symbol('ICurrentProcess');
+export interface ICurrentProcess {
+    env: EnvironmentVariables;
+}

src/client/common/types.ts

@@ -1,40 +1,19 @@
 'use strict';
 // tslint:disable: no-any one-line no-suspicious-comment prefer-template prefer-const no-unnecessary-callback-wrapper no-function-expression no-string-literal no-control-regex no-shadowed-variable
-// TODO: Cleanup this place
-// Add options for execPythonFile
 
 import * as child_process from 'child_process';
 import * as fs from 'fs';
-import * as fsExtra from 'fs-extra';
 import * as os from 'os';
 import * as path from 'path';
-import { Position, Range, TextDocument, Uri } from 'vscode';
-import * as settings from './configSettings';
-import { parseEnvFile } from './envFileParser';
+import { Position, Range, TextDocument } from 'vscode';
 
 export const IS_WINDOWS = /^win/.test(process.platform);
 export const Is_64Bit = os.arch() === 'x64';
 export const PATH_VARIABLE_NAME = IS_WINDOWS ? 'Path' : 'PATH';
 
-const PathValidity: Map<string, boolean> = new Map<string, boolean>();
-export function validatePath(filePath: string): Promise<string> {
-    if (filePath.length === 0) {
-        return Promise.resolve('');
-    }
-    if (PathValidity.has(filePath)) {
-        return Promise.resolve(PathValidity.get(filePath) ? filePath : '');
-    }
-    return new Promise<string>(resolve => {
-        fs.exists(filePath, exists => {
-            PathValidity.set(filePath, exists);
-            return resolve(exists ? filePath : '');
-        });
-    });
-}
 export function fsExistsAsync(filePath: string): Promise<boolean> {
     return new Promise<boolean>(resolve => {
         fs.exists(filePath, exists => {
-            PathValidity.set(filePath, exists);
             return resolve(exists);
         });
     });
@@ -110,45 +89,6 @@ export function getSubDirectories(rootDir: string): Promise<string[]> {
     });
 }
 
-export async function getCustomEnvVars(resource?: Uri): Promise<{} | undefined | null> {
-    const envFile = settings.PythonSettings.getInstance(resource).envFile;
-    if (typeof envFile !== 'string' || envFile.length === 0) {
-        return null;
-    }
-    const exists = await fsExtra.pathExists(envFile);
-    if (!exists) {
-        return null;
-    }
-    try {
-        const vars = parseEnvFile(envFile);
-        if (vars && typeof vars === 'object' && Object.keys(vars).length > 0) {
-            return vars;
-        }
-    } catch (ex) {
-        console.error('Failed to parse env file', ex);
-    }
-    return null;
-}
-export function getCustomEnvVarsSync(resource?: Uri): {} | undefined | null {
-    const envFile = settings.PythonSettings.getInstance(resource).envFile;
-    if (typeof envFile !== 'string' || envFile.length === 0) {
-        return null;
-    }
-    const exists = fsExtra.pathExistsSync(envFile);
-    if (!exists) {
-        return null;
-    }
-    try {
-        const vars = parseEnvFile(envFile);
-        if (vars && typeof vars === 'object' && Object.keys(vars).length > 0) {
-            return vars;
-        }
-    } catch (ex) {
-        console.error('Failed to parse env file', ex);
-    }
-    return null;
-}
-
 export function getWindowsLineEndingCount(document: TextDocument, offset: Number) {
     const eolPattern = new RegExp('\r\n', 'g');
     const readBlock = 1024;

src/client/common/utils.ts

@@ -18,18 +18,15 @@ export class EnvironmentVariablesService implements IEnvironmentVariablesService
         if (!exists) {
             return undefined;
         }
+        if (!fs.lstatSync(filePath).isFile()) {
+            return undefined;
+        }
         return new Promise<EnvironmentVariables | undefined>((resolve, reject) => {
             fs.readFile(filePath, 'utf8', (error, data) => {
                 if (error) {
                     return reject(error);
                 }
-                const vars = parseEnvironmentVariables(data)!;
-                if (!vars || Object.keys(vars).length === 0) {
-                    return resolve(undefined);
-                }
-                this.appendPythonPath(vars, process.env.PYTHONPATH);
-                this.appendPath(vars, process.env[this.pathVariable]);
-                resolve(vars);
+                resolve(parseEnvironmentVariables(data));
             });
         });
     }
@@ -47,28 +44,25 @@ export class EnvironmentVariablesService implements IEnvironmentVariablesService
             }
         });
     }
-    public prependPythonPath(vars: EnvironmentVariables, ...pythonPaths: string[]) {
-        return this.appendOrPrependPaths(vars, 'PYTHONPATH', false, ...pythonPaths);
-    }
     public appendPythonPath(vars: EnvironmentVariables, ...pythonPaths: string[]) {
-        return this.appendOrPrependPaths(vars, 'PYTHONPATH', true, ...pythonPaths);
-    }
-    public prependPath(vars: EnvironmentVariables, ...paths: string[]) {
-        return this.appendOrPrependPaths(vars, this.pathVariable, false, ...paths);
+        return this.appendPaths(vars, 'PYTHONPATH', ...pythonPaths);
     }
     public appendPath(vars: EnvironmentVariables, ...paths: string[]) {
-        return this.appendOrPrependPaths(vars, this.pathVariable, true, ...paths);
+        return this.appendPaths(vars, this.pathVariable, ...paths);
     }
-    private appendOrPrependPaths(vars: EnvironmentVariables, variableName: 'PATH' | 'Path' | 'PYTHONPATH', append: boolean, ...pythonPaths: string[]) {
-        const pathToInsert = pythonPaths.filter(item => typeof item === 'string' && item.length > 0).join(path.delimiter);
-        if (pathToInsert.length === 0) {
+    private appendPaths(vars: EnvironmentVariables, variableName: 'PATH' | 'Path' | 'PYTHONPATH', ...pathsToAppend: string[]) {
+        const valueToAppend = pathsToAppend
+            .filter(item => typeof item === 'string' && item.trim().length > 0)
+            .map(item => item.trim())
+            .join(path.delimiter);
+        if (valueToAppend.length === 0) {
             return vars;
         }
 
         if (typeof vars[variableName] === 'string' && vars[variableName].length > 0) {
-            vars[variableName] = append ? (vars[variableName] + path.delimiter + pathToInsert) : (pathToInsert + path.delimiter + vars[variableName]);
+            vars[variableName] = vars[variableName] + path.delimiter + valueToAppend;
         } else {
-            vars[variableName] = pathToInsert;
+            vars[variableName] = valueToAppend;
         }
         return vars;
     }

src/client/common/variables/environment.ts

@@ -2,54 +2,71 @@
 // Licensed under the MIT License.
 
 import { inject, injectable } from 'inversify';
-import { Disposable, FileSystemWatcher, Uri, workspace } from 'vscode';
+import { Disposable, Event, EventEmitter, FileSystemWatcher, Uri, workspace } from 'vscode';
 import { PythonSettings } from '../configSettings';
 import { NON_WINDOWS_PATH_VARIABLE_NAME, WINDOWS_PATH_VARIABLE_NAME } from '../platform/constants';
-import { IDisposableRegistry, IsWindows } from '../types';
+import { ICurrentProcess, IDisposableRegistry, IsWindows } from '../types';
 import { EnvironmentVariables, IEnvironmentVariablesProvider, IEnvironmentVariablesService } from './types';
 
 @injectable()
 export class EnvironmentVariablesProvider implements IEnvironmentVariablesProvider, Disposable {
-    private cache = new Map<string, { vars: EnvironmentVariables | undefined, mergedWithProc: EnvironmentVariables }>();
+    private cache = new Map<string, EnvironmentVariables>();
     private fileWatchers = new Map<string, FileSystemWatcher>();
     private disposables: Disposable[] = [];
-
+    private changeEventEmitter: EventEmitter<Uri | undefined>;
     constructor( @inject(IEnvironmentVariablesService) private envVarsService: IEnvironmentVariablesService,
-        @inject(IDisposableRegistry) disposableRegistry: Disposable[], @inject(IsWindows) private isWidows: boolean) {
+        @inject(IDisposableRegistry) disposableRegistry: Disposable[], @inject(IsWindows) private isWidows: boolean,
+        @inject(ICurrentProcess) private process: ICurrentProcess) {
         disposableRegistry.push(this);
+        this.changeEventEmitter = new EventEmitter();
+    }
+
+    public get onDidEnvironmentVariablesChange(): Event<Uri | undefined> {
+        return this.changeEventEmitter.event;
     }
 
     public dispose() {
+        this.changeEventEmitter.dispose();
         this.fileWatchers.forEach(watcher => {
             watcher.dispose();
         });
     }
-    public async getEnvironmentVariables(mergeWithProcEnvVariables: boolean, resource?: Uri): Promise<EnvironmentVariables | undefined> {
+    public async getEnvironmentVariables(resource?: Uri): Promise<EnvironmentVariables> {
         const settings = PythonSettings.getInstance(resource);
         if (!this.cache.has(settings.envFile)) {
-            this.createFileWatcher(settings.envFile);
-            const vars = await this.envVarsService.parseFile(settings.envFile);
+            const workspaceFolderUri = this.getWorkspaceFolderUri(resource);
+            this.createFileWatcher(settings.envFile, workspaceFolderUri);
             let mergedVars = await this.envVarsService.parseFile(settings.envFile);
-            if (!mergedVars || Object.keys(mergedVars).length === 0) {
-                mergedVars = { ...process.env };
+            if (!mergedVars) {
+                mergedVars = {};
             }
-            this.envVarsService.mergeVariables(process.env, mergedVars!);
+            this.envVarsService.mergeVariables(this.process.env, mergedVars!);
             const pathVariable = this.isWidows ? WINDOWS_PATH_VARIABLE_NAME : NON_WINDOWS_PATH_VARIABLE_NAME;
-            this.envVarsService.appendPath(mergedVars!, process.env[pathVariable]);
-            this.envVarsService.appendPythonPath(mergedVars!, process.env.PYTHONPATH);
-            this.cache.set(settings.envFile, { vars, mergedWithProc: mergedVars! });
+            this.envVarsService.appendPath(mergedVars!, this.process.env[pathVariable]);
+            this.envVarsService.appendPythonPath(mergedVars!, this.process.env.PYTHONPATH);
+            this.cache.set(settings.envFile, mergedVars);
         }
-        const data = this.cache.get(settings.envFile)!;
-        return mergeWithProcEnvVariables ? data.mergedWithProc : data.vars;
+        return this.cache.get(settings.envFile)!;
     }
-    private createFileWatcher(envFile: string) {
+    private getWorkspaceFolderUri(resource?: Uri): Uri | undefined {
+        if (!resource) {
+            return;
+        }
+        const workspaceFolder = workspace.getWorkspaceFolder(resource!);
+        return workspaceFolder ? workspaceFolder.uri : undefined;
+    }
+    private createFileWatcher(envFile: string, workspaceFolderUri?: Uri) {
         if (this.fileWatchers.has(envFile)) {
             return;
         }
         const envFileWatcher = workspace.createFileSystemWatcher(envFile);
         this.fileWatchers.set(envFile, envFileWatcher);
-        this.disposables.push(envFileWatcher.onDidChange(() => this.cache.delete(envFile)));
-        this.disposables.push(envFileWatcher.onDidCreate(() => this.cache.delete(envFile)));
-        this.disposables.push(envFileWatcher.onDidDelete(() => this.cache.delete(envFile)));
+        this.disposables.push(envFileWatcher.onDidChange(() => this.onEnvironmentFileChanged(envFile, workspaceFolderUri)));
+        this.disposables.push(envFileWatcher.onDidCreate(() => this.onEnvironmentFileChanged(envFile, workspaceFolderUri)));
+        this.disposables.push(envFileWatcher.onDidDelete(() => this.onEnvironmentFileChanged(envFile, workspaceFolderUri)));
+    }
+    private onEnvironmentFileChanged(envFile, workspaceFolderUri?: Uri) {
+        this.cache.delete(envFile);
+        this.changeEventEmitter.fire(workspaceFolderUri);
     }
 }

src/client/common/variables/environmentVariablesProvider.ts

@@ -1,7 +1,7 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 
-import { Uri } from 'vscode';
+import { Event, Uri } from 'vscode';
 
 export type EnvironmentVariables = Object & {
     [key: string]: string;
@@ -12,9 +12,7 @@ export const IEnvironmentVariablesService = Symbol('IEnvironmentVariablesService
 export interface IEnvironmentVariablesService {
     parseFile(filePath: string): Promise<EnvironmentVariables | undefined>;
     mergeVariables(source: EnvironmentVariables, target: EnvironmentVariables): void;
-    prependPythonPath(vars: EnvironmentVariables, ...pythonPaths: string[]): void;
     appendPythonPath(vars: EnvironmentVariables, ...pythonPaths: string[]): void;
-    prependPath(vars: EnvironmentVariables, ...paths: string[]): void;
     appendPath(vars: EnvironmentVariables, ...paths: string[]): void;
 }
 
@@ -40,5 +38,6 @@ export interface ISystemVariables {
 export const IEnvironmentVariablesProvider = Symbol('IEnvironmentVariablesProvider');
 
 export interface IEnvironmentVariablesProvider {
-    getEnvironmentVariables(mergeWithProcEnvVariables: boolean, resource?: Uri): Promise<EnvironmentVariables | undefined>;
+    onDidEnvironmentVariablesChange: Event<Uri | undefined>;
+    getEnvironmentVariables(resource?: Uri): Promise<EnvironmentVariables | undefined>;
 }