Repository: nodejs/node
Pull request: repl: fix getters triggering side effects during completion

The changes in https://github.com/nodejs/node/pull/59044 are incorrect, as pointed out by @BridgeAR here: https://github.com/nodejs/node/pull/59044#pullrequestreview-3180803526

Sorry somehow I totally missed that while reviewing the PR, 59044 added a try-catch around the check to see if an object property is a proxy, the reasoning being that the property could have a getter that could throw an error.
The correct solution is not to try-catch the property access, but to avoid it in the first place. We actually have already the logic to check if a property has a getter, so the correct solution consists  simply in performing the proxy check _after_ we've made sure that the property doesn't have a  getter.


<!--
Before submitting a pull request, please read:

- the CONTRIBUTING guide at https://github.com/nodejs/node/blob/HEAD/CONTRIBUTING.md
- the commit message formatting guidelines at
  https://github.com/nodejs/node/blob/HEAD/doc/contributing/pull-requests.md#commit-message-guidelines

For code changes:
1. Include tests for any bug fixes or new features.
2. Update documentation if relevant.
3. Ensure that `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes.

If you believe this PR should be highlighted in the Node.js CHANGELOG
please add the `notable-change` label.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.
-->


Changed files:
filename: lib/internal/repl/completion.js
patch: @@ -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 = {

filename: test/parallel/test-repl-completion-on-getters-disabled.js
patch: @@ -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', () => {

filename: test/parallel/test-repl-tab-complete-getter-error.js
patch: @@ -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);
-      })
-    );
-  });
-}