diff --git a/CHANGELOG.md b/CHANGELOG.md index 16a472d6a2..5d08a7762a 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 4352a51d94..052d63f48e 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 27dd09c0a6..37761cc556 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 96402f5422..fb740f0b72 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 363ea8d609..bd01c969be 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 56bb9842d8..e77a9b09e1 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", diff --git a/tests/lib/rules/destructuring-assignment.js b/tests/lib/rules/destructuring-assignment.js index a62e3a31cc..b1b5a333a1 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 0000000000..ca110b5d13 --- /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 0000000000..9f6df9703b --- /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 ( + something + ) + } + `); + + assertValid(` + function Test() { + return something; + } + `); + }); + + 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 something + } + } + `); + }); + + it('Can ignore null', () => { + assertInValid(` + function Test() { + return null; + } + `); + }); + }); +});