diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index b616245454e6..75feac4ee49e 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -160,6 +160,7 @@ Then you should add `airbnb` (or `airbnb-base`) to your `extends` section of `.e | [`@typescript-eslint/member-naming`](./docs/rules/member-naming.md) | Enforces naming conventions for class members by visibility | | | | | [`@typescript-eslint/member-ordering`](./docs/rules/member-ordering.md) | Require a consistent member declaration order | | | | | [`@typescript-eslint/no-array-constructor`](./docs/rules/no-array-constructor.md) | Disallow generic `Array` constructors | :heavy_check_mark: | :wrench: | | +| [`@typescript-eslint/no-dynamic-delete`](./docs/rules/no-dynamic-delete.md) | Bans usage of the delete operator with computed key expressions | | :wrench: | | | [`@typescript-eslint/no-empty-function`](./docs/rules/no-empty-function.md) | Disallow empty functions | :heavy_check_mark: | | | | [`@typescript-eslint/no-empty-interface`](./docs/rules/no-empty-interface.md) | Disallow the declaration of empty interfaces | :heavy_check_mark: | | | | [`@typescript-eslint/no-explicit-any`](./docs/rules/no-explicit-any.md) | Disallow usage of the `any` type | :heavy_check_mark: | :wrench: | | diff --git a/packages/eslint-plugin/ROADMAP.md b/packages/eslint-plugin/ROADMAP.md index 06326a5439ec..82e7737feb55 100644 --- a/packages/eslint-plugin/ROADMAP.md +++ b/packages/eslint-plugin/ROADMAP.md @@ -60,7 +60,7 @@ | [`no-duplicate-super`] | 🌟 | [`constructor-super`][constructor-super] | | [`no-duplicate-switch-case`] | 🌟 | [`no-duplicate-case`][no-duplicate-case] | | [`no-duplicate-variable`] | 🌟 | [`no-redeclare`][no-redeclare] | -| [`no-dynamic-delete`] | πŸ›‘ | N/A | +| [`no-dynamic-delete`] | βœ… | [`@typescript-eslint/no-dynamic-delete`] | | [`no-empty`] | 🌟 | [`no-empty`][no-empty] | | [`no-eval`] | 🌟 | [`no-eval`][no-eval] | | [`no-floating-promises`] | βœ… | [`@typescript-eslint/no-floating-promises`] | @@ -613,6 +613,7 @@ Relevant plugins: [`chai-expect-keywords`](https://github.com/gavinaiken/eslint- [`@typescript-eslint/member-delimiter-style`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/member-delimiter-style.md [`@typescript-eslint/prefer-for-of`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/prefer-for-of.md [`@typescript-eslint/no-array-constructor`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-array-constructor.md +[`@typescript-eslint/no-dynamic-delete`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-dynamic-delete.md [`@typescript-eslint/prefer-function-type`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/prefer-function-type.md [`@typescript-eslint/prefer-readonly`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/prefer-readonly.md [`@typescript-eslint/require-await`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/require-await.md diff --git a/packages/eslint-plugin/docs/rules/no-dynamic-delete.md b/packages/eslint-plugin/docs/rules/no-dynamic-delete.md new file mode 100644 index 000000000000..630046823387 --- /dev/null +++ b/packages/eslint-plugin/docs/rules/no-dynamic-delete.md @@ -0,0 +1,49 @@ +# Disallow the delete operator with computed key expressions (no-dynamic-delete) + +Deleting dynamically computed keys can be dangerous and in some cases not well optimized. + +## Rule Details + +Using the `delete` operator on keys that aren't runtime constants could be a sign that you're using the wrong data structures. +Using `Object`s with added and removed keys can cause occasional edge case bugs, such as if a key is named `"hasOwnProperty"`. +Consider using a `Map` or `Set` if you’re storing collections of objects. + +Examples of **correct** code wth this rule: + +```ts +const container: { [i: string]: number } = { + /* ... */ +}; + +// Constant runtime lookups by string index +delete container.aaa; + +// Constants that must be accessed by [] +delete container[7]; +delete container['-Infinity']; +``` + +Examples of **incorrect** code with this rule: + +```ts +// Can be replaced with the constant equivalents, such as container.aaa +delete container['aaa']; +delete container['Infinity']; + +// Dynamic, difficult-to-reason-about lookups +const name = 'name'; +delete container[name]; +delete container[name.toUpperCase()]; +``` + +## When Not To Use It + +When you know your keys are safe to delete, this rule can be unnecessary. +Some environments such as older browsers might not support `Map` and `Set`. + +Do not consider this rule as performance advice before profiling your code's bottlenecks. +Even repeated minor performance slowdowns likely do not significantly affect your application's general perceived speed. + +## Related to + +- TSLint: [no-dynamic-delete](https://palantir.github.io/tslint/rules/no-dynamic-delete) diff --git a/packages/eslint-plugin/src/configs/all.json b/packages/eslint-plugin/src/configs/all.json index 395c1af592ed..c7e9a99bc765 100644 --- a/packages/eslint-plugin/src/configs/all.json +++ b/packages/eslint-plugin/src/configs/all.json @@ -26,6 +26,7 @@ "@typescript-eslint/member-ordering": "error", "no-array-constructor": "off", "@typescript-eslint/no-array-constructor": "error", + "@typescript-eslint/no-dynamic-delete": "error", "no-empty-function": "off", "@typescript-eslint/no-empty-function": "error", "@typescript-eslint/no-empty-interface": "error", diff --git a/packages/eslint-plugin/src/rules/index.ts b/packages/eslint-plugin/src/rules/index.ts index 4aa8beea1f63..11c774610a00 100644 --- a/packages/eslint-plugin/src/rules/index.ts +++ b/packages/eslint-plugin/src/rules/index.ts @@ -18,6 +18,7 @@ import memberDelimiterStyle from './member-delimiter-style'; import memberNaming from './member-naming'; import memberOrdering from './member-ordering'; import noArrayConstructor from './no-array-constructor'; +import noDynamicDelete from './no-dynamic-delete'; import noEmptyFunction from './no-empty-function'; import noEmptyInterface from './no-empty-interface'; import noExplicitAny from './no-explicit-any'; @@ -85,6 +86,7 @@ export default { 'member-naming': memberNaming, 'member-ordering': memberOrdering, 'no-array-constructor': noArrayConstructor, + 'no-dynamic-delete': noDynamicDelete, 'no-empty-function': noEmptyFunction, 'no-empty-interface': noEmptyInterface, 'no-explicit-any': noExplicitAny, diff --git a/packages/eslint-plugin/src/rules/no-dynamic-delete.ts b/packages/eslint-plugin/src/rules/no-dynamic-delete.ts new file mode 100644 index 000000000000..0ee1636ea643 --- /dev/null +++ b/packages/eslint-plugin/src/rules/no-dynamic-delete.ts @@ -0,0 +1,109 @@ +import { + TSESTree, + AST_NODE_TYPES, + TSESLint, +} from '@typescript-eslint/experimental-utils'; +import * as tsutils from 'tsutils'; +import * as util from '../util'; + +export default util.createRule({ + name: 'no-dynamic-delete', + meta: { + docs: { + category: 'Best Practices', + description: + 'Bans usage of the delete operator with computed key expressions', + recommended: false, + }, + fixable: 'code', + messages: { + dynamicDelete: 'Do not delete dynamically computed property keys.', + }, + schema: [], + type: 'suggestion', + }, + defaultOptions: [], + create(context) { + function createFixer( + member: TSESTree.MemberExpression, + ): TSESLint.ReportFixFunction | undefined { + if ( + member.property.type === AST_NODE_TYPES.Literal && + typeof member.property.value === 'string' + ) { + return createPropertyReplacement( + member.property, + member.property.value, + ); + } + + if (member.property.type === AST_NODE_TYPES.Identifier) { + return createPropertyReplacement(member.property, member.property.name); + } + + return undefined; + } + + return { + 'UnaryExpression[operator=delete]'(node: TSESTree.UnaryExpression): void { + if ( + node.argument.type !== AST_NODE_TYPES.MemberExpression || + !node.argument.computed || + isNecessaryDynamicAccess( + diveIntoWrapperExpressions(node.argument.property), + ) + ) { + return; + } + + context.report({ + fix: createFixer(node.argument), + messageId: 'dynamicDelete', + node: node.argument.property, + }); + }, + }; + + function createPropertyReplacement( + property: TSESTree.Expression, + replacement: string, + ) { + return (fixer: TSESLint.RuleFixer): TSESLint.RuleFix => + fixer.replaceTextRange(getTokenRange(property), `.${replacement}`); + } + + function getTokenRange(property: TSESTree.Expression): [number, number] { + const sourceCode = context.getSourceCode(); + + return [ + sourceCode.getTokenBefore(property)!.range[0], + sourceCode.getTokenAfter(property)!.range[1], + ]; + } + }, +}); + +function diveIntoWrapperExpressions( + node: TSESTree.Expression, +): TSESTree.Expression { + if (node.type === AST_NODE_TYPES.UnaryExpression) { + return diveIntoWrapperExpressions(node.argument); + } + + return node; +} + +function isNecessaryDynamicAccess(property: TSESTree.Expression): boolean { + if (property.type !== AST_NODE_TYPES.Literal) { + return false; + } + + if (typeof property.value === 'number') { + return true; + } + + return ( + typeof property.value === 'string' && + !tsutils.isValidPropertyAccess(property.value) + ); +} diff --git a/packages/eslint-plugin/tests/rules/no-dynamic-delete.test.ts b/packages/eslint-plugin/tests/rules/no-dynamic-delete.test.ts new file mode 100644 index 000000000000..e3b4e5366e8b --- /dev/null +++ b/packages/eslint-plugin/tests/rules/no-dynamic-delete.test.ts @@ -0,0 +1,105 @@ +import path from 'path'; +import rule from '../../src/rules/no-dynamic-delete'; +import { RuleTester } from '../RuleTester'; + +const rootDir = path.join(process.cwd(), 'tests/fixtures'); +const ruleTester = new RuleTester({ + parserOptions: { + ecmaVersion: 2015, + tsconfigRootDir: rootDir, + project: './tsconfig.json', + }, + parser: '@typescript-eslint/parser', +}); + +ruleTester.run('no-dynamic-delete', rule, { + valid: [ + `const container: { [i: string]: 0 } = {}; + delete container.aaa;`, + `const container: { [i: string]: 0 } = {}; + delete container.delete;`, + `const container: { [i: string]: 0 } = {}; + delete container[7];`, + `const container: { [i: string]: 0 } = {}; + delete container[-7];`, + `const container: { [i: string]: 0 } = {}; + delete container[+7];`, + `const container: { [i: string]: 0 } = {}; + delete container['-Infinity'];`, + `const container: { [i: string]: 0 } = {}; + delete container['+Infinity'];`, + `const value = 1; + delete value;`, + `const value = 1; + delete -value;`, + ], + invalid: [ + { + code: `const container: { [i: string]: 0 } = {}; + delete container['aaa'];`, + errors: [{ messageId: 'dynamicDelete' }], + output: `const container: { [i: string]: 0 } = {}; + delete container.aaa;`, + }, + { + code: `const container: { [i: string]: 0 } = {}; + delete container [ 'aaa' ] ;`, + errors: [{ messageId: 'dynamicDelete' }], + output: `const container: { [i: string]: 0 } = {}; + delete container .aaa ;`, + }, + { + code: `const container: { [i: string]: 0 } = {}; + delete container['aa' + 'b'];`, + errors: [{ messageId: 'dynamicDelete' }], + }, + { + code: `const container: { [i: string]: 0 } = {}; + delete container['delete'];`, + errors: [{ messageId: 'dynamicDelete' }], + output: `const container: { [i: string]: 0 } = {}; + delete container.delete;`, + }, + { + code: `const container: { [i: string]: 0 } = {}; + delete container[-Infinity];`, + errors: [{ messageId: 'dynamicDelete' }], + }, + { + code: `const container: { [i: string]: 0 } = {}; + delete container[+Infinity];`, + errors: [{ messageId: 'dynamicDelete' }], + }, + { + code: `const container: { [i: string]: 0 } = {}; + delete container[NaN];`, + errors: [{ messageId: 'dynamicDelete' }], + }, + { + code: `const container: { [i: string]: 0 } = {}; + delete container['NaN'];`, + errors: [{ messageId: 'dynamicDelete' }], + output: `const container: { [i: string]: 0 } = {}; + delete container.NaN;`, + }, + { + code: `const container: { [i: string]: 0 } = {}; + delete container [ 'NaN' ] ;`, + errors: [{ messageId: 'dynamicDelete' }], + output: `const container: { [i: string]: 0 } = {}; + delete container .NaN ;`, + }, + { + code: `const container: { [i: string]: 0 } = {}; + const name = 'name'; + delete container[name];`, + errors: [{ messageId: 'dynamicDelete' }], + }, + { + code: `const container: { [i: string]: 0 } = {}; + const getName = () => 'aaa'; + delete container[getName()];`, + errors: [{ messageId: 'dynamicDelete' }], + }, + ], +});