From 2450134188e98382724121b1a355eedbf24d7d0f Mon Sep 17 00:00:00 2001 From: Zach Kirsch Date: Wed, 24 Jun 2020 17:39:31 -0400 Subject: [PATCH 1/4] feat(eslint-plugin): [no-unnecessary-boolean-literal-compare] add null check option to fixer --- .../no-unnecessary-boolean-literal-compare.md | 50 ++++++-- .../no-unnecessary-boolean-literal-compare.ts | 84 ++++++++++++- ...nnecessary-boolean-literal-compare.test.ts | 112 ++++++++++++++++++ 3 files changed, 233 insertions(+), 13 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 d600bb7930c0..2ce8a07f36e5 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 @@ -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 @@ -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, }; ``` @@ -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 Expresssions + +The follow table shows the output of the fixer on comparions 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 comparions 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 comparions 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) 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 fe1fd24cc4f1..a5aac76fd7c1 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 @@ -17,6 +17,9 @@ type Options = [ { allowComparingNullableBooleansToTrue?: boolean; allowComparingNullableBooleansToFalse?: boolean; + fixWithExplicitNullishCheck?: { + nullishSymbol: 'null' | 'undefined'; + }; }, ]; @@ -65,6 +68,14 @@ export default util.createRule({ allowComparingNullableBooleansToFalse: { type: 'boolean', }, + fixWithExplicitNullishCheck: { + type: 'object', + properties: { + nullishSymbol: { + type: 'string', + }, + }, + }, }, additionalProperties: false, }, @@ -201,6 +212,36 @@ export default util.createRule({ ); } + 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); @@ -225,9 +266,41 @@ export default util.createRule({ 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 eplacing 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 || @@ -239,10 +312,13 @@ export default util.createRule({ 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. 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 36eb36033de1..380828d0a4bc 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 @@ -224,5 +224,117 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { } `, }, + { + code: ` + function getBooleanOrNull(): false | null { + Math.random() > 0.5 ? false : null; + } + if (getBooleanOrNull() === true) { + } + `, + options: [ + { + allowComparingNullableBooleansToTrue: false, + fixWithExplicitNullishCheck: { + nullishSymbol: 'null', + }, + }, + ], + errors: [ + { + messageId: 'comparingNullableToTrueDirect', + }, + ], + // can't fix because fixing would call the function twice: + // `getBooleanOrNull() != null && getBooleanOrNull()` + output: ` + function getBooleanOrNull(): false | null { + Math.random() > 0.5 ? false : null; + } + if (getBooleanOrNull() === true) { + } + `, + }, + { + code: ` + declare const varFalseOrNull: false | null; + if (varFalseOrNull !== true) { + } + `, + options: [ + { + allowComparingNullableBooleansToTrue: false, + fixWithExplicitNullishCheck: { + nullishSymbol: 'undefined', + }, + }, + ], + errors: [ + { + messageId: 'comparingNullableToTrueNegated', + }, + ], + // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting + output: ` + declare const varFalseOrNull: false | null; + if ((varFalseOrNull == undefined || !varFalseOrNull)) { + } + `, + }, + { + code: ` + declare const varBooleanOrNull: boolean | null; + declare const otherBoolean: boolean; + if (varBooleanOrNull === false && otherBoolean) { + } + `, + options: [ + { + allowComparingNullableBooleansToFalse: false, + fixWithExplicitNullishCheck: { + nullishSymbol: 'null', + }, + }, + ], + errors: [ + { + messageId: 'comparingNullableToFalse', + }, + ], + // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting + output: ` + declare const varBooleanOrNull: boolean | null; + declare const otherBoolean: boolean; + if ((varBooleanOrNull != null && !varBooleanOrNull) && otherBoolean) { + } + `, + }, + { + code: ` + declare const varBoolOrUndefined: true | false | undefined; + declare const otherBoolean: boolean; + if (varBoolOrUndefined !== false && !otherBoolean) { + } + `, + options: [ + { + allowComparingNullableBooleansToFalse: false, + fixWithExplicitNullishCheck: { + nullishSymbol: 'null', + }, + }, + ], + errors: [ + { + messageId: 'comparingNullableToFalse', + }, + ], + output: ` + declare const varBoolOrUndefined: true | false | undefined; + declare const otherBoolean: boolean; + if ((varBoolOrUndefined == null || varBoolOrUndefined) && !otherBoolean) { + } + `, + }, ], }); From 08958ad20e6caadb215e4ab44afd7529ee1ade27 Mon Sep 17 00:00:00 2001 From: Zach Kirsch Date: Wed, 24 Jun 2020 17:47:57 -0400 Subject: [PATCH 2/4] Escape pipes --- .../rules/no-unnecessary-boolean-literal-compare.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 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 2ce8a07f36e5..aa9d1c06854e 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 @@ -154,12 +154,12 @@ The follow table shows the output of the fixer on comparions that compare a bool The follow table shows the output of the fixer on comparions 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` | +| 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 From 3c2dffc20aaf012a8f51f9ca3c2f2d305963ec49 Mon Sep 17 00:00:00 2001 From: Zach Kirsch Date: Wed, 24 Jun 2020 19:29:03 -0400 Subject: [PATCH 3/4] Fix spelling --- .../docs/rules/no-unnecessary-boolean-literal-compare.md | 8 ++++---- .../src/rules/no-unnecessary-boolean-literal-compare.ts | 2 +- 2 files changed, 5 insertions(+), 5 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 aa9d1c06854e..e2cda8643d00 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 @@ -130,9 +130,9 @@ if (someNullCondition == undefined || someNullCondition) { ## Fixer -### Boolean Expresssions +### Boolean Expressions -The follow table shows the output of the fixer on comparions that compare a boolean expression to a boolean literal. +The follow table shows the output of the fixer on comparisons that compare a boolean expression to a boolean literal. | Comparison | Fixer Output | | :--------------------: | ----------------- | @@ -143,7 +143,7 @@ The follow table shows the output of the fixer on comparions that compare a bool ### Nullable Boolean Expressions -The follow table shows the output of the fixer on comparions that compare a boolean expression to a boolean literal when the `fixWithExplicitNullishCheck` option is `undefined`: +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 | | :----------------------------: | ------------------------------- | ----------------------------------------------------------------------------------- | @@ -152,7 +152,7 @@ The follow table shows the output of the fixer on comparions that compare a bool | `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 comparions that compare a boolean expression to a boolean literal when the `fixWithExplicitNullishCheck` option is `{ nullishSymbol: "null" }`: +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 | | :----------------------------: | ----------------------------------------------------- | ----------------------------------------------------------------------------------- | 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 a5aac76fd7c1..3e3c37c57b06 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 @@ -275,7 +275,7 @@ export default util.createRule({ // `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 eplacing one function call with two) + // 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); From 4013b15b8e12c057abe5b7a7ba289789e0fe48ea Mon Sep 17 00:00:00 2001 From: Zach Kirsch Date: Thu, 25 Jun 2020 00:14:12 -0400 Subject: [PATCH 4/4] Add more tests --- ...nnecessary-boolean-literal-compare.test.ts | 86 ++++++++++++++++++- 1 file changed, 82 insertions(+), 4 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 380828d0a4bc..1d591070f709 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 @@ -257,8 +257,34 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { }, { code: ` - declare const varFalseOrNull: false | null; - if (varFalseOrNull !== true) { + declare const varBoolOrNull: boolean | null; + if (varBoolOrNull === true) { + } + `, + options: [ + { + allowComparingNullableBooleansToTrue: false, + fixWithExplicitNullishCheck: { + nullishSymbol: 'undefined', + }, + }, + ], + errors: [ + { + messageId: 'comparingNullableToTrueDirect', + }, + ], + // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting + output: ` + declare const varBoolOrNull: boolean | null; + if ((varBoolOrNull != undefined && varBoolOrNull)) { + } + `, + }, + { + code: ` + declare const varBoolOrNull: boolean | null; + if (varBoolOrNull !== true) { } `, options: [ @@ -276,8 +302,60 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { ], // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting output: ` - declare const varFalseOrNull: false | null; - if ((varFalseOrNull == undefined || !varFalseOrNull)) { + declare const varBoolOrNull: boolean | null; + if ((varBoolOrNull == undefined || !varBoolOrNull)) { + } + `, + }, + { + code: ` + declare const varBoolOrNull: boolean | null; + if (varBoolOrNull === false) { + } + `, + options: [ + { + allowComparingNullableBooleansToFalse: false, + fixWithExplicitNullishCheck: { + nullishSymbol: 'undefined', + }, + }, + ], + errors: [ + { + messageId: 'comparingNullableToFalse', + }, + ], + // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting + output: ` + declare const varBoolOrNull: boolean | null; + if ((varBoolOrNull != undefined && !varBoolOrNull)) { + } + `, + }, + { + code: ` + declare const varBoolOrNull: boolean | null; + if (varBoolOrNull !== false) { + } + `, + options: [ + { + allowComparingNullableBooleansToFalse: false, + fixWithExplicitNullishCheck: { + nullishSymbol: 'undefined', + }, + }, + ], + errors: [ + { + messageId: 'comparingNullableToFalse', + }, + ], + // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting + output: ` + declare const varBoolOrNull: boolean | null; + if ((varBoolOrNull == undefined || varBoolOrNull)) { } `, },