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(eslint-plugin): [unified-signatures] add ignoreDifferentlyNamedParameters option #4659

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
46 changes: 45 additions & 1 deletion packages/eslint-plugin/docs/rules/unified-signatures.md
Expand Up @@ -6,7 +6,23 @@ Warns for any two overloads that could be unified into one by using a union or a

This rule aims to keep the source code as maintainable as possible by reducing the amount of overloads.

Examples of code for this rule:
## Options

```ts
type Options = {
ignoreDifferentlyNamedParameters?: boolean;
};

const defaultOptions: Options = {
ignoreDifferentlyNamedParameters: false,
};
```

The rule accepts an options object with the following property:

- `ignoreDifferentlyNamedParameters`: whether two parameters with different names at the same index should be considered different even if their types are the same.

Examples of code for this rule with the default options:

<!--tabs-->

Expand All @@ -32,6 +48,34 @@ function x(x: number | string): void;
function y(...x: number[]): void;
```

Examples of code for this rule with `ignoreDifferentlyNamedParameters`:

<!--tabs-->

### ❌ Incorrect

```ts
function f(a: number): void;
function f(a: string): void;
```

```ts
function f(...a: number[]): void;
function f(...b: string[]): void;
```

### ✅ Correct

```ts
function f(a: number): void;
function f(b: string): void;
```

```ts
function f(...a: number[]): void;
function f(...a: string[]): void;
```

## Related To

- TSLint: [`unified-signatures`](https://palantir.github.io/tslint/rules/unified-signatures/)
Expand Down
84 changes: 59 additions & 25 deletions packages/eslint-plugin/src/rules/unified-signatures.ts
Expand Up @@ -48,7 +48,18 @@ type MethodDefinition =
| TSESTree.MethodDefinition
| TSESTree.TSAbstractMethodDefinition;

export default util.createRule({
type MessageIds =
| 'omittingRestParameter'
| 'omittingSingleParameter'
| 'singleParameterDifference';

type Options = [
{
ignoreDifferentlyNamedParameters?: boolean;
},
];

export default util.createRule<Options, MessageIds>({
name: 'unified-signatures',
meta: {
docs: {
Expand All @@ -65,10 +76,24 @@ export default util.createRule({
singleParameterDifference:
'{{failureStringStart}} taking `{{type1}} | {{type2}}`.',
},
schema: [],
schema: [
{
additionalProperties: false,
properties: {
ignoreDifferentlyNamedParameters: {
type: 'boolean',
},
},
type: 'object',
},
],
},
defaultOptions: [],
create(context) {
defaultOptions: [
{
ignoreDifferentlyNamedParameters: false,
},
],
create(context, [{ ignoreDifferentlyNamedParameters }]) {
const sourceCode = context.getSourceCode();

//----------------------------------------------------------------------
Expand Down Expand Up @@ -140,35 +165,19 @@ export default util.createRule({
const result: Failure[] = [];
const isTypeParameter = getIsTypeParameter(typeParameters);
for (const overloads of signatures) {
if (overloads.length === 2) {
const signature0 =
(overloads[0] as MethodDefinition).value || overloads[0];
const signature1 =
(overloads[1] as MethodDefinition).value || overloads[1];
forEachPair(overloads, (a, b) => {
const signature0 = (a as MethodDefinition).value ?? a;
const signature1 = (b as MethodDefinition).value ?? b;

const unify = compareSignatures(
signature0,
signature1,
isTypeParameter,
);
if (unify !== undefined) {
result.push({ unify, only2: true });
result.push({ unify, only2: overloads.length === 2 });
}
} else {
forEachPair(overloads, (a, b) => {
const signature0 = (a as MethodDefinition).value || a;
const signature1 = (b as MethodDefinition).value || b;

const unify = compareSignatures(
signature0,
signature1,
isTypeParameter,
);
if (unify !== undefined) {
result.push({ unify, only2: false });
}
});
}
});
}
return result;
}
Expand Down Expand Up @@ -199,6 +208,21 @@ export default util.createRule({
const bTypeParams =
b.typeParameters !== undefined ? b.typeParameters.params : undefined;

if (
ignoreDifferentlyNamedParameters &&
a.params.length === b.params.length
) {
for (let i = 0; i < a.params.length; i += 1) {
if (
a.params[i].type === b.params[i].type &&
getStaticParameterName(a.params[i]) !==
getStaticParameterName(b.params[i])
) {
return false;
}
}
}

return (
typesAreEqual(a.returnType, b.returnType) &&
// Must take the same type parameters.
Expand Down Expand Up @@ -591,6 +615,16 @@ function getOverloadInfo(node: OverloadNode): string {
}
}

function getStaticParameterName(param: TSESTree.Node): string | undefined {
switch (param.type) {
case AST_NODE_TYPES.Identifier:
return param.name;
case AST_NODE_TYPES.RestElement:
return getStaticParameterName(param.argument);
default:
Copy link
Member

Choose a reason for hiding this comment

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

quick note that these is valid:

function foo({a}: {a: string}): void;
function foo([a]: [a: string]): void;

I think it's fine for us to ignore these cases as they're rare and pretty dumb.

return undefined;
}
}
function isIdentifier(node: TSESTree.Node): node is TSESTree.Identifier {
return node.type === AST_NODE_TYPES.Identifier;
}
95 changes: 95 additions & 0 deletions packages/eslint-plugin/tests/rules/unified-signatures.test.ts
Expand Up @@ -159,10 +159,60 @@ async function rest(...args: any[], y?: string): Promise<number[] | string> {
return y || args;
}
`,
{
code: `
function f(a: number): void;
function f(b: string): void;
function f(a: number | string): void {}
`,
options: [{ ignoreDifferentlyNamedParameters: true }],
},
{
code: `
function f(a: boolean, ...c: number[]): void;
function f(a: boolean, ...d: string[]): void;
function f(a: boolean, ...c: (number | string)[]): void {}
`,
options: [{ ignoreDifferentlyNamedParameters: true }],
},
{
code: `
class C {
constructor();
constructor(a: number, b: number);
constructor(c?: number, b?: number) {}

a(): void;
a(a: number, b: number): void;
a(a?: number, d?: number): void {}
}
`,
options: [{ ignoreDifferentlyNamedParameters: true }],
},
],
invalid: [
{
code: `
function f(a: number): void;
function f(b: string): void;
function f(a: number | string): void {}
`,
errors: [
{
messageId: 'singleParameterDifference',
data: {
failureStringStart:
'These overloads can be combined into one signature',
type1: 'number',
type2: 'string',
},
line: 3,
column: 12,
},
],
},
{
code: `
function f(x: number): void;
function f(x: string): void;
function f(x: any): any {
Expand All @@ -185,6 +235,29 @@ function f(x: any): any {
},
{
code: `
function f(x: number): void;
function f(x: string): void;
function f(x: any): any {
return x;
}
`,
errors: [
{
messageId: 'singleParameterDifference',
data: {
failureStringStart:
'These overloads can be combined into one signature',
type1: 'number',
type2: 'string',
},
line: 3,
column: 12,
},
],
options: [{ ignoreDifferentlyNamedParameters: true }],
},
{
code: `
function opt(xs?: number[]): void;
function opt(xs: number[], y: string): void;
function opt(...args: any[]) {}
Expand Down Expand Up @@ -222,6 +295,28 @@ interface I {
},
],
},
{
// For 3 or more overloads, mentions the line.
code: `
interface I {
a0(): void;
a0(x: string): string;
a0(x: number): void;
}
`,
errors: [
{
messageId: 'omittingSingleParameter',
data: {
failureStringStart:
'This overload and the one on line 3 can be combined into one signature',
},
line: 5,
column: 6,
},
],
options: [{ ignoreDifferentlyNamedParameters: true }],
},
{
// Error for extra parameter.
code: `
Expand Down