From 31b4cb416b51c7689c9a387d6f64b2e8bfc719d4 Mon Sep 17 00:00:00 2001 From: Zach Kirsch Date: Wed, 6 May 2020 17:22:30 -0400 Subject: [PATCH 01/10] feat(eslint-plugin): [no-unnecessary-boolean-literal-compare] add option to check nullable booleans --- .../no-unnecessary-boolean-literal-compare.md | 42 ++++ .../no-unnecessary-boolean-literal-compare.ts | 198 +++++++++++++++--- packages/eslint-plugin/src/util/types.ts | 2 +- ...nnecessary-boolean-literal-compare.test.ts | 126 +++++++++++ 4 files changed, 339 insertions(+), 29 deletions(-) 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 81b4a5c67f6e..485b9fedafc4 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 @@ -36,6 +36,48 @@ if (someUndefinedCondition === false) { } ``` +## Options + +The rule accepts an options object with the following properties: + +```ts +type Options = { + // if true, only comparisons that compare a boolean literal to a boolean will be checked. + // if false, comparisons that compare a boolean literal to a nullable boolean variable will also be checked + allowComparingNullableBooleans?: boolean; +}; + +const defaults = { + allowComparingNullableBooleans: true, +}; +``` + +### `allowComparingNullableBooleans` + +Examples of **incorrect** code for this rule with `{ allowComparingNullableBooleans: false }`: + +```ts +declare const someUndefinedCondition: boolean | undefined; +if (someUndefinedCondition === true) { +} + +declare const someNullCondition: boolean | null; +if (someNullCondition !== false) { +} +``` + +Examples of **correct** code for this rule with `{ allowComparingNullableBooleans: false }`: + +```ts +declare const someUndefinedCondition: boolean | undefined; +if (someUndefinedCondition) { +} + +declare const someNullCondition: boolean | null; +if (someNullCondition ?? true) { +} +``` + ## 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 ffcdd0e07320..b6b3a218baa8 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,32 @@ 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 = [ + { + allowComparingNullableBooleans?: 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 +47,38 @@ 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: { + allowComparingNullableBooleans: { + type: 'boolean', + }, + }, + additionalProperties: false, + }, + ], type: 'suggestion', }, - defaultOptions: [], - create(context) { + defaultOptions: [ + { + allowComparingNullableBooleans: true, + }, + ], + create(context, [options]) { const parserServices = util.getParserServices(context); const checker = parserServices.program.getTypeChecker(); - function getBooleanComparison( + function getBooleanComparisonWithDefault( node: TSESTree.BinaryExpression, - ): BooleanComparison | undefined { + ): BooleanComparisonWithTypeInformation | undefined { const comparison = deconstructComparison(node); if (!comparison) { return undefined; @@ -52,16 +88,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 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; } - return comparison; + 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 +170,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 +188,77 @@ 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); + const comparison = getBooleanComparisonWithDefault(node); // pass optional config + if (comparison === undefined) { + return; + } + + if ( + comparison.expressionIsNullableBoolean && + !options.allowComparingNullableBooleans + ) { + return; + } - if (comparison) { - context.report({ - fix: function* (fixer) { - yield fixer.removeRange(comparison.range); + 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` + + // provide the default `true` + yield fixer.insertTextAfter(node, ' ?? true'); + + // if we're doing `=== false`, the we need to negate the expression + if (!comparison.negated) { + const { parent } = node; + // if the parent is a negation, we can 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, '!('); + yield fixer.insertTextAfter(node, ')'); + } + } + }, + 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 08a67cd7e5d9..10a9db018a6a 100644 --- a/packages/eslint-plugin/src/util/types.ts +++ b/packages/eslint-plugin/src/util/types.ts @@ -305,7 +305,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 6480d5489062..9571d0292b44 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 @@ -63,6 +63,42 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { ], output: 'true;', }, + { + code: 'true !== true;', + errors: [ + { + messageId: 'negated', + }, + ], + output: '!true;', + }, + { + code: 'false === false;', + errors: [ + { + messageId: 'direct', + }, + ], + output: '!false;', + }, + { + code: 'false !== false;', + errors: [ + { + messageId: 'negated', + }, + ], + output: 'false;', + }, + { + code: 'false === true;', + errors: [ + { + messageId: 'direct', + }, + ], + output: 'false;', + }, { code: 'false !== true;', errors: [ @@ -106,5 +142,95 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { } `, }, + { + code: ` + declare const varTrueOrUndefined: true | undefined; + if (varTrueOrUndefined === true) { + } + `, + options: [{ allowComparingNullableBooleans: true }], + errors: [ + { + messageId: 'comparingNullableToTrueDirect', + }, + ], + output: ` + declare const varTrueOrUndefined: true | undefined; + if (varTrueOrUndefined) { + } + `, + }, + { + code: ` + declare const varTrueOrNull: true | null; + if (varTrueOrNull !== true) { + } + `, + options: [{ allowComparingNullableBooleans: true }], + errors: [ + { + messageId: 'comparingNullableToTrueNegated', + }, + ], + output: ` + declare const varTrueOrNull: true | null; + if (!varTrueOrNull) { + } + `, + }, + { + code: ` + declare const varBooleanOrNull: boolean | null; + if (varBooleanOrNull === false) { + } + `, + options: [{ allowComparingNullableBooleans: true }], + errors: [ + { + messageId: 'comparingNullableToFalse', + }, + ], + output: ` + declare const varBooleanOrNull: boolean | null; + if (!(varBooleanOrNull ?? true)) { + } + `, + }, + { + code: ` + declare const varBooleanOrNull: boolean | null; + if (!(varBooleanOrNull === false)) { + } + `, + options: [{ allowComparingNullableBooleans: true }], + errors: [ + { + messageId: 'comparingNullableToFalse', + }, + ], + output: ` + declare const varBooleanOrNull: boolean | null; + if (varBooleanOrNull ?? true) { + } + `, + }, + { + code: ` + declare const varTrueOrFalseOrUndefined: true | false | undefined; + if (varTrueOrFalseOrUndefined !== false) { + } + `, + options: [{ allowComparingNullableBooleans: true }], + errors: [ + { + messageId: 'comparingNullableToFalse', + }, + ], + output: ` + declare const varTrueOrFalseOrUndefined: true | false | undefined; + if (varTrueOrFalseOrUndefined ?? true) { + } + `, + }, ], }); From 4b76661b1ff9cf1cf19155e1c23a6b293b763b86 Mon Sep 17 00:00:00 2001 From: Zach Kirsch Date: Wed, 6 May 2020 17:31:59 -0400 Subject: [PATCH 02/10] Self CR --- .../no-unnecessary-boolean-literal-compare.ts | 12 ++--- ...nnecessary-boolean-literal-compare.test.ts | 54 ++++--------------- 2 files changed, 15 insertions(+), 51 deletions(-) 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 b6b3a218baa8..566006a8114c 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 @@ -76,7 +76,7 @@ export default util.createRule({ const parserServices = util.getParserServices(context); const checker = parserServices.program.getTypeChecker(); - function getBooleanComparisonWithDefault( + function getBooleanComparison( node: TSESTree.BinaryExpression, ): BooleanComparisonWithTypeInformation | undefined { const comparison = deconstructComparison(node); @@ -198,20 +198,20 @@ export default util.createRule({ return { BinaryExpression(node): void { - const comparison = getBooleanComparisonWithDefault(node); // pass optional config + const comparison = getBooleanComparison(node); if (comparison === undefined) { return; } if ( comparison.expressionIsNullableBoolean && - !options.allowComparingNullableBooleans + options.allowComparingNullableBooleans ) { return; } context.report({ - fix: function* (fixer) { + fix: function*(fixer) { yield fixer.removeRange(comparison.range); // if the expression `exp` isn't nullable, or we're comparing to `true`, @@ -232,10 +232,10 @@ export default util.createRule({ // provide the default `true` yield fixer.insertTextAfter(node, ' ?? true'); - // if we're doing `=== false`, the we need to negate the expression + // 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 get rid of the parent's negation. + // 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 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 9571d0292b44..323f77101fae 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 @@ -63,42 +63,6 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { ], output: 'true;', }, - { - code: 'true !== true;', - errors: [ - { - messageId: 'negated', - }, - ], - output: '!true;', - }, - { - code: 'false === false;', - errors: [ - { - messageId: 'direct', - }, - ], - output: '!false;', - }, - { - code: 'false !== false;', - errors: [ - { - messageId: 'negated', - }, - ], - output: 'false;', - }, - { - code: 'false === true;', - errors: [ - { - messageId: 'direct', - }, - ], - output: 'false;', - }, { code: 'false !== true;', errors: [ @@ -148,7 +112,7 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { if (varTrueOrUndefined === true) { } `, - options: [{ allowComparingNullableBooleans: true }], + options: [{ allowComparingNullableBooleans: false }], errors: [ { messageId: 'comparingNullableToTrueDirect', @@ -162,19 +126,19 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { }, { code: ` - declare const varTrueOrNull: true | null; - if (varTrueOrNull !== true) { + declare const varFalseOrNull: false | null; + if (varFalseOrNull !== true) { } `, - options: [{ allowComparingNullableBooleans: true }], + options: [{ allowComparingNullableBooleans: false }], errors: [ { messageId: 'comparingNullableToTrueNegated', }, ], output: ` - declare const varTrueOrNull: true | null; - if (!varTrueOrNull) { + declare const varFalseOrNull: false | null; + if (!varFalseOrNull) { } `, }, @@ -184,7 +148,7 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { if (varBooleanOrNull === false) { } `, - options: [{ allowComparingNullableBooleans: true }], + options: [{ allowComparingNullableBooleans: false }], errors: [ { messageId: 'comparingNullableToFalse', @@ -202,7 +166,7 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { if (!(varBooleanOrNull === false)) { } `, - options: [{ allowComparingNullableBooleans: true }], + options: [{ allowComparingNullableBooleans: false }], errors: [ { messageId: 'comparingNullableToFalse', @@ -220,7 +184,7 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { if (varTrueOrFalseOrUndefined !== false) { } `, - options: [{ allowComparingNullableBooleans: true }], + options: [{ allowComparingNullableBooleans: false }], errors: [ { messageId: 'comparingNullableToFalse', From 45caa4ab7f8e82b043f588dfd1c5ee46bf1ac17e Mon Sep 17 00:00:00 2001 From: Zach Kirsch Date: Wed, 6 May 2020 17:33:22 -0400 Subject: [PATCH 03/10] Run prettier --- .../src/rules/no-unnecessary-boolean-literal-compare.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 566006a8114c..747dd544ff74 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 @@ -211,7 +211,7 @@ export default util.createRule({ } context.report({ - fix: function*(fixer) { + fix: function* (fixer) { yield fixer.removeRange(comparison.range); // if the expression `exp` isn't nullable, or we're comparing to `true`, From 7ea1f85461c4f1d447d198cfcb835e42c8487960 Mon Sep 17 00:00:00 2001 From: Zach Kirsch Date: Wed, 6 May 2020 18:16:13 -0400 Subject: [PATCH 04/10] Improve test coverage --- .../no-unnecessary-boolean-literal-compare.test.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) 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 323f77101fae..09a2bef33e07 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,13 +38,21 @@ 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; From 9dbfd4c80b550ef63561a5f4bb9b3b63e52a2965 Mon Sep 17 00:00:00 2001 From: Zach Kirsch Date: Wed, 6 May 2020 22:12:27 -0400 Subject: [PATCH 05/10] Add surrounding parentheses when adding ?? --- .../no-unnecessary-boolean-literal-compare.ts | 10 +++++----- ...unnecessary-boolean-literal-compare.test.ts | 18 ++++++++++++------ 2 files changed, 17 insertions(+), 11 deletions(-) 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 747dd544ff74..7c531a9f7210 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 @@ -229,9 +229,6 @@ export default util.createRule({ // if we're here, then the expression is a nullable boolean and we're // comparing to a literal `false` - // provide the default `true` - yield fixer.insertTextAfter(node, ' ?? true'); - // if we're doing `== false` or `=== false`, then we need to negate the expression if (!comparison.negated) { const { parent } = node; @@ -243,10 +240,13 @@ export default util.createRule({ // 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, '!('); - yield fixer.insertTextAfter(node, ')'); + yield fixer.insertTextBefore(node, '!'); } } + + // provide the default `true` + yield fixer.insertTextBefore(node, '('); + yield fixer.insertTextAfter(node, ' ?? true)'); }, messageId: comparison.expressionIsNullableBoolean ? comparison.literalBooleanInComparison 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 09a2bef33e07..e998e3384bd0 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 @@ -153,7 +153,8 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { { code: ` declare const varBooleanOrNull: boolean | null; - if (varBooleanOrNull === false) { + declare const otherBoolean: boolean; + if (varBooleanOrNull === false && otherBoolean) { } `, options: [{ allowComparingNullableBooleans: false }], @@ -164,14 +165,16 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { ], output: ` declare const varBooleanOrNull: boolean | null; - if (!(varBooleanOrNull ?? true)) { + declare const otherBoolean: boolean; + if (!(varBooleanOrNull ?? true) && otherBoolean) { } `, }, { code: ` declare const varBooleanOrNull: boolean | null; - if (!(varBooleanOrNull === false)) { + declare const otherBoolean: boolean; + if (!(varBooleanOrNull === false) || otherBoolean) { } `, options: [{ allowComparingNullableBooleans: false }], @@ -182,14 +185,16 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { ], output: ` declare const varBooleanOrNull: boolean | null; - if (varBooleanOrNull ?? true) { + declare const otherBoolean: boolean; + if ((varBooleanOrNull ?? true) || otherBoolean) { } `, }, { code: ` declare const varTrueOrFalseOrUndefined: true | false | undefined; - if (varTrueOrFalseOrUndefined !== false) { + declare const otherBoolean: boolean; + if (varTrueOrFalseOrUndefined !== false && !otherBoolean) { } `, options: [{ allowComparingNullableBooleans: false }], @@ -200,7 +205,8 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { ], output: ` declare const varTrueOrFalseOrUndefined: true | false | undefined; - if (varTrueOrFalseOrUndefined ?? true) { + declare const otherBoolean: boolean; + if ((varTrueOrFalseOrUndefined ?? true) && !otherBoolean) { } `, }, From 56d8497113ead14bfe1ff73bcec10c43f9b4e833 Mon Sep 17 00:00:00 2001 From: zachkirsch Date: Fri, 19 Jun 2020 13:49:43 -0400 Subject: [PATCH 06/10] Update packages/eslint-plugin/src/rules/no-unnecessary-boolean-literal-compare.ts Co-authored-by: Brad Zacher --- .../src/rules/no-unnecessary-boolean-literal-compare.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 7c531a9f7210..2504c216f493 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 @@ -69,7 +69,7 @@ export default util.createRule({ }, defaultOptions: [ { - allowComparingNullableBooleans: true, + allowComparingNullableBooleans: false, }, ], create(context, [options]) { From e92fb36b1f906a2b52a8b325e15272ff9655367a Mon Sep 17 00:00:00 2001 From: zachkirsch Date: Fri, 19 Jun 2020 13:49:50 -0400 Subject: [PATCH 07/10] Update packages/eslint-plugin/docs/rules/no-unnecessary-boolean-literal-compare.md Co-authored-by: Brad Zacher --- .../docs/rules/no-unnecessary-boolean-literal-compare.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 485b9fedafc4..eb3e5eb8a230 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 @@ -48,7 +48,7 @@ type Options = { }; const defaults = { - allowComparingNullableBooleans: true, + allowComparingNullableBooleans: false, }; ``` From a7b2041910431847fc37d91d13835278a02780db Mon Sep 17 00:00:00 2001 From: Zach Kirsch Date: Fri, 19 Jun 2020 14:10:36 -0400 Subject: [PATCH 08/10] Default allowComparingNullableBooleans to true (back-compat) --- .../docs/rules/no-unnecessary-boolean-literal-compare.md | 2 +- .../src/rules/no-unnecessary-boolean-literal-compare.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 eb3e5eb8a230..485b9fedafc4 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 @@ -48,7 +48,7 @@ type Options = { }; const defaults = { - allowComparingNullableBooleans: false, + allowComparingNullableBooleans: true, }; ``` 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 2504c216f493..7c531a9f7210 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 @@ -69,7 +69,7 @@ export default util.createRule({ }, defaultOptions: [ { - allowComparingNullableBooleans: false, + allowComparingNullableBooleans: true, }, ], create(context, [options]) { From acd9d446da24ee48a1b3a6fdf58e95f7b4ab3376 Mon Sep 17 00:00:00 2001 From: Zach Kirsch Date: Fri, 19 Jun 2020 16:54:29 -0400 Subject: [PATCH 09/10] Add separate options for allowing nullable comparisons to true/false --- .../no-unnecessary-boolean-literal-compare.md | 79 +++++++++++++++---- .../no-unnecessary-boolean-literal-compare.ts | 29 +++++-- ...nnecessary-boolean-literal-compare.test.ts | 24 ++++-- 3 files changed, 105 insertions(+), 27 deletions(-) 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 485b9fedafc4..9b9784bdc562 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,31 +36,37 @@ if (someObjectBoolean === true) { declare const someStringBoolean: boolean | string; if (someStringBoolean === true) { } - -declare const someUndefinedCondition: boolean | undefined; -if (someUndefinedCondition === false) { -} ``` ## Options -The rule accepts an options object with the following properties: +The rule accepts an options object with the following properties. ```ts type Options = { - // if true, only comparisons that compare a boolean literal to a boolean will be checked. - // if false, comparisons that compare a boolean literal to a nullable boolean variable will also be checked - allowComparingNullableBooleans?: boolean; + // 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 comparions between a boolean variable and a boolean +literal. Comparisons between nullable boolean variables and boolean literals +are **not** checked by default. +```ts const defaults = { - allowComparingNullableBooleans: true, + allowComparingNullableBooleansToTrue: true, + allowComparingNullableBooleansToFalse: true, }; ``` -### `allowComparingNullableBooleans` +### `allowComparingNullableBooleansToTrue` -Examples of **incorrect** code for this rule with `{ allowComparingNullableBooleans: false }`: +Examples of **incorrect** code for this rule with `{ allowComparingNullableBooleansToTrue: false }`: ```ts declare const someUndefinedCondition: boolean | undefined; @@ -62,11 +74,11 @@ if (someUndefinedCondition === true) { } declare const someNullCondition: boolean | null; -if (someNullCondition !== false) { +if (someNullCondition !== true) { } ``` -Examples of **correct** code for this rule with `{ allowComparingNullableBooleans: false }`: +Examples of **correct** code for this rule with `{ allowComparingNullableBooleansToTrue: false }`: ```ts declare const someUndefinedCondition: boolean | undefined; @@ -74,10 +86,49 @@ if (someUndefinedCondition) { } declare const someNullCondition: boolean | null; -if (someNullCondition ?? true) { +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 7c531a9f7210..fe1fd24cc4f1 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 @@ -15,7 +15,8 @@ type MessageIds = type Options = [ { - allowComparingNullableBooleans?: boolean; + allowComparingNullableBooleansToTrue?: boolean; + allowComparingNullableBooleansToFalse?: boolean; }, ]; @@ -58,7 +59,10 @@ export default util.createRule({ { type: 'object', properties: { - allowComparingNullableBooleans: { + allowComparingNullableBooleansToTrue: { + type: 'boolean', + }, + allowComparingNullableBooleansToFalse: { type: 'boolean', }, }, @@ -69,7 +73,8 @@ export default util.createRule({ }, defaultOptions: [ { - allowComparingNullableBooleans: true, + allowComparingNullableBooleansToTrue: true, + allowComparingNullableBooleansToFalse: true, }, ], create(context, [options]) { @@ -203,11 +208,19 @@ export default util.createRule({ return; } - if ( - comparison.expressionIsNullableBoolean && - options.allowComparingNullableBooleans - ) { - return; + if (comparison.expressionIsNullableBoolean) { + if ( + comparison.literalBooleanInComparison && + options.allowComparingNullableBooleansToTrue + ) { + return; + } + if ( + !comparison.literalBooleanInComparison && + options.allowComparingNullableBooleansToFalse + ) { + return; + } } context.report({ 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 e998e3384bd0..36eb36033de1 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 @@ -57,6 +57,20 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { 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;", ], @@ -120,7 +134,7 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { if (varTrueOrUndefined === true) { } `, - options: [{ allowComparingNullableBooleans: false }], + options: [{ allowComparingNullableBooleansToTrue: false }], errors: [ { messageId: 'comparingNullableToTrueDirect', @@ -138,7 +152,7 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { if (varFalseOrNull !== true) { } `, - options: [{ allowComparingNullableBooleans: false }], + options: [{ allowComparingNullableBooleansToTrue: false }], errors: [ { messageId: 'comparingNullableToTrueNegated', @@ -157,7 +171,7 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { if (varBooleanOrNull === false && otherBoolean) { } `, - options: [{ allowComparingNullableBooleans: false }], + options: [{ allowComparingNullableBooleansToFalse: false }], errors: [ { messageId: 'comparingNullableToFalse', @@ -177,7 +191,7 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { if (!(varBooleanOrNull === false) || otherBoolean) { } `, - options: [{ allowComparingNullableBooleans: false }], + options: [{ allowComparingNullableBooleansToFalse: false }], errors: [ { messageId: 'comparingNullableToFalse', @@ -197,7 +211,7 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { if (varTrueOrFalseOrUndefined !== false && !otherBoolean) { } `, - options: [{ allowComparingNullableBooleans: false }], + options: [{ allowComparingNullableBooleansToFalse: false }], errors: [ { messageId: 'comparingNullableToFalse', From a145644cb0910ad011fdcc006ead04eb6803aaae Mon Sep 17 00:00:00 2001 From: Zach Kirsch Date: Fri, 19 Jun 2020 17:01:21 -0400 Subject: [PATCH 10/10] Fix spelling --- .../docs/rules/no-unnecessary-boolean-literal-compare.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 9b9784bdc562..d600bb7930c0 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 @@ -53,7 +53,7 @@ type Options = { ### Defaults -This rule always checks comparions between a boolean variable and a boolean +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.