From 44c465c08e38f1703ff5a385c97259058b6c5a5c Mon Sep 17 00:00:00 2001 From: Ian Harris Date: Thu, 30 Mar 2023 05:26:18 -0700 Subject: [PATCH 1/4] readline: fix issue with newline-less last line The logic for reading lines was slightly flawed, in that it assumed there would be a final new line. It handled the case where there are no new lines, but this then broke if there were some new lines. The fix in logic is basically removing the special case where there are no new lines by changing it to always read the final line with no new lines. This works because if a file contains no new lines, the final line is the first line, and all is well. There is some subtlety in this functioning, however. If the last line contains no new lines, then `lastIndex` will be the start of the last line, and `kInsertString` will be called from that point. If it does contain a new line, `lastIndex` will be equal to `s.length`, so the slice will be the empty string. Fixes: https://github.com/nodejs/node/issues/47305 --- lib/internal/readline/interface.js | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/lib/internal/readline/interface.js b/lib/internal/readline/interface.js index 4a5ec4973695fa..df08875cc79ae6 100644 --- a/lib/internal/readline/interface.js +++ b/lib/internal/readline/interface.js @@ -1324,21 +1324,19 @@ class Interface extends InterfaceConstructor { if (typeof s === 'string' && s) { // Erase state of previous searches. lineEnding.lastIndex = 0; - let nextMatch = RegExpPrototypeExec(lineEnding, s); - // If no line endings are found, just insert the string as is. - if (nextMatch === null) { - this[kInsertString](s); - } else { - // Keep track of the end of the last match. - let lastIndex = 0; - do { - this[kInsertString](StringPrototypeSlice(s, lastIndex, nextMatch.index)); - ({ lastIndex } = lineEnding); - this[kLine](); - // Restore lastIndex as the call to kLine could have mutated it. - lineEnding.lastIndex = lastIndex; - } while ((nextMatch = RegExpPrototypeExec(lineEnding, s)) !== null); + let nextMatch; + // Keep track of the end of the last match. + let lastIndex = 0; + while ((nextMatch = RegExpPrototypeExec(lineEnding, s)) !== null) { + this[kInsertString](StringPrototypeSlice(s, lastIndex, nextMatch.index)); + ({ lastIndex } = lineEnding); + this[kLine](); + // Restore lastIndex as the call to kLine could have mutated it. + lineEnding.lastIndex = lastIndex; } + // This ensures that the last line is written if it doesn't end in a newline. + // Note that the last line may be the first line, in which case this still works. + this[kInsertString](StringPrototypeSlice(s, lastIndex)); } } } From c04075ad35c95feae931e56f24661934dc9973ba Mon Sep 17 00:00:00 2001 From: Ian Harris Date: Sat, 1 Apr 2023 17:17:43 -0700 Subject: [PATCH 2/4] add repl and readline tests The repl test probably isn't strictly necessary, but this area's tests has been lacking sufficiently varying cases which have caused a few bugs to be missed, so it seems reasonable to include. --- ...repl-load-multiline-no-trailing-newline.js | 6 +++ ...-readline-interface-no-trailing-newline.js | 24 +++++++++++ ...repl-load-multiline-no-trailing-newline.js | 41 +++++++++++++++++++ 3 files changed, 71 insertions(+) create mode 100644 test/fixtures/repl-load-multiline-no-trailing-newline.js create mode 100644 test/parallel/test-readline-interface-no-trailing-newline.js create mode 100644 test/parallel/test-repl-load-multiline-no-trailing-newline.js diff --git a/test/fixtures/repl-load-multiline-no-trailing-newline.js b/test/fixtures/repl-load-multiline-no-trailing-newline.js new file mode 100644 index 00000000000000..448415be96ed52 --- /dev/null +++ b/test/fixtures/repl-load-multiline-no-trailing-newline.js @@ -0,0 +1,6 @@ +const getLunch = () => + placeOrder('tacos') + .then(eat); + +const placeOrder = (order) => Promise.resolve(order); +const eat = (food) => ''; \ No newline at end of file diff --git a/test/parallel/test-readline-interface-no-trailing-newline.js b/test/parallel/test-readline-interface-no-trailing-newline.js new file mode 100644 index 00000000000000..1f4bca3c41c885 --- /dev/null +++ b/test/parallel/test-readline-interface-no-trailing-newline.js @@ -0,0 +1,24 @@ +'use strict'; +const common = require('../common'); +const ArrayStream = require('../common/arraystream'); +const assert = require('assert'); + +common.skipIfDumbTerminal(); + +const readline = require('readline'); +const rli = new readline.Interface({ + terminal: true, + input: new ArrayStream(), + output: new ArrayStream(), +}); + +// Minimal reproduction for #47305 +const testInput = '{\n}'; + +let accum = ''; + +rli.output.write = data => accum += data.replace('\r', ''); + +rli.write(testInput); + +assert.strictEqual(accum, testInput); \ No newline at end of file diff --git a/test/parallel/test-repl-load-multiline-no-trailing-newline.js b/test/parallel/test-repl-load-multiline-no-trailing-newline.js new file mode 100644 index 00000000000000..0544e236b9bc0a --- /dev/null +++ b/test/parallel/test-repl-load-multiline-no-trailing-newline.js @@ -0,0 +1,41 @@ +'use strict'; +const common = require('../common'); +const ArrayStream = require('../common/arraystream'); +const fixtures = require('../common/fixtures'); +const assert = require('assert'); +const repl = require('repl'); + +common.skipIfDumbTerminal(); + +const command = `.load ${fixtures.path('repl-load-multiline-no-trailing-newline.js')}`; +const terminalCode = '\u001b[1G\u001b[0J \u001b[1G'; +const terminalCodeRegex = new RegExp(terminalCode.replace(/\[/g, '\\['), 'g'); + +const expected = `${command} +const getLunch = () => + placeOrder('tacos') + .then(eat); + +const placeOrder = (order) => Promise.resolve(order); +const eat = (food) => ''; +undefined +`; + +let accum = ''; + +const inputStream = new ArrayStream(); +const outputStream = new ArrayStream(); + +outputStream.write = (data) => accum += data.replace('\r', ''); + +const r = repl.start({ + prompt: '', + input: inputStream, + output: outputStream, + terminal: true, + useColors: false +}); + +r.write(`${command}\n`); +assert.strictEqual(accum.replace(terminalCodeRegex, ''), expected); +r.close(); From a407b73b53bef4c8d975e27395eedb6b2c4a0e87 Mon Sep 17 00:00:00 2001 From: Ian Harris Date: Sun, 2 Apr 2023 12:25:26 -0700 Subject: [PATCH 3/4] fix lint issues --- test/parallel/test-readline-interface-no-trailing-newline.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-readline-interface-no-trailing-newline.js b/test/parallel/test-readline-interface-no-trailing-newline.js index 1f4bca3c41c885..b3392db8619c95 100644 --- a/test/parallel/test-readline-interface-no-trailing-newline.js +++ b/test/parallel/test-readline-interface-no-trailing-newline.js @@ -17,8 +17,8 @@ const testInput = '{\n}'; let accum = ''; -rli.output.write = data => accum += data.replace('\r', ''); +rli.output.write = (data) => accum += data.replace('\r', ''); rli.write(testInput); -assert.strictEqual(accum, testInput); \ No newline at end of file +assert.strictEqual(accum, testInput); From 70fa0c79864c25da2525e150dd46822a227cabf1 Mon Sep 17 00:00:00 2001 From: Ian Harris Date: Sun, 2 Apr 2023 12:32:17 -0700 Subject: [PATCH 4/4] add note about intentional lack of newline test file --- test/fixtures/repl-load-multiline-no-trailing-newline.js | 1 + test/parallel/test-repl-load-multiline-no-trailing-newline.js | 1 + 2 files changed, 2 insertions(+) diff --git a/test/fixtures/repl-load-multiline-no-trailing-newline.js b/test/fixtures/repl-load-multiline-no-trailing-newline.js index 448415be96ed52..605d49e2d051bd 100644 --- a/test/fixtures/repl-load-multiline-no-trailing-newline.js +++ b/test/fixtures/repl-load-multiline-no-trailing-newline.js @@ -1,3 +1,4 @@ +// The lack of a newline at the end of this file is intentional. const getLunch = () => placeOrder('tacos') .then(eat); diff --git a/test/parallel/test-repl-load-multiline-no-trailing-newline.js b/test/parallel/test-repl-load-multiline-no-trailing-newline.js index 0544e236b9bc0a..f57638d2521bbe 100644 --- a/test/parallel/test-repl-load-multiline-no-trailing-newline.js +++ b/test/parallel/test-repl-load-multiline-no-trailing-newline.js @@ -12,6 +12,7 @@ const terminalCode = '\u001b[1G\u001b[0J \u001b[1G'; const terminalCodeRegex = new RegExp(terminalCode.replace(/\[/g, '\\['), 'g'); const expected = `${command} +// The lack of a newline at the end of this file is intentional. const getLunch = () => placeOrder('tacos') .then(eat);