From 7117fcc557bb99dcfc7ea6daf8290b213029b040 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Sat, 2 Mar 2024 20:41:10 -0800 Subject: [PATCH 1/7] WIP --- nx.json | 1 + .../src/util/collectUnusedVariables.ts | 44 ++++++- .../no-unused-vars/no-unused-vars.test.ts | 107 ++++++++++++++++++ 3 files changed, 151 insertions(+), 1 deletion(-) diff --git a/nx.json b/nx.json index f18f497922fe..2c9781390b3e 100644 --- a/nx.json +++ b/nx.json @@ -1,5 +1,6 @@ { "$schema": "./node_modules/nx/schemas/nx-schema.json", + "neverConnectToCloud": true, "nxCloudAccessToken": "YjIzMmMxMWItMjhiMS00NWY2LTk1NWYtYWU3YWQ0YjE4YjBlfHJlYWQ=", "release": { "changelog": { diff --git a/packages/eslint-plugin/src/util/collectUnusedVariables.ts b/packages/eslint-plugin/src/util/collectUnusedVariables.ts index dbe749e62900..2617caf69019 100644 --- a/packages/eslint-plugin/src/util/collectUnusedVariables.ts +++ b/packages/eslint-plugin/src/util/collectUnusedVariables.ts @@ -1,6 +1,7 @@ import { ImplicitLibVariable, ScopeType, + Variable, Visitor, } from '@typescript-eslint/scope-manager'; import type { TSESTree } from '@typescript-eslint/utils'; @@ -10,6 +11,7 @@ import { ESLintUtils, TSESLint, } from '@typescript-eslint/utils'; +import { isDefinitionFile } from './misc'; class UnusedVarsVisitor< MessageIds extends string, @@ -22,6 +24,7 @@ class UnusedVarsVisitor< readonly #scopeManager: TSESLint.Scope.ScopeManager; // readonly #unusedVariables = new Set(); + readonly #isDefinitionFile: boolean; private constructor(context: TSESLint.RuleContext) { super({ @@ -32,6 +35,8 @@ class UnusedVarsVisitor< context.sourceCode.scopeManager, 'Missing required scope manager', ); + + this.#isDefinitionFile = isDefinitionFile(context.filename); } public static collectUnusedVariables< @@ -60,7 +65,13 @@ class UnusedVarsVisitor< scope: TSESLint.Scope.Scope, unusedVariables = new Set(), ): ReadonlySet { - for (const variable of scope.variables) { + const implicitlyExported = allVariablesImplicitlyExported( + scope, + this.#isDefinitionFile, + ); + + // TODO: do this better than the ternary + for (const variable of implicitlyExported ? [] : scope.variables) { if ( // skip function expression names, scope.functionExpressionScope || @@ -438,6 +449,37 @@ function isExported(variable: TSESLint.Scope.Variable): boolean { }); } +function allVariablesImplicitlyExported( + scope: TSESLint.Scope.Scope, + isDefinitionFile: boolean, +): boolean { + // TODO: does this also happen in ambient module declarations? + if (!isDefinitionFile || scope.type !== ScopeType.tsModule) { + return false; + } + + function isExportImportEquals(variable: Variable): boolean { + for (const def of variable.defs) { + if ( + def.type === TSESLint.Scope.DefinitionType.ImportBinding && + def.node.type === AST_NODE_TYPES.TSImportEqualsDeclaration && + def.node.parent.type === AST_NODE_TYPES.ExportNamedDeclaration + ) { + return true; + } + } + return false; + } + + for (const variable of scope.variables) { + if (isExported(variable) && !isExportImportEquals(variable)) { + return false; + } + } + + return true; +} + const LOGICAL_ASSIGNMENT_OPERATORS = new Set(['&&=', '||=', '??=']); /** diff --git a/packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts b/packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts index 04c218b7631e..fb6b5cf2b6eb 100644 --- a/packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts @@ -1123,6 +1123,63 @@ export namespace Bar { export import TheFoo = Foo; } `, + ` +const foo = 1; +export = foo; + `, + ` +const Foo = 1; +interface Foo { + bar: string; +} +export = Foo; + `, + ` +interface Foo { + bar: string; +} +export = Foo; + `, + ` +type Foo = 1; +export = Foo; + `, + ` +type Foo = 1; +export = {} as Foo; + `, + ` +declare module 'foo' { + type Foo = 1; + export = Foo; +} + `, + ` +namespace Foo { + export const foo = 1; +} +export namespace Bar { + export import TheFoo = Foo; +} + `, + // https://github.com/typescript-eslint/typescript-eslint/issues/2867 + { + code: ` +export namespace Foo { + const foo: 1234; +} + `, + filename: 'foo.d.ts', + }, + { + code: ` +export namespace Foo { + export import Bar = Something.Bar; + const foo: 1234; +} + `, + filename: 'foo.d.ts', + }, ], invalid: [ @@ -1950,5 +2007,55 @@ export namespace Bar { }, ], }, + // https://github.com/typescript-eslint/typescript-eslint/issues/2867 + { + code: ` +export namespace Foo { + const foo: 1234; + export const bar: string; + export namespace NS { + const baz: 1234; + } +} + `, + filename: 'foo.d.ts', + errors: [ + { + messageId: 'unusedVar', + line: 3, + column: 9, + data: { + varName: 'foo', + action: 'defined', + additional: '', + }, + }, + ], + }, + { + code: ` +export namespace Foo { + export import Bar = Something.Bar; + const foo: 1234; + export const bar: string; + export namespace NS { + const baz: 1234; + } +} + `, + filename: 'foo.d.ts', + errors: [ + { + messageId: 'unusedVar', + line: 4, + column: 9, + data: { + varName: 'foo', + action: 'defined', + additional: '', + }, + }, + ], + }, ], }); From 3d2fc3bf095be8edc5e16967fde098997ff9d591 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Sat, 2 Mar 2024 21:58:48 -0800 Subject: [PATCH 2/7] WIP --- .../src/util/collectUnusedVariables.ts | 13 +++++-- .../no-unused-vars/no-unused-vars.test.ts | 34 +++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/src/util/collectUnusedVariables.ts b/packages/eslint-plugin/src/util/collectUnusedVariables.ts index 2617caf69019..cd215124d64c 100644 --- a/packages/eslint-plugin/src/util/collectUnusedVariables.ts +++ b/packages/eslint-plugin/src/util/collectUnusedVariables.ts @@ -70,7 +70,6 @@ class UnusedVarsVisitor< this.#isDefinitionFile, ); - // TODO: do this better than the ternary for (const variable of implicitlyExported ? [] : scope.variables) { if ( // skip function expression names, @@ -454,10 +453,20 @@ function allVariablesImplicitlyExported( isDefinitionFile: boolean, ): boolean { // TODO: does this also happen in ambient module declarations? - if (!isDefinitionFile || scope.type !== ScopeType.tsModule) { + if ( + !isDefinitionFile || + !( + scope.type === ScopeType.tsModule || + scope.type === ScopeType.module || + scope.type === ScopeType.global + ) + ) { return false; } + // TODO: test modules, globals + // TODO: look for `export {}` + function isExportImportEquals(variable: Variable): boolean { for (const def of variable.defs) { if ( diff --git a/packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts b/packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts index fb6b5cf2b6eb..8827b82f30e2 100644 --- a/packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts @@ -1176,6 +1176,15 @@ export namespace Foo { export namespace Foo { export import Bar = Something.Bar; const foo: 1234; +} + `, + filename: 'foo.d.ts', + }, + { + code: ` +declare module 'foo' { + export import Bar = Something.Bar; + const foo: 1234; } `, filename: 'foo.d.ts', @@ -2041,6 +2050,31 @@ export namespace Foo { export namespace NS { const baz: 1234; } +} + `, + filename: 'foo.d.ts', + errors: [ + { + messageId: 'unusedVar', + line: 4, + column: 9, + data: { + varName: 'foo', + action: 'defined', + additional: '', + }, + }, + ], + }, + { + code: ` +declare module 'foo' { + export import Bar = Something.Bar; + const foo: 1234; + export const bar: string; + export namespace NS { + const baz: 1234; + } } `, filename: 'foo.d.ts', From f9f18c54c8315294588e328848dcb15d42eec81c Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Wed, 6 Mar 2024 15:08:56 -0800 Subject: [PATCH 3/7] revert nx --- nx.json | 1 - 1 file changed, 1 deletion(-) diff --git a/nx.json b/nx.json index 2c9781390b3e..f18f497922fe 100644 --- a/nx.json +++ b/nx.json @@ -1,6 +1,5 @@ { "$schema": "./node_modules/nx/schemas/nx-schema.json", - "neverConnectToCloud": true, "nxCloudAccessToken": "YjIzMmMxMWItMjhiMS00NWY2LTk1NWYtYWU3YWQ0YjE4YjBlfHJlYWQ=", "release": { "changelog": { From 02d1b06960831ed2034326e61faafe3beb3e5e94 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Sat, 25 Jan 2025 13:38:19 +0200 Subject: [PATCH 4/7] initial implementation --- .../src/util/collectUnusedVariables.ts | 109 ++++--- .../no-unused-vars/no-unused-vars.test.ts | 298 ++++++++++++++++-- 2 files changed, 340 insertions(+), 67 deletions(-) diff --git a/packages/eslint-plugin/src/util/collectUnusedVariables.ts b/packages/eslint-plugin/src/util/collectUnusedVariables.ts index a71ffca7bfbd..d03ab74b2e0e 100644 --- a/packages/eslint-plugin/src/util/collectUnusedVariables.ts +++ b/packages/eslint-plugin/src/util/collectUnusedVariables.ts @@ -1,4 +1,7 @@ -import type { ScopeVariable, Variable } from '@typescript-eslint/scope-manager'; +import type { + ScopeManager, + ScopeVariable, +} from '@typescript-eslint/scope-manager'; import type { TSESTree } from '@typescript-eslint/utils'; import { @@ -30,10 +33,7 @@ interface MutableVariableAnalysis { * This class leverages an AST visitor to mark variables as used via the * `eslintUsed` property. */ -class UnusedVarsVisitor< - MessageIds extends string, - Options extends readonly unknown[], -> extends Visitor { +class UnusedVarsVisitor extends Visitor { /** * We keep a weak cache so that multiple rules can share the calculation */ @@ -81,32 +81,26 @@ class UnusedVarsVisitor< readonly #scopeManager: TSESLint.Scope.ScopeManager; - private constructor(context: TSESLint.RuleContext) { + private constructor(scopeManager: ScopeManager, isDefinitionFile: boolean) { super({ visitChildrenEvenIfSelectorExists: true, }); - const scopeManager = ESLintUtils.nullThrows( - context.sourceCode.scopeManager, - 'Missing required scope manager', - ); - this.#scopeManager = scopeManager; - this.#isDefinitionFile = isDefinitionFile(context.filename); + this.#isDefinitionFile = isDefinitionFile; } - public static collectUnusedVariables< - MessageIds extends string, - Options extends readonly unknown[], - >(context: TSESLint.RuleContext): VariableAnalysis { - const program = context.sourceCode.ast; - + public static collectUnusedVariables( + program: TSESTree.Program, + scopeManager: ScopeManager, + isDefinitionFile: boolean, + ): VariableAnalysis { const cached = this.RESULTS_CACHE.get(program); if (cached) { return cached; } - const visitor = new this(context); + const visitor = new this(scopeManager, isDefinitionFile); visitor.visit(program); const unusedVars = visitor.collectUnusedVariables( @@ -188,7 +182,7 @@ class UnusedVarsVisitor< this.#isDefinitionFile, ); - for (const variable of implicitlyExported ? [] : scope.variables) { + for (const variable of scope.variables) { // cases that we don't want to treat as used or unused if ( // implicit lib variables (from @typescript-eslint/scope-manager) @@ -199,6 +193,7 @@ class UnusedVarsVisitor< } if ( + implicitlyExported || // variables marked with markVariableAsUsed() variable.eslintUsed || // basic exported variables @@ -487,44 +482,61 @@ function isExported(variable: ScopeVariable): boolean { }); } +function getStatementsOfNode( + block: TSESTree.Program | TSESTree.TSModuleDeclaration, +): TSESTree.ProgramStatement[] { + if (block.type === AST_NODE_TYPES.Program) { + return block.body; + } + + if (block.body == null) { + return []; + } + + return block.body.body; +} + +function hasOverridingExportStatement( + body: TSESTree.ProgramStatement[], +): boolean { + for (const statement of body) { + if ( + (statement.type === AST_NODE_TYPES.ExportNamedDeclaration && + statement.declaration == null) || + statement.type === AST_NODE_TYPES.ExportAllDeclaration || + statement.type === AST_NODE_TYPES.TSExportAssignment + ) { + return true; + } + + if ( + statement.type === AST_NODE_TYPES.ExportDefaultDeclaration && + statement.declaration.type === AST_NODE_TYPES.Identifier + ) { + return true; + } + } + + return false; +} + function allVariablesImplicitlyExported( scope: TSESLint.Scope.Scope, isDefinitionFile: boolean, ): boolean { - // TODO: does this also happen in ambient module declarations? if ( !isDefinitionFile || - !( - scope.type === ScopeType.tsModule || - scope.type === ScopeType.module || - scope.type === ScopeType.global - ) + !(scope.type === ScopeType.tsModule || scope.type === ScopeType.module) ) { return false; } - // TODO: test modules, globals - // TODO: look for `export {}` + const body = getStatementsOfNode(scope.block); - function isExportImportEquals(variable: Variable): boolean { - for (const def of variable.defs) { - if ( - def.type === TSESLint.Scope.DefinitionType.ImportBinding && - def.node.type === AST_NODE_TYPES.TSImportEqualsDeclaration && - def.node.parent.type === AST_NODE_TYPES.ExportNamedDeclaration - ) { - return true; - } - } + if (hasOverridingExportStatement(body)) { return false; } - for (const variable of scope.variables) { - if (isExported(variable) && !isExportImportEquals(variable)) { - return false; - } - } - return true; } @@ -881,5 +893,12 @@ export function collectVariables< >( context: Readonly>, ): VariableAnalysis { - return UnusedVarsVisitor.collectUnusedVariables(context); + return UnusedVarsVisitor.collectUnusedVariables( + context.sourceCode.ast, + ESLintUtils.nullThrows( + context.sourceCode.scopeManager, + 'Missing required scope manager', + ), + isDefinitionFile(context.filename), + ); } diff --git a/packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts b/packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts index 96efbdedf227..108928882074 100644 --- a/packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts @@ -1168,20 +1168,15 @@ export class Foo { }, ], }, - // https://github.com/typescript-eslint/typescript-eslint/issues/2867 { code: ` export namespace Foo { const foo: 1234; - export const bar: string; - export namespace NS { - const baz: 1234; - } + export {}; } `, errors: [ { - column: 9, data: { action: 'defined', additional: '', @@ -1196,23 +1191,20 @@ export namespace Foo { { code: ` export namespace Foo { - export import Bar = Something.Bar; const foo: 1234; - export const bar: string; - export namespace NS { - const baz: 1234; - } + const bar: 4567; + + export { bar }; } `, errors: [ { - column: 9, data: { action: 'defined', additional: '', varName: 'foo', }, - line: 4, + line: 3, messageId: 'unusedVar', }, ], @@ -1220,24 +1212,186 @@ export namespace Foo { }, { code: ` -declare module 'foo' { - export import Bar = Something.Bar; +export namespace Foo { const foo: 1234; - export const bar: string; - export namespace NS { - const baz: 1234; - } + const bar: 4567; + export const bazz: 4567; + + export { bar }; } `, errors: [ { - column: 9, data: { action: 'defined', additional: '', varName: 'foo', }, - line: 4, + line: 3, + messageId: 'unusedVar', + }, + ], + filename: 'foo.d.ts', + }, + { + code: ` +const foo: 1234; +export {}; + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'foo', + }, + line: 2, + messageId: 'unusedVar', + }, + ], + filename: 'foo.d.ts', + }, + { + code: ` +const foo: 1234; +const bar: 4567; + +export { bar }; + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'foo', + }, + line: 2, + messageId: 'unusedVar', + }, + ], + filename: 'foo.d.ts', + }, + { + code: ` +const foo: 1234; +const bar: 4567; +export const bazz: 4567; + +export { bar }; + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'foo', + }, + line: 2, + messageId: 'unusedVar', + }, + ], + filename: 'foo.d.ts', + }, + { + code: ` +const foo: string; +const bar: number; + +export default bar; + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'foo', + }, + line: 2, + messageId: 'unusedVar', + }, + ], + filename: 'foo.d.ts', + }, + { + code: ` +const foo: string; +const bar: number; +export const bazz: number; + +export default bar; + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'foo', + }, + line: 2, + messageId: 'unusedVar', + }, + ], + filename: 'foo.d.ts', + }, + { + code: ` +const foo: string; +export const bar: number; + +export * from '...'; + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'foo', + }, + line: 2, + messageId: 'unusedVar', + }, + ], + filename: 'foo.d.ts', + }, + { + code: ` +declare module 'foo' { + type Foo = 1; + type Bar = 1; + + export = Bar; +} + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'Foo', + }, + line: 3, + messageId: 'unusedVar', + }, + ], + filename: 'foo.d.ts', + }, + { + code: ` +namespace Foo { + type Foo = 1; + type Bar = 1; + + export = Bar; +} + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'Foo', + }, + line: 3, messageId: 'unusedVar', }, ], @@ -2412,7 +2566,6 @@ export class Foo { } } `, - // https://github.com/typescript-eslint/typescript-eslint/issues/2867 { code: ` export namespace Foo { @@ -2423,6 +2576,20 @@ export namespace Foo { }, { code: ` +declare module 'foo' { + const foo: 1234; +} + `, + filename: 'foo.d.ts', + }, + { + code: ` +const foo: 1234; + `, + filename: 'foo.d.ts', + }, + { + code: ` export namespace Foo { export import Bar = Something.Bar; const foo: 1234; @@ -2439,5 +2606,92 @@ declare module 'foo' { `, filename: 'foo.d.ts', }, + { + code: ` +export import Bar = Something.Bar; +const foo: 1234; + `, + filename: 'foo.d.ts', + }, + { + code: ` +declare module 'foo' { + export import Bar = Something.Bar; + const foo: 1234; + export const bar: string; + export namespace NS { + const baz: 1234; + } +} + `, + filename: 'foo.d.ts', + }, + { + code: ` +export namespace Foo { + export import Bar = Something.Bar; + const foo: 1234; + export const bar: string; + export namespace NS { + const baz: 1234; + } +} + `, + filename: 'foo.d.ts', + }, + { + code: ` +export import Bar = Something.Bar; +const foo: 1234; +export const bar: string; +export namespace NS { + const baz: 1234; +} + `, + filename: 'foo.d.ts', + }, + { + code: ` +export namespace Foo { + const foo: 1234; + export const bar: string; + export namespace NS { + const baz: 1234; + } +} + `, + filename: 'foo.d.ts', + }, + { + code: ` +export namespace Foo { + type Foo = 1; + type Bar = 1; + + export default function foo(): Bar; +} + `, + filename: 'foo.d.ts', + }, + { + code: ` +declare module 'foo' { + type Foo = 1; + type Bar = 1; + + export default function foo(): Bar; +} + `, + filename: 'foo.d.ts', + }, + { + code: ` +type Foo = 1; +type Bar = 1; + +export default function foo(): Bar; + `, + filename: 'foo.d.ts', + }, ], }); From e307ce31ad166cce1c0a317790ea931d0f462f7b Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Sat, 25 Jan 2025 22:23:03 +0200 Subject: [PATCH 5/7] change implementation to use existing ambient code --- .../eslint-plugin/src/rules/no-unused-vars.ts | 118 ++++-- .../src/util/collectUnusedVariables.ts | 74 +--- .../no-unused-vars/no-unused-vars.test.ts | 366 +++++++++++++++++- 3 files changed, 450 insertions(+), 108 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unused-vars.ts b/packages/eslint-plugin/src/rules/no-unused-vars.ts index 3b9cbda2ba04..fe5e67ba94dc 100644 --- a/packages/eslint-plugin/src/rules/no-unused-vars.ts +++ b/packages/eslint-plugin/src/rules/no-unused-vars.ts @@ -144,7 +144,10 @@ export default createRule({ }, defaultOptions: [{}], create(context, [firstOption]) { - const MODULE_DECL_CACHE = new Map(); + const MODULE_DECL_CACHE = new Map< + TSESTree.Program | TSESTree.TSModuleDeclaration, + boolean + >(); const options = ((): TranslatedOptions => { const options: TranslatedOptions = { @@ -557,40 +560,71 @@ export default createRule({ } return { - // declaration file handling - [ambientDeclarationSelector(AST_NODE_TYPES.Program, true)]( + // top-level declaration file handling + [ambientDeclarationSelector(AST_NODE_TYPES.Program)]( node: DeclarationSelectorNode, ): void { if (!isDefinitionFile(context.filename)) { return; } + + const moduleDecl = nullThrows( + node.parent, + NullThrowsReasons.MissingParent, + ) as TSESTree.Program; + + if (checkForOverridingExportStatements(moduleDecl)) { + return; + } + markDeclarationChildAsUsed(node); }, // children of a namespace that is a child of a declared namespace are auto-exported [ambientDeclarationSelector( 'TSModuleDeclaration[declare = true] > TSModuleBlock TSModuleDeclaration > TSModuleBlock', - false, )](node: DeclarationSelectorNode): void { + const moduleDecl = nullThrows( + node.parent.parent, + NullThrowsReasons.MissingParent, + ) as TSESTree.TSModuleDeclaration; + + if (checkForOverridingExportStatements(moduleDecl)) { + return; + } + markDeclarationChildAsUsed(node); }, // declared namespace handling [ambientDeclarationSelector( 'TSModuleDeclaration[declare = true] > TSModuleBlock', - false, )](node: DeclarationSelectorNode): void { const moduleDecl = nullThrows( node.parent.parent, NullThrowsReasons.MissingParent, ) as TSESTree.TSModuleDeclaration; - // declared ambient modules with an `export =` statement will only export that one thing - // all other statements are not automatically exported in this case - if ( - moduleDecl.id.type === AST_NODE_TYPES.Literal && - checkModuleDeclForExportEquals(moduleDecl) - ) { + if (checkForOverridingExportStatements(moduleDecl)) { + return; + } + + markDeclarationChildAsUsed(node); + }, + + // namespace handling in definition files + [ambientDeclarationSelector('TSModuleDeclaration > TSModuleBlock')]( + node: DeclarationSelectorNode, + ): void { + if (!isDefinitionFile(context.filename)) { + return; + } + const moduleDecl = nullThrows( + node.parent.parent, + NullThrowsReasons.MissingParent, + ) as TSESTree.TSModuleDeclaration; + + if (checkForOverridingExportStatements(moduleDecl)) { return; } @@ -670,21 +704,19 @@ export default createRule({ }, }; - function checkModuleDeclForExportEquals( - node: TSESTree.TSModuleDeclaration, + function checkForOverridingExportStatements( + node: TSESTree.Program | TSESTree.TSModuleDeclaration, ): boolean { const cached = MODULE_DECL_CACHE.get(node); if (cached != null) { return cached; } - if (node.body) { - for (const statement of node.body.body) { - if (statement.type === AST_NODE_TYPES.TSExportAssignment) { - MODULE_DECL_CACHE.set(node, true); - return true; - } - } + const body = getStatementsOfNode(node); + + if (hasOverridingExportStatement(body)) { + MODULE_DECL_CACHE.set(node, true); + return true; } MODULE_DECL_CACHE.set(node, false); @@ -700,10 +732,7 @@ export default createRule({ | TSESTree.TSModuleDeclaration | TSESTree.TSTypeAliasDeclaration | TSESTree.VariableDeclaration; - function ambientDeclarationSelector( - parent: string, - childDeclare: boolean, - ): string { + function ambientDeclarationSelector(parent: string): string { return [ // Types are ambiently exported `${parent} > :matches(${[ @@ -717,7 +746,7 @@ export default createRule({ AST_NODE_TYPES.TSEnumDeclaration, AST_NODE_TYPES.TSModuleDeclaration, AST_NODE_TYPES.VariableDeclaration, - ].join(', ')})${childDeclare ? '[declare = true]' : ''}`, + ].join(', ')})`, ].join(', '); } function markDeclarationChildAsUsed(node: DeclarationSelectorNode): void { @@ -774,6 +803,45 @@ export default createRule({ }, }); +function hasOverridingExportStatement( + body: TSESTree.ProgramStatement[], +): boolean { + for (const statement of body) { + if ( + (statement.type === AST_NODE_TYPES.ExportNamedDeclaration && + statement.declaration == null) || + statement.type === AST_NODE_TYPES.ExportAllDeclaration || + statement.type === AST_NODE_TYPES.TSExportAssignment + ) { + return true; + } + + if ( + statement.type === AST_NODE_TYPES.ExportDefaultDeclaration && + statement.declaration.type === AST_NODE_TYPES.Identifier + ) { + return true; + } + } + + return false; +} + +function getStatementsOfNode( + block: TSESTree.Program | TSESTree.TSModuleDeclaration, +): TSESTree.ProgramStatement[] { + if (block.type === AST_NODE_TYPES.Program) { + return block.body; + } + + const body = nullThrows( + block.body, + 'Module declarations with no body are filtered out by the rule', + ); + + return body.body; +} + /* ###### TODO ###### diff --git a/packages/eslint-plugin/src/util/collectUnusedVariables.ts b/packages/eslint-plugin/src/util/collectUnusedVariables.ts index d03ab74b2e0e..d61b121fa5ba 100644 --- a/packages/eslint-plugin/src/util/collectUnusedVariables.ts +++ b/packages/eslint-plugin/src/util/collectUnusedVariables.ts @@ -17,7 +17,6 @@ import { } from '@typescript-eslint/utils'; import { isTypeImport } from './isTypeImport'; -import { isDefinitionFile } from './misc'; import { referenceContainsTypeQuery } from './referenceContainsTypeQuery'; interface VariableAnalysis { @@ -77,30 +76,26 @@ class UnusedVarsVisitor extends Visitor { protected TSMethodSignature = this.visitFunctionTypeSignature; - readonly #isDefinitionFile: boolean; - readonly #scopeManager: TSESLint.Scope.ScopeManager; - private constructor(scopeManager: ScopeManager, isDefinitionFile: boolean) { + private constructor(scopeManager: ScopeManager) { super({ visitChildrenEvenIfSelectorExists: true, }); this.#scopeManager = scopeManager; - this.#isDefinitionFile = isDefinitionFile; } public static collectUnusedVariables( program: TSESTree.Program, scopeManager: ScopeManager, - isDefinitionFile: boolean, ): VariableAnalysis { const cached = this.RESULTS_CACHE.get(program); if (cached) { return cached; } - const visitor = new this(scopeManager, isDefinitionFile); + const visitor = new this(scopeManager); visitor.visit(program); const unusedVars = visitor.collectUnusedVariables( @@ -177,11 +172,6 @@ class UnusedVarsVisitor extends Visitor { // expression to self-reference if it has a name defined !scope.functionExpressionScope ) { - const implicitlyExported = allVariablesImplicitlyExported( - scope, - this.#isDefinitionFile, - ); - for (const variable of scope.variables) { // cases that we don't want to treat as used or unused if ( @@ -193,7 +183,6 @@ class UnusedVarsVisitor extends Visitor { } if ( - implicitlyExported || // variables marked with markVariableAsUsed() variable.eslintUsed || // basic exported variables @@ -482,64 +471,6 @@ function isExported(variable: ScopeVariable): boolean { }); } -function getStatementsOfNode( - block: TSESTree.Program | TSESTree.TSModuleDeclaration, -): TSESTree.ProgramStatement[] { - if (block.type === AST_NODE_TYPES.Program) { - return block.body; - } - - if (block.body == null) { - return []; - } - - return block.body.body; -} - -function hasOverridingExportStatement( - body: TSESTree.ProgramStatement[], -): boolean { - for (const statement of body) { - if ( - (statement.type === AST_NODE_TYPES.ExportNamedDeclaration && - statement.declaration == null) || - statement.type === AST_NODE_TYPES.ExportAllDeclaration || - statement.type === AST_NODE_TYPES.TSExportAssignment - ) { - return true; - } - - if ( - statement.type === AST_NODE_TYPES.ExportDefaultDeclaration && - statement.declaration.type === AST_NODE_TYPES.Identifier - ) { - return true; - } - } - - return false; -} - -function allVariablesImplicitlyExported( - scope: TSESLint.Scope.Scope, - isDefinitionFile: boolean, -): boolean { - if ( - !isDefinitionFile || - !(scope.type === ScopeType.tsModule || scope.type === ScopeType.module) - ) { - return false; - } - - const body = getStatementsOfNode(scope.block); - - if (hasOverridingExportStatement(body)) { - return false; - } - - return true; -} - const LOGICAL_ASSIGNMENT_OPERATORS = new Set(['??=', '&&=', '||=']); /** @@ -899,6 +830,5 @@ export function collectVariables< context.sourceCode.scopeManager, 'Missing required scope manager', ), - isDefinitionFile(context.filename), ); } diff --git a/packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts b/packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts index 108928882074..5ebc8d9a406b 100644 --- a/packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts @@ -1190,11 +1190,27 @@ export namespace Foo { }, { code: ` -export namespace Foo { +const foo: 1234; +export {}; + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'foo', + }, + line: 2, + messageId: 'unusedVar', + }, + ], + filename: 'foo.d.ts', + }, + { + code: ` +declare module 'foo' { const foo: 1234; - const bar: 4567; - - export { bar }; + export {}; } `, errors: [ @@ -1215,7 +1231,6 @@ export namespace Foo { export namespace Foo { const foo: 1234; const bar: 4567; - export const bazz: 4567; export { bar }; } @@ -1236,7 +1251,9 @@ export namespace Foo { { code: ` const foo: 1234; -export {}; +const bar: 4567; + +export { bar }; `, errors: [ { @@ -1253,10 +1270,12 @@ export {}; }, { code: ` -const foo: 1234; -const bar: 4567; +declare module 'foo' { + const foo: 1234; + const bar: 4567; -export { bar }; + export { bar }; +} `, errors: [ { @@ -1265,7 +1284,30 @@ export { bar }; additional: '', varName: 'foo', }, - line: 2, + line: 3, + messageId: 'unusedVar', + }, + ], + filename: 'foo.d.ts', + }, + { + code: ` +export namespace Foo { + const foo: 1234; + const bar: 4567; + export const bazz: 4567; + + export { bar }; +} + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'foo', + }, + line: 3, messageId: 'unusedVar', }, ], @@ -1294,6 +1336,51 @@ export { bar }; }, { code: ` +declare module 'foo' { + const foo: 1234; + const bar: 4567; + export const bazz: 4567; + + export { bar }; +} + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'foo', + }, + line: 3, + messageId: 'unusedVar', + }, + ], + filename: 'foo.d.ts', + }, + { + code: ` +export namespace Foo { + const foo: string; + const bar: number; + + export default bar; +} + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'foo', + }, + line: 3, + messageId: 'unusedVar', + }, + ], + filename: 'foo.d.ts', + }, + { + code: ` const foo: string; const bar: number; @@ -1314,6 +1401,51 @@ export default bar; }, { code: ` +declare module 'foo' { + const foo: string; + const bar: number; + + export default bar; +} + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'foo', + }, + line: 3, + messageId: 'unusedVar', + }, + ], + filename: 'foo.d.ts', + }, + { + code: ` +export namespace Foo { + const foo: string; + const bar: number; + export const bazz: number; + + export default bar; +} + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'foo', + }, + line: 3, + messageId: 'unusedVar', + }, + ], + filename: 'foo.d.ts', + }, + { + code: ` const foo: string; const bar: number; export const bazz: number; @@ -1335,6 +1467,51 @@ export default bar; }, { code: ` +declare module 'foo' { + const foo: string; + const bar: number; + export const bazz: number; + + export default bar; +} + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'foo', + }, + line: 3, + messageId: 'unusedVar', + }, + ], + filename: 'foo.d.ts', + }, + { + code: ` +export namespace Foo { + const foo: string; + export const bar: number; + + export * from '...'; +} + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'foo', + }, + line: 3, + messageId: 'unusedVar', + }, + ], + filename: 'foo.d.ts', + }, + { + code: ` const foo: string; export const bar: number; @@ -1356,6 +1533,28 @@ export * from '...'; { code: ` declare module 'foo' { + const foo: string; + export const bar: number; + + export * from '...'; +} + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'foo', + }, + line: 3, + messageId: 'unusedVar', + }, + ], + filename: 'foo.d.ts', + }, + { + code: ` +namespace Foo { type Foo = 1; type Bar = 1; @@ -1377,7 +1576,27 @@ declare module 'foo' { }, { code: ` -namespace Foo { +type Foo = 1; +type Bar = 1; + +export = Bar; + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'Foo', + }, + line: 2, + messageId: 'unusedVar', + }, + ], + filename: 'foo.d.ts', + }, + { + code: ` +declare module 'foo' { type Foo = 1; type Bar = 1; @@ -1397,6 +1616,124 @@ namespace Foo { ], filename: 'foo.d.ts', }, + { + code: ` +declare module 'foo' { + type Test = 1; + export {}; +} + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'Test', + }, + line: 3, + messageId: 'unusedVar', + }, + ], + filename: 'foo.d.ts', + }, + { + code: ` +export declare namespace Foo { + namespace Bar { + namespace Baz { + namespace Bam { + const x = 1; + } + + export {}; + } + } +} + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'Bam', + }, + line: 5, + messageId: 'unusedVar', + }, + ], + }, + { + code: ` +declare module 'foo' { + namespace Bar { + namespace Baz { + namespace Bam { + const x = 1; + } + + export {}; + } + } +} + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'Bam', + }, + line: 5, + messageId: 'unusedVar', + }, + ], + }, + { + code: ` +declare enum Foo {} +export {}; + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'Foo', + }, + line: 2, + messageId: 'unusedVar', + }, + ], + }, + { + code: ` +class Foo {} +declare class Bar {} + +export {}; + `, + errors: [ + { + data: { + action: 'defined', + additional: '', + varName: 'Foo', + }, + line: 2, + messageId: 'unusedVar', + }, + { + data: { + action: 'defined', + additional: '', + varName: 'Bar', + }, + line: 3, + messageId: 'unusedVar', + }, + ], + filename: 'foo.d.ts', + }, ], valid: [ @@ -2693,5 +3030,12 @@ export default function foo(): Bar; `, filename: 'foo.d.ts', }, + { + code: ` +class Foo {} +declare class Bar {} + `, + filename: 'foo.d.ts', + }, ], }); From 4eb0d58a9e56df9c496d2867552628b23b2cc708 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Mon, 3 Mar 2025 01:29:05 +0200 Subject: [PATCH 6/7] refactor --- .../eslint-plugin/src/rules/no-unused-vars.ts | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unused-vars.ts b/packages/eslint-plugin/src/rules/no-unused-vars.ts index fe5e67ba94dc..2c63067c4603 100644 --- a/packages/eslint-plugin/src/rules/no-unused-vars.ts +++ b/packages/eslint-plugin/src/rules/no-unused-vars.ts @@ -16,6 +16,7 @@ import { getNameLocationInGlobalDirectiveComment, isDefinitionFile, isFunction, + MakeRequired, nullThrows, NullThrowsReasons, } from '../util'; @@ -58,6 +59,11 @@ type VariableType = | 'parameter' | 'variable'; +type ModuleDeclarationWithBody = MakeRequired< + TSESTree.TSModuleDeclaration, + 'body' +>; + export default createRule({ name: 'no-unused-vars', meta: { @@ -145,7 +151,7 @@ export default createRule({ defaultOptions: [{}], create(context, [firstOption]) { const MODULE_DECL_CACHE = new Map< - TSESTree.Program | TSESTree.TSModuleDeclaration, + TSESTree.Program | ModuleDeclarationWithBody, boolean >(); @@ -587,7 +593,7 @@ export default createRule({ const moduleDecl = nullThrows( node.parent.parent, NullThrowsReasons.MissingParent, - ) as TSESTree.TSModuleDeclaration; + ) as ModuleDeclarationWithBody; if (checkForOverridingExportStatements(moduleDecl)) { return; @@ -603,7 +609,7 @@ export default createRule({ const moduleDecl = nullThrows( node.parent.parent, NullThrowsReasons.MissingParent, - ) as TSESTree.TSModuleDeclaration; + ) as ModuleDeclarationWithBody; if (checkForOverridingExportStatements(moduleDecl)) { return; @@ -622,7 +628,7 @@ export default createRule({ const moduleDecl = nullThrows( node.parent.parent, NullThrowsReasons.MissingParent, - ) as TSESTree.TSModuleDeclaration; + ) as ModuleDeclarationWithBody; if (checkForOverridingExportStatements(moduleDecl)) { return; @@ -705,7 +711,7 @@ export default createRule({ }; function checkForOverridingExportStatements( - node: TSESTree.Program | TSESTree.TSModuleDeclaration, + node: TSESTree.Program | ModuleDeclarationWithBody, ): boolean { const cached = MODULE_DECL_CACHE.get(node); if (cached != null) { @@ -828,18 +834,13 @@ function hasOverridingExportStatement( } function getStatementsOfNode( - block: TSESTree.Program | TSESTree.TSModuleDeclaration, + block: TSESTree.Program | ModuleDeclarationWithBody, ): TSESTree.ProgramStatement[] { if (block.type === AST_NODE_TYPES.Program) { return block.body; } - const body = nullThrows( - block.body, - 'Module declarations with no body are filtered out by the rule', - ); - - return body.body; + return block.body.body; } /* From f84ded52ee4b1edb9d896c354f3329ee873d158f Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Mon, 3 Mar 2025 08:07:53 +0200 Subject: [PATCH 7/7] fix lint --- packages/eslint-plugin/src/rules/no-unused-vars.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unused-vars.ts b/packages/eslint-plugin/src/rules/no-unused-vars.ts index 2c63067c4603..f2bfa26bb1a8 100644 --- a/packages/eslint-plugin/src/rules/no-unused-vars.ts +++ b/packages/eslint-plugin/src/rules/no-unused-vars.ts @@ -10,13 +10,14 @@ import { } from '@typescript-eslint/scope-manager'; import { AST_NODE_TYPES, TSESLint } from '@typescript-eslint/utils'; +import type { MakeRequired } from '../util'; + import { collectVariables, createRule, getNameLocationInGlobalDirectiveComment, isDefinitionFile, isFunction, - MakeRequired, nullThrows, NullThrowsReasons, } from '../util'; @@ -151,7 +152,7 @@ export default createRule({ defaultOptions: [{}], create(context, [firstOption]) { const MODULE_DECL_CACHE = new Map< - TSESTree.Program | ModuleDeclarationWithBody, + ModuleDeclarationWithBody | TSESTree.Program, boolean >(); @@ -711,7 +712,7 @@ export default createRule({ }; function checkForOverridingExportStatements( - node: TSESTree.Program | ModuleDeclarationWithBody, + node: ModuleDeclarationWithBody | TSESTree.Program, ): boolean { const cached = MODULE_DECL_CACHE.get(node); if (cached != null) { @@ -834,7 +835,7 @@ function hasOverridingExportStatement( } function getStatementsOfNode( - block: TSESTree.Program | ModuleDeclarationWithBody, + block: ModuleDeclarationWithBody | TSESTree.Program, ): TSESTree.ProgramStatement[] { if (block.type === AST_NODE_TYPES.Program) { return block.body;