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 2 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
33 changes: 13 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 @@ -66,15 +63,23 @@ module.exports = {
function isLogicalIdentity(node, operator) {
switch (node.type) {
case "Literal":
return (operator === "||" && node.value === true) ||
(operator === "&&" && node.value === false);
return (operator === "||" && !!node.value) ||
(operator === "&&" && !node.value);
Copy link
Member

Choose a reason for hiding this comment

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

ESTree doesn't guarantee node.value is a valid value. For example, if you run ESLint on Node.js that doesn't support BigInt, node.value is null at bigint literals. It will cause false positive/negative. Similarly, RegExp literals that contain new syntax, bigdecimal literals (stage 1) are, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about reporting an unsupported error like the following code?

class UnsupportedNodeError extends Error {}

function isNodeTruthy(node) {
    if (node.value === null && node.raw !== 'null') {
        throw new UnsupportedNodeError(`unsupported node: ${node.type}`)
    }

    return !!node.value;
}

function isLogicalIdentity(node, operator) {
    switch (node.type) {
        case "Literal":
            return (operator === "||" && isNodeTruthy(node)) ||
                (operator === "&&" && !isNodeTruthy(node));
        // ...
    }
}

function reportIfConstant(node) {
    try {
        if (node.test && isConstant(node.test, true)) {
            context.report({ node: node.test, messageId: "unexpected" });
        }
    } catch (error) {
        if (error instanceof UnsupportedNodeError) {
            context.report({ node: node.test, message: error.message })
        } else {
            throw error
        }
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

Even if the runtime doesn't support a syntax, ESLint should check the syntax successfully if ESLint supports it. In other words, the result of linting should be the same on all supported runtimes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In other words, the result of linting should be the same on all supported runtimes.

Can we achieve runtime consistency by not supporting bigint on advanced runtime? Though bigint will not behave the same as numbers, just like numbers not behaving the same as booleans currently.

function isNodeTruthy(node) {
    // TODO: bigint is not supported. wait till node v10.4.0+ is popular.
    if (typeof node.value === 'bigint' || (node.value === null && node.raw !== 'null')) {
        return false;
    }

    return !!node.value;
}

Copy link
Member

Choose a reason for hiding this comment

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

As per ESTree spec, the Literal nodes of BigInt have bigint property with the text representation of the bigint value. And the Literal nodes of RegExp have regex property with { pattern: string; flags: string }. The value property of Literal nodes of those may be null, so we have to check the bigint/regex properties to distinguish null literal and the others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, eslint is using espree 6.2.1, which does not support bigint or regex with u flag. 🤦

Copy link
Member

Choose a reason for hiding this comment

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

The actual version is using espree 7.1.0

eslint/package.json

Line 59 in b7d79b1

"espree": "^7.1.0",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was using an old version. Anyway, fallback handling is added. Your suggestions would be helpful.

https://github.com/eslint/eslint/pull/13245/files#diff-aee426385c16cd9eadb5fabd23dfa0bfR64-R84

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 code below is working on node 10.0.0.

if(a);
if(/foo/ui);
if(0n);
if(1n && a);
if(1n || a);
  2:4  error  Unexpected constant condition  no-constant-condition
  3:4  error  Unexpected constant condition  no-constant-condition
  5:4  error  Unexpected constant condition  no-constant-condition


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 +134,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
14 changes: 12 additions & 2 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,8 @@ ruleTester.run("no-constant-condition", rule, {
"if ((foo || 233) <= 666) {}",
"if ((key || 'k') in obj) {}",
"if ((foo || {}) instanceof obj) {}",
"if ((foo || 'bar' || 'bar') === 'bar');",
"if (a && false || b);",

// #12225
"if ('' + [y] === '' + [ty]) {}",
Expand Down Expand Up @@ -121,18 +122,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 +148,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 +158,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 +187,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