-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(eslint-plugin): [no-unused-var] handle implicit exports in declaration files #10714
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(eslint-plugin): [no-unused-var] handle implicit exports in declaration files #10714
Conversation
Thanks for the PR, @ronami! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
View your CI Pipeline Execution ↗ for commit f84ded5.
☁️ Nx Cloud last updated this comment at |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10714 +/- ##
==========================================
+ Coverage 87.19% 87.46% +0.27%
==========================================
Files 450 469 +19
Lines 15632 16073 +441
Branches 4570 4656 +86
==========================================
+ Hits 13630 14058 +428
- Misses 1645 1658 +13
Partials 357 357
Flags with carried forward coverage won't be shown. Click here to find out more.
|
// declaration file handling | ||
[ambientDeclarationSelector(AST_NODE_TYPES.Program, true)]( | ||
// top-level declaration file handling | ||
[ambientDeclarationSelector(AST_NODE_TYPES.Program)]( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing various edge cases, I think that only checking `declare'd module-level values creates some incorrect reports (playground link).
Removing this didn't cause any tests to fail, though I could be missing an edge case.
const moduleDecl = nullThrows( | ||
node.parent, | ||
NullThrowsReasons.MissingParent, | ||
) as TSESTree.Program; | ||
|
||
if (checkForOverridingExportStatements(moduleDecl)) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This addition is so the rule would report the following (playground link):
// should be reported but doesn't
declare class Foo { }
export {}
Along with not reporting on the following (playground link):
// reported but shouldn't
class Foo { }
// even though this shows up as a compiler error, this isn't marked as used
// uncomment the code below and TypeScript would mark this as unused:
// export {};
[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; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This addition is for not reporting the following (playground link):
export namespace Foo {
// reported but shouldn't
const foo: 1234;
}
const moduleDecl = nullThrows( | ||
node.parent.parent, | ||
NullThrowsReasons.MissingParent, | ||
) as TSESTree.TSModuleDeclaration; | ||
|
||
if (checkForOverridingExportStatements(moduleDecl)) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is necessary so the following is reported (playground link):
export declare namespace Foo {
namespace Bar {
namespace Baz {
// should be reported but isn't
namespace Bam {
const x = 1;
}
export { };
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whew! What a PR. This is a tricky rule. I had to go in and out of the old & new code a few times to get a feel for it.
I think everything you're saying makes sense. As I understand it, the main changes around the new hasOverridingExportStatement
& the removal of the no-longer-necessary child selectors seem correct. Anything that reduces ambientDeclarationSelector
's complexity is off to a good start. 😄
👍 from me on all the edge cases as described. I think we should get another review from @typescript-eslint/triage-team just to be thorough.
@@ -144,7 +144,10 @@ export default createRule<Options, MessageIds>({ | |||
}, | |||
defaultOptions: [{}], | |||
create(context, [firstOption]) { | |||
const MODULE_DECL_CACHE = new Map<TSESTree.TSModuleDeclaration, boolean>(); | |||
const MODULE_DECL_CACHE = new Map< | |||
TSESTree.Program | TSESTree.TSModuleDeclaration, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Refactor] The .body
seems to be assumed to always exist in code. So I think anywhere that refers to the TSModuleDeclaration
type can assume this:
type ModuleDeclarationWithBody = MakeRequired<TSESTree.TSModuleDeclaration, 'body'>;
TSESTree.Program | TSESTree.TSModuleDeclaration, | |
TSESTree.Program | ModuleDeclarationWithBody, |
...which means the 'Module declarations with no body are filtered out by the rule'
nullThrows
can be removed.
(unless, did I misinterpret?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I've updated the PR with your suggestion, thanks 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Praise] Really thorough investigation and tests. Nicely done! 👏
This would probably be a lot of work (so I can attempt to do it when I have a chance), but the thing I want to test most is to enable this rule on DT and then verify that what it says makes sense; declaration files have so many edge cases... |
messageId: 'unusedVar', | ||
}, | ||
], | ||
filename: 'foo.d.ts', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I would do here as well is to ensure there's testing for .d.cts
and .d.mts
files (and their associated syntaxes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typescript-eslint/packages/eslint-plugin/src/util/misc.ts
Lines 13 to 29 in 3646ec0
const DEFINITION_EXTENSIONS = [ | |
ts.Extension.Dts, | |
ts.Extension.Dcts, | |
ts.Extension.Dmts, | |
] as const; | |
/** | |
* Check if the context file name is *.d.ts or *.d.tsx | |
*/ | |
export function isDefinitionFile(fileName: string): boolean { | |
const lowerFileName = fileName.toLowerCase(); | |
for (const definitionExt of DEFINITION_EXTENSIONS) { | |
if (lowerFileName.endsWith(definitionExt)) { | |
return true; | |
} | |
} | |
return false; | |
} |
It's covered by this utility so it'll work as expected so long as this utility is right!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, unfortunately that function is incomplete because it doesn't handle filenames like .d.css.ts
(dt-tools uses a glob https://github.com/microsoft/DefinitelyTyped-tools/blob/1f6e243b03e6d557bf43e6312b0b8f2d47819e52/packages/utils/src/miscellany.ts#L97)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are those other file names officially supported by TS...?
We don't really want to be in the business of guessing if a file might be a decl file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, since TS 5.0; the feature is used to provide declarations to files with non-TS extensions, e.g. a .d.css.ts
file can provide types for importing a .css
file.
https://devblogs.microsoft.com/typescript/announcing-typescript-5-0/#allowarbitraryextensions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened #10911 to discuss this 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
If you can I'd suggest trying the rule over the DefinitelyTyped repo to look for false positives. There'll be a lot of noise so worth time-boxing it -- but would be a good final validation that you haven't missed anything.
Thanks! I've checked quite a bit of declaration files from definitelytyped, here are some of the types that this PR affects:
I checked only a small subset of definitelytyped's types, but I didn't find any incorrect or missing reports (I've relied on TypeScript's |
On that first one, I'm actually surprised we are flagging |
To my understanding, in addition to Removing the |
That'd make some sense, yeah. (Well, not that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have some lint errors -- pls fix and we can merge! |
| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.24.0 | 8.26.0 | | npm | @typescript-eslint/parser | 8.24.0 | 8.26.0 | ## [v8.26.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8260-2025-03-03) ##### 🚀 Features - **eslint-plugin:** \[unified-signatures] support ignoring overload signatures with different JSDoc comments ([#10781](typescript-eslint/typescript-eslint#10781)) - **eslint-plugin:** \[explicit-module-boundary-types] add an option to ignore overload implementations ([#10889](typescript-eslint/typescript-eslint#10889)) - **eslint-plugin:** \[no-unused-var] handle implicit exports in declaration files ([#10714](typescript-eslint/typescript-eslint#10714)) - support TypeScript 5.8 ([#10903](typescript-eslint/typescript-eslint#10903)) - **eslint-plugin:** \[no-unnecessary-type-parameters] special case tuples and parameter location arrays as single-use ([#9536](typescript-eslint/typescript-eslint#9536)) ##### 🩹 Fixes - **eslint-plugin:** \[no-unnecessary-type-assertion] handle unknown ([#10875](typescript-eslint/typescript-eslint#10875)) - **eslint-plugin:** \[no-invalid-void-type] report `accessor` properties with an invalid `void` type ([#10864](typescript-eslint/typescript-eslint#10864)) - **eslint-plugin:** \[unified-signatures] does not differentiate truly private methods ([#10806](typescript-eslint/typescript-eslint#10806)) ##### ❤️ Thank You - Andrea Simone Costa [@jfet97](https://github.com/jfet97) - Dirk Luijk [@dirkluijk](https://github.com/dirkluijk) - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) - Yukihiro Hasegawa [@y-hsgw](https://github.com/y-hsgw) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website. ## [v8.25.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8250-2025-02-24) ##### 🚀 Features - **eslint-plugin:** \[no-misused-spread] add suggestions ([#10719](typescript-eslint/typescript-eslint#10719)) ##### 🩹 Fixes - **eslint-plugin:** \[prefer-nullish-coalescing] report on chain expressions in a ternary ([#10708](typescript-eslint/typescript-eslint#10708)) - **eslint-plugin:** \[no-deprecated] report usage of deprecated private identifiers ([#10844](typescript-eslint/typescript-eslint#10844)) - **eslint-plugin:** \[unified-signatures] handle getter-setter ([#10818](typescript-eslint/typescript-eslint#10818)) ##### ❤️ Thank You - Olivier Zalmanski [@OlivierZal](https://github.com/OlivierZal) - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website. ## [v8.24.1](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8241-2025-02-17) ##### 🩹 Fixes - **eslint-plugin:** \[class-methods-use-this] check `accessor` methods with a function initializer ([#10796](typescript-eslint/typescript-eslint#10796)) - **eslint-plugin:** \[no-misused-promises] don't report on `static` `accessor` properties ([#10814](typescript-eslint/typescript-eslint#10814)) - **eslint-plugin:** \[no-deprecated] don't report on deprecated `accessor` property declaration ([#10813](typescript-eslint/typescript-eslint#10813)) - **eslint-plugin:** \[explicit-member-accessibility] check `accessor` class properties for missing accessibility modifier ([#10805](typescript-eslint/typescript-eslint#10805)) - **eslint-plugin:** \[explicit-module-boundary-types] check `accessor` class properties with a function initializer ([#10804](typescript-eslint/typescript-eslint#10804)) - **eslint-plugin:** \[prefer-return-this-type] check `accessor` properties with a function initializer ([#10794](typescript-eslint/typescript-eslint#10794)) - **eslint-plugin:** \[consistent-generic-constructors] check `accessor` class properties ([#10789](typescript-eslint/typescript-eslint#10789)) - **eslint-plugin:** \[no-unsafe-assignment] report on an `any` value assigned as an initializer of an `accessor` property ([#10785](typescript-eslint/typescript-eslint#10785)) - **eslint-plugin:** \[no-unnecessary-template-expression] ignore enum and enum members ([#10782](typescript-eslint/typescript-eslint#10782)) - **eslint-plugin:** \[no-inferrable-types] handle accessor ([#10780](typescript-eslint/typescript-eslint#10780)) ##### ❤️ Thank You - Ronen Amiel - YeonJuan You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.
PR Checklist
Overview
This PR continues #8611 and attempts to tackle #2867.
Note that I'm far from having a thorough understanding of how TypeScript handles exported values/types from a
.d.ts
file, and this seems to have a lot of undescribed edge cases, so I'm mainly basing this off on trying out edge cases and reverse engineering this locally.Some notes/thoughts:
It seems namespaces, modules, and ambient module declarations without specific types of exports (
export { ... }
,export default ...
orexport * from '...'
,export = ...
) have all of their declared variables implicitly exported (multi-file typescript playground link showing some of the cases, I've had a hard time showing other edge cases on a somewhat limited multi-file editor, can provide codesandbox/stackblitz playground URLs).Currently, the rule only checks for
export = ...
syntax and only for namespace/ambient modules, though this seems to be relevant for other cases too.To my understanding, some of this is relevant to non-definition files, mostly around declared modules and namespaces (playground link).
I've used TypeScript's
'xxx' is declared but its value is never read.
messages and tried to replicate that behavior to determine whether a variable should be marked as unused or not.It seems
no-unused-vars
fails to report unused variables in module and ambient module declarations in definition files (playground link).This PR adjusts the rule to report these, though this wasn't described in the original issue. I've opened a new issue to discuss this in Bug: [no-unused-vars] doesn't report unused variables in module and ambient module declarations in definition files #10713. That issue hasn't been triaged yet, but since it's closely related to [no-unused-vars] False positives with members of exported namespace in .d.ts file #2867, it made sense to me to tackle them both on a single PR.
Please let me know if this part should be reverted (or at least if/until the former is triaged) or be in a separate PR.
Interestingly, it seems that if a
default export
references a variable, it will prevent that module/namespace from implicitly exporting its other, non-exported members (playgroun link). However, if thedefault export
doesn't reference an identifier, it won't (playground link). I'm not sure if that's the correct behavior or if I missed something about this.It seems ambient declarations (
declare ...
) behave similarly (playground link).I might be missing something, but based on feat(eslint-plugin): [no-unused-var] handle implicit exports in declaration files #8611, the following, for example, should be reported with an error (playground link):
However, this seems valid, as TypeScript doesn't report any variables as unused, and an importing module can access
foo
.I've continued the work done in feat(eslint-plugin): [no-unused-var] handle implicit exports in declaration files #8611, so this includes attribution for the author; please let me know if this should be changed.