Skip to content

Commit

Permalink
feat(eslint-plugin): [no-unnecessary-boolean-literal-compare] add opt…
Browse files Browse the repository at this point in the history 
…ion to check nullable booleans (#1983)
  • Loading branch information
@zachkirsch
zachkirsch committed Jun 20, 2020
1 parent c3753c2 commit c0b3057
Show file tree
Hide file tree
Showing 4 changed files with 395 additions and 29 deletions.
93 changes: 93 additions & 0 deletions packages/eslint-plugin/docs/rules/no-unnecessary-boolean-literal-compare.md
View file
Expand Up @@ -8,6 +8,12 @@ This rule ensures that you do not include unnecessary comparisons with boolean l
A comparison is considered unnecessary if it checks a boolean literal against any variable 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
used in the examples. However, the implementation of the rule does not
distinguish between strict and loose equality. Any example below that uses
`===` would be treated the same way if `==` was used, and any example below
that uses `!==` would be treated the same way if `!=` was used.

Examples of **incorrect** code for this rule:

```ts
Expand All @@ -30,12 +36,99 @@ if (someObjectBoolean === true) {
declare const someStringBoolean: boolean | string;
if (someStringBoolean === true) {
}
```

## Options

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
allowComparingNullableBooleansToTrue?: boolean;
// if false, comparisons between a nullable boolean variable to `false` will be checked and fixed
allowComparingNullableBooleansToFalse?: boolean;
};
```

### Defaults

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

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

### `allowComparingNullableBooleansToTrue`

Examples of **incorrect** code for this rule with `{ allowComparingNullableBooleansToTrue: false }`:

```ts
declare const someUndefinedCondition: boolean | undefined;
if (someUndefinedCondition === true) {
}

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

Examples of **correct** code for this rule with `{ allowComparingNullableBooleansToTrue: false }`:

```ts
declare const someUndefinedCondition: boolean | undefined;
if (someUndefinedCondition) {
}

declare const someNullCondition: boolean | null;
if (!someNullCondition) {
}
```

### `allowComparingNullableBooleansToFalse`

Examples of **incorrect** code for this rule with `{ allowComparingNullableBooleansToFalse: false }`:

```ts
declare const someUndefinedCondition: boolean | undefined;
if (someUndefinedCondition === false) {
}

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

Examples of **correct** code for this rule with `{ allowComparingNullableBooleansToFalse: false }`:

```ts
declare const someUndefinedCondition: boolean | undefined;
if (someUndefinedCondition ?? true) {
}

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

## Fixer

| 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` |

## Related to

- TSLint: [no-boolean-literal-compare](https://palantir.github.io/tslint/rules/no-boolean-literal-compare)
207 changes: 181 additions & 26 deletions packages/eslint-plugin/src/rules/no-unnecessary-boolean-literal-compare.ts
View file
Expand Up @@ -6,16 +6,33 @@ import * as tsutils from 'tsutils';
import * as ts from 'typescript';
import * as util from '../util';

type MessageIds = 'direct' | 'negated';
type MessageIds =
| 'direct'
| 'negated'
| 'comparingNullableToTrueDirect'
| 'comparingNullableToTrueNegated'
| 'comparingNullableToFalse';

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

interface BooleanComparison {
expression: TSESTree.Expression;
literalBooleanInComparison: boolean;
forTruthy: boolean;
negated: boolean;
range: [number, number];
}

export default util.createRule<[], MessageIds>({
interface BooleanComparisonWithTypeInformation extends BooleanComparison {
expressionIsNullableBoolean: boolean;
}

export default util.createRule<Options, MessageIds>({
name: 'no-unnecessary-boolean-literal-compare',
meta: {
docs: {
Expand All @@ -31,18 +48,42 @@ export default util.createRule<[], MessageIds>({
'This expression unnecessarily compares a boolean value to a boolean instead of using it directly.',
negated:
'This expression unnecessarily compares a boolean value to a boolean instead of negating it.',
comparingNullableToTrueDirect:
'This expression unnecessarily compares a nullable boolean value to true instead of using it directly.',
comparingNullableToTrueNegated:
'This expression unnecessarily compares a nullable boolean value to true instead of negating it.',
comparingNullableToFalse:
'This expression unnecessarily compares a nullable boolean value to false instead of using the ?? operator to provide a default.',
},
schema: [],
schema: [
{
type: 'object',
properties: {
allowComparingNullableBooleansToTrue: {
type: 'boolean',
},
allowComparingNullableBooleansToFalse: {
type: 'boolean',
},
},
additionalProperties: false,
},
],
type: 'suggestion',
},
defaultOptions: [],
create(context) {
defaultOptions: [
{
allowComparingNullableBooleansToTrue: true,
allowComparingNullableBooleansToFalse: true,
},
],
create(context, [options]) {
const parserServices = util.getParserServices(context);
const checker = parserServices.program.getTypeChecker();

function getBooleanComparison(
node: TSESTree.BinaryExpression,
): BooleanComparison | undefined {
): BooleanComparisonWithTypeInformation | undefined {
const comparison = deconstructComparison(node);
if (!comparison) {
return undefined;
Expand All @@ -52,16 +93,67 @@ export default util.createRule<[], MessageIds>({
parserServices.esTreeNodeToTSNodeMap.get(comparison.expression),
);

if (
!tsutils.isTypeFlagSet(
expressionType,
ts.TypeFlags.Boolean | ts.TypeFlags.BooleanLiteral,
)
) {
return undefined;
if (isBooleanType(expressionType)) {
return {
...comparison,
expressionIsNullableBoolean: false,
};
}

if (isNullableBoolean(expressionType)) {
return {
...comparison,
expressionIsNullableBoolean: true,
};
}

return comparison;
return undefined;
}

function isBooleanType(expressionType: ts.Type): boolean {
return tsutils.isTypeFlagSet(
expressionType,
ts.TypeFlags.Boolean | ts.TypeFlags.BooleanLiteral,
);
}

/**
* checks if the expressionType is a union that
* 1) contains at least one nullish type (null or undefined)
* 2) contains at least once boolean type (true or false or boolean)
* 3) does not contain any types besides nullish and boolean types
*/
function isNullableBoolean(expressionType: ts.Type): boolean {
if (!expressionType.isUnion()) {
return false;
}

const { types } = expressionType;

const nonNullishTypes = types.filter(
type =>
!tsutils.isTypeFlagSet(
type,
ts.TypeFlags.Undefined | ts.TypeFlags.Null,
),
);

const hasNonNullishType = nonNullishTypes.length > 0;
if (!hasNonNullishType) {
return false;
}

const hasNullableType = nonNullishTypes.length < types.length;
if (!hasNullableType) {
return false;
}

const allNonNullishTypesAreBoolean = nonNullishTypes.every(isBooleanType);
if (!allNonNullishTypesAreBoolean) {
return false;
}

return true;
}

function deconstructComparison(
Expand All @@ -83,11 +175,12 @@ export default util.createRule<[], MessageIds>({
continue;
}

const { value } = against;
const negated = node.operator.startsWith('!');
const { value: literalBooleanInComparison } = against;
const negated = !comparisonType.isPositive;

return {
forTruthy: value ? !negated : negated,
literalBooleanInComparison,
forTruthy: literalBooleanInComparison ? !negated : negated,
expression,
negated,
range:
Expand All @@ -100,23 +193,85 @@ export default util.createRule<[], MessageIds>({
return undefined;
}

function nodeIsUnaryNegation(node: TSESTree.Node): boolean {
return (
node.type === AST_NODE_TYPES.UnaryExpression &&
node.prefix &&
node.operator === '!'
);
}

return {
BinaryExpression(node): void {
const comparison = getBooleanComparison(node);
if (comparison === undefined) {
return;
}

if (comparison) {
context.report({
fix: function* (fixer) {
yield fixer.removeRange(comparison.range);
if (comparison.expressionIsNullableBoolean) {
if (
comparison.literalBooleanInComparison &&
options.allowComparingNullableBooleansToTrue
) {
return;
}
if (
!comparison.literalBooleanInComparison &&
options.allowComparingNullableBooleansToFalse
) {
return;
}
}

context.report({
fix: function* (fixer) {
yield fixer.removeRange(comparison.range);

// if the expression `exp` isn't nullable, or we're comparing to `true`,
// we can just replace the entire comparison with `exp` or `!exp`
if (
!comparison.expressionIsNullableBoolean ||
comparison.literalBooleanInComparison
) {
if (!comparison.forTruthy) {
yield fixer.insertTextBefore(node, '!');
}
},
messageId: comparison.negated ? 'negated' : 'direct',
node,
});
}
return;
}

// if we're here, then the expression is a nullable boolean and we're
// comparing to a literal `false`

// if 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.
// i.e. instead of resulting in `!(!(exp))`, we can just result in `exp`
if (parent != null && nodeIsUnaryNegation(parent)) {
// remove from the beginning of the parent to the beginning of this node
yield fixer.removeRange([parent.range[0], node.range[0]]);
// remove from the end of the node to the end of the parent
yield fixer.removeRange([node.range[1], parent.range[1]]);
} else {
yield fixer.insertTextBefore(node, '!');
}
}

// provide the default `true`
yield fixer.insertTextBefore(node, '(');
yield fixer.insertTextAfter(node, ' ?? true)');
},
messageId: comparison.expressionIsNullableBoolean
? comparison.literalBooleanInComparison
? comparison.negated
? 'comparingNullableToTrueNegated'
: 'comparingNullableToTrueDirect'
: 'comparingNullableToFalse'
: comparison.negated
? 'negated'
: 'direct',
node,
});
},
};
},
Expand Down
2 changes: 1 addition & 1 deletion packages/eslint-plugin/src/util/types.ts
View file
Expand Up @@ -308,7 +308,7 @@ export function getEqualsKind(operator: string): EqualsKind | undefined {

case '!==':
return {
isPositive: true,
isPositive: false,
isStrict: true,
};

Expand Down

0 comments on commit c0b3057

Please sign in to comment.