Fix #1667 multiline function arguments support in REPL (#1677)

* Fix #1667 multiline function arguments support in REPL

* introduce dependency mechanism to recovery_codes to prevent erroneous suppression

* finish tests

* fix skipped tests

* Fix bug where node env reset also deletes coverage data

Co-authored-by: Andrew Bradley <[email protected]>

Author: d9k

src/repl.ts

@@ -679,17 +679,24 @@ function lineCount(value: string) {
 }
 
 /**
- * TS diagnostic codes which are recoverable, meaning that the user likely entered and incomplete line of code
+ * TS diagnostic codes which are recoverable, meaning that the user likely entered an incomplete line of code
  * and should be prompted for the next.  For example, starting a multi-line for() loop and not finishing it.
+ * null value means code is always recoverable.  `Set` means code is only recoverable when occurring alongside at least one
+ * of the other codes.
  */
-const RECOVERY_CODES: Set<number> = new Set([
-  1003, // "Identifier expected."
-  1005, // "')' expected."
-  1109, // "Expression expected."
-  1126, // "Unexpected end of text."
-  1160, // "Unterminated template literal."
-  1161, // "Unterminated regular expression literal."
-  2355, // "A function whose declared type is neither 'void' nor 'any' must return a value."
+const RECOVERY_CODES: Map<number, Set<number> | null> = new Map([
+  [1003, null], // "Identifier expected."
+  [1005, null], // "')' expected.", "'}' expected."
+  [1109, null], // "Expression expected."
+  [1126, null], // "Unexpected end of text."
+  [1160, null], // "Unterminated template literal."
+  [1161, null], // "Unterminated regular expression literal."
+  [2355, null], // "A function whose declared type is neither 'void' nor 'any' must return a value."
+  [2391, null], // "Function implementation is missing or not immediately following the declaration."
+  [
+    7010, // "Function, which lacks return-type annotation, implicitly has an 'any' return type."
+    new Set([1005]), // happens when fn signature spread across multiple lines: 'function a(\nb: any\n) {'
+  ],
 ]);
 
 /**
@@ -708,7 +715,13 @@ const topLevelAwaitDiagnosticCodes = [
  * Check if a function can recover gracefully.
  */
 function isRecoverable(error: TSError) {
-  return error.diagnosticCodes.every((code) => RECOVERY_CODES.has(code));
+  return error.diagnosticCodes.every((code) => {
+    const deps = RECOVERY_CODES.get(code);
+    return (
+      deps === null ||
+      (deps && error.diagnosticCodes.some((code) => deps.has(code)))
+    );
+  });
 }
 
 /**

src/test/helpers.ts

@@ -12,7 +12,7 @@ import type { Readable } from 'stream';
  */
 import type * as tsNodeTypes from '../index';
 import type _createRequire from 'create-require';
-import { has, once } from 'lodash';
+import { has, mapValues, once } from 'lodash';
 import semver = require('semver');
 const createRequire: typeof _createRequire = require('create-require');
 export { tsNodeTypes };
@@ -218,25 +218,29 @@ export function resetNodeEnvironment() {
   resetObject(require('module'), defaultModule);
 
   // May be modified by REPL tests, since the REPL sets globals.
-  resetObject(global, defaultGlobal);
+  // Avoid deleting nyc's coverage data.
+  resetObject(global, defaultGlobal, ['__coverage__']);
 }
 
 function captureObjectState(object: any) {
+  const descriptors = Object.getOwnPropertyDescriptors(object);
+  const values = mapValues(descriptors, (_d, key) => object[key]);
   return {
-    descriptors: Object.getOwnPropertyDescriptors(object),
-    values: { ...object },
+    descriptors,
+    values,
   };
 }
 // Redefine all property descriptors and delete any new properties
 function resetObject(
   object: any,
-  state: ReturnType<typeof captureObjectState>
+  state: ReturnType<typeof captureObjectState>,
+  doNotDeleteTheseKeys: string[] = []
 ) {
   const currentDescriptors = Object.getOwnPropertyDescriptors(object);
   for (const key of Object.keys(currentDescriptors)) {
-    if (!has(state.descriptors, key)) {
-      delete object[key];
-    }
+    if (doNotDeleteTheseKeys.includes(key)) continue;
+    if (has(state.descriptors, key)) continue;
+    delete object[key];
   }
   // Trigger nyc's setter functions
   for (const [key, value] of Object.entries(state.values)) {

src/test/repl/helpers.ts

@@ -2,6 +2,7 @@ import * as promisify from 'util.promisify';
 import { PassThrough } from 'stream';
 import { getStream, TEST_DIR, tsNodeTypes } from '../helpers';
 import type { ExecutionContext } from 'ava';
+import { expect, TestInterface } from '../testlib';
 
 export interface ContextWithTsNodeUnderTest {
   tsNodeUnderTest: Pick<
@@ -10,12 +11,24 @@ export interface ContextWithTsNodeUnderTest {
   >;
 }
 
+export type ContextWithReplHelpers = ContextWithTsNodeUnderTest &
+  Awaited<ReturnType<typeof contextReplHelpers>>;
+
 export interface CreateReplViaApiOptions {
-  registerHooks: true;
+  registerHooks: boolean;
   createReplOpts?: Partial<tsNodeTypes.CreateReplOptions>;
   createServiceOpts?: Partial<tsNodeTypes.CreateOptions>;
 }
 
+export interface ExecuteInReplOptions extends CreateReplViaApiOptions {
+  waitMs?: number;
+  waitPattern?: string | RegExp;
+  /** When specified, calls `startInternal` instead of `start` and passes options */
+  startInternalOptions?: Parameters<
+    tsNodeTypes.ReplService['startInternal']
+  >[0];
+}
+
 /**
  * pass to test.context() to get REPL testing helper functions
  */
@@ -55,18 +68,7 @@ export async function contextReplHelpers(
     return { stdin, stdout, stderr, replService, service };
   }
 
-  // Todo combine with replApiMacro
-  async function executeInRepl(
-    input: string,
-    options: CreateReplViaApiOptions & {
-      waitMs?: number;
-      waitPattern?: string | RegExp;
-      /** When specified, calls `startInternal` instead of `start` and passes options */
-      startInternalOptions?: Parameters<
-        tsNodeTypes.ReplService['startInternal']
-      >[0];
-    }
-  ) {
+  async function executeInRepl(input: string, options: ExecuteInReplOptions) {
     const {
       waitPattern,
       // Wait longer if there's a signal to end it early
@@ -102,3 +104,53 @@ export async function contextReplHelpers(
     };
   }
 }
+
+export function replMacros<T extends ContextWithReplHelpers>(
+  _test: TestInterface<T>
+) {
+  return { noErrorsAndStdoutContains, stderrContains };
+
+  function noErrorsAndStdoutContains(
+    title: string,
+    script: string,
+    contains: string,
+    options?: Partial<ExecuteInReplOptions>
+  ) {
+    testReplInternal(title, script, contains, undefined, contains, options);
+  }
+  function stderrContains(
+    title: string,
+    script: string,
+    errorContains: string,
+    options?: Partial<ExecuteInReplOptions>
+  ) {
+    testReplInternal(
+      title,
+      script,
+      undefined,
+      errorContains,
+      errorContains,
+      options
+    );
+  }
+  function testReplInternal(
+    title: string,
+    script: string,
+    stdoutContains: string | undefined,
+    stderrContains: string | undefined,
+    waitPattern: string,
+    options?: Partial<ExecuteInReplOptions>
+  ) {
+    _test(title, async (t) => {
+      const { stdout, stderr } = await t.context.executeInRepl(script, {
+        registerHooks: true,
+        startInternalOptions: { useGlobal: false },
+        waitPattern,
+        ...options,
+      });
+      if (stderrContains) expect(stderr).toContain(stderrContains);
+      else expect(stderr).toBe('');
+      if (stdoutContains) expect(stdout).toContain(stdoutContains);
+    });
+  }
+}

src/test/repl/node-repl-tla.ts

@@ -64,7 +64,7 @@ export async function upstreamTopLevelAwaitTests({
     return promise;
   }
 
-  runAndWait([
+  await runAndWait([
     'function foo(x) { return x; }',
     'function koo() { return Promise.resolve(4); }',
   ]);