Switch to using URIs as much as possible (#9641)

* Partially moved to using URI in some spots * Building, but not fully passing yet * Fix a bunch of unit tests * Fix interpreter hashes and a bunch of other tests * Revert launch.json change * Environment unit test fixup * More path fix ups * All but one unit tests passing * All unit tests passing * Add news entry * Fix unit test failures on linux * Build error * Fix tests on windows * Fix maps/sets of URIs * Fix linter * Another spot we could use the URI * Not an actual URI on disk, but just a file path * More places found * More places found * Fixup unit test on Linux * Put back launch.json (just to cause another CI run) * Try to fix failures on linux * Fix path comparisons * Change ids and add more logging * Python extension is cause of the slowdown * Review feedback, rename path to uri
microsoft · Apr 13, 2022 · ecc0aaf · ecc0aaf
1 parent be9a5d5
commit ecc0aaf
Show file tree

Hide file tree

Showing 184 changed files with 5,566 additions and 4,433 deletions.
diff --git a/.vscode/launch.json b/.vscode/launch.json
@@ -100,27 +100,25 @@
                 "order": 3
             }
         },
-		{
-			"name": "Web Tests",
-			"type": "extensionHost",
-			"debugWebWorkerHost": true,
-			"request": "launch",
-			"args": [
-				"--extensionDevelopmentPath=${workspaceFolder}",
+        {
+            "name": "Web Tests",
+            "type": "extensionHost",
+            "debugWebWorkerHost": true,
+            "request": "launch",
+            "args": [
+                "--extensionDevelopmentPath=${workspaceFolder}",
                 "--enable-proposed-api",
-				"--extensionDevelopmentKind=web",
-				"--extensionTestsPath=${workspaceFolder}/out/extension.web.bundle"
-			],
-			"outFiles": [
-				"${workspaceFolder}/out/**/*.*"
-			],
+                "--extensionDevelopmentKind=web",
+                "--extensionTestsPath=${workspaceFolder}/out/extension.web.bundle"
+            ],
+            "outFiles": ["${workspaceFolder}/out/**/*.*"],
             "sourceMaps": true,
             "preLaunchTask": "compile-web-test",
             "presentation": {
                 "group": "2_tests",
                 "order": 11
             }
-		},
+        },
         {
             // Note, for the smoke test you want to debug, you may need to copy the file,
             // rename it and remove a check for only smoke tests.

diff --git a/news/3 Code Health/9599.md b/news/3 Code Health/9599.md
@@ -0,0 +1 @@
+Switch to using URIs wherever possible instead of strings for file paths.
diff --git a/src/extension.node.ts b/src/extension.node.ts
@@ -255,13 +255,11 @@ function addOutputChannel(context: IExtensionContext, serviceManager: IServiceMa
     if (!workspace.workspaceFolders || workspace.workspaceFolders.length === 0) {
         standardOutputChannel.appendLine(`No workspace folder opened.`);
     } else if (workspace.workspaceFolders.length === 1) {
-        standardOutputChannel.appendLine(
-            `Workspace folder ${getDisplayPath(workspace.workspaceFolders[0].uri.fsPath)}`
-        );
+        standardOutputChannel.appendLine(`Workspace folder ${getDisplayPath(workspace.workspaceFolders[0].uri)}`);
     } else {
         standardOutputChannel.appendLine(
             `Multiple Workspace folders opened ${workspace.workspaceFolders
-                .map((item) => getDisplayPath(item.uri.fsPath))
+                .map((item) => getDisplayPath(item.uri))
                 .join(', ')}`
         );
     }

diff --git a/src/intellisense/intellisenseProvider.node.ts b/src/intellisense/intellisenseProvider.node.ts
@@ -15,6 +15,7 @@ import { INotebookLanguageClientProvider, INotebookControllerManager } from '../
 import { LanguageServer } from './languageServer.node';
 import { IInteractiveWindowProvider } from '../interactive-window/types';
 import { IVSCodeNotebookController } from '../notebooks/controllers/types';
+import { getComparisonKey } from '../platform/vscode-path/resources';
 
 const EmptyWorkspaceKey = '';
 
@@ -153,16 +154,17 @@ export class IntellisenseProvider implements INotebookLanguageClientProvider, IE
     }
 
     private getInterpreterIdFromCache(interpreter: PythonEnvironment) {
-        let id = this.interpreterIdCache.get(interpreter.path);
+        const key = getComparisonKey(interpreter.uri);
+        let id = this.interpreterIdCache.get(key);
         if (!id) {
             // Making an assumption that the id for an interpreter never changes.
             id = getInterpreterId(interpreter);
-            this.interpreterIdCache.set(interpreter.path, id);
+            this.interpreterIdCache.set(key, id);
         }
         return id;
     }
 
-    private shouldAllowIntellisense(uri: Uri, interpreterId: string, _interpreterPath: string) {
+    private shouldAllowIntellisense(uri: Uri, interpreterId: string, _interpreterPath: Uri) {
         // We should allow intellisense for a URI when the interpreter matches
         // the controller for the uri
         const notebook = findAssociatedNotebookDocument(uri, this.notebooks, this.interactiveWindowProvider);

diff --git a/src/intellisense/languageServer.node.ts b/src/intellisense/languageServer.node.ts
@@ -119,7 +119,7 @@ export class LanguageServer implements Disposable {
     public static async createLanguageServer(
         middlewareType: 'pylance' | 'jupyter',
         interpreter: PythonEnvironment,
-        shouldAllowIntellisense: (uri: Uri, interpreterId: string, interpreterPath: string) => boolean,
+        shouldAllowIntellisense: (uri: Uri, interpreterId: string, interpreterPath: Uri) => boolean,
         getNotebookHeader: (uri: Uri) => string
     ): Promise<LanguageServer | undefined> {
         const cancellationStrategy = new FileBasedCancellationStrategy();
@@ -134,15 +134,15 @@ export class LanguageServer implements Disposable {
                           () => languageClient,
                           () => noop, // Don't trace output. Slows things down too much
                           NOTEBOOK_SELECTOR,
-                          interpreter.path,
-                          (uri) => shouldAllowIntellisense(uri, interpreterId, interpreter.path),
+                          interpreter.uri.fsPath || interpreter.uri.path,
+                          (uri) => shouldAllowIntellisense(uri, interpreterId, interpreter.uri),
                           getNotebookHeader
                       )
                     : createPylanceMiddleware(
                           () => languageClient,
                           NOTEBOOK_SELECTOR,
-                          interpreter.path,
-                          (uri) => shouldAllowIntellisense(uri, interpreterId, interpreter.path),
+                          interpreter.uri.fsPath || interpreter.uri.path,
+                          (uri) => shouldAllowIntellisense(uri, interpreterId, interpreter.uri),
                           getNotebookHeader
                       );
 
@@ -228,7 +228,7 @@ export class LanguageServer implements Disposable {
             const runJediPath = path.join(python.extensionPath, 'pythonFiles', 'run-jedi-language-server.py');
             if (await fs.pathExists(runJediPath)) {
                 const serverOptions: ServerOptions = {
-                    command: interpreter.path || 'python',
+                    command: interpreter.uri.fsPath || 'python',
                     args: [runJediPath]
                 };
                 return serverOptions;

diff --git a/src/kernels/debugging/interactiveWindowDebugger.node.ts b/src/kernels/debugging/interactiveWindowDebugger.node.ts
@@ -56,7 +56,7 @@ export class InteractiveWindowDebugger implements IInteractiveWindowDebugger, IC
         // The python extension debugger tags the debug configuration with the python path used on the python property
         // by tagging this here (if available) we can treat IW or python extension debug session the same in knowing
         // which python launched them
-        const pythonPath = kernel.kernelConnectionMetadata.interpreter?.path;
+        const pythonPath = kernel.kernelConnectionMetadata.interpreter?.uri;
 
         return this.startDebugSession((c) => this.debugService.startDebugging(undefined, c), kernel, {
             justMyCode: settings.debugJustMyCode,

diff --git a/src/kernels/helpers.node.ts b/src/kernels/helpers.node.ts
@@ -4,6 +4,7 @@
 'use strict';
 
 import * as path from '../platform/vscode-path/path';
+import * as uriPath from '../platform/vscode-path/resources';
 import * as url from 'url';
 import type { KernelSpec } from '@jupyterlab/services';
 // eslint-disable-next-line @typescript-eslint/no-var-requires, @typescript-eslint/no-require-imports
@@ -35,7 +36,6 @@ import { traceError, traceInfo, traceInfoIfCI, traceVerbose, traceWarning } from
 import { getDisplayPath } from '../platform/common/platform/fs-paths';
 import { IPythonExecutionFactory } from '../platform/common/process/types.node';
 import {
-    IPathUtils,
     IConfigurationService,
     Resource,
     IMemento,
@@ -80,6 +80,8 @@ import { IStatusProvider } from '../platform/progress/types';
 import { IRawNotebookProvider } from './raw/types';
 import { IVSCodeNotebookController } from '../notebooks/controllers/types';
 import { isCI } from '../platform/common/constants.node';
+import { fsPathToUri } from '../platform/vscode-path/utils';
+import { deserializePythonEnvironment } from '../platform/api/pythonApi.node';
 
 // Helper functions for dealing with kernels and kernelspecs
 
@@ -140,9 +142,9 @@ export function getKernelId(spec: IJupyterKernelSpec, interpreter?: PythonEnviro
         argsForGenerationOfId = spec.argv.join('#').toLowerCase();
     }
     const prefixForRemoteKernels = remoteBaseUrl ? `${remoteBaseUrl}.` : '';
-    return `${prefixForRemoteKernels}${spec.id || ''}.${specName}.${getNormalizedInterpreterPath(
-        spec.interpreterPath || spec.path
-    )}.${getNormalizedInterpreterPath(interpreter?.path) || ''}.${argsForGenerationOfId}`;
+    return `${prefixForRemoteKernels}${spec.id || ''}.${specName}.${
+        getNormalizedInterpreterPath(fsPathToUri(spec.interpreterPath) || spec.uri).fsPath
+    }.${getNormalizedInterpreterPath(interpreter?.uri).fsPath || ''}.${argsForGenerationOfId}`;
 }
 
 export function getSysInfoReasonHeader(
@@ -228,7 +230,7 @@ function getPythonEnvironmentName(pythonEnv: PythonEnvironment) {
     // In such cases the envName is empty, but it has a path.
     let envName = pythonEnv.envName;
     if (pythonEnv.envPath && pythonEnv.envType === EnvironmentType.Conda && !pythonEnv.envName) {
-        envName = path.basename(pythonEnv.envPath);
+        envName = uriPath.basename(pythonEnv.envPath);
     }
     return envName;
 }
@@ -263,7 +265,7 @@ export function getNameOfKernelConnection(
         : kernelConnection.kernelSpec?.name;
 }
 
-export function getKernelPathFromKernelConnection(kernelConnection?: KernelConnectionMetadata): string | undefined {
+export function getKernelPathFromKernelConnection(kernelConnection?: KernelConnectionMetadata): Uri | undefined {
     if (!kernelConnection) {
         return;
     }
@@ -277,12 +279,15 @@ export function getKernelPathFromKernelConnection(kernelConnection?: KernelConne
             kernelConnection.kind === 'startUsingLocalKernelSpec') &&
             kernelConnection.kernelSpec.language === PYTHON_LANGUAGE)
     ) {
-        return kernelSpec?.metadata?.interpreter?.path || kernelSpec?.interpreterPath || kernelSpec?.path;
+        return fsPathToUri(kernelSpec?.metadata?.interpreter?.path || kernelSpec?.interpreterPath) || kernelSpec?.uri;
     } else {
         // For non python kernels, give preference to the executable path in the kernelspec
         // E.g. if we have a rust kernel, we should show the path to the rust executable not the interpreter (such as conda env that owns the rust runtime).
         return (
-            model?.path || kernelSpec?.path || kernelSpec?.metadata?.interpreter?.path || kernelSpec?.interpreterPath
+            model?.uri ||
+            kernelSpec?.uri ||
+            fsPathToUri(kernelSpec?.metadata?.interpreter?.path) ||
+            fsPathToUri(kernelSpec?.interpreterPath)
         );
     }
 }
@@ -302,7 +307,6 @@ export function getRemoteKernelSessionInformation(
 
 export function getKernelConnectionPath(
     kernelConnection: KernelConnectionMetadata | undefined,
-    pathUtils: IPathUtils,
     workspaceService: IWorkspaceService
 ) {
     if (kernelConnection?.kind === 'connectToLiveRemoteKernel') {
@@ -311,9 +315,8 @@ export function getKernelConnectionPath(
     const kernelPath = getKernelPathFromKernelConnection(kernelConnection);
     // If we have just one workspace folder opened, then ensure to use relative paths
     // where possible (e.g. for virtual environments).
-    const cwd =
-        workspaceService.workspaceFolders?.length === 1 ? workspaceService.workspaceFolders[0].uri.fsPath : undefined;
-    return kernelPath ? pathUtils.getDisplayName(kernelPath, cwd) : '';
+    const folders = workspaceService.workspaceFolders ? workspaceService.workspaceFolders : [];
+    return kernelPath ? getDisplayPath(kernelPath, folders) : '';
 }
 
 export function getInterpreterFromKernelConnectionMetadata(
@@ -327,12 +330,12 @@ export function getInterpreterFromKernelConnectionMetadata(
     }
     const model = kernelConnectionMetadataHasKernelModel(kernelConnection) ? kernelConnection.kernelModel : undefined;
     if (model?.metadata?.interpreter) {
-        return model.metadata.interpreter;
+        return deserializePythonEnvironment(model?.metadata?.interpreter);
     }
     const kernelSpec = kernelConnectionMetadataHasKernelSpec(kernelConnection)
         ? kernelConnection.kernelSpec
         : undefined;
-    return kernelSpec?.metadata?.interpreter;
+    return deserializePythonEnvironment(kernelSpec?.metadata?.interpreter);
 }
 export function isPythonKernelConnection(kernelConnection?: KernelConnectionMetadata): boolean {
     if (!kernelConnection) {
@@ -406,7 +409,7 @@ export function getInterpreterWorkspaceFolder(
     interpreter: PythonEnvironment,
     workspaceService: IWorkspaceService
 ): string | undefined {
-    const folder = workspaceService.getWorkspaceFolder(Uri.file(interpreter.path));
+    const folder = workspaceService.getWorkspaceFolder(interpreter.uri);
     return folder?.uri.fsPath || workspaceService.rootPath;
 }
 /**
@@ -470,16 +473,21 @@ export function getKernelRegistrationInfo(
  */
 export function createInterpreterKernelSpec(
     interpreter?: PythonEnvironment,
-    rootKernelFilePath?: string
+    rootKernelFilePath?: Uri
 ): IJupyterKernelSpec {
+    const interpreterMetadata = interpreter
+        ? {
+              path: interpreter.uri.fsPath
+          }
+        : {};
     // This creates a kernel spec for an interpreter. When launched, 'python' argument will map to using the interpreter
     // associated with the current resource for launching.
     const defaultSpec: KernelSpec.ISpecModel = {
         name: getInterpreterKernelSpecName(interpreter),
         language: 'python',
         display_name: interpreter?.displayName || 'Python 3',
         metadata: {
-            interpreter
+            interpreter: interpreterMetadata
         },
         argv: ['python', '-m', 'ipykernel_launcher', '-f', connectionFilePlaceholder],
         env: {},
@@ -489,10 +497,15 @@ export function createInterpreterKernelSpec(
     // Generate spec file path if we know where kernel files will go
     const specFile =
         rootKernelFilePath && defaultSpec.name
-            ? path.join(rootKernelFilePath, defaultSpec.name, 'kernel.json')
+            ? uriPath.joinPath(rootKernelFilePath, defaultSpec.name, 'kernel.json')
             : undefined;
 
-    return new JupyterKernelSpec(defaultSpec, specFile, interpreter?.path, 'registeredByNewVersionOfExt');
+    return new JupyterKernelSpec(
+        defaultSpec,
+        specFile ? specFile.fsPath : undefined,
+        interpreter?.uri.fsPath,
+        'registeredByNewVersionOfExt'
+    );
 }
 
 export function areKernelConnectionsEqual(
@@ -573,7 +586,7 @@ export function findPreferredKernel(
     traceInfo(
         `Find preferred kernel for ${getDisplayPath(resource)} with metadata ${JSON.stringify(
             notebookMetadata || {}
-        )} & preferred interpreter ${getDisplayPath(preferredInterpreter?.path)}`
+        )} & preferred interpreter ${getDisplayPath(preferredInterpreter?.uri)}`
     );
 
     if (kernels.length === 0) {
@@ -1205,7 +1218,7 @@ function compareAgainstKernelDisplayNameInNotebookMetadata(
         argv: [],
         display_name: notebookMetadata.kernelspec.display_name,
         name: notebookMetadata.kernelspec.name,
-        path: ''
+        uri: Uri.file('')
     });
     if (metadataPointsToADefaultKernelSpec) {
         // If we're dealing with default kernelspec names, then special handling.
@@ -1298,7 +1311,7 @@ function findKernelSpecMatchingInterpreter(
 
     // if we have more than one match then something is wrong.
     if (result.length > 1) {
-        traceError(`More than one kernel spec matches the interpreter ${interpreter.path}.`, result);
+        traceError(`More than one kernel spec matches the interpreter ${interpreter.uri}.`, result);
         if (isCI) {
             throw new Error('More than one kernelspec matches the intererpreter');
         }
@@ -1313,13 +1326,16 @@ function interpreterMatchesThatInNotebookMetadata(
     notebookMetadata?: nbformat.INotebookMetadata
 ) {
     const interpreterHashInMetadata = getInterpreterHashInMetadata(notebookMetadata);
+    const interpreterHashForKernel = kernelConnection.interpreter
+        ? getInterpreterHash(kernelConnection.interpreter)
+        : undefined;
     return (
         interpreterHashInMetadata &&
         (kernelConnection.kind === 'startUsingLocalKernelSpec' ||
             kernelConnection.kind === 'startUsingRemoteKernelSpec' ||
             kernelConnection.kind === 'startUsingPythonInterpreter') &&
         kernelConnection.interpreter &&
-        getInterpreterHash(kernelConnection.interpreter) === interpreterHashInMetadata
+        interpreterHashForKernel === interpreterHashInMetadata
     );
 }
 
@@ -1353,10 +1369,7 @@ export async function sendTelemetryForPythonKernelExecutable(
             return;
         }
         const sysExecutable = concatMultilineString(output.text).trim().toLowerCase();
-        const match = areInterpreterPathsSame(
-            kernelConnection.interpreter.path.toLowerCase(),
-            sysExecutable.toLowerCase()
-        );
+        const match = areInterpreterPathsSame(kernelConnection.interpreter.uri, Uri.file(sysExecutable));
         sendTelemetryEvent(Telemetry.PythonKerneExecutableMatches, undefined, {
             match: match ? 'true' : 'false',
             kernelConnectionType: kernelConnection.kind
@@ -1380,7 +1393,10 @@ export async function sendTelemetryForPythonKernelExecutable(
                     throwOnStdErr: false
                 });
                 if (execOutput.stdout.trim().length > 0) {
-                    const match = areInterpreterPathsSame(execOutput.stdout.trim().toLowerCase(), sysExecutable);
+                    const match = areInterpreterPathsSame(
+                        Uri.file(execOutput.stdout.trim().toLowerCase()),
+                        Uri.file(sysExecutable)
+                    );
                     sendTelemetryEvent(Telemetry.PythonKerneExecutableMatches, undefined, {
                         match: match ? 'true' : 'false',
                         kernelConnectionType: kernelConnection.kind
@@ -1389,8 +1405,8 @@ export async function sendTelemetryForPythonKernelExecutable(
                     if (!match) {
                         traceError(
                             `Interpreter started by kernel does not match expectation, expected ${getDisplayPath(
-                                kernelConnection.interpreter?.path
-                            )}, got ${getDisplayPath(sysExecutable)}`
+                                kernelConnection.interpreter?.uri
+                            )}, got ${getDisplayPath(Uri.file(sysExecutable))}`
                         );
                     }
                 }
@@ -1598,7 +1614,7 @@ export async function handleKernelError(
         // If we failed to start the kernel, then clear cache used to track
         // whether we have dependencies installed or not.
         // Possible something is missing.
-        clearInstalledIntoInterpreterMemento(memento, Product.ipykernel, metadata.interpreter.path).ignoreErrors();
+        clearInstalledIntoInterpreterMemento(memento, Product.ipykernel, metadata.interpreter.uri).ignoreErrors();
     }
 
     const handleResult = await errorHandler.handleKernelError(error, 'start', metadata, resource);

diff --git a/src/kernels/installer/condaInstaller.node.ts b/src/kernels/installer/condaInstaller.node.ts
@@ -103,13 +103,13 @@ export class CondaInstaller extends ModuleInstaller {
         args.push(moduleName);
         args.push('-y');
         return {
-            exe: condaFile,
+            exe: condaFile?.fsPath,
             args
         };
     }
 
     private getEnvironmentPath(interpreter: PythonEnvironment) {
-        const dir = path.dirname(interpreter.path);
+        const dir = path.dirname(interpreter.uri.fsPath);
 
         // If interpreter is in bin or Scripts, then go up one level
         const subDirName = path.basename(dir);
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		Switch to using URIs wherever possible instead of strings for file paths.