Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
feat: support Explicit Resource Management syntax for TS 5.2#7479
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
feat: support Explicit Resource Management syntax for TS 5.2#7479
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Josh-Cena commentedAug 15, 2023
Yes; this is mostly to support downstream parser consumers like Prettier.
Would be good to have one. Snapshots can never be exhaustive. |
nx-cloudbot commentedAug 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.
codecovbot commentedAug 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.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@## main #7479 +/- ##==========================================- Coverage 85.24% 85.10% -0.15%========================================== Files 386 386 Lines 13358 13380 +22 Branches 3944 3955 +11 ========================================== Hits 11387 11387- Misses 1594 1614 +20- Partials 377 379 +2
Flags with carried forward coverage won't be shown.Click here to find out more.
|
netlifybot commentedAug 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. |
Thanks for the PR,@sajikix! 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. |
JoshuaKGoldberg left a comment
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.
Thanks for getting started on this, really exciting work! 🚀
A couple of comments: one bug report on the node flags, and one discussion.
| casets.NodeFlags.Using: | ||
| return'using'; | ||
| casets.NodeFlags.AwaitUsing: | ||
| return'await using'; |
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.
...huh.ts.NodeFlags is not a proper exponents-of-two enum:
enumNodeFlags{None=0,Let=1,Const=2,Using=4,AwaitUsing=6,NestedNamespace=8,Synthesized=16,Namespace=32,// ...}
https://github.com/microsoft/TypeScript/pull/54505/files#diff-e9fd483341eea176a38fbd370590e1dc65ce2d9bf70bfd317c5407f04dba9560R796 isn't the first instance of TS adding a combination value to the enum, but it does mess with our logic.
This direct equality checking doesn't work whennode.flags has additional values on it. For example, putting it inside anasync function causes anawait using'skind to be"const" and ausing'skind to be"var".
So missing at least these cases:
usinginside afunctionusinginside anasync functionawait usinginside anasync function
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.
I appreciate your clarification about the misunderstanding regarding the determination of NodeFlags 🙏
As you rightly mentioned, AwaitUsing corresponds toConst | Using. To address this, I'll be making the following adjustments:
- Before distinguishing
ConstorUsing, I will comparenode.flags & ts.NodeFlags.AwaitUsing(result of&operation) withts.NodeFlags.AwaitUsing.- This comparison will return true only when the
kindof the node isawait using.
- This comparison will return true only when the
- If the comparison mentioned above is not applicable, it indicates that the
kindof the node is notawait using. In this case, I will individually evaluate theConstandUsingflags using&.
| * ``` | ||
| */ | ||
| kind:'const'|'let'|'var'; | ||
| kind:'await using'|'const'|'let'|'using'|'var'; |
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.
In theory, we could adhere more strictly to theExplicit Resource Management > Declarations > VariableDeclaration spec:
If
kindis"using"or"await using", for every declaratordofdeclarations,d.idmust be an Identifier. If the variable declaration is theleftof a ForOfStatement,d.initmust benull, otherwised.initmust be an Expression.
I'm not familiar with our practices around specifics like that. Do we normally?
Without any prior context, I would in theory prefer to make our AST more strict & compliant (at the cost of having more types). That way AST types are more accurate. For example, if someone is writng a rule aroundusing statements, they probably wouldn't want to have to assertdeclarations.id to beIdentifier instead of the default.
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.
yup I'd definitely agree here!
the spec is much stricter, so we can separate the types to make things easier to refine.
we can also enforce this from the parsing side as well - eg error if theid is not anIdentifier.
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.
Looks like the latest commit gets the first bit around.ids, but not the ForOfStatement narrowing. Progress!
bradzacher left a comment
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.
Thanks for getting to this so fast!
- please make the
VariableDeclarationa disjoint union type to refine the "using" case for the.id. - please add error tests for the bad cases (eg
using { a } = {},await using [ b ] = [];, etc)
| * ``` | ||
| */ | ||
| kind:'const'|'let'|'var'; | ||
| kind:'await using'|'const'|'let'|'using'|'var'; |
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.
yup I'd definitely agree here!
the spec is much stricter, so we can separate the types to make things easier to refine.
we can also enforce this from the parsing side as well - eg error if theid is not anIdentifier.
…e and `using` case
sajikix commentedAug 17, 2023
Thank you for reviews! I have made the necessary changes as you pointed out. One thing that concerns me is the name of the type that was originally a
|
JoshuaKGoldberg commentedAug 17, 2023
Ha, when I prototyped it locally I couldn't think of anything better than |
| exportfunctiongetDeclarationKind( | ||
| node:ts.VariableDeclarationList, | ||
| ):'const'|'let'|'var'{ | ||
| ):'const'|'let'|'var'|'using'|'await using'{ |
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.
[Question] What do you think about making a dedicated type for this? Just as a convenience point, not a blocker for the PR.
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.
Looks good!
| // eslint-disable-next-line @typescript-eslint/no-unsafe-enum-comparison | ||
| if((node.flags&ts.NodeFlags.AwaitUsing)===ts.NodeFlags.AwaitUsing){ |
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.
[Aside] Amusing:#6909. (not requesting changes, just amused)
JoshuaKGoldberg left a comment
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.
Great so far! I think ForOfStatements should also be updated forIf the variable declaration is theleft of a ForOfStatement,d.init must benull, otherwised.init must be an Expression., right?
sajikix commentedAug 19, 2023
Thanks again for the review! To make this type stricter, I changed the type of Also added AST validation for using in ForStatement. |
bradzacher left a comment
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 looking good to me!
thanks heaps for helping us support the next TS version!
This PR contains the following updates:| Package | Type | Update | Change ||---|---|---|---|| [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`6.4.1` -> `6.5.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/6.4.1/6.5.0) || [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`6.4.1` -> `6.5.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/6.4.1/6.5.0) || [@typescript-eslint/typescript-estree](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`6.4.1` -> `6.5.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2ftypescript-estree/6.4.1/6.5.0) || [netlify-cli](https://github.com/netlify/cli) | devDependencies | minor | [`16.1.0` -> `16.2.0`](https://renovatebot.com/diffs/npm/netlify-cli/16.1.0/16.2.0) |---### Release Notes<details><summary>typescript-eslint/typescript-eslint (@​typescript-eslint/eslint-plugin)</summary>### [`v6.5.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#650-2023-08-28)[Compare Source](typescript-eslint/typescript-eslint@v6.4.1...v6.5.0)##### Bug Fixes- **eslint-plugin:** \[consistent-type-assertions] wrap object return value with parentheses ([#​6885](typescript-eslint/typescript-eslint#6885)) ([23ac499](typescript-eslint/typescript-eslint@23ac499))You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.#### [6.4.1](typescript-eslint/typescript-eslint@v6.4.0...v6.4.1) (2023-08-21)##### Bug Fixes- **eslint-plugin:** \[no-unnecessary-condition] false positives with branded types ([#​7466](typescript-eslint/typescript-eslint#7466)) ([b52658f](typescript-eslint/typescript-eslint@b52658f)), closes [#​7293](typescript-eslint/typescript-eslint#7293)You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.</details><details><summary>typescript-eslint/typescript-eslint (@​typescript-eslint/parser)</summary>### [`v6.5.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#650-2023-08-28)[Compare Source](typescript-eslint/typescript-eslint@v6.4.1...v6.5.0)**Note:** Version bump only for package [@​typescript-eslint/parser](https://github.com/typescript-eslint/parser)You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.#### [6.4.1](typescript-eslint/typescript-eslint@v6.4.0...v6.4.1) (2023-08-21)**Note:** Version bump only for package [@​typescript-eslint/parser](https://github.com/typescript-eslint/parser)You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.</details><details><summary>typescript-eslint/typescript-eslint (@​typescript-eslint/typescript-estree)</summary>### [`v6.5.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/typescript-estree/CHANGELOG.md#650-2023-08-28)[Compare Source](typescript-eslint/typescript-eslint@v6.4.1...v6.5.0)##### Features- bump supported TS version to 5.2 ([#​7535](typescript-eslint/typescript-eslint#7535)) ([f18c88d](typescript-eslint/typescript-eslint@f18c88d))- support Explicit Resource Management syntax for TS 5.2 ([#​7479](typescript-eslint/typescript-eslint#7479)) ([c11e05c](typescript-eslint/typescript-eslint@c11e05c))You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.#### [6.4.1](typescript-eslint/typescript-eslint@v6.4.0...v6.4.1) (2023-08-21)**Note:** Version bump only for package [@​typescript-eslint/typescript-estree](https://github.com/typescript-eslint/typescript-estree)You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.</details><details><summary>netlify/cli (netlify-cli)</summary>### [`v16.2.0`](https://github.com/netlify/cli/blob/HEAD/CHANGELOG.md#1620-2023-08-29)[Compare Source](netlify/cli@v16.1.0...v16.2.0)##### Features- add support for `context.params` in edge functions ([#​5964](netlify/cli#5964)) ([ed14e05](netlify/cli@ed14e05))- support custom function routes ([#​5954](netlify/cli#5954)) ([82b94b5](netlify/cli@82b94b5))##### Bug Fixes- **deps:** update dependency [@​netlify/edge-bundler](https://github.com/netlify/edge-bundler) to v8.18.0 ([#​5953](netlify/cli#5953)) ([016cdf9](netlify/cli@016cdf9))- **deps:** update dependency [@​netlify/edge-bundler](https://github.com/netlify/edge-bundler) to v8.19.0 ([#​5971](netlify/cli#5971)) ([42478fd](netlify/cli@42478fd))- **deps:** update dependency [@​netlify/serverless-functions-api](https://github.com/netlify/serverless-functions-api) to v1.6.0 ([#​5935](netlify/cli#5935)) ([1d68354](netlify/cli@1d68354))- **deps:** update dependency [@​netlify/serverless-functions-api](https://github.com/netlify/serverless-functions-api) to v1.7.3 ([#​5957](netlify/cli#5957)) ([57d6627](netlify/cli@57d6627))- **deps:** update dependency lambda-local to v2.1.2 ([#​5967](netlify/cli#5967)) ([e592944](netlify/cli@e592944))- **deps:** update netlify packages ([#​5915](netlify/cli#5915)) ([1ad27ac](netlify/cli@1ad27ac))- **deps:** update netlify packages ([#​5965](netlify/cli#5965)) ([654c366](netlify/cli@654c366))</details>---### Configuration📅 **Schedule**: Branch creation - "before 3pm on Sunday" in timezone America/New_York, Automerge - At any time (no schedule defined).🚦 **Automerge**: Enabled.♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired.--- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box---This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi43NC4wIiwidXBkYXRlZEluVmVyIjoiMzYuNzQuMCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->Reviewed-on:https://git.chriswb.dev/chrisw-b/PersonalApi/pulls/3Reviewed-by: chrisw-b <chrisw-b@noreply.localhost>Co-authored-by: Renovate Bot <renovate.bot@chrisb.xyz>Co-committed-by: Renovate Bot <renovate.bot@chrisb.xyz>
PR Checklist
Overview
Supported the
Explicit Resource Managementsyntax supported in TS 5.2 RC.( Note: I know that@sosukesuzuki is working on this#7155 Issue. I am his colleague and worked on this PR with his support. )
I am concerned about the following points and would like to take feedback.
typescript-eslintlayer when I parse the wrong syntax?using {a} = {a:1}orusing a;.using declarationinforOfStatement?letorconsteither.