Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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.

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

Already on GitHub?Sign in to your account

Closed
Show file tree
Hide file tree
Changes fromall commits
Commits
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 deletionpackages/eslint-plugin/src/util/collectUnusedVariables.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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 DownExpand 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.

declarenamespaceFoo{constx=1;exportconsty=3;}Foo.x;Foo.y;

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

Copy link
CollaboratorAuthor

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

@bradzacherbradzacherNov 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 likeexport = is invalid in anamespace but I think that's the only case that invalidates this logic ondeclared namespaces.

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

Copy link
CollaboratorAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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: '',
},
},
],
},
],
});
[8]ページ先頭
©2009-2025 Movatter.jp