From 0d2e7ad560be49447a1ada8a6d22bd3af2b60372 Mon Sep 17 00:00:00 2001 From: nissy-dev Date: Wed, 25 Jan 2023 23:18:11 +0900 Subject: [PATCH 1/8] feat: update rule for unnecessary nullish coalescing operator --- lib/rules/no-constant-binary-expression.js | 6 ++++++ tests/lib/rules/no-constant-binary-expression.js | 8 +++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/rules/no-constant-binary-expression.js b/lib/rules/no-constant-binary-expression.js index dccfa2f5826..0fa0029c3d7 100644 --- a/lib/rules/no-constant-binary-expression.js +++ b/lib/rules/no-constant-binary-expression.js @@ -45,6 +45,12 @@ function hasConstantNullishness(scope, node) { return (functionName === "Boolean" || functionName === "String" || functionName === "Number") && isReferenceToGlobalVariable(scope, node.callee); } + case "LogicalExpression": { + const isLeftConstant = hasConstantNullishness(scope, node.left); + const isRightConstant = hasConstantNullishness(scope, node.right); + + return isLeftConstant || isRightConstant; + } case "AssignmentExpression": if (node.operator === "=") { return hasConstantNullishness(scope, node.right); diff --git a/tests/lib/rules/no-constant-binary-expression.js b/tests/lib/rules/no-constant-binary-expression.js index c430c7787fd..c44fe499120 100644 --- a/tests/lib/rules/no-constant-binary-expression.js +++ b/tests/lib/rules/no-constant-binary-expression.js @@ -308,6 +308,12 @@ ruleTester.run("no-constant-binary-expression", rule, { { code: "x === /[a-z]/", errors: [{ messageId: "alwaysNew" }] }, // It's not obvious what this does, but it compares the old value of `x` to the new object. - { code: "x === (x = {})", errors: [{ messageId: "alwaysNew" }] } + { code: "x === (x = {})", errors: [{ messageId: "alwaysNew" }] }, + + { code: "window.abc && false && anything", errors: [{ messageId: "constantShortCircuit" }] }, + { code: "window.abc || true || anything", errors: [{ messageId: "constantShortCircuit" }] }, + { code: "window.abc ?? 'non-nullish' ?? 'non-nullish'", errors: [{ messageId: "constantShortCircuit" }] }, + { code: "window.abc ?? undefined ?? 'non-nullish'", errors: [{ messageId: "constantShortCircuit" }] }, + { code: "window.abc ?? null ?? 'non-nullish'", errors: [{ messageId: "constantShortCircuit" }] } ] }); From 8971e90fc96413ef2f155458d4b5c898f6fd875d Mon Sep 17 00:00:00 2001 From: nissy-dev Date: Thu, 26 Jan 2023 23:18:22 +0900 Subject: [PATCH 2/8] docs: add incorrect cases --- docs/src/rules/no-constant-binary-expression.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/src/rules/no-constant-binary-expression.md b/docs/src/rules/no-constant-binary-expression.md index 969586c6f7d..e348045563d 100644 --- a/docs/src/rules/no-constant-binary-expression.md +++ b/docs/src/rules/no-constant-binary-expression.md @@ -53,6 +53,12 @@ const value4 = new Boolean(foo) === true; const objIsEmpty = someObj === {}; const arrIsEmpty = someArr === []; + +const shortCircuit1 = condition1 && false && condition2; + +const shortCircuit2 = condition1 || true || condition2; + +const shortCircuit3 = condition1 && "non-nullish" && condition2; ``` ::: From 7c91ba89766c47e538a474f28578493790142ba3 Mon Sep 17 00:00:00 2001 From: nissy-dev Date: Sat, 4 Feb 2023 22:05:29 +0900 Subject: [PATCH 3/8] fix: update logic by review --- lib/rules/no-constant-binary-expression.js | 40 +++++++++---------- .../rules/no-constant-binary-expression.js | 8 ++-- 2 files changed, 22 insertions(+), 26 deletions(-) diff --git a/lib/rules/no-constant-binary-expression.js b/lib/rules/no-constant-binary-expression.js index 0fa0029c3d7..9aeb09277b0 100644 --- a/lib/rules/no-constant-binary-expression.js +++ b/lib/rules/no-constant-binary-expression.js @@ -14,6 +14,23 @@ const NUMERIC_OR_STRING_BINARY_OPERATORS = new Set(["+", "-", "*", "/", "%", "|" // Helpers //------------------------------------------------------------------------------ +/** + * Checks whether or not a node is `null` or `undefined`. Similar to the one + * found in ast-utils.js, but this one correctly handles the edge case that + * `undefined` has been redefined. + * @param {Scope} scope Scope in which the expression was found. + * @param {ASTNode} node A node to check. + * @returns {boolean} Whether or not the node is a `null` or `undefined`. + * @public + */ +function isNullOrUndefined(scope, node) { + return ( + isNullLiteral(node) || + (node.type === "Identifier" && node.name === "undefined" && isReferenceToGlobalVariable(scope, node)) || + (node.type === "UnaryExpression" && node.operator === "void") + ); +} + /** * Test if an AST node has a statically knowable constant nullishness. Meaning, * it will always resolve to a constant value of either: `null`, `undefined` @@ -46,10 +63,7 @@ function hasConstantNullishness(scope, node) { isReferenceToGlobalVariable(scope, node.callee); } case "LogicalExpression": { - const isLeftConstant = hasConstantNullishness(scope, node.left); - const isRightConstant = hasConstantNullishness(scope, node.right); - - return isLeftConstant || isRightConstant; + return node.operator === "??" && (hasConstantNullishness(scope, node.right) && !isNullOrUndefined(scope, node.right)); } case "AssignmentExpression": if (node.operator === "=") { @@ -384,24 +398,6 @@ function isAlwaysNew(scope, node) { } } -/** - * Checks whether or not a node is `null` or `undefined`. Similar to the one - * found in ast-utils.js, but this one correctly handles the edge case that - * `undefined` has been redefined. - * @param {Scope} scope Scope in which the expression was found. - * @param {ASTNode} node A node to check. - * @returns {boolean} Whether or not the node is a `null` or `undefined`. - * @public - */ -function isNullOrUndefined(scope, node) { - return ( - isNullLiteral(node) || - (node.type === "Identifier" && node.name === "undefined" && isReferenceToGlobalVariable(scope, node)) || - (node.type === "UnaryExpression" && node.operator === "void") - ); -} - - /** * Checks if one operand will cause the result to be constant. * @param {Scope} scope Scope in which the expression was found. diff --git a/tests/lib/rules/no-constant-binary-expression.js b/tests/lib/rules/no-constant-binary-expression.js index c44fe499120..5232ef813f2 100644 --- a/tests/lib/rules/no-constant-binary-expression.js +++ b/tests/lib/rules/no-constant-binary-expression.js @@ -59,7 +59,9 @@ ruleTester.run("no-constant-binary-expression", rule, { "function foo(undefined) { undefined === true;}", "[...arr, 1] == true", "[,,,] == true", - { code: "new Foo() === bar;", globals: { Foo: "writable" } } + { code: "new Foo() === bar;", globals: { Foo: "writable" } }, + "(foo && true) ?? bar", + "foo ?? null ?? bar" ], invalid: [ @@ -312,8 +314,6 @@ ruleTester.run("no-constant-binary-expression", rule, { { code: "window.abc && false && anything", errors: [{ messageId: "constantShortCircuit" }] }, { code: "window.abc || true || anything", errors: [{ messageId: "constantShortCircuit" }] }, - { code: "window.abc ?? 'non-nullish' ?? 'non-nullish'", errors: [{ messageId: "constantShortCircuit" }] }, - { code: "window.abc ?? undefined ?? 'non-nullish'", errors: [{ messageId: "constantShortCircuit" }] }, - { code: "window.abc ?? null ?? 'non-nullish'", errors: [{ messageId: "constantShortCircuit" }] } + { code: "window.abc ?? 'non-nullish' ?? 'non-nullish'", errors: [{ messageId: "constantShortCircuit" }] } ] }); From 623747654ab0452d6cf10ef5ada7e2bb9c454cb6 Mon Sep 17 00:00:00 2001 From: Daiki Nishikawa Date: Sat, 11 Feb 2023 16:36:56 +0900 Subject: [PATCH 4/8] Update docs/src/rules/no-constant-binary-expression.md Co-authored-by: Milos Djermanovic --- docs/src/rules/no-constant-binary-expression.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/src/rules/no-constant-binary-expression.md b/docs/src/rules/no-constant-binary-expression.md index e348045563d..792c61d529d 100644 --- a/docs/src/rules/no-constant-binary-expression.md +++ b/docs/src/rules/no-constant-binary-expression.md @@ -58,7 +58,7 @@ const shortCircuit1 = condition1 && false && condition2; const shortCircuit2 = condition1 || true || condition2; -const shortCircuit3 = condition1 && "non-nullish" && condition2; +const shortCircuit3 = condition1 ?? "non-nullish" ?? condition2; ``` ::: From 5fad3e6f03fc79a776b006ff856de3255d0611ea Mon Sep 17 00:00:00 2001 From: nissy-dev Date: Sun, 12 Feb 2023 18:24:10 +0900 Subject: [PATCH 5/8] wip --- lib/rules/no-constant-binary-expression.js | 18 ++++++++++-------- .../lib/rules/no-constant-binary-expression.js | 4 +++- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/lib/rules/no-constant-binary-expression.js b/lib/rules/no-constant-binary-expression.js index 9aeb09277b0..0c596d9c895 100644 --- a/lib/rules/no-constant-binary-expression.js +++ b/lib/rules/no-constant-binary-expression.js @@ -38,9 +38,10 @@ function isNullOrUndefined(scope, node) { * three states at runtime would return `false`. * @param {Scope} scope The scope in which the node was found. * @param {ASTNode} node The AST node being tested. + * @param {boolean} nonNullish node * @returns {boolean} Does `node` have constant nullishness? */ -function hasConstantNullishness(scope, node) { +function hasConstantNullishness(scope, node, nonNullish) { switch (node.type) { case "ObjectExpression": // Objects are never nullish case "ArrayExpression": // Arrays are never nullish @@ -48,11 +49,12 @@ function hasConstantNullishness(scope, node) { case "FunctionExpression": // Functions are never nullish case "ClassExpression": // Classes are never nullish case "NewExpression": // Objects are never nullish - case "Literal": // Nullish, or non-nullish, literals never change case "TemplateLiteral": // A string is never nullish case "UpdateExpression": // Numbers are never nullish case "BinaryExpression": // Numbers, strings, or booleans are never nullish return true; + case "Literal": // Nullish, or non-nullish, literals never change + return nonNullish ? !isNullOrUndefined(scope, node) : true; case "CallExpression": { if (node.callee.type !== "Identifier") { return false; @@ -63,11 +65,11 @@ function hasConstantNullishness(scope, node) { isReferenceToGlobalVariable(scope, node.callee); } case "LogicalExpression": { - return node.operator === "??" && (hasConstantNullishness(scope, node.right) && !isNullOrUndefined(scope, node.right)); + return node.operator === "??" && hasConstantNullishness(scope, node.right, true); } case "AssignmentExpression": if (node.operator === "=") { - return hasConstantNullishness(scope, node.right); + return hasConstantNullishness(scope, node.right, nonNullish); } /* @@ -100,7 +102,7 @@ function hasConstantNullishness(scope, node) { case "SequenceExpression": { const last = node.expressions[node.expressions.length - 1]; - return hasConstantNullishness(scope, last); + return hasConstantNullishness(scope, last, nonNullish); } case "Identifier": return node.name === "undefined" && isReferenceToGlobalVariable(scope, node); @@ -409,14 +411,14 @@ function isAlwaysNew(scope, node) { function findBinaryExpressionConstantOperand(scope, a, b, operator) { if (operator === "==" || operator === "!=") { if ( - (isNullOrUndefined(scope, a) && hasConstantNullishness(scope, b)) || + (isNullOrUndefined(scope, a) && hasConstantNullishness(scope, b, false)) || (isStaticBoolean(scope, a) && hasConstantLooseBooleanComparison(scope, b)) ) { return b; } } else if (operator === "===" || operator === "!==") { if ( - (isNullOrUndefined(scope, a) && hasConstantNullishness(scope, b)) || + (isNullOrUndefined(scope, a) && hasConstantNullishness(scope, b, false)) || (isStaticBoolean(scope, a) && hasConstantStrictBooleanComparison(scope, b)) ) { return b; @@ -455,7 +457,7 @@ module.exports = { if ((operator === "&&" || operator === "||") && isConstant(scope, left, true)) { context.report({ node: left, messageId: "constantShortCircuit", data: { property: "truthiness", operator } }); - } else if (operator === "??" && hasConstantNullishness(scope, left)) { + } else if (operator === "??" && hasConstantNullishness(scope, left, false)) { context.report({ node: left, messageId: "constantShortCircuit", data: { property: "nullishness", operator } }); } }, diff --git a/tests/lib/rules/no-constant-binary-expression.js b/tests/lib/rules/no-constant-binary-expression.js index 5232ef813f2..9fb96f9b432 100644 --- a/tests/lib/rules/no-constant-binary-expression.js +++ b/tests/lib/rules/no-constant-binary-expression.js @@ -61,7 +61,9 @@ ruleTester.run("no-constant-binary-expression", rule, { "[,,,] == true", { code: "new Foo() === bar;", globals: { Foo: "writable" } }, "(foo && true) ?? bar", - "foo ?? null ?? bar" + "foo ?? null ?? bar", + "a ?? (doSomething(), undefined) ?? b", + "a ?? (something = null) ?? b" ], invalid: [ From 0a2fe3e385e9cb85c7a6d08d10240707556af7da Mon Sep 17 00:00:00 2001 From: nissy-dev Date: Sun, 12 Feb 2023 23:08:35 +0900 Subject: [PATCH 6/8] fix: update logic for false negative --- lib/rules/no-constant-binary-expression.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/rules/no-constant-binary-expression.js b/lib/rules/no-constant-binary-expression.js index 0c596d9c895..662e818e7af 100644 --- a/lib/rules/no-constant-binary-expression.js +++ b/lib/rules/no-constant-binary-expression.js @@ -38,23 +38,26 @@ function isNullOrUndefined(scope, node) { * three states at runtime would return `false`. * @param {Scope} scope The scope in which the node was found. * @param {ASTNode} node The AST node being tested. - * @param {boolean} nonNullish node + * @param {boolean} nonNullish if `true` then nullish values are not considered constant. * @returns {boolean} Does `node` have constant nullishness? */ function hasConstantNullishness(scope, node, nonNullish) { + if (nonNullish && isNullOrUndefined(scope, node)) { + return false; + } + switch (node.type) { case "ObjectExpression": // Objects are never nullish case "ArrayExpression": // Arrays are never nullish case "ArrowFunctionExpression": // Functions never nullish case "FunctionExpression": // Functions are never nullish case "ClassExpression": // Classes are never nullish + case "Literal": // Nullish, or non-nullish, literals never change case "NewExpression": // Objects are never nullish case "TemplateLiteral": // A string is never nullish case "UpdateExpression": // Numbers are never nullish case "BinaryExpression": // Numbers, strings, or booleans are never nullish return true; - case "Literal": // Nullish, or non-nullish, literals never change - return nonNullish ? !isNullOrUndefined(scope, node) : true; case "CallExpression": { if (node.callee.type !== "Identifier") { return false; From fbcc0bb4764343da852c465dee20bbe4c9443ce5 Mon Sep 17 00:00:00 2001 From: nissy-dev Date: Sun, 12 Feb 2023 23:11:06 +0900 Subject: [PATCH 7/8] fix; remove unnecessary diff --- lib/rules/no-constant-binary-expression.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rules/no-constant-binary-expression.js b/lib/rules/no-constant-binary-expression.js index 662e818e7af..79432804f73 100644 --- a/lib/rules/no-constant-binary-expression.js +++ b/lib/rules/no-constant-binary-expression.js @@ -52,8 +52,8 @@ function hasConstantNullishness(scope, node, nonNullish) { case "ArrowFunctionExpression": // Functions never nullish case "FunctionExpression": // Functions are never nullish case "ClassExpression": // Classes are never nullish - case "Literal": // Nullish, or non-nullish, literals never change case "NewExpression": // Objects are never nullish + case "Literal": // Nullish, or non-nullish, literals never change case "TemplateLiteral": // A string is never nullish case "UpdateExpression": // Numbers are never nullish case "BinaryExpression": // Numbers, strings, or booleans are never nullish From feb2cad505c29cd53697c044a541e12fb23301b5 Mon Sep 17 00:00:00 2001 From: Daiki Nishikawa Date: Mon, 13 Feb 2023 21:49:43 +0900 Subject: [PATCH 8/8] Update tests/lib/rules/no-constant-binary-expression.js Co-authored-by: Milos Djermanovic --- tests/lib/rules/no-constant-binary-expression.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/lib/rules/no-constant-binary-expression.js b/tests/lib/rules/no-constant-binary-expression.js index 9fb96f9b432..d931e835d73 100644 --- a/tests/lib/rules/no-constant-binary-expression.js +++ b/tests/lib/rules/no-constant-binary-expression.js @@ -316,6 +316,6 @@ ruleTester.run("no-constant-binary-expression", rule, { { code: "window.abc && false && anything", errors: [{ messageId: "constantShortCircuit" }] }, { code: "window.abc || true || anything", errors: [{ messageId: "constantShortCircuit" }] }, - { code: "window.abc ?? 'non-nullish' ?? 'non-nullish'", errors: [{ messageId: "constantShortCircuit" }] } + { code: "window.abc ?? 'non-nullish' ?? anything", errors: [{ messageId: "constantShortCircuit" }] } ] });