repl: improve completion

BridgeAR · targos · commit f2d4db3efd94 · 2020-04-25T16:07:46.000+02:00
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: nodejs#30907 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
diff --git 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
@@ -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');
@@ -558,14 +554,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 === '') {
@@ -591,10 +586,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();
     }
   });
 
@@ -905,7 +900,6 @@ exports.start = function(prompt,
                               ignoreUndefined,
                               replMode);
   if (!exports.repl) exports.repl = repl;
-  replMap.set(repl, repl);
   return repl;
 };
 
@@ -1042,23 +1036,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 =
@@ -1118,40 +1095,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*)$/);
@@ -1475,8 +1425,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
@@ -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
@@ -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
@@ -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
@@ -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);
 }));
 

Original file line number	Diff line number	Diff line change
`@@ -502,7 +502,7 @@ Interface.prototype._tabComplete = function(lastKeypressWasTab) {`
`502`	`502`	`self.resume();`
`503`	`503`
`504`	`504`	`if (err) {`
`505`		- self._writeToOutput(`tab completion error ${inspect(err)}`);
	`505`	+ self._writeToOutput(`Tab completion error: ${inspect(err)}`);
`506`	`506`	`return;`
`507`	`507`	`}`
`508`	`508`