-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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.
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
Update: treat all literals like boolean literal in no-constant-condition #13245
Changes from 5 commits
3afc750
96d0834
540bb20
6eb51a8
17787ea
b0c2e70
c4c8426
ce019be
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,9 +9,6 @@ | |
// Helpers | ||
//------------------------------------------------------------------------------ | ||
|
||
const EQUALITY_OPERATORS = ["===", "!==", "==", "!="]; | ||
const RELATIONAL_OPERATORS = [">", "<", ">=", "<=", "in", "instanceof"]; | ||
|
||
//------------------------------------------------------------------------------ | ||
// Rule Definition | ||
//------------------------------------------------------------------------------ | ||
|
@@ -56,6 +53,39 @@ module.exports = { | |
// Helpers | ||
//-------------------------------------------------------------------------- | ||
|
||
/** | ||
* checks if a node is truthy | ||
* @param {ASTNode} node any literal node | ||
* @returns {boolean} true when node is truthy | ||
*/ | ||
function isLiteralTruthy(node) { | ||
if (node.value === null) { | ||
|
||
/* | ||
* it might be null literal or a bigint/regex literal but not supported on current environment. | ||
* https://github.com/estree/estree/blob/14df8a024956ea289bd55b9c2226a1d5b8a473ee/es5.md#regexpliteral | ||
* https://github.com/estree/estree/blob/14df8a024956ea289bd55b9c2226a1d5b8a473ee/es2020.md#bigintliteral | ||
*/ | ||
|
||
if (node.raw === "null") { | ||
return false; | ||
} | ||
|
||
// regex is always truthy | ||
if (typeof node.regex === "object") { | ||
return true; | ||
} | ||
|
||
// https://tc39.es/ecma262/#sec-literals-numeric-literals | ||
if (typeof node.bigint === "string") { | ||
return !(/^0+$/u.test(node.bigint) || /^0[box]0+$/ui.test(node.bigint)); | ||
} | ||
|
||
throw new Error(`unsupported literal: ${node.raw}`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We aim to not intentionally crash on unknown syntax. Can we return Since it will be a 3-state, maybe we could also rename this function to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
} | ||
|
||
return !!node.value; | ||
} | ||
|
||
/** | ||
* Checks if a branch node of LogicalExpression short circuits the whole condition | ||
|
@@ -66,15 +96,23 @@ module.exports = { | |
function isLogicalIdentity(node, operator) { | ||
switch (node.type) { | ||
case "Literal": | ||
return (operator === "||" && node.value === true) || | ||
(operator === "&&" && node.value === false); | ||
return (operator === "||" && isLiteralTruthy(node)) || | ||
(operator === "&&" && !isLiteralTruthy(node)); | ||
|
||
case "UnaryExpression": | ||
return (operator === "&&" && node.operator === "void"); | ||
|
||
case "LogicalExpression": | ||
return isLogicalIdentity(node.left, node.operator) || | ||
isLogicalIdentity(node.right, node.operator); | ||
|
||
/* | ||
* handles `a && false || b` | ||
* `false` is an identity element of `&&` but not `||` | ||
*/ | ||
return operator === node.operator && | ||
( | ||
isLogicalIdentity(node.left, node.operator) || | ||
isLogicalIdentity(node.right, node.operator) | ||
); | ||
|
||
// no default | ||
} | ||
|
@@ -129,21 +167,9 @@ module.exports = { | |
const isLeftConstant = isConstant(node.left, inBooleanPosition); | ||
const isRightConstant = isConstant(node.right, inBooleanPosition); | ||
const isLeftShortCircuit = (isLeftConstant && isLogicalIdentity(node.left, node.operator)); | ||
const isRightShortCircuit = (isRightConstant && isLogicalIdentity(node.right, node.operator)); | ||
const isRightShortCircuit = (inBooleanPosition && isRightConstant && isLogicalIdentity(node.right, node.operator)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is indeed a bug that should be fixed whether or not #13238 gets accepted. Could we maybe also this: (
!node.parent ||
node.parent.type !== "BinaryExpression" ||
!(EQUALITY_OPERATORS.includes(node.parent.operator) || RELATIONAL_OPERATORS.includes(node.parent.operator))
) replace with just I think it might fix similar bugs, such as: /*eslint no-constant-condition: "error"*/
if ((foo || 'bar' || 'bar') === 'bar'); // error, false positive Though, it seems that the whole condition starting with /*eslint no-constant-condition: "error"*/
if (a && false || b); // error, false positive There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The cause is an inappropriate propagation of short circuit. |
||
|
||
return (isLeftConstant && isRightConstant) || | ||
( | ||
|
||
// in the case of an "OR", we need to know if the right constant value is truthy | ||
node.operator === "||" && | ||
isRightConstant && | ||
node.right.value && | ||
( | ||
!node.parent || | ||
node.parent.type !== "BinaryExpression" || | ||
!(EQUALITY_OPERATORS.includes(node.parent.operator) || RELATIONAL_OPERATORS.includes(node.parent.operator)) | ||
) | ||
) || | ||
isLeftShortCircuit || | ||
isRightShortCircuit; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is well-done, but I'd prefer to remove this branch. All Node versions we support now will have the bigint value in
node.value
, and we're already using that fact in some rules, likeyoda
.In a worst-case scenario (e.g., an old browser, though we don't officially support use of ESLint in browsers) bigint literals will be just ignored, which doesn't look that bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌 That would simplify implementation a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the very quick response!
Sorry if my post was confusing, I meant to remove only the bigint part.
This was ok:
Regex syntax may have more changes in the future, so we will always use
node.regex
in rules.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦♂️ fixed.