Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.9k
chore: extract AST check from convert.ts to ast-checks.ts#11748
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
base:main
Are you sure you want to change the base?
chore: extract AST check from convert.ts to ast-checks.ts#11748
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for the PR,@Lonercode! 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 commentedNov 9, 2025 • 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 project configuration. |
nx-cloudbot commentedNov 9, 2025 • 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.
View yourCI Pipeline Execution ↗ for commit29a9c73
☁️Nx Cloud last updated this comment at |
codecovbot commentedNov 9, 2025 • 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❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (25.75%) is below the target coverage (90.00%). You can increase the patch coverage or adjust thetarget coverage. Additional details and impacted files@@ Coverage Diff @@## main #11748 +/- ##==========================================- Coverage 90.49% 90.48% -0.01%========================================== Files 522 523 +1 Lines 53360 53375 +15 Branches 8911 8913 +2 ==========================================+ Hits 48286 48298 +12- Misses 5059 5062 +3 Partials 15 15
Flags with carried forward coverage won't be shown.Click here to find out more.
🚀 New features to boost your workflow:
|
fisker commentedNov 14, 2025
Can we run the AST check fromhttps://github.com/Lonercode/typescript-eslint/blob/65803e4127b9557ec6cf4802df12e588370d9930/packages/typescript-estree/src/convert.ts#L509, and have a big switch-case in the new file to check every ts node? |
Lonercode commentedNov 14, 2025
AST check |
fisker commentedNov 15, 2025
No need change check-modifiers.ts. In the new file export a function to check ts node, it calls |
| caseSyntaxKind.ForInStatement: | ||
| this.#checkForStatementDeclaration(node.initializer,node.kind); | ||
| this.#checkTSNode(node,node.initializer,node.kind); |
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 should be able to be removed?
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.
What fisker said 🙂
Lonercode commentedNov 19, 2025
@fisker Please let me know if I need to remove every call to the ast check function from convert.ts. |
fisker commentedNov 19, 2025
|
| } | ||
| checkModifiers(node); | ||
| checkTSNode(node,this.#throwError,initializer,kind); |
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.
initializer andkind should access fromnode directly in the check function.
| import{checkModifiers}from'./check-modifiers'; | ||
| import{isValidAssignmentTarget}from'./node-utils'; | ||
| exportfunctioncheckTSNode( |
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 prefer export ascheckSyntaxError, and rename the file accordingly.
| exportfunctioncheckTSNode( | ||
| node:ts.Node, | ||
| throwError:( |
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 won't be necessary to pass, after we merge#11775
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 means we should remove thethrowError parameter then?
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.
#11775 is merged, we should be able to throw directly.
| } | ||
| } | ||
| exportfunctioncheckForStatementDeclaration( |
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 should be an internal functionality, no need export.
| @@ -0,0 +1,196 @@ | |||
| import*astsfrom'typescript'; | |||
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 test is unnecessary, they have already been well tested.
LonercodeNov 23, 2025 • 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.
Was trying to align with codecov requirements. I can remove the file if it could still be merged. Should we also skip testing the newcheckSyntaxError function?
| checkForStatementDeclaration( | ||
| (nodeasts.ForInStatement|ts.ForOfStatement).initializer, | ||
| node.kind, | ||
| ); | ||
| break; | ||
| } | ||
| default:{ | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| functioncheckForStatementDeclaration( | ||
| initializer:ts.ForInitializer, | ||
| kind:ts.SyntaxKind, | ||
| ):void{ |
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.
| checkForStatementDeclaration( | |
| (nodeasts.ForInStatement|ts.ForOfStatement).initializer, | |
| node.kind, | |
| ); | |
| break; | |
| } | |
| default:{ | |
| break; | |
| } | |
| } | |
| } | |
| functioncheckForStatementDeclaration( | |
| initializer:ts.ForInitializer, | |
| kind:ts.SyntaxKind, | |
| ):void{ | |
| checkForStatementDeclaration(node); | |
| break; | |
| } | |
| default:{ | |
| break; | |
| } | |
| } | |
| } | |
| functioncheckForStatementDeclaration(node:ts.ForInStatement|ts.ForOfStatement):void{ | |
| const{initializer, kind}=node; |
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 issue with this is that an error is thrown unless I specify the node type incheckForStatementDeclaration so I may still need to docheckForStatmentDeclaration(node as ts.ForInStatment | ts.ForOfStatment) when calling it incheckSyntaxErrors
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.
Ts can't narrow type?
Looking at the difference,
checkSyntaxError(node: ts.Node)whileconvertNode usesTSNode
maybe change to it will fix the issue?
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.
As I tested, it works
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.
That's true but if I do this, I get an error in the converter method inconvert.ts as node ists.Node notTSNode:
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.
How about
function checkSyntaxError(tsNode: ts.node): void {checkModifiers(tsNode)const node = tsNode as TSNode;switch (...) {}}convert.ts also do similar.
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.
That works :)
| import{checkModifiers}from'./check-modifiers'; | ||
| import{isValidAssignmentTarget,createError}from'./node-utils'; | ||
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 should also define a
const SyntaxKind = ts.SyntaxKind;so other checks can be copied easier to here.
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.
Ehh should we? We don't generally do namespace property shorthands like that in the typescript-eslint monorepo. IMO it just adds a bit of clutter.
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'll add lots ofcases in switch, better align with convert.ts.
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.
At least keep it before it's done?
fisker commentedNov 27, 2025
@JoshuaKGoldberg Are you fine with just one check in this PR? Or should all checks done in this one? If we want to copy variable declaration syntax check, better wait for#11777 and#11778 to merge first. |
| default:{ | ||
| break; | ||
| } |
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.
Add this because ESLint complaint? Will this work?
| default:{ | |
| break; | |
| } | |
| // No 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.
I think it's valid, exiting with the default, for now at least.
fisker commentedNov 27, 2025
Looks good to me, thanks! 🙏 Can't approve since I'm not a team member. |
Lonercode commentedNov 27, 2025
Thanks@fisker |
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! I think we should discuss the direction a bit.
| constnode=tsNodeasTSNode; | ||
| switch(node.kind){ |
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.
[Refactor] I don't think this is a positive change for the logic? Converter functions generally directly call thecheckFor* logic they need. For example, for-in and for-of statements directly callcheckForStatementDeclaration. Now this newcheckSyntaxError function has to again switch onnode.kind. That's extra code structure and work.
@fisker please correct me if I'm wrong here, but I think it'd still be in the spirit of the issue to havecheckForStatementDeclaration be a standalone function separated out fromconvert.ts, and still called directly?
As in, the diffs toconvert.ts would look more like:
+ import { checkForStatementDeclaration } from "./checks/...";... case SyntaxKind.ForInStatement:- this.#checkForStatementDeclaration(node.initializer, node.kind);+ checkForStatementDeclaration(node.initializer, node.kind); return this.createNode<TSESTree.ForInStatement>(node, {...- #checkForStatementDeclaration(...) { ... }
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.
Take this
typescript-eslint/packages/typescript-estree/src/convert.ts
Lines 802 to 811 in16cf0f7
| if( | |
| node.caseBlock.clauses.filter( | |
| switchCase=>switchCase.kind===SyntaxKind.DefaultClause, | |
| ).length>1 | |
| ){ | |
| this.#throwError( | |
| node, | |
| "A 'default' clause cannot appear more than once in a 'switch' statement.", | |
| ); | |
| } |
SyntaxKind.ForInStatement.If we call from covert.ts directly after thecase ..., we'll have lot of lines to add. and forcing many functions to import/export.
I think a single call likecheckModifiers() used did and duplicate the bigswitch(){} in check-syntax-error.ts is much easier to maintain.
Most kind of node check will inline, instead of separate 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.
@JoshuaKGoldberg@fisker Would it prove more efficient to separate outcheckForStatmentDeclaration? I thought the point of the issue was to extract those checks?
PR Checklist
Converter#11695Overview
This PR extracts the AST check (checkForStatementDeclaration) from
convert.tsto a fileast-checks.tsas requested in the issue due to the increasing length of code in the file. Therefore, a new fie has been added (ast-checks.ts), one modified (convert.ts) and tests have been run.