repl: improve REPL disabling completion on proxies and getters

#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

PR-URL: #58891
Reviewed-By: James M Snell <[email protected]>

Author: dario-piotrowicz

lib/repl.js

@@ -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) => {

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

@@ -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"].', []],
         ]);
     });
   });