fix(no-nesting): nested references vars in closure (#361)

eslint-community · Sep 29, 2022 · 08052e8 · 08052e8
1 parent ac98c63
commit 08052e8
Show file tree

Hide file tree

Showing 3 changed files with 140 additions and 2 deletions.
diff --git a/__tests__/no-nesting.js b/__tests__/no-nesting.js
@@ -45,6 +45,17 @@ ruleTester.run('no-nesting', rule, {
     'doThing().then(function() { return Promise.resolve(4) })',
     'doThing().then(() => Promise.resolve(4))',
     'doThing().then(() => Promise.all([a]))',
+
+    // references vars in closure
+    `doThing()
+      .then(a => getB(a)
+        .then(b => getC(a, b))
+      )`,
+    `doThing()
+      .then(a => {
+        const c = a * 2;
+        return getB(c).then(b => getC(c, b))
+      })`,
   ],
 
   invalid: [
@@ -80,5 +91,24 @@ ruleTester.run('no-nesting', rule, {
       code: 'doThing().then(() => b.catch())',
       errors: [{ message: errorMessage }],
     },
+    // references vars in closure
+    {
+      code: `
+      doThing()
+        .then(a => getB(a)
+          .then(b => getC(b))
+        )`,
+      errors: [{ message: errorMessage, line: 4 }],
+    },
+    {
+      code: `
+      doThing()
+        .then(a => getB(a)
+          .then(b => getC(a, b)
+            .then(c => getD(a, c))
+          )
+        )`,
+      errors: [{ message: errorMessage, line: 5 }],
+    },
   ],
 })
diff --git a/rules/lib/has-promise-callback.js b/rules/lib/has-promise-callback.js
@@ -6,6 +6,26 @@
 
 'use strict'
 
+/**
+ * @typedef {import('estree').SimpleCallExpression} CallExpression
+ * @typedef {import('estree').MemberExpression} MemberExpression
+ * @typedef {import('estree').Identifier} Identifier
+ *
+ * @typedef {object} NameIsThenOrCatch
+ * @property {'then' | 'catch'} name
+ *
+ * @typedef {object} PropertyIsThenOrCatch
+ * @property {Identifier & NameIsThenOrCatch} property
+ *
+ * @typedef {object} CalleeIsPromiseCallback
+ * @property {MemberExpression & PropertyIsThenOrCatch} callee
+ *
+ * @typedef {CallExpression & CalleeIsPromiseCallback} HasPromiseCallback
+ */
+/**
+ * @param {import('estree').Node} node
+ * @returns {node is HasPromiseCallback}
+ */
 function hasPromiseCallback(node) {
   // istanbul ignore if -- only being called within `CallExpression`
   if (node.type !== 'CallExpression') return

diff --git a/rules/no-nesting.js b/rules/no-nesting.js
@@ -18,12 +18,100 @@ module.exports = {
     schema: [],
   },
   create(context) {
+    /**
+     * Array of callback function scopes.
+     * Scopes are in order closest to the current node.
+     * @type {import('eslint').Scope.Scope[]}
+     */
+    const callbackScopes = []
+
+    /**
+     * @param {import('eslint').Scope.Scope} scope
+     * @returns {Iterable<import('eslint').Scope.Reference>}
+     */
+    function* iterateDefinedReferences(scope) {
+      for (const variable of scope.variables) {
+        for (const reference of variable.references) {
+          yield reference
+        }
+      }
+    }
+
     return {
+      ':function'(node) {
+        if (isInsidePromise(node)) {
+          callbackScopes.unshift(context.getScope())
+        }
+      },
+      ':function:exit'(node) {
+        if (isInsidePromise(node)) {
+          callbackScopes.shift()
+        }
+      },
       CallExpression(node) {
         if (!hasPromiseCallback(node)) return
-        if (context.getAncestors().some(isInsidePromise)) {
-          context.report({ node, message: 'Avoid nesting promises.' })
+        if (!callbackScopes.length) {
+          // The node is not in the callback function.
+          return
         }
+
+        // Checks if the argument callback uses variables defined in the closest callback function scope.
+        //
+        // e.g.
+        // ```
+        // doThing()
+        //  .then(a => getB(a)
+        //    .then(b => getC(a, b))
+        //  )
+        // ```
+        //
+        // In the above case, Since the variables it references are undef,
+        // we cannot refactor the nesting like following:
+        // ```
+        // doThing()
+        //  .then(a => getB(a))
+        //  .then(b => getC(a, b))
+        // ```
+        //
+        // However, `getD` can be refactored in the following:
+        // ```
+        // doThing()
+        //   .then(a => getB(a)
+        //     .then(b => getC(a, b)
+        //       .then(c => getD(a, c))
+        //     )
+        //   )
+        // ```
+        // ↓
+        // ```
+        // doThing()
+        //   .then(a => getB(a)
+        //     .then(b => getC(a, b))
+        //     .then(c => getD(a, c))
+        //   )
+        // ```
+        // This is why we only check the closest callback function scope.
+        //
+        const closestCallbackScope = callbackScopes[0]
+        for (const reference of iterateDefinedReferences(
+          closestCallbackScope
+        )) {
+          if (
+            node.arguments.some(
+              (arg) =>
+                arg.range[0] <= reference.identifier.range[0] &&
+                reference.identifier.range[1] <= arg.range[1]
+            )
+          ) {
+            // Argument callbacks refer to variables defined in the callback function.
+            return
+          }
+        }
+
+        context.report({
+          node: node.callee.property,
+          message: 'Avoid nesting promises.',
+        })
       },
     }
   },