Skip to content

Commit

Permalink
feat(eslint-plugin): [prefer-nullish-coalescing][prefer-optional-chai…
Browse files Browse the repository at this point in the history 
…n] remove unsafe fixers

These fixers are very unsafe and even providing default options for them are a bad idea.
This ensures that they are suggestion fixers in all cases, and removes the options that allow them not to be.
  • Loading branch information
@bradzacher
bradzacher committed May 21, 2020
1 parent ae82ea4 commit 52b6085
Show file tree
Hide file tree
Showing 7 changed files with 271 additions and 215 deletions.
4 changes: 2 additions & 2 deletions packages/eslint-plugin/README.md
View file
Expand Up @@ -146,8 +146,8 @@ Pro Tip: For larger codebases you may want to consider splitting our linting int
| [`@typescript-eslint/prefer-function-type`](./docs/rules/prefer-function-type.md) | Use function types instead of interfaces with call signatures | | :wrench: | |
| [`@typescript-eslint/prefer-includes`](./docs/rules/prefer-includes.md) | Enforce `includes` method over `indexOf` method | | :wrench: | :thought_balloon: |
| [`@typescript-eslint/prefer-namespace-keyword`](./docs/rules/prefer-namespace-keyword.md) | Require the use of the `namespace` keyword instead of the `module` keyword to declare custom TypeScript modules | :heavy_check_mark: | :wrench: | |
| [`@typescript-eslint/prefer-nullish-coalescing`](./docs/rules/prefer-nullish-coalescing.md) | Enforce the usage of the nullish coalescing operator instead of logical chaining | | :wrench: | :thought_balloon: |
| [`@typescript-eslint/prefer-optional-chain`](./docs/rules/prefer-optional-chain.md) | Prefer using concise optional chain expressions instead of chained logical ands | | :wrench: | |
| [`@typescript-eslint/prefer-nullish-coalescing`](./docs/rules/prefer-nullish-coalescing.md) | Enforce the usage of the nullish coalescing operator instead of logical chaining | | | :thought_balloon: |
| [`@typescript-eslint/prefer-optional-chain`](./docs/rules/prefer-optional-chain.md) | Prefer using concise optional chain expressions instead of chained logical ands | | | |
| [`@typescript-eslint/prefer-readonly`](./docs/rules/prefer-readonly.md) | Requires that private members are marked as `readonly` if they're never modified outside of the constructor | | :wrench: | :thought_balloon: |
| [`@typescript-eslint/prefer-readonly-parameter-types`](./docs/rules/prefer-readonly-parameter-types.md) | Requires that function parameters are typed as readonly to prevent accidental mutation of inputs | | | :thought_balloon: |
| [`@typescript-eslint/prefer-reduce-type-parameter`](./docs/rules/prefer-reduce-type-parameter.md) | Prefer using type parameter when calling `Array#reduce` instead of casting | | :wrench: | :thought_balloon: |
Expand Down
8 changes: 0 additions & 8 deletions packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md
View file
Expand Up @@ -46,15 +46,13 @@ type Options = [
{
ignoreConditionalTests?: boolean;
ignoreMixedLogicalExpressions?: boolean;
forceSuggestionFixer?: boolean;
},
];

const defaultOptions = [
{
ignoreConditionalTests: true,
ignoreMixedLogicalExpressions: true,
forceSuggestionFixer: false,
},
];
```
Expand Down Expand Up @@ -133,12 +131,6 @@ a ?? (b && c && d);

**_NOTE:_** Errors for this specific case will be presented as suggestions (see below), instead of fixes. This is because it is not always safe to automatically convert `||` to `??` within a mixed logical expression, as we cannot tell the intended precedence of the operator. Note that by design, `??` requires parentheses when used with `&&` or `||` in the same expression.

### `forceSuggestionFixer`

Setting this option to `true` will cause the rule to use ESLint's "suggested fix" mode for all fixes. _This option is provided as to aid in transitioning your codebase onto this rule_.

Suggestion fixes cannot be automatically applied via the `--fix` CLI command, but can be _manually_ chosen to be applied one at a time via an IDE or similar. This makes it safe to run autofixers on an existing codebase without worrying about potential runtime behavior changes from this rule's fixer.

## When Not To Use It

If you are not using TypeScript 3.7 (or greater), then you will not be able to use this rule, as the operator is not supported.
Expand Down
15 changes: 1 addition & 14 deletions packages/eslint-plugin/docs/rules/prefer-optional-chain.md
View file
Expand Up @@ -70,20 +70,7 @@ foo?.a?.b?.method?.();
foo?.a?.b?.c?.d?.e;
```

## Options

The rule accepts an options object with the following properties:

```ts
type Options = {
// if true, the rule will only provide suggested fixes instead of automatically modifying code
suggestInsteadOfAutofix?: boolean;
};

const defaults = {
suggestInsteadOfAutofix: false,
};
```
**Note:** there are a few edge cases where this rule will false positive. Use your best judgement when evaluating reported errors.

## When Not To Use It

Expand Down
37 changes: 9 additions & 28 deletions packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts
View file
Expand Up @@ -10,10 +10,9 @@ export type Options = [
{
ignoreConditionalTests?: boolean;
ignoreMixedLogicalExpressions?: boolean;
forceSuggestionFixer?: boolean;
},
];
export type MessageIds = 'preferNullish';
export type MessageIds = 'preferNullish' | 'suggestNullish';

export default util.createRule<Options, MessageIds>({
name: 'prefer-nullish-coalescing',
Expand All @@ -27,10 +26,10 @@ export default util.createRule<Options, MessageIds>({
suggestion: true,
requiresTypeChecking: true,
},
fixable: 'code',
messages: {
preferNullish:
'Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.',
suggestNullish: 'Fix to nullish coalescing operator (`??`).',
},
schema: [
{
Expand All @@ -54,19 +53,9 @@ export default util.createRule<Options, MessageIds>({
{
ignoreConditionalTests: true,
ignoreMixedLogicalExpressions: true,
forceSuggestionFixer: false,
},
],
create(
context,
[
{
ignoreConditionalTests,
ignoreMixedLogicalExpressions,
forceSuggestionFixer,
},
],
) {
create(context, [{ ignoreConditionalTests, ignoreMixedLogicalExpressions }]) {
const parserServices = util.getParserServices(context);
const sourceCode = context.getSourceCode();
const checker = parserServices.program.getTypeChecker();
Expand Down Expand Up @@ -119,23 +108,15 @@ export default util.createRule<Options, MessageIds>({
yield fixer.replaceText(barBarOperator, '??');
}

const fixer =
isMixedLogical || forceSuggestionFixer
? // suggestion instead for cases where we aren't sure if the fixer is completely safe
({
suggest: [
{
messageId: 'preferNullish',
fix,
},
],
} as const)
: { fix };

context.report({
node: barBarOperator,
messageId: 'preferNullish',
...fixer,
suggest: [
{
messageId: 'suggestNullish',
fix,
},
],
});
},
};
Expand Down
63 changes: 15 additions & 48 deletions packages/eslint-plugin/src/rules/prefer-optional-chain.ts
View file
Expand Up @@ -31,15 +31,7 @@ The AST will look like this:
}
*/

type Options = [
{
suggestInsteadOfAutofix?: boolean;
},
];

type MessageIds = 'preferOptionalChain' | 'optionalChainSuggest';

export default util.createRule<Options, MessageIds>({
export default util.createRule({
name: 'prefer-optional-chain',
meta: {
type: 'suggestion',
Expand All @@ -50,30 +42,15 @@ export default util.createRule<Options, MessageIds>({
recommended: false,
suggestion: true,
},
fixable: 'code',
messages: {
preferOptionalChain:
"Prefer using an optional chain expression instead, as it's more concise and easier to read.",
optionalChainSuggest: 'Change to an optional chain.',
},
schema: [
{
type: 'object',
properties: {
suggestInsteadOfAutofix: {
type: 'boolean',
},
},
additionalProperties: false,
},
],
schema: [],
},
defaultOptions: [
{
suggestInsteadOfAutofix: false,
},
],
create(context, [options]) {
defaultOptions: [],
create(context) {
const sourceCode = context.getSourceCode();
return {
[[
Expand Down Expand Up @@ -189,28 +166,18 @@ export default util.createRule<Options, MessageIds>({
} ${sourceCode.getText(previous.right.right)}`;
}

if (!options.suggestInsteadOfAutofix) {
context.report({
node: previous,
messageId: 'preferOptionalChain',
fix(fixer) {
return fixer.replaceText(previous, optionallyChainedCode);
context.report({
node: previous,
messageId: 'preferOptionalChain',
suggest: [
{
messageId: 'optionalChainSuggest',
fix: (fixer): TSESLint.RuleFix[] => [
fixer.replaceText(previous, optionallyChainedCode),
],
},
});
} else {
context.report({
node: previous,
messageId: 'preferOptionalChain',
suggest: [
{
messageId: 'optionalChainSuggest',
fix: (fixer): TSESLint.RuleFix[] => [
fixer.replaceText(previous, optionallyChainedCode),
],
},
],
});
}
],
});
}
},
};
Expand Down

0 comments on commit 52b6085

Please sign in to comment.