📎 Implement nursery/useUnifiedTypeSignature - typescript-eslint/unified-signatures

[Conaclos]

Description

unified-signatures

Want to contribute? Lets you know you are interested! We will assign you to the issue to prevent several people to work on the same issue. Don't worry, we can unassign you later if you are no longer interested in the issue! Read our contributing guide and analyzer contributing guide.

[ematipico]

I suggest useUnifiedTypeSignature

[ematipico]

Ping for @unvalley . Are you still interested?

[unvalley]

@ematipico Sorry for holding If anyone is interested, please work on it.

[n-gude]

I'd like to work on this.

I suggest useUnifiedTypeSignature

I would suggest useUnifiedOverloadSignature. I think that would describe it better, because it essentially unifies the function overload signatures than just the parameters type annotations.

[Conaclos]

All yours :)

useUnifiedOverloadSignature highlights Overload. However, a unified signature unifies overloads into a single signature?

[ematipico]

@n-gude are you still interested in this issue?

[n-gude]

Yes, I'm working on this. It's just more complex than I initially thought and I was very busy the last few weeks. But I'm nearly done.

@Conaclos Sorry, I completely forgot to respond.
Yes. The rule not only catches cases where only one parameter's type is different and can be replaced with a union type

-function foo(a: number, b: number): void;
-function foo(a: number, b: string): void;
+function foo(a: number, b: number | string);

but also catches cases like this:

-function bar(a: number, b: number, c?: number, d?: number): void;
-function bar(a: number, b?: number): void;
+function bar(a: number, b?: number, c?: number, d?: number): void;

Therefore I would prefer useUnifiedOverloadSignatures or useUnifiedSignatures.

Furthermore, I need your opinion on the following:

1. How should the parameter in the following case be named?

   -function f(foo: number, bar: number): void;
   -function f(foo: number, baz: string): void;
   +function foo(a: number, barOrBaz: number | string);

   Luckily JS and TS don't have named arguments and the parameter names of overload signatures aren't used in the function body, but just those of the implementation signature, so it's just a stylistic decision.

2. Should the first or second/last overload signature be replaced with the unified one (and the other removed)?
   If the two overload signatures are next to each other that wouldn't make a difference but if there is (for some reason) another function signature or anything else in between it does. Also just stylistic and based on personal preference, but I would like to hear your opinion. I would go for replacing the second/last one with the unified one.

I haven't added the ignoreDifferentlyNamedParameters option from the original rule, because I think that it defeats the purpose of this rule a little bit and according to Biome's philosophy we want to keep options low.

[Sec-ant]

Should the first or second/last overload signature be replaced with the unified one (and the other removed)?

I don't think we have much choice because a function implementation has to be immediately following the declaration.

Another thing to consider is how to deal with JSDoc/TSDoc or plain comments preceding those function signatures.

It looks like we have too many things to consider if we are to provide a fix action, maybe we shouldn't?

[n-gude]

Function implementation has to be immediately following the declaration.

Ah, yes. For normal function declarations and class methods, you are right, but interface methods could have others in between.

interface I {
  a(x: number): void;
  b(): void;
  a(x: string): void;
}

Another thing to consider is how to deal with JSDoc/TSDoc or plain comments preceding those function signatures.

Yeah, good question. Maybe combine/append them as well? I would make the code fix unsafe anaway because its a complex rule and you should review it.

It looks like we have too many things to consider if we are to provide a fix action, maybe we shouldn't?

That would be sad... That's what annoyed me about the original rule. The diagnostic is so vague sometimes that you don't know how it should be 'fixed'. How would you express the changes for this example in a diagnostic?:

-function bar(a: number, b: number, c?: number, d?: number): void;
-function bar(a: number, b?: number): void;
+function bar(a: number, b?: number, c?: number, d?: number): void;

[Conaclos]

Therefore I would prefer useUnifiedOverloadSignatures or useUnifiedSignatures.

Personally I prefer useUnifiedSignatures. I leave the choice up to you. I think you have gained more background by working on this rule.

Luckily JS and TS don't have named arguments and the parameter names of overload signatures aren't used in the function body, but just those of the implementation signature, so it's just a stylistic decision.

Yes. However, a parameter name could be cited in the documentation of the signature.
Otherwise, we could simply concatenate the names?

- function f(foo: number, bar: number): void;
- function f(foo: number, baz: string): void;
+ function foo(a: number, barBaz: number | string);

I haven't added the ignoreDifferentlyNamedParameters option from the original rule, because I think that it defeats the purpose of this rule a little bit and according to Biome's philosophy we want to keep options low.

+1
I think we don't need any options for this rule.

If the two overload signatures are next to each other that wouldn't make a difference but if there is (for some reason) another function signature or anything else in between it does. Also just stylistic and based on personal preference, but I would like to hear your opinion. I would go for replacing the second/last one with the unified one.

I could replace the last one (or the first one).
Anyway, that's certainly pretty uncommon.
We could even just handle adjacent signature and leave non-adjacent signatures reported by another rule such as adjacent-overload-signatures (A rule not yet implemented in Biome #2090).

If we are going to provide an unsafe fix, then I think we can just concatenate the parameter names (when they are different), and appends comments.

- /** A */
- function f(foo: number, bar: number): void;
- /** B */
- function f(foo: number, baz: string): void;
+ /** A */
+ /** B */
+ function foo(a: number, barBaz: number | string);

[Sec-ant]

I agree what you said above. I just want to add one more thing: for JSDoc/TSDoc comments, they may have parameters and types to describe the function signature, so renaming and retyping the parameters can invalidate these docs. It should be warned in the doc page of this rule.