From 018b28b04bc81ad08874b70e415ee90147d2d0f7 Mon Sep 17 00:00:00 2001 From: Armano Date: Mon, 13 Mar 2023 21:03:15 +0100 Subject: [PATCH 1/2] fix: [no-unnecessary-boolean-literal-compare] simplify fixer and add support for double negation --- .../no-unnecessary-boolean-literal-compare.ts | 66 +++++----- ...nnecessary-boolean-literal-compare.test.ts | 119 ++++++++++++++++++ 2 files changed, 149 insertions(+), 36 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 f14b44fec275..fd1ea302dd7d 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 @@ -22,7 +22,6 @@ type Options = [ interface BooleanComparison { expression: TSESTree.Expression | TSESTree.PrivateIdentifier; literalBooleanInComparison: boolean; - forTruthy: boolean; negated: boolean; } @@ -182,7 +181,6 @@ export default util.createRule({ return { literalBooleanInComparison, - forTruthy: literalBooleanInComparison ? !negated : negated, expression, negated, }; @@ -223,48 +221,44 @@ export default util.createRule({ context.report({ fix: function* (fixer) { + // 1. isUnaryNegation - parent negation + // 2. literalBooleanInComparison - is compared to literal boolean + // 3. negated - is expression negated + + const isUnaryNegation = + node.parent != null && nodeIsUnaryNegation(node.parent); + + const shouldNegate = comparison.literalBooleanInComparison + ? !comparison.negated + : comparison.negated; + + const mutatedNode = isUnaryNegation ? node.parent! : node; + yield fixer.replaceText( - node, + mutatedNode, sourceCode.getText(comparison.expression), ); - // 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, '!'); - if (!util.isStrongPrecedenceNode(comparison.expression)) { - yield fixer.insertTextBefore(node, '('); - yield fixer.insertTextAfter(node, ')'); - } - } - return; - } + // if (XOR) `isUnaryNegation ^ literalBooleanInComparison ^ negated` is true - negate the expression + if (isUnaryNegation === shouldNegate) { + yield fixer.insertTextBefore(mutatedNode, '!'); - // 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, '!'); + // if the expression `exp` is not a strong precedence node, wrap it in parentheses + if (!util.isStrongPrecedenceNode(comparison.expression)) { + yield fixer.insertTextBefore(mutatedNode, '('); + yield fixer.insertTextAfter(mutatedNode, ')'); } } - // provide the default `true` - yield fixer.insertTextBefore(node, '('); - yield fixer.insertTextAfter(node, ' ?? true)'); + // if the expression `exp` is nullable, and we're not comparing to `true`, insert `?? true` + if ( + comparison.expressionIsNullableBoolean && + !comparison.literalBooleanInComparison + ) { + // provide the default `true` + yield fixer.insertTextBefore(mutatedNode, '('); + yield fixer.insertTextAfter(mutatedNode, ' ?? 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 24dce35c7990..7c22e7d8d159 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 @@ -360,5 +360,124 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, { } `, }, + { + code: ` + declare const varBoolean: boolean; + if (!(varBoolean !== false)) { + } + `, + errors: [ + { + messageId: 'negated', + }, + ], + output: ` + declare const varBoolean: boolean; + if (!varBoolean) { + } + `, + }, + { + code: ` + declare const varBoolean: boolean; + if (!(varBoolean === false)) { + } + `, + errors: [ + { + messageId: 'direct', + }, + ], + output: ` + declare const varBoolean: boolean; + if (varBoolean) { + } + `, + }, + { + code: ` + declare const varBoolean: boolean; + if (!(varBoolean instanceof Event == false)) { + } + `, + errors: [ + { + messageId: 'direct', + }, + ], + output: ` + declare const varBoolean: boolean; + if (varBoolean instanceof Event) { + } + `, + }, + { + code: ` + declare const varBoolean: boolean; + if (varBoolean instanceof Event == false) { + } + `, + errors: [ + { + messageId: 'direct', + }, + ], + output: ` + declare const varBoolean: boolean; + if (!(varBoolean instanceof Event)) { + } + `, + }, + { + code: ` + declare const varBoolean: boolean; + if (!((varBoolean ?? false) !== false)) { + } + `, + errors: [ + { + messageId: 'negated', + }, + ], + output: ` + declare const varBoolean: boolean; + if (!(varBoolean ?? false)) { + } + `, + }, + { + code: ` + declare const varBoolean: boolean; + if (!((varBoolean ?? false) === false)) { + } + `, + errors: [ + { + messageId: 'direct', + }, + ], + output: ` + declare const varBoolean: boolean; + if (varBoolean ?? false) { + } + `, + }, + { + code: ` + declare const varBoolean: boolean; + if (!((varBoolean ?? true) !== false)) { + } + `, + errors: [ + { + messageId: 'negated', + }, + ], + output: ` + declare const varBoolean: boolean; + if (!(varBoolean ?? true)) { + } + `, + }, ], }); From 0c941939731e910bc8404fc3af12b74eac7c7921 Mon Sep 17 00:00:00 2001 From: Armano Date: Mon, 13 Mar 2023 21:15:54 +0100 Subject: [PATCH 2/2] fix: correct comment simplify condition --- .../src/rules/no-unnecessary-boolean-literal-compare.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 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 fd1ea302dd7d..ae6c56e82be5 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 @@ -228,9 +228,8 @@ export default util.createRule({ const isUnaryNegation = node.parent != null && nodeIsUnaryNegation(node.parent); - const shouldNegate = comparison.literalBooleanInComparison - ? !comparison.negated - : comparison.negated; + const shouldNegate = + comparison.negated !== comparison.literalBooleanInComparison; const mutatedNode = isUnaryNegation ? node.parent! : node; @@ -239,8 +238,8 @@ export default util.createRule({ sourceCode.getText(comparison.expression), ); - // if (XOR) `isUnaryNegation ^ literalBooleanInComparison ^ negated` is true - negate the expression - if (isUnaryNegation === shouldNegate) { + // if `isUnaryNegation === literalBooleanInComparison === !negated` is true - negate the expression + if (shouldNegate === isUnaryNegation) { yield fixer.insertTextBefore(mutatedNode, '!'); // if the expression `exp` is not a strong precedence node, wrap it in parentheses