From d30eed4d82434441498a7e67219433ba47130bce Mon Sep 17 00:00:00 2001 From: Clay Branch Date: Tue, 28 Nov 2017 14:52:14 -0600 Subject: [PATCH 1/5] errors are now successfully reported on all valid forms of destructuring except for MemberExpressions outside of VariableDeclarators --- lib/rules/prop-types.js | 143 ++++++++++++++++--------- tests/lib/rules/prop-types-specific.js | 55 ++++++++++ tests/lib/rules/prop-types.js | 28 +++-- 3 files changed, 170 insertions(+), 56 deletions(-) create mode 100644 tests/lib/rules/prop-types-specific.js diff --git a/lib/rules/prop-types.js b/lib/rules/prop-types.js index 16d09d8e54..3a4482dc88 100644 --- a/lib/rules/prop-types.js +++ b/lib/rules/prop-types.js @@ -241,6 +241,7 @@ module.exports = { if (typeof propType === 'object' && Object.keys(propType).length === 0) { return true; } + // Consider every children as declared if (propType.children === true) { return true; @@ -346,11 +347,22 @@ module.exports = { const key = getKeyValue(node); const value = node.value; - fn(key, value); + fn(key, value, node); } } } + function traverseProperties(properties, fn, keys) { + keys = keys || [] + iterateProperties(properties, (key, value, property) => { + const updatedKeys = keys.concat(key) + fn(key, value, property, updatedKeys) + if (value && value.properties) { + traverseProperties(value.properties, fn, updatedKeys) + } + }) + } + /** * Creates the representation of the React propTypes for the component. * The representation is used to verify nested used properties. @@ -568,9 +580,29 @@ module.exports = { return void 0; } + /** + * get the allNames array for a given property + * @param {ASTNode} property a property node. + */ + function getAllNamesForMemberExpression(expression, parentNames) { + parentNames = parentNames || [] + const propertyName = expression.property && expression.property.name + const nameToAdd = propertyName || + (expression.type === 'ThisExpression' && 'this') || + (expression.name) + const childNames = nameToAdd ? + parentNames.concat(nameToAdd) + : parentNames + if (expression.object) { + return getAllNamesForMemberExpression(expression.object, parentNames).concat(childNames) + } + return childNames + } + + /** * Mark a prop type as used - * @param {ASTNode} node The AST node being marked. + * @param {ASTNode} properties The AST node being marked. */ function markPropTypesAsUsed(node, parentNames) { parentNames = parentNames || []; @@ -578,6 +610,9 @@ module.exports = { let name; let allNames; let properties; + let parentUsage; + const component = components.get(utils.getParentComponent()); + const usedPropTypes = (component && component.usedPropTypes || []).slice(); switch (node.type) { case 'MemberExpression': name = getPropertyName(node); @@ -588,14 +623,6 @@ module.exports = { } // Do not mark computed props as used. type = name !== '__COMPUTED_PROP__' ? 'direct' : null; - } else if ( - node.parent.id && - node.parent.id.properties && - node.parent.id.properties.length && - getKeyValue(node.parent.id.properties[0]) - ) { - type = 'destructuring'; - properties = node.parent.id.properties; } break; case 'ArrowFunctionExpression': @@ -605,6 +632,26 @@ module.exports = { properties = node.params[0].properties; break; case 'VariableDeclarator': + // let {firstname} = this.props + const initNames = getAllNamesForMemberExpression(node.init) + const thisPropsDestructuring = ( + (utils.getParentES6Component() || utils.getParentES5Component()) && + initNames[0] === 'this' && PROPS_REGEX.test(initNames[1]) + ); + // let {firstname} = props + const directDestructuring = + PROPS_REGEX.test(initNames[0]) && + (utils.getParentStatelessComponent() || inConstructor() || inComponentWillReceiveProps()) + ; + + if (thisPropsDestructuring) { + allNames = initNames.slice(2) + } + + if (directDestructuring) { + allNames = initNames.slice(1) + } + for (let i = 0, j = node.id.properties.length; i < j; i++) { // let {props: {firstname}} = this const thisDestructuring = ( @@ -612,17 +659,20 @@ module.exports = { (PROPS_REGEX.test(node.id.properties[i].key.name) || PROPS_REGEX.test(node.id.properties[i].key.value)) && node.id.properties[i].value.type === 'ObjectPattern' ); - // let {firstname} = props - const directDestructuring = - PROPS_REGEX.test(node.init.name) && - (utils.getParentStatelessComponent() || inConstructor() || inComponentWillReceiveProps()) - ; - - if (thisDestructuring) { - properties = node.id.properties[i].value.properties; - } else if (directDestructuring) { - properties = node.id.properties; - } else { + + properties = thisDestructuring ? node.id.properties[i].value.properties : node.id.properties + if (!thisDestructuring || !directDestructuring || !thisPropsDestructuring) { + parentUsage = usedPropTypes.find( + propType => propType.name === initNames[0] + ) + + // let { a } = props + // let { b } = a + if (parentUsage) { + allNames = initNames.slice(1) + } + } + if (!thisDestructuring && !directDestructuring && !parentUsage && !thisPropsDestructuring) { continue; } type = 'destructuring'; @@ -633,8 +683,6 @@ module.exports = { throw new Error(`${node.type} ASTNodes are not handled by markPropTypesAsUsed`); } - const component = components.get(utils.getParentComponent()); - const usedPropTypes = (component && component.usedPropTypes || []).slice(); switch (type) { case 'direct': @@ -656,33 +704,30 @@ module.exports = { }); break; case 'destructuring': - for (let k = 0, l = properties.length; k < l; k++) { - if (hasSpreadOperator(properties[k]) || properties[k].computed) { - continue; - } - const propName = getKeyValue(properties[k]); - - let currentNode = node; - allNames = []; - while (currentNode.property && !PROPS_REGEX.test(currentNode.property.name)) { - allNames.unshift(currentNode.property.name); - currentNode = currentNode.object; + traverseProperties(properties, (key, value, property, allPropertyNames) => { + if (hasSpreadOperator(property) || property.computed) { + return; } - allNames.push(propName); + const propName = getKeyValue(property) + const propValueName = property.value.name + const isRenamedProperty = propName !== propValueName && property.value.type === 'Identifier' + allNames = allNames || [] if (propName) { + const allParentNames = parentUsage ? parentUsage.allNames.concat(allNames) : allNames usedPropTypes.push({ - name: propName, - allNames: allNames, - node: properties[k] + name: isRenamedProperty ? propValueName : propName, + allNames: allParentNames.concat(allPropertyNames), + node: property }); } - } + }) break; default: break; } + components.set(node, { usedPropTypes: usedPropTypes }); @@ -964,17 +1009,14 @@ module.exports = { }, VariableDeclarator: function(node) { - const destructuring = node.init && node.id && node.id.type === 'ObjectPattern'; - // let {props: {firstname}} = this - const thisDestructuring = destructuring && node.init.type === 'ThisExpression'; - // let {firstname} = props - const directDestructuring = - destructuring && - PROPS_REGEX.test(node.init.name) && - (utils.getParentStatelessComponent() || inConstructor() || inComponentWillReceiveProps()) - ; - - if (!thisDestructuring && !directDestructuring) { + // const isInClassComponent = utils.getParentES6Component() || utils.getParentES5Component(); + // const inComponent = utils.getParentStatelessComponent() || inConstructor() + // || inComponentWillReceiveProps() || isInClassComponent; + // const destructuringInComponent = + // && + // inComponent; + const destructuring = node.init && node.id && node.id.type === 'ObjectPattern' + if (!destructuring) { return; } markPropTypesAsUsed(node); @@ -1080,6 +1122,7 @@ module.exports = { if (!has(list, component) || !mustBeValidated(list[component])) { continue; } + console.log(list[component].usedPropTypes) reportUndeclaredPropTypes(list[component]); } } diff --git a/tests/lib/rules/prop-types-specific.js b/tests/lib/rules/prop-types-specific.js new file mode 100644 index 0000000000..930541a565 --- /dev/null +++ b/tests/lib/rules/prop-types-specific.js @@ -0,0 +1,55 @@ +/** + * @fileoverview Prevent missing props validation in a React component definition + * @author Yannick Croissant + */ +'use strict'; + +// ------------------------------------------------------------------------------ +// Requirements +// ------------------------------------------------------------------------------ + +const rule = require('../../../lib/rules/prop-types'); +const RuleTester = require('eslint').RuleTester; + +const parserOptions = { + ecmaVersion: 8, + sourceType: 'module', + ecmaFeatures: { + experimentalObjectRestSpread: true, + jsx: true + } +}; + +// const settings = { +// react: { +// pragma: 'Foo' +// } +// }; + +require('babel-eslint'); + +// ------------------------------------------------------------------------------ +// Tests +// ------------------------------------------------------------------------------ + +const ruleTester = new RuleTester({parserOptions}); +ruleTester.run('prop-types', rule, { + valid: [ + + ], + invalid: [ + { + code: ` + const Foo = ({ a }) => { + return
{a.c}
+ } + Foo.propTypes = { + a: PropTypes.object.shape({ + }), + } + `, + errors: [], + parser: 'babel-eslint' + } + ] +}); diff --git a/tests/lib/rules/prop-types.js b/tests/lib/rules/prop-types.js index 08153a711a..42985d3281 100644 --- a/tests/lib/rules/prop-types.js +++ b/tests/lib/rules/prop-types.js @@ -34,7 +34,7 @@ require('babel-eslint'); const ruleTester = new RuleTester({parserOptions}); ruleTester.run('prop-types', rule, { - + // valid: [], valid: [ { code: [ @@ -1854,14 +1854,13 @@ ruleTester.run('prop-types', rule, { } A.propTypes = { - a: React.PropTypes.string, - ...SharedPropTypes // eslint-disable-line object-shorthand - }; - `, + a: React.PropTypes.string, + ...SharedPropTypes // eslint-disable-line object-shorthand + }; + `, parser: 'babel-eslint' } ], - invalid: [ { code: [ @@ -3533,6 +3532,23 @@ ruleTester.run('prop-types', rule, { message: '\'fooBar\' is missing in props validation' }], parser: 'babel-eslint' + }, + { + code: ` + const Foo = ({ a: { b } }) => { + const { c } = b + return
test
+ } + Foo.propTypes = { + a: PropTypes.object.shape({ + b: PropTypes.object.shape({ + + }) + }), + } + `, + errors: [], + parser: 'babel-eslint' } ] }); From 81858de4e1676e7d5f0e10fe6dde42f90bc1645e Mon Sep 17 00:00:00 2001 From: Clay Branch Date: Thu, 7 Dec 2017 14:21:46 -0600 Subject: [PATCH 2/5] errors reported on MemberExpressions off of previously destructured variables --- lib/rules/prop-types.js | 142 ++++++++++++++++--------- lib/util/Components.js | 1 + tests/lib/rules/prop-types-specific.js | 13 +-- tests/lib/rules/prop-types.js | 102 +++++++++++++++++- 4 files changed, 194 insertions(+), 64 deletions(-) diff --git a/lib/rules/prop-types.js b/lib/rules/prop-types.js index 3a4482dc88..e34820f141 100644 --- a/lib/rules/prop-types.js +++ b/lib/rules/prop-types.js @@ -139,13 +139,39 @@ module.exports = { * @returns {Boolean} True if we are using a prop, false if not. */ function isPropTypesUsage(node) { - const isClassUsage = ( - (utils.getParentES6Component() || utils.getParentES5Component()) && - node.object.type === 'ThisExpression' && node.property.name === 'props' - ); - const isStatelessFunctionUsage = node.object.name === 'props' && !isAssignmentToProp(node); - const isNextPropsUsage = node.object.name === 'nextProps' && inComponentWillReceiveProps(); - return isClassUsage || isStatelessFunctionUsage || isNextPropsUsage; + // only return usages for bottom level member expression + if (node.object.type !== 'Identifier' && node.object.type !== 'ThisExpression') { + return false + } + const isInClassComponent = !!(utils.getParentES6Component() || utils.getParentES5Component()) + && node.object.name !== 'React'; + + const isDirectProp = DIRECT_PROPS_REGEX.test(sourceCode.getText(node)); + const isInConstructor = inConstructor(); + const isThisPropsUsage = node.object.type === 'ThisExpression' && node.property.name === 'props' + const isPropsUsage = node.object.name === 'props' + const isInStatelessComponent = !!utils.getParentStatelessComponent() && !isInClassComponent + if (isPropsUsage && !isInClassComponent && !isAssignmentToProp(node)) { + return true + } + // using props.foo in a class component, anywhere other than ComponentWillReceiveProps or a constructor + if (isInClassComponent && !isInConstructor && !inComponentWillReceiveProps() && isDirectProp) { + return false + } + if (isInStatelessComponent) { + if (isAssignmentToProp(node)) { + return false + } + if (isThisPropsUsage) { + return false + } + } + + if (!isInStatelessComponent && !isInClassComponent) { + return false + } + + return true } /** @@ -544,16 +570,6 @@ module.exports = { * @return {string} the name of the property or undefined if not found */ function getPropertyName(node) { - const isDirectProp = DIRECT_PROPS_REGEX.test(sourceCode.getText(node)); - const isInClassComponent = utils.getParentES6Component() || utils.getParentES5Component(); - const isNotInConstructor = !inConstructor(); - const isNotInComponentWillReceiveProps = !inComponentWillReceiveProps(); - if (isDirectProp && isInClassComponent && isNotInConstructor && isNotInComponentWillReceiveProps) { - return void 0; - } - if (!isDirectProp) { - node = node.parent; - } const property = node.property; if (property) { switch (property.type) { @@ -586,7 +602,7 @@ module.exports = { */ function getAllNamesForMemberExpression(expression, parentNames) { parentNames = parentNames || [] - const propertyName = expression.property && expression.property.name + const propertyName = getPropertyName(expression) const nameToAdd = propertyName || (expression.type === 'ThisExpression' && 'this') || (expression.name) @@ -599,31 +615,67 @@ module.exports = { return childNames } + function traverseMemberExpression(expression, func, parentNames) { + parentNames = parentNames || [] + const propertyName = getPropertyName(expression); + if (propertyName) { + const childNames = parentNames.concat(propertyName); + func(propertyName, childNames, expression.property) + if (expression.parent.type === 'MemberExpression') { + return parentNames.concat(traverseMemberExpression(expression.parent, func, childNames)); + } + } + return parentNames + } + /** * Mark a prop type as used * @param {ASTNode} properties The AST node being marked. */ - function markPropTypesAsUsed(node, parentNames) { - parentNames = parentNames || []; + function markPropTypesAsUsed(node) { let type; - let name; let allNames; let properties; let parentUsage; + let parentNames = [] const component = components.get(utils.getParentComponent()); const usedPropTypes = (component && component.usedPropTypes || []).slice(); switch (node.type) { case 'MemberExpression': - name = getPropertyName(node); - if (name) { - allNames = parentNames.concat(name); - if (node.parent.type === 'MemberExpression') { - markPropTypesAsUsed(node.parent, allNames); + const isThisProps = (node.object.type === 'ThisExpression' && node.property.name === 'props') + if (isThisProps && node.parent.type !== 'MemberExpression') { + break; + } + const nodeToTraverse = isThisProps ? node.parent : node; + const directlyOffOfProps = (nodeToTraverse.object.name === 'props' || nodeToTraverse.object.name === 'nextProps') + if (!directlyOffOfProps && !isThisProps) { + const lookupName = nodeToTraverse.object.name + parentUsage = usedPropTypes.find( + propType => propType.name === lookupName + ) + if (!parentUsage) { + break; } - // Do not mark computed props as used. - type = name !== '__COMPUTED_PROP__' ? 'direct' : null; + parentNames = parentUsage.allNames } + + traverseMemberExpression(nodeToTraverse, (propertyName, currentNames, currentNode) => { + // Ignore Object methods + if (Object.prototype[propertyName] || propertyName === '__COMPUTED_PROP__') { + return + } + + // Ignore Array methods + if (Array.prototype[propertyName]) { + return + } + usedPropTypes.push({ + name: propertyName, + allNames: currentNames, + node: currentNode + }); + }, parentNames) break; case 'ArrowFunctionExpression': case 'FunctionDeclaration': @@ -633,15 +685,17 @@ module.exports = { break; case 'VariableDeclarator': // let {firstname} = this.props + const isInClassComponent = (utils.getParentES6Component() || utils.getParentES5Component()) const initNames = getAllNamesForMemberExpression(node.init) + const isInStatelessComponent = !!utils.getParentStatelessComponent() & !isInClassComponent const thisPropsDestructuring = ( - (utils.getParentES6Component() || utils.getParentES5Component()) && + isInClassComponent && initNames[0] === 'this' && PROPS_REGEX.test(initNames[1]) ); // let {firstname} = props const directDestructuring = PROPS_REGEX.test(initNames[0]) && - (utils.getParentStatelessComponent() || inConstructor() || inComponentWillReceiveProps()) + (isInStatelessComponent || inConstructor() || inComponentWillReceiveProps()) ; if (thisPropsDestructuring) { @@ -685,24 +739,6 @@ module.exports = { switch (type) { - case 'direct': - // Ignore Object methods - if (Object.prototype[name]) { - break; - } - - const isDirectProp = DIRECT_PROPS_REGEX.test(sourceCode.getText(node)); - - usedPropTypes.push({ - name: name, - allNames: allNames, - node: ( - !isDirectProp && !inConstructor() && !inComponentWillReceiveProps() ? - node.parent.property : - node.property - ) - }); - break; case 'destructuring': traverseProperties(properties, (key, value, property, allPropertyNames) => { if (hasSpreadOperator(property) || property.computed) { @@ -727,7 +763,6 @@ module.exports = { break; } - components.set(node, { usedPropTypes: usedPropTypes }); @@ -1035,10 +1070,10 @@ module.exports = { MemberExpression: function(node) { let type; - if (isPropTypesUsage(node)) { - type = 'usage'; - } else if (propsUtil.isPropTypesDeclaration(node)) { + if (propsUtil.isPropTypesDeclaration(node)) { type = 'declaration'; + } else if (isPropTypesUsage(node)) { + type = 'usage'; } switch (type) { @@ -1122,7 +1157,8 @@ module.exports = { if (!has(list, component) || !mustBeValidated(list[component])) { continue; } - console.log(list[component].usedPropTypes) + + // console.log(list[component].usedPropTypes) reportUndeclaredPropTypes(list[component]); } } diff --git a/lib/util/Components.js b/lib/util/Components.js index 90ec38550f..3e83a66488 100644 --- a/lib/util/Components.js +++ b/lib/util/Components.js @@ -463,6 +463,7 @@ function componentRule(rule, context) { let scope = context.getScope(); while (scope) { const node = scope.block; + const isClass = node.type === 'ClassExpression'; const isFunction = /Function/.test(node.type); // Functions const isMethod = node.parent && node.parent.type === 'MethodDefinition'; // Classes methods diff --git a/tests/lib/rules/prop-types-specific.js b/tests/lib/rules/prop-types-specific.js index 930541a565..3dabcd6457 100644 --- a/tests/lib/rules/prop-types-specific.js +++ b/tests/lib/rules/prop-types-specific.js @@ -35,20 +35,21 @@ require('babel-eslint'); const ruleTester = new RuleTester({parserOptions}); ruleTester.run('prop-types', rule, { valid: [ - ], invalid: [ { code: ` - const Foo = ({ a }) => { - return
{a.c}
+ const Foo = ({ a: renamedA }) => { + const { b } = renamedA + return
test
} Foo.propTypes = { - a: PropTypes.object.shape({ - }), + a: PropTypes.object.shape({}), } `, - errors: [], + errors: [ + {message: '\'a.b\' is missing in props validation'} + ], parser: 'babel-eslint' } ] diff --git a/tests/lib/rules/prop-types.js b/tests/lib/rules/prop-types.js index 42985d3281..710129a333 100644 --- a/tests/lib/rules/prop-types.js +++ b/tests/lib/rules/prop-types.js @@ -2444,7 +2444,6 @@ ruleTester.run('prop-types', rule, { parser: 'babel-eslint', errors: [ {message: '\'names\' is missing in props validation'}, - {message: '\'names.map\' is missing in props validation'}, {message: '\'company\' is missing in props validation'} ] }, { @@ -3537,18 +3536,111 @@ ruleTester.run('prop-types', rule, { code: ` const Foo = ({ a: { b } }) => { const { c } = b - return
test
+ return
{c.d}
} Foo.propTypes = { a: PropTypes.object.shape({ b: PropTypes.object.shape({ - + c: PropTypes.object.shape({}) }) }), } `, - errors: [], + errors: [ + {message: '\'a.b.c.d\' is missing in props validation'} + ], parser: 'babel-eslint' - } + }, + { + code: ` + const Foo = props => { + const { ignore } = this.props + const { b } = props.a + return
{b}
+ } + Foo.propTypes = { + a: PropTypes.object.shape({}), + } + `, + errors: [ + {message: '\'a.b\' is missing in props validation'} + ], + parser: 'babel-eslint' + }, + { + code: ` + class Foo extends React.Component { + handler = () => { + const { handlerIgnore } = props + } + render = () => { + const { renderIgnore } = props + const { a } = this.props + const { c } = a.b + } + } + Foo.propTypes = { + a: PropTypes.object.shape({ + b: PropTypes.object.shape({}) + }), + } + `, + errors: [ + {message: '\'a.b.c\' is missing in props validation'} + ], + parser: 'babel-eslint' + }, + { + code: ` + class Foo extends React.Component { + componentWillReceiveProps = nextProps => { + const { b } = nextProps.a + const { d } = this.props.c + } + render = () => { + return
test
+ } + } + Foo.propTypes = { + a: PropTypes.object.shape({}), + c: PropTypes.object.shape({}), + } + `, + errors: [ + {message: '\'a.b\' is missing in props validation'}, + {message: '\'c.d\' is missing in props validation'} + ], + parser: 'babel-eslint' + }, + { + code: ` + const Foo = ({ a: renamedA }) => { + const { b } = renamedA + return
test
+ } + Foo.propTypes = { + a: PropTypes.object.shape({}), + } + `, + errors: [ + {message: '\'a.b\' is missing in props validation'} + ], + parser: 'babel-eslint' + }, + { + code: ` + const Foo = ({ a: renamedA }) => { + return
{renamedA.b}
+ } + Foo.propTypes = { + a: PropTypes.object.shape({}), + } + `, + errors: [ + {message: '\'a.b\' is missing in props validation'} + ], + parser: 'babel-eslint' + }, + ] }); From a52fec44744b0f313d9fec8d740c135b54d6bdbb Mon Sep 17 00:00:00 2001 From: Clay Branch Date: Tue, 12 Dec 2017 14:51:00 -0600 Subject: [PATCH 3/5] Removed duplicated coded, moved second switch statement in markPropsTypesAsUsed into its own function and simplified processing flow --- lib/rules/prop-types.js | 316 ++++++++++++++----------- tests/lib/rules/prop-types-specific.js | 56 ----- tests/lib/rules/prop-types.js | 3 +- 3 files changed, 178 insertions(+), 197 deletions(-) delete mode 100644 tests/lib/rules/prop-types-specific.js diff --git a/lib/rules/prop-types.js b/lib/rules/prop-types.js index e34820f141..274c799f68 100644 --- a/lib/rules/prop-types.js +++ b/lib/rules/prop-types.js @@ -135,43 +135,49 @@ module.exports = { /** * Checks if we are using a prop - * @param {ASTNode} node The AST node being checked. + * @param {ASTNode} node The MemberExpression node being checked. * @returns {Boolean} True if we are using a prop, false if not. */ function isPropTypesUsage(node) { - // only return usages for bottom level member expression + // only return usages for the MemberExpressions at the bottom of the tree if (node.object.type !== 'Identifier' && node.object.type !== 'ThisExpression') { - return false + return false; } + const isInClassComponent = !!(utils.getParentES6Component() || utils.getParentES5Component()) && node.object.name !== 'React'; + const isDirectProp = DIRECT_PROPS_REGEX.test(sourceCode.getText(node)); - const isInConstructor = inConstructor(); - const isThisPropsUsage = node.object.type === 'ThisExpression' && node.property.name === 'props' - const isPropsUsage = node.object.name === 'props' - const isInStatelessComponent = !!utils.getParentStatelessComponent() && !isInClassComponent + const isThisPropsUsage = node.object.type === 'ThisExpression' && node.property.name === 'props'; + const isPropsUsage = node.object.name === 'props'; + const isInStatelessComponent = !!utils.getParentStatelessComponent() && !isInClassComponent; + if (isPropsUsage && !isInClassComponent && !isAssignmentToProp(node)) { - return true + return true; } - // using props.foo in a class component, anywhere other than ComponentWillReceiveProps or a constructor - if (isInClassComponent && !isInConstructor && !inComponentWillReceiveProps() && isDirectProp) { - return false + + // props.foo usages are only allowed + // in the constructor and componentWillReceiveProps in class components + if (isInClassComponent && !inConstructor() && !inComponentWillReceiveProps() && isDirectProp) { + return false; } + if (isInStatelessComponent) { if (isAssignmentToProp(node)) { - return false + return false; } + // don't allow this.props usage in functional components if (isThisPropsUsage) { - return false + return false; } } if (!isInStatelessComponent && !isInClassComponent) { - return false + return false; } - return true + return true; } /** @@ -378,15 +384,23 @@ module.exports = { } } + /** + * Traverse down the tree of properties in a node, build up an array of keys + * and call a function + * @param {Object[]} properties Array of properties to traverse. + * @param {Function} fn Function to call on each property, receives property key, property value + * property node and all keys above this node in the tree + * (key, value, property, keys) => void + */ function traverseProperties(properties, fn, keys) { - keys = keys || [] + keys = keys || []; iterateProperties(properties, (key, value, property) => { - const updatedKeys = keys.concat(key) - fn(key, value, property, updatedKeys) + const updatedKeys = keys.concat(key); + fn(key, value, property, updatedKeys); if (value && value.properties) { - traverseProperties(value.properties, fn, updatedKeys) + traverseProperties(value.properties, fn, updatedKeys); } - }) + }); } /** @@ -597,114 +611,172 @@ module.exports = { } /** - * get the allNames array for a given property - * @param {ASTNode} property a property node. + * Mark prop type usages in an array of properties + * @param {ASTNode} properties An array of properties to search. + * @param {Object} usedPropTypes An array of previous prop type usages. + * @param {Object} allNames any names that come from parent prop type usages. */ - function getAllNamesForMemberExpression(expression, parentNames) { - parentNames = parentNames || [] - const propertyName = getPropertyName(expression) - const nameToAdd = propertyName || - (expression.type === 'ThisExpression' && 'this') || - (expression.name) - const childNames = nameToAdd ? - parentNames.concat(nameToAdd) - : parentNames - if (expression.object) { - return getAllNamesForMemberExpression(expression.object, parentNames).concat(childNames) - } - return childNames + function markPropTypesInProperties(properties, usedPropTypes, allNames) { + allNames = allNames || []; + traverseProperties(properties, (key, value, property, allPropertyNames) => { + if (hasSpreadOperator(property) || property.computed) { + return; + } + const propName = getKeyValue(property); + const propValueName = property.value.name; + const isRenamedProperty = propName !== propValueName && property.value.type === 'Identifier'; + if (propName) { + usedPropTypes.push({ + name: isRenamedProperty ? propValueName : propName, + allNames: allNames.concat(allPropertyNames), + node: property + }); + } + }); } - function traverseMemberExpression(expression, func, parentNames) { - parentNames = parentNames || [] + /** + * traverse up a member expression and a call a callback + * @param {ASTNode} expression a member expression node. + * @param {Function} fn Function to call on each expression, receives property name, + * the from steps further down the tree, and the expression + * (propertyName, parentNames, expression) => void + * @param {ASTNode} parentNames the names from steps further down the tree. + */ + function traverseMemberExpression(expression, fn, parentNames) { + parentNames = parentNames || []; const propertyName = getPropertyName(expression); if (propertyName) { const childNames = parentNames.concat(propertyName); - func(propertyName, childNames, expression.property) + fn(propertyName, childNames, expression); if (expression.parent.type === 'MemberExpression') { - return parentNames.concat(traverseMemberExpression(expression.parent, func, childNames)); + return traverseMemberExpression(expression.parent, fn, childNames); } } - return parentNames + return parentNames; + } + + /** + * Checks if the node is using destructuring + * @param {ASTNode} node a VariableDeclarator node. + * @returns {Boolean} True if destructing, false if not. + */ + function isDestucturedVariableDeclaration(node) { + return node.init && node.id && node.id.type === 'ObjectPattern'; + } + + /** + * Check if the nodes parent is using destructuring + * @param {ASTNode} node any node. + * @returns {Boolean} True if destructing, false if not. + */ + function parentIsDestructured(node) { + return node.parent.type === 'VariableDeclarator' && isDestucturedVariableDeclaration(node.parent); } /** - * Mark a prop type as used - * @param {ASTNode} properties The AST node being marked. + * Handle all of the variations of MemberExpression nodes (props, vs this.props etc) + * and return the starting node and parent usage if needed + * @param {ASTNode} node a MemberExpression node. + * @param {ASTNode} usedPropTypes an array of previous prop type usages. + * @returns {Object} An object containing the startingNode MemberExpression to traverse + * and the parentUsage if one exists, or returns void if its not a valid node + */ + function getProcessedMemberExpression(node, usedPropTypes) { + const isThisProps = (node.object.type === 'ThisExpression' && node.property.name === 'props'); + // if this is the top MemberExpression node + if (isThisProps && node.parent.type !== 'MemberExpression') { + if (!parentIsDestructured(node)) { + return void 0; + } + + // if the parent is using destructuring, mark prop types for its properties + const properties = node.parent.id.properties; + markPropTypesInProperties(properties, usedPropTypes); + return void 0; + } + + // this.props vs props + const startingNode = isThisProps ? node.parent : node; + const directlyOffOfProps = startingNode.object && + (startingNode.object.name === 'props' || startingNode.object.name === 'nextProps'); + + // if this member expression isn't coming off props in some way + // don't search for a parent usage + if (directlyOffOfProps || isThisProps) { + return { + startingNode: startingNode, + parentUsage: void 0 + }; + } + + // let { a } = this.props + //
{a.b}
+ const parentUsage = usedPropTypes.find( + propType => propType.name === node.object.name + ); + + // if we're not directly off of props + // and there isn't a parent usage, skip this node + if (!parentUsage) { + return void 0; + } + return { + startingNode: startingNode, + parentUsage: parentUsage + }; + } + + + /** + * Mark a node as using a prop type + * @param {ASTNode} node The AST node being marked. */ function markPropTypesAsUsed(node) { - let type; - let allNames; - let properties; - let parentUsage; - let parentNames = [] const component = components.get(utils.getParentComponent()); const usedPropTypes = (component && component.usedPropTypes || []).slice(); switch (node.type) { case 'MemberExpression': - const isThisProps = (node.object.type === 'ThisExpression' && node.property.name === 'props') - if (isThisProps && node.parent.type !== 'MemberExpression') { + const processedExpression = getProcessedMemberExpression(node, usedPropTypes); + if (!processedExpression) { break; } - const nodeToTraverse = isThisProps ? node.parent : node; - const directlyOffOfProps = (nodeToTraverse.object.name === 'props' || nodeToTraverse.object.name === 'nextProps') - if (!directlyOffOfProps && !isThisProps) { - const lookupName = nodeToTraverse.object.name - parentUsage = usedPropTypes.find( - propType => propType.name === lookupName - ) - if (!parentUsage) { - break; - } - parentNames = parentUsage.allNames - } - - traverseMemberExpression(nodeToTraverse, (propertyName, currentNames, currentNode) => { + const startingNames = processedExpression.parentUsage ? processedExpression.parentUsage.allNames : []; + traverseMemberExpression(processedExpression.startingNode, (propertyName, currentNames, currentNode) => { // Ignore Object methods if (Object.prototype[propertyName] || propertyName === '__COMPUTED_PROP__') { - return + return; } // Ignore Array methods if (Array.prototype[propertyName]) { - return + return; } + usedPropTypes.push({ name: propertyName, allNames: currentNames, - node: currentNode + node: currentNode.property }); - }, parentNames) + if (parentIsDestructured(currentNode)) { + markPropTypesInProperties(currentNode.parent.id.properties, usedPropTypes, currentNames); + } + }, startingNames); break; case 'ArrowFunctionExpression': case 'FunctionDeclaration': case 'FunctionExpression': - type = 'destructuring'; - properties = node.params[0].properties; + markPropTypesInProperties(node.params[0].properties, usedPropTypes); break; case 'VariableDeclarator': - // let {firstname} = this.props - const isInClassComponent = (utils.getParentES6Component() || utils.getParentES5Component()) - const initNames = getAllNamesForMemberExpression(node.init) - const isInStatelessComponent = !!utils.getParentStatelessComponent() & !isInClassComponent - const thisPropsDestructuring = ( - isInClassComponent && - initNames[0] === 'this' && PROPS_REGEX.test(initNames[1]) - ); - // let {firstname} = props - const directDestructuring = - PROPS_REGEX.test(initNames[0]) && - (isInStatelessComponent || inConstructor() || inComponentWillReceiveProps()) - ; + const isInClassComponent = (utils.getParentES6Component() || utils.getParentES5Component()); + const isInStatelessComponent = !!utils.getParentStatelessComponent() & !isInClassComponent; - if (thisPropsDestructuring) { - allNames = initNames.slice(2) - } - - if (directDestructuring) { - allNames = initNames.slice(1) - } + // // let {firstname} = props + const directDestructuring = + PROPS_REGEX.test(node.init.name) && + (isInStatelessComponent || inConstructor() || inComponentWillReceiveProps()); for (let i = 0, j = node.id.properties.length; i < j; i++) { // let {props: {firstname}} = this @@ -714,22 +786,22 @@ module.exports = { node.id.properties[i].value.type === 'ObjectPattern' ); - properties = thisDestructuring ? node.id.properties[i].value.properties : node.id.properties - if (!thisDestructuring || !directDestructuring || !thisPropsDestructuring) { - parentUsage = usedPropTypes.find( - propType => propType.name === initNames[0] - ) - - // let { a } = props + const properties = thisDestructuring ? node.id.properties[i].value.properties : node.id.properties; + let allNames = []; + if (!thisDestructuring && !directDestructuring) { + // let {props: { a }} // let { b } = a + const parentUsage = usedPropTypes.find( + propType => propType.name === node.init.name + ); + if (!parentUsage) { + continue; + } if (parentUsage) { - allNames = initNames.slice(1) + allNames = parentUsage.allNames; } } - if (!thisDestructuring && !directDestructuring && !parentUsage && !thisPropsDestructuring) { - continue; - } - type = 'destructuring'; + markPropTypesInProperties(properties, usedPropTypes, allNames); break; } break; @@ -737,37 +809,12 @@ module.exports = { throw new Error(`${node.type} ASTNodes are not handled by markPropTypesAsUsed`); } - - switch (type) { - case 'destructuring': - traverseProperties(properties, (key, value, property, allPropertyNames) => { - if (hasSpreadOperator(property) || property.computed) { - return; - } - - const propName = getKeyValue(property) - const propValueName = property.value.name - const isRenamedProperty = propName !== propValueName && property.value.type === 'Identifier' - allNames = allNames || [] - if (propName) { - const allParentNames = parentUsage ? parentUsage.allNames.concat(allNames) : allNames - usedPropTypes.push({ - name: isRenamedProperty ? propValueName : propName, - allNames: allParentNames.concat(allPropertyNames), - node: property - }); - } - }) - break; - default: - break; - } - components.set(node, { usedPropTypes: usedPropTypes }); } + /** * Marks all props found inside ObjectTypeAnnotaiton as declared. * @@ -1017,6 +1064,7 @@ module.exports = { markAnnotatedFunctionArgumentsAsDeclared(node); } + // -------------------------------------------------------------------------- // Public // -------------------------------------------------------------------------- @@ -1044,17 +1092,9 @@ module.exports = { }, VariableDeclarator: function(node) { - // const isInClassComponent = utils.getParentES6Component() || utils.getParentES5Component(); - // const inComponent = utils.getParentStatelessComponent() || inConstructor() - // || inComponentWillReceiveProps() || isInClassComponent; - // const destructuringInComponent = - // && - // inComponent; - const destructuring = node.init && node.id && node.id.type === 'ObjectPattern' - if (!destructuring) { - return; + if (isDestucturedVariableDeclaration(node) && node.init.type !== 'MemberExpression') { + markPropTypesAsUsed(node); } - markPropTypesAsUsed(node); }, FunctionDeclaration: handleStatelessComponent, @@ -1157,8 +1197,6 @@ module.exports = { if (!has(list, component) || !mustBeValidated(list[component])) { continue; } - - // console.log(list[component].usedPropTypes) reportUndeclaredPropTypes(list[component]); } } diff --git a/tests/lib/rules/prop-types-specific.js b/tests/lib/rules/prop-types-specific.js deleted file mode 100644 index 3dabcd6457..0000000000 --- a/tests/lib/rules/prop-types-specific.js +++ /dev/null @@ -1,56 +0,0 @@ -/** - * @fileoverview Prevent missing props validation in a React component definition - * @author Yannick Croissant - */ -'use strict'; - -// ------------------------------------------------------------------------------ -// Requirements -// ------------------------------------------------------------------------------ - -const rule = require('../../../lib/rules/prop-types'); -const RuleTester = require('eslint').RuleTester; - -const parserOptions = { - ecmaVersion: 8, - sourceType: 'module', - ecmaFeatures: { - experimentalObjectRestSpread: true, - jsx: true - } -}; - -// const settings = { -// react: { -// pragma: 'Foo' -// } -// }; - -require('babel-eslint'); - -// ------------------------------------------------------------------------------ -// Tests -// ------------------------------------------------------------------------------ - -const ruleTester = new RuleTester({parserOptions}); -ruleTester.run('prop-types', rule, { - valid: [ - ], - invalid: [ - { - code: ` - const Foo = ({ a: renamedA }) => { - const { b } = renamedA - return
test
- } - Foo.propTypes = { - a: PropTypes.object.shape({}), - } - `, - errors: [ - {message: '\'a.b\' is missing in props validation'} - ], - parser: 'babel-eslint' - } - ] -}); diff --git a/tests/lib/rules/prop-types.js b/tests/lib/rules/prop-types.js index 710129a333..34fbdcdf19 100644 --- a/tests/lib/rules/prop-types.js +++ b/tests/lib/rules/prop-types.js @@ -34,7 +34,6 @@ require('babel-eslint'); const ruleTester = new RuleTester({parserOptions}); ruleTester.run('prop-types', rule, { - // valid: [], valid: [ { code: [ @@ -3640,7 +3639,7 @@ ruleTester.run('prop-types', rule, { {message: '\'a.b\' is missing in props validation'} ], parser: 'babel-eslint' - }, + } ] }); From c92c347265a2ec0388a987ad03ad722f72735f53 Mon Sep 17 00:00:00 2001 From: Clay Branch Date: Wed, 13 Dec 2017 20:29:21 -0600 Subject: [PATCH 4/5] cleaning up unnecessary changes, confirming that ignored object and array methods are functions --- lib/rules/prop-types.js | 15 +++++++++------ lib/util/Components.js | 1 - tests/lib/rules/prop-types.js | 9 +++++---- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/lib/rules/prop-types.js b/lib/rules/prop-types.js index 274c799f68..55c8e93a73 100644 --- a/lib/rules/prop-types.js +++ b/lib/rules/prop-types.js @@ -661,7 +661,7 @@ module.exports = { * @param {ASTNode} node a VariableDeclarator node. * @returns {Boolean} True if destructing, false if not. */ - function isDestucturedVariableDeclaration(node) { + function isDestructuredVariableDeclaration(node) { return node.init && node.id && node.id.type === 'ObjectPattern'; } @@ -671,7 +671,7 @@ module.exports = { * @returns {Boolean} True if destructing, false if not. */ function parentIsDestructured(node) { - return node.parent.type === 'VariableDeclarator' && isDestucturedVariableDeclaration(node.parent); + return node.parent.type === 'VariableDeclarator' && isDestructuredVariableDeclaration(node.parent); } @@ -728,7 +728,6 @@ module.exports = { }; } - /** * Mark a node as using a prop type * @param {ASTNode} node The AST node being marked. @@ -744,13 +743,17 @@ module.exports = { } const startingNames = processedExpression.parentUsage ? processedExpression.parentUsage.allNames : []; traverseMemberExpression(processedExpression.startingNode, (propertyName, currentNames, currentNode) => { + if(propertyName === '__COMPUTED_PROP__') { + return; + } + // Ignore Object methods - if (Object.prototype[propertyName] || propertyName === '__COMPUTED_PROP__') { + if (Object.prototype[propertyName] && typeof Object.prototype[propertyName] === 'function') { return; } // Ignore Array methods - if (Array.prototype[propertyName]) { + if (Array.prototype[propertyName] && typeof Array.prototype[propertyName] === 'function') { return; } @@ -1092,7 +1095,7 @@ module.exports = { }, VariableDeclarator: function(node) { - if (isDestucturedVariableDeclaration(node) && node.init.type !== 'MemberExpression') { + if (isDestructuredVariableDeclaration(node) && node.init.type !== 'MemberExpression') { markPropTypesAsUsed(node); } }, diff --git a/lib/util/Components.js b/lib/util/Components.js index 3e83a66488..90ec38550f 100644 --- a/lib/util/Components.js +++ b/lib/util/Components.js @@ -463,7 +463,6 @@ function componentRule(rule, context) { let scope = context.getScope(); while (scope) { const node = scope.block; - const isClass = node.type === 'ClassExpression'; const isFunction = /Function/.test(node.type); // Functions const isMethod = node.parent && node.parent.type === 'MethodDefinition'; // Classes methods diff --git a/tests/lib/rules/prop-types.js b/tests/lib/rules/prop-types.js index 34fbdcdf19..9342fa2722 100644 --- a/tests/lib/rules/prop-types.js +++ b/tests/lib/rules/prop-types.js @@ -34,6 +34,7 @@ require('babel-eslint'); const ruleTester = new RuleTester({parserOptions}); ruleTester.run('prop-types', rule, { + valid: [ { code: [ @@ -1853,10 +1854,10 @@ ruleTester.run('prop-types', rule, { } A.propTypes = { - a: React.PropTypes.string, - ...SharedPropTypes // eslint-disable-line object-shorthand - }; - `, + a: React.PropTypes.string, + ...SharedPropTypes // eslint-disable-line object-shorthand + }; + `, parser: 'babel-eslint' } ], From aa827013edb59415d30625dea24bb894c0cb911b Mon Sep 17 00:00:00 2001 From: Clay Branch Date: Wed, 13 Dec 2017 20:30:59 -0600 Subject: [PATCH 5/5] fixed linter error --- lib/rules/prop-types.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rules/prop-types.js b/lib/rules/prop-types.js index 55c8e93a73..b45ca15e97 100644 --- a/lib/rules/prop-types.js +++ b/lib/rules/prop-types.js @@ -743,7 +743,7 @@ module.exports = { } const startingNames = processedExpression.parentUsage ? processedExpression.parentUsage.allNames : []; traverseMemberExpression(processedExpression.startingNode, (propertyName, currentNames, currentNode) => { - if(propertyName === '__COMPUTED_PROP__') { + if (propertyName === '__COMPUTED_PROP__') { return; }