fix(eslint-plugin): [no-unused-vars] no is assigned a value but only used as a type error when it has a same name in export statement#11322
Conversation
|
Thanks for the PR, @nayounsang! 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 project configuration. |
is assigned a value but only used as a type error when it has a same name type alias declaration exporteis assigned a value but only used as a type error when it has a same name
|
View your CI Pipeline Execution ↗ for commit a95d35e
☁️ Nx Cloud last updated this comment at |
Codecov Report✅ All modified and coverable lines are covered by tests. Please upload reports for the commit fccdd43 to get more accurate results. Additional details and impacted files@@ Coverage Diff @@
## main #11322 +/- ##
=======================================
Coverage 90.87% 90.88%
=======================================
Files 505 505
Lines 51146 51157 +11
Branches 8424 8429 +5
=======================================
+ Hits 46480 46494 +14
+ Misses 4652 4649 -3
Partials 14 14
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Its a different issue:#8315, but we need to figure out if this is a workable solution. |
Ah, it is different. should change validate logic. |
@nayounsang apologies, I'm not following - are you suggesting we should or shouldn't review this PR? |
|
@JoshuaKGoldberg The comments are my own monologue. I tend to take notes of everything and I just saw another issue that seemed related to this PR. After looking into it, it seems that even if this PR is resolved, there will be additional work needed to resolve the other issues. |
bradzacher
left a comment
There was a problem hiding this comment.
this is almost there -- but it misses this case:
const A = 0;
export interface A {}| function isMergedTypeDeclaration( | ||
| variable: Variable, | ||
| node: TSESTree.Node, | ||
| ): boolean { | ||
| return ( | ||
| node.type === AST_NODE_TYPES.TSTypeAliasDeclaration && | ||
| variable.isTypeVariable && | ||
| variable.isValueVariable | ||
| ); | ||
| } |
There was a problem hiding this comment.
A better implementation to catch the interface case would leverage variable.defs:
function isMergedTypeDeclaration(
variable: Variable,
): boolean {
return (
variable.defs.length > 1 &&
variable.isValueVariable &&
variable.isTypeVariable &&
variable.defs.some(d => d.type === DefinitionType.Type)
);
}There was a problem hiding this comment.
It can't cover this case:
// valid, but report
export const Foo = 0;
export type Foo = typeof Foo;
export const Foo = 0;
export interface Foo {}What I meant was to check for the export type Foo = ... statement. But in this case, it returns true if there are other type declarations besides the export statement. This is because node exists within the export statement when the isMergedTypeDeclaration is called.
So I've taken a different approach, can you please tell me if I'm going in the right direction?
is assigned a value but only used as a type error when it has a same nameis assigned a value but only used as a type error when it has a same name in export statement
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
LGTM, thanks! I'll defer to @bradzacher as well on review. 👍
bradzacher
left a comment
There was a problem hiding this comment.
There are the following 4 classes of cases here and we need to make sure we're reporting correctly on all of them.
///
/// case 1 -- both exported -- expected no reports
///
export interface Foo1 { }
export const Foo1 = 0;
export type Bar1 = 0;
export const Bar1 = 0;
export const Foo2 = 0;
export interface Foo2 { }
export const Bar2 = 0;
export type Bar2 = 0;
///
/// case 2 -- both exported via named export -- expected no reports
///
interface Foo7 { }
const Foo7 = 0;
export { Foo7 };
type Bar7 = 0;
const Bar7 = 0;
export { Bar7 };
const Foo8 = 0;
interface Foo8 { }
export { Foo8 };
const Bar8 = 0;
type Bar8 = 0;
export { Bar8 };
///
/// case 3 -- just value exported -- expected type is reported
///
interface Foo3 { }
export const Foo3 = 0;
type Bar3 = 0;
export const Bar3 = 0;
export const Foo4 = 0;
interface Foo4 { }
export const Bar4 = 0;
type Bar4 = 0;
///
/// case 4 -- just type exported -- expected value is reported
///
export interface Foo5 { }
const Foo5 = 0;
export type Bar5 = 0;
const Bar5 = 0;
const Foo6 = 0;
export interface Foo6 { }
const Bar6 = 0;
export type Bar6 = 0;The current implementation reports nothing on this code (which is technically half right heh).
Your current implementation:
- does not report on case 1 -- ✔️
- does not report on case 2 -- ✔️
- does not report on case 3 -- ❌
- reports on case 4 -- ✔️
So we'll need to handle this and add test cases
|
Hi. @bradzacher, There are some valid test cases I don't understand among the ones wrote before. This is added from #6873 |
|
@nayounsang the test cases may have been added in the past, but that doesn't mean they're right! They were right in so far as they allowed the rule to understand when a multi-declaration variable was exported in its second declaration, but the result is wrong. interface Foo {
// ^^^ Type Foo is unused
bar: string;
}
export const Foo = 'bar';export const Foo = 'bar';
interface Foo {
// ^^^ Type Foo is unused
bar: string;
}This is what we should expect to report for these two cases -- we should be able to understand that the variable's second declaration is unused. |
|
@kirkwaiblinger I'm going to delegate this issue on to another contributor. |
|
No worries, thanks for getting started on this! It's totally reasonable to pass on issues you don't have time for. FWIW we've also been pretty busy in the maintenance team. If anybody seeing this wants to continue this work / generally tackle #10658: please do! #10658 at time of writing is still accepting PRs. Just note that if you reuse code from this PR, please add a co-authored-by attribution to give nayounsang credit for the work they've done that's being used. Cheers! 💙 |
PR Checklist
is assigned a value but only used as a typeerror when it has a same name type alias declaration exported #10658Overview
Strict checks on exports with same type and variable name
isTypeVariable && isValueVariable.ClassName,TSEnumName,TSModuleName,ImportBindingandargumentsalso have same characteristicsPerform additional validating for abnormal cases
def.node.type !== AST_NODE_TYPES.TSTypeAliasDeclarationAdd test cases