Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
feat(eslint-plugin): added new rule no-dynamic-delete (#565)
Co-authored-by: Josh Goldberg <josh@fullscreenmario.com> Co-authored-by: Brad Zacher <brad.zacher@gmail.com>
- Loading branch information
3 people
committed
Nov 12, 2019
1 parent
62b5a94
commit 864c811
Showing
7 changed files
with
269 additions
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
| ); | ||
| } |
105 changes: 105 additions & 0 deletions
105
packages/eslint-plugin/tests/rules/no-dynamic-delete.test.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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' }], | ||
| }, | ||
| ], | ||
| }); |