Skip to content

feat(eslint-plugin): [no-unnecessary-boolean-literal-compare] add nullish check option for fixer #2252

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

feat(eslint-plugin): [no-unnecessary-boolean-literal-compare] add nullish check option for fixer #2252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2450134
feat(eslint-plugin): [no-unnecessary-boolean-literal-compare] add nul…
Jun 24, 2020
08958ad
Escape pipes
Jun 24, 2020
3c2dffc
Fix spelling
Jun 24, 2020
4013b15
Add more tests
Jun 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 41 additions & 9 deletions packages/eslint-plugin/docs/rules/no-unnecessary-boolean-literal-compare.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ Comparing boolean values to boolean literals is unnecessary, those comparisons r
## Rule Details

This rule ensures that you do not include unnecessary comparisons with boolean literals.
A comparison is considered unnecessary if it checks a boolean literal against any variable with just the `boolean` type.
A comparison is considered unnecessary if it checks a boolean literal against any expression with just the `boolean` type.
A comparison is **_not_** considered unnecessary if the type is a union of booleans (`string | boolean`, `someObject | boolean`).

**Note**: Throughout this page, only strict equality (`===` and `!==`) are
Expand Down Expand Up @@ -44,23 +44,31 @@ The rule accepts an options object with the following properties.

```ts
type Options = {
// if false, comparisons between a nullable boolean variable to `true` will be checked and fixed
// if false, comparisons between a nullable boolean expression to `true` will be checked and fixed
allowComparingNullableBooleansToTrue?: boolean;
// if false, comparisons between a nullable boolean variable to `false` will be checked and fixed
// if false, comparisons between a nullable boolean expression to `false` will be checked and fixed
allowComparingNullableBooleansToFalse?: boolean;
// if defined, comparisons between a nullable boolean expression to boolean literals are replaced
// with a preceeding null/undefined check
fixWithExplicitNullishCheck?: {
// whether the check should be against null or undefined.
// since loose equality (==, !=) is used for this check, this is just a style choice.
nullishSymbol: 'null' | 'undefined';
};
};
```

### Defaults

This rule always checks comparisons between a boolean variable and a boolean
literal. Comparisons between nullable boolean variables and boolean literals
This rule always checks comparisons between a boolean expression and a boolean
literal. Comparisons between nullable boolean expressions and boolean literals
are **not** checked by default.

```ts
const defaults = {
allowComparingNullableBooleansToTrue: true,
allowComparingNullableBooleansToFalse: true,
fixWithExplicitNullishCheck: undefined,
};
```

Expand Down Expand Up @@ -110,25 +118,49 @@ Examples of **correct** code for this rule with `{ allowComparingNullableBoolean
declare const someUndefinedCondition: boolean | undefined;
if (someUndefinedCondition ?? true) {
}
if (someUndefinedCondition != null && !someUndefinedCondition) {
}

declare const someNullCondition: boolean | null;
if (!(someNullCondition ?? true)) {
}
if (someNullCondition == undefined || someNullCondition) {
}
```

## Fixer

### Boolean Expressions

The follow table shows the output of the fixer on comparisons that compare a boolean expression to a boolean literal.

| Comparison | Fixer Output |
| :--------------------: | ----------------- |
| `booleanVar === true` | `booleanLiteral` |
| `booleanVar !== true` | `!booleanLiteral` |
| `booleanVar === false` | `!booleanLiteral` |
| `booleanVar !== false` | `booleanLiteral` |

### Nullable Boolean Expressions

The follow table shows the output of the fixer on comparisons that compare a boolean expression to a boolean literal when the `fixWithExplicitNullishCheck` option is `undefined`:

| Comparison | Fixer Output | Notes |
| :----------------------------: | ------------------------------- | ----------------------------------------------------------------------------------- |
| `booleanVar === true` | `booleanLiteral` | |
| `booleanVar !== true` | `!booleanLiteral` | |
| `booleanVar === false` | `!booleanLiteral` | |
| `booleanVar !== false` | `booleanLiteral` | |
| `nullableBooleanVar === true` | `nullableBooleanVar` | Only checked/fixed if the `allowComparingNullableBooleansToTrue` option is `false` |
| `nullableBooleanVar !== true` | `!nullableBooleanVar` | Only checked/fixed if the `allowComparingNullableBooleansToTrue` option is `false` |
| `nullableBooleanVar === false` | `nullableBooleanVar ?? true` | Only checked/fixed if the `allowComparingNullableBooleansToFalse` option is `false` |
| `nullableBooleanVar !== false` | `!(nullableBooleanVar ?? true)` | Only checked/fixed if the `allowComparingNullableBooleansToFalse` option is `false` |

The follow table shows the output of the fixer on comparisons that compare a boolean expression to a boolean literal when the `fixWithExplicitNullishCheck` option is `{ nullishSymbol: "null" }`:

| Comparison | Fixer Output | Notes |
| :----------------------------: | ----------------------------------------------------- | ----------------------------------------------------------------------------------- |
| `nullableBooleanVar === true` | `nullableBooleanVar != null && nullableBooleanVar` | Only checked/fixed if the `allowComparingNullableBooleansToTrue` option is `false` |
| `nullableBooleanVar !== true` | `nullableBooleanVar == null \|\| !nullableBooleanVar` | Only checked/fixed if the `allowComparingNullableBooleansToTrue` option is `false` |
| `nullableBooleanVar === false` | `nullableBooleanVar != null && !nullableBooleanVar` | Only checked/fixed if the `allowComparingNullableBooleansToFalse` option is `false` |
| `nullableBooleanVar !== false` | `nullableBooleanVar == null \|\| nullableBooleanVar` | Only checked/fixed if the `allowComparingNullableBooleansToFalse` option is `false` |

## Related to

- TSLint: [no-boolean-literal-compare](https://palantir.github.io/tslint/rules/no-boolean-literal-compare)
84 changes: 80 additions & 4 deletions packages/eslint-plugin/src/rules/no-unnecessary-boolean-literal-compare.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ type Options = [
{
allowComparingNullableBooleansToTrue?: boolean;
allowComparingNullableBooleansToFalse?: boolean;
fixWithExplicitNullishCheck?: {
nullishSymbol: 'null' | 'undefined';
};
},
];

Expand Down Expand Up @@ -65,6 +68,14 @@ export default util.createRule<Options, MessageIds>({
allowComparingNullableBooleansToFalse: {
type: 'boolean',
},
fixWithExplicitNullishCheck: {
type: 'object',
properties: {
nullishSymbol: {
type: 'string',
},
},
},
},
additionalProperties: false,
},
Expand Down Expand Up @@ -201,6 +212,36 @@ export default util.createRule<Options, MessageIds>({
);
}

function getExplicitNullishCheck(
{ literalBooleanInComparison, negated }: BooleanComparison,
expressionBeingCompared: string,
nullishSymbol: 'null' | 'undefined',
): string {
if (literalBooleanInComparison) {
// replace `nullableBooleanVar === true` with
// nullableBooleanVar != null && nullableBooleanVar
if (!negated) {
return `${expressionBeingCompared} != ${nullishSymbol} && `;
}

// replace `nullableBooleanVar !== true` with
// nullableBooleanVar == null || !nullableBooleanVar
if (negated) {
return `${expressionBeingCompared} == ${nullishSymbol} || !`;
}
}

// replace `nullableBooleanVar === false` with
// nullableBooleanVar != null && !nullableBooleanVar
if (!negated) {
return `${expressionBeingCompared} != ${nullishSymbol} && !`;
}

// replace `nullableBooleanVar !== false` with
// nullableBooleanVar == null || nullableBooleanVar
return `${expressionBeingCompared} == ${nullishSymbol} || `;
}

return {
BinaryExpression(node): void {
const comparison = getBooleanComparison(node);
Expand All @@ -225,9 +266,41 @@ export default util.createRule<Options, MessageIds>({

context.report({
fix: function* (fixer) {
if (
comparison.expressionIsNullableBoolean &&
options.fixWithExplicitNullishCheck !== undefined
) {
// if fixWithExplicitNullishCheck is true, then we're replacing
// something like `booleanOrFalse === true` with
// `booleanOrFalse != null && booleanOrFalse`. We only apply these
// fixes for identifiers, for more complicated expressions, we
// could change the program behavior (e.g. if the expression is a
// function call, we would be replacing one function call with two)
if (comparison.expression.type === AST_NODE_TYPES.Identifier) {
// remove the comparison and the boolean literal
yield fixer.removeRange(comparison.range);

// add the null check
yield fixer.insertTextBefore(
node,
'(' + // with an opening parenthesis
getExplicitNullishCheck(
comparison,
context.getSourceCode().getText(comparison.expression),
options.fixWithExplicitNullishCheck.nullishSymbol,
),
);

// add the closing parenthesis
yield fixer.insertTextAfter(node, ')');
}
return;
}

// remove the comparison and the boolean literal
yield fixer.removeRange(comparison.range);

// if the expression `exp` isn't nullable, or we're comparing to `true`,
// if the expression `exp` isn't nullable,
// we can just replace the entire comparison with `exp` or `!exp`
if (
!comparison.expressionIsNullableBoolean ||
Expand All @@ -239,10 +312,13 @@ export default util.createRule<Options, MessageIds>({
return;
}

// if we're here, then the expression is a nullable boolean and we're
// comparing to a literal `false`
// if we're here, then:
// 1) the expression is a nullable boolean
// 2) the "fixWithExplicitNullishCheck" option is undefined
// 3) comparing to a literal `false`

// if we're doing `== false` or `=== false`, then we need to negate the expression
// if the comparison is not negated (i.e. we're doing `== false` or `=== false`),
// then we need to negate the expression
if (!comparison.negated) {
const { parent } = node;
// if the parent is a negation, we can instead just get rid of the parent's negation.
Expand Down
Loading