unuse completeDomain and disable completion on call expression

Author: islandryu

lib/repl.js

@@ -353,7 +353,6 @@ function REPLServer(prompt,
   this.allowBlockingCompletions = !!options.allowBlockingCompletions;
   this.useColors = !!options.useColors;
   this._domain = options.domain || domain.create();
-  this._completeDomain = domain.create();
   this.useGlobal = !!useGlobal;
   this.ignoreUndefined = !!ignoreUndefined;
   this.replMode = replMode || module.exports.REPL_MODE_SLOPPY;
@@ -671,8 +670,6 @@ function REPLServer(prompt,
   }
 
   self.eval = self._domain.bind(eval_);
-  self.completeEval = self._completeDomain.bind(eval_);
-  self._completeDomain.on('error', (err) => { });
 
   self._domain.on('error', function debugDomainError(e) {
     debug('domain error');
@@ -1543,10 +1540,10 @@ function complete(line, callback) {
       return completionGroupsLoaded();
     }
 
-    return includesProxiesOrGetters(
+    return potentiallySideEffectfulAccess(
       completeTargetAst.body[0].expression,
       parsableCompleteTarget,
-      this.completeEval,
+      this.eval,
       this.context,
       (includes) => {
         if (includes) {
@@ -1563,7 +1560,7 @@ function complete(line, callback) {
 
         const memberGroups = [];
         const evalExpr = `try { ${expr} } catch {}`;
-        this.completeEval(evalExpr, this.context, getREPLResourceName(), (e, obj) => {
+        this.eval(evalExpr, this.context, getREPLResourceName(), (e, obj) => {
           try {
             reclusiveCatchPromise(obj);
             let p;
@@ -1756,10 +1753,11 @@ function findExpressionCompleteTarget(code) {
 }
 
 /**
- * Utility used to determine if an expression includes object getters or proxies.
+ * Utility to determine if accessing a given expression could have side effects.
  *
- * Example: given `obj.foo`, the function lets you know if `foo` has a getter function
- * associated to it, or if `obj` is a proxy
+ * Example: given `obj.foo`, this function checks if accessing `foo` may trigger a getter,
+ * or if any part of the chain is a Proxy, or if evaluating the property could cause side effects.
+ * This is used to avoid triggering user code or side effects during tab completion.
  * @param {any} expr The expression, in AST format to analyze
  * @param {string} exprStr The string representation of the expression
  * @param {(str: string, ctx: any, resourceName: string, cb: (error, evaled) => void) => void} evalFn
@@ -1768,15 +1766,19 @@ function findExpressionCompleteTarget(code) {
  * @param {(includes: boolean) => void} callback Callback that will be called with the result of the operation
  * @returns {void}
  */
-function includesProxiesOrGetters(expr, exprStr, evalFn, ctx, callback) {
+function potentiallySideEffectfulAccess(expr, exprStr, evalFn, ctx, callback) {
   if (expr?.type !== 'MemberExpression') {
     // If the expression is not a member one for obvious reasons no getters are involved
     return callback(false);
   }
 
+  if (expr.object.type === 'CallExpression' || expr.property.type === 'CallExpression') {
+    return callback(true);
+  }
+
   if (expr.object.type === 'MemberExpression') {
     // The object itself is a member expression, so we need to recurse (e.g. the expression is `obj.foo.bar`)
-    return includesProxiesOrGetters(
+    return potentiallySideEffectfulAccess(
       expr.object,
       exprStr.slice(0, expr.object.end),
       evalFn,

test/fixtures/repl-tab-completion-nested-repls.js

@@ -32,7 +32,7 @@ const putIn = new ArrayStream();
 const testMe = repl.start('', putIn);
 
 // Some errors are passed to the domain, but do not callback.
-testMe._completeDomain.on('error', function(err) {
+testMe._domain.on('error', function(err) {
   throw err;
 });
 

test/parallel/test-completion-on-function-disabled.js

@@ -0,0 +1,50 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('node:assert');
+const { test } = require('node:test');
+
+
+const ArrayStream = require('../common/arraystream');
+const repl = require('node:repl');
+
+function runCompletionTests(replInit, tests) {
+  const stream = new ArrayStream();
+  const testRepl = repl.start({ stream });
+
+  // Some errors are passed to the domain
+  testRepl._domain.on('error', assert.ifError);
+
+  testRepl.write(replInit);
+  testRepl.write('\n');
+
+  tests.forEach(([query, expectedCompletions]) => {
+    testRepl.complete(query, common.mustCall((error, data) => {
+      const actualCompletions = data[0];
+      if (expectedCompletions.length === 0) {
+        assert.deepStrictEqual(actualCompletions, []);
+      } else {
+        expectedCompletions.forEach((expectedCompletion) =>
+          assert(actualCompletions.includes(expectedCompletion), `completion '${expectedCompletion}' not found`)
+        );
+      }
+    }));
+  });
+}
+
+test('REPL completion on function disabled', () => {
+  runCompletionTests(`
+        function foo() { return { a: { b: 5 } } } 
+        const obj = { a: 5 }
+        const getKey = () => 'a';
+        `, [
+    ['foo().', []],
+    ['foo().a', []],
+    ['foo().a.', []],
+    ['foo()["a"]', []],
+    ['foo()["a"].', []],
+    ['foo()["a"].b', []],
+    ['obj[getKey()].', []],
+  ]);
+
+});

test/parallel/test-repl-completion-on-getters-disabled.js

@@ -90,7 +90,6 @@ describe('REPL completion in relation of getters', () => {
           ["objWithGetters[keys['foo key']].b", ["objWithGetters[keys['foo key']].bar"]],
           ['objWithGetters[fooKey].b', ['objWithGetters[fooKey].bar']],
           ["objWithGetters['f' + 'oo'].b", ["objWithGetters['f' + 'oo'].bar"]],
-          ['objWithGetters[getFooKey()].b', ['objWithGetters[getFooKey()].bar']],
         ]);
     });
 

test/parallel/test-repl-tab-complete-getter-error.js

@@ -21,7 +21,7 @@ async function runTest() {
     terminal: true
   });
 
-  replServer._completeDomain.on('error', (e) => {
+  replServer._domain.on('error', (e) => {
     assert.fail(`Error in REPL domain: ${e}`);
   });
 

test/parallel/test-repl-tab-complete-promises.js

@@ -19,8 +19,7 @@ const completionTests = [
   { run: `const foo = { get name() { return Promise.reject(); } };`,
     send: `foo.name` },
   { run: 'const baz = { get bar() { return ""; } }; const getPropText = () => Promise.reject();',
-    send: 'baz[getPropText()].',
-    completeError: true },
+    send: 'baz[getPropText()].' },
   {
     send: 'const quux = { bar: { return Promise.reject(); } }; const getPropText = () => "bar"; quux[getPropText()].',
   },
@@ -46,7 +45,7 @@ async function runReplCompleteTests(tests) {
     assert.fail(`Unexpected domain error: ${err.message}`);
   });
 
-  for (const { send, run, completeError = false } of tests) {
+  for (const { send, run } of tests) {
     if (run) {
       await new Promise((resolve, reject) => {
         replServer.eval(run, replServer.context, '', (err) => {
@@ -59,23 +58,7 @@ async function runReplCompleteTests(tests) {
       });
     }
 
-    const onError = (e) => {
-      assert.fail(`Unexpected error: ${e.message}`);
-    };
-
-    let completeErrorPromise = Promise.resolve();
-
-    if (completeError) {
-      completeErrorPromise = new Promise((resolve) => {
-        const handleError = () => {
-          replServer._completeDomain.removeListener('error', handleError);
-          resolve();
-        };
-        replServer._completeDomain.on('error', common.mustCall(handleError));
-      });
-    } else {
-      replServer._completeDomain.on('error', onError);
-    }
+    const completeErrorPromise = Promise.resolve();
 
     await replServer.complete(
       send,
@@ -89,13 +72,5 @@ async function runReplCompleteTests(tests) {
 
     await completeErrorPromise;
 
-    if (!completeError) {
-      await new Promise((resolve) => {
-        setImmediate(() => {
-          replServer._completeDomain.removeListener('error', onError);
-          resolve();
-        });
-      });
-    }
   }
 }

test/parallel/test-repl-tab-complete.js

@@ -44,7 +44,7 @@ function prepareREPL() {
   });
 
   // Some errors are passed to the domain, but do not callback
-  replServer._completeDomain.on('error', assert.ifError);
+  replServer._domain.on('error', assert.ifError);
 
   return { replServer, input };
 }