Repository: nodejs/node
Pull request: repl: improve REPL disabling completion on proxies and getters

https://github.com/nodejs/node/pull/57909 introduced the disabling of REPL tab completion on object containing proxies and getters (since such completion triggers code evaluation which can be unexpected/disruptive for the user)

the solution in 57909 did not address all possible such cases, the changes here improve on such solution by using acorn and AST analysis to cover most if not all possible cases

<!--
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/repl.js
patch: @@ -1523,8 +1523,24 @@ function complete(line, callback) {
       return;
     }
 
+    // If the target ends with a dot (e.g. `obj.foo.`) such code won't be valid for AST parsing
+    // so in order to make it correct we add an identifier to its end (e.g. `obj.foo.x`)
+    const parsableCompleteTarget = completeTarget.endsWith('.') ? `${completeTarget}x` : completeTarget;
+
+    let completeTargetAst;
+    try {
+      completeTargetAst = acornParse(
+        parsableCompleteTarget, { __proto__: null, sourceType: 'module', ecmaVersion: 'latest' },
+      );
+    } catch { /* No need to specifically handle parse errors */ }
+
+    if (!completeTargetAst) {
+      return completionGroupsLoaded();
+    }
+
     return includesProxiesOrGetters(
-      StringPrototypeSplit(completeTarget, '.'),
+      completeTargetAst.body[0].expression,
+      parsableCompleteTarget,
       this.eval,
       this.context,
       (includes) => {
@@ -1729,32 +1745,156 @@ function findExpressionCompleteTarget(code) {
   return code;
 }
 
-function includesProxiesOrGetters(exprSegments, evalFn, context, callback, currentExpr = '', idx = 0) {
-  const currentSegment = exprSegments[idx];
-  currentExpr += `${currentExpr.length === 0 ? '' : '.'}${currentSegment}`;
-  evalFn(`try { ${currentExpr} } catch { }`, context, getREPLResourceName(), (_, currentObj) => {
-    if (typeof currentObj !== 'object' || currentObj === null) {
-      return callback(false);
-    }
+/**
+ * Utility used to determine if an expression includes object getters or proxies.
+ *
+ * Example: given `obj.foo`, the function lets you know if `foo` has a getter function
+ * associated to it, or if `obj` is a proxy
+ * @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
+ *        Eval function to use
+ * @param {any} ctx The context to use for any code evaluation
+ * @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) {
+  if (expr?.type !== 'MemberExpression') {
+    // If the expression is not a member one for obvious reasons no getters are involved
+    return callback(false);
+  }
 
-    if (isProxy(currentObj)) {
-      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(
+      expr.object,
+      exprStr.slice(0, expr.object.end),
+      evalFn,
+      ctx,
+      (includes, lastEvaledObj) => {
+        if (includes) {
+          // If the recurred call found a getter we can also terminate
+          return callback(includes);
+        }
+
+        if (isProxy(lastEvaledObj)) {
+          return callback(true);
+        }
+
+        // If a getter/proxy hasn't been found by the recursion call we need to check if maybe a getter/proxy
+        // is present here (e.g. in `obj.foo.bar` we found that `obj.foo` doesn't involve any getters so we now
+        // need to check if `bar` on `obj.foo` (i.e. `lastEvaledObj`) has a getter or if `obj.foo.bar` is a proxy)
+        return hasGetterOrIsProxy(lastEvaledObj, expr.property, (doesHaveGetterOrIsProxy) => {
+          return callback(doesHaveGetterOrIsProxy);
+        });
+      },
+    );
+  }
+
+  // This is the base of the recursion we have an identifier for the object and an identifier or literal
+  // for the property (e.g. we have `obj.foo` or `obj['foo']`, `obj` is the object identifier and `foo`
+  // is the property identifier/literal)
+  if (expr.object.type === 'Identifier') {
+    return evalFn(`try { ${expr.object.name} } catch {}`, ctx, getREPLResourceName(), (err, obj) => {
+      if (err) {
+        return callback(false);
+      }
+
+      if (isProxy(obj)) {
+        return callback(true);
+      }
+
+      return hasGetterOrIsProxy(obj, expr.property, (doesHaveGetterOrIsProxy) => {
+        if (doesHaveGetterOrIsProxy) {
+          return callback(true);
+        }
+
+        return evalFn(
+          `try { ${exprStr} } catch {} `, ctx, getREPLResourceName(), (err, obj) => {
+            if (err) {
+              return callback(false);
+            }
+            return callback(false, obj);
+          });
+      });
+    });
+  }
+
+  /**
+   * Utility to see if a property has a getter associated to it or if
+   * the property itself is a proxy object.
+   */
+  function hasGetterOrIsProxy(obj, astProp, cb) {
+    if (!obj || !astProp) {
+      return cb(false);
     }
 
-    const nextIdx = idx + 1;
+    if (astProp.type === 'Literal') {
+      // We have something like `obj['foo'].x` where `x` is the literal
 
-    if (nextIdx >= exprSegments.length) {
-      return callback(false);
+      if (isProxy(obj[astProp.value])) {
+        return cb(true);
+      }
+
+      const propDescriptor = ObjectGetOwnPropertyDescriptor(
+        obj,
+        `${astProp.value}`,
+      );
+      const propHasGetter = typeof propDescriptor?.get === 'function';
+      return cb(propHasGetter);
     }
 
-    const nextSegmentProp = ObjectGetOwnPropertyDescriptor(currentObj, exprSegments[nextIdx]);
-    const nextSegmentPropHasGetter = typeof nextSegmentProp?.get === 'function';
-    if (nextSegmentPropHasGetter) {
-      return callback(true);
+    if (
+      astProp.type === 'Identifier' &&
+      exprStr.at(astProp.start - 1) === '.'
+    ) {
+      // We have something like `obj.foo.x` where `foo` is the identifier
+
+      if (isProxy(obj[astProp.name])) {
+        return cb(true);
+      }
+
+      const propDescriptor = ObjectGetOwnPropertyDescriptor(
+        obj,
+        `${astProp.name}`,
+      );
+      const propHasGetter = typeof propDescriptor?.get === 'function';
+      return cb(propHasGetter);
     }
 
-    return includesProxiesOrGetters(exprSegments, evalFn, context, callback, currentExpr, nextIdx);
-  });
+    return evalFn(
+      // Note: this eval runs the property expression, which might be side-effectful, for example
+      //       the user could be running `obj[getKey()].` where `getKey()` has some side effects.
+      //       Arguably this behavior should not be too surprising, but if it turns out that it is,
+      //       then we can revisit this behavior and add logic to analyze the property expression
+      //       and eval it only if we can confidently say that it can't have any side effects
+      `try { ${exprStr.slice(astProp.start, astProp.end)} } catch {} `,
+      ctx,
+      getREPLResourceName(),
+      (err, evaledProp) => {
+        if (err) {
+          return callback(false);
+        }
+
+        if (typeof evaledProp === 'string') {
+          if (isProxy(obj[evaledProp])) {
+            return cb(true);
+          }
+
+          const propDescriptor = ObjectGetOwnPropertyDescriptor(
+            obj,
+            evaledProp,
+          );
+          const propHasGetter = typeof propDescriptor?.get === 'function';
+          return cb(propHasGetter);
+        }
+
+        return callback(false);
+      },
+    );
+  }
+
+  return callback(false);
 }
 
 REPLServer.prototype.completeOnEditorMode = (callback) => (err, results) => {

filename: test/parallel/test-repl-completion-on-getters-disabled.js
patch: @@ -61,48 +61,102 @@ describe('REPL completion in relation of getters', () => {
     test(`completions are generated for properties that don't trigger getters`, () => {
       runCompletionTests(
         `
+        function getFooKey() {
+          return "foo";
+        }
+
+        const fooKey = "foo";
+
+        const keys = {
+          "foo key": "foo",
+        };
+
         const objWithGetters = {
-          foo: { bar: { baz: {} }, get gBar() { return { baz: {} } } },
+          foo: { bar: { baz: { buz: {} } }, get gBar() { return { baz: {} } } },
           get gFoo() { return { bar: { baz: {} } }; }
         };
         `, [
           ['objWithGetters.', ['objWithGetters.foo']],
           ['objWithGetters.f', ['objWithGetters.foo']],
           ['objWithGetters.foo', ['objWithGetters.foo']],
+          ['objWithGetters["foo"].b', ['objWithGetters["foo"].bar']],
           ['objWithGetters.foo.', ['objWithGetters.foo.bar']],
           ['objWithGetters.foo.bar.b', ['objWithGetters.foo.bar.baz']],
           ['objWithGetters.gFo', ['objWithGetters.gFoo']],
           ['objWithGetters.foo.gB', ['objWithGetters.foo.gBar']],
+          ["objWithGetters.foo['bar'].b", ["objWithGetters.foo['bar'].baz"]],
+          ["objWithGetters['foo']['bar'].b", ["objWithGetters['foo']['bar'].baz"]],
+          ["objWithGetters['foo']['bar']['baz'].b", ["objWithGetters['foo']['bar']['baz'].buz"]],
+          ["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('no completions are generated for properties that trigger getters', () => {
       runCompletionTests(
         `
+        function getGFooKey() {
+          return "g" + "Foo";
+        }
+
+        const gFooKey = "gFoo";
+
+        const keys = {
+          "g-foo key": "gFoo",
+        };
+
         const objWithGetters = {
-          foo: { bar: { baz: {} }, get gBar() { return { baz: {} } } },
+          foo: { bar: { baz: {} }, get gBar() { return { baz: {}, get gBuz() { return 5; } } } },
           get gFoo() { return { bar: { baz: {} } }; }
         };
         `,
         [
           ['objWithGetters.gFoo.', []],
           ['objWithGetters.gFoo.b', []],
+          ['objWithGetters["gFoo"].b', []],
           ['objWithGetters.gFoo.bar.b', []],
           ['objWithGetters.foo.gBar.', []],
           ['objWithGetters.foo.gBar.b', []],
+          ["objWithGetters.foo['gBar'].b", []],
+          ["objWithGetters['foo']['gBar'].b", []],
+          ["objWithGetters['foo']['gBar']['gBuz'].", []],
+          ["objWithGetters[keys['g-foo key']].b", []],
+          ['objWithGetters[gFooKey].b', []],
+          ["objWithGetters['g' + 'Foo'].b", []],
+          ['objWithGetters[getGFooKey()].b', []],
         ]);
     });
   });
 
   describe('completions on proxies', () => {
     test('no completions are generated for a proxy object', () => {
-      runCompletionTests('const proxyObj = new Proxy({ foo: { bar: { baz: {} } } }, {});', [
-        ['proxyObj.', []],
-        ['proxyObj.f', []],
-        ['proxyObj.foo', []],
-        ['proxyObj.foo.', []],
-        ['proxyObj.foo.bar.b', []],
-      ]);
+      runCompletionTests(
+        `
+        function getFooKey() {
+          return "foo";
+        }
+
+        const fooKey = "foo";
+
+        const keys = {
+          "foo key": "foo",
+        };
+
+        const proxyObj = new Proxy({ foo: { bar: { baz: {} } } }, {});
+        `, [
+          ['proxyObj.', []],
+          ['proxyObj.f', []],
+          ['proxyObj.foo', []],
+          ['proxyObj.foo.', []],
+          ['proxyObj.["foo"].', []],
+          ['proxyObj.["f" + "oo"].', []],
+          ['proxyObj.[fooKey].', []],
+          ['proxyObj.[getFooKey()].', []],
+          ['proxyObj.[keys["foo key"]].', []],
+          ['proxyObj.foo.bar.b', []],
+        ]);
     });
 
     test('no completions are generated for a proxy present in a standard object', () => {
@@ -113,6 +167,7 @@ describe('REPL completion in relation of getters', () => {
           ['objWithProxy.foo.', ['objWithProxy.foo.bar']],
           ['objWithProxy.foo.b', ['objWithProxy.foo.bar']],
           ['objWithProxy.foo.bar.', []],
+          ['objWithProxy.foo["b" + "ar"].', []],
         ]);
     });
   });