chore: extract AST check from convert.ts to ast-checks.ts #11748

Lonercode · 2025-11-09T14:35:34Z

PR Checklist

Addresses an existing open issue: fixes Repo: Extract AST check from Converter #11695
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

This PR extracts the AST check (checkForStatementDeclaration) from convert.ts to a file ast-checks.ts as 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.

typescript-eslint · 2025-11-09T14:35:37Z

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

netlify · 2025-11-09T14:35:39Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`29a9c73`
🔍 Latest deploy log	https://app.netlify.com/projects/typescript-eslint/deploys/692889024bd90500080490fe
😎 Deploy Preview	https://deploy-preview-11748--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 92 (🔴 down 6 from production) Accessibility: 97 (no change from production) Best Practices: 100 (no change from production) SEO: 92 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

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

nx-cloud · 2025-11-09T14:37:13Z

View your CI Pipeline Execution ↗ for commit 29a9c73

Command	Status	Duration	Result
`nx test eslint-plugin --coverage=false`	✅ Succeeded	5m 14s	View ↗
`nx run-many -t lint`	✅ Succeeded	3m 16s	View ↗
`nx run-many -t typecheck`	✅ Succeeded	2m 9s	View ↗
`nx test typescript-estree --coverage=false`	✅ Succeeded	21s	View ↗
`nx test eslint-plugin-internal --coverage=false`	✅ Succeeded	11s	View ↗
`nx run types:build`	✅ Succeeded	2s	View ↗
`nx run generate-configs`	✅ Succeeded	7s	View ↗
`nx run integration-tests:test`	✅ Succeeded	3s	View ↗
`Additional runs (29)`	✅ Succeeded	...	View ↗

☁️ Nx Cloud last updated this comment at 2025-11-27 17:35:07 UTC

codecov · 2025-11-09T14:47:40Z

Codecov Report

❌ Patch coverage is 25.75758% with 49 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.48%. Comparing base (3252978) to head (29a9c73).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...kages/typescript-estree/src/check-syntax-errors.ts	20.96%	49 Missing ⚠️

❌ 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 the target 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

Flag	Coverage Δ
unittest	`90.48% <25.75%> (-0.01%)`	⬇️

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

Files with missing lines	Coverage Δ
packages/typescript-estree/src/convert.ts	`31.33% <100.00%> (+0.46%)`	⬆️
...kages/typescript-estree/src/check-syntax-errors.ts	`20.96% <20.96%> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

fisker · 2025-11-14T01:02:01Z

Can we run the AST check from https://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 · 2025-11-14T13:29:08Z

Can we run the AST check from https://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?

AST check checkModifier function is in https://github.com/Lonercode/typescript-eslint/blob/65803e4127b9557ec6cf4802df12e588370d9930/packages/typescript-estree/src/check-modifiers.ts#L139. Do you mean it should be in the new file with a check for every ts node? Or I modify checkForStatementDeclaration in the new file?

fisker · 2025-11-15T00:51:30Z

No need change check-modifiers.ts.

In the new file export a function to check ts node, it calls checkModifier and do other checks currently lives in convert.ts

fisker · 2025-11-16T19:02:19Z

packages/typescript-estree/src/convert.ts


      case SyntaxKind.ForInStatement:
-        this.#checkForStatementDeclaration(node.initializer, node.kind);
+        this.#checkTSNode(node, node.initializer, node.kind);


This should be able to be removed?

JoshuaKGoldberg

What fisker said 🙂

Lonercode · 2025-11-19T09:16:25Z

@fisker Please let me know if I need to remove every call to the ast check function from convert.ts.

fisker · 2025-11-19T12:42:42Z

checkTSNode should only call in converter, like the old checkModofiers did.

fisker · 2025-11-19T13:02:41Z

packages/typescript-estree/src/convert.ts

    }

-    checkModifiers(node);
+    checkTSNode(node, this.#throwError, initializer, kind);


initializer and kind should access from node directly in the check function.

fisker · 2025-11-23T08:02:05Z

packages/typescript-estree/src/ast-checks.ts

+import { checkModifiers } from './check-modifiers';
+import { isValidAssignmentTarget } from './node-utils';
+
+export function checkTSNode(


I prefer export as checkSyntaxError, and rename the file accordingly.

fisker · 2025-11-23T08:03:05Z

packages/typescript-estree/src/ast-checks.ts

+
+export function checkTSNode(
+  node: ts.Node,
+  throwError: (


This won't be necessary to pass, after we merge #11775

This means we should remove the throwError parameter then?

#11775 is merged, we should be able to throw directly.

fisker · 2025-11-23T08:03:57Z

packages/typescript-estree/src/ast-checks.ts

+  }
+}
+
+export function checkForStatementDeclaration(


This should be an internal functionality, no need export.

fisker · 2025-11-23T08:05:11Z

packages/typescript-estree/tests/lib/ast-checks.test.ts

@@ -0,0 +1,196 @@
+import * as ts from 'typescript';


This test is unnecessary, they have already been well tested.

Was trying to align with codecov requirements. I can remove the file if it could still be merged. Should we also skip testing the new checkSyntaxError function?

…m/Lonercode/typescript-eslint into extract-ast-checks-from-converter

fisker · 2025-11-27T15:13:11Z

packages/typescript-estree/src/check-syntax-errors.ts

+      checkForStatementDeclaration(
+        (node as ts.ForInStatement | ts.ForOfStatement).initializer,
+        node.kind,
+      );
+      break;
+    }
+    default: {
+      break;
+    }
+  }
+}
+
+function checkForStatementDeclaration(
+  initializer: ts.ForInitializer,
+  kind: ts.SyntaxKind,
+): void {


Suggested change

checkForStatementDeclaration(

(node as ts.ForInStatement | ts.ForOfStatement).initializer,

node.kind,

);

break;

}

default: {

break;

}

}

}

function checkForStatementDeclaration(

initializer: ts.ForInitializer,

kind: ts.SyntaxKind,

): void {

checkForStatementDeclaration(node);

break;

}

default: {

break;

}

}

}

function checkForStatementDeclaration(node: ts.ForInStatement | ts.ForOfStatement): void {

const {initializer, kind} = node;

The only issue with this is that an error is thrown unless I specify the node type in checkForStatementDeclaration so I may still need to do checkForStatmentDeclaration(node as ts.ForInStatment | ts.ForOfStatment) when calling it in checkSyntaxErrors

Ts can't narrow type?

Looking at the difference,

checkSyntaxError(node: ts.Node)

while convertNode uses TSNode

https://github.com/Lonercode/typescript-eslint/blob/ee1ada9db5229902f70faa19dc27950dc457cc7d/packages/typescript-estree/src/convert.ts#L666

maybe change to it will fix the issue?

As I tested, it works

That's true but if I do this, I get an error in the converter method in convert.ts as node is ts.Node not TSNode:

How about

function checkSyntaxError(tsNode: ts.node): void { checkModifiers(tsNode) const node = tsNode as TSNode; switch (...) {} }

convert.ts also do similar.

That works :)

fisker · 2025-11-27T15:51:10Z

packages/typescript-estree/src/check-syntax-errors.ts

+
+import { checkModifiers } from './check-modifiers';
+import { isValidAssignmentTarget, createError } from './node-utils';
+


We should also define a

const SyntaxKind = ts.SyntaxKind;

here, https://github.com/Lonercode/typescript-eslint/blob/ee1ada9db5229902f70faa19dc27950dc457cc7d/packages/typescript-estree/src/convert.ts#L42

so other checks can be copied easier to here.

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.

We'll add lots of cases in switch, better align with convert.ts.

At least keep it before it's done?

fisker · 2025-11-27T15:54:43Z

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

fisker · 2025-11-27T17:34:38Z

packages/typescript-estree/src/check-syntax-errors.ts

+    default: {
+      break;
+    }


Add this because ESLint complaint? Will this work?

Suggested change

default: {

break;

}

// No default

I think it's valid, exiting with the default, for now at least.

fisker · 2025-11-27T17:37:26Z

Looks good to me, thanks! 🙏

Can't approve since I'm not a team member.

Lonercode · 2025-11-27T17:43:01Z

Thanks @fisker

JoshuaKGoldberg

Thanks for getting started on this! I think we should discuss the direction a bit.

JoshuaKGoldberg · 2025-11-28T17:45:45Z

packages/typescript-estree/src/check-syntax-errors.ts

+
+  const node = tsNode as TSNode;
+
+  switch (node.kind) {


[Refactor] I don't think this is a positive change for the logic? Converter functions generally directly call the checkFor* logic they need. For example, for-in and for-of statements directly call checkForStatementDeclaration. Now this new checkSyntaxError function has to again switch on node.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 have checkForStatementDeclaration be a standalone function separated out from convert.ts, and still called directly?

As in, the diffs to convert.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(...) { ... }

Take this

typescript-eslint/packages/typescript-estree/src/convert.ts

Lines 802 to 811 in 16cf0f7

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

);

}

as example, normally the check just few lines (there are even simpler checks, eg: 1). Not every node need such long code like SyntaxKind.ForInStatement.

If we call from covert.ts directly after the case ..., we'll have lot of lines to add. and forcing many functions to import/export.

I think a single call like checkModifiers() used did and duplicate the big switch(){} in check-syntax-error.ts is much easier to maintain.

Most kind of node check will inline, instead of separate function.

@JoshuaKGoldberg @fisker Would it prove more efficient to separate out checkForStatmentDeclaration? I thought the point of the issue was to extract those checks?

Chore: Extract AST check from convert.ts to ast-checks.ts

744c678

Lonercode changed the title ~~Chore: Extract AST check from convert.ts to ast-checks.ts~~ chore: Extract AST check from convert.ts to ast-checks.ts Nov 9, 2025

chore: extract AST check from convert.ts to ast-checks.ts

b52f216

Lonercode changed the title ~~chore: Extract AST check from convert.ts to ast-checks.ts~~ chore: extract AST check from convert.ts to ast-checks.ts Nov 9, 2025

Lonercode added 3 commits November 9, 2025 16:51

chore: add test for ast-checks.ts

9270fc4

chore: add test for ast-checks.ts

6ced6b3

chore: add test for ast-checks.ts

65803e4

Lonercode added 3 commits November 15, 2025 13:53

chore: add function in ast-checks to run ts node checks

b628bfa

chore: fix lint errors

78f1686

chore: update test

001ea12

fisker reviewed Nov 16, 2025

View reviewed changes

JoshuaKGoldberg requested changes Nov 17, 2025

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Nov 18, 2025

chore: address review

f696ce1

fisker reviewed Nov 19, 2025

View reviewed changes

chore: review update

5eb48ef

fisker reviewed Nov 23, 2025

View reviewed changes

chore: update based on reviews

37c500f

Lonercode and others added 4 commits November 27, 2025 15:24

Merge branch 'main' into extract-ast-checks-from-converter

dc4dcc0

chore: minor fix

4e5902b

Merge branch 'extract-ast-checks-from-converter' of https://github.co…

91d2b00

…m/Lonercode/typescript-eslint into extract-ast-checks-from-converter

chore: merge main and update code

336c584

fisker reviewed Nov 27, 2025

View reviewed changes

chore: minor update

ee1ada9

fisker reviewed Nov 27, 2025

View reviewed changes

chore: update based on reviews

29a9c73

fisker reviewed Nov 27, 2025

View reviewed changes

JoshuaKGoldberg requested changes Nov 28, 2025

View reviewed changes


		import { checkModifiers } from './check-modifiers';
		import { isValidAssignmentTarget, createError } from './node-utils';

	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.",
	);
	}

Uh oh!

chore: extract AST check from convert.ts to ast-checks.ts #11748

Are you sure you want to change the base?

chore: extract AST check from convert.ts to ast-checks.ts #11748

Uh oh!

Conversation

Lonercode commented Nov 9, 2025

PR Checklist

Overview

Uh oh!

typescript-eslint bot commented Nov 9, 2025

Uh oh!

netlify bot commented Nov 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for typescript-eslint ready!

Uh oh!

nx-cloud bot commented Nov 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fisker commented Nov 14, 2025

Uh oh!

Lonercode commented Nov 14, 2025

Uh oh!

fisker commented Nov 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Lonercode commented Nov 19, 2025

Uh oh!

fisker commented Nov 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisker Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lonercode Nov 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisker Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisker Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

netlify bot commented Nov 9, 2025 •

edited

Loading

nx-cloud bot commented Nov 9, 2025 •

edited

Loading

codecov bot commented Nov 9, 2025 •

edited

Loading

fisker Nov 26, 2025 •

edited

Loading

Lonercode Nov 23, 2025 •

edited

Loading

fisker Nov 27, 2025 •

edited

Loading

fisker Nov 27, 2025 •

edited

Loading

fisker Nov 28, 2025 •

edited

Loading