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 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
1 change: 1 addition & 0 deletions packages/eslint-plugin/README.md
Expand Up @@ -163,6 +163,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 | :white_check_mark: | :wrench: | :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 | | :wrench: | :thought_balloon: |
| [`@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
90 changes: 90 additions & 0 deletions packages/eslint-plugin/docs/rules/prefer-return-this-type.md
@@ -0,0 +1,90 @@
# 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;
};
f3(): Foo | undefined {
return Math.random() > 0.5 ? this : undefined;
}
}
```

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

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

class Base {}
class Derived extends Base {
f(): Base {
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 @@ -129,6 +129,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
2 changes: 2 additions & 0 deletions packages/eslint-plugin/src/rules/index.ts
Expand Up @@ -94,6 +94,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 @@ -212,6 +213,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
178 changes: 178 additions & 0 deletions packages/eslint-plugin/src/rules/prefer-return-this-type.ts
@@ -0,0 +1,178 @@
import {
TSESTree,
AST_NODE_TYPES,
} from '@typescript-eslint/experimental-utils';
import { createRule, forEachReturnStatement, getParserServices } from '../util';
import * as ts from 'typescript';

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: false,
requiresTypeChecking: true,
},
messages: {
useThisType: 'use `this` type instead.',
},
schema: [],
fixable: 'code',
},

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

function tryGetNameInType(
name: string,
typeNode: TSESTree.TypeNode,
): TSESTree.Identifier | undefined {
if (
typeNode.type === AST_NODE_TYPES.TSTypeReference &&
typeNode.typeName.type === AST_NODE_TYPES.Identifier &&
typeNode.typeName.name === name
) {
return typeNode.typeName;
}

if (typeNode.type === AST_NODE_TYPES.TSUnionType) {
for (const type of typeNode.types) {
const found = tryGetNameInType(name, type);
if (found) {
return found;
}
}
}

return undefined;
}

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

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

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

if (!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 hasReturnThis = false;
let hasReturnClassType = false;

forEachReturnStatement(func.body as ts.Block, stmt => {
const expr = stmt.expression;
if (!expr) {
return;
}

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

const type = checker.getTypeAtLocation(expr);
if (classType === type) {
hasReturnClassType = true;
return true;
}

if (classType.thisType === type) {
hasReturnThis = true;
return;
}

return;
});

return !hasReturnClassType && hasReturnThis;
}

function checkFunction(
originalFunc: FunctionLike,
originalClass: ClassLikeDeclaration,
): void {
const className = originalClass.id?.name;
if (!className) {
return;
}

if (!originalFunc.returnType) {
return;
}

const classNameRef = tryGetNameInType(
className,
originalFunc.returnType.typeAnnotation,
);
if (!classNameRef) {
return;
}

if (isFunctionReturningThis(originalFunc, originalClass)) {
context.report({
node: classNameRef,
messageId: 'useThisType',
fix(fixer) {
return fixer.replaceText(classNameRef, '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 './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;
}
}