Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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 <milos.djermanovic@gmail.com>

* 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 <milos.djermanovic@gmail.com>

---------

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
  • Loading branch information
nissy-dev and mdjermanovic committed Feb 13, 2023
1 parent ed2999b commit 71f6f0d
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 26 deletions.
6 changes: 6 additions & 0 deletions docs/src/rules/no-constant-binary-expression.md
Expand Up @@ -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;
```
:::
Expand Down
55 changes: 31 additions & 24 deletions lib/rules/no-constant-binary-expression.js
Expand Up @@ -14,16 +14,38 @@ 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`
* or not `null` _or_ `undefined`. An expression that can vary between those
* 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
Expand All @@ -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);
}

/*
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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.
Expand All @@ -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;
Expand Down Expand Up @@ -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 } });
}
},
Expand Down
12 changes: 10 additions & 2 deletions tests/lib/rules/no-constant-binary-expression.js
Expand Up @@ -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: [

Expand Down Expand Up @@ -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" }] }
]
});

0 comments on commit 71f6f0d

Please sign in to comment.