Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(eslint-plugin): [no-unnecessary-boolean-literal-compare] fixer should handle parentheses #6569

Merged
merged 4 commits into from Mar 13, 2023
Merged

fix(eslint-plugin): [no-unnecessary-boolean-literal-compare] fixer should handle parentheses #6569

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bdf04c3
fix(eslint-plugin): [no-unnecessary-boolean-literal-compare] fixer sh…
armano2 Mar 4, 2023
c481273
Merge remote-tracking branch 'origin/main' into fix/no-unnecessary-bo…
armano2 Mar 4, 2023
768cc36
fix: simplify code and add missing handling for additional cases
armano2 Mar 4, 2023
79de5c2
fix: small code refactor
armano2 Mar 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 9 additions & 6 deletions packages/eslint-plugin/src/rules/no-unnecessary-boolean-literal-compare.ts
View file
Open in desktop
Expand Up @@ -24,7 +24,6 @@ interface BooleanComparison {
literalBooleanInComparison: boolean;
forTruthy: boolean;
negated: boolean;
range: [number, number];
}

interface BooleanComparisonWithTypeInformation extends BooleanComparison {
Expand Down Expand Up @@ -82,6 +81,7 @@ export default util.createRule<Options, MessageIds>({
create(context, [options]) {
const parserServices = util.getParserServices(context);
const checker = parserServices.program.getTypeChecker();
const sourceCode = context.getSourceCode();

function getBooleanComparison(
node: TSESTree.BinaryExpression,
Expand Down Expand Up @@ -185,10 +185,6 @@ export default util.createRule<Options, MessageIds>({
forTruthy: literalBooleanInComparison ? !negated : negated,
expression,
negated,
range:
expression.range[0] < against.range[0]
? [expression.range[1], against.range[1]]
: [against.range[0], expression.range[0]],
};
}

Expand Down Expand Up @@ -227,7 +223,10 @@ export default util.createRule<Options, MessageIds>({

context.report({
fix: function* (fixer) {
yield fixer.removeRange(comparison.range);
yield fixer.replaceText(
node,
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`
Copy link
Member Author

@armano2 armano2 Mar 5, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after further investigation we could simplify this fixer to

          fix: function* (fixer) {
            const isUnaryNegation =
              node.parent != null && nodeIsUnaryNegation(node.parent);

            const mutatedNode = isUnaryNegation ? node.parent! : node;

            yield fixer.replaceText(
              mutatedNode,
              sourceCode.getText(comparison.expression),
            );

            const shouldNegate = comparison.literalBooleanInComparison
              ? !comparison.negated
              : comparison.negated;

            if (isUnaryNegation === shouldNegate) {
              yield fixer.insertTextBefore(mutatedNode, '!');

              if (!util.isStrongPrecedenceNode(comparison.expression)) {
                yield fixer.insertTextBefore(mutatedNode, '(');
                yield fixer.insertTextAfter(mutatedNode, ')');
              }
            }

            // if the expression `exp` is nullable, and we're not comparing to `true`,
            // we can just replace the entire comparison with `exp` or `!exp`
            if (
              comparison.expressionIsNullableBoolean &&
              !comparison.literalBooleanInComparison
            ) {
              // provide the default `true`
              yield fixer.insertTextBefore(mutatedNode, '(');
              yield fixer.insertTextAfter(mutatedNode, ' ?? true)');
            }
          }

that should allow us to provide better fixer for

// source
if (!(varBoolean === false)) {}
// current
if (!(!varBoolean)) {}
// new
if (varBoolean) {}

i'm not sure if we should push this with this fix

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it 😄 +1 to including it here. Also a +1 to filing a followup if you want to get this in sooner.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to land this as-is for now, just so we can get this fix out with the next release.
We can follow-up next week with the changes to simplify this fixer later

Expand All @@ -237,6 +236,10 @@ export default util.createRule<Options, MessageIds>({
) {
if (!comparison.forTruthy) {
yield fixer.insertTextBefore(node, '!');
if (!util.isStrongPrecedenceNode(comparison.expression)) {
yield fixer.insertTextBefore(node, '(');
yield fixer.insertTextAfter(node, ')');
}
}
return;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/eslint-plugin/src/util/getWrappingFixer.ts
View file
Open in desktop
Expand Up @@ -69,7 +69,7 @@ export function getWrappingFixer(
/**
* Check if a node will always have the same precedence if it's parent changes.
*/
function isStrongPrecedenceNode(innerNode: TSESTree.Node): boolean {
export function isStrongPrecedenceNode(innerNode: TSESTree.Node): boolean {
return (
innerNode.type === AST_NODE_TYPES.Literal ||
innerNode.type === AST_NODE_TYPES.Identifier ||
Expand Down
104 changes: 103 additions & 1 deletion packages/eslint-plugin/tests/rules/no-unnecessary-boolean-literal-compare.test.ts
View file
Open in desktop
@@ -1,5 +1,5 @@
import rule from '../../src/rules/no-unnecessary-boolean-literal-compare';
import { getFixturesRootDir, RuleTester } from '../RuleTester';
import { getFixturesRootDir, noFormat, RuleTester } from '../RuleTester';

const rootDir = getFixturesRootDir();
const ruleTester = new RuleTester({
Expand Down Expand Up @@ -258,5 +258,107 @@ ruleTester.run('no-unnecessary-boolean-literal-compare', rule, {
}
`,
},
{
code: noFormat`
declare const x;
if ((x instanceof Error) === false) {
}
`,
errors: [
{
messageId: 'direct',
},
],
output: `
declare const x;
if (!(x instanceof Error)) {
}
`,
},
{
code: noFormat`
declare const x;
if (false === (x instanceof Error)) {
}
`,
errors: [
{
messageId: 'direct',
},
],
output: `
declare const x;
if (!(x instanceof Error)) {
}
`,
},
{
code: `
declare const x;
if (x instanceof Error === false) {
}
`,
errors: [
{
messageId: 'direct',
},
],
output: `
declare const x;
if (!(x instanceof Error)) {
}
`,
},
{
code: noFormat`
declare const x;
if (typeof x === 'string' === false) {
}
`,
errors: [
{
messageId: 'direct',
},
],
output: `
declare const x;
if (!(typeof x === 'string')) {
}
`,
},
{
code: noFormat`
declare const x;
if (x instanceof Error === (false)) {
}
`,
errors: [
{
messageId: 'direct',
},
],
output: `
declare const x;
if (!(x instanceof Error)) {
}
`,
},
{
code: noFormat`
declare const x;
if ((false) === x instanceof Error) {
}
`,
errors: [
{
messageId: 'direct',
},
],
output: `
declare const x;
if (!(x instanceof Error)) {
}
`,
},
],
});