Make error lines match the original file (#9984)

* Working line rewrite

* Add href and tests

* Support unix too

* Add news entry

* Fix linter problems

Author: rchiodo

news/2 Fixes/6370.md

@@ -0,0 +1 @@
+Make line numbers in errors for the Interactive window match the original file and make them clickable for jumping back to an error location.

src/client/common/platform/fileSystem.ts

@@ -129,7 +129,7 @@ export class RawFileSystem implements IRawFileSystem {
         paths?: IRawPath,
         vscfs?: IVSCodeFileSystemAPI,
         fsExtra?: IRawFSExtra
-    ): RawFileSystem{
+    ): RawFileSystem {
         // prettier-ignore
         return new RawFileSystem(
             paths || FileSystemPaths.withDefaults(),
@@ -472,6 +472,9 @@ export class FileSystem implements IFileSystem {
     public arePathsSame(path1: string, path2: string): boolean {
         return this.utils.pathUtils.arePathsSame(path1, path2);
     }
+    public getDisplayName(path: string): string {
+        return this.utils.pathUtils.getDisplayName(path);
+    }
     public async stat(filename: string): Promise<FileStat> {
         return this.utils.raw.stat(filename);
     }

src/client/common/platform/types.ts

@@ -192,6 +192,7 @@ export interface IFileSystem {
     // path-related
     directorySeparatorChar: string;
     arePathsSame(path1: string, path2: string): boolean;
+    getDisplayName(path: string): string;
 
     // "raw" operations
     stat(filePath: string): Promise<FileStat>;

src/client/datascience/interactive-common/linkProvider.ts

@@ -4,20 +4,26 @@
 import '../../common/extensions';
 
 import { inject, injectable } from 'inversify';
-import { Event, EventEmitter } from 'vscode';
+import { Event, EventEmitter, Position, Range, TextEditorRevealType, Uri } from 'vscode';
 
-import { IApplicationShell } from '../../common/application/types';
+import { IApplicationShell, IDocumentManager } from '../../common/application/types';
 import { IFileSystem } from '../../common/platform/types';
 import * as localize from '../../common/utils/localize';
 import { noop } from '../../common/utils/misc';
 import { IInteractiveWindowListener } from '../types';
 import { InteractiveWindowMessages } from './interactiveWindowTypes';
 
+const LineQueryRegex = /line=(\d+)/;
+
 // tslint:disable: no-any
 @injectable()
 export class LinkProvider implements IInteractiveWindowListener {
     private postEmitter: EventEmitter<{ message: string; payload: any }> = new EventEmitter<{ message: string; payload: any }>();
-    constructor(@inject(IApplicationShell) private applicationShell: IApplicationShell, @inject(IFileSystem) private fileSystem: IFileSystem) {
+    constructor(
+        @inject(IApplicationShell) private applicationShell: IApplicationShell,
+        @inject(IFileSystem) private fileSystem: IFileSystem,
+        @inject(IDocumentManager) private documentManager: IDocumentManager
+    ) {
         noop();
     }
 
@@ -29,7 +35,13 @@ export class LinkProvider implements IInteractiveWindowListener {
         switch (message) {
             case InteractiveWindowMessages.OpenLink:
                 if (payload) {
-                    this.applicationShell.openUrl(payload.toString());
+                    // Special case file URIs
+                    const href = payload.toString();
+                    if (href.startsWith('file')) {
+                        this.openFile(href);
+                    } else {
+                        this.applicationShell.openUrl(href);
+                    }
                 }
                 break;
             case InteractiveWindowMessages.SavePng:
@@ -59,4 +71,27 @@ export class LinkProvider implements IInteractiveWindowListener {
     public dispose(): void | undefined {
         noop();
     }
+
+    private openFile(fileUri: string) {
+        const uri = Uri.parse(fileUri);
+        let selection: Range = new Range(new Position(0, 0), new Position(0, 0));
+        if (uri.query) {
+            // Might have a line number query on the file name
+            const lineMatch = LineQueryRegex.exec(uri.query);
+            if (lineMatch) {
+                const lineNumber = parseInt(lineMatch[1], 10);
+                selection = new Range(new Position(lineNumber, 0), new Position(lineNumber, 0));
+            }
+        }
+
+        // Show the matching editor if there is one
+        const editor = this.documentManager.visibleTextEditors.find(e => this.fileSystem.arePathsSame(e.document.fileName, uri.fsPath));
+        if (editor) {
+            this.documentManager.showTextDocument(editor.document, { selection, viewColumn: editor.viewColumn }).then(() => {
+                editor.revealRange(selection, TextEditorRevealType.InCenter);
+            });
+        } else {
+            this.documentManager.showTextDocument(uri, { selection });
+        }
+    }
 }

src/client/datascience/jupyter/jupyterNotebook.ts

@@ -36,7 +36,7 @@ import {
     INotebookServerLaunchInfo,
     InterruptResult
 } from '../types';
-import { expandWorkingDir } from './jupyterUtils';
+import { expandWorkingDir, modifyTraceback } from './jupyterUtils';
 import { LiveKernelModel } from './kernels/types';
 
 // tslint:disable-next-line: no-require-imports
@@ -1079,7 +1079,7 @@ export class JupyterNotebookBase implements INotebook {
             output_type: 'error',
             ename: msg.content.ename,
             evalue: msg.content.evalue,
-            traceback: msg.content.traceback
+            traceback: modifyTraceback(cell.file, this.fs.getDisplayName(cell.file), cell.line, msg.content.traceback)
         };
         this.addToCellData(cell, output, clearState);
         cell.state = CellState.error;

src/client/datascience/jupyter/jupyterUtils.ts

@@ -10,8 +10,12 @@ import { IWorkspaceService } from '../../common/application/types';
 import { IDataScienceSettings } from '../../common/types';
 import { noop } from '../../common/utils/misc';
 import { SystemVariables } from '../../common/variables/systemVariables';
+import { Identifiers } from '../constants';
 import { IConnection } from '../types';
 
+// tslint:disable-next-line:no-require-imports no-var-requires
+const _escapeRegExp = require('lodash/escapeRegExp') as typeof import('lodash/escapeRegExp');
+
 export function expandWorkingDir(workingDir: string | undefined, launchingFile: string, workspace: IWorkspaceService): string {
     if (workingDir) {
         const variables = new SystemVariables(Uri.file(launchingFile), undefined, workspace);
@@ -45,3 +49,33 @@ export function createRemoteConnectionInfo(uri: string, settings: IDataScienceSe
         dispose: noop
     };
 }
+
+const LineMatchRegex = /(;32m[ ->]*?)(\d+)/g;
+const IPythonMatchRegex = /(<ipython-input.*?>)/g;
+
+function modifyLineNumbers(entry: string, file: string, startLine: number): string {
+    return entry.replace(LineMatchRegex, (_s, prefix, num) => {
+        const n = parseInt(num, 10);
+        const newLine = startLine + n;
+        return `${prefix}<a href='file://${file}?line=${newLine}'>${newLine + 1}</a>`;
+    });
+}
+
+function modifyTracebackEntry(fileMatchRegex: RegExp, file: string, fileDisplayName: string, startLine: number, entry: string): string {
+    if (fileMatchRegex.test(entry)) {
+        return modifyLineNumbers(entry, file, startLine);
+    } else if (IPythonMatchRegex.test(entry)) {
+        const ipythonReplaced = entry.replace(IPythonMatchRegex, fileDisplayName);
+        return modifyLineNumbers(ipythonReplaced, file, startLine);
+    }
+    return entry;
+}
+
+export function modifyTraceback(file: string, fileDisplayName: string, startLine: number, traceback: string[]): string[] {
+    if (file && file !== Identifiers.EmptyFileName) {
+        const escaped = _escapeRegExp(fileDisplayName);
+        const fileMatchRegex = new RegExp(`\\[.*?;32m${escaped}`);
+        return traceback.map(modifyTracebackEntry.bind(undefined, fileMatchRegex, file, fileDisplayName, startLine));
+    }
+    return traceback;
+}

src/test/datascience/jupyterUtils.unit.test.ts

@@ -6,7 +6,7 @@ import { anything, instance, mock, when } from 'ts-mockito';
 import { Uri } from 'vscode';
 import { WorkspaceService } from '../../client/common/application/workspace';
 import { IS_WINDOWS } from '../../client/common/platform/constants';
-import { expandWorkingDir } from '../../client/datascience/jupyter/jupyterUtils';
+import { expandWorkingDir, modifyTraceback } from '../../client/datascience/jupyter/jupyterUtils';
 
 suite('Data Science JupyterUtils', () => {
     const workspaceService = mock(WorkspaceService);
@@ -33,4 +33,45 @@ suite('Data Science JupyterUtils', () => {
         assert.equal(expandWorkingDir('${workspaceFolder}', 'test/xyz/bip/foo.baz', inst), Uri.file('test/bar').fsPath);
         assert.equal(expandWorkingDir('${cwd}-${file}', 'bar/bip/foo.baz', inst), `${Uri.file('test/bar').fsPath}-${Uri.file('bar/bip/foo.baz').fsPath}`);
     });
+
+    test('modifying traceback', () => {
+        const trace1 = [
+            '"\u001b[1;36m  File \u001b[1;32m"<ipython-input-2-940d61ce6e42>"\u001b[1;36m, line \u001b[1;32m599999\u001b[0m\n\u001b[1;33m    sys.\u001b[0m\n\u001b[1;37m        ^\u001b[0m\n\u001b[1;31mSyntaxError\u001b[0m\u001b[1;31m:\u001b[0m invalid syntax\n"'
+        ];
+        const after1 = [
+            `"\u001b[1;36m  File \u001b[1;32m"footastic.py"\u001b[1;36m, line \u001b[1;32m<a href='file://foo.py?line=600001'>600002</a>\u001b[0m\n\u001b[1;33m    sys.\u001b[0m\n\u001b[1;37m        ^\u001b[0m\n\u001b[1;31mSyntaxError\u001b[0m\u001b[1;31m:\u001b[0m invalid syntax\n"`
+        ];
+        const file1 = 'foo.py';
+        // Use a join after to make the assert show the results
+        assert.equal(after1.join('\n'), modifyTraceback(file1, 'footastic.py', 2, trace1).join('\n'), 'Syntax error failure');
+        const trace2 = [
+            '\u001b[1;31m---------------------------------------------------------------------------\u001b[0m',
+            '\u001b[1;31mException\u001b[0m                                 Traceback (most recent call last)',
+            "\u001b[1;32md:\\Training\\SnakePython\\manualTestFile.py\u001b[0m in \u001b[0;36m<module>\u001b[1;34m\u001b[0m\n\u001b[0;32m      3\u001b[0m \u001b[1;32mfor\u001b[0m \u001b[0mi\u001b[0m \u001b[1;32min\u001b[0m \u001b[0mtrange\u001b[0m\u001b[1;33m(\u001b[0m\u001b[1;36m100\u001b[0m\u001b[1;33m)\u001b[0m\u001b[1;33m:\u001b[0m\u001b[1;33m\u001b[0m\u001b[1;33m\u001b[0m\u001b[0m\n\u001b[0;32m      4\u001b[0m     \u001b[0mtime\u001b[0m\u001b[1;33m.\u001b[0m\u001b[0msleep\u001b[0m\u001b[1;33m(\u001b[0m\u001b[1;36m0.01\u001b[0m\u001b[1;33m)\u001b[0m\u001b[1;33m\u001b[0m\u001b[1;33m\u001b[0m\u001b[0m\n\u001b[1;32m----> 5\u001b[1;33m     \u001b[1;32mraise\u001b[0m \u001b[0mException\u001b[0m\u001b[1;33m(\u001b[0m\u001b[1;34m'spam'\u001b[0m\u001b[1;33m)\u001b[0m\u001b[1;33m\u001b[0m\u001b[1;33m\u001b[0m\u001b[0m\n\u001b[0m",
+            '\u001b[1;31mException\u001b[0m: spam'
+        ];
+        const after2 = [
+            '\u001b[1;31m---------------------------------------------------------------------------\u001b[0m',
+            '\u001b[1;31mException\u001b[0m                                 Traceback (most recent call last)',
+            `\u001b[1;32md:\\Training\\SnakePython\\manualTestFile.py\u001b[0m in \u001b[0;36m<module>\u001b[1;34m\u001b[0m\n\u001b[0;32m      <a href='file://d:\\Training\\SnakePython\\manualTestFile.py?line=23'>24</a>\u001b[0m \u001b[1;32mfor\u001b[0m \u001b[0mi\u001b[0m \u001b[1;32min\u001b[0m \u001b[0mtrange\u001b[0m\u001b[1;33m(\u001b[0m\u001b[1;36m100\u001b[0m\u001b[1;33m)\u001b[0m\u001b[1;33m:\u001b[0m\u001b[1;33m\u001b[0m\u001b[1;33m\u001b[0m\u001b[0m\n\u001b[0;32m      <a href='file://d:\\Training\\SnakePython\\manualTestFile.py?line=24'>25</a>\u001b[0m     \u001b[0mtime\u001b[0m\u001b[1;33m.\u001b[0m\u001b[0msleep\u001b[0m\u001b[1;33m(\u001b[0m\u001b[1;36m0.01\u001b[0m\u001b[1;33m)\u001b[0m\u001b[1;33m\u001b[0m\u001b[1;33m\u001b[0m\u001b[0m\n\u001b[1;32m----> <a href='file://d:\\Training\\SnakePython\\manualTestFile.py?line=25'>26</a>\u001b[1;33m     \u001b[1;32mraise\u001b[0m \u001b[0mException\u001b[0m\u001b[1;33m(\u001b[0m\u001b[1;34m'spam'\u001b[0m\u001b[1;33m)\u001b[0m\u001b[1;33m\u001b[0m\u001b[1;33m\u001b[0m\u001b[0m\n\u001b[0m`,
+            '\u001b[1;31mException\u001b[0m: spam'
+        ];
+        const file2 = 'd:\\Training\\SnakePython\\manualTestFile.py';
+        assert.equal(after2.join('\n'), modifyTraceback(file2, file2, 20, trace2).join('\n'), 'Exception failure');
+        const trace3 = [
+            '\u001b[0;31m---------------------------------------------------------------------------\u001b[0m',
+            '\u001b[0;31mModuleNotFoundError\u001b[0m                       Traceback (most recent call last)',
+            '\u001b[0;32m~/Test/manualTestFile.py\u001b[0m in \u001b[0;36m<module>\u001b[0;34m\u001b[0m\n\u001b[0;32m----> 4\u001b[0;31m \u001b[0;32mimport\u001b[0m \u001b[0mnumpy\u001b[0m \u001b[0;32mas\u001b[0m \u001b[0mnp\u001b[0m\u001b[0;34m\u001b[0m\u001b[0;34m\u001b[0m\u001b[0m\n\u001b[0m\u001b[1;32m      5\u001b[0m \u001b[0;32mimport\u001b[0m \u001b[0mpandas\u001b[0m \u001b[0;32mas\u001b[0m \u001b[0mpd\u001b[0m\u001b[0;34m\u001b[0m\u001b[0;34m\u001b[0m\u001b[0m\n\u001b[1;32m      6\u001b[0m \u001b[0;32mimport\u001b[0m \u001b[0mmatplotlib\u001b[0m\u001b[0;34m.\u001b[0m\u001b[0mpyplot\u001b[0m \u001b[0;32mas\u001b[0m \u001b[0mplt\u001b[0m\u001b[0;34m\u001b[0m\u001b[0;34m\u001b[0m\u001b[0m\n',
+            "\u001b[0;31mModuleNotFoundError\u001b[0m: No module named 'numpy'"
+        ];
+        const after3 = [
+            '\u001b[0;31m---------------------------------------------------------------------------\u001b[0m',
+            '\u001b[0;31mModuleNotFoundError\u001b[0m                       Traceback (most recent call last)',
+            "\u001b[0;32m~/Test/manualTestFile.py\u001b[0m in \u001b[0;36m<module>\u001b[0;34m\u001b[0m\n\u001b[0;32m----> <a href='file:///home/rich/Test/manualTestFile.py?line=24'>25</a>\u001b[0;31m \u001b[0;32mimport\u001b[0m \u001b[0mnumpy\u001b[0m \u001b[0;32mas\u001b[0m \u001b[0mnp\u001b[0m\u001b[0;34m\u001b[0m\u001b[0;34m\u001b[0m\u001b[0m\n\u001b[0m\u001b[1;32m      <a href='file:///home/rich/Test/manualTestFile.py?line=25'>26</a>\u001b[0m \u001b[0;32mimport\u001b[0m \u001b[0mpandas\u001b[0m \u001b[0;32mas\u001b[0m \u001b[0mpd\u001b[0m\u001b[0;34m\u001b[0m\u001b[0;34m\u001b[0m\u001b[0m\n\u001b[1;32m      <a href='file:///home/rich/Test/manualTestFile.py?line=26'>27</a>\u001b[0m \u001b[0;32mimport\u001b[0m \u001b[0mmatplotlib\u001b[0m\u001b[0;34m.\u001b[0m\u001b[0mpyplot\u001b[0m \u001b[0;32mas\u001b[0m \u001b[0mplt\u001b[0m\u001b[0;34m\u001b[0m\u001b[0;34m\u001b[0m\u001b[0m\n",
+            "\u001b[0;31mModuleNotFoundError\u001b[0m: No module named 'numpy'"
+        ];
+        const file3 = '/home/rich/Test/manualTestFile.py';
+        const display3 = '~/Test/manualTestFile.py';
+        assert.equal(after3.join('\n'), modifyTraceback(file3, display3, 20, trace3).join('\n'), 'Exception unix failure');
+    });
 });