repl: catch promise errors during eval in completion

lib/repl.js

@@ -43,6 +43,7 @@
 'use strict';
 
 const {
+  ArrayIsArray,
   ArrayPrototypeAt,
   ArrayPrototypeFilter,
   ArrayPrototypeFindLastIndex,
@@ -98,6 +99,7 @@ const {
 
 const {
   isProxy,
+  isPromise,
 } = require('internal/util/types');
 
 const { BuiltinModule } = require('internal/bootstrap/realm');
@@ -1538,7 +1540,7 @@ function complete(line, callback) {
       return completionGroupsLoaded();
     }
 
-    return includesProxiesOrGetters(
+    return potentiallySideEffectfulAccess(
       completeTargetAst.body[0].expression,
       parsableCompleteTarget,
       this.eval,
@@ -1560,6 +1562,7 @@ function complete(line, callback) {
         const evalExpr = `try { ${expr} } catch {}`;
         this.eval(evalExpr, this.context, getREPLResourceName(), (e, obj) => {
           try {
+            reclusiveCatchPromise(obj);
             let p;
             if ((typeof obj === 'object' && obj !== null) ||
               typeof obj === 'function') {
@@ -1750,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
@@ -1762,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,
@@ -1800,6 +1808,7 @@ function includesProxiesOrGetters(expr, exprStr, evalFn, ctx, callback) {
   // is the property identifier/literal)
   if (expr.object.type === 'Identifier') {
     return evalFn(`try { ${expr.object.name} } catch {}`, ctx, getREPLResourceName(), (err, obj) => {
+      reclusiveCatchPromise(obj);
       if (err) {
         return callback(false);
       }
@@ -1815,6 +1824,7 @@ function includesProxiesOrGetters(expr, exprStr, evalFn, ctx, callback) {
 
         return evalFn(
           `try { ${exprStr} } catch {} `, ctx, getREPLResourceName(), (err, obj) => {
+            reclusiveCatchPromise(obj);
             if (err) {
               return callback(false);
             }
@@ -1877,6 +1887,7 @@ function includesProxiesOrGetters(expr, exprStr, evalFn, ctx, callback) {
       ctx,
       getREPLResourceName(),
       (err, evaledProp) => {
+        reclusiveCatchPromise(evaledProp);
         if (err) {
           return callback(false);
         }
@@ -1902,7 +1913,9 @@ function includesProxiesOrGetters(expr, exprStr, evalFn, ctx, callback) {
   function safeIsProxyAccess(obj, prop) {
     // Accessing `prop` may trigger a getter that throws, so we use try-catch to guard against it
     try {
-      return isProxy(obj[prop]);
+      const value = obj[prop];
+      reclusiveCatchPromise(value);
+      return isProxy(value);
     } catch {
       return false;
     }
@@ -1911,6 +1924,33 @@ function includesProxiesOrGetters(expr, exprStr, evalFn, ctx, callback) {
   return callback(false);
 }
 
+function reclusiveCatchPromise(obj, seen = new SafeWeakSet()) {
+  if (isPromise(obj)) {
+    return obj.catch(() => {});
+  } else if (ArrayIsArray(obj)) {
+    obj.forEach((item) => {
+      reclusiveCatchPromise(item, seen);
+    });
+  } else if (obj && typeof obj === 'object') {
+    if (seen.has(obj)) return;
+    seen.add(obj);
+
+    let props;
+    try {
+      props = ObjectGetOwnPropertyNames(obj);
+    } catch {
+      return;
+    }
+    for (const key of props) {
+      try {
+        reclusiveCatchPromise(obj[key], seen);
+      } catch {
+        continue;
+      }
+    }
+  }
+}
+
 REPLServer.prototype.completeOnEditorMode = (callback) => (err, results) => {
   if (err) return callback(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-eval-promises.js

@@ -0,0 +1,62 @@
+'use strict';
+
+const common = require('../common');
+const repl = require('repl');
+const ArrayStream = require('../common/arraystream');
+const assert = require('assert');
+
+const tests = [
+  {
+    send: 'Promise.reject()',
+    expect: /Promise \{[\s\S]*?Uncaught undefined\n?$/
+  },
+  {
+    send: 'let p = Promise.reject()',
+    expect: /undefined\nUncaught undefined\n?$/
+  },
+  {
+    send: `Promise.resolve()`,
+    expect: /Promise \{[\s\S]*?}\n?$/
+  },
+  {
+    send: `Promise.resolve().then(() => {})`,
+    expect: /Promise \{[\s\S]*?}\n?$/
+  },
+  {
+    send: `async function f() { throw new Error('test'); };f();`,
+    expect: /Promise \{[\s\S]*?<rejected> Error: test[\s\S]*?Uncaught Error: test[\s\S]*?\n?$/
+  },
+  {
+    send: `async function f() {};f();`,
+    expect: /Promise \{[\s\S]*?}\n?$/
+  },
+];
+
+(async function() {
+  await runReplTests(tests);
+})().then(common.mustCall());
+
+async function runReplTests(tests) {
+  for (const { send, expect } of tests) {
+    const input = new ArrayStream();
+    const output = new ArrayStream();
+    let outputText = '';
+    function write(data) {
+      outputText += data;
+    }
+    output.write = write;
+    const replServer = repl.start({
+      prompt: '',
+      input,
+      output: output,
+    });
+    input.emit('data', `${send}\n`);
+    await new Promise((resolve) => {
+      setTimeout(() => {
+        assert.match(outputText, expect);
+        replServer.close();
+        resolve();
+      }, 10);
+    });
+  }
+}

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

@@ -0,0 +1,76 @@
+'use strict';
+
+const common = require('../common');
+const repl = require('repl');
+const ArrayStream = require('../common/arraystream');
+const assert = require('assert');
+
+const completionTests = [
+  { send: 'Promise.reject().' },
+  { send: 'let p = Promise.reject().' },
+  { send: 'Promise.resolve().' },
+  { send: 'Promise.resolve().then(() => {}).' },
+  { send: `async function f() {throw new Error('test');}; f().` },
+  { send: `async function f() {}; f().` },
+  { send: 'const foo = { bar: Promise.reject() }; foo.bar.' },
+  // Test for that reclusiveCatchPromise does not infinitely recurse
+  // see lib/repl.js:reclusiveCatchPromise
+  { send: 'const a = {}; a.self = a; a.self.' },
+  { run: `const foo = { get name() { return Promise.reject(); } };`,
+    send: `foo.name` },
+  { run: 'const baz = { get bar() { return ""; } }; const getPropText = () => Promise.reject();',
+    send: 'baz[getPropText()].' },
+  {
+    send: 'const quux = { bar: { return Promise.reject(); } }; const getPropText = () => "bar"; quux[getPropText()].',
+  },
+];
+
+(async function() {
+  await runReplCompleteTests(completionTests);
+})().then(common.mustCall());
+
+async function runReplCompleteTests(tests) {
+  const input = new ArrayStream();
+  const output = new ArrayStream();
+
+  const replServer = repl.start({
+    prompt: '',
+    input,
+    output: output,
+    allowBlockingCompletions: true,
+    terminal: true
+  });
+
+  replServer._domain.on('error', (err) => {
+    assert.fail(`Unexpected domain error: ${err.message}`);
+  });
+
+  for (const { send, run } of tests) {
+    if (run) {
+      await new Promise((resolve, reject) => {
+        replServer.eval(run, replServer.context, '', (err) => {
+          if (err) {
+            reject(err);
+          } else {
+            resolve();
+          }
+        });
+      });
+    }
+
+    const completeErrorPromise = Promise.resolve();
+
+    await replServer.complete(
+      send,
+      common.mustCall((error, data) => {
+        assert.strictEqual(error, null);
+        assert.strictEqual(data.length, 2);
+        assert.strictEqual(typeof data[1], 'string');
+        assert.ok(send.includes(data[1]));
+      })
+    );
+
+    await completeErrorPromise;
+
+  }
+}