feat: support Explicit Resource Management syntax for TS 5.2 #7479

sajikix · 2023-08-15T18:23:27Z

PR Checklist

Addresses an existing open issue: RC support of TypeScript 5.2 Support #7155
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

Supported the Explicit Resource Management syntax 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.

Should I throw an error in the typescript-eslint layer when I parse the wrong syntax?
- For example, using {a} = {a:1} or using a;.
Should I add a snapshot test that uses using declaration in forOfStatement?
- I couldn't find any test cases for let or const either.

Josh-Cena · 2023-08-15T18:29:05Z

Should I throw an error in the typescript-eslint layer when I parse the wrong syntax?

Yes; this is mostly to support downstream parser consumers like Prettier.

Should I add a snapshot test that uses using declaration in forOfStatement?

Would be good to have one. Snapshots can never be exhaustive.

nx-cloud · 2023-08-15T18:29:33Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 96514c0. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 37 targets

Sent with 💌 from NxCloud.

codecov · 2023-08-15T18:36:17Z

Codecov Report

Merging #7479 (96514c0) into main (66cc514) will decrease coverage by 0.15%.
The diff coverage is 0.00%.

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

Flag	Coverage Δ
unittest	`85.10% <0.00%> (-0.15%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
packages/typescript-estree/src/convert.ts	`29.15% <0.00%> (-0.69%)`	⬇️
packages/typescript-estree/src/node-utils.ts	`53.14% <0.00%> (-0.86%)`	⬇️

netlify · 2023-08-15T19:02:01Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`96514c0`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/64dfb00f60608d00085c7839
😎 Deploy Preview	https://deploy-preview-7479--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

typescript-eslint · 2023-08-15T19:15:14Z

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 on https://opencollective.com/typescript-eslint.

JoshuaKGoldberg

Thanks for getting started on this, really exciting work! 🚀

A couple of comments: one bug report on the node flags, and one discussion.

JoshuaKGoldberg · 2023-08-15T20:26:57Z

packages/typescript-estree/src/node-utils.ts

+    case ts.NodeFlags.Using:
+      return 'using';
+    case ts.NodeFlags.AwaitUsing:
+      return 'await using';


...huh. ts.NodeFlags is not a proper exponents-of-two enum:

enum NodeFlags { 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 when node.flags has additional values on it. For example, putting it inside an async function causes an await using's kind to be "const" and a using's kind to be "var".

So missing at least these cases:

using inside a function

using inside an async function

await using inside an async function

I appreciate your clarification about the misunderstanding regarding the determination of NodeFlags 🙏

As you rightly mentioned, AwaitUsing corresponds to Const | Using. To address this, I'll be making the following adjustments:

Before distinguishingConst or Using, I will compare node.flags & ts.NodeFlags.AwaitUsing(result of & operation) with ts.NodeFlags.AwaitUsing.

This comparison will return true only when the kind of the node is await using.

If the comparison mentioned above is not applicable, it indicates that the kind of the node is not await using. In this case, I will individually evaluate the Const and Using flags using &.

JoshuaKGoldberg · 2023-08-15T20:37:22Z

packages/ast-spec/src/declaration/VariableDeclaration/spec.ts

@@ -29,5 +29,5 @@ export interface VariableDeclaration extends BaseNode {
   * var z = 3;
   * ```
   */
-  kind: 'const' | 'let' | 'var';
+  kind: 'await using' | 'const' | 'let' | 'using' | 'var';


In theory, we could adhere more strictly to the Explicit Resource Management > Declarations > VariableDeclaration spec:

If kind is "using" or "await using", for every declarator d of declarations, d.id must be an Identifier. If the variable declaration is the left of a ForOfStatement, d.init must be null, otherwise d.init must 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 around using statements, they probably wouldn't want to have to assert declarations.id to be Identifier instead of the default.

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 the id is not an Identifier.

Looks like the latest commit gets the first bit around .ids, but not the ForOfStatement narrowing. Progress!

bradzacher

Thanks for getting to this so fast!

please make the VariableDeclaration a 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)

bradzacher · 2023-08-16T06:21:50Z

packages/ast-spec/src/declaration/VariableDeclaration/spec.ts

@@ -29,5 +29,5 @@ export interface VariableDeclaration extends BaseNode {
   * var z = 3;
   * ```
   */
-  kind: 'const' | 'let' | 'var';
+  kind: 'await using' | 'const' | 'let' | 'using' | 'var';


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 the id is not an Identifier.

…e and `using` case

sajikix · 2023-08-17T13:40:51Z

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 VariableDeclaration

It's currently named LetOrConstOrVarDeclaration. This is because the Explicit Resource Management specification appears to call it LetOrConst.
But I feel it is a little straightforward. Therefore, if you have any better naming suggestions, I would be eager to make the change accordingly.

JoshuaKGoldberg · 2023-08-17T21:13:26Z

Ha, when I prototyped it locally I couldn't think of anything better than TraditionalVariableDeclaration. Which I don't like at all. 🤷

JoshuaKGoldberg · 2023-08-17T21:15:17Z

packages/typescript-estree/src/node-utils.ts

@@ -335,13 +335,20 @@ export function isJSXToken(node: ts.Node): boolean {
 */
 export function getDeclarationKind(
  node: ts.VariableDeclarationList,
-): 'const' | 'let' | 'var' {
+): 'const' | 'let' | 'var' | 'using' | 'await using' {


[Question] What do you think about making a dedicated type for this? Just as a convenience point, not a blocker for the PR.

Looks good!

JoshuaKGoldberg · 2023-08-17T21:17:02Z

packages/typescript-estree/src/node-utils.ts

+  // eslint-disable-next-line @typescript-eslint/no-unsafe-enum-comparison
+  if ((node.flags & ts.NodeFlags.AwaitUsing) === ts.NodeFlags.AwaitUsing) {


[Aside] Amusing: #6909. (not requesting changes, just amused)

JoshuaKGoldberg

Great so far! I think ForOfStatements should also be updated for If the variable declaration is the left of a ForOfStatement, d.init must be null, otherwise d.init must be an Expression., right?

sajikix · 2023-08-19T14:34:12Z

Thanks again for the review!
I had overlooked that the left node type in ForOfStatements can be made even stricter...

To make this type stricter, I changed the type of UsingDeclaration to two types of union(UsingInForOfDeclaration and UsingInNomalConextDeclaration) again. (I can't come up with a good idea for naming these two types either. ....)

Also added AST validation for using in ForStatement.

bradzacher

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. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://git.chriswb.dev/chrisw-b/PersonalApi/pulls/3 Reviewed-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>

sajikix added 6 commits August 14, 2023 15:37

feat: update VariableDeclaration spec

66ff863

feat: support using declaration for getDeclarationKind

f64efd7

feat: add snapshot test

a428311

fix: getDeclarationKind logic

014ca89

fix: sort kind property member.

3543126

fix: reduce switch cases

8df31d2

sajikix changed the title ~~Support using declarations ts 5.2~~ feat: support Explicit Resource Management syntax Aug 15, 2023

sajikix changed the title ~~feat: support Explicit Resource Management syntax~~ feat: support Explicit Resource Management syntax for TS 5.2 RC Aug 15, 2023

JoshuaKGoldberg requested changes Aug 15, 2023

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Aug 15, 2023

fix: getDeclarationKind flag discrimination logic

77ec66d

bradzacher requested changes Aug 16, 2023

View reviewed changes

sajikix added 6 commits August 17, 2023 11:43

feat: throw errors in binding patterns or not initialized

3631100

feat: add snapshot test error case

50f2934

fix: lint errors in convert

9080985

feat: make VariableDeclaration to union type of let/const/var cas…

a718755

…e and `using` case

Merge branch 'main' into support-using-declarations-ts-5.2

4c85cd5

fix: export LetOrConstOrVarDeclaration and UsingDeclaration type

f465d57

sajikix requested a review from JoshuaKGoldberg August 17, 2023 13:46

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Aug 17, 2023

sajikix requested a review from bradzacher August 17, 2023 13:47

sajikix added 2 commits August 17, 2023 23:39

Merge branch 'main' into support-using-declarations-ts-5.2

76a26a3

feat: add using declaration in for-of statement test

468f107

JoshuaKGoldberg reviewed Aug 17, 2023

View reviewed changes

JoshuaKGoldberg requested changes Aug 17, 2023

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Aug 17, 2023

sajikix added 6 commits August 18, 2023 13:31

fix: move fixtures to ForOfStatement

5073ce1

feat: update Declaration type in forOfStatement

9456e37

fix: apply lint fix

a9e716a

feat: add validation when using is used in for-of statement

d9e3ae3

feat: define DeclarationKind type

d7721a7

Merge branch 'main' into support-using-declarations-ts-5.2

96514c0

sajikix requested a review from JoshuaKGoldberg August 19, 2023 14:34

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Aug 19, 2023

bradzacher added the enhancement New feature or request label Aug 24, 2023

bradzacher approved these changes Aug 24, 2023

View reviewed changes

bradzacher changed the title ~~feat: support Explicit Resource Management syntax for TS 5.2 RC~~ feat: support Explicit Resource Management syntax for TS 5.2 Aug 24, 2023

bradzacher merged commit c11e05c into typescript-eslint:main Aug 24, 2023

bradzacher mentioned this pull request Aug 24, 2023

TypeScript 5.2 Support #7155

Closed

This was referenced Aug 25, 2023

Support TypeScript 5.2 prettier/prettier#15004

Closed

chore(deps): update dependency typescript to v5.2.2 prettier/prettier#15311

Closed

github-actions bot locked as resolved and limited conversation to collaborators Sep 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support Explicit Resource Management syntax for TS 5.2 #7479

feat: support Explicit Resource Management syntax for TS 5.2 #7479

sajikix commented Aug 15, 2023

Josh-Cena commented Aug 15, 2023

nx-cloud bot commented Aug 15, 2023 •

edited

Loading

codecov bot commented Aug 15, 2023 •

edited

Loading

netlify bot commented Aug 15, 2023 •

edited

Loading

typescript-eslint bot commented Aug 15, 2023

JoshuaKGoldberg left a comment

JoshuaKGoldberg Aug 15, 2023

sajikix Aug 16, 2023

JoshuaKGoldberg Aug 15, 2023

bradzacher Aug 16, 2023

JoshuaKGoldberg Aug 17, 2023

bradzacher left a comment

bradzacher Aug 16, 2023

sajikix commented Aug 17, 2023

JoshuaKGoldberg commented Aug 17, 2023

JoshuaKGoldberg Aug 17, 2023

sajikix Aug 19, 2023

JoshuaKGoldberg Aug 17, 2023

JoshuaKGoldberg left a comment

sajikix commented Aug 19, 2023

bradzacher left a comment

		// eslint-disable-next-line @typescript-eslint/no-unsafe-enum-comparison
		if ((node.flags & ts.NodeFlags.AwaitUsing) === ts.NodeFlags.AwaitUsing) {

feat: support Explicit Resource Management syntax for TS 5.2 #7479

feat: support Explicit Resource Management syntax for TS 5.2 #7479

Conversation

sajikix commented Aug 15, 2023

PR Checklist

Overview

Josh-Cena commented Aug 15, 2023

nx-cloud bot commented Aug 15, 2023 • edited Loading

☁️ Nx Cloud Report

codecov bot commented Aug 15, 2023 • edited Loading

Codecov Report

netlify bot commented Aug 15, 2023 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

typescript-eslint bot commented Aug 15, 2023

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradzacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sajikix commented Aug 17, 2023

JoshuaKGoldberg commented Aug 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

sajikix commented Aug 19, 2023

bradzacher left a comment

Choose a reason for hiding this comment

nx-cloud bot commented Aug 15, 2023 •

edited

Loading

codecov bot commented Aug 15, 2023 •

edited

Loading

netlify bot commented Aug 15, 2023 •

edited

Loading