From f61d421c968717b2bc55cf7971c05a144b28f715 Mon Sep 17 00:00:00 2001 From: Ricky Lippmann <3674067+ldrick@users.noreply.github.com> Date: Sat, 11 May 2019 01:55:49 +0200 Subject: [PATCH] feat(eslint-plugin): add prefer-regexp-exec rule (#305) --- packages/eslint-plugin/README.md | 1 + .../docs/rules/prefer-regexp-exec.md | 53 ++++++ .../src/rules/prefer-regexp-exec.ts | 66 +++++++ .../rules/prefer-string-starts-ends-with.ts | 57 +----- packages/eslint-plugin/src/util/types.ts | 57 ++++++ .../tests/rules/prefer-regexp-exec.test.ts | 180 ++++++++++++++++++ 6 files changed, 361 insertions(+), 53 deletions(-) create mode 100644 packages/eslint-plugin/docs/rules/prefer-regexp-exec.md create mode 100644 packages/eslint-plugin/src/rules/prefer-regexp-exec.ts create mode 100644 packages/eslint-plugin/tests/rules/prefer-regexp-exec.test.ts diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index b296291f4ae..22afc3659ac 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -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) | | | | diff --git a/packages/eslint-plugin/docs/rules/prefer-regexp-exec.md b/packages/eslint-plugin/docs/rules/prefer-regexp-exec.md new file mode 100644 index 00000000000..282907851b8 --- /dev/null +++ b/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/; +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. diff --git a/packages/eslint-plugin/src/rules/prefer-regexp-exec.ts b/packages/eslint-plugin/src/rules/prefer-regexp-exec.ts new file mode 100644 index 00000000000..ea4e6742d05 --- /dev/null +++ b/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; + } + }, + }; + }, +}); diff --git a/packages/eslint-plugin/src/rules/prefer-string-starts-ends-with.ts b/packages/eslint-plugin/src/rules/prefer-string-starts-ends-with.ts index 565a4cb83db..ce45e860068 100644 --- a/packages/eslint-plugin/src/rules/prefer-string-starts-ends-with.ts +++ b/packages/eslint-plugin/src/rules/prefer-string-starts-ends-with.ts @@ -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(); @@ -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'; } /** diff --git a/packages/eslint-plugin/src/util/types.ts b/packages/eslint-plugin/src/util/types.ts index 866ff80a45e..55567f00aea 100644 --- a/packages/eslint-plugin/src/util/types.ts +++ b/packages/eslint-plugin/src/util/types.ts @@ -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. */ diff --git a/packages/eslint-plugin/tests/rules/prefer-regexp-exec.test.ts b/packages/eslint-plugin/tests/rules/prefer-regexp-exec.test.ts new file mode 100644 index 00000000000..98381cda6a1 --- /dev/null +++ b/packages/eslint-plugin/tests/rules/prefer-regexp-exec.test.ts @@ -0,0 +1,180 @@ +import path from 'path'; +import rule from '../../src/rules/prefer-regexp-exec'; +import { RuleTester } from '../RuleTester'; + +const rootPath = path.join(process.cwd(), 'tests/fixtures/'); + +const ruleTester = new RuleTester({ + parser: '@typescript-eslint/parser', + parserOptions: { + tsconfigRootDir: rootPath, + project: './tsconfig.json', + }, +}); + +ruleTester.run('prefer-regexp-exec', rule, { + valid: [ + '"something".match();', + '"something".match(/thing/g);', + ` +const text = "something"; +const search = /thing/g; +text.match(search); +`, + ` +const match = (s: RegExp) => "something"; +match(/thing/); +`, + ` +const a = {match : (s: RegExp) => "something"}; +a.match(/thing/); +`, + ` +function f(s: string | string[]) { + s.match(/e/); +} +`, + ], + invalid: [ + { + code: '"something".match(/thing/);', + errors: [ + { + messageId: 'regExpExecOverStringMatch', + line: 1, + column: 1, + }, + ], + }, + { + code: ` +const text = "something"; +const search = /thing/; +text.match(search); +`, + errors: [ + { + messageId: 'regExpExecOverStringMatch', + line: 4, + column: 1, + }, + ], + }, + { + code: '"212".match(2);', + errors: [ + { + messageId: 'regExpExecOverStringMatch', + line: 1, + column: 1, + }, + ], + }, + { + code: '"212".match(+2);', + errors: [ + { + messageId: 'regExpExecOverStringMatch', + line: 1, + column: 1, + }, + ], + }, + { + code: '"oNaNo".match(NaN);', + errors: [ + { + messageId: 'regExpExecOverStringMatch', + line: 1, + column: 1, + }, + ], + }, + { + code: + '"Infinity contains -Infinity and +Infinity in JavaScript.".match(Infinity);', + errors: [ + { + messageId: 'regExpExecOverStringMatch', + line: 1, + column: 1, + }, + ], + }, + { + code: + '"Infinity contains -Infinity and +Infinity in JavaScript.".match(+Infinity);', + errors: [ + { + messageId: 'regExpExecOverStringMatch', + line: 1, + column: 1, + }, + ], + }, + { + code: + '"Infinity contains -Infinity and +Infinity in JavaScript.".match(-Infinity);', + errors: [ + { + messageId: 'regExpExecOverStringMatch', + line: 1, + column: 1, + }, + ], + }, + { + code: '"void and null".match(null);', + errors: [ + { + messageId: 'regExpExecOverStringMatch', + line: 1, + column: 1, + }, + ], + }, + { + code: ` +function f(s: 'a' | 'b') { + s.match('a'); +} +`, + errors: [ + { + messageId: 'regExpExecOverStringMatch', + line: 3, + column: 3, + }, + ], + }, + { + code: ` +type SafeString = string & {__HTML_ESCAPED__: void} +function f(s: SafeString) { + s.match(/thing/); +} +`, + errors: [ + { + messageId: 'regExpExecOverStringMatch', + line: 4, + column: 3, + }, + ], + }, + { + code: ` +function f(s: T) { + s.match(/thing/); +} + `, + errors: [ + { + messageId: 'regExpExecOverStringMatch', + line: 3, + column: 3, + }, + ], + }, + ], +});