repl: fix getters triggering side effects during completion

Author: dario-piotrowicz

lib/internal/repl/completion.js

@@ -732,16 +732,20 @@ 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);
+      if (propHasGetter) {
+        return cb(true);
+      }
+
+      if (isProxy(obj[astProp.value])) {
+        return cb(true);
+      }
+
+      return cb(false);
     }
 
     if (
@@ -750,16 +754,20 @@ function includesProxiesOrGetters(expr, exprStr, evalFn, ctx, callback) {
     ) {
       // 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);
+      if (propHasGetter) {
+        return cb(true);
+      }
+
+      if (isProxy(obj[astProp.name])) {
+        return cb(true);
+      }
+
+      return cb(false);
     }
 
     return evalFn(
@@ -773,36 +781,31 @@ 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);
+          if (propHasGetter) {
+            return cb(true);
+          }
+
+          if (isProxy(obj[evaledProp])) {
+            return cb(true);
+          }
+
+          return cb(false);
         }
 
-        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);
 }
 

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