From 7131de5f777170f5f2fb06180b493ba93cd57740 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 11 Dec 2019 19:26:47 +0100 Subject: [PATCH] repl: improve completion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This improves the completion output by removing the nested special handling. It never fully worked as expected and required a lot of hacks to even keep it working halfway reliable. Our tests did not cover syntax errors though and those can not be handled by this implementation. Those break the layout and confuse the REPL. Besides that the completion now also works in case the current line has leading whitespace. Also improve the error output in case the completion fails. PR-URL: https://github.com/nodejs/node/pull/30907 Reviewed-By: Michaël Zasso Reviewed-By: Rich Trott --- lib/readline.js | 2 +- lib/repl.js | 73 +++---------------- .../repl-tab-completion-nested-repls.js | 2 +- test/parallel/test-repl-save-load.js | 5 +- .../test-repl-tab-complete-nested-repls.js | 2 + test/parallel/test-repl-tab-complete.js | 29 +++----- 6 files changed, 27 insertions(+), 86 deletions(-) diff --git a/lib/readline.js b/lib/readline.js index 853500f5961aea..bd70754b5b48a2 100644 --- a/lib/readline.js +++ b/lib/readline.js @@ -502,7 +502,7 @@ Interface.prototype._tabComplete = function(lastKeypressWasTab) { self.resume(); if (err) { - self._writeToOutput(`tab completion error ${inspect(err)}`); + self._writeToOutput(`Tab completion error: ${inspect(err)}`); return; } diff --git a/lib/repl.js b/lib/repl.js index ba8bd3b4d2c8dd..9d28234900f215 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -59,7 +59,6 @@ const { RegExp, Set, Symbol, - WeakMap, WeakSet, } = primordials; @@ -78,7 +77,6 @@ const { deprecate } = require('internal/util'); const { inspect } = require('internal/util/inspect'); -const Stream = require('stream'); const vm = require('vm'); const path = require('path'); const fs = require('fs'); @@ -123,7 +121,6 @@ const { } = internalBinding('contextify'); const history = require('internal/repl/history'); -const { setImmediate } = require('timers'); // Lazy-loaded. let processTopLevelAwait; @@ -132,7 +129,6 @@ const globalBuiltins = new Set(vm.runInNewContext('Object.getOwnPropertyNames(globalThis)')); const parentModule = module; -const replMap = new WeakMap(); const domainSet = new WeakSet(); const kBufferedCommandSymbol = Symbol('bufferedCommand'); @@ -562,14 +558,13 @@ function REPLServer(prompt, self.lastError = e; } - const top = replMap.get(self); if (options[kStandaloneREPL] && process.listenerCount('uncaughtException') !== 0) { process.nextTick(() => { process.emit('uncaughtException', e); - top.clearBufferedCommand(); - top.lines.level = []; - top.displayPrompt(); + self.clearBufferedCommand(); + self.lines.level = []; + self.displayPrompt(); }); } else { if (errStack === '') { @@ -595,10 +590,10 @@ function REPLServer(prompt, } // Normalize line endings. errStack += errStack.endsWith('\n') ? '' : '\n'; - top.outputStream.write(errStack); - top.clearBufferedCommand(); - top.lines.level = []; - top.displayPrompt(); + self.outputStream.write(errStack); + self.clearBufferedCommand(); + self.lines.level = []; + self.displayPrompt(); } }); @@ -909,7 +904,6 @@ exports.start = function(prompt, ignoreUndefined, replMode); if (!exports.repl) exports.repl = repl; - replMap.set(repl, repl); return repl; }; @@ -1046,23 +1040,6 @@ REPLServer.prototype.turnOffEditorMode = deprecate( 'REPLServer.turnOffEditorMode() is deprecated', 'DEP0078'); -// A stream to push an array into a REPL -// used in REPLServer.complete -function ArrayStream() { - Stream.call(this); - - this.run = function(data) { - for (let n = 0; n < data.length; n++) - this.emit('data', `${data[n]}\n`); - }; -} -ObjectSetPrototypeOf(ArrayStream.prototype, Stream.prototype); -ObjectSetPrototypeOf(ArrayStream, Stream); -ArrayStream.prototype.readable = true; -ArrayStream.prototype.writable = true; -ArrayStream.prototype.resume = function() {}; -ArrayStream.prototype.write = function() {}; - const requireRE = /\brequire\s*\(['"](([\w@./-]+\/)?(?:[\w@./-]*))/; const fsAutoCompleteRE = /fs(?:\.promises)?\.\s*[a-z][a-zA-Z]+\(\s*["'](.*)/; const simpleExpressionRE = @@ -1122,40 +1099,13 @@ REPLServer.prototype.complete = function() { // Warning: This eval's code like "foo.bar.baz", so it will run property // getter code. function complete(line, callback) { - // There may be local variables to evaluate, try a nested REPL - if (this[kBufferedCommandSymbol] !== undefined && - this[kBufferedCommandSymbol].length) { - // Get a new array of inputted lines - const tmp = this.lines.slice(); - // Kill off all function declarations to push all local variables into - // global scope - for (let n = 0; n < this.lines.level.length; n++) { - const kill = this.lines.level[n]; - if (kill.isFunction) - tmp[kill.line] = ''; - } - const flat = new ArrayStream(); // Make a new "input" stream. - const magic = new REPLServer('', flat); // Make a nested REPL. - replMap.set(magic, replMap.get(this)); - flat.run(tmp); // `eval` the flattened code. - // All this is only profitable if the nested REPL does not have a - // bufferedCommand. - if (!magic[kBufferedCommandSymbol]) { - magic._domain.on('error', (err) => { - if (!cjsLoader.asyncRunMain) - throw err; - setImmediate(() => { - throw err; - }); - }); - return magic.complete(line, callback); - } - } - // List of completion lists, one for each inheritance "level" let completionGroups = []; let completeOn, group; + // Ignore right whitespace. It could change the outcome. + line = line.trimLeft(); + // REPL commands (e.g. ".break"). let filter; let match = line.match(/^\s*\.(\w*)$/); @@ -1479,8 +1429,7 @@ function _memory(cmd) { // scope will not work for this function. self.lines.level.push({ line: self.lines.length - 1, - depth: depth, - isFunction: /\bfunction\b/.test(cmd) + depth: depth }); } else if (depth < 0) { // Going... up. diff --git a/test/fixtures/repl-tab-completion-nested-repls.js b/test/fixtures/repl-tab-completion-nested-repls.js index ceff6e79453705..1d2b154f2b3341 100644 --- a/test/fixtures/repl-tab-completion-nested-repls.js +++ b/test/fixtures/repl-tab-completion-nested-repls.js @@ -31,7 +31,7 @@ const repl = require('repl'); const putIn = new ArrayStream(); const testMe = repl.start('', putIn); -// Some errors are passed to the domain, but do not callback +// Some errors are passed to the domain, but do not callback. testMe._domain.on('error', function(err) { throw err; }); diff --git a/test/parallel/test-repl-save-load.js b/test/parallel/test-repl-save-load.js index ef9ff8f6498877..f6ecc8d4ab67e9 100644 --- a/test/parallel/test-repl-save-load.js +++ b/test/parallel/test-repl-save-load.js @@ -44,8 +44,9 @@ testMe._domain.on('error', function(reason) { }); const testFile = [ - 'let top = function() {', - 'let inner = {one:1};' + 'let inner = (function() {', + ' return {one:1};', + '})()' ]; const saveFileName = join(tmpdir.path, 'test.save.js'); diff --git a/test/parallel/test-repl-tab-complete-nested-repls.js b/test/parallel/test-repl-tab-complete-nested-repls.js index 36547e8d9fb5be..3cac02f20562bc 100644 --- a/test/parallel/test-repl-tab-complete-nested-repls.js +++ b/test/parallel/test-repl-tab-complete-nested-repls.js @@ -19,3 +19,5 @@ const result = spawnSync(process.execPath, [testFile]); // test here is to make sure that the error information bubbles up to the // calling process. assert.ok(result.status, 'testFile swallowed its error'); +const err = result.stderr.toString(); +assert.ok(err.includes('fhqwhgads'), err); diff --git a/test/parallel/test-repl-tab-complete.js b/test/parallel/test-repl-tab-complete.js index 1c66f9a3238230..6cf689c4b11074 100644 --- a/test/parallel/test-repl-tab-complete.js +++ b/test/parallel/test-repl-tab-complete.js @@ -54,12 +54,9 @@ const putIn = new ArrayStream(); const testMe = repl.start('', putIn); // Some errors are passed to the domain, but do not callback -testMe._domain.on('error', function(err) { - assert.ifError(err); -}); +testMe._domain.on('error', assert.ifError); // Tab Complete will not break in an object literal -putIn.run(['.clear']); putIn.run([ 'var inner = {', 'one:1' @@ -93,9 +90,7 @@ putIn.run([ 'var top = function() {', 'var inner = {one:1};' ]); -testMe.complete('inner.o', common.mustCall(function(error, data) { - assert.deepStrictEqual(data, works); -})); +testMe.complete('inner.o', getNoResultsFunction()); // When you close the function scope tab complete will not return the // locally scoped variable @@ -111,9 +106,7 @@ putIn.run([ ' one:1', '};' ]); -testMe.complete('inner.o', common.mustCall(function(error, data) { - assert.deepStrictEqual(data, works); -})); +testMe.complete('inner.o', getNoResultsFunction()); putIn.run(['.clear']); @@ -125,9 +118,7 @@ putIn.run([ ' one:1', '};' ]); -testMe.complete('inner.o', common.mustCall(function(error, data) { - assert.deepStrictEqual(data, works); -})); +testMe.complete('inner.o', getNoResultsFunction()); putIn.run(['.clear']); @@ -140,9 +131,7 @@ putIn.run([ ' one:1', '};' ]); -testMe.complete('inner.o', common.mustCall(function(error, data) { - assert.deepStrictEqual(data, works); -})); +testMe.complete('inner.o', getNoResultsFunction()); putIn.run(['.clear']); @@ -155,9 +144,7 @@ putIn.run([ ' one:1', '};' ]); -testMe.complete('inner.o', common.mustCall(function(error, data) { - assert.deepStrictEqual(data, works); -})); +testMe.complete('inner.o', getNoResultsFunction()); putIn.run(['.clear']); @@ -204,7 +191,9 @@ const spaceTimeout = setTimeout(function() { }, 1000); testMe.complete(' ', common.mustCall(function(error, data) { - assert.deepStrictEqual(data, [[], undefined]); + assert.ifError(error); + assert.strictEqual(data[1], ''); + assert.ok(data[0].includes('globalThis')); clearTimeout(spaceTimeout); }));