repl: avoid crash when typing incomplete import statement#63573
Open
awbx wants to merge 2 commits into
Open
Conversation
Typing `import` alone in the REPL triggers V8's "Cannot use import statement outside a module" SyntaxError, which the REPL was enriching with a dynamic-import suggestion via `toDynamicImport`. That helper called acorn to re-parse the line; on incomplete input acorn throws, the exception escaped the keypress handler, and the process exited with code 1. Catch the parse error inside `toDynamicImport` and fall back to a plain error message when the input cannot be parsed as a complete import statement. Fixes: nodejs#63551
Replace the child_process.spawn-based regression test with the in-process startNewREPLServer pattern (similar to test-repl-null.js). This is faster, avoids subprocess flake, and lets the test assert on the REPL output stream directly instead of bytes piped through stdio. Verified to still catch the regression: the test exits with code 1 against unpatched v26.1.0 (acorn SyntaxError escapes) and exits 0 against the fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #63551. Typing
import(or any incompleteimportform) at the Node.js REPL terminates the process with exit code 1 since v25.x, when the dynamic-import-suggestion helper was introduced. v22.x is unaffected.Reproduction (broken on v25.x and v26.x, exits with status 1)
Root cause
import\n.SyntaxError("Cannot use import statement outside a module").toDynamicImport(line)to build a "use dynamic import: …" suggestion.toDynamicImportcallsacornParse(line, { sourceType: 'module' })with notry/catch. For incomplete input acorn throws.lib/internal/readline/emitKeypressEvents.js, gets caught and rethrown (line 74).Fix
Two small changes in
lib/repl.js:toDynamicImportwraps theacornParsecall intry/catchand returns an empty string on parse failure.toDynamicImportonce, stores the result, and only appends the "alternatively use dynamic import: …" suffix when a non-empty string came back. When the line cannot be parsed (e.g.importalone), it falls back to a plain'Cannot use import statement inside the Node.js REPL.'message — same intent, no crash.When
toDynamicImportsucceeds, the produced message is byte-identical to the previous version, so the existing assertions intest/parallel/test-repl.js(lines 865–935, covering completeimportforms) continue to pass without modification.Tests
Added
test/parallel/test-repl-import-statement-no-crash.jsusing the in-processstartNewREPLServerhelper (the same pattern astest-repl-null.js). The test emits an incompleteimportline through the REPL and asserts:SyntaxErrorand the user-facing messageCannot use import statement inside the Node.js REPLon the output stream.Verified to catch the regression: against unpatched v26.1.0 the test exits with code 1 (acorn
SyntaxErrorescapes); against this branch's build the test exits 0.Validated locally on Apple Silicon Mac (Node built from this branch):
out/Release/node test/parallel/test-repl-import-statement-no-crash.js→ passesout/Release/node test/parallel/test-repl.js→ passes (the existing dynamic-import-suggestion assertions are unchanged)Manual sanity check after build:
Note on overlap with #63552
I noticed #63552 after submitting this — sorry for the duplicated effort. The root-cause analysis is the same and the core fix shape is functionally equivalent. The differences here are:
''fromtoDynamicImporton failure and updates the callsite to emit'Cannot use import statement inside the Node.js REPL.'(no suggestion). repl: fix crash when bare 'import' is typed #63552 returnscodeLineon failure, which makes the message read'… alternatively use dynamic import: import'. Minor UX preference.startNewREPLServerhelper (the change @aduh95 requested on repl: fix crash when bare 'import' is typed #63552's spawn-based test).Happy to defer to #63552 if a maintainer prefers — closing this is one click.
Fixes: #63551