Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.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 ourterms of service andprivacy 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
Uh oh!
There was an error while loading.Please reload this page.
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 onhttps://opencollective.com/typescript-eslint. |
netlifybot commentedJan 26, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview fortypescript-eslint ready!
To edit notification comments on pull requests, go to yourNetlify site configuration. |
nx-cloudbot commentedJan 26, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
View yourCI Pipeline Execution ↗ for commitf84ded5.
☁️Nx Cloud last updated this comment at |
codecovbot commentedJan 26, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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-leveldeclaration 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'tdeclareclassFoo{}export{}
Along with not reporting on the following (playground link):
// reported but shouldn'tclassFoo{}// 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):
exportnamespaceFoo{// reported but shouldn'tconstfoo: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):
exportdeclarenamespaceFoo{namespaceBar{namespaceBaz{// should be reported but isn'tnamespaceBam{constx=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 newhasOverridingExportStatement & the removal of the no-longer-necessary child selectors seem correct. Anything that reducesambientDeclarationSelector'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 theTSModuleDeclaration type can assume this:
typeModuleDeclarationWithBody=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 in3646ec0
| constDEFINITION_EXTENSIONS=[ | |
| ts.Extension.Dts, | |
| ts.Extension.Dcts, | |
| ts.Extension.Dmts, | |
| ]asconst; | |
| /** | |
| * Check if the context file name is *.d.ts or *.d.tsx | |
| */ | |
| exportfunctionisDefinitionFile(fileName:string):boolean{ | |
| constlowerFileName=fileName.toLowerCase(); | |
| for(constdefinitionExtofDEFINITION_EXTENSIONS){ | |
| if(lowerFileName.endsWith(definitionExt)){ | |
| returntrue; | |
| } | |
| } | |
| returnfalse; | |
| } |
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 globhttps://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.
ronami commentedMar 2, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 |
ronami commentedMar 2, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
To my understanding, in addition to Removing the |
jakebailey commentedMar 2, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
LGTM -- with the DefinitelyTyped testing I think we're good to go here.
We have some lint errors -- pls fix and we can merge! |
d8d4b3c intotypescript-eslint:mainUh oh!
There was an error while loading.Please reload this page.
| 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- YeonJuanYou 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.
Uh oh!
There was an error while loading.Please reload this page.
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.tsfile, 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 ruleonly 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-varsfails 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 inBug: [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 exportreferences a variable, it will prevent that module/namespace from implicitly exporting its other, non-exported members (playgroun link). However, if thedefault exportdoesn'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 onfeat(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 infeat(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.