fix(typescript-estree): disallow using as the variable keyword for for..in loops
#7649
Conversation
|
Thanks for the PR, @Solo-steven! 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. |
|
The reason that the CI job failed seems because OOM ( out of memory ), local linter result is pass, and since I only changed the |
There was a problem hiding this comment.
The only thing forbidden is for (using x in y), because in this case x is statically known to be non-disposable (it can only be string/symbol), so this is likely a mistake. For all other constructs, x can actually be a disposable, and the grammar allows using.
| expect(() => instance.convertProgram()).toThrow(); | ||
| }); | ||
| it('using should be forbidden in for of statement ', () => { | ||
| const ast = convertCode('for(using foo of {});'); |
| expect(() => instance.convertProgram()).toThrow(); | ||
| }); | ||
| it('using should be forbidden in for statement ', () => { | ||
| const ast = convertCode('for(using i; i <= 0 ; ++i);'); |
|
👋 Hey @Solo-steven! Just checking in, is this still something you have time for? No worries if not - I just don't want to leave it hanging. Thanks! |
Hi @JoshuaKGoldberg , sorry for the delay, I am missing this issue from my schedule, I change the test case and only disallow using when |
|
Okie dokie, when you're ready for it to be re-reviewed you can re-request a review. |
using keywordfor..in loops
| if ( | ||
| !( | ||
| initializer.flags & ts.NodeFlags.Const || | ||
| initializer.flags & ts.NodeFlags.Let || | ||
| initializer.flags === ts.NodeFlags.None | ||
| ) | ||
| ) { |
There was a problem hiding this comment.
instead of checking for it "not being one of the valid states", instead can we just check for the one state we want to error on - (initializer.flags & ts.NodeFlags.Using) !== 0
| ) { | ||
| this.#throwError( | ||
| initializer, | ||
| " The left-hand side of a 'for...in' statement cannot be a 'using' declaration.", |
There was a problem hiding this comment.
nit remove the space
| " The left-hand side of a 'for...in' statement cannot be a 'using' declaration.", | |
| "The left-hand side of a 'for...in' statement cannot be a 'using' declaration.", |
| describe('using should be forbidden in for-related initializer ', () => { | ||
| it('using should be forbidden in for in statement ', () => { | ||
| const ast = convertCode('for(using foo in {});'); | ||
|
|
||
| const instance = new Converter(ast); | ||
|
|
||
| expect(() => instance.convertProgram()).toThrow(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
We don't put AST tests in this location - this is more for testing our infra.
Instead we place our AST tests in the ast-spec package alongside the AST declarations.
We haven't written proper docs for this yet (sorry!) but the steps for doing this is as follows:
- within this folder create a folder called
fixtures. - within
fixturescreate a_error_folder - within
_error_create a folder with a name for your case - for example maybeusing-initializer? - within that folder, create a file called
fixture.ts - within that file add your test code (eg
for(using foo in {});) cd packages/ast-spec && yarn testto run the fixture tests and generate the necessary snapshots.
|
Hi @Solo-steven, do you have time to take a look at the review? |
Hi @Josh-Cena Sorry about the late response, I am kind of busy this month, but I will fix this PR this weekend! thanks for your reminder ~ |
|
👋 checking in again @Solo-steven - by "this month" do you mean January? No worries if you're busy, just checking in! |
…e test case to ast-spec * shorten syntax check for using keyword in for-in statement. * move test file to ast-spec.
|
I @JoshuaKGoldberg sorry about being so late, i pushed a new commit to fix the above problem, thanks for your patience and comment on this PR ~ |
|
Ok great, thanks! Tip: if you ever want us to take another look, re-requesting review through the GitHub UI will auto-remove the |
for..in loopsusing as the variable keyword for for..in loops
PR Checklist
for (using foo in {});should be invalid #7555Overview
fix issue #7555 by check TypeScript
nodeflag, one problem is I not sure I add test case in correct file, if there are some wrong, please tell me, thanks ~