-
-
Notifications
You must be signed in to change notification settings - Fork 485
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
feat(useExplicitType): support explicit function argument types #4463
base: main
Are you sure you want to change the base?
Conversation
45fa76f
to
925a373
Compare
CodSpeed Performance ReportMerging #4463 will degrade performances by 9.81%Comparing Summary
Benchmarks breakdown
|
} | ||
} | ||
|
||
declare_node_union! { | ||
pub AnyJsFunctionWithReturnType = AnyJsFunction | JsMethodClassMember | JsMethodObjectMember | JsGetterClassMember | JsGetterObjectMember | ||
pub AnyJsFunctionWithReturnTypeOrJsParameters = AnyJsFunction | JsMethodClassMember | JsMethodObjectMember | JsGetterClassMember | JsGetterObjectMember | JsParameters |
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 just noticed that some types are missing for checking if the return type is explicitly set:
TsMethodSignatureTypeMember
TsCallSignatureTypeMember
TsMethodSignatureClassMember
TsSetterSignatureClassMember
TsDeclareFunctionDeclaration
TsDeclareFunctionExportDefaultDeclaration
I wonder if directly matching against JsParameters
is a good idea. I see two possible alternative approaches:
- matching directly against
JsFormalParameter
,JsRestParameter
, andTsThisParameter
- matching against functions with parameters (We already have most of them, we need to add setter and constructors).
Personally I could choose (2) because this allows us to issue a single diagnostic when a parameter or a return type is absent on a function.
Also, similarly to return type, we should ignore callbacks.
Besides, we should think to a better name for this union because we will have to add properties and top-level variables. Maybe AnyJsTypeableEntity
? Have you a better suggestion?
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.
Thanks for the review, and apologies for the late response.
I've just noticed that some types are missing for checking if the return type is explicitly set:
I understand. I'll create a separate for this this.
I wonder if directly matching against JsParameters is a good idea. I see two possible alternative approaches:
I see your point. I'll revisit this after implementing the missing return types.
Maybe AnyJsTypeableEntity? Do you have a better suggestion?
AnyJsTypeableEntity looks good to me.
For now, I will make this PR draft.
|
||
enum UseExplicitTypeCause { | ||
MissingReturnType, | ||
MissingArgumentnType(String), |
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.
Usually we try to avoid allocating a string. You could directly take the token:
MissingArgumentnType(String), | |
MissingArgumentnType(JsSyntaxToken), |
In this case we could rewrite UseExplicitTypeState
as:
pub enum UseExplicitTypeState {
MissingReturnType(TextRange),
MissingArgumentnType(JsSyntaxToken),
}
Because we can get the range from a token.
for p in parameters.items() { | ||
let param = p.ok()?; |
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.
We could avoid returning as soon as we met an error by iterating only over node without errors:
for p in parameters.items() { | |
let param = p.ok()?; | |
for p in parameters.items().into_iter().flatten() { |
fn check_function_parameters_type(parameters: &JsParameters) -> Option<UseExplicitTypeState> { | ||
for p in parameters.items() { | ||
let param = p.ok()?; | ||
let formal_param = param.as_any_js_formal_parameter()?; |
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.
We should certainly handle the other parameter kinds (rest parameter and this
parameter).
Summary
related: #2017
This PR adds support for enforcing explicit type annotations on arguments in all functions and class methods. The rule is inspired by the explicit-module-boundary-types rule, but it expands coverage beyond exported functions to include all functions and methods within a class.