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): new rule method-signature-style #1685

Merged
merged 10 commits into from Apr 3, 2020
24 changes: 13 additions & 11 deletions .cspell.json
Expand Up @@ -34,45 +34,47 @@
"\\(#.+?\\)"
],
"words": [
"ASTs",
"Airbnb",
"Airbnb's",
"Codecov",
"Crockford",
"errored",
"IDE's",
"IIFE",
"IIFEs",
"OOM",
"OOMs",
"Premade",
"ROADMAP",
"ASTs",
"autofix",
"autofixers",
"backticks",
"bigint",
"bivariant",
"blockless",
"codebases",
"Codecov",
"contravariant",
"Crockford",
"declarators",
"destructure",
"destructured",
"errored",
"erroring",
"ESLint",
"ESLint's",
"espree",
"estree",
"IDE's",
"IIFE",
"IIFEs",
"linebreaks",
"necroing",
"nocheck",
"nullish",
"OOM",
"OOMs",
"parameterised",
"performant",
"pluggable",
"postprocess",
"Premade",
"prettier's",
"recurse",
"reimplement",
"resync",
"ROADMAP",
"ruleset",
"rulesets",
"superset",
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/README.md
Expand Up @@ -106,6 +106,7 @@ Pro Tip: For larger codebases you may want to consider splitting our linting int
| [`@typescript-eslint/explicit-module-boundary-types`](./docs/rules/explicit-module-boundary-types.md) | Require explicit return and argument types on exported functions' and classes' public class methods | | | |
| [`@typescript-eslint/member-delimiter-style`](./docs/rules/member-delimiter-style.md) | Require a specific member delimiter style for interfaces and type literals | :heavy_check_mark: | :wrench: | |
| [`@typescript-eslint/member-ordering`](./docs/rules/member-ordering.md) | Require a consistent member declaration order | | | |
| [`@typescript-eslint/method-signature-style`](./docs/rules/method-signature-style.md) | Enforces using a particular method signature syntax. | | :wrench: | |
| [`@typescript-eslint/naming-convention`](./docs/rules/naming-convention.md) | Enforces naming conventions for everything across a codebase | | | :thought_balloon: |
| [`@typescript-eslint/no-base-to-string`](./docs/rules/no-base-to-string.md) | Requires that `.toString()` is only called on objects which provide useful information when stringified | | | :thought_balloon: |
| [`@typescript-eslint/no-dynamic-delete`](./docs/rules/no-dynamic-delete.md) | Disallow the delete operator with computed key expressions | | :wrench: | |
Expand Down
83 changes: 83 additions & 0 deletions packages/eslint-plugin/docs/rules/method-signature-style.md
@@ -0,0 +1,83 @@
# Enforces using a particular method signature syntax. (`method-signature-style`)

There are two ways to define an object/interface function property.

```ts
// method shorthand syntax
interface T1 {
func(arg: string): number;
}

// regular property with function type
interface T2 {
func: (arg: string) => number;
}
```

A good practice is to use the TypeScript's `strict` option (which implies `strictFunctionTypes`) which enables correct typechecking for function properties only (method signatures get old behavior).

TypeScript FAQ:

> A method and a function property of the same type behave differently.
> Methods are always bivariant in their argument, while function properties are contravariant in their argument under `strictFunctionTypes`.

See the reasoning behind that in the [TypeScript PR for the compiler option](https://github.com/microsoft/TypeScript/pull/18654).

## Options

This rule accepts one string option:

- `"property"`: Enforce using property signature for functions. Use this to enforce maximum correctness together with TypeScript's strict mode.
- `"method"`: Enforce using method signature for functions. Use this if you aren't using TypeScript's strict mode and prefer this style.

The default is `"property"`.

## Rule Details

Examples of **incorrect** code with `property` option.

```ts
interface T1 {
func(arg: string): number;
}
type T2 = {
func(arg: boolean): void;
};
```

Examples of **correct** code with `property` option.

```ts
interface T1 {
func: (arg: string) => number;
}
type T2 = {
func: (arg: boolean) => void;
};
```

Examples of **incorrect** code with `method` option.

```ts
interface T1 {
func: (arg: string) => number;
}
type T2 = {
func: (arg: boolean) => void;
};
```

Examples of **correct** code with `method` option.

```ts
interface T1 {
func(arg: string): number;
}
type T2 = {
func(arg: boolean): void;
};
```

## When Not To Use It

If you don't want to enforce a particular style for object/interface function types, and/or if you don't use `strictFunctionTypes`, then you don't need this rule.
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/configs/all.json
Expand Up @@ -23,6 +23,7 @@
"@typescript-eslint/indent": "error",
"@typescript-eslint/member-delimiter-style": "error",
"@typescript-eslint/member-ordering": "error",
"@typescript-eslint/method-signature-style": "error",
"@typescript-eslint/naming-convention": "error",
"no-array-constructor": "off",
"@typescript-eslint/no-array-constructor": "error",
Expand Down
6 changes: 4 additions & 2 deletions packages/eslint-plugin/src/rules/index.ts
Expand Up @@ -21,6 +21,7 @@ import interfaceNamePrefix from './interface-name-prefix';
import memberDelimiterStyle from './member-delimiter-style';
import memberNaming from './member-naming';
import memberOrdering from './member-ordering';
import methodSignatureStyle from './method-signature-style';
import namingConvention from './naming-convention';
import noArrayConstructor from './no-array-constructor';
import noBaseToString from './no-base-to-string';
Expand All @@ -29,10 +30,10 @@ import noDynamicDelete from './no-dynamic-delete';
import noEmptyFunction from './no-empty-function';
import noEmptyInterface from './no-empty-interface';
import noExplicitAny from './no-explicit-any';
import noExtraneousClass from './no-extraneous-class';
import noExtraNonNullAssertion from './no-extra-non-null-assertion';
import noExtraParens from './no-extra-parens';
import noExtraSemi from './no-extra-semi';
import noExtraneousClass from './no-extraneous-class';
import noFloatingPromises from './no-floating-promises';
import noForInArray from './no-for-in-array';
import noImpliedEval from './no-implied-eval';
Expand Down Expand Up @@ -98,7 +99,6 @@ export default {
'ban-ts-comment': banTsComment,
'ban-ts-ignore': banTsIgnore,
'ban-types': banTypes,
'no-base-to-string': noBaseToString,
'brace-style': braceStyle,
camelcase: camelcase,
'class-name-casing': classNameCasing,
Expand All @@ -116,8 +116,10 @@ export default {
'member-delimiter-style': memberDelimiterStyle,
'member-naming': memberNaming,
'member-ordering': memberOrdering,
'method-signature-style': methodSignatureStyle,
'naming-convention': namingConvention,
'no-array-constructor': noArrayConstructor,
'no-base-to-string': noBaseToString,
'no-dupe-class-members': noDupeClassMembers,
'no-dynamic-delete': noDynamicDelete,
'no-empty-function': noEmptyFunction,
Expand Down
118 changes: 118 additions & 0 deletions packages/eslint-plugin/src/rules/method-signature-style.ts
@@ -0,0 +1,118 @@
import {
AST_NODE_TYPES,
TSESTree,
} from '@typescript-eslint/experimental-utils';
import * as util from '../util';

export type Options = ['property' | 'method'];

export type MessageId = 'errorMethod' | 'errorProperty';

export default util.createRule<Options, MessageId>({
name: 'method-signature-style',
meta: {
type: 'suggestion',
docs: {
description: 'Enforces using a particular method signature syntax.',
category: 'Best Practices',
recommended: false,
},
fixable: 'code',
messages: {
errorMethod:
'Shorthand method signature is forbidden. Use a function property instead.',
errorProperty:
'Function property signature is forbidden. Use a method shorthand instead.',
},
schema: [
{
enum: ['property', 'method'],
},
],
},
defaultOptions: ['property'],

create(context, [mode]) {
const sourceCode = context.getSourceCode();

function getMethodKey(
node: TSESTree.TSMethodSignature | TSESTree.TSPropertySignature,
): string {
let key = sourceCode.getText(node.key);
if (node.computed) {
key = `[${key}]`;
}
if (node.optional) {
key = `${key}?`;
}
if (node.readonly) {
key = `readonly ${key}`;
}
return key;
}

function getMethodParams(
node: TSESTree.TSMethodSignature | TSESTree.TSFunctionType,
): string {
let params = node.params.map(node => sourceCode.getText(node)).join(', ');
params = `(${params})`;
if (node.typeParameters != null) {
const typeParams = sourceCode.getText(node.typeParameters);
params = `${typeParams}${params}`;
}
return params;
}
Copy link
Member

Choose a reason for hiding this comment

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

This will remove all comments inside the type.
For the most part I don't have problems with a fixer removing comments in weird places if it makes the fixer simpler and easier to maintain.

But one place we should accept and support comments is around parameters, eg:

method(/* leading comment */arg: string): void

So I would suggest converting this to something like:

  • grab first open paren token
  • grab last end paren token
  • grab source code between the two

That way we preserve the "expected" comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just did that, but it only works if there's at least one argument. If the argument list is (/* comment */) then the comment is lost. The problem is that there's no way to find the paren tokens because the method node's "params" property is just an empty array. This is contrary to "typeParams" which is a separate AST node so I can just getText it even if it's empty.


function getMethodReturnType(
node: TSESTree.TSMethodSignature | TSESTree.TSFunctionType,
): string {
return sourceCode.getText(node.returnType!.typeAnnotation);
}

return {
TSMethodSignature(methodNode): void {
if (mode === 'method') {
return;
}

context.report({
node: methodNode,
messageId: 'errorMethod',
fix: fixer => {
const key = getMethodKey(methodNode);
const params = getMethodParams(methodNode);
const returnType = getMethodReturnType(methodNode);
return fixer.replaceText(
methodNode,
`${key}: ${params} => ${returnType}`,
);
},
});
},
TSPropertySignature(propertyNode): void {
const typeNode = propertyNode.typeAnnotation?.typeAnnotation;
if (typeNode?.type !== AST_NODE_TYPES.TSFunctionType) {
return;
}

if (mode === 'property') {
return;
}

context.report({
node: propertyNode,
messageId: 'errorProperty',
fix: fixer => {
const key = getMethodKey(propertyNode);
const params = getMethodParams(typeNode);
const returnType = getMethodReturnType(typeNode);
return fixer.replaceText(
propertyNode,
`${key}${params}: ${returnType}`,
);
},
});
},
};
},
});