Fix: add better TS generic support for arrow-parens (fixes #12570) #12587

matmalkowski · 2019-11-21T16:17:46Z

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:
Extend test coverage to include typescript generics as params with constrains

What changes did you make? (Give an overview)
I've added additional test cases for arrow-parens rule that that include usage of Typescript generics - refactored the rule itself so that new scenarios with generics are working correctly, by changing how param tokens are being picked. This fixes #12570
Is there anything you'd like reviewers to focus on?

platinumazure

Thanks for contributing!

matmalkowski · 2019-11-23T10:34:32Z

I have modified the rule to handle better typescript Generics, please have another look @kaicataldo @platinumazure

mdjermanovic

Thanks for this change! I left a couple of notes/questions for the start.

lib/rules/arrow-parens.js

kaicataldo · 2019-12-21T00:15:37Z

@matmalkowski Friendly ping

matmalkowski · 2019-12-21T05:52:27Z

@kaicataldo Just came back from leave, will try to take a look at the comments tomorrow / Monday 👍

matmalkowski · 2019-12-26T16:43:00Z

@mdjermanovic @kaicataldo Hey, finally I had a moment to take a look at the change and do some fixes. Apart from making the fixer more generic, I've also added yield test case to the suite.

lib/rules/arrow-parens.js

mdjermanovic · 2019-12-27T00:55:59Z

Apart from making the fixer more generic, I've also added yield test case to the suite.

That part looks good, thanks for fixing the fixer :)

This reverts commit 1bf3ff0.

mdjermanovic · 2020-01-01T22:38:43Z

lib/rules/arrow-parens.js

@@ -111,15 +141,17 @@ module.exports = {
                node.params.length === 1 &&
                node.params[0].type === "Identifier" &&
                !node.params[0].typeAnnotation &&
+                !node.typeParameters &&


This works, but we shouldn't use node.typeParameters because it isn't defined in estree extensions.

There is a simple alternative using the tokens only: Once we have openingParenToken, we could take the first token of the function (the second if the function is async) and check by reference whether it is the openingParenToken. If it isn't, then the rule should not warn to remove parens. That way, we are not using undocumented AST properties. It's a simple check, though it doesn't make sense for JS-only input so the rule becomes aware that there can be something unknown before the params (I don't think there is a way to avoid that).

@platinumazure @kaicataldo would this be acceptable? This is to avoid removing necessary parens in cases such as this:

/* eslint arrow-parens: ["error", "as-needed"] */ <T>(x) => {};

I'm no longer on the team, but I would be okay with this approach. @kaicataldo?

mdjermanovic · 2020-01-01T23:56:18Z

lib/rules/arrow-parens.js

+            const arrowToken = sourceCode.getTokenBefore(node.body, astUtils.isArrowToken);
+
+            const openingParenToken = node.params.length >= 1 && node.params[0].type === "Identifier"
+                ? sourceCode.getTokenBefore(node.params[0], t => isTokenPartOfNode(t, node) && astUtils.isOpeningParenToken(t))


If there is no opening paren for params (e.g., x => x), I think this would go through all tokens before the param until it reaches the beginning of the source code.

Maybe we could just unconditionally get the first token before params[0] and then check if it is an opening paren that is part of the node.

Also, I think there is no need to check node.params[0].type === "Identifier" here.

mdjermanovic · 2020-01-02T00:28:31Z

lib/rules/arrow-parens.js

+
+            const openingParenToken = node.params.length >= 1 && node.params[0].type === "Identifier"
+                ? sourceCode.getTokenBefore(node.params[0], t => isTokenPartOfNode(t, node) && astUtils.isOpeningParenToken(t))
+                : sourceCode.getTokenBefore(arrowToken, t => isTokenPartOfNode(t, node) && astUtils.isOpeningParenToken(t));


This could find an opening paren in default values for params.

Maybe we should just always get the first token before params[0], and make sure that the rule works only on single-param arrow function by changing the ArrowFunctionExpression selector to:

"ArrowFunctionExpression[params.length=1]"

matmalkowski · 2020-02-20T16:50:50Z

@mdjermanovic @platinumazure @kaicataldo Hey, I would like to get back to this issue, there is still one question unanswered, could you please help with it? I would get back on fixes after it

platinumazure · 2020-02-20T19:08:41Z

@matmalkowski Thanks for the ping. I'm no longer on the team, but I have left my own comment on what I think is the open question and asked Kai to follow up as well. Thank you for your patience!

kaicataldo · 2020-03-28T00:23:15Z

@matmalkowski Is there anything else we can do to help you land this?

nzakas · 2020-06-11T18:25:16Z

@matmalkowski are you still working on this? Is there anything you were waiting on?

matmalkowski · 2020-06-26T06:55:48Z

@nzakas Hey, unfortunately I didn't have much time to take a look at it - right now I'm unable to commit on any timeline. As for progress - I believe PR is missing some cleanup / refactor, as the main functionality/fix is already there in place - maybe someone would be interested in picking it up?

kaicataldo · 2020-06-26T19:53:37Z

@matmalkowski No worries. I think we should go ahead and close this PR so folks know they can work on this. If someone else ends up working on this and wants to finish it up, please feel free (including @matmalkowski, if things change 😄).

eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Nov 21, 2019

Chore: Adds typescript generics tests for arrow-parens (refs #12570)

7945e7a

matmalkowski changed the title ~~Chore: Adds more typescript test coverage for arrow-parens rule refs #12570~~ Chore: Adds typescript generics tests for arrow-parens (refs #12570) Nov 21, 2019

matmalkowski mentioned this pull request Nov 21, 2019

arrow-parens crashes when parsing typescript function with generics #12570

Closed

platinumazure approved these changes Nov 21, 2019

View reviewed changes

platinumazure added chore This change is not user-facing rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Nov 21, 2019

kaicataldo approved these changes Nov 22, 2019

View reviewed changes

Fix: add better TS generic support for arrow-parens (fixes #12570)

d33c7f0

matmalkowski changed the title ~~Chore: Adds typescript generics tests for arrow-parens (refs #12570)~~ Fix: add better TS generic support for arrow-parens (fixes #12570) Nov 23, 2019

matmalkowski requested review from platinumazure and kaicataldo November 24, 2019 11:19

mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly 3rd party plugin This is an issue related to a 3rd party plugin, config, or parser and removed chore This change is not user-facing labels Dec 8, 2019

mdjermanovic requested changes Dec 8, 2019

View reviewed changes

lib/rules/arrow-parens.js Outdated Show resolved Hide resolved

lib/rules/arrow-parens.js Outdated Show resolved Hide resolved

lib/rules/arrow-parens.js Outdated Show resolved Hide resolved

Fix: code review comments

d2084cd

matmalkowski requested a review from mdjermanovic December 26, 2019 16:41

mdjermanovic reviewed Dec 27, 2019

View reviewed changes

lib/rules/arrow-parens.js Outdated Show resolved Hide resolved

matmalkowski added 3 commits December 28, 2019 15:06

Fix: add additional test case for TS generic

1bf3ff0

Revert "Fix: add additional test case for TS generic"

444dc14

This reverts commit 1bf3ff0.

Fix: code review comments

eb30038

mdjermanovic requested changes Jan 2, 2020

View reviewed changes

kaicataldo requested review from mdjermanovic and removed request for platinumazure March 2, 2020 18:17

kaicataldo closed this Jun 26, 2020

eslint-deprecated bot locked and limited conversation to collaborators Dec 24, 2020

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: add better TS generic support for arrow-parens (fixes #12570) #12587

Fix: add better TS generic support for arrow-parens (fixes #12570) #12587

matmalkowski commented Nov 21, 2019 •

edited

platinumazure left a comment

matmalkowski commented Nov 23, 2019

mdjermanovic left a comment

kaicataldo commented Dec 21, 2019

matmalkowski commented Dec 21, 2019

matmalkowski commented Dec 26, 2019

mdjermanovic commented Dec 27, 2019

mdjermanovic Jan 1, 2020

platinumazure Feb 20, 2020

kaicataldo Feb 20, 2020

mdjermanovic Jan 1, 2020

mdjermanovic Jan 2, 2020

matmalkowski commented Feb 20, 2020

platinumazure commented Feb 20, 2020

kaicataldo commented Mar 28, 2020

nzakas commented Jun 11, 2020

matmalkowski commented Jun 26, 2020

kaicataldo commented Jun 26, 2020

Fix: add better TS generic support for arrow-parens (fixes #12570) #12587

Fix: add better TS generic support for arrow-parens (fixes #12570) #12587

Conversation

matmalkowski commented Nov 21, 2019 • edited

platinumazure left a comment

Choose a reason for hiding this comment

matmalkowski commented Nov 23, 2019

mdjermanovic left a comment

Choose a reason for hiding this comment

kaicataldo commented Dec 21, 2019

matmalkowski commented Dec 21, 2019

matmalkowski commented Dec 26, 2019

mdjermanovic commented Dec 27, 2019

mdjermanovic Jan 1, 2020

Choose a reason for hiding this comment

platinumazure Feb 20, 2020

Choose a reason for hiding this comment

kaicataldo Feb 20, 2020

Choose a reason for hiding this comment

mdjermanovic Jan 1, 2020

Choose a reason for hiding this comment

mdjermanovic Jan 2, 2020

Choose a reason for hiding this comment

matmalkowski commented Feb 20, 2020

platinumazure commented Feb 20, 2020

kaicataldo commented Mar 28, 2020

nzakas commented Jun 11, 2020

matmalkowski commented Jun 26, 2020

kaicataldo commented Jun 26, 2020

matmalkowski commented Nov 21, 2019 •

edited