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): add prefer-regexp-exec rule #305

Merged
merged 25 commits into from May 10, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
504c701
feat(eslint-plugin): add rule prefer-regexp-exec
lippmannr Feb 20, 2019
f5ee042
Merge branch 'master' into prefer-regexp-exec
ldrick Feb 20, 2019
5ed44d2
test(eslint-plugin): add more tests
ldrick Feb 20, 2019
d7b6148
docs(eslint-plugin): add documentation for prefer-regexp-exec rule
ldrick Feb 21, 2019
99e14d7
test(eslint-plugin): improve coverage for prefer-regexp-exec rule
ldrick Feb 21, 2019
8f4a014
docs(eslint-plugin): improve docs for prefer-regexp-exec rule
ldrick Feb 22, 2019
9aa8f35
chore: merge master
ldrick Feb 23, 2019
7134535
fix(eslint-plugin): remove author
ldrick Feb 23, 2019
b6d7590
feat(eslint-plugin): add config all.json
ldrick Feb 24, 2019
f578be8
chore: merge master
ldrick Feb 25, 2019
e9b7914
Merge branch 'master' into prefer-regexp-exec
ldrick Mar 3, 2019
862672e
Merge branch 'master' into prefer-regexp-exec
ldrick Mar 10, 2019
47b0f12
Merge remote-tracking branch 'upstream/master' into prefer-regexp-exec
ldrick Mar 21, 2019
f7ddbc5
Merge branch 'master' of https://github.com/ldrick/typescript-eslint …
ldrick Apr 23, 2019
c8db8e0
Merge branch 'master' into prefer-regexp-exec
ldrick Apr 23, 2019
44007fa
Merge remote-tracking branch 'upstream/master' into prefer-regexp-exec
lippmannr Apr 24, 2019
f8aba10
Revert "feat(eslint-plugin): add config all.json"
ldrick Apr 24, 2019
5babaa3
feat(eslint-plugin): move getTypeName to util
ldrick Apr 25, 2019
b186cbf
Merge branch 'master' into prefer-regexp-exec
ldrick Apr 26, 2019
45dbcb8
Merge branch 'master' into prefer-regexp-exec
ldrick Apr 30, 2019
02e66be
Merge branch 'master' into prefer-regexp-exec
ldrick May 3, 2019
92e3ecf
Merge branch 'master' into prefer-regexp-exec
ldrick May 6, 2019
de05e3e
Merge branch 'master' into prefer-regexp-exec
bradzacher May 8, 2019
80dff66
Merge remote-tracking branch 'upstream/master' into prefer-regexp-exec
ldrick May 10, 2019
d234f35
Merge branch 'master' into prefer-regexp-exec
bradzacher May 10, 2019
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 @@ Then you should add `airbnb` (or `airbnb-base`) to your `extends` section of `.e
| [`@typescript-eslint/semi`](./docs/rules/semi.md) | Require or disallow semicolons instead of ASI | | :wrench: | |
| [`@typescript-eslint/type-annotation-spacing`](./docs/rules/type-annotation-spacing.md) | Require consistent spacing around type annotations (`typedef-whitespace` from TSLint) | :heavy_check_mark: | :wrench: | |
| [`@typescript-eslint/unbound-method`](./docs/rules/unbound-method.md) | Enforces unbound methods are called with their expected scope. (`no-unbound-method` from TSLint) | :heavy_check_mark: | | :thought_balloon: |
| [`@typescript-eslint/prefer-regexp-exec`](./docs/rules/prefer-regexp-exec.md) | Enforce to use `RegExp#exec` over `String#match` | | | :thought_balloon: |
| [`@typescript-eslint/unified-signatures`](./docs/rules/unified-signatures.md) | Warns for any two overloads that could be unified into one. (`unified-signatures` from TSLint) | | | |

<!-- end rule list -->
53 changes: 53 additions & 0 deletions packages/eslint-plugin/docs/rules/prefer-regexp-exec.md
@@ -0,0 +1,53 @@
# Enforce to use `RegExp#exec` over `String#match` (prefer-regexp-exec)

`RegExp#exec` is faster than `String#match` and both work the same when not using the `/g` flag.

## Rule Details

This rule is aimed at enforcing the more performant way of applying regular expressions on strings.

From [`String#match` on MDN](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/match):

> If the regular expression does not include the g flag, returns the same result as RegExp.exec().

From [Stack Overflow](https://stackoverflow.com/questions/9214754/what-is-the-difference-between-regexp-s-exec-function-and-string-s-match-fun)

> `RegExp.prototype.exec` is a lot faster than `String.prototype.match`, but that’s because they are not exactly the same thing, they are different.

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

```ts
'something'.match(/thing/);

'some things are just things'.match(/thing/);

const text = 'something';
const search = /thing/;
ldrick marked this conversation as resolved.
Show resolved Hide resolved
text.match(search);
```

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

```ts
/thing/.exec('something');

'some things are just things'.match(/thing/g);

const text = 'something';
const search = /thing/;
search.exec(text);
```

## Options

There are no options.

```json
{
"@typescript-eslint/prefer-regexp-exec": "error"
}
```

## When Not To Use It

If you prefer consistent use of `String#match` for both, with `g` flag and without it, you can turn this rule off.
66 changes: 66 additions & 0 deletions packages/eslint-plugin/src/rules/prefer-regexp-exec.ts
@@ -0,0 +1,66 @@
import { TSESTree } from '@typescript-eslint/typescript-estree';
import { createRule, getParserServices, getTypeName } from '../util';
import { getStaticValue } from 'eslint-utils';

export default createRule({
name: 'prefer-regexp-exec',
defaultOptions: [],

meta: {
type: 'suggestion',
docs: {
description:
'Prefer RegExp#exec() over String#match() if no global flag is provided.',
category: 'Best Practices',
recommended: false,
},
messages: {
regExpExecOverStringMatch: 'Use the `RegExp#exec()` method instead.',
},
schema: [],
},

create(context) {
const globalScope = context.getScope();
const service = getParserServices(context);
const typeChecker = service.program.getTypeChecker();

/**
* Check if a given node is a string.
* @param node The node to check.
*/
function isStringType(node: TSESTree.Node): boolean {
const objectType = typeChecker.getTypeAtLocation(
service.esTreeNodeToTSNodeMap.get(node),
);
return getTypeName(typeChecker, objectType) === 'string';
}

return {
"CallExpression[arguments.length=1] > MemberExpression.callee[property.name='match'][computed=false]"(
node: TSESTree.MemberExpression,
) {
const callNode = node.parent as TSESTree.CallExpression;
const arg = callNode.arguments[0];
const evaluated = getStaticValue(arg, globalScope);

// Don't report regular expressions with global flag.
if (
evaluated &&
evaluated.value instanceof RegExp &&
evaluated.value.flags.includes('g')
) {
return;
}

if (isStringType(node.object)) {
context.report({
node: callNode,
messageId: 'regExpExecOverStringMatch',
});
return;
}
},
};
},
});
57 changes: 4 additions & 53 deletions packages/eslint-plugin/src/rules/prefer-string-starts-ends-with.ts
Expand Up @@ -5,8 +5,7 @@ import {
getStaticValue,
} from 'eslint-utils';
import { RegExpParser, AST as RegExpAST } from 'regexpp';
import ts from 'typescript';
import { createRule, getParserServices } from '../util';
import { createRule, getParserServices, getTypeName } from '../util';

const EQ_OPERATORS = /^[=!]=/;
const regexpp = new RegExpParser();
Expand Down Expand Up @@ -35,65 +34,17 @@ export default createRule({
const globalScope = context.getScope();
const sourceCode = context.getSourceCode();
const service = getParserServices(context);
const types = service.program.getTypeChecker();

/**
* Get the type name of a given type.
* @param type The type to get.
*/
function getTypeName(type: ts.Type): string {
// It handles `string` and string literal types as string.
if ((type.flags & ts.TypeFlags.StringLike) !== 0) {
return 'string';
}

// If the type is a type parameter which extends primitive string types,
// but it was not recognized as a string like. So check the constraint
// type of the type parameter.
if ((type.flags & ts.TypeFlags.TypeParameter) !== 0) {
// `type.getConstraint()` method doesn't return the constraint type of
// the type parameter for some reason. So this gets the constraint type
// via AST.
const node = type.symbol.declarations[0] as ts.TypeParameterDeclaration;
if (node.constraint != null) {
return getTypeName(types.getTypeFromTypeNode(node.constraint));
}
}

// If the type is a union and all types in the union are string like,
// return `string`. For example:
// - `"a" | "b"` is string.
// - `string | string[]` is not string.
if (
type.isUnion() &&
type.types.map(getTypeName).every(t => t === 'string')
) {
return 'string';
}

// If the type is an intersection and a type in the intersection is string
// like, return `string`. For example: `string & {__htmlEscaped: void}`
if (
type.isIntersection() &&
type.types.map(getTypeName).some(t => t === 'string')
) {
return 'string';
}

return types.typeToString(type);
}
const typeChecker = service.program.getTypeChecker();

/**
* Check if a given node is a string.
* @param node The node to check.
*/
function isStringType(node: TSESTree.Node): boolean {
const objectType = types.getTypeAtLocation(
const objectType = typeChecker.getTypeAtLocation(
service.esTreeNodeToTSNodeMap.get(node),
);
const typeName = getTypeName(objectType);

return typeName === 'string';
return getTypeName(typeChecker, objectType) === 'string';
}

/**
Expand Down
57 changes: 57 additions & 0 deletions packages/eslint-plugin/src/util/types.ts
Expand Up @@ -41,6 +41,63 @@ export function containsTypeByName(
);
}

/**
* Get the type name of a given type.
* @param typeChecker The context sensitive TypeScript TypeChecker.
* @param type The type to get the name of.
*/
export function getTypeName(
typeChecker: ts.TypeChecker,
type: ts.Type,
): string {
// It handles `string` and string literal types as string.
if ((type.flags & ts.TypeFlags.StringLike) !== 0) {
return 'string';
}

// If the type is a type parameter which extends primitive string types,
// but it was not recognized as a string like. So check the constraint
// type of the type parameter.
if ((type.flags & ts.TypeFlags.TypeParameter) !== 0) {
// `type.getConstraint()` method doesn't return the constraint type of
// the type parameter for some reason. So this gets the constraint type
// via AST.
const node = type.symbol.declarations[0] as ts.TypeParameterDeclaration;
if (node.constraint != null) {
return getTypeName(
typeChecker,
typeChecker.getTypeFromTypeNode(node.constraint),
);
}
}

// If the type is a union and all types in the union are string like,
// return `string`. For example:
// - `"a" | "b"` is string.
// - `string | string[]` is not string.
if (
type.isUnion() &&
type.types
.map(value => getTypeName(typeChecker, value))
.every(t => t === 'string')
) {
return 'string';
}

// If the type is an intersection and a type in the intersection is string
// like, return `string`. For example: `string & {__htmlEscaped: void}`
if (
type.isIntersection() &&
type.types
.map(value => getTypeName(typeChecker, value))
.some(t => t === 'string')
) {
return 'string';
}

return typeChecker.typeToString(type);
}

/**
* Resolves the given node's type. Will resolve to the type's generic constraint, if it has one.
*/
Expand Down