Add empty params check for unused prop types rule to fix empty proptype functions from causing crashes #1600

kevinzwhuang · 2017-12-13T00:38:15Z

Summary

There seems to still be an unresolved regression introduced in 7.5.0 from #1507
(see #1581 (comment))

When passing an empty function as a PropType - it causes the no-unused-prop-types to crash eslint with the following error:

Cannot read property 'properties' of undefined
TypeError: Cannot read property 'properties' of undefined

When this part of no-unused-prop-types is run:
https://github.com/yannickcr/eslint-plugin-react/blob/f6e4c891a474aaff8f69088b174de8c1317f8fa8/lib/rules/no-unused-prop-types.js#L652-L660

it breaks because node.params is an empty array. In my testing, it looks like #1507 introduced this breakage (the test I've included does not break when you revert it), it's unclear and hard to determine why #1507 caused this change.

This PR keeps the tests #1507 introduced passing, while also fixing the regression mentioned.

Changes Proposed

Add empty params check in the markPropTypesAsUsed function to fix empty propType functions from causing crashes.
Add test for validating empty propType functions by no-unused-prop-types

To reproduce

Lint this code with no-unused-prop-types and it will crash. We're also seeing this live in CI deployments at https://github.com/react-bootstrap/react-bootstrap in tests relating to propTypes.

class MyComponent extends React.Component<Props> {
  render() {
    return <div>{ this.props.other }</div>
  }
}
MyComponent.propTypes = { other: () => {} };

Fixes #1542, #1581
cc: @ljharb

…pe functions from causing crashes

kevinzwhuang · 2017-12-13T00:42:31Z

This fix is similar to #1545

ljharb

Thanks!

Could we also add test cases for { other() {} } and { other: function () { } } and { * other() {} }?

ljharb · 2017-12-13T18:33:03Z

tests/lib/rules/no-unused-prop-types.js

+        }
+        MyComponent.propTypes = { other: () => {} };
+      `,
+      parser: 'babel-eslint'


this shouldn't need babel-eslint; can you either add a duplicate test case (one using the default parser, and one using babel-eslint), or, remove the parser line and only test using the default parser?

You're right, I'll take out the parser line and leave the test cases using the default parser

…ypes and remove parser option

kevinzwhuang · 2017-12-13T19:18:01Z

@ljharb Added test cases for { other() {} }, { other: function () { } }, and { * other() {} }.

Also can confirm the new test cases still crash without the fix:

  3067 passing (11s)
  4 failing

  1) no-unused-prop-types
       valid

        class MyComponent extends React.Component {
          render() {
            return <div>{ this.props.other }</div>
          }
        }
        MyComponent.propTypes = { other: () => {} };
      :
     TypeError: Cannot read property 'properties' of undefined
      at markPropTypesAsUsed (lib/rules/no-unused-prop-types.js:9:30298)
      at iterateProperties (lib/rules/no-unused-prop-types.js:9:39583)
      at iterateProperties (lib/rules/no-unused-prop-types.js:9:14919)
      at markPropTypesAsDeclared (lib/rules/no-unused-prop-types.js:9:38650)
      at MemberExpression (lib/rules/no-unused-prop-types.js:9:50264)
      at listeners.(anonymous function).forEach.listener (node_modules/eslint/lib/util/safe-emitter.js:47:58)
      at Array.forEach (native)
      at Object.emit (node_modules/eslint/lib/util/safe-emitter.js:47:38)
      at NodeEventGenerator.applySelector (node_modules/eslint/lib/util/node-event-generator.js:251:26)
      at NodeEventGenerator.applySelectors (node_modules/eslint/lib/util/node-event-generator.js:280:22)
      at NodeEventGenerator.enterNode (node_modules/eslint/lib/util/node-event-generator.js:294:14)
      at CodePathAnalyzer.enterNode (node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:608:23)
      at Traverser.enter (node_modules/eslint/lib/linter.js:956:32)
      at Traverser.__execute (node_modules/estraverse/estraverse.js:397:31)
      at Traverser.traverse (node_modules/estraverse/estraverse.js:501:28)
      at Traverser.traverse (node_modules/eslint/lib/util/traverser.js:31:22)
      at Linter._verifyWithoutProcessors (node_modules/eslint/lib/linter.js:953:19)
      at preprocess.map.textBlock (node_modules/eslint/lib/linter.js:994:35)
      at Array.map (native)
      at Linter.verify (node_modules/eslint/lib/linter.js:993:42)
      at runRuleForItem (node_modules/eslint/lib/testers/rule-tester.js:366:34)
      at testValidTemplate (node_modules/eslint/lib/testers/rule-tester.js:396:28)
      at Context.RuleTester.it (node_modules/eslint/lib/testers/rule-tester.js:540:25)

  2) no-unused-prop-types
       valid

        class MyComponent extends React.Component {
          render() {
            return <div>{ this.props.other }</div>
          }
        }
        MyComponent.propTypes = { other() {} };
      :
     TypeError: Cannot read property 'properties' of undefined
      at markPropTypesAsUsed (lib/rules/no-unused-prop-types.js:9:30298)
      at iterateProperties (lib/rules/no-unused-prop-types.js:9:39583)
      at iterateProperties (lib/rules/no-unused-prop-types.js:9:14919)
      at markPropTypesAsDeclared (lib/rules/no-unused-prop-types.js:9:38650)
      at MemberExpression (lib/rules/no-unused-prop-types.js:9:50264)
      at listeners.(anonymous function).forEach.listener (node_modules/eslint/lib/util/safe-emitter.js:47:58)
      at Array.forEach (native)
      at Object.emit (node_modules/eslint/lib/util/safe-emitter.js:47:38)
      at NodeEventGenerator.applySelector (node_modules/eslint/lib/util/node-event-generator.js:251:26)
      at NodeEventGenerator.applySelectors (node_modules/eslint/lib/util/node-event-generator.js:280:22)
      at NodeEventGenerator.enterNode (node_modules/eslint/lib/util/node-event-generator.js:294:14)
      at CodePathAnalyzer.enterNode (node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:608:23)
      at Traverser.enter (node_modules/eslint/lib/linter.js:956:32)
      at Traverser.__execute (node_modules/estraverse/estraverse.js:397:31)
      at Traverser.traverse (node_modules/estraverse/estraverse.js:501:28)
      at Traverser.traverse (node_modules/eslint/lib/util/traverser.js:31:22)
      at Linter._verifyWithoutProcessors (node_modules/eslint/lib/linter.js:953:19)
      at preprocess.map.textBlock (node_modules/eslint/lib/linter.js:994:35)
      at Array.map (native)
      at Linter.verify (node_modules/eslint/lib/linter.js:993:42)
      at runRuleForItem (node_modules/eslint/lib/testers/rule-tester.js:366:34)
      at testValidTemplate (node_modules/eslint/lib/testers/rule-tester.js:396:28)
      at Context.RuleTester.it (node_modules/eslint/lib/testers/rule-tester.js:540:25)

  3) no-unused-prop-types
       valid

        class MyComponent extends React.Component {
          render() {
            return <div>{ this.props.other }</div>
          }
        }
        MyComponent.propTypes = { other: function () {} };
      :
     TypeError: Cannot read property 'properties' of undefined
      at markPropTypesAsUsed (lib/rules/no-unused-prop-types.js:9:30298)
      at iterateProperties (lib/rules/no-unused-prop-types.js:9:39583)
      at iterateProperties (lib/rules/no-unused-prop-types.js:9:14919)
      at markPropTypesAsDeclared (lib/rules/no-unused-prop-types.js:9:38650)
      at MemberExpression (lib/rules/no-unused-prop-types.js:9:50264)
      at listeners.(anonymous function).forEach.listener (node_modules/eslint/lib/util/safe-emitter.js:47:58)
      at Array.forEach (native)
      at Object.emit (node_modules/eslint/lib/util/safe-emitter.js:47:38)
      at NodeEventGenerator.applySelector (node_modules/eslint/lib/util/node-event-generator.js:251:26)
      at NodeEventGenerator.applySelectors (node_modules/eslint/lib/util/node-event-generator.js:280:22)
      at NodeEventGenerator.enterNode (node_modules/eslint/lib/util/node-event-generator.js:294:14)
      at CodePathAnalyzer.enterNode (node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:608:23)
      at Traverser.enter (node_modules/eslint/lib/linter.js:956:32)
      at Traverser.__execute (node_modules/estraverse/estraverse.js:397:31)
      at Traverser.traverse (node_modules/estraverse/estraverse.js:501:28)
      at Traverser.traverse (node_modules/eslint/lib/util/traverser.js:31:22)
      at Linter._verifyWithoutProcessors (node_modules/eslint/lib/linter.js:953:19)
      at preprocess.map.textBlock (node_modules/eslint/lib/linter.js:994:35)
      at Array.map (native)
      at Linter.verify (node_modules/eslint/lib/linter.js:993:42)
      at runRuleForItem (node_modules/eslint/lib/testers/rule-tester.js:366:34)
      at testValidTemplate (node_modules/eslint/lib/testers/rule-tester.js:396:28)
      at Context.RuleTester.it (node_modules/eslint/lib/testers/rule-tester.js:540:25)

  4) no-unused-prop-types
       valid

        class MyComponent extends React.Component {
          render() {
            return <div>{ this.props.other }</div>
          }
        }
        MyComponent.propTypes = { * other() {} };
      :
     TypeError: Cannot read property 'properties' of undefined
      at markPropTypesAsUsed (lib/rules/no-unused-prop-types.js:9:30298)
      at iterateProperties (lib/rules/no-unused-prop-types.js:9:39583)
      at iterateProperties (lib/rules/no-unused-prop-types.js:9:14919)
      at markPropTypesAsDeclared (lib/rules/no-unused-prop-types.js:9:38650)
      at MemberExpression (lib/rules/no-unused-prop-types.js:9:50264)
      at listeners.(anonymous function).forEach.listener (node_modules/eslint/lib/util/safe-emitter.js:47:58)
      at Array.forEach (native)
      at Object.emit (node_modules/eslint/lib/util/safe-emitter.js:47:38)
      at NodeEventGenerator.applySelector (node_modules/eslint/lib/util/node-event-generator.js:251:26)
      at NodeEventGenerator.applySelectors (node_modules/eslint/lib/util/node-event-generator.js:280:22)
      at NodeEventGenerator.enterNode (node_modules/eslint/lib/util/node-event-generator.js:294:14)
      at CodePathAnalyzer.enterNode (node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:608:23)
      at Traverser.enter (node_modules/eslint/lib/linter.js:956:32)
      at Traverser.__execute (node_modules/estraverse/estraverse.js:397:31)
      at Traverser.traverse (node_modules/estraverse/estraverse.js:501:28)
      at Traverser.traverse (node_modules/eslint/lib/util/traverser.js:31:22)
      at Linter._verifyWithoutProcessors (node_modules/eslint/lib/linter.js:953:19)
      at preprocess.map.textBlock (node_modules/eslint/lib/linter.js:994:35)
      at Array.map (native)
      at Linter.verify (node_modules/eslint/lib/linter.js:993:42)
      at runRuleForItem (node_modules/eslint/lib/testers/rule-tester.js:366:34)
      at testValidTemplate (node_modules/eslint/lib/testers/rule-tester.js:396:28)
      at Context.RuleTester.it (node_modules/eslint/lib/testers/rule-tester.js:540:25)

ljharb

Thanks!

Add empty params check for unused prop types rule to fix empty propty…

d331af6

…pe functions from causing crashes

kevinzwhuang mentioned this pull request Dec 13, 2017

no-unused-prop-types errors when declaring an empty function as a default component prop #1581

Closed

ljharb requested changes Dec 13, 2017

View reviewed changes

ljharb added the bug label Dec 13, 2017

Add test cases for other forms of empty functions in no-unused-prop-t…

6015af2

…ypes and remove parser option

ljharb approved these changes Dec 13, 2017

View reviewed changes

ljharb requested review from yannickcr, lencioni and EvHaus December 13, 2017 19:19

ljharb merged commit 9b8ea5e into jsx-eslint:master Dec 20, 2017

kevinzwhuang deleted the fix-unused-prop-types branch December 20, 2017 23:54

renovate bot mentioned this pull request Mar 22, 2018

Update eslint-plugin-react to 7.8.2 sbardian/DebtTrackerAPI#26

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add empty params check for unused prop types rule to fix empty proptype functions from causing crashes #1600

Add empty params check for unused prop types rule to fix empty proptype functions from causing crashes #1600

kevinzwhuang commented Dec 13, 2017 •

edited

Loading

kevinzwhuang commented Dec 13, 2017

ljharb left a comment

ljharb Dec 13, 2017

kevinzwhuang Dec 13, 2017

kevinzwhuang commented Dec 13, 2017

ljharb left a comment

Add empty params check for unused prop types rule to fix empty proptype functions from causing crashes #1600

Add empty params check for unused prop types rule to fix empty proptype functions from causing crashes #1600

Conversation

kevinzwhuang commented Dec 13, 2017 • edited Loading

Summary

Changes Proposed

To reproduce

kevinzwhuang commented Dec 13, 2017

ljharb left a comment

Choose a reason for hiding this comment

ljharb Dec 13, 2017

Choose a reason for hiding this comment

kevinzwhuang Dec 13, 2017

Choose a reason for hiding this comment

kevinzwhuang commented Dec 13, 2017

ljharb left a comment

Choose a reason for hiding this comment

kevinzwhuang commented Dec 13, 2017 •

edited

Loading