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): added new rule no-dynamic-delete #565

Merged
Show file tree
Hide file tree
Changes from 7 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
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ Then you should add `airbnb` (or `airbnb-base`) to your `extends` section of `.e
| [`@typescript-eslint/member-ordering`](./docs/rules/member-ordering.md) | Require a consistent member declaration order | | | |
| [`@typescript-eslint/no-angle-bracket-type-assertion`](./docs/rules/no-angle-bracket-type-assertion.md) | Enforces the use of `as Type` assertions instead of `<Type>` assertions | :heavy_check_mark: | | |
| [`@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 | | | |
| [`@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: | |
Expand Down
3 changes: 2 additions & 1 deletion packages/eslint-plugin/ROADMAP.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,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`] |
Expand Down Expand Up @@ -610,6 +610,7 @@ Relevant plugins: [`chai-expect-keywords`](https://github.com/gavinaiken/eslint-
[`@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/prefer-interface`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/prefer-interface.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/no-for-in-array`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-for-in-array.md
Expand Down
44 changes: 44 additions & 0 deletions packages/eslint-plugin/docs/rules/no-dynamic-delete.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# 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'];
```

## 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)
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/configs/all.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
"@typescript-eslint/no-angle-bracket-type-assertion": "error",
"no-array-constructor": "off",
"@typescript-eslint/no-array-constructor": "error",
"@typescript-eslint/no-dynamic-delete": "error",
"@typescript-eslint/no-empty-function": "error",
"@typescript-eslint/no-empty-interface": "error",
"@typescript-eslint/no-explicit-any": "error",
Expand Down
2 changes: 2 additions & 0 deletions packages/eslint-plugin/src/rules/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import memberNaming from './member-naming';
import memberOrdering from './member-ordering';
import noAngleBracketTypeAssertion from './no-angle-bracket-type-assertion';
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';
Expand Down Expand Up @@ -79,6 +80,7 @@ export default {
'member-ordering': memberOrdering,
'no-angle-bracket-type-assertion': noAngleBracketTypeAssertion,
'no-array-constructor': noArrayConstructor,
'no-dynamic-delete': noDynamicDelete,
'no-empty-function': noEmptyFunction,
'no-empty-interface': noEmptyInterface,
'no-explicit-any': noExplicitAny,
Expand Down
99 changes: 99 additions & 0 deletions packages/eslint-plugin/src/rules/no-dynamic-delete.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
import {
TSESTree,
AST_NODE_TYPES,
} from '@typescript-eslint/experimental-utils';
import * as tsutils from 'tsutils';
import * as util from '../util';
import { ReportFixFunction } from '@typescript-eslint/experimental-utils/dist/ts-eslint';
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

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,
): ReportFixFunction | undefined {
if (
member.property.type === AST_NODE_TYPES.Literal &&
typeof member.property.value === 'string'
) {
const { value } = member.property;
return fixer =>
fixer.replaceTextRange(
[member.property.range[0] - 1, member.property.range[1] + 1],
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
`.${value}`,
);
}

if (member.property.type === AST_NODE_TYPES.Identifier) {
const { name } = member.property;
return fixer =>
fixer.replaceTextRange(
[member.property.range[0] - 1, member.property.range[1] + 1],
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
`.${name}`,
);
}

return undefined;
}

return {
'UnaryExpression[operator=delete]'(node: TSESTree.UnaryExpression) {
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 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)
);
}
91 changes: 91 additions & 0 deletions packages/eslint-plugin/tests/rules/no-dynamic-delete.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
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['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 } = {};
const name = 'name';
delete container[name];`,
errors: [{ messageId: 'dynamicDelete' }],
},
{
code: `const container: { [i: string]: 0 } = {};
const getName = () => 'aaa';
delete container[getName()];`,
errors: [{ messageId: 'dynamicDelete' }],
},
],
});