feat: support TypeScript syntax in `no-unused-vars` #19812

Tanujkanti4441 · 2025-06-02T12:29:27Z

Prerequisites checklist

I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Added typescript syntax support in no-unused-vars rule.

Is there anything you'd like reviewers to focus on?

Refs #19173

Moving forward #19580

netlify · 2025-06-02T12:29:32Z

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	`4213540`
🔍 Latest deploy log	https://app.netlify.com/projects/docs-eslint/deploys/687a0e80b60e5200086b15e9

github-actions · 2025-06-20T22:35:19Z

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

lumirlumir

Nice work! I’ve left a few comments about some missing parts 😄

lumirlumir · 2025-07-07T10:58:36Z

lib/rules/no-unused-vars.js

+ * @private
+ */
+function isDefinitionFile(filename) {
+	return filename?.endsWith(".d.ts") ?? false;


Can we add a logic to the isDefinitionFile helper function to also check for .d.mts and .d.cts files?

Here is a reference for it:

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-7.html#new-file-extensions

lumirlumir · 2025-07-07T11:03:24Z

lib/rules/no-unused-vars.js

 		},
 	},


Suggested change

},

},

},

dialects: ["typescript", "javascript"],

language: "javascript",

},

Could you add dialects and language fields to the meta property? As far as I know, the newly updated rules that support TypeScript use these fields.

eslint/lib/rules/no-var.js

Lines 202 to 203 in 28cc7ab

dialects: ["typescript", "javascript"],

language: "javascript",

lumirlumir · 2025-07-07T11:08:05Z

lib/rules/no-unused-vars.js

+ * @returns {boolean} `true` if the identifier is only used in a type position
+ * @private
+ */
+function isTypeReference(node) {


Can we move the isDefinitionFile and isTypeReference helper functions under the Helpers comment?

Here is a reference for it:

eslint/lib/rules/accessor-pairs.js

Lines 31 to 60 in 28cc7ab

//------------------------------------------------------------------------------

// Helpers

//------------------------------------------------------------------------------

/**

* Checks whether or not the given lists represent the equal tokens in the same order.

* Tokens are compared by their properties, not by instance.

* @param {Token[]} left First list of tokens.

* @param {Token[]} right Second list of tokens.

* @returns {boolean} `true` if the lists have same tokens.

*/

function areEqualTokenLists(left, right) {

if (left.length !== right.length) {

return false;

}

for (let i = 0; i < left.length; i++) {

const leftToken = left[i],

rightToken = right[i];

if (

leftToken.type !== rightToken.type ||

leftToken.value !== rightToken.value

) {

return false;

}

}

return true;

}

JoshuaKGoldberg

Very exciting!

JoshuaKGoldberg · 2025-07-08T13:08:04Z

lib/rules/no-unused-vars.js

+		 * @throws {Error} If `value` is null or undefined.
+		 * @returns {ASTNode} The provided node, if it is not null or undefined.
+		 */
+		function nullThrows(value, message) {


This is an interesting piece to copy over. The typescript-eslint project style is to very aggressively handle edge cases exposed by types. But it does sometimes go overboard and seem to require extra logic in places that don't need it (typescript-eslint/typescript-eslint#10077).

Some of the places that nullThrows are also unnecessary. For example, the node.parent passed to nullThrows in the Program selector will be unnecessary once typescript-eslint/typescript-eslint#11334 is resolved.

The ESLint project doesn't generally go that extra step. So, raising for other reviewers: should we just omit nullThrows & NullThrowsReasons in this PR?

JoshuaKGoldberg · 2025-07-08T13:09:38Z

lib/rules/no-unused-vars.js

+				if (
+					parent.parent === "FunctionExpression" &&
+					parent.parent.decarators &&
+					parent.parent.decarators.length > 0


[Bug] decorators, not decarators. The fact that this isn't failing any tests belies a lack of coverage.

JoshuaKGoldberg · 2025-07-08T13:13:47Z

lib/rules/no-unused-vars.js

+			return typeof filename === "string"
+				? /\.d\.(ts|mts|cts)$/u.test(filename)
+				: false;
+		}


[Refactor] This is very similar to the isDeclareInDTSFile logic in no-shadow.js, added in #19565. Which also had a review comment about matching all definition file extensions: #19565 (comment)

I think that that's a sign there needs to be a shared utility function.

JoshuaKGoldberg · 2025-07-08T13:18:54Z

lib/rules/no-unused-vars.js

-				if (nodes.includes(scope.block)) {
+				if (
+					(Array.isArray(nodes) && nodes.includes(scope.block)) ||
+					(nodes instanceof Set && nodes.has(scope.block))


[Docs] The JSDoc for this function still says nodes can only ever be an array.

JoshuaKGoldberg · 2025-07-08T13:19:53Z

lib/rules/no-unused-vars.js

@@ -755,7 +926,7 @@ module.exports = {

 					// skip variables marked with markVariableAsUsed()
 					if (
-						!config.reportUsedIgnorePattern &&
+						// !config.reportUsedIgnorePattern &&


Forgot to remove this one, but with this line following test is falling:

{ code: ` export enum Foo { _A, } `, options: [ { reportUsedIgnorePattern: true, varsIgnorePattern: "_" }, ], },

And without this line, I think reportUsedIgnorePattern: true won't report the markedAsUsedVariables with ignored pattern when they are used, so should we report them in this condition?

tests/lib/rules/no-unused-vars.js

JoshuaKGoldberg · 2025-07-08T13:38:09Z

tests/lib/rules/no-unused-vars.js

+		},
+		{
+			filename: "foo.d.ts",
+			code: "class Foo {}\ndeclare class Bar {}\nexport {};",


[Question] Is there a reason this uses a single line string instead of a multi-line template string? The original tseslint tests use the latter.

Actually, while writing suggestions, the column number is not matching properly for me, so I tried this. Please suggest if I can do something better here?

I don't follow, what do you mean by the column number not matching properly?

Like here column: 10 must be same for both code and output but somehow I was getting error on multiline code.

{ code: "import { Nullable } from 'nullable';\nconst a: string = 'hello';\nconsole.log(a);", errors: [ { column: 10, data: { action: "defined", additional: "", varName: "Nullable", }, line: 1, messageId: "unusedVar", suggestions: [ { output: "\nconst a: string = 'hello';\nconsole.log(a);", messageId: "removeVar", data: { varName: "Nullable" }, }, ], }, ], },

It passes on my machine. Maybe there's something with indentation that's misaligned?

Reference commit: 59ba196

nzakas

This adds over 500 lines to the current rule, and I'm not sure this is the right place to add all of that logic. I think ideally, reference tracking would end up in eslint-scope and not be implemented inside of a single rule.

I'd like to propose we hold off on this and think about whether or not we want to add type reference tracking into eslint-scope. @eslint/eslint-tsc

nzakas · 2025-07-23T15:18:08Z

package.json

@@ -117,6 +117,7 @@
    "@humanwhocodes/retry": "^0.4.2",
    "@types/estree": "^1.0.6",
    "@types/json-schema": "^7.0.15",
+    "@typescript-eslint/scope-manager": "^8.34.1",


I think this was added accidentally?

No, it is an intentional change. It is used in the rule.

mdjermanovic · 2025-07-24T14:06:41Z

This adds over 500 lines to the current rule, and I'm not sure this is the right place to add all of that logic. I think ideally, reference tracking would end up in eslint-scope and not be implemented inside of a single rule.

I'd like to propose we hold off on this and think about whether or not we want to add type reference tracking into eslint-scope.

Doesn't @typescript-eslint/scope-manager already track type references? So, when this rule is used with @typescript-eslint/parser, which uses @typescript-eslint/scope-manager as scope manager, type references should already be available to this rule in scopes?

nzakas · 2025-07-24T15:43:36Z

@mdjermanovic Perhaps? 🤔 Do we want to assume that @typescript-eslint/scope-manager is always being used when we see TypeScript syntax? That seems like a shaky assumption moving forward.

mdjermanovic · 2025-07-24T16:55:40Z

Based on this playground example, it seems that @typescript-eslint/scope-manager does track type references.

I'd say it's reasonable to expect that a TypeScript-aware parser provides a TypeScript-aware scope manager.

nzakas · 2025-07-29T14:27:56Z

I suppose --- I'm just still uncomfortable with the added 500 lines of code to this rule. Is that really needed?

mdjermanovic · 2025-08-01T11:07:53Z

I haven't reviewed the changes in detail, but I see that @typescript-eslint/scope-manager is used directly in the rule:

https://github.com/Tanujkanti4441/eslint/blob/421354008ddfbd023f3d9d038b0277dc19445af7/lib/rules/no-unused-vars.js#L13-L16

@Tanujkanti4441 if we would assume that, for TypeScript code, scopes have already been analyzed by @typescript-eslint/scope-manager or a compatible scope manager, would that, and how much, reduce the complexity and the number of lines added to this rule?

Tanujkanti4441 · 2025-08-10T15:22:49Z

I haven't reviewed the changes in detail, but I see that @typescript-eslint/scope-manager is used directly in the rule:

https://github.com/Tanujkanti4441/eslint/blob/421354008ddfbd023f3d9d038b0277dc19445af7/lib/rules/no-unused-vars.js#L13-L16

@Tanujkanti4441 if we would assume that, for TypeScript code, scopes have already been analyzed by @typescript-eslint/scope-manager or a compatible scope manager, would that, and how much, reduce the complexity and the number of lines added to this rule?

@mdjermanovic, I mostly tried to replicate the code of typescript-eslint's no-unused-vars rule and using of @typescript-eslint/scope-manager is also a part of that process and in most of the code we are marking the identifiers in ts specific nodes as used, so can we assume that we may not simplify this further with the use of @typescript-eslint/scope-manager? Or am I missing any detail here?

nzakas · 2025-08-19T14:22:20Z

@Tanujkanti4441 the overall question is about the amount of code this adds to the rule. It seems like if variable reference tracking is already implemented in the typescript-eslint scope manager, that it shouldn't take 500 lines of code to track TypeScript identifiers.

feat: support TS syntax in no-unused-vars

828f5db

github-project-automation bot added this to Triage Jun 2, 2025

eslint-github-bot bot added the feature This change adds a new feature to ESLint label Jun 2, 2025

github-project-automation bot moved this to Needs Triage in Triage Jun 2, 2025

github-actions bot added the rule Relates to ESLint's core rules label Jun 2, 2025

add tests

8c8e73d

nzakas moved this from Needs Triage to Implementing in Triage Jun 10, 2025

add tests

42fc6da

github-actions bot added the Stale label Jun 20, 2025

add more ts tests

084a407

Tanujkanti4441 removed the Stale label Jun 24, 2025

Tanujkanti4441 added 4 commits July 4, 2025 15:40

add tests

02cdd7f

fix CI error

c7140a0

add default case to resolve error

9c1a1c9

add docs examples

47a0c71

Tanujkanti4441 marked this pull request as ready for review July 4, 2025 15:25

Tanujkanti4441 requested a review from a team as a code owner July 4, 2025 15:25

lumirlumir added the accepted There is consensus among the team that this change meets the criteria for inclusion label Jul 7, 2025

lumirlumir requested changes Jul 7, 2025

View reviewed changes

apply suggestions

e4494fa

JoshuaKGoldberg mentioned this pull request Jul 8, 2025

Change Request: Make rules TypeScript syntax-aware #19173

Open

19 tasks

JoshuaKGoldberg requested changes Jul 8, 2025

View reviewed changes

Tanujkanti4441 added 2 commits July 11, 2025 13:55

apply some suggestions

602f618

format test code

4213540

nzakas requested a review from lumirlumir July 23, 2025 15:16

nzakas reviewed Jul 23, 2025

View reviewed changes

-		},
-	},
+                },
+                dialects: ["typescript", "javascript"],
+		language: "javascript",
+        },

	dialects: ["typescript", "javascript"],
	language: "javascript",

	//------------------------------------------------------------------------------
	// Helpers
	//------------------------------------------------------------------------------

	/**
	* Checks whether or not the given lists represent the equal tokens in the same order.
	* Tokens are compared by their properties, not by instance.
	* @param {Token[]} left First list of tokens.
	* @param {Token[]} right Second list of tokens.
	* @returns {boolean} `true` if the lists have same tokens.
	*/
	function areEqualTokenLists(left, right) {
	if (left.length !== right.length) {
	return false;
	}

	for (let i = 0; i < left.length; i++) {
	const leftToken = left[i],
	rightToken = right[i];

	if (
	leftToken.type !== rightToken.type \|\|
	leftToken.value !== rightToken.value
	) {
	return false;
	}
	}

	return true;
	}

Uh oh!

feat: support TypeScript syntax in no-unused-vars #19812

Are you sure you want to change the base?

feat: support TypeScript syntax in no-unused-vars #19812

Conversation

Tanujkanti4441 commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

Uh oh!

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

Uh oh!

✅ Deploy Preview for docs-eslint canceled.

Uh oh!

github-actions bot commented Jun 20, 2025

Uh oh!

lumirlumir left a comment

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!

JoshuaKGoldberg left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Tanujkanti4441 Jul 13, 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!

nzakas left a comment

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!

mdjermanovic commented Jul 24, 2025

Uh oh!

nzakas commented Jul 24, 2025

Uh oh!

mdjermanovic commented Jul 24, 2025

Uh oh!

nzakas commented Jul 29, 2025

Uh oh!

mdjermanovic commented Aug 1, 2025

Uh oh!

Tanujkanti4441 commented Aug 10, 2025

Uh oh!

feat: support TypeScript syntax in `no-unused-vars` #19812

feat: support TypeScript syntax in `no-unused-vars` #19812

Tanujkanti4441 commented Jun 2, 2025 •

edited

Loading

netlify bot commented Jun 2, 2025 •

edited

Loading

Tanujkanti4441 Jul 13, 2025 •

edited

Loading