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

getJSDocComment causing crash for eslint-plugin-react #9101

Closed
jseminck opened this issue Aug 12, 2017 · 4 comments
Closed

getJSDocComment causing crash for eslint-plugin-react #9101

jseminck opened this issue Aug 12, 2017 · 4 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion works as intended The behavior described in this issue is working correctly

Comments

@jseminck
Copy link

jseminck commented Aug 12, 2017

Tell us about your environment

  • ESLint Version: Latest
  • Node Version: 6.9.1
  • npm Version: 3.10.9

What parser (default, Babel-ESLint, etc.) are you using?
babel-eslint

Please show your full configuration:

Configuration
const parserOptions = {
  ecmaVersion: 6,
  ecmaFeatures: {
    jsx: true
  }
};

  valid: [{
    code: `
          export function subscribeBatch({ b }) {
              return a.bind(b);
          }

          /**
           * abc
           */
          function a() {
          }
    `,
    parser: 'babel-eslint',
    parserOptions: parserOptions
  }

What did you do? Please include the actual source code causing the issue.

A rule in eslint-plugin-react is crashing (jsx-eslint/eslint-plugin-react#1353). See the source code below. We call the sourceCode.getJSDocComment(node) with a node of type FunctionDeclaration.

https://github.com/yannickcr/eslint-plugin-react/blob/master/lib/util/Components.js#L218

In eslint source code, we then check if the parent of that node "looks like an export". Now... it turns out that the parent of this specific FunctionDeclaration is undefined.

let parent = node.parent;
...
case "FunctionDeclaration":
    if (looksLikeExport(parent)) {

https://github.com/eslint/eslint/blob/master/lib/util/source-code.js#L311

          export function subscribeBatch({ b }) {
              return a.bind(b);
          }

          /**
           * abc
           */
          function a() {
          }

What did you expect to happen?

No error. Can we guard with an additional undefined check? Or is there something else wrong here?

if (parent && looksLikeExport(parent)) {

What actually happened? Please include the actual, raw output from ESLint.

TypeError: Cannot read property 'type' of undefined
  at looksLikeExport (c:\project\node_modules\eslint\lib\util\source-code.js:78:19)
  at SourceCode.getJSDocComment (c:\project\node_modules\eslint\lib\util\source-code.js:311:21)
  at Object.isExplicitComponent (c:\project\node_modules\eslint-plugin-react\lib\util\Components.js:218:34)
  at Object.isES6Component (c:\project\node_modules\eslint-plugin-react\lib\util\Components.js:201:17)
  at Linter.MemberExpression (c:\project\node_modules\eslint-plugin-react\lib\rules\no-typos.js:73:18)
  at emitOne (events.js:101:20)
... (rest of stack trace is probably not useful)
@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Aug 12, 2017
@not-an-aardvark not-an-aardvark added bug ESLint is working incorrectly core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Aug 12, 2017
@not-an-aardvark
Copy link
Member

Thanks for the report. It looks like the getJSDocComment method relies on the given node having a parent property. ESLint gives nodes a parent property as it traverses the AST and calls the listeners in rules, so a node will only have a parent property if it has already been traversed.

For example, if you do something like this in a rule:

return {
    MemberExpression(node) {
        // do something with `node`
    }
}

At the time when the MemberExpression listener is called, the MemberExpression node will have a parent property, but the children of the MemberExpression node will not have a parent property. If you do something with the children that relies on them having a parent property, an error will be thrown. As a workaround, you could either (a) add a listener for the children and handle them at that point, or (b) use a "MemberExpression:exit" listener to trigger the listener when exiting the MemberExpression (at which point the parent property will already be present on the children, since they will have already been traversed).

This can sometimes be a bit inconvenient/confusing. In eslint/eslint-scope#27 there was some discussion about adding all the parent properties before the traversal starts, but for the moment this is technically working as intended.

@not-an-aardvark not-an-aardvark added works as intended The behavior described in this issue is working correctly and removed bug ESLint is working incorrectly core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Aug 12, 2017
@jseminck
Copy link
Author

Thanks for the thorough explanation. If this is working as intended then I will close the issue. However, using the "MemberExpression:exit" did not solve the problem for us as the FunctionDeclaration that we end up passing in to getJSDocComment is not a child of the MemberExpression that we are listening to.

The MemberExpression is: a.bind. And then eslint-plugin-react provides a helper to find the "relatedComponent" for that MemberExpression which is a function declared somewhere else within the same tree: function a() {}.

Anyway, I have some ideas how to work around it. 😄

@jseminck
Copy link
Author

jseminck commented Aug 17, 2017

Hi @not-an-aardvark

From eslint-plugin-react side, I currently see no other option than wrapping the call to getJSDocComment in a try/catch. That means that some use-cases will not be supported properly, but it is probably OK.

I figured I'd try and explain the use-case to you. Then eslint can decide if it should prioritise that eslint-scope issue. BTW - I'd love to help out as well, but I not sure how difficult the task is. 😄

So eslint-plugin-react has added a rule that wants to detect typos:

MyComponent.propTypes = {} // GOOD
MyComponent.PROPTYPES = {} // BAD

In order to do that, we look at the MemberExpression "MyComponent.propTypes". We then look up MyComponent in the AST because we want to know whether it is a React component or not (as we only want to perform the linting on React components).

One of the ways to detect React components is to parse the JSDoc and look for @extends React.Component (note this is a huge anti-pattern, but it is supported):

/** @extends React.Component */
class MyComponent extends BaseComponent

Now what happens is when the code is structured like this:

MyComponent.PROPTYPES = {} // BAD
/** @extends React.Component */
class MyComponent extends BaseComponent
  1. First we see MemberExpression MyComponent.PROPTYPES
  2. We detect related component class MyComponent from that expression. It is declared later in the code, so eslint has not yet visited it and there is no parent defined yet.
  3. MemberExpression:exit does not help, because class MyComponent is not a child in the AST of the MemberExpression MyComponent.PROPTYPES

I think eslint-plugin-react is fine not supporting this case for now - but we added a comment to remove the try/catch if/when eslint/eslint-scope#27 is implemented.

@not-an-aardvark
Copy link
Member

There are a few possible approaches you could take:

  1. Rather than checking the MyComponent class right away when you find MyComponent.PROPTYPES, you could instead add the node to a list, and check everything in the list as part of the Program:exit handler. At that point, all parent properties will be present.
  2. You could use the current listeners to schedule calls in a queue, and then invoke all the listeners sequentially in the Program:exit handler.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 11, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion works as intended The behavior described in this issue is working correctly
Projects
None yet
Development

No branches or pull requests

3 participants