From a1478ae9d94aade1a66ea47601d3b4144b761dab Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Mon, 16 Nov 2020 11:35:37 -0800 Subject: [PATCH] feat(eslint-plugin): [no-unused-vars] fork the base rule I wanted to avoid doing this, but not doing this restricts our logic too much - it causes problems when we want to consider reporting on things that the base rule wouldn't report on. Fixes #2714 Fixes #2648 Closes #2679 --- .../eslint-plugin/src/rules/no-unused-vars.ts | 500 +++++++----- .../src/util/collectUnusedVariables.ts | 729 ++++++++++++++++++ packages/eslint-plugin/src/util/index.ts | 1 + .../tests/rules/no-unused-vars.test.ts | 64 +- .../eslint-plugin/typings/eslint-rules.d.ts | 14 + .../src/ast-utils/predicates.ts | 22 + .../experimental-utils/src/ts-eslint/Scope.ts | 75 +- .../src/referencer/VisitorBase.ts | 9 +- 8 files changed, 1194 insertions(+), 220 deletions(-) create mode 100644 packages/eslint-plugin/src/util/collectUnusedVariables.ts diff --git a/packages/eslint-plugin/src/rules/no-unused-vars.ts b/packages/eslint-plugin/src/rules/no-unused-vars.ts index 8753452dd09..33153561a36 100644 --- a/packages/eslint-plugin/src/rules/no-unused-vars.ts +++ b/packages/eslint-plugin/src/rules/no-unused-vars.ts @@ -4,11 +4,33 @@ import { TSESTree, } from '@typescript-eslint/experimental-utils'; import { PatternVisitor } from '@typescript-eslint/scope-manager'; -import baseRule from 'eslint/lib/rules/no-unused-vars'; +import { getNameLocationInGlobalDirectiveComment } from 'eslint/lib/rules/utils/ast-utils'; import * as util from '../util'; -type MessageIds = util.InferMessageIdsTypeFromRule; -type Options = util.InferOptionsTypeFromRule; +type MessageIds = 'unusedVar'; +type Options = [ + | 'all' + | 'local' + | { + vars?: 'all' | 'local'; + varsIgnorePattern?: string; + args?: 'all' | 'after-used' | 'none'; + ignoreRestSiblings?: boolean; + argsIgnorePattern?: string; + caughtErrors?: 'all' | 'none'; + caughtErrorsIgnorePattern?: string; + }, +]; + +interface TranslatedOptions { + vars: 'all' | 'local'; + varsIgnorePattern?: RegExp; + args: 'all' | 'after-used' | 'none'; + ignoreRestSiblings: boolean; + argsIgnorePattern?: RegExp; + caughtErrors: 'all' | 'none'; + caughtErrorsIgnorePattern?: RegExp; +} export default util.createRule({ name: 'no-unused-vars', @@ -20,195 +42,200 @@ export default util.createRule({ recommended: 'warn', extendsBaseRule: true, }, - schema: baseRule.meta.schema, - messages: baseRule.meta.messages ?? { + schema: [ + { + oneOf: [ + { + enum: ['all', 'local'], + }, + { + type: 'object', + properties: { + vars: { + enum: ['all', 'local'], + }, + varsIgnorePattern: { + type: 'string', + }, + args: { + enum: ['all', 'after-used', 'none'], + }, + ignoreRestSiblings: { + type: 'boolean', + }, + argsIgnorePattern: { + type: 'string', + }, + caughtErrors: { + enum: ['all', 'none'], + }, + caughtErrorsIgnorePattern: { + type: 'string', + }, + }, + additionalProperties: false, + }, + ], + }, + ], + messages: { unusedVar: "'{{varName}}' is {{action}} but never used{{additional}}.", }, }, defaultOptions: [{}], create(context) { - const rules = baseRule.create(context); const filename = context.getFilename(); + const sourceCode = context.getSourceCode(); const MODULE_DECL_CACHE = new Map(); - /** - * Gets a list of TS module definitions for a specified variable. - * @param variable eslint-scope variable object. - */ - function getModuleNameDeclarations( - variable: TSESLint.Scope.Variable, - ): TSESTree.TSModuleDeclaration[] { - const moduleDeclarations: TSESTree.TSModuleDeclaration[] = []; - - variable.defs.forEach(def => { - if (def.type === 'TSModuleName') { - moduleDeclarations.push(def.node); - } - }); - - return moduleDeclarations; - } + const options = ((): TranslatedOptions => { + const options: TranslatedOptions = { + vars: 'all', + args: 'after-used', + ignoreRestSiblings: false, + caughtErrors: 'none', + }; + + const firstOption = context.options[0]; + + if (firstOption) { + if (typeof firstOption === 'string') { + options.vars = firstOption; + } else { + options.vars = firstOption.vars ?? options.vars; + options.args = firstOption.args ?? options.args; + options.ignoreRestSiblings = + firstOption.ignoreRestSiblings ?? options.ignoreRestSiblings; + options.caughtErrors = + firstOption.caughtErrors ?? options.caughtErrors; + + if (firstOption.varsIgnorePattern) { + options.varsIgnorePattern = new RegExp( + firstOption.varsIgnorePattern, + 'u', + ); + } - /** - * Determine if an identifier is referencing an enclosing name. - * This only applies to declarations that create their own scope (modules, functions, classes) - * @param ref The reference to check. - * @param nodes The candidate function nodes. - * @returns True if it's a self-reference, false if not. - */ - function isBlockSelfReference( - ref: TSESLint.Scope.Reference, - nodes: TSESTree.Node[], - ): boolean { - let scope: TSESLint.Scope.Scope | null = ref.from; + if (firstOption.argsIgnorePattern) { + options.argsIgnorePattern = new RegExp( + firstOption.argsIgnorePattern, + 'u', + ); + } - while (scope) { - if (nodes.indexOf(scope.block) >= 0) { - return true; + if (firstOption.caughtErrorsIgnorePattern) { + options.caughtErrorsIgnorePattern = new RegExp( + firstOption.caughtErrorsIgnorePattern, + 'u', + ); + } } - - scope = scope.upper; } - - return false; - } - - function isExported( - variable: TSESLint.Scope.Variable, - target: AST_NODE_TYPES, - ): boolean { - // TS will require that all merged namespaces/interfaces are exported, so we only need to find one - return variable.defs.some( - def => - def.node.type === target && - (def.node.parent?.type === AST_NODE_TYPES.ExportNamedDeclaration || - def.node.parent?.type === AST_NODE_TYPES.ExportDefaultDeclaration), - ); - } - - return { - ...rules, - 'TSCallSignatureDeclaration, TSConstructorType, TSConstructSignatureDeclaration, TSDeclareFunction, TSEmptyBodyFunctionExpression, TSFunctionType, TSMethodSignature'( - node: - | TSESTree.TSCallSignatureDeclaration - | TSESTree.TSConstructorType - | TSESTree.TSConstructSignatureDeclaration - | TSESTree.TSDeclareFunction - | TSESTree.TSEmptyBodyFunctionExpression - | TSESTree.TSFunctionType - | TSESTree.TSMethodSignature, - ): void { - // function type signature params create variables because they can be referenced within the signature, - // but they obviously aren't unused variables for the purposes of this rule. - for (const param of node.params) { - visitPattern(param, name => { - context.markVariableAsUsed(name.name); + return options; + })(); + + function collectUnusedVariables(): TSESLint.Scope.Variable[] { + /** + * Determines if a variable has a sibling rest property + * @param variable eslint-scope variable object. + * @returns True if the variable is exported, false if not. + */ + function hasRestSpreadSibling( + variable: TSESLint.Scope.Variable, + ): boolean { + if (options.ignoreRestSiblings) { + return variable.defs.some(def => { + const propertyNode = def.name.parent!; + const patternNode = propertyNode.parent!; + + return ( + propertyNode.type === AST_NODE_TYPES.Property && + patternNode.type === AST_NODE_TYPES.ObjectPattern && + patternNode.properties[patternNode.properties.length - 1].type === + AST_NODE_TYPES.RestElement + ); }); } - }, - TSEnumDeclaration(): void { - // enum members create variables because they can be referenced within the enum, - // but they obviously aren't unused variables for the purposes of this rule. - const scope = context.getScope(); - for (const variable of scope.variables) { - context.markVariableAsUsed(variable.name); - } - }, - TSMappedType(node): void { - // mapped types create a variable for their type name, but it's not necessary to reference it, - // so we shouldn't consider it as unused for the purpose of this rule. - context.markVariableAsUsed(node.typeParameter.name.name); - }, - TSModuleDeclaration(): void { - const childScope = context.getScope(); - const scope = util.nullThrows( - context.getScope().upper, - util.NullThrowsReasons.MissingToken(childScope.type, 'upper scope'), + + return false; + } + + /** + * Checks whether the given variable is after the last used parameter. + * @param variable The variable to check. + * @returns `true` if the variable is defined after the last used parameter. + */ + function isAfterLastUsedArg(variable: TSESLint.Scope.Variable): boolean { + const def = variable.defs[0]; + const params = context.getDeclaredVariables(def.node); + const posteriorParams = params.slice(params.indexOf(variable) + 1); + + // If any used parameters occur after this parameter, do not report. + return !posteriorParams.some( + v => v.references.length > 0 || v.eslintUsed, ); - for (const variable of scope.variables) { - const moduleNodes = getModuleNameDeclarations(variable); + } - if ( - moduleNodes.length === 0 || - // ignore unreferenced module definitions, as the base rule will report on them - variable.references.length === 0 || - // ignore exported nodes - isExported(variable, AST_NODE_TYPES.TSModuleDeclaration) - ) { - continue; + const unusedVariablesOriginal = util.collectUnusedVariables(context); + const unusedVariablesReturn: TSESLint.Scope.Variable[] = []; + for (const variable of unusedVariablesOriginal) { + // explicit global variables don't have definitions. + const def = variable.defs[0]; + if (def) { + // skip catch variables + if (def.type === TSESLint.Scope.DefinitionType.CatchClause) { + if (options.caughtErrors === 'none') { + continue; + } + // skip ignored parameters + if ( + 'name' in def.name && + options.caughtErrorsIgnorePattern?.test(def.name.name) + ) { + continue; + } } - // check if the only reference to a module's name is a self-reference in its body - // this won't be caught by the base rule because it doesn't understand TS modules - const isOnlySelfReferenced = variable.references.every(ref => { - return isBlockSelfReference(ref, moduleNodes); - }); - - if (isOnlySelfReferenced) { - context.report({ - node: variable.identifiers[0], - messageId: 'unusedVar', - data: { - varName: variable.name, - action: 'defined', - additional: '', - }, - }); + if (def.type === TSESLint.Scope.DefinitionType.Parameter) { + // if "args" option is "none", skip any parameter + if (options.args === 'none') { + continue; + } + // skip ignored parameters + if ( + 'name' in def.name && + options.argsIgnorePattern?.test(def.name.name) + ) { + continue; + } + // if "args" option is "after-used", skip used variables + if ( + options.args === 'after-used' && + util.isFunction(def.name.parent) && + !isAfterLastUsedArg(variable) + ) { + continue; + } + } else { + // skip ignored variables + if ( + 'name' in def.name && + options.varsIgnorePattern?.test(def.name.name) + ) { + continue; + } } } - }, - [[ - 'TSParameterProperty > AssignmentPattern > Identifier.left', - 'TSParameterProperty > Identifier.parameter', - ].join(', ')](node: TSESTree.Identifier): void { - // just assume parameter properties are used as property usage tracking is beyond the scope of this rule - context.markVariableAsUsed(node.name); - }, - ':matches(FunctionDeclaration, FunctionExpression, ArrowFunctionExpression) > Identifier[name="this"].params'( - node: TSESTree.Identifier, - ): void { - // this parameters should always be considered used as they're pseudo-parameters - context.markVariableAsUsed(node.name); - }, - 'TSInterfaceDeclaration, TSTypeAliasDeclaration'( - node: TSESTree.TSInterfaceDeclaration | TSESTree.TSTypeAliasDeclaration, - ): void { - const variable = context.getScope().set.get(node.id.name); - if (!variable) { - return; - } - if ( - variable.references.length === 0 || - // ignore exported nodes - isExported(variable, node.type) - ) { - return; - } - // check if the type is only self-referenced - // this won't be caught by the base rule because it doesn't understand self-referencing types - const isOnlySelfReferenced = variable.references.every(ref => { - if ( - ref.identifier.range[0] >= node.range[0] && - ref.identifier.range[1] <= node.range[1] - ) { - return true; - } - return false; - }); - if (isOnlySelfReferenced) { - context.report({ - node: variable.identifiers[0], - messageId: 'unusedVar', - data: { - varName: variable.name, - action: 'defined', - additional: '', - }, - }); + if (!hasRestSpreadSibling(variable)) { + unusedVariablesReturn.push(variable); } - }, + } + return unusedVariablesReturn; + } + + return { // declaration file handling [ambientDeclarationSelector(AST_NODE_TYPES.Program, true)]( node: DeclarationSelectorNode, @@ -219,11 +246,6 @@ export default util.createRule({ markDeclarationChildAsUsed(node); }, - // global augmentation can be in any file, and they do not need exports - 'TSModuleDeclaration[declare = true][global = true]'(): void { - context.markVariableAsUsed('global'); - }, - // children of a namespace that is a child of a declared namespace are auto-exported [ambientDeclarationSelector( 'TSModuleDeclaration[declare = true] > TSModuleBlock TSModuleDeclaration > TSModuleBlock', @@ -253,6 +275,111 @@ export default util.createRule({ markDeclarationChildAsUsed(node); }, + + // collect + 'Program:exit'(programNode): void { + /** + * Generates the message data about the variable being defined and unused, + * including the ignore pattern if configured. + * @param unusedVar eslint-scope variable object. + * @returns The message data to be used with this unused variable. + */ + function getDefinedMessageData( + unusedVar: TSESLint.Scope.Variable, + ): Record { + const defType = unusedVar?.defs[0]?.type; + let type; + let pattern; + + if ( + defType === TSESLint.Scope.DefinitionType.CatchClause && + options.caughtErrorsIgnorePattern + ) { + type = 'args'; + pattern = options.caughtErrorsIgnorePattern.toString(); + } else if ( + defType === TSESLint.Scope.DefinitionType.Parameter && + options.argsIgnorePattern + ) { + type = 'args'; + pattern = options.argsIgnorePattern.toString(); + } else if ( + defType !== TSESLint.Scope.DefinitionType.Parameter && + options.varsIgnorePattern + ) { + type = 'vars'; + pattern = options.varsIgnorePattern.toString(); + } + + const additional = type + ? `. Allowed unused ${type} must match ${pattern}` + : ''; + + return { + varName: unusedVar.name, + action: 'defined', + additional, + }; + } + + /** + * Generate the warning message about the variable being + * assigned and unused, including the ignore pattern if configured. + * @param unusedVar eslint-scope variable object. + * @returns The message data to be used with this unused variable. + */ + function getAssignedMessageData( + unusedVar: TSESLint.Scope.Variable, + ): Record { + const additional = options.varsIgnorePattern + ? `. Allowed unused vars must match ${options.varsIgnorePattern.toString()}` + : ''; + + return { + varName: unusedVar.name, + action: 'assigned a value', + additional, + }; + } + + const unusedVars = collectUnusedVariables(); + + for (let i = 0, l = unusedVars.length; i < l; ++i) { + const unusedVar = unusedVars[i]; + + // Report the first declaration. + if (unusedVar.defs.length > 0) { + context.report({ + node: unusedVar.references.length + ? unusedVar.references[unusedVar.references.length - 1] + .identifier + : unusedVar.identifiers[0], + messageId: 'unusedVar', + data: unusedVar.references.some(ref => ref.isWrite()) + ? getAssignedMessageData(unusedVar) + : getDefinedMessageData(unusedVar), + }); + + // If there are no regular declaration, report the first `/*globals*/` comment directive. + } else if ( + 'eslintExplicitGlobalComments' in unusedVar && + unusedVar.eslintExplicitGlobalComments + ) { + const directiveComment = unusedVar.eslintExplicitGlobalComments[0]; + + context.report({ + node: programNode, + loc: getNameLocationInGlobalDirectiveComment( + sourceCode, + directiveComment, + unusedVar.name, + ), + messageId: 'unusedVar', + data: getDefinedMessageData(unusedVar), + }); + } + } + }, }; function checkModuleDeclForExportEquals( @@ -391,6 +518,31 @@ function bar( // bar should be unused _arg: typeof bar ) {} + +--- if an interface is merged into a namespace --- +--- NOTE - TS gets these cases wrong + +namespace Test { + interface Foo { // Foo should be unused here + a: string; + } + export namespace Foo { + export type T = 'b'; + } +} +type T = Test.Foo; // Error: Namespace 'Test' has no exported member 'Foo'. + + +namespace Test { + export interface Foo { + a: string; + } + namespace Foo { // Foo should be unused here + export type T = 'b'; + } +} +type T = Test.Foo.T; // Error: Namespace 'Test' has no exported member 'Foo'. + */ /* diff --git a/packages/eslint-plugin/src/util/collectUnusedVariables.ts b/packages/eslint-plugin/src/util/collectUnusedVariables.ts new file mode 100644 index 00000000000..0f8d0efc741 --- /dev/null +++ b/packages/eslint-plugin/src/util/collectUnusedVariables.ts @@ -0,0 +1,729 @@ +import { + AST_NODE_TYPES, + TSESLint, + TSESTree, +} from '@typescript-eslint/experimental-utils'; +import { ImplicitLibVariable } from '@typescript-eslint/scope-manager'; +import { Visitor } from '@typescript-eslint/scope-manager/src/referencer/Visitor'; +import * as util from '.'; + +class UnusedVarsVisitor< + TMessageIds extends string, + TOptions extends readonly unknown[] +> extends Visitor { + private static readonly RESULTS_CACHE = new WeakMap< + TSESTree.Program, + ReadonlySet + >(); + + readonly #scopeManager: TSESLint.Scope.ScopeManager; + // readonly #unusedVariables = new Set(); + + private constructor(context: TSESLint.RuleContext) { + super({ + visitChildrenEvenIfSelectorExists: true, + }); + + this.#scopeManager = util.nullThrows( + context.getSourceCode().scopeManager, + 'Missing required scope manager', + ); + } + + public static collectUnusedVariables< + TMessageIds extends string, + TOptions extends readonly unknown[] + >( + context: TSESLint.RuleContext, + ): ReadonlySet { + const program = context.getSourceCode().ast; + const cached = this.RESULTS_CACHE.get(program); + if (cached) { + return cached; + } + + const visitor = new this(context); + visitor.visit(program); + + const unusedVars = visitor.collectUnusedVariables( + visitor.getScope(program), + ); + this.RESULTS_CACHE.set(program, unusedVars); + return unusedVars; + } + + private collectUnusedVariables( + scope: TSESLint.Scope.Scope, + unusedVariables = new Set(), + ): ReadonlySet { + for (const variable of scope.variables) { + if ( + // skip function expression names, + scope.functionExpressionScope || + // variables marked with markVariableAsUsed(), + variable.eslintUsed || + // implicit lib variables (from @typescript-eslint/scope-manager), + variable instanceof ImplicitLibVariable || + // basic exported variables + isExported(variable) || + // variables implicitly exported via a merged declaration + isMergableExported(variable) || + // used variables + isUsedVariable(variable) + ) { + continue; + } + + unusedVariables.add(variable); + } + + for (const childScope of scope.childScopes) { + this.collectUnusedVariables(childScope, unusedVariables); + } + + return unusedVariables; + } + + //#region HELPERS + + private getScope( + currentNode: TSESTree.Node, + ): T { + // On Program node, get the outermost scope to avoid return Node.js special function scope or ES modules scope. + const inner = currentNode.type !== AST_NODE_TYPES.Program; + + for ( + let node: TSESTree.Node | undefined = currentNode; + node; + node = node.parent + ) { + const scope = this.#scopeManager.acquire(node, inner); + + if (scope) { + if (scope.type === 'function-expression-name') { + return scope.childScopes[0] as T; + } + return scope as T; + } + } + + return this.#scopeManager.scopes[0] as T; + } + + private markVariableAsUsed(variable: TSESLint.Scope.Variable): void; + private markVariableAsUsed(identifier: TSESTree.Identifier): void; + private markVariableAsUsed(name: string, parent: TSESTree.Node): void; + private markVariableAsUsed( + variableOrIdentifierOrName: + | TSESLint.Scope.Variable + | TSESTree.Identifier + | string, + parent?: TSESTree.Node, + ): void { + if ( + typeof variableOrIdentifierOrName !== 'string' && + !('type' in variableOrIdentifierOrName) + ) { + variableOrIdentifierOrName.eslintUsed = true; + return; + } + + let name: string; + let node: TSESTree.Node; + if (typeof variableOrIdentifierOrName === 'string') { + name = variableOrIdentifierOrName; + node = parent!; + } else { + name = variableOrIdentifierOrName.name; + node = variableOrIdentifierOrName; + } + + let currentScope: TSESLint.Scope.Scope | null = this.getScope(node); + while (currentScope) { + const variable = currentScope.variables.find( + scopeVar => scopeVar.name === name, + ); + + if (variable) { + variable.eslintUsed = true; + return; + } + + currentScope = currentScope.upper; + } + } + + private visitFunctionTypeSignature( + node: + | TSESTree.TSCallSignatureDeclaration + | TSESTree.TSConstructorType + | TSESTree.TSConstructSignatureDeclaration + | TSESTree.TSDeclareFunction + | TSESTree.TSEmptyBodyFunctionExpression + | TSESTree.TSFunctionType + | TSESTree.TSMethodSignature, + ): void { + // function type signature params create variables because they can be referenced within the signature, + // but they obviously aren't unused variables for the purposes of this rule. + for (const param of node.params) { + this.visitPattern(param, name => { + this.markVariableAsUsed(name); + }); + } + } + + //#endregion HELPERS + + //#region VISITORS + // NOTE - This is a simple visitor - meaning it does not support selectors + + protected ClassDeclaration(node: TSESTree.ClassDeclaration): void { + // skip a variable of class itself name in the class scope + const scope = this.getScope(node); + for (const variable of scope.variables) { + if (variable.identifiers[0] === scope.block.id) { + this.markVariableAsUsed(variable); + return; + } + } + } + + protected Identifier(node: TSESTree.Identifier): void { + const scope = this.getScope(node); + if (scope.type === TSESLint.Scope.ScopeType.function) { + switch (node.name) { + case 'this': { + // this parameters should always be considered used as they're pseudo-parameters + if ('params' in scope.block && scope.block.params.includes(node)) { + this.markVariableAsUsed(node); + } + + break; + } + + case 'arguments': { + // skip implicit "arguments" variable + this.markVariableAsUsed(node); + break; + } + } + } + } + + protected MethodDefinition(node: TSESTree.MethodDefinition): void { + if (node.kind === 'set') { + // ignore setter parameters because they're syntactically required to exist + for (const param of node.value.params) { + this.visitPattern(param, id => { + this.markVariableAsUsed(id); + }); + } + } + } + + protected TSCallSignatureDeclaration = this.visitFunctionTypeSignature; + + protected TSConstructorType = this.visitFunctionTypeSignature; + + protected TSConstructSignatureDeclaration = this.visitFunctionTypeSignature; + + protected TSDeclareFunction = this.visitFunctionTypeSignature; + + protected TSEmptyBodyFunctionExpression = this.visitFunctionTypeSignature; + + protected TSEnumDeclaration(node: TSESTree.TSEnumDeclaration): void { + // enum members create variables because they can be referenced within the enum, + // but they obviously aren't unused variables for the purposes of this rule. + const scope = this.getScope(node); + for (const variable of scope.variables) { + this.markVariableAsUsed(variable); + } + } + + protected TSFunctionType = this.visitFunctionTypeSignature; + + protected TSMappedType(node: TSESTree.TSMappedType): void { + // mapped types create a variable for their type name, but it's not necessary to reference it, + // so we shouldn't consider it as unused for the purpose of this rule. + this.markVariableAsUsed(node.typeParameter.name); + } + + protected TSMethodSignature = this.visitFunctionTypeSignature; + + protected TSModuleDeclaration(node: TSESTree.TSModuleDeclaration): void { + // global augmentation can be in any file, and they do not need exports + if (node.global === true) { + this.markVariableAsUsed('global', node.parent!); + } + } + + protected TSParameterProperty(node: TSESTree.TSParameterProperty): void { + let identifier: TSESTree.Identifier | null = null; + switch (node.parameter.type) { + case AST_NODE_TYPES.AssignmentPattern: + if (node.parameter.left.type === AST_NODE_TYPES.Identifier) { + identifier = node.parameter.left; + } + break; + + case AST_NODE_TYPES.Identifier: + identifier = node.parameter; + break; + } + + if (identifier) { + this.markVariableAsUsed(identifier); + } + } + + //#endregion VISITORS +} + +//#region private helpers + +/** + * Checks the position of given nodes. + * @param inner A node which is expected as inside. + * @param outer A node which is expected as outside. + * @returns `true` if the `inner` node exists in the `outer` node. + */ +function isInside(inner: TSESTree.Node, outer: TSESTree.Node): boolean { + return inner.range[0] >= outer.range[0] && inner.range[1] <= outer.range[1]; +} + +/** + * Determine if an identifier is referencing an enclosing name. + * This only applies to declarations that create their own scope (modules, functions, classes) + * @param ref The reference to check. + * @param nodes The candidate function nodes. + * @returns True if it's a self-reference, false if not. + */ +function isSelfReference( + ref: TSESLint.Scope.Reference, + nodes: Set, +): boolean { + let scope: TSESLint.Scope.Scope | null = ref.from; + + while (scope) { + if (nodes.has(scope.block)) { + return true; + } + + scope = scope.upper; + } + + return false; +} + +const MERGABLE_TYPES = new Set([ + AST_NODE_TYPES.TSInterfaceDeclaration, + AST_NODE_TYPES.TSTypeAliasDeclaration, + AST_NODE_TYPES.TSModuleDeclaration, + AST_NODE_TYPES.ClassDeclaration, + AST_NODE_TYPES.FunctionDeclaration, +]); +/** + * Determine if the variable is directly exported + * @param variable the variable to check + * @param target the type of node that is expected to be exported + */ +function isMergableExported(variable: TSESLint.Scope.Variable): boolean { + // If all of the merged things are of the same type, TS will error if not all of them are exported - so we only need to find one + for (const def of variable.defs) { + if ( + (MERGABLE_TYPES.has(def.node.type) && + def.node.parent?.type === AST_NODE_TYPES.ExportNamedDeclaration) || + def.node.parent?.type === AST_NODE_TYPES.ExportDefaultDeclaration + ) { + return true; + } + } + + return false; +} + +/** + * Determines if a given variable is being exported from a module. + * @param variable eslint-scope variable object. + * @returns True if the variable is exported, false if not. + */ +function isExported(variable: TSESLint.Scope.Variable): boolean { + const definition = variable.defs[0]; + + if (definition) { + let node = definition.node; + + if (node.type === AST_NODE_TYPES.VariableDeclarator) { + node = node.parent!; + } else if (definition.type === 'Parameter') { + return false; + } + + return node.parent!.type.indexOf('Export') === 0; + } + return false; +} + +/** + * Determines if the variable is used. + * @param variable The variable to check. + * @returns True if the variable is used + */ +function isUsedVariable(variable: TSESLint.Scope.Variable): boolean { + /** + * Gets a list of function definitions for a specified variable. + * @param variable eslint-scope variable object. + * @returns Function nodes. + */ + function getFunctionDefinitions( + variable: TSESLint.Scope.Variable, + ): Set { + const functionDefinitions = new Set(); + + variable.defs.forEach(def => { + // FunctionDeclarations + if (def.type === TSESLint.Scope.DefinitionType.FunctionName) { + functionDefinitions.add(def.node); + } + + // FunctionExpressions + if ( + def.type === TSESLint.Scope.DefinitionType.Variable && + (def.node.init?.type === AST_NODE_TYPES.FunctionExpression || + def.node.init?.type === AST_NODE_TYPES.ArrowFunctionExpression) + ) { + functionDefinitions.add(def.node.init); + } + }); + return functionDefinitions; + } + + function getTypeDeclarations( + variable: TSESLint.Scope.Variable, + ): Set { + const nodes = new Set(); + + variable.defs.forEach(def => { + if ( + def.node.type === AST_NODE_TYPES.TSInterfaceDeclaration || + def.node.type === AST_NODE_TYPES.TSTypeAliasDeclaration + ) { + nodes.add(def.node); + } + }); + + return nodes; + } + + function getModuleDeclarations( + variable: TSESLint.Scope.Variable, + ): Set { + const nodes = new Set(); + + variable.defs.forEach(def => { + if (def.node.type === AST_NODE_TYPES.TSModuleDeclaration) { + nodes.add(def.node); + } + }); + + return nodes; + } + + /** + * Checks if the ref is contained within one of the given nodes + */ + function isInsideOneOf( + ref: TSESLint.Scope.Reference, + nodes: Set, + ): boolean { + for (const node of nodes) { + if (isInside(ref.identifier, node)) { + return true; + } + } + + return false; + } + + /** + * If a given reference is left-hand side of an assignment, this gets + * the right-hand side node of the assignment. + * + * In the following cases, this returns null. + * + * - The reference is not the LHS of an assignment expression. + * - The reference is inside of a loop. + * - The reference is inside of a function scope which is different from + * the declaration. + * @param ref A reference to check. + * @param prevRhsNode The previous RHS node. + * @returns The RHS node or null. + */ + function getRhsNode( + ref: TSESLint.Scope.Reference, + prevRhsNode: TSESTree.Node | null, + ): TSESTree.Node | null { + /** + * Checks whether the given node is in a loop or not. + * @param node The node to check. + * @returns `true` if the node is in a loop. + */ + function isInLoop(node: TSESTree.Node): boolean { + let currentNode: TSESTree.Node | undefined = node; + while (currentNode) { + if (util.isFunction(currentNode)) { + break; + } + + if (util.isLoop(currentNode)) { + return true; + } + + currentNode = currentNode.parent; + } + + return false; + } + + const id = ref.identifier; + const parent = id.parent!; + const grandparent = parent.parent!; + const refScope = ref.from.variableScope; + const varScope = ref.resolved!.scope.variableScope; + const canBeUsedLater = refScope !== varScope || isInLoop(id); + + /* + * Inherits the previous node if this reference is in the node. + * This is for `a = a + a`-like code. + */ + if (prevRhsNode && isInside(id, prevRhsNode)) { + return prevRhsNode; + } + + if ( + parent.type === AST_NODE_TYPES.AssignmentExpression && + grandparent.type === AST_NODE_TYPES.ExpressionStatement && + id === parent.left && + !canBeUsedLater + ) { + return parent.right; + } + return null; + } + + /** + * Checks whether a given reference is a read to update itself or not. + * @param ref A reference to check. + * @param rhsNode The RHS node of the previous assignment. + * @returns The reference is a read to update itself. + */ + function isReadForItself( + ref: TSESLint.Scope.Reference, + rhsNode: TSESTree.Node | null, + ): boolean { + /** + * Checks whether a given Identifier node exists inside of a function node which can be used later. + * + * "can be used later" means: + * - the function is assigned to a variable. + * - the function is bound to a property and the object can be used later. + * - the function is bound as an argument of a function call. + * + * If a reference exists in a function which can be used later, the reference is read when the function is called. + * @param id An Identifier node to check. + * @param rhsNode The RHS node of the previous assignment. + * @returns `true` if the `id` node exists inside of a function node which can be used later. + */ + function isInsideOfStorableFunction( + id: TSESTree.Node, + rhsNode: TSESTree.Node, + ): boolean { + /** + * Finds a function node from ancestors of a node. + * @param node A start node to find. + * @returns A found function node. + */ + function getUpperFunction(node: TSESTree.Node): TSESTree.Node | null { + let currentNode: TSESTree.Node | undefined = node; + while (currentNode) { + if (util.isFunction(currentNode)) { + return currentNode; + } + currentNode = currentNode.parent; + } + + return null; + } + + /** + * Checks whether a given function node is stored to somewhere or not. + * If the function node is stored, the function can be used later. + * @param funcNode A function node to check. + * @param rhsNode The RHS node of the previous assignment. + * @returns `true` if under the following conditions: + * - the funcNode is assigned to a variable. + * - the funcNode is bound as an argument of a function call. + * - the function is bound to a property and the object satisfies above conditions. + */ + function isStorableFunction( + funcNode: TSESTree.Node, + rhsNode: TSESTree.Node, + ): boolean { + let node = funcNode; + let parent = funcNode.parent; + + while (parent && isInside(parent, rhsNode)) { + switch (parent.type) { + case AST_NODE_TYPES.SequenceExpression: + if (parent.expressions[parent.expressions.length - 1] !== node) { + return false; + } + break; + + case AST_NODE_TYPES.CallExpression: + case AST_NODE_TYPES.NewExpression: + return parent.callee !== node; + + case AST_NODE_TYPES.AssignmentExpression: + case AST_NODE_TYPES.TaggedTemplateExpression: + case AST_NODE_TYPES.YieldExpression: + return true; + + default: + if ( + parent.type.endsWith('Statement') || + parent.type.endsWith('Declaration') + ) { + /* + * If it encountered statements, this is a complex pattern. + * Since analyzing complex patterns is hard, this returns `true` to avoid false positive. + */ + return true; + } + } + + node = parent; + parent = parent.parent; + } + + return false; + } + + const funcNode = getUpperFunction(id); + + return ( + !!funcNode && + isInside(funcNode, rhsNode) && + isStorableFunction(funcNode, rhsNode) + ); + } + + const id = ref.identifier; + const parent = id.parent!; + const grandparent = parent.parent!; + + return ( + ref.isRead() && // in RHS of an assignment for itself. e.g. `a = a + 1` + // self update. e.g. `a += 1`, `a++` + ((parent.type === AST_NODE_TYPES.AssignmentExpression && + grandparent.type === AST_NODE_TYPES.ExpressionStatement && + parent.left === id) || + (parent.type === AST_NODE_TYPES.UpdateExpression && + grandparent.type === AST_NODE_TYPES.ExpressionStatement) || + (!!rhsNode && + isInside(id, rhsNode) && + !isInsideOfStorableFunction(id, rhsNode))) + ); + } + + /** + * (bradzacher): I hate that this has to exist. + * But it is required for compat with the base ESLint rule. + * + * In 2015, ESLint decided to add an exception for this specific code + * ``` + * for (var key in object) return; + * ``` + * + * I disagree with it, but what are you going to do. + * + * https://github.com/eslint/eslint/issues/2342 + */ + function isForInRef(ref: TSESLint.Scope.Reference): boolean { + let target = ref.identifier.parent!; + + // "for (var ...) { return; }" + if (target.type === AST_NODE_TYPES.VariableDeclarator) { + target = target.parent!.parent!; + } + + if (target.type !== AST_NODE_TYPES.ForInStatement) { + return false; + } + + // "for (...) { return; }" + if (target.body.type === AST_NODE_TYPES.BlockStatement) { + target = target.body.body[0]; + + // "for (...) return;" + } else { + target = target.body; + } + + // For empty loop body + if (!target) { + return false; + } + + return target.type === AST_NODE_TYPES.ReturnStatement; + } + + const functionNodes = getFunctionDefinitions(variable); + const isFunctionDefinition = functionNodes.size > 0; + + const typeDeclNodes = getTypeDeclarations(variable); + const isTypeDecl = typeDeclNodes.size > 0; + + const moduleDeclNodes = getModuleDeclarations(variable); + const isModuleDecl = moduleDeclNodes.size > 0; + + let rhsNode: TSESTree.Node | null = null; + + return variable.references.some(ref => { + if (isForInRef(ref)) { + return true; + } + + const forItself = isReadForItself(ref, rhsNode); + + rhsNode = getRhsNode(ref, rhsNode); + + return ( + ref.isRead() && + !forItself && + !(isFunctionDefinition && isSelfReference(ref, functionNodes)) && + !(isTypeDecl && isInsideOneOf(ref, typeDeclNodes)) && + !(isModuleDecl && isSelfReference(ref, moduleDeclNodes)) + ); + }); +} + +//#endregion private helpers + +/** + * Collects the set of unused variables for a given context. + * + * Due to complexity, this does not take into consideration: + * - variables within declaration files + * - variables within ambient module declarations + */ +function collectUnusedVariables< + TMessageIds extends string, + TOptions extends readonly unknown[] +>( + context: Readonly>, +): ReadonlySet { + return UnusedVarsVisitor.collectUnusedVariables(context); +} + +export { collectUnusedVariables }; diff --git a/packages/eslint-plugin/src/util/index.ts b/packages/eslint-plugin/src/util/index.ts index 672f50dc4ff..af0a64eddbf 100644 --- a/packages/eslint-plugin/src/util/index.ts +++ b/packages/eslint-plugin/src/util/index.ts @@ -1,6 +1,7 @@ import { ESLintUtils } from '@typescript-eslint/experimental-utils'; export * from './astUtils'; +export * from './collectUnusedVariables'; export * from './createRule'; export * from './isTypeReadonly'; export * from './misc'; diff --git a/packages/eslint-plugin/tests/rules/no-unused-vars.test.ts b/packages/eslint-plugin/tests/rules/no-unused-vars.test.ts index 9bc46bc75ad..49d744d4fd7 100644 --- a/packages/eslint-plugin/tests/rules/no-unused-vars.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unused-vars.test.ts @@ -927,6 +927,39 @@ export declare function setAlignment(value: \`\${VerticalAlignment}-\${Horizonta type EnthusiasticGreeting = \`\${Uppercase} - \${Lowercase} - \${Capitalize} - \${Uncapitalize}\`; export type HELLO = EnthusiasticGreeting<"heLLo">; `, + // https://github.com/typescript-eslint/typescript-eslint/issues/2714 + { + code: ` +interface IItem { + title: string; + url: string; + children?: IItem[]; +} + `, + // unreported because it's in a decl file, even though it's only self-referenced + filename: 'foo.d.ts', + }, + // https://github.com/typescript-eslint/typescript-eslint/issues/2648 + { + code: ` +namespace _Foo { + export const bar = 1; + export const baz = Foo.bar; +} + `, + // ignored by pattern, even though it's only self-referenced + options: [{ varsIgnorePattern: '^_' }], + }, + { + code: ` +interface _Foo { + a: string; + b: Foo; +} + `, + // ignored by pattern, even though it's only self-referenced + options: [{ varsIgnorePattern: '^_' }], + }, ], invalid: [ @@ -1376,8 +1409,8 @@ namespace Foo { action: 'defined', additional: '', }, - line: 2, - column: 11, + line: 4, + column: 15, }, ], }, @@ -1408,8 +1441,8 @@ namespace Foo { action: 'defined', additional: '', }, - line: 3, - column: 13, + line: 5, + column: 17, }, ], }, @@ -1424,7 +1457,7 @@ interface Foo { errors: [ { messageId: 'unusedVar', - line: 2, + line: 4, data: { varName: 'Foo', action: 'defined', @@ -1575,5 +1608,26 @@ export namespace Foo { }, ], }, + { + code: ` +interface Foo { + a: string; +} +interface Foo { + b: Foo; +} + `, + errors: [ + { + messageId: 'unusedVar', + line: 6, + data: { + varName: 'Foo', + action: 'defined', + additional: '', + }, + }, + ], + }, ], }); diff --git a/packages/eslint-plugin/typings/eslint-rules.d.ts b/packages/eslint-plugin/typings/eslint-rules.d.ts index 4e10fe23578..93b75e3a956 100644 --- a/packages/eslint-plugin/typings/eslint-rules.d.ts +++ b/packages/eslint-plugin/typings/eslint-rules.d.ts @@ -811,3 +811,17 @@ declare module 'eslint/lib/rules/space-infix-ops' { >; export = rule; } + +declare module 'eslint/lib/rules/utils/ast-utils' { + import { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils'; + + const utils: { + getNameLocationInGlobalDirectiveComment( + sourceCode: TSESLint.SourceCode, + comment: TSESTree.Comment, + name: string, + ): TSESTree.SourceLocation; + }; + + export = utils; +} diff --git a/packages/experimental-utils/src/ast-utils/predicates.ts b/packages/experimental-utils/src/ast-utils/predicates.ts index c23f30f1707..bf62a6f77cb 100644 --- a/packages/experimental-utils/src/ast-utils/predicates.ts +++ b/packages/experimental-utils/src/ast-utils/predicates.ts @@ -214,6 +214,27 @@ function isAwaitKeyword( return node?.type === AST_TOKEN_TYPES.Identifier && node.value === 'await'; } +function isLoop( + node: TSESTree.Node | undefined | null, +): node is + | TSESTree.DoWhileStatement + | TSESTree.ForStatement + | TSESTree.ForInStatement + | TSESTree.ForOfStatement + | TSESTree.WhileStatement { + if (!node) { + return false; + } + + return ( + node.type === AST_NODE_TYPES.DoWhileStatement || + node.type === AST_NODE_TYPES.ForStatement || + node.type === AST_NODE_TYPES.ForInStatement || + node.type === AST_NODE_TYPES.ForOfStatement || + node.type === AST_NODE_TYPES.WhileStatement + ); +} + export { isAwaitExpression, isAwaitKeyword, @@ -223,6 +244,7 @@ export { isFunctionOrFunctionType, isFunctionType, isIdentifier, + isLoop, isLogicalOrOperator, isNonNullAssertionPunctuator, isNotNonNullAssertionPunctuator, diff --git a/packages/experimental-utils/src/ts-eslint/Scope.ts b/packages/experimental-utils/src/ts-eslint/Scope.ts index 2934a4d2756..6907e2290fd 100644 --- a/packages/experimental-utils/src/ts-eslint/Scope.ts +++ b/packages/experimental-utils/src/ts-eslint/Scope.ts @@ -1,56 +1,51 @@ /* eslint-disable @typescript-eslint/no-namespace */ import * as scopeManager from '@typescript-eslint/scope-manager'; -import { TSESTree } from '@typescript-eslint/types'; namespace Scope { - // ESLint defines global variables using the eslint-scope Variable class - // So a variable in the scope may be either of these - declare class ESLintScopeVariable { - public readonly defs: Definition[]; - public readonly identifiers: TSESTree.Identifier[]; - public readonly name: string; - public readonly references: Reference[]; - public readonly scope: Scope; - - /** - * Written to by ESLint. - * If this key exists, this variable is a global variable added by ESLint. - * If this is `true`, this variable can be assigned arbitrary values. - * If this is `false`, this variable is readonly. - */ - public writeable?: boolean; // note that this isn't a typo - ESlint uses this spelling here - - /** - * Written to by ESLint. - * This property is undefined if there are no globals directive comments. - * The array of globals directive comments which defined this global variable in the source code file. - */ - public eslintExplicitGlobal?: boolean; - - /** - * Written to by ESLint. - * The configured value in config files. This can be different from `variable.writeable` if there are globals directive comments. - */ - public eslintImplicitGlobalSetting?: 'readonly' | 'writable'; - - /** - * Written to by ESLint. - * If this key exists, it is a global variable added by ESLint. - * If `true`, this global variable was defined by a globals directive comment in the source code file. - */ - public eslintExplicitGlobalComments?: TSESTree.Comment[]; - } - export type ScopeManager = scopeManager.ScopeManager; export type Reference = scopeManager.Reference; - export type Variable = scopeManager.Variable | ESLintScopeVariable; + export type Variable = + | scopeManager.Variable + | scopeManager.ESLintScopeVariable; export type Scope = scopeManager.Scope; export const ScopeType = scopeManager.ScopeType; // TODO - in the next major, clean this up with a breaking change export type DefinitionType = scopeManager.Definition; export type Definition = scopeManager.Definition; export const DefinitionType = scopeManager.DefinitionType; + + export namespace Definitions { + export type CatchClauseDefinition = scopeManager.CatchClauseDefinition; + export type ClassNameDefinition = scopeManager.ClassNameDefinition; + export type FunctionNameDefinition = scopeManager.FunctionNameDefinition; + export type ImplicitGlobalVariableDefinition = scopeManager.ImplicitGlobalVariableDefinition; + export type ImportBindingDefinition = scopeManager.ImportBindingDefinition; + export type ParameterDefinition = scopeManager.ParameterDefinition; + export type TSEnumMemberDefinition = scopeManager.TSEnumMemberDefinition; + export type TSEnumNameDefinition = scopeManager.TSEnumNameDefinition; + export type TSModuleNameDefinition = scopeManager.TSModuleNameDefinition; + export type TypeDefinition = scopeManager.TypeDefinition; + export type VariableDefinition = scopeManager.VariableDefinition; + } + export namespace Scopes { + export type BlockScope = scopeManager.BlockScope; + export type CatchScope = scopeManager.CatchScope; + export type ClassScope = scopeManager.ClassScope; + export type ConditionalTypeScope = scopeManager.ConditionalTypeScope; + export type ForScope = scopeManager.ForScope; + export type FunctionExpressionNameScope = scopeManager.FunctionExpressionNameScope; + export type FunctionScope = scopeManager.FunctionScope; + export type FunctionTypeScope = scopeManager.FunctionTypeScope; + export type GlobalScope = scopeManager.GlobalScope; + export type MappedTypeScope = scopeManager.MappedTypeScope; + export type ModuleScope = scopeManager.ModuleScope; + export type SwitchScope = scopeManager.SwitchScope; + export type TSEnumScope = scopeManager.TSEnumScope; + export type TSModuleScope = scopeManager.TSModuleScope; + export type TypeScope = scopeManager.TypeScope; + export type WithScope = scopeManager.WithScope; + } } export { Scope }; diff --git a/packages/scope-manager/src/referencer/VisitorBase.ts b/packages/scope-manager/src/referencer/VisitorBase.ts index 0d37ac31e35..eb4f55eccea 100644 --- a/packages/scope-manager/src/referencer/VisitorBase.ts +++ b/packages/scope-manager/src/referencer/VisitorBase.ts @@ -3,6 +3,7 @@ import { visitorKeys, VisitorKeys } from '@typescript-eslint/visitor-keys'; interface VisitorOptions { childVisitorKeys?: VisitorKeys | null; + visitChildrenEvenIfSelectorExists?: boolean; } function isObject(obj: unknown): obj is Record { @@ -18,8 +19,11 @@ type NodeVisitor = { abstract class VisitorBase { readonly #childVisitorKeys: VisitorKeys; + readonly #visitChildrenEvenIfSelectorExists: boolean; constructor(options: VisitorOptions) { this.#childVisitorKeys = options.childVisitorKeys ?? visitorKeys; + this.#visitChildrenEvenIfSelectorExists = + options.visitChildrenEvenIfSelectorExists ?? false; } /** @@ -69,7 +73,10 @@ abstract class VisitorBase { const visitor = (this as NodeVisitor)[node.type]; if (visitor) { - return visitor.call(this, node); + visitor.call(this, node); + if (!this.#visitChildrenEvenIfSelectorExists) { + return; + } } this.visitChildren(node);