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): [prefer-return-this-type] add a new rule #3228

Merged
merged 12 commits into from Jul 31, 2021
Merged
Show file tree
Hide file tree
Changes from 6 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
1 change: 1 addition & 0 deletions packages/eslint-plugin/README.md
Expand Up @@ -162,6 +162,7 @@ Pro Tip: For larger codebases you may want to consider splitting our linting int
| [`@typescript-eslint/prefer-readonly-parameter-types`](./docs/rules/prefer-readonly-parameter-types.md) | Requires that function parameters are typed as readonly to prevent accidental mutation of inputs | | | :thought_balloon: |
| [`@typescript-eslint/prefer-reduce-type-parameter`](./docs/rules/prefer-reduce-type-parameter.md) | Prefer using type parameter when calling `Array#reduce` instead of casting | | :wrench: | :thought_balloon: |
| [`@typescript-eslint/prefer-regexp-exec`](./docs/rules/prefer-regexp-exec.md) | Enforce that `RegExp#exec` is used instead of `String#match` if no global flag is provided | :heavy_check_mark: | | :thought_balloon: |
| [`@typescript-eslint/prefer-return-this-type`](./docs/rules/prefer-return-this-type.md) | Enforce that `this` is used when only `this` type is returned | :heavy_check_mark: | :wrench: | :thought_balloon: |
Copy link
Member

Choose a reason for hiding this comment

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

you need to regen the docs to remove the recommended tick

| [`@typescript-eslint/prefer-string-starts-ends-with`](./docs/rules/prefer-string-starts-ends-with.md) | Enforce the use of `String#startsWith` and `String#endsWith` instead of other equivalent methods of checking substrings | | :wrench: | :thought_balloon: |
| [`@typescript-eslint/prefer-ts-expect-error`](./docs/rules/prefer-ts-expect-error.md) | Recommends using `@ts-expect-error` over `@ts-ignore` | | :wrench: | |
| [`@typescript-eslint/promise-function-async`](./docs/rules/promise-function-async.md) | Requires any function or method that returns a Promise to be marked async | | :wrench: | :thought_balloon: |
Expand Down
83 changes: 83 additions & 0 deletions packages/eslint-plugin/docs/rules/prefer-return-this-type.md
@@ -0,0 +1,83 @@
# Enforce that `this` is used when only `this` type is returned (`prefer-return-this-type`)

[Method chaining](https://en.wikipedia.org/wiki/Method_chaining) is a common pattern in OOP languages and TypeScript provides a special [polymorphic this type](https://www.typescriptlang.org/docs/handbook/2/classes.html#this-types).
If any type other than `this` is specified as the return type of these chaining methods, TypeScript will fail to cast it when invoking in subclass.

```ts
class Animal {
eat(): Animal {
console.log("I'm moving!");
return this;
}
}

class Cat extends Animal {
meow(): Cat {
console.log('Meow~');
return this;
}
}

const cat = new Cat();
// Error: Property 'meow' does not exist on type 'Animal'.
// because `eat` returns `Animal` and not all animals meow.
cat.eat().meow();

// the error can be fixed by removing the return type of `eat` or use `this` as the return type.
class Animal {
eat(): this {
console.log("I'm moving!");
return this;
}
}

class Cat extends Animal {
meow(): this {
console.log('Meow~');
return this;
}
}

const cat = new Cat();
// no errors. Because `eat` returns `Cat` now
cat.eat().meow();
```

Examples of **incorrect** code for this rule:

```ts
class Foo {
f1(): Foo {
return this;
}
f2 = (): Foo => {
return this;
};
}
```

Examples of **correct** code for this rule:

```ts
class Foo {
f1(): this {
return this;
}
f2() {
return this;
}
f3(): Foo | undefined {
Copy link
Member

Choose a reason for hiding this comment

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

why not this | undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌

return Math.random() > 0.5 ? this : undefined;
}
f4 = (): this => {
return this;
};
f5 = () => {
return this;
};
}
```

## When Not To Use It

If you don't use method chaining or explicit return values, you can safely turn this rule off.
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/configs/all.ts
Expand Up @@ -128,6 +128,7 @@ export = {
'@typescript-eslint/prefer-readonly-parameter-types': 'error',
'@typescript-eslint/prefer-reduce-type-parameter': 'error',
'@typescript-eslint/prefer-regexp-exec': 'error',
'@typescript-eslint/prefer-return-this-type': 'error',
'@typescript-eslint/prefer-string-starts-ends-with': 'error',
'@typescript-eslint/prefer-ts-expect-error': 'error',
'@typescript-eslint/promise-function-async': 'error',
Expand Down
Expand Up @@ -17,6 +17,7 @@ export = {
'@typescript-eslint/no-unsafe-member-access': 'error',
'@typescript-eslint/no-unsafe-return': 'error',
'@typescript-eslint/prefer-regexp-exec': 'error',
'@typescript-eslint/prefer-return-this-type': 'error',
Copy link
Member

Choose a reason for hiding this comment

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

you need to regenerate the configs

'require-await': 'off',
'@typescript-eslint/require-await': 'error',
'@typescript-eslint/restrict-plus-operands': 'error',
Expand Down
2 changes: 2 additions & 0 deletions packages/eslint-plugin/src/rules/index.ts
Expand Up @@ -93,6 +93,7 @@ import preferReadonly from './prefer-readonly';
import preferReadonlyParameterTypes from './prefer-readonly-parameter-types';
import preferReduceTypeParameter from './prefer-reduce-type-parameter';
import preferRegexpExec from './prefer-regexp-exec';
import preferReturnThisType from './prefer-return-this-type';
import preferStringStartsEndsWith from './prefer-string-starts-ends-with';
import preferTsExpectError from './prefer-ts-expect-error';
import promiseFunctionAsync from './promise-function-async';
Expand Down Expand Up @@ -210,6 +211,7 @@ export default {
'prefer-readonly-parameter-types': preferReadonlyParameterTypes,
'prefer-reduce-type-parameter': preferReduceTypeParameter,
'prefer-regexp-exec': preferRegexpExec,
'prefer-return-this-type': preferReturnThisType,
'prefer-string-starts-ends-with': preferStringStartsEndsWith,
'prefer-ts-expect-error': preferTsExpectError,
'promise-function-async': promiseFunctionAsync,
Expand Down
161 changes: 161 additions & 0 deletions packages/eslint-plugin/src/rules/prefer-return-this-type.ts
@@ -0,0 +1,161 @@
import {
TSESTree,
AST_NODE_TYPES,
} from '@typescript-eslint/experimental-utils';
import { createRule, forEachReturnStatement, getParserServices } from '../util';
import * as ts from 'typescript';

const IgnoreTypes = new Set<TSESTree.TypeNode['type']>([
AST_NODE_TYPES.TSThisType,
AST_NODE_TYPES.TSAnyKeyword,
AST_NODE_TYPES.TSUnknownKeyword,
]);

type ClassLikeDeclaration =
| TSESTree.ClassDeclaration
| TSESTree.ClassExpression;

type FunctionLike =
| TSESTree.MethodDefinition['value']
| TSESTree.ArrowFunctionExpression;

export default createRule({
name: 'prefer-return-this-type',
defaultOptions: [],

meta: {
type: 'suggestion',
docs: {
description:
'Enforce that `this` is used when only `this` type is returned',
category: 'Best Practices',
recommended: 'error',
Zzzen marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

adding to the recommended set is a breaking change

Suggested change
recommended: 'error',
recommended: false,

requiresTypeChecking: true,
},
messages: {
UseThisType: 'use `this` type instead.',
Zzzen marked this conversation as resolved.
Show resolved Hide resolved
},
schema: [],
fixable: 'code',
},

create(context) {
const parserServices = getParserServices(context);
const checker = parserServices.program.getTypeChecker();

function isThisSpecifiedInParameters(originalFunc: FunctionLike): boolean {
const firstArg = originalFunc.params[0];
return (
firstArg &&
firstArg.type === AST_NODE_TYPES.Identifier &&
firstArg.name === 'this'
);
}

function isFunctionAlwaysReturningThis(
originalFunc: FunctionLike,
originalClass: ClassLikeDeclaration,
): boolean {
if (isThisSpecifiedInParameters(originalFunc)) {
return false;
}

const func = parserServices.esTreeNodeToTSNodeMap.get(originalFunc);

// two things to note here:
// 1. arrow function without brackets is flagged as HasImplicitReturn by ts.
// 2. ts.NodeFlags.HasImplicitReturn is not accurate. TypeScript compiler uses control flow
// analysis to determine if a function has implicit return.
const hasImplicitReturn =
func.flags & ts.NodeFlags.HasImplicitReturn &&
!(
func.kind === ts.SyntaxKind.ArrowFunction &&
func.body?.kind !== ts.SyntaxKind.Block
);

if (hasImplicitReturn || !func.body) {
return false;
}

const classType = checker.getTypeAtLocation(
parserServices.esTreeNodeToTSNodeMap.get(originalClass),
) as ts.InterfaceType;

if (func.body.kind !== ts.SyntaxKind.Block) {
const type = checker.getTypeAtLocation(func.body);
return classType.thisType === type;
}

let alwaysReturnsThis = true;
let hasReturnStatements = false;

forEachReturnStatement(func.body as ts.Block, stmt => {
hasReturnStatements = true;

const expr = stmt.expression;
if (!expr) {
alwaysReturnsThis = false;
return true;
}

// fast check
if (expr.kind === ts.SyntaxKind.ThisKeyword) {
return;
}

const type = checker.getTypeAtLocation(expr);
if (classType.thisType !== type) {
alwaysReturnsThis = false;
return true;
}

return undefined;
});

return hasReturnStatements && alwaysReturnsThis;
}

function checkFunction(
originalFunc: FunctionLike,
originalClass: ClassLikeDeclaration,
): void {
if (
!originalFunc.returnType ||
IgnoreTypes.has(originalFunc.returnType.typeAnnotation.type)
) {
return;
}

if (isFunctionAlwaysReturningThis(originalFunc, originalClass)) {
context.report({
node: originalFunc.returnType,
messageId: 'UseThisType',
fix(fixer) {
return fixer.replaceText(
originalFunc.returnType!.typeAnnotation,
'this',
);
},
});
}
}

return {
'ClassBody > MethodDefinition'(node: TSESTree.MethodDefinition): void {
checkFunction(node.value, node.parent!.parent as ClassLikeDeclaration);
},
'ClassBody > ClassProperty'(node: TSESTree.ClassProperty): void {
if (
!(
node.value?.type === AST_NODE_TYPES.FunctionExpression ||
node.value?.type === AST_NODE_TYPES.ArrowFunctionExpression
)
) {
return;
}

checkFunction(node.value, node.parent!.parent as ClassLikeDeclaration);
},
Comment on lines +161 to +175
Copy link
Member

Choose a reason for hiding this comment

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

one easier way to do this is to use a stack to track your state.
It saves you doing manual traversal of the tree

interface Stack { classLike: ClassLikeDeclaration, methodLike: FunctionLike | null }
const stack: Stack[] = [];
function getCurrentStack(): Stack | undefined {
  return stack[stack.length - 1];
}

return {
  'ClassDeclaration, ClassExpression:enter'(node: ClassLikeDeclaration) {
    stack.push({ classLike: node, methodLike: null });
  },
  'ClassDeclaration, ClassExpression:exit'(node: ClassLikeDeclaration) {
    stack.pop();
  },
  'ClassBody > MethodDefinition:enter, ClassBody > ClassProperty > :matches(FunctionExpression, ArrowFunctionExpression).value:enter'(node: FunctionLike ) {
    const current = getCurrentStack();
    if (current != null) {
      current.methodLike = node;
    }
  },
  'ClassBody > MethodDefinition, ClassBody > ClassProperty > :matches(FunctionExpression, ArrowFunctionExpression).value:exit'(node: FunctionLike ) {
    const current = getCurrentStack();
    if (current != null) {
      current.methodLike = null;
    }
  },
  ReturnStatement(node) {
    const current = getCurrentStack();
    if (current != null && current.methodLike != null) {
      // check return and report
    }
  },
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I haven't seen this pattern before, will try it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is more complicated than I have expected. We may have functions inside methods and need to track them too.

class Foo {
  bar() {
    const f = function() {
       return this;
    }
  }
}

};
},
});
36 changes: 36 additions & 0 deletions packages/eslint-plugin/src/util/astUtils.ts
@@ -1,5 +1,6 @@
import type { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils';
import escapeRegExp from 'lodash/escapeRegExp';
import * as ts from 'typescript';

// deeply re-export, for convenience
export * from '@typescript-eslint/experimental-utils/dist/ast-utils';
Expand Down Expand Up @@ -41,3 +42,38 @@ export function getNameLocationInGlobalDirectiveComment(

return { start, end };
}

// Copied from typescript https://github.com/microsoft/TypeScript/blob/42b0e3c4630c129ca39ce0df9fff5f0d1b4dd348/src/compiler/utilities.ts#L1335
// Warning: This has the same semantics as the forEach family of functions,
// in that traversal terminates in the event that 'visitor' supplies a truthy value.
export function forEachReturnStatement<T>(
body: ts.Block,
visitor: (stmt: ts.ReturnStatement) => T,
): T | undefined {
return traverse(body);

function traverse(node: ts.Node): T | undefined {
switch (node.kind) {
case ts.SyntaxKind.ReturnStatement:
return visitor(<ts.ReturnStatement>node);
case ts.SyntaxKind.CaseBlock:
case ts.SyntaxKind.Block:
case ts.SyntaxKind.IfStatement:
case ts.SyntaxKind.DoStatement:
case ts.SyntaxKind.WhileStatement:
case ts.SyntaxKind.ForStatement:
case ts.SyntaxKind.ForInStatement:
case ts.SyntaxKind.ForOfStatement:
case ts.SyntaxKind.WithStatement:
case ts.SyntaxKind.SwitchStatement:
case ts.SyntaxKind.CaseClause:
case ts.SyntaxKind.DefaultClause:
case ts.SyntaxKind.LabeledStatement:
case ts.SyntaxKind.TryStatement:
case ts.SyntaxKind.CatchClause:
return ts.forEachChild(node, traverse);
}

return undefined;
}
}