Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression in 7.17 through latest: “Cannot read property 'type' of undefined” at getKeyValue #2570

Closed
wchargin opened this issue Feb 8, 2020 · 6 comments
Assignees

Comments

@wchargin
Copy link

wchargin commented Feb 8, 2020

Repro code:

// @flow
import {Component} from "react";
type LinkProps = {...{}};
class Link extends Component<LinkProps> {}

Stack trace:

TypeError: Cannot read property 'type' of undefined
Occurred while linting /tmp/tmp.HiVXmY0CJv/test.js:4
    at getKeyValue (/tmp/tmp.HiVXmY0CJv/node_modules/eslint-plugin-react/lib/util/ast.js:172:14)
    at iterateProperties (/tmp/tmp.HiVXmY0CJv/node_modules/eslint-plugin-react/lib/util/propTypes.js:271:19)
    at iterateProperties (/tmp/tmp.HiVXmY0CJv/node_modules/eslint-plugin-react/lib/util/propTypes.js:45:9)
    at declarePropTypesForObjectTypeAnnotation (/tmp/tmp.HiVXmY0CJv/node_modules/eslint-plugin-react/lib/util/propTypes.js:258:5)
    at markPropTypesAsDeclared (/tmp/tmp.HiVXmY0CJv/node_modules/eslint-plugin-react/lib/util/propTypes.js:441:33)
    at Object.ClassDeclaration (/tmp/tmp.HiVXmY0CJv/node_modules/eslint-plugin-react/lib/util/propTypes.js:649:9)
    at updatedRuleInstructions.(anonymous function) (/tmp/tmp.HiVXmY0CJv/node_modules/eslint-plugin-react/lib/util/Components.js:900:43)
    at listeners.(anonymous function).forEach.listener (/tmp/tmp.HiVXmY0CJv/node_modules/eslint/lib/linter/safe-emitter.js:45:58)
    at Array.forEach (<anonymous>)
    at Object.emit (/tmp/tmp.HiVXmY0CJv/node_modules/eslint/lib/linter/safe-emitter.js:45:38)

Config:

module.exports = {
  parser: "babel-eslint",
  plugins: ["react"],
  parserOptions: {
    ecmaVersion: 2018,
    sourceType: "module",
    ecmaFeatures: {
      jsx: true,
    },
  },
  extends: [
    // It also suffices to pick a single "react/*" rule (tested "display-name",
    // "require-render-return", "no-deprecated").
    "plugin:react/recommended",
  ],
  settings: {
    react: {
      version: "16.4.1",
    },
  },
};

package.json:

{
  "name": "tmp.HiVXmY0CJv",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "dependencies": {
    "babel-eslint": "^10.0.3",
    "eslint": "^6.8.0",
    "eslint-plugin-react": "^7.18.3"
  }
}

yarn.lock:
https://gist.github.com/wchargin/141116df4ff3d1ef2da77b75b5fad51e#file-yarn-lock

Self-contained repro repository (warning: runs code from the internet):

url='https://gist.github.com/wchargin/141116df4ff3d1ef2da77b75b5fad51e.git' &&
git clone "${url}" repro &&
cd repro &&
yarn &&
yarn eslint test.js

Versions tested:

  • 7.16.0: OK
  • 7.17.0: FAIL
  • 7.18.3: FAIL

Original code from which repro was created for context, and to answer
any “why would anyone do that?” questions:
https://github.com/sourcecred/sourcecred/blob/8c47dd1c14ed8c7117f3707a52c481b753d18138/src/webutil/Link.js#L24-L31

@ljharb
Copy link
Member

ljharb commented Feb 8, 2020

Thanks for the simple repro!

@wchargin
Copy link
Author

wchargin commented Feb 9, 2020

Thank you for the quick patch! This fixes my repro, but doesn’t fix a
slightly different case that I actually have in real code (linked at
bottom of original issue). Here is a second small repro:

// @flow
import {Component} from "react";
type LinkProps = $ReadOnly<{
  ...React$ElementConfig<"a">,
  ...{|+to: string|} | {|+href: string|},
  +styles?: $ReadOnlyArray<
    Object | false | null | void
  > /* Aphrodite styles, as passed to `css` */,
}>;
class Link extends Component<LinkProps> {}

This still has the same error message after updating the dependency on
this package. It looks like the following patch suffices as a “fix”:

diff --git a/lib/util/ast.js b/lib/util/ast.js
index 731a2733..867560f3 100644
--- a/lib/util/ast.js
+++ b/lib/util/ast.js
@@ -171,6 +171,9 @@ function getKeyValue(context, node) {
   if (node.type === 'ObjectTypeAnnotation') {
     return;
   }
+  if (node.type === 'UnionTypeAnnotation') {
+    return;
+  }
   const key = node.key || node.argument;
   return key.type === 'Identifier' ? key.name : key.value;
 }

…but this smells like could be more missing cases—intersection type
annotations?—so may or may not be exactly what you’re looking for. (In
case you do want to use this, I release the above patch under the MIT
and Apache-2 licenses, at your discretion.)

Does it make sense to re-open this, or should I file a new issue?

@ljharb
Copy link
Member

ljharb commented Feb 9, 2020

There's probably lots of missing cases :-) a new issue (or even better, a new PR) covering as many cases as you can think of would be great!

@ljharb
Copy link
Member

ljharb commented Feb 16, 2020

@wchargin ping :-)

@Beanow
Copy link

Beanow commented Mar 6, 2020

@ljharb this issue still persists with v7.18.3. Failed build for this file.

I believe #2570 (comment) still applies as detailed enough to be an issue on it's own :]

Beanow added a commit to sourcecred/sourcecred that referenced this issue Mar 6, 2020
Preparing for release #1679

Note: due to a regression, not upgrading eslint-plugin-react
See jsx-eslint/eslint-plugin-react#2570
@ljharb
Copy link
Member

ljharb commented Mar 6, 2020

@Beanow my hope was that @wchargin would make a PR with that; my time is limited, is that something you could do?

Beanow added a commit to sourcecred/sourcecred that referenced this issue Mar 6, 2020
Preparing for release #1679

Note: due to a regression, not upgrading eslint-plugin-react
See jsx-eslint/eslint-plugin-react#2570

Also updated package.json to latest semver in-range versions.
Note, this changes all packages (other than eslint-plugin-react)
to ^x.x.x format.
Beanow added a commit to sourcecred/sourcecred that referenced this issue Mar 7, 2020
Preparing for release #1679

Note: due to a regression, not upgrading eslint-plugin-react
See jsx-eslint/eslint-plugin-react#2570

Also updated package.json to latest semver in-range versions.
Note, this changes all packages (other than eslint-plugin-react)
to ^x.x.x format.
BrianLitwin pushed a commit to sourcecred/sourcecred that referenced this issue Mar 11, 2020
Preparing for release #1679

Note: due to a regression, not upgrading eslint-plugin-react
See jsx-eslint/eslint-plugin-react#2570

Also updated package.json to latest semver in-range versions.
Note, this changes all packages (other than eslint-plugin-react)
to ^x.x.x format.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants