repl: fix getters triggering side effects during completion

PR-URL: #61043
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Aviv Keller <[email protected]>

Author: dario-piotrowicz

lib/internal/repl/completion.js

@@ -731,35 +731,15 @@ 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 (
       astProp.type === 'Identifier' &&
       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,37 +753,48 @@ 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);
   }
 
-  return callback(false);
+  if (isProxy(obj[prop])) {
+    return cb(true);
+  }
+
+  return cb(false);
 }
 
 module.exports = {

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', () => {