Fix: Improve keyword-spacing typescript support (fixes #8110)

[soda0289]

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

[ ] Documentation update
[X ] 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:
#8110

What changes did you make? (Give an overview)
Allow keyword-spacing rule to find the first token better by skipping over decorators, generated by typescript, and not including symbol keyword when parsing arrow functions.

Is there anything you'd like reviewers to focus on?
Is this the best solution to allow the typescript parser to work correctly with the keyword-spacing rule?

[mention-bot]

@soda0289, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mysticatea, @vitorbal and @gyandeeps to be potential reviewers.

[eslintbot]

LGTM

[not-an-aardvark]

Thanks! I think this looks fine, but may you please add tests for this change? Examples of tests with a custom parser can be found in arrow-parens.

[soda0289]

Sure thing

[eslintbot]

LGTM

[eslintbot]

LGTM

[eslintbot]

LGTM

[eslintbot]

LGTM

[soda0289]

@not-an-aardvark
I have added some tests that ensure no exceptions are thrown when using a decorator with class or method and using some keywords as a parameter in arrow functions.

I also added back the check for firstToken.type === "Keyword" but included the check for firstToken.value === "function"

[mysticatea]

@eslint/eslint-team What do you think if we have typescript in devDependencies? As my understanding, we have been avoiding that we have custom parsers in devDependencies. Though I'm not oppose it...

[mysticatea]

I'm not familiar with TypeScript and Decorators, but I guess that decorators can have parameters. I'd like to look at tests for decorators which have parameters, especially the cases that the parameters have keywords.

[eslintbot]

LGTM

[soda0289]

@mysticatea
I have added more tests for decorators with normal parameters, and with keyword parameters. I also added a test for abstract classes with decorators.

[JamesHenry]

The part of me that wants to see a resolution to a long-standing issue thinks "hellz yes!" 😄

Other immediate thoughts:

• Keeping the peerDependency version of TypeScript within the typescript-eslint-parser, and this new devDependency of ESLint in sync would be a bit of a pain... Not sure if there is anything super simple we can do about it, though.

  • Crazy idea - ESLint Bot could open a PR to ESLint every time a new version of the typescript-eslint-parser is published, which updates the devDependencies:
    • typescript-eslint-parser: {{ whatever that new version was }}
    • typescript: {{ whatever the peerDependency of typescript-eslint-parser is }}

• I'm excited about this PR because how ever we do manage this kind of change in ESLint can hopefully serve as a template for similar things in future

• @nzakas has definitely given some thought to how changes like this would first start getting introduced into ESLint, so this is one rare occasion that I would like to make sure we get his input. He has very limited availability at the moment but kindly offered that I email him with stuff it is important that he sees. I will follow-up with him now.

[soda0289]

I can understand the concern of increasing the number of devDependencies and including parsers in the test suite. One way I can see to avoid this is to create another package or repository that handled eslint parser integration tests. That way if you change how eslint works you would not need to run all the parser integration tests unless it modified how parsing or rules were handled. New rules and rule modifications could run the integration tests and we could learn more about how these changes would effect users of those parser. A failing test in the integration test suite might indicate a problem with the parser itself and not eslint.

Another idea would be to hardcode the AST that typescript-eslint-parser generates and use that as test input instead of code.

[nzakas]

I'm inclined to think that we shouldn't bundle the TypeScript parser and run a lot of TypeScript tests in this repo. Instead, I'd suggest creating a stub custom parser for testing purposes where you can return whatever AST you want (I did that for type annotations already). I'd just cover a couple basic cases rather than the large number of tests this PR adds.

Then, I like the idea of having some kind of separate integration test. It seems like that could live in the TypeScript parser repo? I'm also not opposed to a separate repo for that.

[soda0289]

@nzakas I see something similar was done in this PR #7820. Will follow the pattern found there. Thanks!

[mysticatea]

@soda0289 I'm sorry for my unclear request. My concern was a code like class Foo { @desc({set a(value) {}, get a() {}, async c() {}}) async[foo]() {} }. In this case, I guessed the 2nd async would be ignored.

[eslintbot]

LGTM

[soda0289]

@mysticatea
Thanks for that test case it was very valuable. The old logic, of finding the keyword token by starting from the beginning of the method declaration, fails when keywords are used in decorators. I had not considered this case before. I have changed it so that we search backwards from the node.key, the method name node, until the first keyword token is found. Can you think of cases where this would fail?

@not-an-aardvark
I misunderstand what you meant by custom parser and have since rewrote the tests using a parser that has the typescript produced AST results hard coded. This is a much cleaner solution as @nzakas helped me realize.

These test no longer depend on typescript or typescript-eslint-parser. Thanks for your patience and feedback I appreciate it.

[ilyavolodin]

Stubbed parser looks like a much cleaner approach!

[mysticatea]

LGTM, thank you!

[not-an-aardvark]

LGTM. Thanks!