-
-
Notifications
You must be signed in to change notification settings - Fork 792
fix(useExplicitType): rewrite top-level variables checking #5935
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
fix(useExplicitType): rewrite top-level variables checking #5935
Conversation
|
QQ: typescript-eslint allows untyped setters. Should we add an option to optionally prohibit untyped setters? That's IMO quite a counter-intuitive behavior and explicit hints help define the public interface much better. E.g. this passes cleanly: const x = {
get foo(): number { return 1; }
set foo(newFoo) { console.log(newFoo); }I believe that |
I believe we should enforce a type on setters too. |
ematipico
left a comment
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.
Some snapshots changed too much and needs to be addressed
crates/biome_js_analyze/tests/specs/nursery/useExplicitType/invalid.ts.snap
Show resolved
Hide resolved
...es/biome_js_analyze/tests/specs/nursery/useExplicitType/invalidDeclarationStatements.ts.snap
Show resolved
Hide resolved
crates/biome_js_analyze/tests/specs/nursery/useExplicitType/namespace.ts.snap
Show resolved
Hide resolved
CodSpeed Performance ReportMerging #5935 will not alter performanceComparing Summary
|
|
So many questions. I'm fine with checking setter types, but the rule seems still horribly broken... Currently the following is flagged as error: arr.map((item) => item + 1);
new Promise((resolve) => resolve(1));and that's even checked in tests. Usually such inline functions are left untyped; I expect a lot of projects using a line similar to the above without annotating |
| return None; | ||
| } | ||
|
|
||
| // TODO: why only arrow functions are ignored inside typed 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.
I'll point this out in a review comment as it really makes no sense to me. Why should namedFunc and arrowFunc be handled differently (namedFunc rejected, arrowFunc allowed) in the following case?
interface Behavior {
attribute: string;
namedFunc: () => string;
arrowFunc: () => string;
}
function getObjectWithFunction(): Behavior {
return {
namedFunc: function myFunc() { return "value" },
arrowFunc: () => {},
}
};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.
It's a good point, unfortunately, we miss some historical facts. Perhaps you could bring this up in the original issue and continue the discussion there.
|
I hope this is ready for another round. Does this update warrant a |
|
This rule was present before 2.0 right? If that is the case, then yes, it should have a changeset. |
ematipico
left a comment
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.
Thank you @sterliakov !
Summary
This PR significantly changes handling of top-level
const,letandvarvariables byuseExplicitTypenursery rule.Fixes #5932.
I went with the following heuristics:
letandvarbindings with literalnullorundefinedRHS as those don't usually have realnullorundefinedtype but are later reassigned to something more useful.x as SomeTypeand legacy-style<SomeType>xis also trivial.var,letorconstdeclaration of a variable without an explicit type annotation is flagged by this rule unless the RHS is trivial.Going down this rabbit hole,
The rules outlined above solve one of the existing pain points. The following code has one missing type, and yet current biome version reports two errors on it (for the variable and for the method).
Test Plan
I added several simple testcases and will expand that list as the PR grows.