fix(eslint-plugin): [no-unused-vars] no is assigned a value but only used as a type error when it has a same name
#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 85d49d7.
☁️ 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 #11322 +/- ##
=======================================
Coverage 90.84% 90.85%
=======================================
Files 501 501
Lines 50919 50929 +10
Branches 8387 8390 +3
=======================================
+ Hits 46256 46269 +13
+ Misses 4648 4645 -3
Partials 15 15
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| function isSafeUnusedExportCondition(variable: ScopeVariable): boolean { | ||
| if (variable instanceof ESLintScopeVariable) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
need advice
I don't know why this condition is necessary. Can't the arg type of isExported, isMergableExported be used as Variable?
Actually, it is used as Variable only.
There was a problem hiding this comment.
it is used as
Variableonly
👍 any reason not to change its type from ScopeVariable to Variable? I tried the three : ScopeVariable parameters to : Variable locally and everything looked happy.
|
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. |
| @@ -441,7 +467,9 @@ function isMergableExported(variable: ScopeVariable): boolean { | |||
| def.node.parent?.type === AST_NODE_TYPES.ExportNamedDeclaration) || | |||
| def.node.parent?.type === AST_NODE_TYPES.ExportDefaultDeclaration | |||
| ) { | |||
| return true; | |||
| return ( | |||
| safeFlag || def.node.type !== AST_NODE_TYPES.TSTypeAliasDeclaration | |||
There was a problem hiding this comment.
[Performance] There's no need to run through isSafeUnusedExportCondition(...) until it's used:
| safeFlag || def.node.type !== AST_NODE_TYPES.TSTypeAliasDeclaration | |
| def.node.type !== AST_NODE_TYPES.TSTypeAliasDeclaration || isSafeUnusedExportCondition(variable) |
There was a problem hiding this comment.
Yes, I learned more and your opinion is reasonable.
The number of defs is small, so the overhead of calling them unconditionally is greater than the complexity due to nested loops under certain conditions.
I committed.
| function isSafeUnusedExportCondition(variable: ScopeVariable): boolean { | ||
| if (variable instanceof ESLintScopeVariable) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
it is used as
Variableonly
👍 any reason not to change its type from ScopeVariable to Variable? I tried the three : ScopeVariable parameters to : Variable locally and everything looked happy.
| if (variable.isTypeVariable && variable.isValueVariable) { | ||
| return !variable.defs | ||
| .map(d => d.type) | ||
| .every(t => exportExceptDefTypes.includes(t)); |
There was a problem hiding this comment.
[Testing] If I comment out this .every, then all tests still pass. In fact, if the inside of the if just does return false; then tests still pass. So either this is unnecessary code or there's a gap in test coverage.
There was a problem hiding this comment.
The reason for this is because of #11322 (comment).
If type of node.parent start with Export and node is a TSTypeAliasDeclaration, then these syntaxes exist:
export type Foo = /* What */
declare namespace(module) Foo {
export type A = / * What */
}
Now, let's see what's possible with /* What */.
This issue is about variable that isTypeVariable && isValueVariable
So, tests are passed even if commented code.
But it's inconvenient. I think there's something I haven't anticipated yet. I need to dig deeper & I can refactor the code to make it clean.
I will test these cases in play ground
There was a problem hiding this comment.
- More detail memo...
- TSTypeAliasDeclaration & export
declare has been verified before, so we only test one (which also succeeds locally).
export type Foo = /* What */
- isTypeVariable && isValueVariable
Merged declaration, ClassName, TSEnumName,TSEnumMemberDefinition, TSModuleName, ImportBinding and arguments satisfy this condition.
Most of these are impossible syntax with export type A = /* ... */ .
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