arrow-parens crashes when parsing typescript function with generics

[matmalkowski]

Tell us about your environment

Node version: v10.15.3
npm version: v6.12.0
Local ESLint version: v6.6.0 (Currently used)

What parser (default, Babel-ESLint, etc.) are you using?
@typescript-eslint/parser
Please show your full configuration:

Configuration

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

const useABTests = <T extends ABTests, TName extends Extract<keyof T, string | number>>() => {
 ...
}

What did you expect to happen?

What actually happened? Please include the actual, raw output from ESLint.
the arrow-parens rule for above code detects firstTokenOfParam as < and that is incorrect. As result of this, later on, when asserting the rule, it does:

!astUtils.isOpeningParenToken(firstTokenOfParam)
 // return token.value === "(" && token.type === "Punctuator";

and it marks the typescript code as breaking the rule settings, so it tries to log the error message, but it will fail, since getLocation helper always grabs starting location from params array that in this case is undefined, resulting in crash

TypeError: Cannot read property 'loc' of undefined

Are you willing to submit a pull request to fix this bug?
Yeah, I've been looking at the code and with small guidance I could maybe do the fix

[kaicataldo]

I’m not sure we support generics - I don’t think they are defined in the type annotations spec in ESTree.

I wonder if the best solution here would be to make a complimentary rule in typescript-eslint?

Let’s see what the rest of the team thinks.

[mdjermanovic]

Maybe we could change the rule to:

• Check the number of params before anything else. If it isn't exactly 1 then the parens are mandatory and must be already there in the code, so there is no need to continue and do anything?
• To find out is that 1 param wrapped in parens, check the first token on the param node's left side. It should be an opening paren within the range of the arrow function.

I think that some similar refactorings to indirectly avoid problems with typescript code were being accepted before?

[g-plane]

I think that some similar refactorings to indirectly avoid problems with typescript code were being accepted before?

Yes.

[kaicataldo]

That makes sense to me, particularly if we can do it in a generic way. 👍🏼

[matmalkowski]

I think that some similar refactorings to indirectly avoid problems with typescript code were being accepted before?

Yes.

Do you by any chance have example PR of such refactor I could refer to? I'd like to contribute and seems like its a good starting issue

[kaicataldo]

That's great! You can find an example here. Please feel free to ask questions here or to stop by our Gitter chat.

[matmalkowski]

I've created initial PR #12587 with 2 additional test cases that should cover this bug, bug it doesn't get reproduced in the tests, I'm confused now :/

I've even added additional test with following code:

const someconst = <T extends Object, TName extends Extract<keyof T, string | number>>() => { return null }

so exactly how it is in my case in my repo, and the test still passes on my local, while when I try to run eslint, in fails (As on travis CI)

TypeError: Cannot read property 'loc' of undefined
Occurred while linting /home/travis/build/matmalkowski/react-handyman/packages/ab-test-jsx/src/useABTests/useABTests.tsx:11
    at getLocation (/home/travis/build/matmalkowski/react-handyman/node_modules/eslint/lib/rules/arrow-parens.js:24:31)
    at parens (/home/travis/build/matmalkowski/react-handyman/node_modules/eslint/lib/rules/arrow-parens.js:136:30)
    at listeners.(anonymous function).forEach.listener (/home/travis/build/matmalkowski/react-handyman/node_modules/eslint/lib/linter/safe-emitter.js:45:58)
    at Array.forEach (<anonymous>)
    at Object.emit (/home/travis/build/matmalkowski/react-handyman/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/home/travis/build/matmalkowski/react-handyman/node_modules/eslint/lib/linter/node-event-generator.js:254:26)
    at NodeEventGenerator.applySelectors (/home/travis/build/matmalkowski/react-handyman/node_modules/eslint/lib/linter/node-event-generator.js:283:22)
    at NodeEventGenerator.enterNode (/home/travis/build/matmalkowski/react-handyman/node_modules/eslint/lib/linter/node-event-generator.js:297:14)
    at CodePathAnalyzer.enterNode (/home/travis/build/matmalkowski/react-handyman/node_modules/eslint/lib/linter/code-path-analysis/code-path-analyzer.js:634:23)
    at nodeQueue.forEach.traversalInfo (/home/travis/build/matmalkowski/react-handyman/node_modules/eslint/lib/linter/linter.js:936:32)
error Command failed with exit code 2.

and yeah, the useABTests.tsx:11 is basicly same as in the test input I'm running succesfully with the eslint testsuite

Any clues what I might be doing wrong in terms of tests? Or my configuration?

[mdjermanovic]

Seems that your configuration is:

"arrow-parens": ["error", "as-needed", { "requireForBlockBody": true }]

And the rule most likely crashes on an arrow function that has block body.

[matmalkowski]

Seems that your configuration is:

"arrow-parens": ["error", "as-needed", { "requireForBlockBody": true }]

And the rule most likely crashes on an arrow function that has block body.

Nice catch! Yeah, added another 2 test cases, this time with requireForBlockBody and it fails / crashes. I will keep the current PR (I guess it can be merged?) and will try create another one on top of it with fix

[matmalkowski]

Hey, just a followup, had anyone had a chance to take another look at my pull request? I was able to fix the bug

[DanielSWolf]

@matmalkowski It seems that some feedback has been added to your PR. I'd really love to see this merged!

[matmalkowski]

@DanielSWolf Hey, I'm doing some vacation now, so will get back to fixing the comments probably on this weekend 👍

[DanielSWolf]

Sounds great! Have a nice remaining vacation!

[eslint-deprecated]

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that accepted issues failing to be implemented after 90 days tend to
never be implemented, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

[iFwu]

Is there any update on this issue?

[matmalkowski]

@iFwu #12587 is kinda WIP - I didn't have time over the weekend to actually address any issue, but hopefully I should find some minutes to do so by end of this week

[kaicataldo]

Reopening this since we might be able to solve this on our end in a generic way.

[eslint-deprecated]

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that accepted issues failing to be implemented after 90 days tend to
never be implemented, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

[mdjermanovic]

Reopening since there's an open PR #12587.

[mdjermanovic]

I'll work on this.