fix(lint): fix noConfusingVoidType to accept generic types in a union type
#611
Conversation
github-actions
bot
added
A-Linter
unvalley
changed the title
noConfusingVoidType to accept generic types in a union type
|
Thanks for your reactivity! :) Could you explain the nature of the change? I wonder: should we accept all unions including |
If
Yes, right. I should add tests in this PR, at least tests for
Probably yes, because we don't have an option to get generic type identifier to accept. // typescript-eslint no-invalid-void-type test case example
{
code: 'type voidPromiseUnion = void | Promise<void>;',
options: [{ allowInGenericTypeArguments: ['Promise'] }],
},Our |
| for parent in node.parent()?.ancestors() { | ||
| match parent.kind() { | ||
| JsSyntaxKind::TS_UNION_TYPE_VARIANT_LIST => { | ||
| // checks if the union type contains generic type | ||
| for children in parent.descendants() { |
There was a problem hiding this comment.
I am not sure that descendants is the best choice here because it traverses the entire subtree. Instead, we should traverse every constituent and verify if the type is void or a parametrized type with void as argument.
This is the approach used by the ESLint rule.
Otherwise, the implementation is greatly improved :)
| @@ -100,7 +100,6 @@ function doSomething(this: void) {} | |||
|
|
|||
| ```ts | |||
| function printArg<T = void>(arg: T) {} | |||
| printArg<void>(undefined); | |||
There was a problem hiding this comment.
This is an incorrect case. so I remove it.
unvalley
force-pushed
the
fix-no-confusing-void-type
branch
from
c5f68cd to
983fb8a
Compare
chore: restore waste change
unvalley
force-pushed
the
fix-no-confusing-void-type
branch
from
983fb8a to
8ab3842
Compare
| JsSyntaxKind::TS_TYPE_ARGUMENT_LIST => { | ||
| // handle generic function that has a void type argument | ||
| // e.g. functionGeneric<void>(undefined); | ||
| let Some(grand_parent) = parent.grand_parent() else { | ||
| continue; | ||
| }; | ||
| if grand_parent.kind() == JsSyntaxKind::JS_CALL_EXPRESSION { | ||
| return Some(VoidTypeContext::Unknown); | ||
| } | ||
| return None; |
There was a problem hiding this comment.
We need to address the scenario where fn<void>(undefined).
I discovered this issue when I adde tests, so I addressed.
There was a problem hiding this comment.
I think this void in function's generic type may confuse developers because the void is useless to be generic.
Referring to typescript-eslint no-invalid-void-type, I think we should make this case invalid.
Previously we treated it as valid: https://github.com/biomejs/biome/pull/611/files#diff-519e64910a99b91258aea6d0bfeb4a64d44dc190c4c15b7f06d9dccc76ec605eL4
Summary
Closes #604
Fix
noConfusingVoidTypenoConfusingVoidType: allowPromise<void> | void#604Test Plan
cargo test -p biome_js_analyze no_confusing_void_typeExisting test plans should pass, and add a test case that filed on #604Update test cases by referencing typescript-eslint no-invalid-void-type test cases.