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

Update: treat all literals like boolean literal in no-constant-condition #13245

Merged
merged 8 commits into from Oct 23, 2020
Merged

Update: treat all literals like boolean literal in no-constant-condition #13245

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3afc750
Fix: treet all literals like boolean literal in no-constant-condition
Zzzen May 1, 2020
96d0834
fix unexpected short circuit
Zzzen May 2, 2020
540bb20
handle bigin/regex on unsupported runtime
Zzzen Jun 23, 2020
6eb51a8
Merge remote-tracking branch 'eslint/master'
Zzzen Jun 23, 2020
17787ea
fix bigint tests
Zzzen Jun 25, 2020
b0c2e70
simplify literal truthiness
Zzzen Oct 18, 2020
c4c8426
handle regex literal in unsupported environments
Zzzen Oct 18, 2020
ce019be
Update lib/rules/no-constant-condition.js
Zzzen Oct 21, 2020
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
61 changes: 41 additions & 20 deletions lib/rules/no-constant-condition.js
View file
Open in desktop
Expand Up @@ -9,9 +9,6 @@
// Helpers
//------------------------------------------------------------------------------

const EQUALITY_OPERATORS = ["===", "!==", "==", "!="];
const RELATIONAL_OPERATORS = [">", "<", ">=", "<=", "in", "instanceof"];

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------
Expand Down Expand Up @@ -56,6 +53,34 @@ module.exports = {
// Helpers
//--------------------------------------------------------------------------

/**
* checks if a node is truthy
* @param {ASTNode} node any literal node
* @returns {boolean | null} true when node is truthy, null when it cannot be determined.
Zzzen marked this conversation as resolved.
Show resolved Hide resolved
*/
function getBooleanValue(node) {
if (node.value === null) {

/*
* it might be a null literal or bigint/regex literal in unsupported environments .
* 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;
}

return null;
}

return !!node.value;
}

/**
* Checks if a branch node of LogicalExpression short circuits the whole condition
Expand All @@ -66,15 +91,23 @@ module.exports = {
function isLogicalIdentity(node, operator) {
switch (node.type) {
case "Literal":
return (operator === "||" && node.value === true) ||
(operator === "&&" && node.value === false);
return (operator === "||" && getBooleanValue(node) === true) ||
(operator === "&&" && getBooleanValue(node) === false);

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
}
Expand Down Expand Up @@ -129,21 +162,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));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#11308 did not handle following code:

if ((foo || true) === 'baz') {}

demo

Copy link
Member

Choose a reason for hiding this comment

The 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 inBooleanPosition?

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 node.operator === "||" && might be removed in this PR (if #13238 gets accepted), but we should also doublecheck what's happening with the recursion in isLogicalIdentity. It already looks buggy in the actual version:

/*eslint no-constant-condition: "error"*/

if (a && false || b); // error, false positive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The cause is an inappropriate propagation of short circuit.
fixed in 96d0834


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;
}
Expand Down
38 changes: 35 additions & 3 deletions tests/lib/rules/no-constant-condition.js
View file
Open in desktop
Expand Up @@ -74,12 +74,11 @@ ruleTester.run("no-constant-condition", rule, {
"if(true && typeof abc==='string'){}",

// #11181, string literals
"if('str' || a){}",
"if('str1' && a){}",
"if(a && 'str'){}",
"if('str' || abc==='str'){}",

// #11306
"if ((foo || true) === 'baz') {}",
"if ((foo || 'bar') === 'baz') {}",
"if ((foo || 'bar') !== 'baz') {}",
"if ((foo || 'bar') == 'baz') {}",
Expand All @@ -90,6 +89,19 @@ ruleTester.run("no-constant-condition", rule, {
"if ((foo || 233) <= 666) {}",
"if ((key || 'k') in obj) {}",
"if ((foo || {}) instanceof obj) {}",
"if ((foo || 'bar' || 'bar') === 'bar');",
{
code: "if ((foo || 1n) === 'baz') {}",
parserOptions: { ecmaVersion: 11 }
},
{
code: "if (a && 0n || b);",
parserOptions: { ecmaVersion: 11 }
},
aladdin-add marked this conversation as resolved.
Show resolved Hide resolved
{
code: "if(1n && a){};",
parserOptions: { ecmaVersion: 11 }
},

// #12225
"if ('' + [y] === '' + [ty]) {}",
Expand Down Expand Up @@ -121,18 +133,22 @@ ruleTester.run("no-constant-condition", rule, {
{ code: "for(;`foo`;);", errors: [{ messageId: "unexpected", type: "TemplateLiteral" }] },
{ code: "for(;`foo${bar}`;);", errors: [{ messageId: "unexpected", type: "TemplateLiteral" }] },
{ code: "do{}while(true)", errors: [{ messageId: "unexpected", type: "Literal" }] },
{ code: "do{}while('1')", errors: [{ messageId: "unexpected", type: "Literal" }] },
{ code: "do{}while(0)", errors: [{ messageId: "unexpected", type: "Literal" }] },
{ code: "do{}while(t = -2)", errors: [{ messageId: "unexpected", type: "AssignmentExpression" }] },
{ code: "do{}while(``)", errors: [{ messageId: "unexpected", type: "TemplateLiteral" }] },
{ code: "do{}while(`foo`)", errors: [{ messageId: "unexpected", type: "TemplateLiteral" }] },
{ code: "do{}while(`foo${bar}`)", errors: [{ messageId: "unexpected", type: "TemplateLiteral" }] },
{ code: "true ? 1 : 2;", errors: [{ messageId: "unexpected", type: "Literal" }] },
{ code: "1 ? 1 : 2;", errors: [{ messageId: "unexpected", type: "Literal" }] },
{ code: "q = 0 ? 1 : 2;", errors: [{ messageId: "unexpected", type: "Literal" }] },
{ code: "(q = 0) ? 1 : 2;", errors: [{ messageId: "unexpected", type: "AssignmentExpression" }] },
{ code: "`` ? 1 : 2;", errors: [{ messageId: "unexpected", type: "TemplateLiteral" }] },
{ code: "`foo` ? 1 : 2;", errors: [{ messageId: "unexpected", type: "TemplateLiteral" }] },
{ code: "`foo${bar}` ? 1 : 2;", errors: [{ messageId: "unexpected", type: "TemplateLiteral" }] },
{ code: "if(-2);", errors: [{ messageId: "unexpected", type: "UnaryExpression" }] },
{ code: "if(true);", errors: [{ messageId: "unexpected", type: "Literal" }] },
{ code: "if(1);", errors: [{ messageId: "unexpected", type: "Literal" }] },
{ code: "if({});", errors: [{ messageId: "unexpected", type: "ObjectExpression" }] },
{ code: "if(0 < 1);", errors: [{ messageId: "unexpected", type: "BinaryExpression" }] },
{ code: "if(0 || 1);", errors: [{ messageId: "unexpected", type: "LogicalExpression" }] },
Expand All @@ -143,6 +159,7 @@ ruleTester.run("no-constant-condition", rule, {
{ code: "if(`${'bar'}`);", errors: [{ messageId: "unexpected", type: "TemplateLiteral" }] },
{ code: "if(`${'bar' + `foo`}`);", errors: [{ messageId: "unexpected", type: "TemplateLiteral" }] },
{ code: "if(`foo${false || true}`);", errors: [{ messageId: "unexpected", type: "TemplateLiteral" }] },
{ code: "if(`foo${0 || 1}`);", errors: [{ messageId: "unexpected", type: "TemplateLiteral" }] },
{ code: "if(`foo${bar}`);", errors: [{ messageId: "unexpected", type: "TemplateLiteral" }] },
{ code: "if(`${bar}foo`);", errors: [{ messageId: "unexpected", type: "TemplateLiteral" }] },

Expand All @@ -152,6 +169,7 @@ ruleTester.run("no-constant-condition", rule, {
{ code: "while(x = 1);", errors: [{ messageId: "unexpected", type: "AssignmentExpression" }] },
{ code: "while(function(){});", errors: [{ messageId: "unexpected", type: "FunctionExpression" }] },
{ code: "while(true);", errors: [{ messageId: "unexpected", type: "Literal" }] },
{ code: "while(1);", errors: [{ messageId: "unexpected", type: "Literal" }] },
{ code: "while(() => {});", errors: [{ messageId: "unexpected", type: "ArrowFunctionExpression" }] },
{ code: "while(`foo`);", errors: [{ messageId: "unexpected", type: "TemplateLiteral" }] },
{ code: "while(``);", errors: [{ messageId: "unexpected", type: "TemplateLiteral" }] },
Expand Down Expand Up @@ -180,12 +198,15 @@ ruleTester.run("no-constant-condition", rule, {
// #5693
{ code: "if(false && abc==='str'){}", errors: [{ messageId: "unexpected", type: "LogicalExpression" }] },
{ code: "if(true || abc==='str'){}", errors: [{ messageId: "unexpected", type: "LogicalExpression" }] },
{ code: "if(1 || abc==='str'){}", errors: [{ messageId: "unexpected", type: "LogicalExpression" }] },
{ code: "if(abc==='str' || true){}", errors: [{ messageId: "unexpected", type: "LogicalExpression" }] },
{ code: "if(abc==='str' || true || def ==='str'){}", errors: [{ messageId: "unexpected", type: "LogicalExpression" }] },
{ code: "if(false || true){}", errors: [{ messageId: "unexpected", type: "LogicalExpression" }] },
{ code: "if(typeof abc==='str' || true){}", errors: [{ messageId: "unexpected", type: "LogicalExpression" }] },

// #11181, string literals
{ code: "if('str' || a){}", errors: [{ messageId: "unexpected", type: "LogicalExpression" }] },
{ code: "if('str' || abc==='str'){}", errors: [{ messageId: "unexpected", type: "LogicalExpression" }] },
{ code: "if('str1' || 'str2'){}", errors: [{ messageId: "unexpected", type: "LogicalExpression" }] },
{ code: "if('str1' && 'str2'){}", errors: [{ messageId: "unexpected", type: "LogicalExpression" }] },
{ code: "if(abc==='str' || 'str'){}", errors: [{ messageId: "unexpected", type: "LogicalExpression" }] },
Expand Down Expand Up @@ -272,6 +293,17 @@ ruleTester.run("no-constant-condition", rule, {
{
code: "if ([,] + ''){}",
errors: [{ messageId: "unexpected", type: "BinaryExpression" }]
}
},

// #13238
{ code: "if(/foo/ui);", parserOptions: { ecmaVersion: 11 }, errors: [{ messageId: "unexpected", type: "Literal" }] },
{ code: "if(0n);", parserOptions: { ecmaVersion: 11 }, errors: [{ messageId: "unexpected", type: "Literal" }] },
{ code: "if(0b0n);", parserOptions: { ecmaVersion: 11 }, errors: [{ messageId: "unexpected", type: "Literal" }] },
{ code: "if(0o0n);", parserOptions: { ecmaVersion: 11 }, errors: [{ messageId: "unexpected", type: "Literal" }] },
{ code: "if(0x0n);", parserOptions: { ecmaVersion: 11 }, errors: [{ messageId: "unexpected", type: "Literal" }] },
{ code: "if(0b1n);", parserOptions: { ecmaVersion: 11 }, errors: [{ messageId: "unexpected", type: "Literal" }] },
{ code: "if(0o1n);", parserOptions: { ecmaVersion: 11 }, errors: [{ messageId: "unexpected", type: "Literal" }] },
{ code: "if(0x1n);", parserOptions: { ecmaVersion: 11 }, errors: [{ messageId: "unexpected", type: "Literal" }] },
{ code: "if(0x1n || foo);", parserOptions: { ecmaVersion: 11 }, errors: [{ messageId: "unexpected", type: "LogicalExpression" }] }
]
});