Skip to content

feat(eslint-plugin): [no-unused-var] handle implicit exports in declaration files #8611

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

feat(eslint-plugin): [no-unused-var] handle implicit exports in declaration files #8611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7117fcc
WIP
jakebailey Mar 3, 2024
3d2fc3b
WIP
jakebailey Mar 3, 2024
f9f18c5
revert nx
jakebailey Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 52 additions & 1 deletion packages/eslint-plugin/src/util/collectUnusedVariables.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {
ImplicitLibVariable,
ScopeType,
Variable,
Visitor,
} from '@typescript-eslint/scope-manager';
import type { TSESTree } from '@typescript-eslint/utils';
Expand All @@ -10,6 +11,7 @@ import {
ESLintUtils,
TSESLint,
} from '@typescript-eslint/utils';
import { isDefinitionFile } from './misc';

class UnusedVarsVisitor<
MessageIds extends string,
Expand All @@ -22,6 +24,7 @@ class UnusedVarsVisitor<

readonly #scopeManager: TSESLint.Scope.ScopeManager;
// readonly #unusedVariables = new Set<TSESLint.Scope.Variable>();
readonly #isDefinitionFile: boolean;

private constructor(context: TSESLint.RuleContext<MessageIds, Options>) {
super({
Expand All @@ -32,6 +35,8 @@ class UnusedVarsVisitor<
context.sourceCode.scopeManager,
'Missing required scope manager',
);

this.#isDefinitionFile = isDefinitionFile(context.filename);
}

public static collectUnusedVariables<
Expand Down Expand Up @@ -60,7 +65,12 @@ class UnusedVarsVisitor<
scope: TSESLint.Scope.Scope,
unusedVariables = new Set<TSESLint.Scope.Variable>(),
): ReadonlySet<TSESLint.Scope.Variable> {
for (const variable of scope.variables) {
const implicitlyExported = allVariablesImplicitlyExported(
scope,
this.#isDefinitionFile,
);

for (const variable of implicitlyExported ? [] : scope.variables) {
if (
// skip function expression names,
scope.functionExpressionScope ||
Expand Down Expand Up @@ -438,6 +448,47 @@ 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 ||
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 (
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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

declare namespace Foo {
  const x = 1;
  export const y = 3;
}

Foo.x;
Foo.y;

The export doesn't always restrict other implicit, ambient exports, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, the rules for this are super annoying. What I have currently may just be true only for declaration files, but "ambient" ones with declare (in ts files only?) may have different rules?

Copy link
Member

@bradzacher bradzacher Nov 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like these rules apply to both ts files and dts files as well
playground? Or did I misunderstand you?

It looks like export = is invalid in a namespace but I think that's the only case that invalidates this logic on declared namespaces.

So I think this entire function can just be updated to also short-circuit on if (namespace.declare) { return namespace_has_export_equals }

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are more issues than just this one IIRC; I've been so busy with other stuff that I haven't been able to sit down and actually fix this PR, sorry.

I'm also not 100% sure what works and doesn't work in the playground when it comes to d.ts files; not really something we often play with; I've been meaning to figure out something better for testing this

If it's bugging you all to stay open, I can close it, though if someone else attempts it too I'd love to review it and double check everything (but I don't think anyone's working on it besides me)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah Josh just pinged me cos I didn't respond :P
No rush at all.

return false;
}
}

return true;
}

const LOGICAL_ASSIGNMENT_OPERATORS = new Set(['&&=', '||=', '??=']);

/**
Expand Down
141 changes: 141 additions & 0 deletions packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1123,6 +1123,72 @@ 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',
},
{
code: `
declare module 'foo' {
export import Bar = Something.Bar;
const foo: 1234;
}
`,
filename: 'foo.d.ts',
},
],

invalid: [
Expand Down Expand Up @@ -1950,5 +2016,80 @@ 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: '',
},
},
],
},
{
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',
errors: [
{
messageId: 'unusedVar',
line: 4,
column: 9,
data: {
varName: 'foo',
action: 'defined',
additional: '',
},
},
],
},
],
});