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 index 81b4a5c67f6..d600bb7930c 100644 --- 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 index ffcdd0e0732..fe1fd24cc4f 100644 --- 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({ 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 index 2bd7e199739..d030b829cd4 100644 --- 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 index 6480d548906..36eb36033de 100644 --- 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 @@ -38,17 +38,39 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { varObject == false; `, ` - declare const varBooleanOrString: boolean | undefined; + declare const varNullOrUndefined: null | undefined; + varNullOrUndefined === false; + `, + ` + declare const varBooleanOrString: boolean | string; varBooleanOrString === false; `, ` - declare const varBooleanOrString: boolean | undefined; + declare const varBooleanOrString: boolean | string; varBooleanOrString == true; `, + ` + declare const varTrueOrStringOrUndefined: true | string | undefined; + varTrueOrStringOrUndefined == true; + `, ` declare const varBooleanOrUndefined: boolean | undefined; varBooleanOrUndefined === true; `, + { + code: ` + declare const varBooleanOrUndefined: boolean | undefined; + varBooleanOrUndefined === true; + `, + options: [{ allowComparingNullableBooleansToFalse: false }], + }, + { + code: ` + declare const varBooleanOrUndefined: boolean | undefined; + varBooleanOrUndefined === false; + `, + options: [{ allowComparingNullableBooleansToTrue: false }], + }, "'false' === true;", "'true' === false;", ], @@ -106,5 +128,101 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { } `, }, + { + code: ` + declare const varTrueOrUndefined: true | undefined; + if (varTrueOrUndefined === true) { + } + `, + options: [{ allowComparingNullableBooleansToTrue: false }], + errors: [ + { + messageId: 'comparingNullableToTrueDirect', + }, + ], + output: ` + declare const varTrueOrUndefined: true | undefined; + if (varTrueOrUndefined) { + } + `, + }, + { + code: ` + declare const varFalseOrNull: false | null; + if (varFalseOrNull !== true) { + } + `, + options: [{ allowComparingNullableBooleansToTrue: false }], + errors: [ + { + messageId: 'comparingNullableToTrueNegated', + }, + ], + output: ` + declare const varFalseOrNull: false | null; + if (!varFalseOrNull) { + } + `, + }, + { + code: ` + declare const varBooleanOrNull: boolean | null; + declare const otherBoolean: boolean; + if (varBooleanOrNull === false && otherBoolean) { + } + `, + options: [{ allowComparingNullableBooleansToFalse: false }], + errors: [ + { + messageId: 'comparingNullableToFalse', + }, + ], + output: ` + declare const varBooleanOrNull: boolean | null; + declare const otherBoolean: boolean; + if (!(varBooleanOrNull ?? true) && otherBoolean) { + } + `, + }, + { + code: ` + declare const varBooleanOrNull: boolean | null; + declare const otherBoolean: boolean; + if (!(varBooleanOrNull === false) || otherBoolean) { + } + `, + options: [{ allowComparingNullableBooleansToFalse: false }], + errors: [ + { + messageId: 'comparingNullableToFalse', + }, + ], + output: ` + declare const varBooleanOrNull: boolean | null; + declare const otherBoolean: boolean; + if ((varBooleanOrNull ?? true) || otherBoolean) { + } + `, + }, + { + code: ` + declare const varTrueOrFalseOrUndefined: true | false | undefined; + declare const otherBoolean: boolean; + if (varTrueOrFalseOrUndefined !== false && !otherBoolean) { + } + `, + options: [{ allowComparingNullableBooleansToFalse: false }], + errors: [ + { + messageId: 'comparingNullableToFalse', + }, + ], + output: ` + declare const varTrueOrFalseOrUndefined: true | false | undefined; + declare const otherBoolean: boolean; + if ((varTrueOrFalseOrUndefined ?? true) && !otherBoolean) { + } + `, + }, ], });