From 71f6f0dcd574320ee71c3eb1f313841899bdf260 Mon Sep 17 00:00:00 2001 From: Daiki Nishikawa Date: Tue, 14 Feb 2023 06:26:16 +0900 Subject: [PATCH] feat: report more cases with `??` in no-constant-binary-expression (#16826) * feat: update rule for unnecessary nullish coalescing operator * docs: add incorrect cases * fix: update logic by review * Update docs/src/rules/no-constant-binary-expression.md Co-authored-by: Milos Djermanovic * wip * fix: update logic for false negative * fix; remove unnecessary diff * Update tests/lib/rules/no-constant-binary-expression.js Co-authored-by: Milos Djermanovic --------- Co-authored-by: Milos Djermanovic --- .../rules/no-constant-binary-expression.md | 6 ++ lib/rules/no-constant-binary-expression.js | 55 +++++++++++-------- .../rules/no-constant-binary-expression.js | 12 +++- 3 files changed, 47 insertions(+), 26 deletions(-) diff --git a/docs/src/rules/no-constant-binary-expression.md b/docs/src/rules/no-constant-binary-expression.md index 969586c6f7d..792c61d529d 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; ``` ::: diff --git a/lib/rules/no-constant-binary-expression.js b/lib/rules/no-constant-binary-expression.js index b6ba1752a8c..2cd8928ebfb 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` @@ -21,9 +38,14 @@ const NUMERIC_OR_STRING_BINARY_OPERATORS = new Set(["+", "-", "*", "/", "%", "|" * 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 if `true` then nullish values are not considered constant. * @returns {boolean} Does `node` have constant nullishness? */ -function hasConstantNullishness(scope, node) { +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 @@ -45,9 +67,12 @@ function hasConstantNullishness(scope, node) { return (functionName === "Boolean" || functionName === "String" || functionName === "Number") && isReferenceToGlobalVariable(scope, node.callee); } + case "LogicalExpression": { + return node.operator === "??" && hasConstantNullishness(scope, node.right, true); + } case "AssignmentExpression": if (node.operator === "=") { - return hasConstantNullishness(scope, node.right); + return hasConstantNullishness(scope, node.right, nonNullish); } /* @@ -80,7 +105,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); @@ -378,24 +403,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. @@ -407,14 +414,14 @@ function isNullOrUndefined(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; @@ -453,7 +460,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 c430c7787fd..d931e835d73 100644 --- a/tests/lib/rules/no-constant-binary-expression.js +++ b/tests/lib/rules/no-constant-binary-expression.js @@ -59,7 +59,11 @@ 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", + "a ?? (doSomething(), undefined) ?? b", + "a ?? (something = null) ?? b" ], invalid: [ @@ -308,6 +312,10 @@ 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' ?? anything", errors: [{ messageId: "constantShortCircuit" }] } ] });