Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
fix(typescript-estree): disallowusing as the variable keyword forfor..in loops#7649
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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 onhttps://opencollective.com/typescript-eslint. |
netlifybot commentedSep 15, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview fortypescript-eslint ready!
To edit notification comments on pull requests, go to yourNetlify site configuration. |
nx-cloudbot commentedSep 15, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Solo-steven commentedSep 15, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The reason that the CI job failed seems because OOM ( out of memory ), local linter result is pass, and since I only changed the  |
Josh-Cena left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The only thing forbidden isfor (using x in y), because in this casex 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 allowsusing.
| expect(() => instance.convertProgram()).toThrow(); | ||
| }); | ||
| it('using should be forbidden in for of statement ', () => { | ||
| const ast = convertCode('for(using foo of {});'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is valid code.
| expect(() => instance.convertProgram()).toThrow(); | ||
| }); | ||
| it('using should be forbidden in for statement ', () => { | ||
| const ast = convertCode('for(using i; i <= 0 ; ++i);'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So is this.
👋 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 canre-request a review. |
using keywordfor..in loopsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
thanks for your work on this! sorry for taking so long to get back to this.
| if ( | ||
| !( | ||
| initializer.flags & ts.NodeFlags.Const || | ||
| initializer.flags & ts.NodeFlags.Let || | ||
| initializer.flags === ts.NodeFlags.None | ||
| ) | ||
| ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We don't put AST tests in this location - this is more for testing our infra.
Instead we place our AST tests in theast-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:
- withinthis 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 forfor..in loopsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this LGTM - thanks for adding this in!
Uh oh!
There was an error while loading.Please reload this page.
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 ~