From d934305822571865c390cf0f861b543b79e48012 Mon Sep 17 00:00:00 2001 From: Joey Baker Date: Wed, 30 Aug 2017 10:06:25 -0700 Subject: [PATCH] Add(no-mutation-props) catches way more things Fixes #1113 Now warns on: * array mutations * `Object.assign` * `Object.defineProperty` * `delete` * all of the above also works with array and object destructuring and variable assignment --- docs/rules/no-mutation-props.md | 37 ++++ lib/rules/no-mutation-props.js | 157 ++++++++++++--- tests/lib/rules/no-mutation-props.js | 283 +++++++++++++++++++++++++-- 3 files changed, 444 insertions(+), 33 deletions(-) diff --git a/docs/rules/no-mutation-props.md b/docs/rules/no-mutation-props.md index d6b23082a8..e21260ffce 100644 --- a/docs/rules/no-mutation-props.md +++ b/docs/rules/no-mutation-props.md @@ -16,4 +16,41 @@ var Hello = React.createClass({ return
Hello {this.props.name}
; } }); + +var Hello = React.createClass({ + render: function() { + const {list} = this.props; + list.push(2); + return
{list.length} things
; + } +}); + +var Hello = React.createClass({ + render: function() { + const [firstThing] = this.props.list; + firstThing.foo = 'bar'; + return
{firstThing.foo}
; + } +}); + +var Hello = React.createClass({ + render: function() { + delete this.props.foo; + return
{Object.keys(this.props).length} props
; + } +}); + +var Hello = React.createClass({ + render: function() { + Object.assign(this.props.foo, {bar: 'baz'}) + return
{this.props.foo.bar}
; + } +}); + +var Hello = React.createClass({ + render: function() { + Object.defineProperty(this.props, 'foo') + return
{this.props.foo}
; + } +}); ``` diff --git a/lib/rules/no-mutation-props.js b/lib/rules/no-mutation-props.js index c669478887..c39c74d880 100644 --- a/lib/rules/no-mutation-props.js +++ b/lib/rules/no-mutation-props.js @@ -1,10 +1,11 @@ /** * @fileoverview Prevent mutation of this.props - * @author Ian Schmitz + * @author Ian Schmitz, Joey Baker */ 'use strict'; -var Components = require('../util/Components'); +const Components = require('../util/Components'); +const ARRAY_MUTATIONS = ['push', 'pop', 'shift', 'unshift']; // ------------------------------------------------------------------------------ // Rule Definition @@ -19,18 +20,65 @@ module.exports = { } }, - create: Components.detect(function (context, components, utils) { + create: Components.detect((context, components, utils) => { + /** + * Determines if a given node is from `this.props` + * @param {Object} node The node to examine + */ + function isPropsNode (node) { + if (!node) { + return false; + } + + let topObject = node; + + // when looking at an object path, get the top-most object + while (topObject.object) { + if (topObject.object && topObject.object.object) { + topObject = topObject.object; + } else { + break; + } + } + + // if this is a simple `this.props` call, we can bail early + if (topObject.object && topObject.object.type === 'ThisExpression') { + return topObject.property && topObject.property.name === 'props'; + } + + // if this is a variable, we need to look at the scope to determine if + // it is actually a reference to a prop + const name = topObject.name || (topObject.object && topObject.object.name); + const originalDeclaration = context.getScope().variableScope.set.get(name); + if (originalDeclaration) { + const originalIdentifiers = originalDeclaration.identifiers; + // if the get call returns falsey, we've not set it in our cache + // and it can't have have been from props. See `VariableDeclaration` + const currentIdentifier = components.get(originalIdentifiers[originalIdentifiers.length - 1]); + return currentIdentifier && currentIdentifier.isFromProps; + } + + return false; + } + + function setPropMutation(mutation, node) { + const component = components.get(utils.getParentComponent()); + const propMutations = component && component.propMutations + ? component.propMutations.concat([mutation]) + : [mutation]; + components.set(node || mutation, {propMutations}); + } /** * Reports this.props mutations for a given component * @param {Object} component The component to process */ function reportMutations(component) { - if (!component.mutations) { + if (!component.propMutations) { return; } - component.mutations.forEach(function(mutation) { + component.propMutations.forEach(mutation => { context.report({ node: mutation, message: 'A component must never modify its own props.' @@ -43,38 +91,103 @@ module.exports = { // -------------------------------------------------------------------------- return { - AssignmentExpression: function (node) { - var item; + VariableDeclaration: function(node) { + if (!node.declarations) { + return; + } + + node.declarations.forEach(declaration => { + const init = declaration.init; + const id = declaration.id; + const isProps = isPropsNode(init); + if (!isProps) { + return; + } + const assignments = + // if this is a simple assignment `const b = this.props.b` + (id.type === 'Identifier' && [id]) + // if this is object destructuring `const {b} = this.props` + || (id.type === 'ObjectPattern' && id.properties.map(property => property.value)) + // if this is array destructuring `const [b] = this.props.list` + || (id.type === 'ArrayPattern' && id.elements); + + // remember that this node is a prop, `isFromProps` relies on this + assignments.forEach(assignment => { + components.add(assignment); + components.set(assignment, { + isFromProps: true + }); + }); + }); + }, - if (!node.left || !node.left.object || !node.left.object.object) { + UnaryExpression: function(node) { + const isDeleteOperation = node.operator === 'delete'; + if (!isDeleteOperation) { + return; + } + const isProp = isPropsNode(node.argument); + if (!isProp) { return; } - item = node.left.object; - while (item.object && item.object.property) { - item = item.object; + setPropMutation(node); + }, + + CallExpression: function(node) { + const callee = node.callee; + const args = node.arguments; + const calleeName = callee.property && callee.property.name; + + const isObjectMutation = + callee.type === 'MemberExpression' + && callee.object.name === 'Object' + && ( + calleeName === 'assign' + || calleeName === 'defineProperty' + ); + + const isArrayMutation = + callee.type === 'MemberExpression' + && ARRAY_MUTATIONS.indexOf(calleeName) > -1; + + if (!isObjectMutation && !isArrayMutation) { + return; } - if (item.object.type === 'ThisExpression' && item.property.name === 'props') { - var component = components.get(utils.getParentComponent()); - var mutations = component && component.mutations || []; - mutations.push(node.left.object); - components.set(node, { - mutations: mutations - }); + const isCloneOperation = args.length && args[0].type === 'ObjectExpression'; + + if (isObjectMutation && isCloneOperation) { + return; + } + + // object mutations are a function call where the possible prop is + // always the first arg. Array mutations are always method calls on the + // possible prop + const isProps = isPropsNode(isObjectMutation ? args[0] : callee.object); + + if (!isProps) { + return; + } + + setPropMutation(node); + }, + + AssignmentExpression: function (node) { + if (isPropsNode(node.left)) { + setPropMutation(node.left.object, node); } }, 'Program:exit': function () { - var list = components.list(); + const list = components.list(); - Object.keys(list).forEach(function (key) { - var component = list[key]; + Object.keys(list).forEach(key => { + const component = list[key]; reportMutations(component); }); } }; - }) }; diff --git a/tests/lib/rules/no-mutation-props.js b/tests/lib/rules/no-mutation-props.js index 139136ab64..678796d5d6 100644 --- a/tests/lib/rules/no-mutation-props.js +++ b/tests/lib/rules/no-mutation-props.js @@ -1,6 +1,6 @@ /** * @fileoverview Prevent mutation of this.props - * @author Ian Schmitz + * @author Ian Schmitz, Joey Baker */ 'use strict'; @@ -8,10 +8,10 @@ // Requirements // ------------------------------------------------------------------------------ -var rule = require('../../../lib/rules/no-mutation-props'); -var RuleTester = require('eslint').RuleTester; +const rule = require('../../../lib/rules/no-mutation-props'); +const RuleTester = require('eslint').RuleTester; -var parserOptions = { +const parserOptions = { ecmaVersion: 6, ecmaFeatures: { jsx: true @@ -24,7 +24,8 @@ require('babel-eslint'); // Tests // ------------------------------------------------------------------------------ -var ruleTester = new RuleTester(); +const errorMessage = 'A component must never modify its own props.'; +const ruleTester = new RuleTester(); ruleTester.run('no-mutation-props', rule, { valid: [{ @@ -63,6 +64,58 @@ ruleTester.run('no-mutation-props', rule, { '}' ].join('\n'), parserOptions: parserOptions + }, { + code: [ + 'class Hello extends React.Component {', + ' render() {', + ' const {list} = this.thing;', + ' list.push(1);', + ' this.thing.list.push(1);', + ' this.thing.foo.list.push(1);', + ' const foo = [];', + ' foo.push(1);', + ' return
;', + ' }', + '}' + ].join('\n'), + parserOptions: parserOptions + }, { + code: [ + 'class Hello extends React.Component {', + ' render() {', + ' const {foo} = this.thing', + ' delete foo.bar;', + ' delete this.thing.foo', + ' const bar = {a: 1};', + ' delete bar.a;', + ' }', + '}' + ].join('\n'), + parserOptions: parserOptions + }, { + code: [ + 'class Hello extends React.Component {', + ' render() {', + ' const foo = {}', + ' Object.defineProperty(foo, "bar");', + ' Object.defineProperty(this.foo, "bar");', + ' }', + '}' + ].join('\n'), + parserOptions: parserOptions + }, { + code: [ + 'class Hello extends React.Component {', + ' render() {', + ' const {foo} = this.props', + ' Object.assign({}, foo, {bar: 1});', + ' Object.assign(this.foo, {foo: 1});', + ' Object.assign({}, this.props, {foo: 1});', + ' Object.assign({}, this.props.foo, {foo: 1});', + ' }', + '}' + ].join('\n'), + parserOptions: parserOptions }], invalid: [{ @@ -76,7 +129,7 @@ ruleTester.run('no-mutation-props', rule, { ].join('\n'), parserOptions: parserOptions, errors: [{ - message: 'A component must never modify its own props.' + message: errorMessage }] }, { code: [ @@ -89,7 +142,7 @@ ruleTester.run('no-mutation-props', rule, { ].join('\n'), parserOptions: parserOptions, errors: [{ - message: 'A component must never modify its own props.' + message: errorMessage }] }, { code: [ @@ -102,7 +155,7 @@ ruleTester.run('no-mutation-props', rule, { ].join('\n'), parserOptions: parserOptions, errors: [{ - message: 'A component must never modify its own props.' + message: errorMessage }] }, { code: [ @@ -116,13 +169,221 @@ ruleTester.run('no-mutation-props', rule, { ].join('\n'), parserOptions: parserOptions, errors: [{ - message: 'A component must never modify its own props.', + message: errorMessage, line: 3, column: 5 }, { - message: 'A component must never modify its own props.', + message: errorMessage, + line: 4, + column: 5 + }] + }, + { + code: [ + 'class Hello extends React.Component {', + ' helper() {', + ' const {foo} = this.props;', + ' foo.bar = 1;', + ' }', + ' render() {', + ' return
;', + ' }', + '}' + ].join('\n'), + parserOptions: parserOptions, + errors: [{ + message: errorMessage, + line: 4, + column: 5 + }] + }, + { + code: [ + 'class Hello extends React.Component {', + ' render() {', + ' const {list} = this.props;', + ' list.push(1);', + ' list.pop();', + ' list.shift();', + ' list.unshift(1);', + ' this.props.foo.push(1);', + ' this.props.foo.pop();', + ' this.props.foo.shift();', + ' this.props.foo.unshift(1);', + ' this.props.foo.list.push(1);', + ' this.props.foo.list.pop();', + ' this.props.foo.list.shift();', + ' this.props.foo.list.unshift(1);', + ' return
;', + ' }', + '}' + ].join('\n'), + parserOptions: parserOptions, + errors: [{ + message: errorMessage, + line: 4, + column: 5 + }, { + message: errorMessage, + line: 5, + column: 5 + }, { + message: errorMessage, + line: 6, + column: 5 + }, { + message: errorMessage, + line: 7, + column: 5 + }, { + message: errorMessage, + line: 8, + column: 5 + }, { + message: errorMessage, + line: 9, + column: 5 + }, { + message: errorMessage, + line: 10, + column: 5 + }, { + message: errorMessage, + line: 11, + column: 5 + }, { + message: errorMessage, + line: 12, + column: 5 + }, { + message: errorMessage, + line: 13, + column: 5 + }, { + message: errorMessage, + line: 14, + column: 5 + }, { + message: errorMessage, + line: 15, + column: 5 + }] + }, + { + code: [ + 'class Hello extends React.Component {', + ' render() {', + ' const {foo} = this.props;', + ' delete foo.bar;', + ' const [bar] = this.props.thing;', + ' delete bar.baz;', + ' const baz = this.props.baz', + ' delete baz.a;', + ' delete this.props.foo;', + ' delete this.props.foo.bar;', + ' return
;', + ' }', + '}' + ].join('\n'), + parserOptions: parserOptions, + errors: [{ + message: errorMessage, + line: 4, + column: 5 + }, { + message: errorMessage, + line: 6, + column: 5 + }, { + message: errorMessage, + line: 8, + column: 5 + }, { + message: errorMessage, + line: 9, + column: 5 + }, { + message: errorMessage, + line: 10, + column: 5 + }] + }, + { + code: [ + 'class Hello extends React.Component {', + ' render() {', + ' const {foo} = this.props;', + ' Object.defineProperty(foo, "bar");', + ' const [bar] = this.props.thing', + ' Object.defineProperty(bar, "baz");', + ' const baz = this.props.baz', + ' Object.defineProperty(baz, "thing");', + ' Object.defineProperty(this.props, "foo");', + ' Object.defineProperty(this.props.foo, "foo");', + ' return
;', + ' }', + '}' + ].join('\n'), + parserOptions: parserOptions, + errors: [{ + message: errorMessage, line: 4, column: 5 + }, { + message: errorMessage, + line: 6, + column: 5 + }, { + message: errorMessage, + line: 8, + column: 5 + }, { + message: errorMessage, + line: 9, + column: 5 + }, { + message: errorMessage, + line: 10, + column: 5 + }] + }, + { + code: [ + 'class Hello extends React.Component {', + ' render() {', + ' const {foo} = this.props;', + ' Object.assign(foo, {bar: 1});', + ' const [bar] = this.props.thing;', + ' Object.assign(bar, {baz: 1});', + ' const baz = this.props.baz;', + ' Object.assign(baz, {bat: 1});', + ' Object.assign(this.props, {foo: 1});', + ' Object.assign(this.props.baz, {foo: 1});', + ' return
;', + ' }', + '}' + ].join('\n'), + parserOptions: parserOptions, + errors: [{ + message: errorMessage, + line: 4, + column: 5 + }, { + message: errorMessage, + line: 6, + column: 5 + }, { + message: errorMessage, + line: 8, + column: 5 + }, { + message: errorMessage, + line: 9, + column: 5 + }, { + message: errorMessage, + line: 10, + column: 5 }] } /** @@ -139,7 +400,7 @@ ruleTester.run('no-mutation-props', rule, { ].join('\n'), parserOptions: parserOptions, errors: [{ - message: 'A component must never modify its own props.' + message: errorMessage }] }*/ ]