Skip to content
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 #4647

Closed
wants to merge 2 commits into from

Conversation

kaykdm
Copy link
Contributor

@kaykdm kaykdm commented Nov 27, 2024

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.

@github-actions github-actions bot added A-CLI Area: CLI A-Project Area: project A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Nov 27, 2024
Copy link

codspeed-hq bot commented Nov 27, 2024

CodSpeed Performance Report

Merging #4647 will not alter performance

Comparing kaykdm:feature/explicit-argument-types (98064bc) with main (cd1c8ec)

Summary

✅ 97 untouched benchmarks


pub enum UseExplicitTypeState {
MissingReturnType(TextRange),
MissingArgumentnType(TextRange, String),
Copy link
Contributor Author

@kaykdm kaykdm Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to continue the discussion from this

Usually we try to avoid allocating a string. You could directly take the token:

@Conaclos

Thank you for the feedback! I decided to keep the same implementation for the following reasons:

  • Even if we use the token directly instead of passing a String, we still need to call the to_string() method to pass the value to markup! {}, which will allocate a string regardless. So, both approaches result in string allocation.
  • Additionally, in cases like function foo({a: b}), we need to handle tokens differently. For instance, the first token in this scenario would be '{'.

Comment on lines +317 to +319
| JsFormalParameter
| JsRestParameter
| TsThisParameter
Copy link
Contributor Author

@kaykdm kaykdm Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to continue the discussion from this

I wonder if directly matching against JsParameters is a good idea. I see two possible alternative approaches:

  • matching directly against JsFormalParameter, JsRestParameter, and TsThisParameter
  • matching against functions with parameters (We already have most of them, we need to add setter and constructors).

@Conaclos
Thank you for the suggestion! I decided to implement matching directly against JsFormalParameter, JsRestParameter, and TsThisParameter because it keeps the code simpler and more straightforward.

Copy link
Member

@Conaclos Conaclos Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with (1) is that you have to check again if the argument can be untyped (That corresponds to the same heuristics that allows untyped return type). I noticed you didn't implemnt these exceptions.

For insatnce, teh following code should be accepted:

const f: MyFunction = (a) => a;

Also, creating a diagnostic for every argument / return type seems noisy?

@kaykdm kaykdm marked this pull request as ready for review November 27, 2024 11:10
@ematipico
Copy link
Member

@kaykdm just checking in if this PR still needs a review or not?

@kaykdm
Copy link
Contributor Author

kaykdm commented Dec 14, 2024

@kaykdm just checking in if this PR still needs a review or not?

@ematipico @Conaclos
Sorry for the late response. I still need to work on this PR, but I haven't had much time lately. I'll close this PR for now. Thank you for checking in!

@kaykdm kaykdm closed this Dec 14, 2024
@kaykdm kaykdm deleted the feature/explicit-argument-types branch December 14, 2024 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI A-Linter Area: linter A-Project Area: project L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants