feat(eslint-plugin): [no-unnecessary-boolean-literal-compare] add option to check nullable booleans (#1983)

zachkirsch · web-flow · commit c0b3057b7f7d · 2020-06-20T13:28:14.000-07:00
diff --git a/packages/eslint-plugin/docs/rules/no-unnecessary-boolean-literal-compare.md b/packages/eslint-plugin/docs/rules/no-unnecessary-boolean-literal-compare.md
@@ -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
@@ -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)
diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-boolean-literal-compare.ts b/packages/eslint-plugin/src/rules/no-unnecessary-boolean-literal-compare.ts
@@ -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: {
@@ -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;
@@ -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(
@@ -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:
@@ -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,
+        });
       },
     };
   },
diff --git a/packages/eslint-plugin/src/util/types.ts b/packages/eslint-plugin/src/util/types.ts
@@ -308,7 +308,7 @@ export function getEqualsKind(operator: string): EqualsKind | undefined {
 
     case '!==':
       return {
-        isPositive: true,
+        isPositive: false,
         isStrict: true,
       };
 
diff --git a/packages/eslint-plugin/tests/rules/no-unnecessary-boolean-literal-compare.test.ts b/packages/eslint-plugin/tests/rules/no-unnecessary-boolean-literal-compare.test.ts
