Repository: yannickcr/eslint-plugin-react (js)

Issue:
`destructuring-assignment` shouldn't trigger on method calls
The rule `react/destructuring-assignment` is also triggered when using methods of params.

```jsx
      formatter: (val) => {
        return <span>{val.toPrecision(2)}</span>;
      },
```


<img width="706" alt="image" src="https://user-images.githubusercontent.com/31949290/113500966-b3fb4800-953f-11eb-970f-48328ac251f1.png">


Might also be related to #1950

Pull request description:
Improve component detection
+ Added [estraverse](https://github.com/estools/estraverse) as dependency.
+ Implemented function `traverseReturns` using `estraverse` to improve return value analysis.
+ Rewrote `isReturningJSX` function to handle return statements nested inside other structures (`IfConditions`, `SwitchStatements`, etc)
+ This should also fix #2960

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 16a472d6..5d08a776 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,6 +5,11 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
 
 ## Unreleased
 
+### Fixed
+*  component detection: use `estraverse` to improve component detection ([#2992][] @Wesitos)
+
+[#2992]: https://github.com/yannickcr/eslint-plugin-react/pull/2992
+
 ## [7.24.0] - 2021.05.27
 
 ### Added
diff --git a/lib/rules/no-typos.js b/lib/rules/no-typos.js
index 4352a51d..052d63f4 100644
--- a/lib/rules/no-typos.js
+++ b/lib/rules/no-typos.js
@@ -241,8 +241,9 @@ module.exports = {
 
         if (
           relatedComponent
-          && (utils.isES6Component(relatedComponent.node) || utils.isReturningJSX(relatedComponent.node))
-          && (node.parent && node.parent.type === 'AssignmentExpression' && node.parent.right)
+            && (utils.isES6Component(relatedComponent.node) || (
+              relatedComponent.node.type !== 'ClassDeclaration' && utils.isReturningJSX(relatedComponent.node)))
+            && (node.parent && node.parent.type === 'AssignmentExpression' && node.parent.right)
         ) {
           reportErrorIfPropertyCasingTypo(node.parent.right, node.property, true);
         }
diff --git a/lib/util/Components.js b/lib/util/Components.js
index 27dd09c0..37761cc5 100644
--- a/lib/util/Components.js
+++ b/lib/util/Components.js
@@ -47,36 +47,6 @@ function mergeUsedPropTypes(propsList, newPropsList) {
   return propsList.concat(propsToAdd);
 }
 
-function isReturnsConditionalJSX(node, property, strict) {
-  const returnsConditionalJSXConsequent = node[property]
-    && node[property].type === 'ConditionalExpression'
-    && jsxUtil.isJSX(node[property].consequent);
-  const returnsConditionalJSXAlternate = node[property]
-    && node[property].type === 'ConditionalExpression'
-    && jsxUtil.isJSX(node[property].alternate);
-  return strict
-    ? (returnsConditionalJSXConsequent && returnsConditionalJSXAlternate)
-    : (returnsConditionalJSXConsequent || returnsConditionalJSXAlternate);
-}
-
-function isReturnsLogicalJSX(node, property, strict) {
-  const returnsLogicalJSXLeft = node[property]
-    && node[property].type === 'LogicalExpression'
-    && jsxUtil.isJSX(node[property].left);
-  const returnsLogicalJSXRight = node[property]
-    && node[property].type === 'LogicalExpression'
-    && jsxUtil.isJSX(node[property].right);
-  return strict
-    ? (returnsLogicalJSXLeft && returnsLogicalJSXRight)
-    : (returnsLogicalJSXLeft || returnsLogicalJSXRight);
-}
-
-function isReturnsSequentialJSX(node, property) {
-  return node[property]
-    && node[property].type === 'SequenceExpression'
-    && jsxUtil.isJSX(node[property].expressions[node[property].expressions.length - 1]);
-}
-
 const Lists = new WeakMap();
 
 /**
@@ -466,65 +436,12 @@ function componentRule(rule, context) {
       };
     },
 
-    /**
-     * Check if the node is returning JSX
-     *
-     * @param {ASTNode} ASTnode The AST node being checked
-     * @param {Boolean} [strict] If true, in a ternary condition the node must return JSX in both cases
-     * @returns {Boolean} True if the node is returning JSX, false if not
-     */
-    isReturningJSX(ASTnode, strict) {
-      const nodeAndProperty = utils.getReturnPropertyAndNode(ASTnode);
-      const node = nodeAndProperty.node;
-      const property = nodeAndProperty.property;
-
-      if (!node) {
-        return false;
-      }
-
-      const returnsConditionalJSX = isReturnsConditionalJSX(node, property, strict);
-      const returnsLogicalJSX = isReturnsLogicalJSX(node, property, strict);
-      const returnsSequentialJSX = isReturnsSequentialJSX(node, property);
-
-      const returnsJSX = node[property] && jsxUtil.isJSX(node[property]);
-      const returnsPragmaCreateElement = this.isCreateElement(node[property]);
-
-      return !!(
-        returnsConditionalJSX
-        || returnsLogicalJSX
-        || returnsSequentialJSX
-        || returnsJSX
-        || returnsPragmaCreateElement
-      );
+    isReturningJSX(ASTNode, strict) {
+      return jsxUtil.isReturningJSX(this.isCreateElement.bind(this), ASTNode, strict, true);
     },
 
-    /**
-     * Check if the node is returning null
-     *
-     * @param {ASTNode} ASTnode The AST node being checked
-     * @returns {Boolean} True if the node is returning null, false if not
-     */
-    isReturningNull(ASTnode) {
-      const nodeAndProperty = utils.getReturnPropertyAndNode(ASTnode);
-      const property = nodeAndProperty.property;
-      const node = nodeAndProperty.node;
-
-      if (!node) {
-        return false;
-      }
-
-      return node[property] && node[property].value === null;
-    },
-
-    /**
-     * Check if the node is returning JSX or null
-     *
-     * @param {ASTNode} ASTNode The AST node being checked
-     * @param {Boolean} [strict] If true, in a ternary condition the node must return JSX in both cases
-     * @returns {Boolean} True if the node is returning JSX or null, false if not
-     */
     isReturningJSXOrNull(ASTNode, strict) {
-      return utils.isReturningJSX(ASTNode, strict) || utils.isReturningNull(ASTNode);
+      return jsxUtil.isReturningJSX(this.isCreateElement.bind(this), ASTNode, strict);
     },
 
     getPragmaComponentWrapper(node) {
@@ -734,9 +651,20 @@ function componentRule(rule, context) {
           return undefined;
         }
         if (utils.isInAllowedPositionForComponent(node) && utils.isReturningJSXOrNull(node)) {
-          if (!node.id || isFirstLetterCapitalized(node.id.name)) {
+          const isMethod = node.parent.type === 'Property' && node.parent.method;
+
+          if (isMethod && !isFirstLetterCapitalized(node.parent.key.name)) {
+            return utils.isReturningJSX(node) ? node : undefined;
+          }
+
+          if (node.id && isFirstLetterCapitalized(node.id.name)) {
             return node;
           }
+
+          if (!node.id) {
+            return node;
+          }
+
           return undefined;
         }
 
diff --git a/lib/util/ast.js b/lib/util/ast.js
index 96402f54..fb740f0b 100644
--- a/lib/util/ast.js
+++ b/lib/util/ast.js
@@ -4,6 +4,29 @@
 
 'use strict';
 
+const estraverse = require('estraverse');
+
+/**
+ * Wrapper for estraverse.traverse
+ *
+ * @param {ASTNode} ASTnode The AST node being checked
+ * @param {Object} visitor Visitor Object for estraverse
+ */
+function traverse(ASTnode, visitor) {
+  const opts = Object.assign({}, {
+    fallback(node) {
+      return Object.keys(node).filter((key) => key === 'children' || key === 'argument');
+    }
+  }, visitor);
+
+  opts.keys = Object.assign({}, visitor.keys, {
+    JSXElement: ['children'],
+    JSXFragment: ['children']
+  });
+
+  estraverse.traverse(ASTnode, opts);
+}
+
 /**
  * Find a return statment in the current node
  *
@@ -37,6 +60,49 @@ function findReturnStatement(node) {
   }(bodyNodes));
 }
 
+/**
+ * Helper function for traversing "returns" (return statements or the
+ * returned expression in the case of an arrow function) of a function
+ *
+ * @param {ASTNode} ASTNode The AST node being checked
+ * @param {function} enterFunc Function to execute for each returnStatement found
+ * @returns {undefined}
+ */
+function traverseReturns(ASTNode, enterFunc) {
+  const nodeType = ASTNode.type;
+
+  if (nodeType === 'ReturnStatement') {
+    return enterFunc(ASTNode);
+  }
+
+  if (nodeType === 'ArrowFunctionExpression' && ASTNode.expression) {
+    return enterFunc(ASTNode.body);
+  }
+
+  if (nodeType !== 'FunctionExpression'
+      && nodeType !== 'FunctionDeclaration'
+      && nodeType !== 'ArrowFunctionExpression'
+      && nodeType !== 'MethodDefinition'
+  ) {
+    throw new TypeError('only function nodes are expected');
+  }
+
+  traverse(ASTNode.body, {
+    enter(node) {
+      switch (node.type) {
+        case 'ReturnStatement':
+          this.skip();
+          return enterFunc(node);
+        case 'FunctionExpression':
+        case 'FunctionDeclaration':
+        case 'ArrowFunctionExpression':
+          return this.skip();
+        default:
+      }
+    }
+  });
+}
+
 /**
  * Get node with property's name
  * @param {Object} node - Property.
@@ -268,6 +334,7 @@ function isTSTypeParameterInstantiation(node) {
 }
 
 module.exports = {
+  traverse,
   findReturnStatement,
   getFirstNodeInLine,
   getPropertyName,
@@ -280,6 +347,7 @@ module.exports = {
   isFunctionLikeExpression,
   isNodeFirstInLine,
   unwrapTSAsExpression,
+  traverseReturns,
   isTSTypeReference,
   isTSTypeAnnotation,
   isTSTypeLiteral,
diff --git a/lib/util/jsx.js b/lib/util/jsx.js
index 363ea8d6..bd01c969 100644
--- a/lib/util/jsx.js
+++ b/lib/util/jsx.js
@@ -4,8 +4,11 @@
 
 'use strict';
 
+const estraverse = require('estraverse');
 const elementType = require('jsx-ast-utils/elementType');
 
+const astUtil = require('./ast');
+
 // See https://github.com/babel/babel/blob/ce420ba51c68591e057696ef43e028f41c6e04cd/packages/babel-types/src/validators/react/isCompatTag.js
 // for why we only test for the first character
 const COMPAT_TAG_REGEX = /^[a-z]/;
@@ -79,10 +82,75 @@ function isWhiteSpaces(value) {
   return typeof value === 'string' ? /^\s*$/.test(value) : false;
 }
 
+/**
+ * Check if the node is returning JSX or null
+ *
+ * @param {Function} isCreateElement Function to determine if a CallExpresion is
+ *   a createElement one
+ * @param {ASTNode} ASTnode The AST node being checked
+ * @param {Boolean} [strict] If true, in a ternary condition the node must return JSX in both cases
+ * @param {Boolean} [ignoreNull] If true, null return values will be ignored
+ * @returns {Boolean} True if the node is returning JSX or null, false if not
+ */
+function isReturningJSX(isCreateElement, ASTnode, strict, ignoreNull) {
+  let found = false;
+  astUtil.traverseReturns(ASTnode, (node) => {
+    // Traverse return statement
+    astUtil.traverse(node, {
+      enter(childNode) {
+        const setFound = () => {
+          found = true;
+          this.skip();
+        };
+        switch (childNode.type) {
+          case 'FunctionExpression':
+          case 'FunctionDeclaration':
+          case 'ArrowFunctionExpression':
+            // Do not traverse into inner function definitions
+            return this.skip();
+          case 'ConditionalExpression':
+            if (!strict) break;
+            if (isJSX(childNode.consequent) && isJSX(childNode.alternate)) {
+              setFound();
+            }
+            this.skip();
+            break;
+          case 'LogicalExpression':
+            if (!strict) break;
+            if (isJSX(childNode.left) && isJSX(childNode.right)) {
+              setFound();
+            }
+            this.skip();
+            break;
+          case 'JSXElement':
+          case 'JSXFragment':
+            setFound(); break;
+          case 'CallExpression':
+            if (isCreateElement(childNode)) {
+              setFound();
+            }
+            break;
+          case 'Literal':
+            if (!ignoreNull && childNode.value === null) {
+              setFound();
+            }
+            break;
+          default:
+        }
+      }
+    });
+
+    return found && estraverse.VisitorOption.Break;
+  });
+
+  return found;
+}
+
 module.exports = {
   isDOMComponent,
   isFragment,
   isJSX,
   isJSXAttributeKey,
-  isWhiteSpaces
+  isWhiteSpaces,
+  isReturningJSX
 };
diff --git a/package.json b/package.json
index 56bb9842..e77a9b09 100644
--- a/package.json
+++ b/package.json
@@ -31,6 +31,7 @@
     "array-includes": "^3.1.3",
     "array.prototype.flatmap": "^1.2.4",
     "doctrine": "^2.1.0",
+    "estraverse": "^5.2.0",
     "has": "^1.0.3",
     "jsx-ast-utils": "^2.4.1 || ^3.0.0",
     "minimatch": "^3.0.4",


Test patch:
diff --git a/tests/lib/rules/destructuring-assignment.js b/tests/lib/rules/destructuring-assignment.js
index a62e3a31..b1b5a333 100644
--- a/tests/lib/rules/destructuring-assignment.js
+++ b/tests/lib/rules/destructuring-assignment.js
@@ -187,6 +187,15 @@ ruleTester.run('destructuring-assignment', rule, {
     `,
     options: ['always'],
     parser: parsers.BABEL_ESLINT
+  }, {
+    code: `
+      const obj = {
+        foo(arg) {
+          const a = arg.func();
+          return null;
+       },
+      };
+     `
   }],
 
   invalid: [{
diff --git a/tests/util/ast.js b/tests/util/ast.js
new file mode 100644
index 00000000..ca110b5d
--- /dev/null
+++ b/tests/util/ast.js
@@ -0,0 +1,102 @@
+'use strict';
+
+const assert = require('assert');
+const sinon = require('sinon');
+const espree = require('espree');
+
+const ast = require('../../lib/util/ast');
+
+const traverseReturns = ast.traverseReturns;
+
+const DEFAULT_CONFIG = {
+  ecmaVersion: 6
+};
+
+const parseCode = (code) => {
+  const ASTnode = espree.parse(code, DEFAULT_CONFIG);
+  // Return only first statement
+  return ASTnode.body[0];
+};
+
+describe('ast', () => {
+  describe('traverseReturnStatements', () => {
+    it('Correctly traverses function declarations', () => {
+      const spy = sinon.spy();
+      traverseReturns(parseCode(`
+        function foo({prop}) {
+          return;
+        }
+      `), spy);
+
+      assert(spy.calledOnce);
+    });
+
+    it('Correctly traverses function expressions', () => {
+      const spy = sinon.spy();
+      traverseReturns(parseCode(`
+        const foo = function({prop}) {
+          return;
+        }
+      `).declarations[0].init, spy);
+
+      assert(spy.calledOnce);
+    });
+
+    it('Correctly traverses arrow functions', () => {
+      const spy = sinon.spy();
+      traverseReturns(parseCode(`
+        ({prop}) => {
+          return;
+        }
+      `).expression, spy);
+
+      assert(spy.calledOnce);
+
+      spy.resetHistory();
+
+      traverseReturns(parseCode(`
+        ({prop}) => 'someething'
+      `).expression, spy);
+
+      assert(spy.calledOnce);
+    });
+
+    it('Correctly traverses inside control flow expressions', () => {
+      const spy = sinon.spy();
+      traverseReturns(parseCode(`
+        function foo({prop}) {
+          if (prop) {
+            return 0;
+          } else {
+            return 1;
+          }
+
+          while(prop) {
+            return 2;
+          }
+
+          for (;;) {
+            return 3;
+          }
+
+          switch (prop) {
+            case 'a':
+              return 4;
+            default:
+              return 5;
+          }
+
+          const foo = () => 'not valid';
+        }
+      `), spy);
+
+      const enterCalls = spy.getCalls();
+
+      assert.strictEqual(enterCalls.length, 6);
+
+      enterCalls.forEach((node, idx) => {
+        assert.strictEqual(node.lastArg.argument.value, idx);
+      });
+    });
+  });
+});
diff --git a/tests/util/jsx.js b/tests/util/jsx.js
new file mode 100644
index 00000000..9f6df970
--- /dev/null
+++ b/tests/util/jsx.js
@@ -0,0 +1,79 @@
+'use strict';
+
+const assert = require('assert');
+const espree = require('espree');
+
+const jsxUtil = require('../../lib/util/jsx');
+
+const isReturningJSX = jsxUtil.isReturningJSX;
+
+const DEFAULT_CONFIG = {
+  ecmaVersion: 6,
+  ecmaFeatures: {
+    jsx: true
+  }
+};
+
+const parseCode = (code) => {
+  const ASTnode = espree.parse(code, DEFAULT_CONFIG);
+  // Return only first statement
+  return ASTnode.body[0];
+};
+
+describe('jsxUtil', () => {
+  describe('isReturningJSX', () => {
+    const assertValid = (codeStr) => assert(
+      isReturningJSX(() => false, parseCode(codeStr))
+    );
+    const assertInValid = (codeStr) => assert(
+      !!isReturningJSX(() => false, parseCode(codeStr))
+    );
+    it('Works when returning JSX', () => {
+      assertValid(`
+        function Test() {
+          return (
+            <a>something</a>
+          )
+        }
+      `);
+
+      assertValid(`
+        function Test() {
+          return <a>something</a>;
+        }
+      `);
+    });
+
+    it('Works when returning null', () => {
+      assertValid(`
+        function Test() {
+          return null;
+        }
+      `);
+
+      assertValid(`
+        function Test({prop}) {
+          return prop || null;
+        }
+      `);
+    });
+
+    it('Works with nested return', () => {
+      assertValid(`
+        function Test({prop}) {
+          if (prop) {
+            return <a>something</a>
+          }
+        }
+      `);
+    });
+
+    it('Can ignore null', () => {
+      assertInValid(`
+        function Test() {
+          return null;
+        }
+      `);
+    });
+  });
+});