From 98f30e85c8a78450cf8de0da68beeeaa9e875c4c Mon Sep 17 00:00:00 2001 From: Dmitry Date: Mon, 21 Mar 2022 02:32:58 +0300 Subject: [PATCH] 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 --- src/repl.ts | 33 ++++-- src/test/helpers.ts | 20 ++-- src/test/repl/helpers.ts | 78 ++++++++++--- src/test/repl/node-repl-tla.ts | 2 +- src/test/repl/repl.spec.ts | 206 ++++++++++++++++++--------------- 5 files changed, 213 insertions(+), 126 deletions(-) diff --git a/src/repl.ts b/src/repl.ts index 21935ddbb..ae48c7816 100644 --- a/src/repl.ts +++ b/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 = 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 | 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))) + ); + }); } /** diff --git a/src/test/helpers.ts b/src/test/helpers.ts index cddf7575d..bdddc21f4 100644 --- a/src/test/helpers.ts +++ b/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 + state: ReturnType, + 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)) { diff --git a/src/test/repl/helpers.ts b/src/test/repl/helpers.ts index a085a266e..692ca0daa 100644 --- a/src/test/repl/helpers.ts +++ b/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>; + export interface CreateReplViaApiOptions { - registerHooks: true; + registerHooks: boolean; createReplOpts?: Partial; createServiceOpts?: Partial; } +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( + _test: TestInterface +) { + return { noErrorsAndStdoutContains, stderrContains }; + + function noErrorsAndStdoutContains( + title: string, + script: string, + contains: string, + options?: Partial + ) { + testReplInternal(title, script, contains, undefined, contains, options); + } + function stderrContains( + title: string, + script: string, + errorContains: string, + options?: Partial + ) { + testReplInternal( + title, + script, + undefined, + errorContains, + errorContains, + options + ); + } + function testReplInternal( + title: string, + script: string, + stdoutContains: string | undefined, + stderrContains: string | undefined, + waitPattern: string, + options?: Partial + ) { + _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); + }); + } +} diff --git a/src/test/repl/node-repl-tla.ts b/src/test/repl/node-repl-tla.ts index c3926566f..3e071ea87 100644 --- a/src/test/repl/node-repl-tla.ts +++ b/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); }', ]); diff --git a/src/test/repl/repl.spec.ts b/src/test/repl/repl.spec.ts index 3a106a389..d23ed445e 100644 --- a/src/test/repl/repl.spec.ts +++ b/src/test/repl/repl.spec.ts @@ -1,5 +1,5 @@ import { _test, expect } from '../testlib'; -import { ts } from '../helpers'; +import { resetNodeEnvironment, ts } from '../helpers'; import semver = require('semver'); import { CMD_TS_NODE_WITH_PROJECT_FLAG, @@ -9,10 +9,15 @@ import { } from '../helpers'; import { createExec, createExecTester } from '../exec-helpers'; import { upstreamTopLevelAwaitTests } from './node-repl-tla'; -import { contextReplHelpers } from './helpers'; +import { contextReplHelpers, replMacros } from './helpers'; import { promisify } from 'util'; const test = _test.context(contextTsNodeUnderTest).context(contextReplHelpers); +test.beforeEach(async (t) => { + t.teardown(() => { + resetNodeEnvironment(); + }); +}); const exec = createExec({ cwd: TEST_DIR, @@ -258,7 +263,7 @@ test.suite('top level await', (_test) => { test('should pass upstream test cases', async (t) => { const { tsNodeUnderTest } = t.context; - upstreamTopLevelAwaitTests({ TEST_DIR, tsNodeUnderTest }); + await upstreamTopLevelAwaitTests({ TEST_DIR, tsNodeUnderTest }); }); } else { test('should throw error when attempting to use top level await on TS < 3.8', async (t) => { @@ -308,12 +313,12 @@ test.suite( test.suite( 'REPL inputs are syntactically independent of each other', (test) => { - // Serial because it's timing-sensitive - test.serial( - 'arithmetic operators are independent of previous values', - async (t) => { - const { stdout, stderr } = await t.context.executeInRepl( - `9 + // Serial because they're timing-sensitive + test.runSerially(); + + test('arithmetic operators are independent of previous values', async (t) => { + const { stdout, stderr } = await t.context.executeInRepl( + `9 + 3 7 - 3 @@ -325,108 +330,121 @@ test.suite( ** 2\n.break console.log('done!') `, - { - registerHooks: true, - startInternalOptions: { useGlobal: false }, - waitPattern: 'done!\nundefined\n>', - } - ); - expect(stdout).not.toContain('12'); - expect(stdout).not.toContain('4'); - expect(stdout).not.toContain('21'); - expect(stdout).not.toContain('50'); - expect(stdout).not.toContain('25'); - expect(stdout).toContain('3'); - expect(stdout).toContain('-3'); - } - ); + { + registerHooks: true, + startInternalOptions: { useGlobal: false }, + waitPattern: 'done!\nundefined\n>', + } + ); + expect(stdout).not.toContain('12'); + expect(stdout).not.toContain('4'); + expect(stdout).not.toContain('21'); + expect(stdout).not.toContain('50'); + expect(stdout).not.toContain('25'); + expect(stdout).toContain('3'); + expect(stdout).toContain('-3'); + }); - // Serial because it's timing-sensitive - test.serial( - 'automatically inserted semicolons do not appear in error messages at the end', - async (t) => { - const { stdout, stderr } = await t.context.executeInRepl( - `( + test('automatically inserted semicolons do not appear in error messages at the end', async (t) => { + const { stdout, stderr } = await t.context.executeInRepl( + `( a console.log('done!')`, - { - registerHooks: true, - startInternalOptions: { useGlobal: false }, - waitPattern: 'done!\nundefined\n>', - } - ); - expect(stderr).toContain("error TS1005: ')' expected."); - expect(stderr).not.toContain(';'); - } - ); + { + registerHooks: true, + startInternalOptions: { useGlobal: false }, + waitPattern: 'done!\nundefined\n>', + } + ); + expect(stderr).toContain("error TS1005: ')' expected."); + expect(stderr).not.toContain(';'); + }); - // Serial because it's timing-sensitive - test.serial( - 'automatically inserted semicolons do not appear in error messages at the start', - async (t) => { - const { stdout, stderr } = await t.context.executeInRepl( - `) + test('automatically inserted semicolons do not appear in error messages at the start', async (t) => { + const { stdout, stderr } = await t.context.executeInRepl( + `) console.log('done!')`, - { - registerHooks: true, - startInternalOptions: { useGlobal: false }, - waitPattern: 'done!\nundefined\n>', - } - ); - expect(stderr).toContain( - 'error TS1128: Declaration or statement expected.' - ); - expect(stderr).toContain(')'); - expect(stderr).not.toContain(';'); - } - ); + { + registerHooks: true, + startInternalOptions: { useGlobal: false }, + waitPattern: 'done!\nundefined\n>', + } + ); + expect(stderr).toContain( + 'error TS1128: Declaration or statement expected.' + ); + expect(stderr).toContain(')'); + expect(stderr).not.toContain(';'); + }); - // Serial because it's timing-sensitive - test.serial( - 'automatically inserted semicolons do not break function calls', - async (t) => { - const { stdout, stderr } = await t.context.executeInRepl( - `function foo(a: number) { + test('automatically inserted semicolons do not break function calls', async (t) => { + const { stdout, stderr } = await t.context.executeInRepl( + `function foo(a: number) { return a + 1; } foo( 1 )`, - { - registerHooks: true, - startInternalOptions: { useGlobal: false }, - waitPattern: '2\n>', - } - ); - expect(stderr).toBe(''); - expect(stdout).toContain('2'); - } - ); + { + registerHooks: true, + startInternalOptions: { useGlobal: false }, + waitPattern: '2\n>', + } + ); + expect(stderr).toBe(''); + expect(stdout).toContain('2'); + }); - // Serial because it's timing-sensitive - test.serial( - 'automatically inserted semicolons do not affect subsequent line numbers', - async (t) => { - // If first line of input ends in a semicolon, should not add a second semicolon. - // That will cause an extra blank line in the compiled output which will - // offset the stack line number. - const { stdout, stderr } = await t.context.executeInRepl( - `1; + test('automatically inserted semicolons do not affect subsequent line numbers', async (t) => { + // If first line of input ends in a semicolon, should not add a second semicolon. + // That will cause an extra blank line in the compiled output which will + // offset the stack line number. + const { stdout, stderr } = await t.context.executeInRepl( + `1; new Error().stack!.split('\\n')[1] console.log('done!')`, - { - registerHooks: true, - startInternalOptions: { useGlobal: false }, - waitPattern: 'done!', - } - ); - expect(stderr).toBe(''); - expect(stdout).toContain(":1:1'\n"); - } - ); + { + registerHooks: true, + startInternalOptions: { useGlobal: false }, + waitPattern: 'done!', + } + ); + expect(stderr).toBe(''); + expect(stdout).toContain(":1:1'\n"); + }); } ); +test.suite('Multiline inputs and RECOVERY_CODES', (test) => { + test.runSerially(); + const macros = replMacros(test); + + macros.noErrorsAndStdoutContains( + 'multiline function args declaration', + ` + function myFn( + a: string, + b: string + ) { + return a + ' ' + b + } + myFn('test', '!') + `, + 'test !' + ); + + macros.stderrContains( + 'Conditional recovery codes: this one-liner *should* raise an error; should not be recoverable', + ` + (a: any) => a = null; + `, + 'error TS', + { + createServiceOpts: { compilerOptions: { strictNullChecks: false } }, + } + ); +}); + test.suite('REPL works with traceResolution', (test) => { test.serial( 'startup traces should print before the prompt appears when traceResolution is enabled',