fix(eslint-plugin): [consistent-type-assertions] prevent syntax errors on arrow functions #8826
Conversation
|
Thanks for the PR, @kirkwaiblinger! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
kirkwaiblinger
changed the title
| const x = new (Generic<string> as Foo)(); | ||
| const x = new (Generic<string> as Foo)('string'); | ||
| const x = new ((Generic<string>) as Foo)(); | ||
| const x = new ((Generic<string>) as Foo)('string'); |
There was a problem hiding this comment.
From playing around with the AST viewer, I think this test case was simply incorrect (though tbh this syntax is bonkers and I struggle to follow it so I'm not totally certain.)
For reference, the inputs are
const x = new (<Foo>Generic<string>)();
const x = new (<Foo>Generic<string>)('string');(located above in the ANGLE_BRACKET_TESTS_EXCEPT_CONST_CASE string.)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8826 +/- ##
==========================================
- Coverage 88.02% 87.42% -0.61%
==========================================
Files 405 255 -150
Lines 14089 12513 -1576
Branches 4125 3927 -198
==========================================
- Hits 12402 10939 -1463
+ Misses 1382 1297 -85
+ Partials 305 277 -28
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| ], | ||
| }, | ||
| { | ||
| code: 'const f = <any>function () {};', |
There was a problem hiding this comment.
note that non-arrow functions behave differently from arrow functions
| ], | ||
| }, | ||
| { | ||
| // prettier wants to remove the parens around the yield expression, |
There was a problem hiding this comment.
I think this is a prettier bug but tbh I'm not 100% certain. Filed prettier/prettier#16192; we'll see what they say.
There was a problem hiding this comment.
was able to get this bug fixed. This will play nicely with #6128
There was a problem hiding this comment.
Nice!! Love to see an upstream bug discovered as part of our testing 😄
❤️
I personally do not know for sure. But looking at our precedences, eslint's precedences, and prettier's precedences, none of them seems to match 1:1 with the table on MDN. And, for TS syntax, part of that is explainable by there being additional operators not present in raw JS.... If I had to really speculate, though, I'd guess most of it comes down to the fact that we use the precedence switch for printing not for parsing, and
so what we see may just be the organic result of getting test cases to pass. To get more concrete info, we'd need to file a new issue to investigate, I think. |
|
Yeah, we should probably put more thought into this. I personally wrote that MDN table and towards the bottom things definitely get much murkier. |
kirkwaiblinger
changed the title
JoshuaKGoldberg
merged commit 342b873
into
typescript-eslint:main
PR Checklist
Overview
Upon looking into this bug, I learned that there are a whole host of circumstances where the
<T>(expr)=>(expr) as Tfix failed to put necessary parens around expr (since it was coded to never put any parens around expr). A generic fix has been added, and test cases from lots of precedence categories.Also, had to change the precedence pertaining to the
=>operator to address the specific issue in the bug, bringing the=>precedence into agreement with its ranking on MDN. Feels suspicious, but didn't seem to break any test cases.