repl: fix getters triggering side effects during completion

PR-URL: https://github.com/nodejs/node/pull/61043
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Aviv Keller <me@aviv.sh>

lib/internal/repl/completion.js

@@ -731,17 +731,7 @@ function includesProxiesOrGetters(expr, exprStr, evalFn, ctx, callback) {
 
     if (astProp.type === 'Literal') {
       // We have something like `obj['foo'].x` where `x` is the literal
-
-      if (safeIsProxyAccess(obj, astProp.value)) {
-        return cb(true);
-      }
-
-      const propDescriptor = ObjectGetOwnPropertyDescriptor(
-        obj,
-        `${astProp.value}`,
-      );
-      const propHasGetter = typeof propDescriptor?.get === 'function';
-      return cb(propHasGetter);
+      return propHasGetterOrIsProxy(obj, astProp.value, cb);
     }
 
     if (
@@ -749,17 +739,7 @@ function includesProxiesOrGetters(expr, exprStr, evalFn, ctx, callback) {
       exprStr.at(astProp.start - 1) === '.'
     ) {
       // We have something like `obj.foo.x` where `foo` is the identifier
-
-      if (safeIsProxyAccess(obj, astProp.name)) {
-        return cb(true);
-      }
-
-      const propDescriptor = ObjectGetOwnPropertyDescriptor(
-        obj,
-        `${astProp.name}`,
-      );
-      const propHasGetter = typeof propDescriptor?.get === 'function';
-      return cb(propHasGetter);
+      return propHasGetterOrIsProxy(obj, astProp.name, cb);
     }
 
     return evalFn(
@@ -773,39 +753,50 @@ function includesProxiesOrGetters(expr, exprStr, evalFn, ctx, callback) {
       getREPLResourceName(),
       (err, evaledProp) => {
         if (err) {
-          return callback(false);
+          return cb(false);
         }
 
         if (typeof evaledProp === 'string') {
-          if (safeIsProxyAccess(obj, evaledProp)) {
-            return cb(true);
-          }
-
-          const propDescriptor = ObjectGetOwnPropertyDescriptor(
-            obj,
-            evaledProp,
-          );
-          const propHasGetter = typeof propDescriptor?.get === 'function';
-          return cb(propHasGetter);
+          return propHasGetterOrIsProxy(obj, evaledProp, cb);
         }
 
-        return callback(false);
+        return cb(false);
       },
     );
   }
 
-  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]);
-    } catch {
-      return false;
-    }
-  }
-
   return callback(false);
 }
 
+/**
+ * Given an object and a property name, checks whether the property has a getter, if not checks whether its
+ * value is a proxy.
+ *
+ * Note: the order is relevant here, we want to check whether the property has a getter _before_ we check
+ *       whether its value is a proxy, to ensure that is the property does have a getter we don't end up
+ *       triggering it when checking its value
+ * @param {any} obj The target object
+ * @param {string | number | bigint | boolean | RegExp} prop The target property
+ * @param {(includes: boolean) => void} cb Callback that will be called with the result of the operation
+ * @returns {void}
+ */
+function propHasGetterOrIsProxy(obj, prop, cb) {
+  const propDescriptor = ObjectGetOwnPropertyDescriptor(
+    obj,
+    prop,
+  );
+  const propHasGetter = typeof propDescriptor?.get === 'function';
+  if (propHasGetter) {
+    return cb(true);
+  }
+
+  if (isProxy(obj[prop])) {
+    return cb(true);
+  }
+
+  return cb(false);
+}
+
 module.exports = {
   complete,
 };

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

@@ -115,6 +115,33 @@ describe('REPL completion in relation of getters', () => {
           ['objWithGetters[getGFooKey()].b', []],
         ]);
     });
+
+    test('no side effects are triggered for getters during completion', async () => {
+      const { replServer } = startNewREPLServer();
+
+      await new Promise((resolve, reject) => {
+        replServer.eval('const foo = { get name() { globalThis.nameGetterRun = true; throw new Error(); } };',
+                        replServer.context, '', (err) => {
+                          if (err) {
+                            reject(err);
+                          } else {
+                            resolve();
+                          }
+                        });
+      });
+
+      ['foo.name.', 'foo["name"].'].forEach((test) => {
+        replServer.complete(
+          test,
+          common.mustCall((error, data) => {
+            // The context's nameGetterRun variable hasn't been set
+            assert.strictEqual(replServer.context.nameGetterRun, undefined);
+            // No errors has been thrown
+            assert.strictEqual(error, null);
+          })
+        );
+      });
+    });
   });
 
   describe('completions on proxies', () => {

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

@@ -1,36 +0,0 @@
-'use strict';
-
-const common = require('../common');
-const assert = require('assert');
-const { startNewREPLServer } = require('../common/repl');
-
-(async function() {
-  await runTest();
-})().then(common.mustCall());
-
-async function runTest() {
-  const { replServer } = startNewREPLServer();
-
-  await new Promise((resolve, reject) => {
-    replServer.eval(`
-     const foo = { get name() { throw new Error(); } };
-     `, replServer.context, '', (err) => {
-      if (err) {
-        reject(err);
-      } else {
-        resolve();
-      }
-    });
-  });
-
-  ['foo.name.', 'foo["name"].'].forEach((test) => {
-    replServer.complete(
-      test,
-      common.mustCall((error, data) => {
-        assert.strictEqual(error, null);
-        assert.strictEqual(data.length, 2);
-        assert.strictEqual(data[1], test);
-      })
-    );
-  });
-}