Skip to content

Commit

Permalink
fix(eslint-plugin): [no-unused-vars] correct detection of unused vars…
Browse files Browse the repository at this point in the history
… in a declared module with `export =` (#2505)

If a `declare module` has an `export =` in its body, then TS will only export that.
If it doesn't have an `export =`, then all things are ambiently exported.

This adds handling to correctly detect this case.
  • Loading branch information
bradzacher committed Sep 6, 2020
1 parent 2ada5af commit 3d07a99
Show file tree
Hide file tree
Showing 7 changed files with 105 additions and 16 deletions.
Expand Up @@ -10,7 +10,10 @@ type RuleNode =
| TSESTree.TSModuleBlock
| TSESTree.TSTypeLiteral
| TSESTree.TSInterfaceBody;
type Member = TSESTree.ClassElement | TSESTree.Statement | TSESTree.TypeElement;
type Member =
| TSESTree.ClassElement
| TSESTree.ProgramStatement
| TSESTree.TypeElement;

export default util.createRule({
name: 'adjacent-overload-signatures',
Expand Down
2 changes: 1 addition & 1 deletion packages/eslint-plugin/src/rules/indent.ts
Expand Up @@ -410,7 +410,7 @@ export default util.createRule<Options, MessageIds>({
// transform it to a BlockStatement
return rules['BlockStatement, ClassBody']({
type: AST_NODE_TYPES.BlockStatement,
body: node.body,
body: node.body as any,

// location data
parent: node.parent,
Expand Down
37 changes: 34 additions & 3 deletions packages/eslint-plugin/src/rules/no-unused-vars.ts
Expand Up @@ -29,6 +29,7 @@ export default util.createRule<Options, MessageIds>({
create(context) {
const rules = baseRule.create(context);
const filename = context.getFilename();
const MODULE_DECL_CACHE = new Map<TSESTree.TSModuleDeclaration, boolean>();

/**
* Gets a list of TS module definitions for a specified variable.
Expand Down Expand Up @@ -209,7 +210,7 @@ export default util.createRule<Options, MessageIds>({
},

// declaration file handling
[declarationSelector(AST_NODE_TYPES.Program, true)](
[ambientDeclarationSelector(AST_NODE_TYPES.Program, true)](
node: DeclarationSelectorNode,
): void {
if (!util.isDefinitionFile(filename)) {
Expand All @@ -219,14 +220,44 @@ export default util.createRule<Options, MessageIds>({
},

// declared namespace handling
[declarationSelector(
[ambientDeclarationSelector(
'TSModuleDeclaration[declare = true] > TSModuleBlock',
false,
)](node: DeclarationSelectorNode): void {
const moduleDecl = util.nullThrows(
node.parent?.parent,
util.NullThrowsReasons.MissingParent,
) as TSESTree.TSModuleDeclaration;

// declared modules with an `export =` statement will only export that one thing
// all other statements are not automatically exported in this case
if (checkModuleDeclForExportEquals(moduleDecl)) {
return;
}

markDeclarationChildAsUsed(node);
},
};

function checkModuleDeclForExportEquals(
node: TSESTree.TSModuleDeclaration,
): boolean {
const cached = MODULE_DECL_CACHE.get(node);
if (cached != null) {
return cached;
}

for (const statement of node.body?.body ?? []) {
if (statement.type === AST_NODE_TYPES.TSExportAssignment) {
MODULE_DECL_CACHE.set(node, true);
return true;
}
}

MODULE_DECL_CACHE.set(node, false);
return false;
}

type DeclarationSelectorNode =
| TSESTree.TSInterfaceDeclaration
| TSESTree.TSTypeAliasDeclaration
Expand All @@ -236,7 +267,7 @@ export default util.createRule<Options, MessageIds>({
| TSESTree.TSEnumDeclaration
| TSESTree.TSModuleDeclaration
| TSESTree.VariableDeclaration;
function declarationSelector(
function ambientDeclarationSelector(
parent: string,
childDeclare: boolean,
): string {
Expand Down
32 changes: 32 additions & 0 deletions packages/eslint-plugin/tests/rules/no-unused-vars.test.ts
Expand Up @@ -848,6 +848,18 @@ export type Test<U> = U extends (arg: {
jsxFragmentName: 'Fragment',
},
},
`
declare module 'foo' {
type Test = 1;
}
`,
`
declare module 'foo' {
type Test = 1;
const x: Test = 1;
export = x;
}
`,
],

invalid: [
Expand Down Expand Up @@ -1424,5 +1436,25 @@ export const ComponentFoo = () => {
},
],
},
{
code: `
declare module 'foo' {
type Test = any;
const x = 1;
export = x;
}
`,
errors: [
{
messageId: 'unusedVar',
line: 3,
data: {
varName: 'Test',
action: 'defined',
additional: '',
},
},
],
},
],
});
1 change: 0 additions & 1 deletion packages/eslint-plugin/typings/eslint-rules.d.ts
Expand Up @@ -48,7 +48,6 @@ declare module 'eslint/lib/rules/camelcase' {
declare module 'eslint/lib/rules/indent' {
import { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils';

type Listener = (node: TSESTree.Node) => void;
type ElementList = number | 'first' | 'off';
const rule: TSESLint.RuleModule<
'wrongIndentation',
Expand Down
26 changes: 18 additions & 8 deletions packages/scope-manager/src/scope/ScopeBase.ts
Expand Up @@ -15,6 +15,7 @@ import {
ReferenceTypeFlag,
} from '../referencer/Reference';
import { Variable } from '../variable';
import { TSModuleScope } from './TSModuleScope';

/**
* Test if scope is strict
Expand Down Expand Up @@ -124,6 +125,14 @@ function registerScope(scopeManager: ScopeManager, scope: Scope): void {

const generator = createIdGenerator();

type VariableScope = GlobalScope | FunctionScope | ModuleScope | TSModuleScope;
const VARIABLE_SCOPE_TYPES = new Set([
ScopeType.global,
ScopeType.function,
ScopeType.module,
ScopeType.tsModule,
]);

type AnyScope = ScopeBase<ScopeType, TSESTree.Node, Scope | null>;
abstract class ScopeBase<
TType extends ScopeType,
Expand Down Expand Up @@ -209,11 +218,11 @@ abstract class ScopeBase<
*/
public readonly variables: Variable[] = [];
/**
* For 'global', 'function', and 'module' scopes, this is a self-reference.
* For scopes that can contain variable declarations, this is a self-reference.
* For other scope types this is the *variableScope* value of the parent scope.
* @public
*/
public readonly variableScope: GlobalScope | FunctionScope | ModuleScope;
public readonly variableScope: VariableScope;

constructor(
scopeManager: ScopeManager,
Expand All @@ -228,12 +237,9 @@ abstract class ScopeBase<
this.#dynamic =
this.type === ScopeType.global || this.type === ScopeType.with;
this.block = block;
this.variableScope =
this.type === 'global' ||
this.type === 'function' ||
this.type === 'module'
? (this as AnyScope['variableScope'])
: upperScopeAsScopeBase.variableScope;
this.variableScope = this.isVariableScope()
? this
: upperScopeAsScopeBase.variableScope;
this.upper = upperScope;

/**
Expand All @@ -252,6 +258,10 @@ abstract class ScopeBase<
registerScope(scopeManager, this as Scope);
}

private isVariableScope(): this is VariableScope {
return VARIABLE_SCOPE_TYPES.has(this.type);
}

public shouldStaticallyClose(): boolean {
return !this.#dynamic;
}
Expand Down
18 changes: 16 additions & 2 deletions packages/types/src/ts-estree.ts
Expand Up @@ -457,6 +457,20 @@ export type PrimaryExpression =
| TemplateLiteral
| ThisExpression
| TSNullKeyword;
export type ProgramStatement =
| ClassDeclaration
| ExportAllDeclaration
| ExportDefaultDeclaration
| ExportNamedDeclaration
| ImportDeclaration
| Statement
| TSDeclareFunction
| TSEnumDeclaration
| TSExportAssignment
| TSImportEqualsDeclaration
| TSInterfaceDeclaration
| TSNamespaceExportDeclaration
| TSTypeAliasDeclaration;
export type Property = PropertyComputedName | PropertyNonComputedName;
export type PropertyName = PropertyNameComputed | PropertyNameNonComputed;
export type PropertyNameComputed = Expression;
Expand Down Expand Up @@ -1146,7 +1160,7 @@ export interface ObjectPattern extends BaseNode {

export interface Program extends BaseNode {
type: AST_NODE_TYPES.Program;
body: Statement[];
body: ProgramStatement[];
sourceType: 'module' | 'script';
comments?: Comment[];
tokens?: Token[];
Expand Down Expand Up @@ -1473,7 +1487,7 @@ export interface TSMethodSignatureNonComputedName

export interface TSModuleBlock extends BaseNode {
type: AST_NODE_TYPES.TSModuleBlock;
body: Statement[];
body: ProgramStatement[];
}

export interface TSModuleDeclaration extends BaseNode {
Expand Down

0 comments on commit 3d07a99

Please sign in to comment.