feat: support Explicit Resource Management syntax for TS 5.2

[sajikix]

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]

• 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]

☁️ 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

• nx build website
• nx test typescript-estree --coverage=false
• nx run-many --target=typecheck --parallel
• nx run types:build
• Node 20 - nx test typescript-estree
• nx test eslint-plugin-tslint --coverage=false
• nx run-many --target=lint --parallel
• nx test eslint-plugin --coverage=false
• Node 20 - nx test visitor-keys
• Node 20 - nx run types:build
• Node 20 - nx test utils
• nx test eslint-plugin-internal --coverage=false
• nx run integration-tests:test
• Node 20 - nx test type-utils
• nx run eslint-plugin:generate:configs
• Node 20 - nx test rule-schema-to-typescript-types
• Node 20 - nx test parser
• Node 16 - nx test scope-manager --coverage=false
• Node 16 - nx test typescript-estree --coverage=false
• Node 20 - nx test repo-tools
• Node 20 - nx test scope-manager
• Node 20 - nx test ast-spec
• Node 20 - nx test eslint-plugin-tslint
• Node 20 - nx test eslint-plugin-internal
• Node 16 - nx test rule-schema-to-typescript-types --coverage=false
• Node 16 - nx run types:build
• Node 20 - nx test eslint-plugin
• Node 16 - nx test utils --coverage=false
• Node 16 - nx test type-utils --coverage=false
• Node 16 - nx test parser --coverage=false
• Node 16 - nx test visitor-keys --coverage=false
• Node 16 - nx test eslint-plugin-tslint --coverage=false
• Node 16 - nx test repo-tools --coverage=false
• Node 16 - nx test ast-spec --coverage=false
• Node 16 - nx test eslint-plugin --coverage=false
• Node 16 - nx test eslint-plugin-internal --coverage=false
• nx run-many --target=build --parallel --exclude=website --exclude=website-eslint

---

Sent with 💌 from NxCloud.

[codecov]

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]

✅ 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]

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]

...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

[sajikix]

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]

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.

[bradzacher]

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.

[JoshuaKGoldberg]

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!

1. please make the VariableDeclaration a disjoint union type to refine the "using" case for the .id.
2. please add error tests for the bad cases (eg using { a } = {}, await using [ b ] = [];, etc)

[bradzacher]

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.

[sajikix]

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]

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

[JoshuaKGoldberg]

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

[sajikix]

Looks good!

[JoshuaKGoldberg]

[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]

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!