feat(biome_js_analyzer): useAdjacentOverloadSignatures

[chansuke]

Summary

Fixes: #2090

The motivation behind this change is to address the issue of non-adjacent overload signatures, as exemplified below:

interface Foo {
  foo_interface(s: string): void;
  foo_interface(n: number): void;
  bar_interface(): void;
  foo_interface(sn: string | number): void; // Should be adjacent
}

Inspired by the example provided in typescript-eslint, I undertook the task of examining each type of signature. To accomplish this, I utilized the JsModuleItemList for querying, as illustrated here.

The process involved collecting methods through a pattern match.

In the implementation, I opted not to use AnyJsStatement::TsDeclareStatement due to the necessity of handling export functions. Although AnyJsModuleItem::JsExport was used, it inadvertently captured export functions within declare namespace Foo {}.

declare namespace Foo {
  export function foo_declare(s: string): void;
  export function bar_declare(): void;
  export function foo_declare(n: number): void;
  export function foo_declare(sn: string | number): void;
}

export function foo(s: string): void;
export function foo(n: number): void;
export function bar(): void;
export function foo(sn: string | number): void;

To avoid duplicate diagnostics, I devised a solution.

The core logic for detecting non-adjacent signatures is implemented here.

This involves iterating through groups (e.g., interface, type, class) and examining the positions.

Test Plan

• Verified snapshot tests to ensure that existing functionalities are unaffected by the changes.

• Tested the behavior of the rule using the typescript-eslint playground, cross-referencing the results with our logic. Confirmed that diagnostics are correctly positioned, as demonstrated below:

class B {
  fooB(s: string): void; // biome diagnostic
  barB(): void { };
  fooB(n: number): void; // typescript-eslint diagnostic
  fooB(sn: string | number): void { };
}

• Ensured that the rule accurately identifies non-adjacent overload signatures and generates diagnostics in accordance with the expected outcomes observed in the typescript-eslint playground.

[codspeed-hq]

CodSpeed Performance Report

Merging #2508 will not alter performance

_{Comparing chansuke:feature/adjacent-overload-signatures (4c6d328) with main (26f6282)}

Summary

✅ 92 untouched benchmarks

[ematipico]

@chansuke it seems your local main branch is out of sync with upstream, you added a website/ file that we removed some time ago. Would you mind fixing that?

[chansuke]

@ematipico
Oh,I will fix that soon.

[ematipico]

@chansuke, This rule is very complex, so we would appreciate it if, in the description, you explain to us how the logic of your code works, so we can make a better review of the logic.

Also, would you mind filling up the Test plan header of the template?

[ematipico]

I can't really review without a technical explanation of the logic. For now, I just did a small review around docs.

[Conaclos]

Thanks for taking this hard issue! I left suggestions :)

[Conaclos]

We could flatten to a single array and add a type identifier. This identifier could be an integer we increase every time we visit a new module item. This will avoid many allocations.

Also, we could use u32 instead of usize.

Suggested change

	let mut methods: Vec<Vec<(SyntaxNodeText, usize, TextRange)>> = Vec::new();
	let mut methods: Vec<(u32, SyntaxNodeText, u32, TextRange)> = Vec::new();

I was even thinking about a deeper refactor:

Currently, there are two phases: a first phase collects all method/function signatures and a second phase searches for non-adjacent overloads. This results in many allocations and to two traversals (a first traversal of the list of module items and a second traversal of the list of signatures).

We could have a single phase where we do it all at once.
A rustc_hash::FxHashSet<TokenText> could keep track of already seen methods.
For every method that has not the same name as the previous method, we verify whether the method name was already registered. If it was previously registered we found a non-adjacent overload that we can add to the Vec (Vec returned as state). If the method was not seen we add it in the set.
Every time we visit another type/interface/... we clear the set. This allows us to reuse the already allocated set.

Note: This is a suggestion. We could make this improvement in another PR/in the future.

[chansuke]

@Conaclos
I've checked rustc_hash::FxHashSet<TokenText> and sounds nice. I'd like to make this improvement in an another PR.

[Conaclos]

Thanks @chansuke! I will create a new issue for the potential improvements of the rule.