From 0c06bb1d8ed7578ea6e09d7905a5a7322af7e47d Mon Sep 17 00:00:00 2001 From: golopot Date: Sat, 8 Jun 2019 01:08:08 +0800 Subject: [PATCH 1/4] [New] `prop-types`: handle variables defined as props --- lib/util/usedPropTypes.js | 85 +++++++++++++++++++++- tests/lib/rules/no-unused-prop-types.js | 74 +++++++++++++------ tests/lib/rules/prop-types.js | 97 +++++++++++++++++++++---- 3 files changed, 218 insertions(+), 38 deletions(-) diff --git a/lib/util/usedPropTypes.js b/lib/util/usedPropTypes.js index 27b7551e02..59c71f8272 100755 --- a/lib/util/usedPropTypes.js +++ b/lib/util/usedPropTypes.js @@ -15,6 +15,46 @@ const ast = require('./ast'); const LIFE_CYCLE_METHODS = ['componentWillReceiveProps', 'shouldComponentUpdate', 'componentWillUpdate', 'componentDidUpdate']; const ASYNC_SAFE_LIFE_CYCLE_METHODS = ['getDerivedStateFromProps', 'getSnapshotBeforeUpdate', 'UNSAFE_componentWillReceiveProps', 'UNSAFE_componentWillUpdate']; +function createPropVariables() { + /** @type {Map} Maps the variable to its definition. `props.a.b` is stored as `['a', 'b']` */ + let propVariables = new Map(); + let hasBeenWritten = false; + const stack = [{propVariables, hasBeenWritten}]; + return { + pushScope() { + // popVariables is not copied until first write. + stack.push({propVariables, hasBeenWritten: false}); + }, + popScope() { + stack.pop(); + propVariables = stack[stack.length - 1].propVariables; + hasBeenWritten = stack[stack.length - 1].hasBeenWritten; + }, + /** + * Add a variable name to the current scope + * @param {string} name + * @param {string[]} allNames Example: `props.a.b` should be formatted as `['a', 'b']` + */ + set(name, allNames) { + if (!hasBeenWritten) { + // copy on write + propVariables = new Map(propVariables); + stack[stack.length - 1].propVariables = propVariables; + stack[stack.length - 1].hasBeenWritten = true; + } + return propVariables.set(name, allNames); + }, + /** + * Get the definition of a variable. + * @param {string} name + * @returns {string[]} Example: `props.a.b` is represented by `['a', 'b']` + */ + get(name) { + return propVariables.get(name); + } + }; +} + /** * Checks if the string is one of `props`, `nextProps`, or `prevProps` * @param {string} name The AST node being checked. @@ -230,6 +270,8 @@ function isPropTypesUsageByMemberExpression(node, context, utils, checkAsyncSafe module.exports = function usedPropTypesInstructions(context, components, utils) { const checkAsyncSafeLifeCycles = versionUtil.testReactVersion(context, '16.3.0'); + const propVariables = createPropVariables(); + /** * Mark a prop type as used * @param {ASTNode} node The AST node being marked. @@ -261,6 +303,14 @@ module.exports = function usedPropTypesInstructions(context, components, utils) node.parent.id.parent = node.parent; // patch for bug in eslint@4 in which ObjectPattern has no parent markPropTypesAsUsed(node.parent.id, allNames); } + + // const a = props.a + if ( + node.parent.type === 'VariableDeclarator' && + node.parent.id.type === 'Identifier' + ) { + propVariables.set(node.parent.id.name, allNames); + } // Do not mark computed props as used. type = name !== '__COMPUTED_PROP__' ? 'direct' : null; } @@ -314,6 +364,7 @@ module.exports = function usedPropTypesInstructions(context, components, utils) const propName = ast.getKeyValue(context, properties[k]); if (propName) { + propVariables.set(propName, parentNames.concat([propName])); usedPropTypes.push({ allNames: parentNames.concat([propName]), name: propName, @@ -371,6 +422,7 @@ module.exports = function usedPropTypesInstructions(context, components, utils) * FunctionDeclaration, or FunctionExpression */ function handleFunctionLikeExpressions(node) { + propVariables.pushScope(); handleSetStateUpdater(node); markDestructuredFunctionArgumentsAsUsed(node); } @@ -392,6 +444,11 @@ module.exports = function usedPropTypesInstructions(context, components, utils) return { VariableDeclarator(node) { + // let props = this.props + if (isThisDotProps(node.init) && isInClassComponent(utils) && node.id.type === 'Identifier') { + propVariables.set(node.id.name, []); + } + // Only handles destructuring if (node.id.type !== 'ObjectPattern') { return; @@ -400,14 +457,19 @@ module.exports = function usedPropTypesInstructions(context, components, utils) // let {props: {firstname}} = this const propsProperty = node.id.properties.find(property => ( property.key && - (property.key.name === 'props' || property.key.value === 'props') && - property.value.type === 'ObjectPattern' + (property.key.name === 'props' || property.key.value === 'props') )); - if (propsProperty && node.init.type === 'ThisExpression') { + if (node.init.type === 'ThisExpression' && propsProperty && propsProperty.value.type === 'ObjectPattern') { markPropTypesAsUsed(propsProperty.value); return; } + // let {props} = this + if (node.init.type === 'ThisExpression' && propsProperty && propsProperty.value.name === 'props') { + propVariables.set('props', []); + return; + } + // let {firstname} = props if ( isCommonVariableNameForProps(node.init.name) && @@ -420,6 +482,12 @@ module.exports = function usedPropTypesInstructions(context, components, utils) // let {firstname} = this.props if (isThisDotProps(node.init) && isInClassComponent(utils)) { markPropTypesAsUsed(node.id); + return; + } + + // let {firstname} = thing, where thing is defined by const thing = this.props.**.* + if (propVariables.get(node.init.name)) { + markPropTypesAsUsed(node, propVariables.get(node.init.name)); } }, @@ -429,6 +497,12 @@ module.exports = function usedPropTypesInstructions(context, components, utils) FunctionExpression: handleFunctionLikeExpressions, + 'FunctionDeclaration:exit': propVariables.popScope, + + 'ArrowFunctionExpression:exit': propVariables.popScope, + + 'FunctionExpression:exit': propVariables.popScope, + JSXSpreadAttribute(node) { const component = components.get(utils.getParentComponent()); components.set(component ? component.node : node, { @@ -439,6 +513,11 @@ module.exports = function usedPropTypesInstructions(context, components, utils) MemberExpression(node) { if (isPropTypesUsageByMemberExpression(node, context, utils, checkAsyncSafeLifeCycles)) { markPropTypesAsUsed(node); + return; + } + + if (propVariables.get(node.object.name)) { + markPropTypesAsUsed(node, propVariables.get(node.object.name)); } }, diff --git a/tests/lib/rules/no-unused-prop-types.js b/tests/lib/rules/no-unused-prop-types.js index 67853ec3ef..d646fd7333 100644 --- a/tests/lib/rules/no-unused-prop-types.js +++ b/tests/lib/rules/no-unused-prop-types.js @@ -479,6 +479,59 @@ ruleTester.run('no-unused-prop-types', rule, { '};' ].join('\n'), parser: parsers.BABEL_ESLINT + }, { + code: ` + function Foo({ a }) { + return <>{ a.b } + } + Foo.propTypes = { + a: PropTypes.shape({ + b: PropType.string, + }) + } + `, + options: [{skipShapeProps: false}], + parser: parsers.BABEL_ESLINT + }, { + // Destructured assignment with Shape propTypes with skipShapeProps off issue #816 + code: ` + class Thing extends React.Component { + static propTypes = { + i18n: PropTypes.shape({ + gettext: PropTypes.func, + }), + } + + render() { + const { i18n } = this.props; + return ( +

{i18n.gettext('Some Text')}

+ ); + } + } + `, + parser: parsers.BABEL_ESLINT, + options: [{skipShapeProps: false}] + }, + { + code: ` + class Thing extends React.Component { + static propTypes = { + a: PropTypes.shape({ + b: PropTypes.string, + }), + } + + render() { + const { a } = this.props; + return ( +

{ a.b }

+ ); + } + } + `, + parser: parsers.BABEL_ESLINT, + options: [{skipShapeProps: false}] }, { code: [ 'var Hello = createReactClass({', @@ -4472,27 +4525,6 @@ ruleTester.run('no-unused-prop-types', rule, { }, { message: '\'prop2.*\' PropType is defined but prop is never used' }] - }, { - // Destructured assignment with Shape propTypes with skipShapeProps off issue #816 - code: [ - 'export default class NavigationButton extends React.Component {', - ' static propTypes = {', - ' route: PropTypes.shape({', - ' getBarTintColor: PropTypes.func.isRequired,', - ' }).isRequired,', - ' };', - - ' renderTitle() {', - ' const { route } = this.props;', - ' return TITLE;', - ' }', - '}' - ].join('\n'), - parser: parsers.BABEL_ESLINT, - options: [{skipShapeProps: false}], - errors: [{ - message: '\'route.getBarTintColor\' PropType is defined but prop is never used' - }] }, { code: [ // issue #1097 diff --git a/tests/lib/rules/prop-types.js b/tests/lib/rules/prop-types.js index bbd1c07ba5..301a6f78a7 100755 --- a/tests/lib/rules/prop-types.js +++ b/tests/lib/rules/prop-types.js @@ -870,17 +870,6 @@ ruleTester.run('prop-types', rule, { '};' ].join('\n'), parser: parsers.BABEL_ESLINT - }, { - // Reassigned props are ignored - code: [ - 'export class Hello extends Component {', - ' render() {', - ' const props = this.props;', - ' return
Hello {props.name.firstname} {props[\'name\'].lastname}
', - ' }', - '}' - ].join('\n'), - parser: parsers.BABEL_ESLINT }, { code: [ 'export default function FooBar(props) {', @@ -2453,6 +2442,82 @@ ruleTester.run('prop-types', rule, { {message: "'foo' is missing in props validation"}, {message: "'foo.bar' is missing in props validation"} ] + }, + { + code: ` + function Foo({ a }) { + return

{ a.nope }

+ } + + Foo.propTypes = { + a: PropTypes.shape({ + _: PropType.string, + }) + } + `, + errors: [ + {message: "'a.nope' is missing in props validation"} + ] + }, + { + code: ` + function Foo(props) { + const { a } = props + return

{ a.nope }

+ } + + Foo.propTypes = { + a: PropTypes.shape({ + _: PropType.string, + }) + } + `, + errors: [ + {message: "'a.nope' is missing in props validation"} + ] + }, + { + code: ` + function Foo(props) { + const a = props.a + return

{ a.nope }

+ } + + Foo.propTypes = { + a: PropTypes.shape({ + _: PropType.string, + }) + } + `, + errors: [ + {message: "'a.nope' is missing in props validation"} + ] + }, + { + code: ` + class Foo extends Component { + render() { + const props = this.props + return
{props.cat}
+ } + } + `, + errors: [ + {message: "'cat' is missing in props validation"} + ] + }, + { + code: ` + class Foo extends Component { + render() { + const {props} = this + return
{props.cat}
+ } + } + `, + errors: [ + {message: "'cat' is missing in props validation"} + ] }, { code: [ 'class Hello extends React.Component {', @@ -3364,6 +3429,8 @@ ruleTester.run('prop-types', rule, { ].join('\n'), errors: [{ message: '\'names\' is missing in props validation' + }, { + message: '\'names.map\' is missing in props validation' }] }, { code: [ @@ -4521,9 +4588,11 @@ ruleTester.run('prop-types', rule, { } `, parser: parsers.BABEL_ESLINT, - errors: [{ - message: '\'a\' is missing in props validation' - }] + errors: [ + {message: '\'a\' is missing in props validation'}, + {message: '\'a.b\' is missing in props validation'}, + {message: '\'a.b.c\' is missing in props validation'} + ] }, { code: ` From 89b8143ac18f4d3f0d45717436ad2aa344e2d494 Mon Sep 17 00:00:00 2001 From: golopot Date: Wed, 12 Jun 2019 15:08:17 +0800 Subject: [PATCH 2/4] Apply suggestion: replace concat([a]) with concat(a) Co-Authored-By: Jordan Harband --- lib/util/usedPropTypes.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/util/usedPropTypes.js b/lib/util/usedPropTypes.js index 59c71f8272..48198c3172 100755 --- a/lib/util/usedPropTypes.js +++ b/lib/util/usedPropTypes.js @@ -364,7 +364,7 @@ module.exports = function usedPropTypesInstructions(context, components, utils) const propName = ast.getKeyValue(context, properties[k]); if (propName) { - propVariables.set(propName, parentNames.concat([propName])); + propVariables.set(propName, parentNames.concat(propName)); usedPropTypes.push({ allNames: parentNames.concat([propName]), name: propName, From 3a1a0d1d159efd6bb3c597671e4ec8afaee22018 Mon Sep 17 00:00:00 2001 From: golopot Date: Wed, 12 Jun 2019 15:08:58 +0800 Subject: [PATCH 3/4] Apply suggestion: replace mutation with Object.assign Co-Authored-By: Jordan Harband --- lib/util/usedPropTypes.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/util/usedPropTypes.js b/lib/util/usedPropTypes.js index 48198c3172..b81049b5fa 100755 --- a/lib/util/usedPropTypes.js +++ b/lib/util/usedPropTypes.js @@ -39,7 +39,7 @@ function createPropVariables() { if (!hasBeenWritten) { // copy on write propVariables = new Map(propVariables); - stack[stack.length - 1].propVariables = propVariables; + Object.assign(stack[stack.length - 1], { propVariables, hasBeenWritten: true }); stack[stack.length - 1].hasBeenWritten = true; } return propVariables.set(name, allNames); From 9a63e19460e7ddbf988f328cf9f9730f4f156e0d Mon Sep 17 00:00:00 2001 From: golopot Date: Wed, 12 Jun 2019 15:37:45 +0800 Subject: [PATCH 4/4] Immediately destructure out propVariables rather than using it as a namespace --- lib/util/usedPropTypes.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/util/usedPropTypes.js b/lib/util/usedPropTypes.js index b81049b5fa..b783bbaf1a 100755 --- a/lib/util/usedPropTypes.js +++ b/lib/util/usedPropTypes.js @@ -39,7 +39,7 @@ function createPropVariables() { if (!hasBeenWritten) { // copy on write propVariables = new Map(propVariables); - Object.assign(stack[stack.length - 1], { propVariables, hasBeenWritten: true }); + Object.assign(stack[stack.length - 1], {propVariables, hasBeenWritten: true}); stack[stack.length - 1].hasBeenWritten = true; } return propVariables.set(name, allNames); @@ -271,6 +271,8 @@ module.exports = function usedPropTypesInstructions(context, components, utils) const checkAsyncSafeLifeCycles = versionUtil.testReactVersion(context, '16.3.0'); const propVariables = createPropVariables(); + const pushScope = propVariables.pushScope; + const popScope = propVariables.popScope; /** * Mark a prop type as used @@ -422,7 +424,7 @@ module.exports = function usedPropTypesInstructions(context, components, utils) * FunctionDeclaration, or FunctionExpression */ function handleFunctionLikeExpressions(node) { - propVariables.pushScope(); + pushScope(); handleSetStateUpdater(node); markDestructuredFunctionArgumentsAsUsed(node); } @@ -497,11 +499,11 @@ module.exports = function usedPropTypesInstructions(context, components, utils) FunctionExpression: handleFunctionLikeExpressions, - 'FunctionDeclaration:exit': propVariables.popScope, + 'FunctionDeclaration:exit': popScope, - 'ArrowFunctionExpression:exit': propVariables.popScope, + 'ArrowFunctionExpression:exit': popScope, - 'FunctionExpression:exit': propVariables.popScope, + 'FunctionExpression:exit': popScope, JSXSpreadAttribute(node) { const component = components.get(utils.getParentComponent());