A different way of fixing escaping (#14186)

* Move escaping to just output

* Add some tests to verify round tripping

* Fixup test for rountrip and make roundtripping actually work

* Add news entry

* Add to manual test file

* Fix streaming problem and add more to the test

* Fix traceback unit test

* Fix problem caught by functional tests :)

* Another functional test catch

news/2 Fixes/14182.md

@@ -0,0 +1 @@
+Do not escape output in the actual ipynb file.

src/client/datascience/editor-integration/cellhashprovider.ts

@@ -29,7 +29,9 @@ import {
 
 // tslint:disable-next-line:no-require-imports no-var-requires
 const _escapeRegExp = require('lodash/escapeRegExp') as typeof import('lodash/escapeRegExp'); // NOSONAR
-const LineNumberMatchRegex = /(;32m[ ->]*?)(\d+)/g;
+// tslint:disable-next-line: no-require-imports no-var-requires
+const _escape = require('lodash/escape') as typeof import('lodash/escape'); // NOSONAR
+const LineNumberMatchRegex = /(;32m[ ->]*?)(\d+)(.*)/g;
 
 interface IRangedCellHash extends ICellHash {
     code: string;
@@ -133,7 +135,7 @@ export class CellHashProvider implements ICellHashProvider, INotebookExecutionLo
                 ...msg,
                 content: {
                     ...msg.content,
-                    traceback: this.modifyTraceback(msg as KernelMessage.IErrorMsg) // NOSONAR
+                    transient: this.modifyTraceback(msg as KernelMessage.IErrorMsg) // NOSONAR
                 }
             };
         }
@@ -423,14 +425,16 @@ export class CellHashProvider implements ICellHashProvider, INotebookExecutionLo
             // Now attempt to find a cell that matches these source lines
             const offset = this.findCellOffset(this.hashes.get(match[0]), sourceLines);
             if (offset !== undefined) {
-                return traceFrame.replace(LineNumberMatchRegex, (_s, prefix, num) => {
+                return traceFrame.replace(LineNumberMatchRegex, (_s, prefix, num, suffix) => {
                     const n = parseInt(num, 10);
                     const newLine = offset + n - 1;
-                    return `${prefix}<a href='file://${match[0]}?line=${newLine}'>${newLine + 1}</a>`;
+                    return `${_escape(prefix)}<a href='file://${match[0]}?line=${newLine}'>${newLine + 1}</a>${_escape(
+                        suffix
+                    )}`;
                 });
             }
         }
-        return traceFrame;
+        return _escape(traceFrame);
     }
 }
 

src/client/datascience/jupyter/jupyterDebugger.ts

@@ -3,8 +3,6 @@
 'use strict';
 import type { nbformat } from '@jupyterlab/coreutils';
 import { inject, injectable, named } from 'inversify';
-// tslint:disable-next-line: no-require-imports
-import unescape = require('lodash/unescape');
 import * as path from 'path';
 import * as uuid from 'uuid/v4';
 import { DebugConfiguration, Disposable } from 'vscode';
@@ -475,10 +473,8 @@ export class JupyterDebugger implements IJupyterDebugger, ICellHashListener {
             if (outputs.length > 0) {
                 const data = outputs[0].data;
                 if (data && data.hasOwnProperty('text/plain')) {
-                    // Plain text should be escaped by our execution engine. Unescape it so
-                    // we can parse it.
                     // tslint:disable-next-line:no-any
-                    return unescape((data as any)['text/plain']);
+                    return (data as any)['text/plain'];
                 }
                 if (outputs[0].output_type === 'stream') {
                     const stream = outputs[0] as nbformat.IStream;

src/client/datascience/jupyter/jupyterNotebook.ts

@@ -40,11 +40,7 @@ import { KernelConnectionMetadata } from './kernels/types';
 
 // tslint:disable-next-line: no-require-imports
 import cloneDeep = require('lodash/cloneDeep');
-// tslint:disable-next-line: no-require-imports
-import escape = require('lodash/escape');
-// tslint:disable-next-line: no-require-imports
-import unescape = require('lodash/unescape');
-import { concatMultilineString, formatStreamText } from '../../../datascience-ui/common';
+import { concatMultilineString, formatStreamText, splitMultilineString } from '../../../datascience-ui/common';
 import { RefBool } from '../../common/refBool';
 import { PythonEnvironment } from '../../pythonEnvironments/info';
 import { getInterpreterFromKernelConnectionMetadata, isPythonKernelConnection } from './kernels/helpers';
@@ -787,12 +783,12 @@ export class JupyterNotebookBase implements INotebook {
                 outputs.forEach((o) => {
                     if (o.output_type === 'stream') {
                         const stream = o as nbformat.IStream;
-                        result = result.concat(formatStreamText(unescape(concatMultilineString(stream.text, true))));
+                        result = result.concat(formatStreamText(concatMultilineString(stream.text, true)));
                     } else {
                         const data = o.data;
                         if (data && data.hasOwnProperty('text/plain')) {
                             // tslint:disable-next-line:no-any
-                            result = result.concat(unescape((data as any)['text/plain']));
+                            result = result.concat((data as any)['text/plain']);
                         }
                     }
                 });
@@ -1206,12 +1202,7 @@ export class JupyterNotebookBase implements INotebook {
 
     private addToCellData = (
         cell: ICell,
-        output:
-            | nbformat.IUnrecognizedOutput
-            | nbformat.IExecuteResult
-            | nbformat.IDisplayData
-            | nbformat.IStream
-            | nbformat.IError,
+        output: nbformat.IExecuteResult | nbformat.IDisplayData | nbformat.IStream | nbformat.IError,
         clearState: RefBool
     ) => {
         const data: nbformat.ICodeCell = cell.data as nbformat.ICodeCell;
@@ -1237,7 +1228,10 @@ export class JupyterNotebookBase implements INotebook {
     ) {
         // Check our length on text output
         if (msg.content.data && msg.content.data.hasOwnProperty('text/plain')) {
-            msg.content.data['text/plain'] = escape(trimFunc(msg.content.data['text/plain'] as string));
+            msg.content.data['text/plain'] = splitMultilineString(
+                // tslint:disable-next-line: no-any
+                trimFunc(concatMultilineString(msg.content.data['text/plain'] as any))
+            );
         }
 
         this.addToCellData(
@@ -1265,14 +1259,15 @@ export class JupyterNotebookBase implements INotebook {
             reply.payload.forEach((o) => {
                 if (o.data && o.data.hasOwnProperty('text/plain')) {
                     // tslint:disable-next-line: no-any
-                    const str = (o.data as any)['text/plain'].toString();
-                    const data = escape(trimFunc(str)) as string;
+                    const str = concatMultilineString((o.data as any)['text/plain']); // NOSONAR
+                    const data = trimFunc(str);
                     this.addToCellData(
                         cell,
                         {
                             // Mark as stream output so the text is formatted because it likely has ansi codes in it.
                             output_type: 'stream',
-                            text: data,
+                            text: splitMultilineString(data),
+                            name: 'stdout',
                             metadata: {},
                             execution_count: reply.execution_count
                         },
@@ -1313,23 +1308,25 @@ export class JupyterNotebookBase implements INotebook {
                 ? data.outputs[data.outputs.length - 1]
                 : undefined;
         if (existing) {
-            // tslint:disable-next-line:restrict-plus-operands
-            existing.text = existing.text + escape(msg.content.text);
-            const originalText = formatStreamText(concatMultilineString(existing.text));
+            const originalText = formatStreamText(
+                // tslint:disable-next-line: no-any
+                `${concatMultilineString(existing.text as any)}${concatMultilineString(msg.content.text)}`
+            );
             originalTextLength = originalText.length;
-            existing.text = trimFunc(originalText);
-            trimmedTextLength = existing.text.length;
+            const newText = trimFunc(originalText);
+            trimmedTextLength = newText.length;
+            existing.text = splitMultilineString(newText);
         } else {
-            const originalText = formatStreamText(concatMultilineString(escape(msg.content.text)));
+            const originalText = formatStreamText(concatMultilineString(msg.content.text));
             originalTextLength = originalText.length;
             // Create a new stream entry
             const output: nbformat.IStream = {
                 output_type: 'stream',
                 name: msg.content.name,
-                text: trimFunc(originalText)
+                text: [trimFunc(originalText)]
             };
             data.outputs = [...data.outputs, output];
-            trimmedTextLength = output.text.length;
+            trimmedTextLength = output.text[0].length;
             cell.data = data;
         }
 
@@ -1338,23 +1335,16 @@ export class JupyterNotebookBase implements INotebook {
         // the output is trimmed and what setting changes that.
         // * If data.metadata.tags is undefined, define it so the following
         //   code is can rely on it being defined.
-        if (data.metadata.tags === undefined) {
-            data.metadata.tags = [];
-        }
-
-        data.metadata.tags = data.metadata.tags.filter((t) => t !== 'outputPrepend');
-
         if (trimmedTextLength < originalTextLength) {
+            if (data.metadata.tags === undefined) {
+                data.metadata.tags = [];
+            }
+            data.metadata.tags = data.metadata.tags.filter((t) => t !== 'outputPrepend');
             data.metadata.tags.push('outputPrepend');
         }
     }
 
     private handleDisplayData(msg: KernelMessage.IDisplayDataMsg, clearState: RefBool, cell: ICell) {
-        // Escape text output
-        if (msg.content.data && msg.content.data.hasOwnProperty('text/plain')) {
-            msg.content.data['text/plain'] = escape(msg.content.data['text/plain'] as string);
-        }
-
         const output: nbformat.IDisplayData = {
             output_type: 'display_data',
             data: msg.content.data,
@@ -1402,10 +1392,14 @@ export class JupyterNotebookBase implements INotebook {
     private handleError(msg: KernelMessage.IErrorMsg, clearState: RefBool, cell: ICell) {
         const output: nbformat.IError = {
             output_type: 'error',
-            ename: escape(msg.content.ename),
-            evalue: escape(msg.content.evalue),
-            traceback: msg.content.traceback.map(escape)
+            ename: msg.content.ename,
+            evalue: msg.content.evalue,
+            traceback: msg.content.traceback
         };
+        if (msg.content.hasOwnProperty('transient')) {
+            // tslint:disable-next-line: no-any
+            output.transient = (msg.content as any).transient;
+        }
         this.addToCellData(cell, output, clearState);
         cell.state = CellState.error;
 

src/client/datascience/jupyter/kernelVariables.ts

@@ -6,8 +6,6 @@ import { inject, injectable } from 'inversify';
 import stripAnsi from 'strip-ansi';
 import * as uuid from 'uuid/v4';
 
-// tslint:disable-next-line: no-require-imports
-import unescape = require('lodash/unescape');
 import { CancellationToken, Event, EventEmitter, Uri } from 'vscode';
 import { PYTHON_LANGUAGE } from '../../common/constants';
 import { traceError } from '../../common/logger';
@@ -248,7 +246,7 @@ export class KernelVariables implements IJupyterVariables {
 
     // Pull our text result out of the Jupyter cell
     private deserializeJupyterResult<T>(cells: ICell[]): T {
-        const text = unescape(this.extractJupyterResultText(cells));
+        const text = this.extractJupyterResultText(cells);
         return JSON.parse(text) as T;
     }
 
@@ -373,7 +371,7 @@ export class KernelVariables implements IJupyterVariables {
         // Now execute the query
         if (notebook && query) {
             const cells = await notebook.execute(query.query, Identifiers.EmptyFileName, 0, uuid(), token, true);
-            const text = unescape(this.extractJupyterResultText(cells));
+            const text = this.extractJupyterResultText(cells);
 
             // Apply the expression to it
             const matches = this.getAllMatches(query.parser, text);

src/client/datascience/jupyter/oldJupyterVariables.ts

@@ -11,8 +11,6 @@ import { Event, EventEmitter, Uri } from 'vscode';
 import { PYTHON_LANGUAGE } from '../../common/constants';
 import { traceError } from '../../common/logger';
 
-// tslint:disable-next-line: no-require-imports
-import unescape = require('lodash/unescape');
 import { IConfigurationService } from '../../common/types';
 import * as localize from '../../common/utils/localize';
 import { EXTENSION_ROOT_DIR } from '../../constants';
@@ -234,7 +232,7 @@ export class OldJupyterVariables implements IJupyterVariables {
 
     // Pull our text result out of the Jupyter cell
     private deserializeJupyterResult<T>(cells: ICell[]): T {
-        const text = unescape(this.extractJupyterResultText(cells));
+        const text = this.extractJupyterResultText(cells);
         return JSON.parse(text) as T;
     }
 
@@ -359,7 +357,7 @@ export class OldJupyterVariables implements IJupyterVariables {
         // Now execute the query
         if (notebook && query) {
             const cells = await notebook.execute(query.query, Identifiers.EmptyFileName, 0, uuid(), undefined, true);
-            const text = unescape(this.extractJupyterResultText(cells));
+            const text = this.extractJupyterResultText(cells);
 
             // Apply the expression to it
             const matches = this.getAllMatches(query.parser, text);

src/datascience-ui/interactive-common/cellOutput.tsx

@@ -20,6 +20,8 @@ import { getRichestMimetype, getTransform, isIPyWidgetOutput, isMimeTypeSupporte
 
 // tslint:disable-next-line: no-var-requires no-require-imports
 const ansiToHtml = require('ansi-to-html');
+// tslint:disable-next-line: no-var-requires no-require-imports
+const lodashEscape = require('lodash/escape');
 
 // tslint:disable-next-line: no-require-imports no-var-requires
 const cloneDeep = require('lodash/cloneDeep');
@@ -328,7 +330,7 @@ export class CellOutput extends React.Component<ICellOutputProps> {
             // tslint:disable-next-line: no-any
             const text = (input as any)['text/plain'];
             input = {
-                'text/html': text // XML tags should have already been escaped.
+                'text/html': lodashEscape(concatMultilineString(text))
             };
         } else if (output.output_type === 'stream') {
             mimeType = 'text/html';
@@ -337,7 +339,7 @@ export class CellOutput extends React.Component<ICellOutputProps> {
             renderWithScrollbars = true;
             // Sonar is wrong, TS won't compile without this AS
             const stream = output as nbformat.IStream; // NOSONAR
-            const concatted = concatMultilineString(stream.text);
+            const concatted = lodashEscape(concatMultilineString(stream.text));
             input = {
                 'text/html': concatted // XML tags should have already been escaped.
             };
@@ -363,14 +365,18 @@ export class CellOutput extends React.Component<ICellOutputProps> {
             const error = output as nbformat.IError; // NOSONAR
             try {
                 const converter = new CellOutput.ansiToHtmlClass(CellOutput.getAnsiToHtmlOptions());
-                const trace = error.traceback.length ? converter.toHtml(error.traceback.join('\n')) : error.evalue;
+                // Modified traceback may exist. If so use that instead. It's only at run time though
+                const traceback: string[] = error.transient
+                    ? (error.transient as string[])
+                    : error.traceback.map(lodashEscape);
+                const trace = traceback ? converter.toHtml(traceback.join('\n')) : error.evalue;
                 input = {
                     'text/html': trace
                 };
             } catch {
                 // This can fail during unit tests, just use the raw data
                 input = {
-                    'text/html': error.evalue
+                    'text/html': lodashEscape(error.evalue)
                 };
             }
         } else if (input) {
@@ -395,6 +401,12 @@ export class CellOutput extends React.Component<ICellOutputProps> {
             data = fixMarkdown(concatMultilineString(data as nbformat.MultilineString, true), true);
         }
 
+        // Make sure text output is escaped (nteract texttransform won't)
+        if (mimeType === 'text/plain' && data) {
+            data = lodashEscape(data.toString());
+            mimeType = 'text/html';
+        }
+
         return {
             isText,
             isError,

src/datascience-ui/react-common/postOffice.ts

@@ -88,7 +88,7 @@ export class PostOffice implements IDisposable {
                 // See ./src/datascience-ui/native-editor/index.html
                 // tslint:disable-next-line: no-any
                 const api = (this.vscodeApi as any) as { handleMessage?: Function };
-                if (api.handleMessage) {
+                if (api && api.handleMessage) {
                     api.handleMessage(this.handleMessages.bind(this));
                 }
             } catch {

src/test/datascience/interactiveWindow.functional.test.tsx

@@ -63,6 +63,8 @@ import {
     verifyLastCellInputState
 } from './testHelpers';
 import { ITestInteractiveWindowProvider } from './testInteractiveWindowProvider';
+// tslint:disable-next-line: no-require-imports no-var-requires
+const _escape = require('lodash/escape') as typeof import('lodash/escape'); // NOSONAR
 
 // tslint:disable:max-func-body-length trailing-comma no-any no-multiline-string
 suite('DataScience Interactive Window output tests', () => {
@@ -385,6 +387,9 @@ for _ in range(50):
     time.sleep(0.1)
     sys.stdout.write('\\r')`;
 
+            const exception = 'raise Exception("<html check>")';
+            addMockData(ioc, exception, `"<html check>"`, 'text/html', 'error');
+
             addMockData(ioc, badPanda, `pandas has no attribute 'read'`, 'text/html', 'error');
             addMockData(ioc, goodPanda, `<td>A table</td>`, 'text/html');
             addMockData(ioc, matPlotLib, matPlotLibResults, 'text/html');
@@ -401,6 +406,9 @@ for _ in range(50):
                 return Promise.resolve({ result: result, haveMore: loops > 0 });
             });
 
+            await addCode(ioc, exception, true);
+            verifyHtmlOnInteractiveCell(_escape(`<html check>`), CellPosition.Last);
+
             await addCode(ioc, badPanda, true);
             verifyHtmlOnInteractiveCell(`has no attribute 'read'`, CellPosition.Last);